[PATCH v5 0/3] Enumerate all pixels formats

[PATCH v5 0/3] Enumerate all pixels formats

[Benjamin Gaignard]

The goal of this series is to help userspace applications, like Gstreamer
or Chromium, to categorize decoders and so avoid trying to use decoder
that can't do the require task because it won't support the needed pixel
format.
As example, in today implementation we need to simulate 10 bit header
control to know if the driver support 10 bits pixel formats. With this
new flag it will simpler for userspace applications know if driver
support 10 bit pixel formats and if it is supported by userspace.

An example of how it can be used in GStreamer to discover the
supported pixels formats for stateless decoders is available here:
https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/v4l2codecs_enum_all_supported_formats?ref_type=heads

v4l2-compliance test of this flag:
https://gitlab.collabora.com/benjamin.gaignard/v4l-utils/-/tree/enum_all_formats?ref_type=heads

changes in version 5:
- Reset the proposal to follow Hans's advices
- Add new flag to be used with index field.
- Make vicodec and visl test driver use the new flag
- Doing the same for Verisilicon driver.

changes in version 4:
- Explicitly document that the new flags are targeting mem2mem devices.

changes in version 3:
- Add a flag to inform userspace application that driver
  as take care of the flag.

changes in version 2:
- Clarify documentation.
- Only keep V4L2_FMT_FLAG_ALL_FORMATS flag in ioctl.

Benjamin

Benjamin Gaignard (3):
  media: videodev2: Add flag to unconditionnaly enumerate pixels formats
  media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL flag
  media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL flag

 .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 12 +++++++++++-
 .../media/videodev2.h.rst.exceptions            |  1 +
 .../media/platform/verisilicon/hantro_v4l2.c    | 17 ++++++++++++-----
 .../media/test-drivers/vicodec/vicodec-core.c   |  7 ++++---
 drivers/media/test-drivers/visl/visl-video.c    | 11 +++++++----
 include/uapi/linux/videodev2.h                  |  3 +++
 6 files changed, 38 insertions(+), 13 deletions(-)

-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 1/3] media: videodev2: Add flag to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

When the index field is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver
will ignore any configuration and enumerate all the possible formats.
Drivers which do not support this flag yet always return an EINVAL
error code.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
changes in version 5:
- Reset the proposal to follow Hans's advices
- Add new flag to be used with index field.

 .../userspace-api/media/v4l/vidioc-enum-fmt.rst      | 12 +++++++++++-
 .../userspace-api/media/videodev2.h.rst.exceptions   |  1 +
 include/uapi/linux/videodev2.h                       |  3 +++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
index 3adb3d205531..12e1e65e6a71 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
@@ -85,7 +85,11 @@ the ``mbus_code`` field is handled differently:
     * - __u32
       - ``index``
       - Number of the format in the enumeration, set by the application.
-	This is in no way related to the ``pixelformat`` field.
+        This is in no way related to the ``pixelformat`` field. When the
+        index is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver will ignore
+        any configuration and enumerate all the possible formats. Drivers
+        which do not support this flag yet always return an ``EINVAL``
+        error code.
     * - __u32
       - ``type``
       - Type of the data stream, set by the application. Only these types
@@ -234,6 +238,12 @@ the ``mbus_code`` field is handled differently:
 	valid. The buffer consists of ``height`` lines, each having ``width``
 	Data Units of data and the offset (in bytes) between the beginning of
 	each two consecutive lines is ``bytesperline``.
+    * - ``V4L2_FMT_FLAG_ENUM_ALL``
+      - 0x80000000
+      - When the applications ORs ``index`` with ``V4L2_FMT_FLAG_ENUM_ALL`` flag
+        the driver enumerates all the possible pixel formats without taking care
+        of any already set configuration. Drivers which do not support this flag
+        yet always return ``EINVAL``.
 
 Return Value
 ============
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index bdc628e8c1d6..8dc10a500fc6 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
 replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
 replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
 replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
+replace define V4L2_FMT_FLAG_ENUM_ALL fmtdesc-flags
 
 # V4L2 timecode types
 replace define V4L2_TC_TYPE_24FPS timecode-type
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 4e91362da6da..3d11f91273a1 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
 #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
 #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
 
