linux-kernel - Re: [PATCH] media: vimc: get pixformat info from v4l2_format

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a5b1c17-639e-7c40-e439-35acb3042e9e@xs4all.nl>
Date:   Fri, 13 Dec 2019 11:09:48 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     "Carlos E. C. Barbosa" <climacobarbosacee@...il.com>,
        Helen Koike <helen.koike@...labora.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        lkcamp@...ts.libreplanetbr.org
Cc:     "Carlos E.C. Barbosa" <carlosecb@...anota.com>
Subject: Re: [PATCH] media: vimc: get pixformat info from v4l2_format_info to
 avoid code repetition

On 10/16/19 12:52 AM, Carlos E. C. Barbosa wrote:
> From: "Carlos E.C. Barbosa" <carlosecb@...anota.com>
> 
> There is overlapping code over two distinct lists. This repurposes one
> of vimc_pix_map for strictly mapping formats and remaps other calls to
> the matching v4l2_format_info.
> 
> ---
> 
> Change in v2:
> - Change commit message
> - Remove struct with mbus code and pointer to v4l2_format_info
> - The map and info are directly called when strictly needed
> 
> Signed-off-by: Carlos E. C. Barbosa <carlosecb@...anota.com>
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 14 ++--
>  drivers/media/platform/vimc/vimc-common.c  | 78 ++++++----------------
>  drivers/media/platform/vimc/vimc-common.h  |  9 +--
>  drivers/media/platform/vimc/vimc-debayer.c |  9 ++-
>  drivers/media/platform/vimc/vimc-scaler.c  | 39 +++++++++--
>  drivers/media/platform/vimc/vimc-sensor.c  | 26 +++++---
>  6 files changed, 89 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 602f80323031..65282d4d8a1e 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -85,7 +85,8 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  				    struct v4l2_format *f)
>  {
>  	struct v4l2_pix_format *format = &f->fmt.pix;
> -	const struct vimc_pix_map *vpix;
> +	struct vimc_pix_map *vpix;
> +	struct v4l2_format_info *vinfo;

No const here,

>  
>  	format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH,
>  				VIMC_FRAME_MAX_WIDTH) & ~1;
> @@ -99,7 +100,8 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  		vpix = vimc_pix_map_by_pixelformat(format->pixelformat);
>  	}
>  	/* TODO: Add support for custom bytesperline values */
> -	format->bytesperline = format->width * vpix->bpp;
> +	vinfo = v4l2_format_info(vpix->pixelformat);

but v4l2_format_info returns a const pointer. Does this patch even
compile without compiler warnings?

> +	format->bytesperline = format->width * vinfo->bpp[0];
>  	format->sizeimage = format->bytesperline * format->height;
>  
>  	if (format->field == V4L2_FIELD_ANY)
> @@ -159,7 +161,7 @@ static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
>  static int vimc_cap_enum_framesizes(struct file *file, void *fh,
>  				    struct v4l2_frmsizeenum *fsize)
>  {
> -	const struct vimc_pix_map *vpix;
> +	struct vimc_pix_map *vpix;
>  
>  	if (fsize->index)
>  		return -EINVAL;
> @@ -387,7 +389,8 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>  				     const char *vcfg_name)
>  {
>  	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
> -	const struct vimc_pix_map *vpix;
> +	struct vimc_pix_map *vpix;
> +	struct v4l2_format_info *vinfo;
>  	struct vimc_cap_device *vcap;
>  	struct video_device *vdev;
>  	struct vb2_queue *q;
> @@ -443,7 +446,8 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>  	/* Set default frame format */
>  	vcap->format = fmt_default;
>  	vpix = vimc_pix_map_by_pixelformat(vcap->format.pixelformat);
> -	vcap->format.bytesperline = vcap->format.width * vpix->bpp;
> +	vinfo = v4l2_format_info(vpix->pixelformat);
> +	vcap->format.bytesperline = vcap->format.width * vinfo->bpp[0];
>  	vcap->format.sizeimage = vcap->format.bytesperline *
>  				 vcap->format.height;
>  
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index a3120f4f7a90..73feea089921 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -14,185 +14,151 @@
>   * NOTE: non-bayer formats need to come first (necessary for enum_mbus_code
>   * in the scaler)
>   */
> -static const struct vimc_pix_map vimc_pix_map_list[] = {
> +static struct vimc_pix_map vimc_pix_map_list[] = {

Why was const removed here?

This patch also needs to be rebased, so I stop reviewing here and
wait for a v2.

Regards,

	Hans