diff mbox series

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

Message ID 20210105152852.5733-21-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series [01/75] media: imx: Drop dependency on I2C | expand

Commit Message

Laurent Pinchart Jan. 5, 2021, 3:27 p.m. UTC
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(-)

Comments

Steve Longerbeam Jan. 6, 2021, 5:51 p.m. UTC | #1
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;
Philipp Zabel Jan. 7, 2021, 10:52 a.m. UTC | #2
Hi Steve, Laurent,

On Wed, 2021-01-06 at 09:51 -0800, Steve Longerbeam wrote:
> 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!


I don't think Laurent considers the video node legacy, just the fact
that the current implementation looks at the subdev source pad's active
format in ENUM_FRAMESIZES and ENUM_/G/S/TRY_FMT.

I see the behavior of VIDIOC_ENUM_FMT was extended/defined for MC-
centric devices last year, to allow enumerating all pixel formats or
filter pixel formats for a given mbus format:

e5b6b07a1b45 ("media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices")
cfe9e707c564 ("media: open.rst: document mc-centric and video-node-centric")

> 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.


But I don't quite understand why G/S/TRY_FMT should not respect the
connected subdev source pad's active format. Should MC-centric devices
allow to set non-working configurations and only error out on stream
start? Is this documented?

The current "legacy" vb2_ops check the subdev in ENUM_FRAMESIZES and
ENUM_FRAMEINTERVALS, and in TRY_FMT/S_FMT to determine format and
possible interlacing options. If the MC-centric ops just drop that,
there is no way to determine which interlacing combinations are actually
supported.

regards
Philipp
Fabio Estevam Jan. 8, 2021, 12:03 a.m. UTC | #3
Hi Philipp,

On Thu, Jan 7, 2021 at 7:53 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:

> But I don't quite understand why G/S/TRY_FMT should not respect the
> connected subdev source pad's active format. Should MC-centric devices
> allow to set non-working configurations and only error out on stream
> start? Is this documented?
>
> The current "legacy" vb2_ops check the subdev in ENUM_FRAMESIZES and
> ENUM_FRAMEINTERVALS, and in TRY_FMT/S_FMT to determine format and
> possible interlacing options. If the MC-centric ops just drop that,
> there is no way to determine which interlacing combinations are actually
> supported.

You make a good point and it is aligned with what I am seeing here too.

When I tested this series, I noticed a regression on imx6ul-evk
capture and doing a bisect it points to:
media: imx: imx7-media-csi: Disable legacy video node API

Reverting it back to using the legacy API fixed things for me.

When the MC-centric API is used the camera dimensions reported by
Gstreamer are no longer correct.

Regards,

Fabio Estevam
Laurent Pinchart Jan. 9, 2021, 1:09 a.m. UTC | #4
Hi Philipp,

On Thu, Jan 07, 2021 at 11:52:33AM +0100, Philipp Zabel wrote:
> On Wed, 2021-01-06 at 09:51 -0800, Steve Longerbeam wrote:

> > 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!

> 

> I don't think Laurent considers the video node legacy, just the fact

> that the current implementation looks at the subdev source pad's active

> format in ENUM_FRAMESIZES and ENUM_/G/S/TRY_FMT.


Correct. The legacy part here is control of the source subdev through
the video node. It should instead be handled with the media controller
API. The DMA engine side is still handled through the video node, and
that's totally fine.

> I see the behavior of VIDIOC_ENUM_FMT was extended/defined for MC-

> centric devices last year, to allow enumerating all pixel formats or

> filter pixel formats for a given mbus format:

> 

> e5b6b07a1b45 ("media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices")

> cfe9e707c564 ("media: open.rst: document mc-centric and video-node-centric")

> 

> > 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.

> 

> But I don't quite understand why G/S/TRY_FMT should not respect the

> connected subdev source pad's active format. Should MC-centric devices

> allow to set non-working configurations and only error out on stream

> start? Is this documented?


It's the currently recommended practice, but I'm not sure it's
documented.

There are multiple issues here. One of them is that we could limit
VIDIOC_S_FMT to formats compatible with the connected source subdev with
minimum complexity in the implementation, but the source subdev could
then see its configuration being changed by userspace, and updating the
format on the video node automatically would be not only more difficult,
but potentially confusing for userspace as formats are not supposed to
change automatically. For this reason, we need to validate the full
pipeline at stream on time in any case, and restricting the format
ioctls on the video node to the active configuration of the connected
subdev would thus not be that useful.

Another issue is that we've had cases in the past where it was useful
for userspace to configure the video node with a format not matching the
connected subdev, with the subdev configuration later being changed to
match the video node, before starting the stream. I'm not sure if this
use case is still valid today though.

> The current "legacy" vb2_ops check the subdev in ENUM_FRAMESIZES and

> ENUM_FRAMEINTERVALS, and in TRY_FMT/S_FMT to determine format and

> possible interlacing options. If the MC-centric ops just drop that,

> there is no way to determine which interlacing combinations are actually

> supported.


