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?
>
>> 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...
Regards,
Hans