+/*  Format description flag, to be ORed with the index */
+#define V4L2_FMT_FLAG_ENUM_ALL			0x80000000
+
 	/* Frame Size and frame rate enumeration */
 /*
  *	F R A M E   S I Z E   E N U M E R A T I O N
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 1/3] media: videodev2: Add flag to unconditionnaly enumerate pixels formats

[Sebastian Fricke]

Hey Benjamin,

in the subject line:

s/unconditionnaly enumerate pixels/unconditionally enumerate pixel/

On 22.07.2024 17:05, Benjamin Gaignard wrote:
>When the index field is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver

s/is ORed with/is set with/ (same meaning but a lot less confusing)

>will ignore any configuration and enumerate all the possible formats.
>Drivers which do not support this flag yet always return an EINVAL

s/yet always/yet, always/

>error code.
>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
>changes in version 5:
>- Reset the proposal to follow Hans's advices
>- Add new flag to be used with index field.
>
> .../userspace-api/media/v4l/vidioc-enum-fmt.rst      | 12 +++++++++++-
> .../userspace-api/media/videodev2.h.rst.exceptions   |  1 +
> include/uapi/linux/videodev2.h                       |  3 +++
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
>diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>index 3adb3d205531..12e1e65e6a71 100644
>--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>@@ -85,7 +85,11 @@ the ``mbus_code`` field is handled differently:
>     * - __u32
>       - ``index``
>       - Number of the format in the enumeration, set by the application.
>-	This is in no way related to the ``pixelformat`` field.
>+        This is in no way related to the ``pixelformat`` field. When the
>+        index is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver will ignore

s/ORed/set/ (same as above)

>+        any configuration and enumerate all the possible formats. Drivers
>+        which do not support this flag yet always return an ``EINVAL``

s/yet always/yet, always/

>+        error code.
>     * - __u32
>       - ``type``
>       - Type of the data stream, set by the application. Only these types
>@@ -234,6 +238,12 @@ the ``mbus_code`` field is handled differently:
> 	valid. The buffer consists of ``height`` lines, each having ``width``
> 	Data Units of data and the offset (in bytes) between the beginning of
> 	each two consecutive lines is ``bytesperline``.
>+    * - ``V4L2_FMT_FLAG_ENUM_ALL``
>+      - 0x80000000
>+      - When the applications ORs ``index`` with ``V4L2_FMT_FLAG_ENUM_ALL`` flag

s/applications/application/
s/ORs/sets/

>+        the driver enumerates all the possible pixel formats without taking care
>+        of any already set configuration. Drivers which do not support this flag
>+        yet always return ``EINVAL``.

s/yet always/yet, always/

>
> Return Value
> ============
>diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>index bdc628e8c1d6..8dc10a500fc6 100644
>--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>@@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
> replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
> replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
> replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
>+replace define V4L2_FMT_FLAG_ENUM_ALL fmtdesc-flags
>
> # V4L2 timecode types
> replace define V4L2_TC_TYPE_24FPS timecode-type
>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>index 4e91362da6da..3d11f91273a1 100644
>--- a/include/uapi/linux/videodev2.h
>+++ b/include/uapi/linux/videodev2.h
>@@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
> #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>
>+/*  Format description flag, to be ORed with the index */

s/ORed/set/

Regards,
Sebastian