Could you elaborate a little bit here ? We don't have an API to
explicitly enumerate supported interlacing types. This can be done by
calling VIDIOC_TRY_FMT with all field types and see which ones are
supported. You can still do so with the MC-based API, the video node
will return from VIDIOC_TRY_FMT the interlacing types intrinsicly
supported by the video node, and you can query from the source subdev
the interlacing types supported by the source. Userspace can then
combine the information to find what is supported. In this case, with a
source producing V4L2_FIELD_SEQ_TB, and the video node not reporting
V4L2_FIELD_SEQ_TB but reporting V4L2_FIELD_INTERLACED_TB, wouldn't the
application be able to know that V4L2_FIELD_INTERLACED_TB will work ?

-- 
Regards,

Laurent Pinchart
Philipp Zabel Jan. 11, 2021, 8:40 a.m. UTC | #5
Hi Laurent,

On Sat, 2021-01-09 at 03:09 +0200, Laurent Pinchart wrote:
> Could you elaborate a little bit here ? We don't have an API to

> explicitly enumerate supported interlacing types. This can be done by

> calling VIDIOC_TRY_FMT with all field types and see which ones are

> supported. You can still do so with the MC-based API, the video node

> will return from VIDIOC_TRY_FMT the interlacing types intrinsicly

> supported by the video node, and you can query from the source subdev

> the interlacing types supported by the source. Userspace can then

> combine the information to find what is supported.


The i.MX6 CSI always captures whole frames, so SEQ_TB or SEQ_BT at its
source pad.
The IDMAC supports "interlaced scan" of SEQ_TB into INTERLACED_TB and
SEQ_BT into INTERLACED_BT when writing to memory. It can't change the
field timing order (BT <-> TB) as that was already decided at the CSI.

So for capture of interlaced material, the video device currently allows
either SEQ_TB and INTERLACED_TB or SEQ_BT and INTERLACED_BT, depending
on which field order is configured at the CSI source pad.

See d969291d8479 ("media: imx: Fix field negotiation") for details.

regards
Philipp
Laurent Pinchart Feb. 14, 2021, 9:33 p.m. UTC | #6
Hi Philipp,

(CC'ing Sakari)

On Mon, Jan 11, 2021 at 09:40:17AM +0100, Philipp Zabel wrote:
> On Sat, 2021-01-09 at 03:09 +0200, Laurent Pinchart wrote:

> > Could you elaborate a little bit here ? We don't have an API to

> > explicitly enumerate supported interlacing types. This can be done by

> > calling VIDIOC_TRY_FMT with all field types and see which ones are

> > supported. You can still do so with the MC-based API, the video node

> > will return from VIDIOC_TRY_FMT the interlacing types intrinsicly

> > supported by the video node, and you can query from the source subdev

> > the interlacing types supported by the source. Userspace can then

> > combine the information to find what is supported.

> 

> The i.MX6 CSI always captures whole frames, so SEQ_TB or SEQ_BT at its

> source pad.

> The IDMAC supports "interlaced scan" of SEQ_TB into INTERLACED_TB and

> SEQ_BT into INTERLACED_BT when writing to memory. It can't change the

> field timing order (BT <-> TB) as that was already decided at the CSI.

> 

> So for capture of interlaced material, the video device currently allows

> either SEQ_TB and INTERLACED_TB or SEQ_BT and INTERLACED_BT, depending

> on which field order is configured at the CSI source pad.

> 

> See d969291d8479 ("media: imx: Fix field negotiation") for details.


Thanks for the explanation.

With the MC-based API, and unless I'm mistaken, userspace can determine
what interlacing options are supported as follows. Sakari, please feel
free to confirm, or infirm.

- The video node would report support for V4L2_FIELD_NONE,
  V4L2_FIELD_SEQ_(TB|BT) and V4L2_FIELD_INTERLACED_(TB|BT).

- The CSI subdev's source pad would report V4L2_FIELD_NONE for
  progressive sources, and V4L2_FIELD_ALTERNATE for interlaced sources.
  At the bus level top and bottom frames alternate, so
  V4L2_FIELD_SEQ_(TB|BT) isn't valid as it's defined based on buffers.
  V4L2_FIELD_INTERLACED_(TB|BT) isn't valid either in this case when
  frames alternate between top and bottom (they're not interleaved on
  the bus).

- When starting the video stream, the driver would reject a mismatch
  between the source pad and the video node (V4L2_FIELD_NONE on the
  source pad and V4L2_FIELD_SEQ_* or V4L2_FIELD_INTERLACED_* on the
  video node, or the other way around).

- From a userspace point of view, the fact that both V4L2_FIELD_SEQ_*
  and V4L2_FIELD_INTERLACED_* are supported on the video node (which can
  be queried using VIDIOC_S_FMT or VIDIOC_TRY_FMT) means that the DMA
  engine can interleave interlaced content. If the IDMAC wasn't able to
  interleave lines, but was only able to capture the two fields
  sequentially in the same buffer, it would only report
  V4L2_FIELD_SEQ_*, not V4L2_FIELD_INTERLACED_*.

Does this make sense ?

-- 
Regards,

Laurent Pinchart
diff mbox series

Patch

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;