Re: [PATCH v2 05/15] media: vimc: cap: Add handler for singleplanar fmt ioctls

[Helen Koike]

Hi Andre,

On 3/27/19 12:17 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().

I would rename the commit title as you are not really adding handlers,
the handlers are already there, you are refactoring to make it easier to
add multiplanar handlers in the next commit.

Maybe:
media: vimc: cap: refactor singleplanar as a subset of multiplanar

> 
> Signed-off-by: Andrà Almeida <andrealmeid@xxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Make more clear that the generic function _vimc_cap_try_fmt_vid_cap_mp
> expects a multiplanar format as input 
> 
>  drivers/media/platform/vimc/vimc-capture.c | 75 +++++++++++++++++-----
>  1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index add6df565fa9..24d70c2c9db7 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -128,10 +128,10 @@ static int vimc_cap_g_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv,
> +static int _vimc_cap_try_fmt_vid_cap_mp(struct file *file, void *priv,
>  				    struct v4l2_format *f)

Please align the argument.

>  {
> -	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;
> @@ -149,20 +149,60 @@ 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,
> -				format->width, format->height);
> +	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,
> -				  struct v4l2_format *f)
> +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];
> +
> +	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)
> +{
> +	struct vimc_cap_device *vcap = video_drvdata(file);
> +
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;

I think you can get rid of these checks, you just need to make sure only
sp callbacks are registered in vimc_cap_fops if you are in sp mode; And
only mp callbacks are registered in vimc_cap_fops if you are in mp mode
(as we don't support both at the same time atm.

You could check the planar mode in the bind function, then you can alter
fops before calling video_register_device(), .

This would clean your code a bit, and you won't need these two functions
in your next patch:
_vimc_cap_try_fmt_vid_cap_mp
vimc_cap_try_fmt_vid_cap_mp
as the first just performs this check and then calls the second, you can
eliminate the check and remove _vimc_cap_try_fmt_vid_cap_mp.

Helen


> +
> +	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)
>  {
>  	struct vimc_cap_device *vcap = video_drvdata(file);
>  
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;
> +
> +	return v4l2_fmt_sp2mp_func(file, priv, f, _vimc_cap_try_fmt_vid_cap_mp);
> +}
> +
> +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 (IS_MULTIPLANAR(vcap))
> +		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_mp);
>  
>  	dev_dbg(vcap->dev, "%s: format update: "
>  		"old:%dx%d (0x%x, %d, %d, %d, %d) "
> @@ -185,15 +225,15 @@ 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,
> -				     struct v4l2_fmtdesc *f)
> +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))
> -		return -EINVAL;
> +	struct vimc_cap_device *vcap = video_drvdata(file);
>  
> -	f->pixelformat = vimc_cap_supported_pixfmt[f->index];
> +	if (IS_MULTIPLANAR(vcap))
> +		return -EINVAL;
>  
> -	return 0;
> +	return vimc_cap_enum_fmt_vid_cap(file, priv, f);
>  }
>  
>  static bool vimc_cap_is_pixfmt_supported(u32 pixelformat)
> @@ -239,10 +279,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,
>