Re: [PATCH] media: v4l2-subdev: Remove stream support for the crop API

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> > ---
> > 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@ideasonboard.com/
> > ---
> >  .../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@ideasonboard.com/

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