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

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

[Laurent Pinchart]

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];
 };
 
 #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001

base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Thu, Apr 04, 2024 at 01:42:33AM +0300, 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.

Done, see https://lore.kernel.org/linux-media/20240403232425.22304-1-laurent.pinchart@ideasonboard.com/

> 
> [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];
>  };
>  
>  #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001
> 
> base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f

-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

Hi Laurent,

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.

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/

Regards,

	Hans

>  
>  #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE	0x00000001
> 
> base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f

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

[Sakari Ailus]

Hi Hans,

On Thu, Apr 04, 2024 at 10:27:49AM +0200, Hans Verkuil wrote:
> Hi Laurent,
> 
> 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.
> 
> 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/

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.

-- 
Kind regards,

Sakari Ailus

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

[Hans Verkuil]

On 04/04/2024 10:58, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Apr 04, 2024 at 10:27:49AM +0200, Hans Verkuil wrote:
>> Hi Laurent,
>>
>> 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.
>>
>> 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/
> 
> 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.

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.

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.

Regards,

	Hans

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

[Laurent Pinchart]

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

> 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 :-)

-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

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

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

On 04/04/2024 13: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@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.
>>>>
>>>> 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/
>>>
>>> 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.

This was also my argument: it looks very trivial to fix the crop API, 
and while I didn't know the extent of the effort to properly remove the 
streams support for the crop API, I had a gut feeling that it's more work.

>> 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.
> 
>> 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 :-)

That would work too. In that case I think we should check that the 
stream field is set to 0 if the userspace supports streams.

However, I think fixing the bug still makes most sense.

  Tomi

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

[Laurent Pinchart]

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