Re: [PATCH 20/75] media: imx: capture: Rename ioctl operations with legacy prefix

[Steve Longerbeam <slongerbeam@gmail.com>]

Hi Laurent,

I guess I have fallen behind the times with v4l2, but I wasn't aware 
that the /dev/video nodes and VIDIOC_* APIs are now considered legacy!

Steve

On 1/5/21 7:27 AM, Laurent Pinchart wrote:
> The i.MX media drivers implement a legacy video node API, where the
> format of the video node is influenced by the active format of the
> connected subdev (both for enumeration and for the get, set and try
> format ioctls), and where controls exposed by the subdevs in the
> pipeline are inherited by the video node.
>
> At the same time, the drivers implement the media controller API and
> expose subdev video nodes to userspace. Those two modes of operation are
> incompatible and should not be exposed together. Furthermore, the legacy
> API gets in the way of proper enumeration of pixel formats on the video
> node, as it prevents compliance with the V4L2 specification.
>
> As a first step towards fixing this, rename all V4L2 video node ioctl
> handlers with a legacy prefix. This will allow implementing a new set of
> ioctls in parallel and gradually switching drivers. Add a task to the
> TODO file for the removal of the legacy API.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/staging/media/imx/TODO                |   9 +-
>   drivers/staging/media/imx/imx-media-capture.c | 210 +++++++++---------
>   2 files changed, 116 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> index 9cfc1c1e78dc..2384f4c6b09d 100644
> --- a/drivers/staging/media/imx/TODO
> +++ b/drivers/staging/media/imx/TODO
> @@ -17,9 +17,12 @@
>   - This media driver supports inheriting V4L2 controls to the
>     video capture devices, from the subdevices in the capture device's
>     pipeline. The controls for each capture device are updated in the
> -  link_notify callback when the pipeline is modified. It should be
> -  decided whether this feature is useful enough to make it generally
> -  available by exporting to v4l2-core.
> +  link_notify callback when the pipeline is modified. This feature should be
> +  removed, userspace should use the subdev-based userspace API instead.
> +
> +- Similarly to the legacy control handling, legacy format handling where
> +  formats on the video nodes are influenced by the active format of the
> +  connected subdev should be removed.
>   
>   - i.MX7: all of the above, since it uses the imx media core
>   
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index db1f551b86ea..0775e60ad894 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -52,8 +52,8 @@ struct capture_priv {
>   /* In bytes, per queue */
>   #define VID_MEM_LIMIT	SZ_64M
>   
> -/*
> - * Video ioctls follow
> +/* -----------------------------------------------------------------------------
> + * Common Video IOCTLs
>    */
>   
>   static int capture_querycap(struct file *file, void *fh,
> @@ -69,8 +69,52 @@ static int capture_querycap(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_enum_framesizes(struct file *file, void *fh,
> -				   struct v4l2_frmsizeenum *fsize)
> +static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> +				 struct v4l2_format *f)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +
> +	f->fmt.pix = priv->vdev.fmt;
> +
> +	return 0;
> +}
> +
> +static int capture_g_selection(struct file *file, void *fh,
> +			       struct v4l2_selection *s)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +
> +	switch (s->target) {
> +	case V4L2_SEL_TGT_COMPOSE:
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +		/* The compose rectangle is fixed to the source format. */
> +		s->r = priv->vdev.compose;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> +		/*
> +		 * The hardware writes with a configurable but fixed DMA burst
> +		 * size. If the source format width is not burst size aligned,
> +		 * the written frame contains padding to the right.
> +		 */
> +		s->r.left = 0;
> +		s->r.top = 0;
> +		s->r.width = priv->vdev.fmt.width;
> +		s->r.height = priv->vdev.fmt.height;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Legacy Video IOCTLs
> + */
> +
> +static int capture_legacy_enum_framesizes(struct file *file, void *fh,
> +					  struct v4l2_frmsizeenum *fsize)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	const struct imx_media_pixfmt *cc;
> @@ -109,8 +153,8 @@ static int capture_enum_framesizes(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_enum_frameintervals(struct file *file, void *fh,
> -				       struct v4l2_frmivalenum *fival)
> +static int capture_legacy_enum_frameintervals(struct file *file, void *fh,
> +					      struct v4l2_frmivalenum *fival)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	const struct imx_media_pixfmt *cc;
> @@ -140,8 +184,8 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
> -				    struct v4l2_fmtdesc *f)
> +static int capture_legacy_enum_fmt_vid_cap(struct file *file, void *fh,
> +					   struct v4l2_fmtdesc *f)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	const struct imx_media_pixfmt *cc_src;
> @@ -184,21 +228,11 @@ static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_g_fmt_vid_cap(struct file *file, void *fh,
> -				 struct v4l2_format *f)
> -{
> -	struct capture_priv *priv = video_drvdata(file);
> -
> -	f->fmt.pix = priv->vdev.fmt;
> -
> -	return 0;
> -}
> -
> -static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
> -				     struct v4l2_subdev_format *fmt_src,
> -				     struct v4l2_format *f,
> -				     const struct imx_media_pixfmt **retcc,
> -				     struct v4l2_rect *compose)
> +static int __capture_legacy_try_fmt(struct capture_priv *priv,
> +				    struct v4l2_subdev_format *fmt_src,
> +				    struct v4l2_format *f,
> +				    const struct imx_media_pixfmt **retcc,
> +				    struct v4l2_rect *compose)
>   {
>   	const struct imx_media_pixfmt *cc, *cc_src;
>   
> @@ -255,8 +289,8 @@ static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
>   	return 0;
>   }
>   
> -static int capture_try_fmt_vid_cap(struct file *file, void *fh,
> -				   struct v4l2_format *f)
> +static int capture_legacy_try_fmt_vid_cap(struct file *file, void *fh,
> +					  struct v4l2_format *f)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	struct v4l2_subdev_format fmt_src;
> @@ -268,11 +302,11 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>   	if (ret)
>   		return ret;
>   
> -	return __capture_try_fmt_vid_cap(priv, &fmt_src, f, NULL, NULL);
> +	return __capture_legacy_try_fmt(priv, &fmt_src, f, NULL, NULL);
>   }
>   
> -static int capture_s_fmt_vid_cap(struct file *file, void *fh,
> -				 struct v4l2_format *f)
> +static int capture_legacy_s_fmt_vid_cap(struct file *file, void *fh,
> +					struct v4l2_format *f)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	struct v4l2_subdev_format fmt_src;
> @@ -289,8 +323,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   	if (ret)
>   		return ret;
>   
> -	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f, &priv->vdev.cc,
> -					&priv->vdev.compose);
> +	ret = __capture_legacy_try_fmt(priv, &fmt_src, f, &priv->vdev.cc,
> +				       &priv->vdev.compose);
>   	if (ret)
>   		return ret;
>   
> @@ -299,21 +333,22 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_querystd(struct file *file, void *fh, v4l2_std_id *std)
> +static int capture_legacy_querystd(struct file *file, void *fh,
> +				   v4l2_std_id *std)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   
>   	return v4l2_subdev_call(priv->src_sd, video, querystd, std);
>   }
>   
> -static int capture_g_std(struct file *file, void *fh, v4l2_std_id *std)
> +static int capture_legacy_g_std(struct file *file, void *fh, v4l2_std_id *std)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   
>   	return v4l2_subdev_call(priv->src_sd, video, g_std, std);
>   }
>   
> -static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
> +static int capture_legacy_s_std(struct file *file, void *fh, v4l2_std_id std)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   
> @@ -323,38 +358,8 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
>   	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
>   }
>   
> -static int capture_g_selection(struct file *file, void *fh,
> -			       struct v4l2_selection *s)
> -{
> -	struct capture_priv *priv = video_drvdata(file);
> -
> -	switch (s->target) {
> -	case V4L2_SEL_TGT_COMPOSE:
> -	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> -	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> -		/* The compose rectangle is fixed to the source format. */
> -		s->r = priv->vdev.compose;
> -		break;
> -	case V4L2_SEL_TGT_COMPOSE_PADDED:
> -		/*
> -		 * The hardware writes with a configurable but fixed DMA burst
> -		 * size. If the source format width is not burst size aligned,
> -		 * the written frame contains padding to the right.
> -		 */
> -		s->r.left = 0;
> -		s->r.top = 0;
> -		s->r.width = priv->vdev.fmt.width;
> -		s->r.height = priv->vdev.fmt.height;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -static int capture_g_parm(struct file *file, void *fh,
> -			  struct v4l2_streamparm *a)
> +static int capture_legacy_g_parm(struct file *file, void *fh,
> +				 struct v4l2_streamparm *a)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	struct v4l2_subdev_frame_interval fi;
> @@ -375,8 +380,8 @@ static int capture_g_parm(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_s_parm(struct file *file, void *fh,
> -			  struct v4l2_streamparm *a)
> +static int capture_legacy_s_parm(struct file *file, void *fh,
> +				 struct v4l2_streamparm *a)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
>   	struct v4l2_subdev_frame_interval fi;
> @@ -398,8 +403,8 @@ static int capture_s_parm(struct file *file, void *fh,
>   	return 0;
>   }
>   
> -static int capture_subscribe_event(struct v4l2_fh *fh,
> -				   const struct v4l2_event_subscription *sub)
> +static int capture_legacy_subscribe_event(struct v4l2_fh *fh,
> +					  const struct v4l2_event_subscription *sub)
>   {
>   	switch (sub->type) {
>   	case V4L2_EVENT_IMX_FRAME_INTERVAL_ERROR:
> @@ -413,42 +418,42 @@ static int capture_subscribe_event(struct v4l2_fh *fh,
>   	}
>   }
>   
> -static const struct v4l2_ioctl_ops capture_ioctl_ops = {
> -	.vidioc_querycap	= capture_querycap,
> +static const struct v4l2_ioctl_ops capture_legacy_ioctl_ops = {
> +	.vidioc_querycap		= capture_querycap,
>   
> -	.vidioc_enum_framesizes = capture_enum_framesizes,
> -	.vidioc_enum_frameintervals = capture_enum_frameintervals,
> +	.vidioc_enum_framesizes		= capture_legacy_enum_framesizes,
> +	.vidioc_enum_frameintervals	= capture_legacy_enum_frameintervals,
>   
> -	.vidioc_enum_fmt_vid_cap        = capture_enum_fmt_vid_cap,
> -	.vidioc_g_fmt_vid_cap           = capture_g_fmt_vid_cap,
> -	.vidioc_try_fmt_vid_cap         = capture_try_fmt_vid_cap,
> -	.vidioc_s_fmt_vid_cap           = capture_s_fmt_vid_cap,
> +	.vidioc_enum_fmt_vid_cap	= capture_legacy_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap		= capture_g_fmt_vid_cap,
> +	.vidioc_try_fmt_vid_cap		= capture_legacy_try_fmt_vid_cap,
> +	.vidioc_s_fmt_vid_cap		= capture_legacy_s_fmt_vid_cap,
>   
> -	.vidioc_querystd        = capture_querystd,
> -	.vidioc_g_std           = capture_g_std,
> -	.vidioc_s_std           = capture_s_std,
> +	.vidioc_querystd		= capture_legacy_querystd,
> +	.vidioc_g_std			= capture_legacy_g_std,
> +	.vidioc_s_std			= capture_legacy_s_std,
>   
> -	.vidioc_g_selection	= capture_g_selection,
> +	.vidioc_g_selection		= capture_g_selection,
>   
> -	.vidioc_g_parm          = capture_g_parm,
> -	.vidioc_s_parm          = capture_s_parm,
> +	.vidioc_g_parm			= capture_legacy_g_parm,
> +	.vidioc_s_parm			= capture_legacy_s_parm,
>   
> -	.vidioc_reqbufs		= vb2_ioctl_reqbufs,
> -	.vidioc_create_bufs     = vb2_ioctl_create_bufs,
> -	.vidioc_prepare_buf     = vb2_ioctl_prepare_buf,
> -	.vidioc_querybuf	= vb2_ioctl_querybuf,
> -	.vidioc_qbuf		= vb2_ioctl_qbuf,
> -	.vidioc_dqbuf		= vb2_ioctl_dqbuf,
> -	.vidioc_expbuf		= vb2_ioctl_expbuf,
> -	.vidioc_streamon	= vb2_ioctl_streamon,
> -	.vidioc_streamoff	= vb2_ioctl_streamoff,
> +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
> +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf		= vb2_ioctl_querybuf,
> +	.vidioc_qbuf			= vb2_ioctl_qbuf,
> +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
> +	.vidioc_expbuf			= vb2_ioctl_expbuf,
> +	.vidioc_streamon		= vb2_ioctl_streamon,
> +	.vidioc_streamoff		= vb2_ioctl_streamoff,
>   
> -	.vidioc_subscribe_event = capture_subscribe_event,
> -	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +	.vidioc_subscribe_event		= capture_legacy_subscribe_event,
> +	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
>   };
>   
> -/*
> - * Queue operations
> +/* -----------------------------------------------------------------------------
> + * Queue Operations
>    */
>   
>   static int capture_queue_setup(struct vb2_queue *vq,
> @@ -540,7 +545,7 @@ static int capture_validate_fmt(struct capture_priv *priv)
>   
>   	v4l2_fill_pix_format(&f.fmt.pix, &fmt_src.format);
>   
> -	ret = __capture_try_fmt_vid_cap(priv, &fmt_src, &f, &cc, &compose);
> +	ret = __capture_legacy_try_fmt(priv, &fmt_src, &f, &cc, &compose);
>   	if (ret)
>   		return ret;
>   
> @@ -616,9 +621,10 @@ static const struct vb2_ops capture_qops = {
>   	.stop_streaming  = capture_stop_streaming,
>   };
>   
> -/*
> - * File operations
> +/* -----------------------------------------------------------------------------
> + * File Operations
>    */
> +
>   static int capture_open(struct file *file)
>   {
>   	struct capture_priv *priv = video_drvdata(file);
> @@ -672,6 +678,10 @@ static const struct v4l2_file_operations capture_fops = {
>   	.mmap		= vb2_fop_mmap,
>   };
>   
> +/* -----------------------------------------------------------------------------
> + * Public API
> + */
> +
>   struct imx_media_buffer *
>   imx_media_capture_device_next_buf(struct imx_media_video_dev *vdev)
>   {
> @@ -821,7 +831,7 @@ imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
>   		return ERR_PTR(-ENOMEM);
>   
>   	vfd->fops = &capture_fops;
> -	vfd->ioctl_ops = &capture_ioctl_ops;
> +	vfd->ioctl_ops = &capture_legacy_ioctl_ops;
>   	vfd->minor = -1;
>   	vfd->release = video_device_release;
>   	vfd->vfl_dir = VFL_DIR_RX;