>+#define V4L2_FMT_FLAG_ENUM_ALL			0x80000000
>+
> 	/* Frame Size and frame rate enumeration */
> /*
>  *	F R A M E   S I Z E   E N U M E R A T I O N
>-- 
>2.43.0
>
>_______________________________________________
>Kernel mailing list -- kernel@mailman.collabora.com
>To unsubscribe send an email to kernel-leave@mailman.collabora.com
>This list is managed by https://mailman.collabora.com

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 1/3] media: videodev2: Add flag to unconditionnaly enumerate pixels formats

[Hans Verkuil]

On 22/07/2024 17:05, Benjamin Gaignard wrote:
> When the index field is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver
> will ignore any configuration and enumerate all the possible formats.
> Drivers which do not support this flag yet always return an EINVAL
> error code.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> changes in version 5:
> - Reset the proposal to follow Hans's advices
> - Add new flag to be used with index field.
> 
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst      | 12 +++++++++++-
>  .../userspace-api/media/videodev2.h.rst.exceptions   |  1 +
>  include/uapi/linux/videodev2.h                       |  3 +++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 3adb3d205531..12e1e65e6a71 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -85,7 +85,11 @@ the ``mbus_code`` field is handled differently:
>      * - __u32
>        - ``index``
>        - Number of the format in the enumeration, set by the application.
> -	This is in no way related to the ``pixelformat`` field.
> +        This is in no way related to the ``pixelformat`` field. When the

You need to start a new paragraph before 'When'. Otherwise you might read
that the 'When' sentence is somehow related to the previous sentence.

> +        index is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver will ignore
> +        any configuration and enumerate all the possible formats. Drivers

I'd rephrase this a little bit:

the driver will enumerate all the possible formats, ignoring any limitations
from the current configuration.

And after that I would like to see an example of a use-case.

> +        which do not support this flag yet always return an ``EINVAL``
> +        error code.
>      * - __u32
>        - ``type``
>        - Type of the data stream, set by the application. Only these types
> @@ -234,6 +238,12 @@ the ``mbus_code`` field is handled differently:
>  	valid. The buffer consists of ``height`` lines, each having ``width``
>  	Data Units of data and the offset (in bytes) between the beginning of
>  	each two consecutive lines is ``bytesperline``.
> +    * - ``V4L2_FMT_FLAG_ENUM_ALL``

I am not really happy with this name since the prefix is identical to that
of other V4L2_FMT_FLAG_ defines. How about: V4L2_FMTDESC_FLAG_ENUM_ALL?
Or V4L2_FMT_IDX_ENUM_ALL?

> +      - 0x80000000
> +      - When the applications ORs ``index`` with ``V4L2_FMT_FLAG_ENUM_ALL`` flag
> +        the driver enumerates all the possible pixel formats without taking care
> +        of any already set configuration. Drivers which do not support this flag
> +        yet always return ``EINVAL``.
>  
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index bdc628e8c1d6..8dc10a500fc6 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
>  replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
>  replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
>  replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
> +replace define V4L2_FMT_FLAG_ENUM_ALL fmtdesc-flags
>  
>  # V4L2 timecode types
>  replace define V4L2_TC_TYPE_24FPS timecode-type
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 4e91362da6da..3d11f91273a1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>  
> +/*  Format description flag, to be ORed with the index */
> +#define V4L2_FMT_FLAG_ENUM_ALL			0x80000000
> +
>  	/* Frame Size and frame rate enumeration */
>  /*
>   *	F R A M E   S I Z E   E N U M E R A T I O N

Regards,

	Hans

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 1/3] media: videodev2: Add flag to unconditionnaly enumerate pixels formats

[Hans Verkuil]

On 30/07/2024 09:08, Hans Verkuil wrote:
> On 22/07/2024 17:05, Benjamin Gaignard wrote:
>> When the index field is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver
>> will ignore any configuration and enumerate all the possible formats.
>> Drivers which do not support this flag yet always return an EINVAL
>> error code.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> changes in version 5:
>> - Reset the proposal to follow Hans's advices
>> - Add new flag to be used with index field.
>>
>>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst      | 12 +++++++++++-
>>  .../userspace-api/media/videodev2.h.rst.exceptions   |  1 +
>>  include/uapi/linux/videodev2.h                       |  3 +++
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> index 3adb3d205531..12e1e65e6a71 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> @@ -85,7 +85,11 @@ the ``mbus_code`` field is handled differently:
>>      * - __u32
>>        - ``index``
>>        - Number of the format in the enumeration, set by the application.
>> -	This is in no way related to the ``pixelformat`` field.
>> +        This is in no way related to the ``pixelformat`` field. When the
> 
> You need to start a new paragraph before 'When'. Otherwise you might read
> that the 'When' sentence is somehow related to the previous sentence.
> 
>> +        index is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver will ignore
>> +        any configuration and enumerate all the possible formats. Drivers
> 
> I'd rephrase this a little bit:
> 
> the driver will enumerate all the possible formats, ignoring any limitations
> from the current configuration.
> 
> And after that I would like to see an example of a use-case.

Should the flag be kept on return of VIDIOC_ENUM_FMT or should it be cleared?
For reference: VIDIOC_QUERYCTRL will clear the V4L2_CTRL_FLAG_NEXT_CTRL flag.

Regardless of what we pick, it should be documented.

Regards,

	Hans

> 
>> +        which do not support this flag yet always return an ``EINVAL``
>> +        error code.
>>      * - __u32
>>        - ``type``
>>        - Type of the data stream, set by the application. Only these types
>> @@ -234,6 +238,12 @@ the ``mbus_code`` field is handled differently:
>>  	valid. The buffer consists of ``height`` lines, each having ``width``
>>  	Data Units of data and the offset (in bytes) between the beginning of
>>  	each two consecutive lines is ``bytesperline``.
>> +    * - ``V4L2_FMT_FLAG_ENUM_ALL``
> 
> I am not really happy with this name since the prefix is identical to that
> of other V4L2_FMT_FLAG_ defines. How about: V4L2_FMTDESC_FLAG_ENUM_ALL?
> Or V4L2_FMT_IDX_ENUM_ALL?
> 
>> +      - 0x80000000
>> +      - When the applications ORs ``index`` with ``V4L2_FMT_FLAG_ENUM_ALL`` flag
>> +        the driver enumerates all the possible pixel formats without taking care
>> +        of any already set configuration. Drivers which do not support this flag
>> +        yet always return ``EINVAL``.
>>  
>>  Return Value
>>  ============
>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> index bdc628e8c1d6..8dc10a500fc6 100644
>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> @@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
>>  replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
>>  replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
>>  replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
>> +replace define V4L2_FMT_FLAG_ENUM_ALL fmtdesc-flags
>>  
>>  # V4L2 timecode types
>>  replace define V4L2_TC_TYPE_24FPS timecode-type
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 4e91362da6da..3d11f91273a1 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
>>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>>  
>> +/*  Format description flag, to be ORed with the index */
>> +#define V4L2_FMT_FLAG_ENUM_ALL			0x80000000
>> +
>>  	/* Frame Size and frame rate enumeration */
>>  /*
>>   *	F R A M E   S I Z E   E N U M E R A T I O N
> 
> Regards,
> 
> 	Hans
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 1/3] media: videodev2: Add flag to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

Le 30/07/2024 à 09:19, Hans Verkuil a écrit :
> On 30/07/2024 09:08, Hans Verkuil wrote:
>> On 22/07/2024 17:05, Benjamin Gaignard wrote:
>>> When the index field is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver
>>> will ignore any configuration and enumerate all the possible formats.
>>> Drivers which do not support this flag yet always return an EINVAL
>>> error code.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>> changes in version 5:
>>> - Reset the proposal to follow Hans's advices
>>> - Add new flag to be used with index field.
>>>
>>>   .../userspace-api/media/v4l/vidioc-enum-fmt.rst      | 12 +++++++++++-
>>>   .../userspace-api/media/videodev2.h.rst.exceptions   |  1 +
>>>   include/uapi/linux/videodev2.h                       |  3 +++
>>>   3 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>> index 3adb3d205531..12e1e65e6a71 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>> @@ -85,7 +85,11 @@ the ``mbus_code`` field is handled differently:
>>>       * - __u32
>>>         - ``index``
>>>         - Number of the format in the enumeration, set by the application.
>>> -	This is in no way related to the ``pixelformat`` field.
>>> +        This is in no way related to the ``pixelformat`` field. When the
>> You need to start a new paragraph before 'When'. Otherwise you might read
>> that the 'When' sentence is somehow related to the previous sentence.
>>
>>> +        index is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver will ignore
>>> +        any configuration and enumerate all the possible formats. Drivers
>> I'd rephrase this a little bit:
>>
>> the driver will enumerate all the possible formats, ignoring any limitations
>> from the current configuration.
>>
>> And after that I would like to see an example of a use-case.
> Should the flag be kept on return of VIDIOC_ENUM_FMT or should it be cleared?
> For reference: VIDIOC_QUERYCTRL will clear the V4L2_CTRL_FLAG_NEXT_CTRL flag.
>
> Regardless of what we pick, it should be documented.

I believe most of the flags are cleared in v4l2 so I will to do the same and
document it.

>
> Regards,
>
> 	Hans
>
>>> +        which do not support this flag yet always return an ``EINVAL``
>>> +        error code.
>>>       * - __u32
>>>         - ``type``
>>>         - Type of the data stream, set by the application. Only these types
>>> @@ -234,6 +238,12 @@ the ``mbus_code`` field is handled differently:
>>>   	valid. The buffer consists of ``height`` lines, each having ``width``
>>>   	Data Units of data and the offset (in bytes) between the beginning of
>>>   	each two consecutive lines is ``bytesperline``.
>>> +    * - ``V4L2_FMT_FLAG_ENUM_ALL``
>> I am not really happy with this name since the prefix is identical to that
>> of other V4L2_FMT_FLAG_ defines. How about: V4L2_FMTDESC_FLAG_ENUM_ALL?
>> Or V4L2_FMT_IDX_ENUM_ALL?
>>
>>> +      - 0x80000000
>>> +      - When the applications ORs ``index`` with ``V4L2_FMT_FLAG_ENUM_ALL`` flag
>>> +        the driver enumerates all the possible pixel formats without taking care
>>> +        of any already set configuration. Drivers which do not support this flag
>>> +        yet always return ``EINVAL``.
>>>   
>>>   Return Value
>>>   ============
>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> index bdc628e8c1d6..8dc10a500fc6 100644
>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> @@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
>>>   replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
>>>   replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
>>>   replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
>>> +replace define V4L2_FMT_FLAG_ENUM_ALL fmtdesc-flags
>>>   
>>>   # V4L2 timecode types
>>>   replace define V4L2_TC_TYPE_24FPS timecode-type
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 4e91362da6da..3d11f91273a1 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
>>>   #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>>   #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>>>   
>>> +/*  Format description flag, to be ORed with the index */
>>> +#define V4L2_FMT_FLAG_ENUM_ALL			0x80000000
>>> +
>>>   	/* Frame Size and frame rate enumeration */
>>>   /*
>>>    *	F R A M E   S I Z E   E N U M E R A T I O N
>> Regards,
>>
>> 	Hans
>>
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 1/3] media: videodev2: Add flag to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

Le 30/07/2024 à 09:08, Hans Verkuil a écrit :
> On 22/07/2024 17:05, Benjamin Gaignard wrote:
>> When the index field is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver
>> will ignore any configuration and enumerate all the possible formats.
>> Drivers which do not support this flag yet always return an EINVAL
>> error code.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> changes in version 5:
>> - Reset the proposal to follow Hans's advices
>> - Add new flag to be used with index field.
>>
>>   .../userspace-api/media/v4l/vidioc-enum-fmt.rst      | 12 +++++++++++-
>>   .../userspace-api/media/videodev2.h.rst.exceptions   |  1 +
>>   include/uapi/linux/videodev2.h                       |  3 +++
>>   3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> index 3adb3d205531..12e1e65e6a71 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> @@ -85,7 +85,11 @@ the ``mbus_code`` field is handled differently:
>>       * - __u32
>>         - ``index``
>>         - Number of the format in the enumeration, set by the application.
>> -	This is in no way related to the ``pixelformat`` field.
>> +        This is in no way related to the ``pixelformat`` field. When the
> You need to start a new paragraph before 'When'. Otherwise you might read
> that the 'When' sentence is somehow related to the previous sentence.
>
>> +        index is ORed with V4L2_FMT_FLAG_ENUM_ALL the driver will ignore
>> +        any configuration and enumerate all the possible formats. Drivers
> I'd rephrase this a little bit:
>
> the driver will enumerate all the possible formats, ignoring any limitations
> from the current configuration.
>
> And after that I would like to see an example of a use-case.
>
>> +        which do not support this flag yet always return an ``EINVAL``
>> +        error code.
>>       * - __u32
>>         - ``type``
>>         - Type of the data stream, set by the application. Only these types
>> @@ -234,6 +238,12 @@ the ``mbus_code`` field is handled differently:
>>   	valid. The buffer consists of ``height`` lines, each having ``width``
>>   	Data Units of data and the offset (in bytes) between the beginning of
>>   	each two consecutive lines is ``bytesperline``.
>> +    * - ``V4L2_FMT_FLAG_ENUM_ALL``
> I am not really happy with this name since the prefix is identical to that
> of other V4L2_FMT_FLAG_ defines. How about: V4L2_FMTDESC_FLAG_ENUM_ALL?
> Or V4L2_FMT_IDX_ENUM_ALL?

I will use V4L2_FMTDESC_FLAG_ENUM_ALL in the next version.

Regards,
Benjamin

>
>> +      - 0x80000000
>> +      - When the applications ORs ``index`` with ``V4L2_FMT_FLAG_ENUM_ALL`` flag
>> +        the driver enumerates all the possible pixel formats without taking care
>> +        of any already set configuration. Drivers which do not support this flag
>> +        yet always return ``EINVAL``.
>>   
>>   Return Value
>>   ============
>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> index bdc628e8c1d6..8dc10a500fc6 100644
>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> @@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
>>   replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
>>   replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
>>   replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
>> +replace define V4L2_FMT_FLAG_ENUM_ALL fmtdesc-flags
>>   
>>   # V4L2 timecode types
>>   replace define V4L2_TC_TYPE_24FPS timecode-type
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 4e91362da6da..3d11f91273a1 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
>>   #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>   #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>>   
>> +/*  Format description flag, to be ORed with the index */
>> +#define V4L2_FMT_FLAG_ENUM_ALL			0x80000000
>> +
>>   	/* Frame Size and frame rate enumeration */
>>   /*
>>    *	F R A M E   S I Z E   E N U M E R A T I O N
> Regards,
>
> 	Hans
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Benjamin Gaignard]

Since V4L2_FMT_FLAG_ENUM_ALL flag mostly targeting stateless
decoder pixel formats enumeration, update vicodec visl test
drivers to use it.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 drivers/media/test-drivers/vicodec/vicodec-core.c |  7 ++++---
 drivers/media/test-drivers/visl/visl-video.c      | 11 +++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
index 3e011fe62ae1..1b4cd8ddd7c2 100644
--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
@@ -706,6 +706,7 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
 		    bool is_out)
 {
 	bool is_uncomp = (ctx->is_enc && is_out) || (!ctx->is_enc && !is_out);
+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(f->type) && !multiplanar)
 		return -EINVAL;
@@ -718,18 +719,18 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
 
 		if (ctx->is_enc ||
 		    !vb2_is_streaming(&ctx->fh.m2m_ctx->cap_q_ctx.q))
-			info = v4l2_fwht_get_pixfmt(f->index);
+			info = v4l2_fwht_get_pixfmt(index);
 		else
 			info = v4l2_fwht_find_nth_fmt(info->width_div,
 						     info->height_div,
 						     info->components_num,
 						     info->pixenc,
-						     f->index);
+						     index);
 		if (!info)
 			return -EINVAL;
 		f->pixelformat = info->id;
 	} else {
-		if (f->index)
+		if (index)
 			return -EINVAL;
 		f->pixelformat = ctx->is_stateless ?
 			V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
index f8d970319764..c5f3e13b4198 100644
--- a/drivers/media/test-drivers/visl/visl-video.c
+++ b/drivers/media/test-drivers/visl/visl-video.c
@@ -341,21 +341,24 @@ static int visl_enum_fmt_vid_cap(struct file *file, void *priv,
 				 struct v4l2_fmtdesc *f)
 {
 	struct visl_ctx *ctx = visl_file_to_ctx(file);
+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
 
-	if (f->index >= ctx->coded_format_desc->num_decoded_fmts)
+	if (index >= ctx->coded_format_desc->num_decoded_fmts)
 		return -EINVAL;
 
-	f->pixelformat = ctx->coded_format_desc->decoded_fmts[f->index];
+	f->pixelformat = ctx->coded_format_desc->decoded_fmts[index];
 	return 0;
 }
 
 static int visl_enum_fmt_vid_out(struct file *file, void *priv,
 				 struct v4l2_fmtdesc *f)
 {
-	if (f->index >= ARRAY_SIZE(visl_coded_fmts))
+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
+
+	if (index >= ARRAY_SIZE(visl_coded_fmts))
 		return -EINVAL;
 
-	f->pixelformat = visl_coded_fmts[f->index].pixelformat;
+	f->pixelformat = visl_coded_fmts[index].pixelformat;
 	return 0;
 }
 
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Sebastian Fricke]

Hey Benjamin,

On 22.07.2024 17:05, Benjamin Gaignard wrote:
>Since V4L2_FMT_FLAG_ENUM_ALL flag mostly targeting stateless

s/Since/Since the/
s/targeting/targets/

>decoder pixel formats enumeration, update vicodec visl test

s/pixel formats/pixel-format/

>drivers to use it.

s/drivers/driver/

The rest below basically just strips the flag from every use-case of the
index, before using it.

I wonder couldn't we implement a macro for this, as I believe this will
have to be done in a lot of places, something like:

/*
  * Drivers can support an enumeration of all formats, by ORing the
  * V4L2_FMT_FLAG_ENUM_ALL flag into the index field.
  * In order to use the index field, strip the flag first.
  *
  * See vidioc-enum-fmt.rst documentation for more details.
  */
