Hi Hans,
On Thu, Apr 04, 2024 at 10:27:49AM +0200, Hans Verkuil wrote:
> On 04/04/2024 00:42, Laurent Pinchart wrote:
> > When support for streams was added to the V4L2 subdev API, the
> > v4l2_subdev_crop structure was extended with a stream field, but the
> > field was not handled in the core code that translates the
> > VIDIOC_SUBDEV_[GS]_CROP ioctls to the selection API. This could be
> > fixed, but the crop API is deprecated and shouldn't be used by new
> > userspace code. It's therefore best to avoid extending it with new
> > features. Drop the stream field from the v4l2_subdev_crop structure, and
> > update the documentation and kernel code accordingly.
> >
> > Fixes: 2f91e10ee6fd ("media: subdev: add stream based configuration")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > This supersedes the "[PATCH] media: v4l2-subdev: Fix stream handling for
> > crop API" patch ([1]). I'll submit matching patches for v4l2-compliance.
> >
> > [1] https://patchwork.linuxtv.org/project/linux-media/patch/20240401233725.2401-1-laurent.pinchart@xxxxxxxxxxxxxxxx/
> > ---
> > .../userspace-api/media/v4l/vidioc-subdev-g-crop.rst | 5 +----
> > drivers/media/v4l2-core/v4l2-subdev.c | 6 ------
> > include/uapi/linux/v4l2-subdev.h | 4 +---
> > 3 files changed, 2 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst
> > index 92d933631fda..7eeb7b553abf 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst
> > @@ -96,10 +96,7 @@ modified format should be as close as possible to the original request.
> > - ``rect``
> > - Crop rectangle boundaries, in pixels.
> > * - __u32
> > - - ``stream``
> > - - Stream identifier.
> > - * - __u32
> > - - ``reserved``\ [7]
> > + - ``reserved``\ [8]
> > - Reserved for future extensions. Applications and drivers must set
> > the array to zero.
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 4c6198c48dd6..02c2a2b472df 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -725,9 +725,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > struct v4l2_subdev_crop *crop = arg;
> > struct v4l2_subdev_selection sel;
> >
> > - if (!client_supports_streams)
> > - crop->stream = 0;
> > -
> > memset(crop->reserved, 0, sizeof(crop->reserved));
> > memset(&sel, 0, sizeof(sel));
> > sel.which = crop->which;
> > @@ -749,9 +746,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > if (crop->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
> > return -EPERM;
> >
> > - if (!client_supports_streams)
> > - crop->stream = 0;
> > -
> > memset(crop->reserved, 0, sizeof(crop->reserved));
> > memset(&sel, 0, sizeof(sel));
> > sel.which = crop->which;
> > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > index 7048c51581c6..f7eea12d8a2c 100644
> > --- a/include/uapi/linux/v4l2-subdev.h
> > +++ b/include/uapi/linux/v4l2-subdev.h
> > @@ -48,15 +48,13 @@ struct v4l2_subdev_format {
> > * @which: format type (from enum v4l2_subdev_format_whence)
> > * @pad: pad number, as reported by the media API
> > * @rect: pad crop rectangle boundaries
> > - * @stream: stream number, defined in subdev routing
> > * @reserved: drivers and applications must zero this array
> > */
> > struct v4l2_subdev_crop {
> > __u32 which;
> > __u32 pad;
> > struct v4l2_rect rect;
> > - __u32 stream;
> > - __u32 reserved[7];
> > + __u32 reserved[8];
> > };
>
> Sorry, but you can't remove this field. This field has been in the uAPI since
> v6.3, and applications might be using it, even if only to set it to 0. Removing
> this field will break compilation of such applications.
No application should set it to anything else than 0, as stream support
is disabled in the mainline kernel. However, even if I think the risk is
very small, there is indeed a risk than an application may be setting it
to 0.
> Just fix the stream support instead, rather than removing it, as you did in
> your original patch:
>
> https://lore.kernel.org/linux-media/20240401233725.2401-1-laurent.pinchart@xxxxxxxxxxxxxxxx/
We can also mark it as deprecated but stop short of removing it.
> >
> > #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE 0x00000001
> >
> > base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
--
Regards,
Laurent Pinchart
