On Thu, Apr 04, 2024 at 12:24:19PM +0200, Hans Verkuil wrote:
> On 04/04/2024 12:19, Laurent Pinchart wrote:
> > On Thu, Apr 04, 2024 at 12:09:38PM +0200, Hans Verkuil wrote:
> >> On 04/04/2024 10:58, Sakari Ailus wrote:
> >>> 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.
> >>>>
> >>>> 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/
> >>>
> >>> Referring to the discussion that has already taken place, we'd rather offer
> >>> a single API to control cropping and that is the selection API. But I agree
> >>> that there is a theoretical possibility someone might have set this to zero
> >>> and thus compilation could fail.
> >>>
> >>> I'm sure this could be handled on the application still as there was never
> >>> anything to configure here. Breaking binary compatibility would be a real
> >>> issue but that's not what we have here.
> >>
> >> So there is one patch that just fixes the bug and allows the old crop API to be used
> >> with streams, and one kernel patch + several v4l-utils to remove support for it and
> >> potentially break compilation for applications.
> >>
> >> It's silly to remove support when you can just fix it. Yes, there are (and have been
> >> for a long time) two crop APIs (crop and selection), but as long as drivers just have
> >> to deal with one API (selection), I don't really see why you care if applications use
> >> the crop API. You can't remove that ioctl anyway, and the impact is minimal if it is
> >> handled in the core.
> >>
> >> It is really too late to remove the stream field from the crop struct.
> >
> > I should have replied to this e-mail instead of an earlier one in the
> > thread.
> >
> > No application should set the stream field 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.
> >
> >> Perhaps instead make a patch adding a comment to v4l2-subdev.h that that crop struct
> >> is frozen and must not be extended with new features going forward, to prevent the
> >> same thing happening in the future.
> >
> > That's a good idea.
>
> Besides commenting this in the v4l2-subdev.h header, it is probably also good to
> add a comment in v4l2-subdev.c. And perhaps mark it as such in the documentation
> as well?
Sounds good.
> >> Sorry, but I'm not going to accept this patch. It is trivial to fix the crop stream
> >> support, and that patch looks good. And adding a patch to note that v4l2_subdev_crop is
> >> frozen is fine as well and makes perfect sense.
> >
> > Another option is to keep the stream field in the structure, document it
> > as not being used (which is the current behaviour), and dropping the
> > partial handling inside the kernel. I have a feeling this may not be
> > favoured by many though :-)
> >
>
> I'd be willing to consider that if the patch fixing crop stream support
> was huge :-), but since it just adds two lines, that's not exactly the case...
Fair enough.
Tomi has reviewed the original patch ("[PATCH] media: v4l2-subdev: Fix
stream handling for crop API", see [1]). I think we can merge it.
[1] https://lore.kernel.org/linux-media/20240401233725.2401-1-laurent.pinchart@xxxxxxxxxxxxxxxx/
--
Regards,
Laurent Pinchart