#define STRIP_ENUM_ALL_FLAG(index) \
   index & ~V4L2_FMT_FLAG_ENUM_ALL

And then use it like this:

u32 index = STRIP_ENUM_ALL_FLAG(f->index);

If we define this in a common header, then every driver can easily
utilize it and you have built-in documentation.

What do you think?

Regards,
Sebastian

>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
> drivers/media/test-drivers/vicodec/vicodec-core.c |  7 ++++---
> drivers/media/test-drivers/visl/visl-video.c      | 11 +++++++----
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
>index 3e011fe62ae1..1b4cd8ddd7c2 100644
>--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
>+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
>@@ -706,6 +706,7 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
> 		    bool is_out)
> {
> 	bool is_uncomp = (ctx->is_enc && is_out) || (!ctx->is_enc && !is_out);
>+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>
> 	if (V4L2_TYPE_IS_MULTIPLANAR(f->type) && !multiplanar)
> 		return -EINVAL;
>@@ -718,18 +719,18 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>
> 		if (ctx->is_enc ||
> 		    !vb2_is_streaming(&ctx->fh.m2m_ctx->cap_q_ctx.q))
>-			info = v4l2_fwht_get_pixfmt(f->index);
>+			info = v4l2_fwht_get_pixfmt(index);
> 		else
> 			info = v4l2_fwht_find_nth_fmt(info->width_div,
> 						     info->height_div,
> 						     info->components_num,
> 						     info->pixenc,
>-						     f->index);
>+						     index);
> 		if (!info)
> 			return -EINVAL;
> 		f->pixelformat = info->id;
> 	} else {
>-		if (f->index)
>+		if (index)
> 			return -EINVAL;
> 		f->pixelformat = ctx->is_stateless ?
> 			V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
>diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
>index f8d970319764..c5f3e13b4198 100644
>--- a/drivers/media/test-drivers/visl/visl-video.c
>+++ b/drivers/media/test-drivers/visl/visl-video.c
>@@ -341,21 +341,24 @@ static int visl_enum_fmt_vid_cap(struct file *file, void *priv,
> 				 struct v4l2_fmtdesc *f)
> {
> 	struct visl_ctx *ctx = visl_file_to_ctx(file);
>+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>
>-	if (f->index >= ctx->coded_format_desc->num_decoded_fmts)
>+	if (index >= ctx->coded_format_desc->num_decoded_fmts)
> 		return -EINVAL;
>
>-	f->pixelformat = ctx->coded_format_desc->decoded_fmts[f->index];
>+	f->pixelformat = ctx->coded_format_desc->decoded_fmts[index];
> 	return 0;
> }
>
> static int visl_enum_fmt_vid_out(struct file *file, void *priv,
> 				 struct v4l2_fmtdesc *f)
> {
>-	if (f->index >= ARRAY_SIZE(visl_coded_fmts))
>+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>+
>+	if (index >= ARRAY_SIZE(visl_coded_fmts))
> 		return -EINVAL;
>
>-	f->pixelformat = visl_coded_fmts[f->index].pixelformat;
>+	f->pixelformat = visl_coded_fmts[index].pixelformat;
> 	return 0;
> }
>
>-- 
>2.43.0
>
>_______________________________________________
>Kernel mailing list -- kernel@mailman.collabora.com
>To unsubscribe send an email to kernel-leave@mailman.collabora.com
>This list is managed by https://mailman.collabora.com

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Hans Verkuil]

