| Message ID | 20210105152852.5733-21-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Miscellaneous fixes and cleanups for i.MX7 | expand |
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;
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
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
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 ?
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
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 ?
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;
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(-)