Re: [PATCH 07/16] media: vimc: cap: Add handler for singleplanar fmt ioctls

[Helen Koike <helen.koike@collabora.com>]

On 3/15/19 1:43 PM, André Almeida wrote:
> Since multiplanar is a superset of single planar formats, instead of
> having different implementations for them, treat all formats as
> multiplanar. If we need to work with single planar formats, convert them to
> multiplanar (with num_planes = 1), re-use the multiplanar code, and
> transform them back to single planar. This is implemented with
> v4l2_fmt_sp2mp_func().
> 
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
>  drivers/media/platform/vimc/vimc-capture.c | 63 +++++++++++++++++-----
>  1 file changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index a93241d53953..47acf50f1ad2 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -127,7 +127,7 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
>  static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  				    struct v4l2_format *f)

I think you can add the _mp suffix to vimc_cap_try_fmt_vid_cap, so it
makes it explicit that it deals with mp formats.

But as you also adds a vimc_cap_try_fmt_vid_cap_mp in the next commit,
you can use a _vimc_cap_try_fmt_vid_cap_mp to have both functions.

Example:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-osd.c#n129
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-osd.c#n169

Regards,
Helen

>  {
> -	struct v4l2_pix_format *format = &f->fmt.pix;
> +	struct v4l2_pix_format_mplane *format = &f->fmt.pix_mp;
>  
>  	format->width = clamp_t(u32, format->width, VIMC_FRAME_MIN_WIDTH,
>  				VIMC_FRAME_MAX_WIDTH) & ~1;
> @@ -145,20 +145,58 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
>  	if (!v4l2_format_info(format->pixelformat))
>  		format->pixelformat = fmt_default.fmt.pix.pixelformat;
>  
> -	return v4l2_fill_pixfmt(format, format->pixelformat,
> +	return v4l2_fill_pixfmt_mp(format, format->pixelformat,
>  				format->width, format->height);
>  }
>  
> -static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
> +static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
> +				     struct v4l2_fmtdesc *f)
> +{
> +	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
> +		return -EINVAL;
> +
> +	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> +	strncpy(f->description, v4l2_get_fourcc_name(f->pixelformat), 4);
> +	f->description[4] = '\0';
> +
> +	return 0;
> +}
> +
> +/*
> + * VIDIOC FMT handlers for single-planar
> + */
> +
> +static int vimc_cap_g_fmt_vid_cap_sp(struct file *file, void *priv,
> +				  struct v4l2_format *f)
> +{
> +	if (multiplanar)
> +		return -EINVAL;
> +
> +	return vimc_cap_g_fmt_vid_cap(file, priv, f);
> +}
> +
> +static int vimc_cap_try_fmt_vid_cap_sp(struct file *file, void *priv,
> +				  struct v4l2_format *f)
> +{
> +	if (multiplanar)
> +		return -EINVAL;
> +
> +	return v4l2_fmt_sp2mp_func(file, priv, f, vimc_cap_try_fmt_vid_cap);
> +}
> +
> +static int vimc_cap_s_fmt_vid_cap_sp(struct file *file, void *priv,
>  				  struct v4l2_format *f)
>  {
>  	struct vimc_cap_device *vcap = video_drvdata(file);
>  
> +	if (multiplanar)
> +		return -EINVAL;
> +
>  	/* Do not change the format while stream is on */
>  	if (vb2_is_busy(&vcap->queue))
>  		return -EBUSY;
>  
> -	vimc_cap_try_fmt_vid_cap(file, priv, f);
> +	v4l2_fmt_sp2mp_func(file, priv, f, vimc_cap_try_fmt_vid_cap);
>  
>  	dev_dbg(vcap->dev, "%s: format update: "
>  		"old:%dx%d (0x%x, %d, %d, %d, %d) "
> @@ -181,15 +219,13 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
> +static int vimc_cap_enum_fmt_vid_cap_sp(struct file *file, void *priv,
>  				     struct v4l2_fmtdesc *f)
>  {
> -	if (f->index >= ARRAY_SIZE(vimc_cap_supported_pixfmt))
> +	if (multiplanar)
>  		return -EINVAL;
>  
> -	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> -
> -	return 0;
> +	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
>  }
>  
>  static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
> @@ -235,10 +271,11 @@ static const struct v4l2_file_operations vimc_cap_fops = {
>  static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
>  	.vidioc_querycap = vimc_cap_querycap,
>  
> -	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap,
> -	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap,
> -	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap,
> -	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap = vimc_cap_g_fmt_vid_cap_sp,
> +	.vidioc_s_fmt_vid_cap = vimc_cap_s_fmt_vid_cap_sp,
> +	.vidioc_try_fmt_vid_cap = vimc_cap_try_fmt_vid_cap_sp,
> +	.vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap_sp,
> +
>  	.vidioc_enum_framesizes = vimc_cap_enum_framesizes,
>  
>  	.vidioc_reqbufs = vb2_ioctl_reqbufs,
>