On 23/07/2024 11:49, Sebastian Fricke wrote:
> Hey Benjamin,
> 
> On 22.07.2024 17:05, Benjamin Gaignard wrote:
>> Since V4L2_FMT_FLAG_ENUM_ALL flag mostly targeting stateless
> 
> s/Since/Since the/
> s/targeting/targets/
> 
>> decoder pixel formats enumeration, update vicodec visl test
> 
> s/pixel formats/pixel-format/
> 
>> drivers to use it.
> 
> s/drivers/driver/
> 
> The rest below basically just strips the flag from every use-case of the
> index, before using it.
> 
> I wonder couldn't we implement a macro for this, as I believe this will
> have to be done in a lot of places, something like:
> 
> /*
>  * Drivers can support an enumeration of all formats, by ORing the
>  * V4L2_FMT_FLAG_ENUM_ALL flag into the index field.
>  * In order to use the index field, strip the flag first.
>  *
>  * See vidioc-enum-fmt.rst documentation for more details.
>  */
> #define STRIP_ENUM_ALL_FLAG(index) \
>   index & ~V4L2_FMT_FLAG_ENUM_ALL
> 
> And then use it like this:
> 
> u32 index = STRIP_ENUM_ALL_FLAG(f->index);

There is really no advantage to that. It is much better to be explicit
about it rather than hiding it in a define.

Regards,

	Hans

> 
> If we define this in a common header, then every driver can easily
> utilize it and you have built-in documentation.
> 
> What do you think?
> 
> Regards,
> Sebastian
> 
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> drivers/media/test-drivers/vicodec/vicodec-core.c |  7 ++++---
>> drivers/media/test-drivers/visl/visl-video.c      | 11 +++++++----
>> 2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
>> index 3e011fe62ae1..1b4cd8ddd7c2 100644
>> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
>> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
>> @@ -706,6 +706,7 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>>             bool is_out)
>> {
>>     bool is_uncomp = (ctx->is_enc && is_out) || (!ctx->is_enc && !is_out);
>> +    u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>>
>>     if (V4L2_TYPE_IS_MULTIPLANAR(f->type) && !multiplanar)
>>         return -EINVAL;
>> @@ -718,18 +719,18 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>>
>>         if (ctx->is_enc ||
>>             !vb2_is_streaming(&ctx->fh.m2m_ctx->cap_q_ctx.q))
>> -            info = v4l2_fwht_get_pixfmt(f->index);
>> +            info = v4l2_fwht_get_pixfmt(index);
>>         else
>>             info = v4l2_fwht_find_nth_fmt(info->width_div,
>>                              info->height_div,
>>                              info->components_num,
>>                              info->pixenc,
>> -                             f->index);
>> +                             index);
>>         if (!info)
>>             return -EINVAL;
>>         f->pixelformat = info->id;
>>     } else {
>> -        if (f->index)
>> +        if (index)
>>             return -EINVAL;
>>         f->pixelformat = ctx->is_stateless ?
>>             V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
>> diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
>> index f8d970319764..c5f3e13b4198 100644
>> --- a/drivers/media/test-drivers/visl/visl-video.c
>> +++ b/drivers/media/test-drivers/visl/visl-video.c
>> @@ -341,21 +341,24 @@ static int visl_enum_fmt_vid_cap(struct file *file, void *priv,
>>                  struct v4l2_fmtdesc *f)
>> {
>>     struct visl_ctx *ctx = visl_file_to_ctx(file);
>> +    u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>>
>> -    if (f->index >= ctx->coded_format_desc->num_decoded_fmts)
>> +    if (index >= ctx->coded_format_desc->num_decoded_fmts)
>>         return -EINVAL;
>>
>> -    f->pixelformat = ctx->coded_format_desc->decoded_fmts[f->index];
>> +    f->pixelformat = ctx->coded_format_desc->decoded_fmts[index];
>>     return 0;
>> }
>>
>> static int visl_enum_fmt_vid_out(struct file *file, void *priv,
>>                  struct v4l2_fmtdesc *f)
>> {
>> -    if (f->index >= ARRAY_SIZE(visl_coded_fmts))
>> +    u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>> +
>> +    if (index >= ARRAY_SIZE(visl_coded_fmts))
>>         return -EINVAL;
>>
>> -    f->pixelformat = visl_coded_fmts[f->index].pixelformat;
>> +    f->pixelformat = visl_coded_fmts[index].pixelformat;
>>     return 0;
>> }
>>
>> -- 
>> 2.43.0
>>
>> _______________________________________________
>> Kernel mailing list -- kernel@mailman.collabora.com
>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>> This list is managed by https://mailman.collabora.com
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Hans Verkuil]

On 22/07/2024 17:05, Benjamin Gaignard wrote:
> Since V4L2_FMT_FLAG_ENUM_ALL flag mostly targeting stateless
> decoder pixel formats enumeration, update vicodec visl test
> drivers to use it.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  drivers/media/test-drivers/vicodec/vicodec-core.c |  7 ++++---
>  drivers/media/test-drivers/visl/visl-video.c      | 11 +++++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
> index 3e011fe62ae1..1b4cd8ddd7c2 100644
> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
> @@ -706,6 +706,7 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>  		    bool is_out)
>  {
>  	bool is_uncomp = (ctx->is_enc && is_out) || (!ctx->is_enc && !is_out);
> +	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;

This is not what I am looking for: to properly test this in v4l2-compliance this
flag actually has to make a difference in the result. I.e. you actually have to
add some limitation. This might be easier to do in visl than vicodec. As long as
at least one test-driver support this, then that's good enough for me.

Regards,

	Hans

>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(f->type) && !multiplanar)
>  		return -EINVAL;
> @@ -718,18 +719,18 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>  
>  		if (ctx->is_enc ||
>  		    !vb2_is_streaming(&ctx->fh.m2m_ctx->cap_q_ctx.q))
> -			info = v4l2_fwht_get_pixfmt(f->index);
> +			info = v4l2_fwht_get_pixfmt(index);
>  		else
>  			info = v4l2_fwht_find_nth_fmt(info->width_div,
>  						     info->height_div,
>  						     info->components_num,
>  						     info->pixenc,
> -						     f->index);
> +						     index);
>  		if (!info)
>  			return -EINVAL;
>  		f->pixelformat = info->id;
>  	} else {
> -		if (f->index)
> +		if (index)
>  			return -EINVAL;
>  		f->pixelformat = ctx->is_stateless ?
>  			V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
> diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
> index f8d970319764..c5f3e13b4198 100644
> --- a/drivers/media/test-drivers/visl/visl-video.c
> +++ b/drivers/media/test-drivers/visl/visl-video.c
> @@ -341,21 +341,24 @@ static int visl_enum_fmt_vid_cap(struct file *file, void *priv,
>  				 struct v4l2_fmtdesc *f)
>  {
>  	struct visl_ctx *ctx = visl_file_to_ctx(file);
> +	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>  
> -	if (f->index >= ctx->coded_format_desc->num_decoded_fmts)
> +	if (index >= ctx->coded_format_desc->num_decoded_fmts)
>  		return -EINVAL;
>  
> -	f->pixelformat = ctx->coded_format_desc->decoded_fmts[f->index];
> +	f->pixelformat = ctx->coded_format_desc->decoded_fmts[index];
>  	return 0;
>  }
>  
>  static int visl_enum_fmt_vid_out(struct file *file, void *priv,
>  				 struct v4l2_fmtdesc *f)
>  {
> -	if (f->index >= ARRAY_SIZE(visl_coded_fmts))
> +	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
> +
> +	if (index >= ARRAY_SIZE(visl_coded_fmts))
>  		return -EINVAL;
>  
> -	f->pixelformat = visl_coded_fmts[f->index].pixelformat;
> +	f->pixelformat = visl_coded_fmts[index].pixelformat;
>  	return 0;
>  }
>  


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Benjamin Gaignard]

Le 30/07/2024 à 09:13, Hans Verkuil a écrit :
> On 22/07/2024 17:05, Benjamin Gaignard wrote:
>> Since V4L2_FMT_FLAG_ENUM_ALL flag mostly targeting stateless
>> decoder pixel formats enumeration, update vicodec visl test
>> drivers to use it.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   drivers/media/test-drivers/vicodec/vicodec-core.c |  7 ++++---
>>   drivers/media/test-drivers/visl/visl-video.c      | 11 +++++++----
>>   2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
>> index 3e011fe62ae1..1b4cd8ddd7c2 100644
>> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
>> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
>> @@ -706,6 +706,7 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>>   		    bool is_out)
>>   {
>>   	bool is_uncomp = (ctx->is_enc && is_out) || (!ctx->is_enc && !is_out);
>> +	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
> This is not what I am looking for: to properly test this in v4l2-compliance this
> flag actually has to make a difference in the result. I.e. you actually have to
> add some limitation. This might be easier to do in visl than vicodec. As long as
> at least one test-driver support this, then that's good enough for me.

Ok I will focus on visl and made it return another list of formats when the
flag is set.

Regards,
Benjamin

> Regards,
>
> 	Hans
>
>>   
>>   	if (V4L2_TYPE_IS_MULTIPLANAR(f->type) && !multiplanar)
>>   		return -EINVAL;
>> @@ -718,18 +719,18 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>>   
>>   		if (ctx->is_enc ||
>>   		    !vb2_is_streaming(&ctx->fh.m2m_ctx->cap_q_ctx.q))
>> -			info = v4l2_fwht_get_pixfmt(f->index);
>> +			info = v4l2_fwht_get_pixfmt(index);
>>   		else
>>   			info = v4l2_fwht_find_nth_fmt(info->width_div,
>>   						     info->height_div,
>>   						     info->components_num,
>>   						     info->pixenc,
>> -						     f->index);
>> +						     index);
>>   		if (!info)
>>   			return -EINVAL;
>>   		f->pixelformat = info->id;
>>   	} else {
>> -		if (f->index)
>> +		if (index)
>>   			return -EINVAL;
>>   		f->pixelformat = ctx->is_stateless ?
>>   			V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
>> diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
>> index f8d970319764..c5f3e13b4198 100644
>> --- a/drivers/media/test-drivers/visl/visl-video.c
>> +++ b/drivers/media/test-drivers/visl/visl-video.c
>> @@ -341,21 +341,24 @@ static int visl_enum_fmt_vid_cap(struct file *file, void *priv,
>>   				 struct v4l2_fmtdesc *f)
>>   {
>>   	struct visl_ctx *ctx = visl_file_to_ctx(file);
>> +	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>>   
>> -	if (f->index >= ctx->coded_format_desc->num_decoded_fmts)
>> +	if (index >= ctx->coded_format_desc->num_decoded_fmts)
>>   		return -EINVAL;
>>   
>> -	f->pixelformat = ctx->coded_format_desc->decoded_fmts[f->index];
>> +	f->pixelformat = ctx->coded_format_desc->decoded_fmts[index];
>>   	return 0;
>>   }
>>   
>>   static int visl_enum_fmt_vid_out(struct file *file, void *priv,
>>   				 struct v4l2_fmtdesc *f)
>>   {
>> -	if (f->index >= ARRAY_SIZE(visl_coded_fmts))
>> +	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>> +
>> +	if (index >= ARRAY_SIZE(visl_coded_fmts))
>>   		return -EINVAL;
>>   
>> -	f->pixelformat = visl_coded_fmts[f->index].pixelformat;
>> +	f->pixelformat = visl_coded_fmts[index].pixelformat;
>>   	return 0;
>>   }
>>   
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 3/3] media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Benjamin Gaignard]

By adding support of V4L2_FMT_FLAG_ENUM_ALL flag into the driver
we allowing userspce applications to discover all possible
pixel formats of the hardware block. This way userspace can decide
of which decoder to use given the support pixel formats.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../media/platform/verisilicon/hantro_v4l2.c    | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index df6f2536263b..77f024aaa22d 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -201,7 +201,14 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
 	struct hantro_ctx *ctx = fh_to_ctx(priv);
 	const struct hantro_fmt *fmt, *formats;
 	unsigned int num_fmts, i, j = 0;
-	bool skip_mode_none;
+	bool skip_mode_none, enum_all_formats;
+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
+
+	/*
+	 * If V4L2_FMT_FLAG_ENUM_ALL flag is set, we want to enumerate all
+	 * hardware supported pixels formats
+	 */
+	enum_all_formats = !!(f->index & V4L2_FMT_FLAG_ENUM_ALL);
 
 	/*
 	 * When dealing with an encoder:
@@ -222,9 +229,9 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
 
 		if (skip_mode_none == mode_none)
 			continue;
-		if (!hantro_check_depth_match(fmt, ctx->bit_depth))
+		if (!hantro_check_depth_match(fmt, ctx->bit_depth) && !enum_all_formats)
 			continue;
-		if (j == f->index) {
+		if (j == index) {
 			f->pixelformat = fmt->fourcc;
 			return 0;
 		}
@@ -242,9 +249,9 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
 	for (i = 0; i < num_fmts; i++) {
 		fmt = &formats[i];
 
-		if (!hantro_check_depth_match(fmt, ctx->bit_depth))
+		if (!hantro_check_depth_match(fmt, ctx->bit_depth) && !enum_all_formats)
 			continue;
-		if (j == f->index) {
+		if (j == index) {
 			f->pixelformat = fmt->fourcc;
 			return 0;
 		}
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/3] media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL flag

[Sebastian Fricke]

Hey Benjamin,

On 22.07.2024 17:05, Benjamin Gaignard wrote:
>By adding support of V4L2_FMT_FLAG_ENUM_ALL flag into the driver

s/support of/support for the/

>we allowing userspce applications to discover all possible

Either:
s/we allowing/we allow/
or
s/we allowing/we are allowing/
are correct.

s/userspce/userspace/

>pixel formats of the hardware block. This way userspace can decide
>of which decoder to use given the support pixel formats.

s/of which/which/

s/support/supported/

>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
> .../media/platform/verisilicon/hantro_v4l2.c    | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>index df6f2536263b..77f024aaa22d 100644
>--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
>+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
>@@ -201,7 +201,14 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
> 	struct hantro_ctx *ctx = fh_to_ctx(priv);
> 	const struct hantro_fmt *fmt, *formats;
> 	unsigned int num_fmts, i, j = 0;
>-	bool skip_mode_none;
>+	bool skip_mode_none, enum_all_formats;
>+	u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;

As described in the previous patch, I think this operation will turn out
to be very common, so maybe we can add a macro for that.

>+
>+	/*
>+	 * If V4L2_FMT_FLAG_ENUM_ALL flag is set, we want to enumerate all

s/If/If the/

>+	 * hardware supported pixels formats

s/pixels/pixel/

Greetings,
Sebastian

>+	 */
>+	enum_all_formats = !!(f->index & V4L2_FMT_FLAG_ENUM_ALL);
>
> 	/*
> 	 * When dealing with an encoder:
>@@ -222,9 +229,9 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
>
> 		if (skip_mode_none == mode_none)
> 			continue;
>-		if (!hantro_check_depth_match(fmt, ctx->bit_depth))
>+		if (!hantro_check_depth_match(fmt, ctx->bit_depth) && !enum_all_formats)
> 			continue;
>-		if (j == f->index) {
>+		if (j == index) {
> 			f->pixelformat = fmt->fourcc;
> 			return 0;
> 		}
>@@ -242,9 +249,9 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
> 	for (i = 0; i < num_fmts; i++) {
> 		fmt = &formats[i];
>
>-		if (!hantro_check_depth_match(fmt, ctx->bit_depth))
>+		if (!hantro_check_depth_match(fmt, ctx->bit_depth) && !enum_all_formats)
> 			continue;
>-		if (j == f->index) {
>+		if (j == index) {
> 			f->pixelformat = fmt->fourcc;
> 			return 0;
> 		}
>-- 
>2.43.0
>
>_______________________________________________
>Kernel mailing list -- kernel@mailman.collabora.com
>To unsubscribe send an email to kernel-leave@mailman.collabora.com
>This list is managed by https://mailman.collabora.com

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip