[PATCH v4 0/2] Enumerate all pixels formats

[PATCH v4 0/2] Enumerate all pixels formats

[Benjamin Gaignard]

The goal of this series is to let userland applications enumerate
all the supported pixels formats of a stateless decoder without
setting all the possible codec-dependent control.
That offer a simplest solution for applications to discover
supported pixels formats and possibly let them doing smarter
choice between stateless decoders.

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

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 (2):
  media: videodev2: Add flags to unconditionnaly enumerate pixels
    formats
  media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag

 .../media/v4l/dev-stateless-decoder.rst          |  6 ++++++
 .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 11 +++++++++++
 .../media/videodev2.h.rst.exceptions             |  2 ++
 drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c             |  3 +++
 include/uapi/linux/videodev2.h                   |  2 ++
 6 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.43.0


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

[PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
ignore the configuration and return the hardware supported pixel
formats for the specified queue.
To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
flag must be set by the drivers to highlight support of this feature
to user space applications.
This will permit to discover which pixel formats are supported
without setting codec-specific information so userland can more easily
know if the driver suits its needs well.
The main target are stateless decoders so update the documentation
about how to use this flag.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
changes in version 4:
- Explicitly document that the new flags are targeting mem2mem devices.

 .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
 .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
 .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
 drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
 include/uapi/linux/videodev2.h                        |  2 ++
 5 files changed, 24 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
index 35ed05f2695e..b0b657de910d 100644
--- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
+++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
@@ -58,6 +58,12 @@ Querying capabilities
      default values for these controls being used, and a returned set of formats
      that may not be usable for the media the client is trying to decode.
 
+   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
+     all the supported formats without taking care of codec-dependent controls
+     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
+     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
+     enumerating.
+
 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
    resolutions for a given format, passing desired pixel format in
    :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
index 3adb3d205531..15bc2f59c05a 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
@@ -234,6 +234,17 @@ 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_FORMATS``
+      - 0x0400
+      - Set by userland applications to enumerate all possible pixel formats
+        without taking care of any OUTPUT or CAPTURE queue configuration.
+        This flag is relevant only for mem2mem devices.
+    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
+      - 0x0800
+      - Set by the driver to indicated that format have been enumerated because
+        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
+        been set by the userland application.
+        This flag is relevant only for mem2mem devices.
 
 Return Value
 ============
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index bdc628e8c1d6..7a3a1e9dc055 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
+replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
 
 # V4L2 timecode types
 replace define V4L2_TC_TYPE_24FPS timecode-type
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 4c76d17b4629..5785a98b6ba2 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
 	int ret = check_fmt(file, p->type);
 	u32 mbus_code;
 	u32 cap_mask;
+	u32 flags;
 
 	if (ret)
 		return ret;
@@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
 		p->mbus_code = 0;
 
 	mbus_code = p->mbus_code;
+	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
 	memset_after(p, 0, type);
 	p->mbus_code = mbus_code;
+	p->flags = flags;
 
 	switch (p->type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fe6b67e83751..b6a5da79ba21 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
 #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
 #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
 #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
+#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
+#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
 
 	/* Frame Size and frame rate enumeration */
 /*
-- 
2.43.0


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

Re: [PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Jacopo Mondi]

Hi Benjamin

On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote:
> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
> ignore the configuration and return the hardware supported pixel
> formats for the specified queue.
> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
> flag must be set by the drivers to highlight support of this feature
> to user space applications.
> This will permit to discover which pixel formats are supported
> without setting codec-specific information so userland can more easily
> know if the driver suits its needs well.
> The main target are stateless decoders so update the documentation
> about how to use this flag.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> changes in version 4:
> - Explicitly document that the new flags are targeting mem2mem devices.
>
>  .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
>  .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
>  include/uapi/linux/videodev2.h                        |  2 ++
>  5 files changed, 24 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> index 35ed05f2695e..b0b657de910d 100644
> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> @@ -58,6 +58,12 @@ Querying capabilities
>       default values for these controls being used, and a returned set of formats
>       that may not be usable for the media the client is trying to decode.
>
> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
> +     all the supported formats without taking care of codec-dependent controls
> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
> +     enumerating.
> +
>  3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>     resolutions for a given format, passing desired pixel format in
>     :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 3adb3d205531..15bc2f59c05a 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -234,6 +234,17 @@ 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_FORMATS``
> +      - 0x0400
> +      - Set by userland applications to enumerate all possible pixel formats
> +        without taking care of any OUTPUT or CAPTURE queue configuration.
> +        This flag is relevant only for mem2mem devices.
> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> +      - 0x0800
> +      - Set by the driver to indicated that format have been enumerated because
> +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
> +        been set by the userland application.
> +        This flag is relevant only for mem2mem devices.

Thanks, however I think this can be wrapper on the previous line

>
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index bdc628e8c1d6..7a3a1e9dc055 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>
>  # V4L2 timecode types
>  replace define V4L2_TC_TYPE_24FPS timecode-type
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 4c76d17b4629..5785a98b6ba2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  	int ret = check_fmt(file, p->type);
>  	u32 mbus_code;
>  	u32 cap_mask;
> +	u32 flags;
>
>  	if (ret)
>  		return ret;
> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  		p->mbus_code = 0;
>
>  	mbus_code = p->mbus_code;
> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>  	memset_after(p, 0, type);
>  	p->mbus_code = mbus_code;
> +	p->flags = flags;

Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
flags returned to userspace ? Shouldn't be drivers to set
V4L2_FMT_FLAG_ALL_FORMATS instead ?

>
>  	switch (p->type) {
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index fe6b67e83751..b6a5da79ba21 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>
>  	/* Frame Size and frame rate enumeration */
>  /*
> --
> 2.43.0
>
>

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

Re: [PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

Le 17/07/2024 à 15:20, Jacopo Mondi a écrit :
> Hi Benjamin
>
> On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote:
>> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
>> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
>> ignore the configuration and return the hardware supported pixel
>> formats for the specified queue.
>> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
>> flag must be set by the drivers to highlight support of this feature
>> to user space applications.
>> This will permit to discover which pixel formats are supported
>> without setting codec-specific information so userland can more easily
>> know if the driver suits its needs well.
>> The main target are stateless decoders so update the documentation
>> about how to use this flag.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> changes in version 4:
>> - Explicitly document that the new flags are targeting mem2mem devices.
>>
>>   .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
>>   .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
>>   .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
>>   drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
>>   include/uapi/linux/videodev2.h                        |  2 ++
>>   5 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>> index 35ed05f2695e..b0b657de910d 100644
>> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>> @@ -58,6 +58,12 @@ Querying capabilities
>>        default values for these controls being used, and a returned set of formats
>>        that may not be usable for the media the client is trying to decode.
>>
>> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
>> +     all the supported formats without taking care of codec-dependent controls
>> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
>> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
>> +     enumerating.
>> +
>>   3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>>      resolutions for a given format, passing desired pixel format in
>>      :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> index 3adb3d205531..15bc2f59c05a 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> @@ -234,6 +234,17 @@ 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_FORMATS``
>> +      - 0x0400
>> +      - Set by userland applications to enumerate all possible pixel formats
>> +        without taking care of any OUTPUT or CAPTURE queue configuration.
>> +        This flag is relevant only for mem2mem devices.
>> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
>> +      - 0x0800
>> +      - Set by the driver to indicated that format have been enumerated because
>> +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
>> +        been set by the userland application.
>> +        This flag is relevant only for mem2mem devices.
> Thanks, however I think this can be wrapper on the previous line

ok

>
>>   Return Value
>>   ============
>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> index bdc628e8c1d6..7a3a1e9dc055 100644
>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
>> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>>
>>   # V4L2 timecode types
>>   replace define V4L2_TC_TYPE_24FPS timecode-type
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 4c76d17b4629..5785a98b6ba2 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>   	int ret = check_fmt(file, p->type);
>>   	u32 mbus_code;
>>   	u32 cap_mask;
>> +	u32 flags;
>>
>>   	if (ret)
>>   		return ret;
>> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>   		p->mbus_code = 0;
>>
>>   	mbus_code = p->mbus_code;
>> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>>   	memset_after(p, 0, type);
>>   	p->mbus_code = mbus_code;
>> +	p->flags = flags;
> Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
> flags returned to userspace ? Shouldn't be drivers to set
> V4L2_FMT_FLAG_ALL_FORMATS instead ?

memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
flag to drivers. Return it to userspace is a side effect but I don't that is problem
since it set it anyway.

>
>>   	switch (p->type) {
>>   	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index fe6b67e83751..b6a5da79ba21 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>>   #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>>   #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>   #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
>> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>>
>>   	/* Frame Size and frame rate enumeration */
>>   /*
>> --
>> 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Jacopo Mondi]

Hi Benjamin

On Wed, Jul 17, 2024 at 03:44:24PM GMT, Benjamin Gaignard wrote:
>
> Le 17/07/2024 à 15:20, Jacopo Mondi a écrit :
> > Hi Benjamin
> >
> > On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote:
> > > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
> > > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
> > > ignore the configuration and return the hardware supported pixel
> > > formats for the specified queue.
> > > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
> > > flag must be set by the drivers to highlight support of this feature
> > > to user space applications.
> > > This will permit to discover which pixel formats are supported
> > > without setting codec-specific information so userland can more easily
> > > know if the driver suits its needs well.
> > > The main target are stateless decoders so update the documentation
> > > about how to use this flag.
> > >
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > ---
> > > changes in version 4:
> > > - Explicitly document that the new flags are targeting mem2mem devices.
> > >
> > >   .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
> > >   .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
> > >   .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
> > >   drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
> > >   include/uapi/linux/videodev2.h                        |  2 ++
> > >   5 files changed, 24 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > > index 35ed05f2695e..b0b657de910d 100644
> > > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > > @@ -58,6 +58,12 @@ Querying capabilities
> > >        default values for these controls being used, and a returned set of formats
> > >        that may not be usable for the media the client is trying to decode.
> > >
> > > +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
> > > +     all the supported formats without taking care of codec-dependent controls
> > > +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
> > > +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
> > > +     enumerating.
> > > +
> > >   3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
> > >      resolutions for a given format, passing desired pixel format in
> > >      :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
> > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > index 3adb3d205531..15bc2f59c05a 100644
> > > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > @@ -234,6 +234,17 @@ 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_FORMATS``
> > > +      - 0x0400
> > > +      - Set by userland applications to enumerate all possible pixel formats
> > > +        without taking care of any OUTPUT or CAPTURE queue configuration.
> > > +        This flag is relevant only for mem2mem devices.
> > > +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> > > +      - 0x0800
> > > +      - Set by the driver to indicated that format have been enumerated because
> > > +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
> > > +        been set by the userland application.
> > > +        This flag is relevant only for mem2mem devices.
> > Thanks, however I think this can be wrapper on the previous line
>
> ok
>
> >
> > >   Return Value
> > >   ============
> > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > index bdc628e8c1d6..7a3a1e9dc055 100644
> > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
> > > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
> > >
> > >   # V4L2 timecode types
> > >   replace define V4L2_TC_TYPE_24FPS timecode-type
> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > index 4c76d17b4629..5785a98b6ba2 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > >   	int ret = check_fmt(file, p->type);
> > >   	u32 mbus_code;
> > >   	u32 cap_mask;
> > > +	u32 flags;
> > >
> > >   	if (ret)
> > >   		return ret;
> > > @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > >   		p->mbus_code = 0;
> > >
> > >   	mbus_code = p->mbus_code;
> > > +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
> > >   	memset_after(p, 0, type);
> > >   	p->mbus_code = mbus_code;
> > > +	p->flags = flags;
> > Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
> > flags returned to userspace ? Shouldn't be drivers to set
> > V4L2_FMT_FLAG_ALL_FORMATS instead ?
>
> memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
> flag to drivers. Return it to userspace is a side effect but I don't that is problem
> since it set it anyway.
>

Ok, if the expectation is that the flag is preserved through the ioctl
call, this is fine with me

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> >
> > >   	switch (p->type) {
> > >   	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > index fe6b67e83751..b6a5da79ba21 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
> > >   #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> > >   #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> > >   #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> > > +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> > > +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
> > >
> > >   	/* Frame Size and frame rate enumeration */
> > >   /*
> > > --
> > > 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Nicolas Dufresne]

Hi

Le mercredi 17 juillet 2024 à 16:04 +0200, Jacopo Mondi a écrit :
> Hi Benjamin
> 
> On Wed, Jul 17, 2024 at 03:44:24PM GMT, Benjamin Gaignard wrote:
> > 
> > Le 17/07/2024 à 15:20, Jacopo Mondi a écrit :
> > > Hi Benjamin
> > > 
> > > On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote:
> > > > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
> > > > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
> > > > ignore the configuration and return the hardware supported pixel
> > > > formats for the specified queue.
> > > > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
> > > > flag must be set by the drivers to highlight support of this feature
> > > > to user space applications.
> > > > This will permit to discover which pixel formats are supported
> > > > without setting codec-specific information so userland can more easily
> > > > know if the driver suits its needs well.
> > > > The main target are stateless decoders so update the documentation
> > > > about how to use this flag.
> > > > 
> > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > > ---
> > > > changes in version 4:
> > > > - Explicitly document that the new flags are targeting mem2mem devices.
> > > > 
> > > >   .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
> > > >   .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
> > > >   .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
> > > >   drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
> > > >   include/uapi/linux/videodev2.h                        |  2 ++
> > > >   5 files changed, 24 insertions(+)
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > > > index 35ed05f2695e..b0b657de910d 100644
> > > > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > > > @@ -58,6 +58,12 @@ Querying capabilities
> > > >        default values for these controls being used, and a returned set of formats
> > > >        that may not be usable for the media the client is trying to decode.
> > > > 
> > > > +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
> > > > +     all the supported formats without taking care of codec-dependent controls
> > > > +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
> > > > +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
> > > > +     enumerating.
> > > > +
> > > >   3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
> > > >      resolutions for a given format, passing desired pixel format in
> > > >      :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
> > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > > index 3adb3d205531..15bc2f59c05a 100644
> > > > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > > @@ -234,6 +234,17 @@ 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_FORMATS``
> > > > +      - 0x0400
> > > > +      - Set by userland applications to enumerate all possible pixel formats
> > > > +        without taking care of any OUTPUT or CAPTURE queue configuration.
> > > > +        This flag is relevant only for mem2mem devices.
> > > > +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> > > > +      - 0x0800
> > > > +      - Set by the driver to indicated that format have been enumerated because
> > > > +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
> > > > +        been set by the userland application.
> > > > +        This flag is relevant only for mem2mem devices.
> > > Thanks, however I think this can be wrapper on the previous line
> > 
> > ok
> > 
> > > 
> > > >   Return Value
> > > >   ============
> > > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > > index bdc628e8c1d6..7a3a1e9dc055 100644
> > > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > > > @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
> > > > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
> > > > 
> > > >   # V4L2 timecode types
> > > >   replace define V4L2_TC_TYPE_24FPS timecode-type
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > index 4c76d17b4629..5785a98b6ba2 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > >   	int ret = check_fmt(file, p->type);
> > > >   	u32 mbus_code;
> > > >   	u32 cap_mask;
> > > > +	u32 flags;
> > > > 
> > > >   	if (ret)
> > > >   		return ret;
> > > > @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > >   		p->mbus_code = 0;
> > > > 
> > > >   	mbus_code = p->mbus_code;
> > > > +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
> > > >   	memset_after(p, 0, type);
> > > >   	p->mbus_code = mbus_code;
> > > > +	p->flags = flags;
> > > Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
> > > flags returned to userspace ? Shouldn't be drivers to set
> > > V4L2_FMT_FLAG_ALL_FORMATS instead ?
> > 
> > memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
> > flag to drivers. Return it to userspace is a side effect but I don't that is problem
> > since it set it anyway.
> > 
> 
> Ok, if the expectation is that the flag is preserved through the ioctl
> call, this is fine with me

I might be missing something other similar features are explicitly advertised by
drivers. This way, the generic layer can keep or clear the flag based of if its
supported. The fact the flag persist the ioctl() or not endup having a useful
semantic.

Could we do the same?

> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> > > 
> > > >   	switch (p->type) {
> > > >   	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > > index fe6b67e83751..b6a5da79ba21 100644
> > > > --- a/include/uapi/linux/videodev2.h
> > > > +++ b/include/uapi/linux/videodev2.h
> > > > @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
> > > >   #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> > > >   #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> > > >   #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> > > > +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> > > > +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
> > > > 
> > > >   	/* Frame Size and frame rate enumeration */
> > > >   /*
> > > > --
> > > > 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit :
> Hi
>
> Le mercredi 17 juillet 2024 à 16:04 +0200, Jacopo Mondi a écrit :
>> Hi Benjamin
>>
>> On Wed, Jul 17, 2024 at 03:44:24PM GMT, Benjamin Gaignard wrote:
>>> Le 17/07/2024 à 15:20, Jacopo Mondi a écrit :
>>>> Hi Benjamin
>>>>
>>>> On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote:
>>>>> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
>>>>> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
>>>>> ignore the configuration and return the hardware supported pixel
>>>>> formats for the specified queue.
>>>>> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
>>>>> flag must be set by the drivers to highlight support of this feature
>>>>> to user space applications.
>>>>> This will permit to discover which pixel formats are supported
>>>>> without setting codec-specific information so userland can more easily
>>>>> know if the driver suits its needs well.
>>>>> The main target are stateless decoders so update the documentation
>>>>> about how to use this flag.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>> ---
>>>>> changes in version 4:
>>>>> - Explicitly document that the new flags are targeting mem2mem devices.
>>>>>
>>>>>    .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
>>>>>    .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
>>>>>    .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
>>>>>    drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
>>>>>    include/uapi/linux/videodev2.h                        |  2 ++
>>>>>    5 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>>>>> index 35ed05f2695e..b0b657de910d 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>>>>> @@ -58,6 +58,12 @@ Querying capabilities
>>>>>         default values for these controls being used, and a returned set of formats
>>>>>         that may not be usable for the media the client is trying to decode.
>>>>>
>>>>> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
>>>>> +     all the supported formats without taking care of codec-dependent controls
>>>>> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
>>>>> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
>>>>> +     enumerating.
>>>>> +
>>>>>    3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>>>>>       resolutions for a given format, passing desired pixel format in
>>>>>       :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>>>> index 3adb3d205531..15bc2f59c05a 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>>>>> @@ -234,6 +234,17 @@ 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_FORMATS``
>>>>> +      - 0x0400
>>>>> +      - Set by userland applications to enumerate all possible pixel formats
>>>>> +        without taking care of any OUTPUT or CAPTURE queue configuration.
>>>>> +        This flag is relevant only for mem2mem devices.
>>>>> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
>>>>> +      - 0x0800
>>>>> +      - Set by the driver to indicated that format have been enumerated because
>>>>> +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
>>>>> +        been set by the userland application.
>>>>> +        This flag is relevant only for mem2mem devices.
>>>> Thanks, however I think this can be wrapper on the previous line
>>> ok
>>>
>>>>>    Return Value
>>>>>    ============
>>>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>>>> index bdc628e8c1d6..7a3a1e9dc055 100644
>>>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>>>> @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
>>>>> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>>>>>
>>>>>    # V4L2 timecode types
>>>>>    replace define V4L2_TC_TYPE_24FPS timecode-type
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> index 4c76d17b4629..5785a98b6ba2 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>>>>    	int ret = check_fmt(file, p->type);
>>>>>    	u32 mbus_code;
>>>>>    	u32 cap_mask;
>>>>> +	u32 flags;
>>>>>
>>>>>    	if (ret)
>>>>>    		return ret;
>>>>> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>>>>    		p->mbus_code = 0;
>>>>>
>>>>>    	mbus_code = p->mbus_code;
>>>>> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>>>>>    	memset_after(p, 0, type);
>>>>>    	p->mbus_code = mbus_code;
>>>>> +	p->flags = flags;
>>>> Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
>>>> flags returned to userspace ? Shouldn't be drivers to set
>>>> V4L2_FMT_FLAG_ALL_FORMATS instead ?
>>> memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
>>> flag to drivers. Return it to userspace is a side effect but I don't that is problem
>>> since it set it anyway.
>>>
>> Ok, if the expectation is that the flag is preserved through the ioctl
>> call, this is fine with me
> I might be missing something other similar features are explicitly advertised by
> drivers. This way, the generic layer can keep or clear the flag based of if its
> supported. The fact the flag persist the ioctl() or not endup having a useful
> semantic.
>
> Could we do the same?

It is the only flag set by userspace when calling the ioctl(), all others
are set by the drivers.
I can clean it from the ioctl() structure after driver call but that won't change anything.

>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>
>>>>>    	switch (p->type) {
>>>>>    	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>> index fe6b67e83751..b6a5da79ba21 100644
>>>>> --- a/include/uapi/linux/videodev2.h
>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>>>>>    #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>>>>>    #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>>>>    #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>>>>> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
>>>>> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>>>>>
>>>>>    	/* Frame Size and frame rate enumeration */
>>>>>    /*
>>>>> --
>>>>> 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Nicolas Dufresne]

Hi,

Le jeudi 18 juillet 2024 à 09:04 +0200, Benjamin Gaignard a écrit :
> Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit :
> > 

[...]

> > > > > > @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > > > >    	int ret = check_fmt(file, p->type);
> > > > > >    	u32 mbus_code;
> > > > > >    	u32 cap_mask;
> > > > > > +	u32 flags;
> > > > > > 
> > > > > >    	if (ret)
> > > > > >    		return ret;
> > > > > > @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > > > >    		p->mbus_code = 0;
> > > > > > 
> > > > > >    	mbus_code = p->mbus_code;
> > > > > > +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
> > > > > >    	memset_after(p, 0, type);
> > > > > >    	p->mbus_code = mbus_code;
> > > > > > +	p->flags = flags;
> > > > > Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
> > > > > flags returned to userspace ? Shouldn't be drivers to set
> > > > > V4L2_FMT_FLAG_ALL_FORMATS instead ?
> > > > memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
> > > > flag to drivers. Return it to userspace is a side effect but I don't that is problem
> > > > since it set it anyway.
> > > > 
> > > Ok, if the expectation is that the flag is preserved through the ioctl
> > > call, this is fine with me
> > I might be missing something other similar features are explicitly advertised by
> > drivers. This way, the generic layer can keep or clear the flag based of if its
> > supported. The fact the flag persist the ioctl() or not endup having a useful
> > semantic.
> > 
> > Could we do the same?
> 
> It is the only flag set by userspace when calling the ioctl(), all others
> are set by the drivers.
> I can clean it from the ioctl() structure after driver call but that won't change anything.

This does not answer my question. In other similar feature, we have an
**internal** flag set by drivers to tell the framework that such feature is
abled. Using that, the framework can keep or remove that flag based on if its
supported or not. This way, userspace have a clue if the driver do have this
support or if the returned result (in that case) is just a subset matching the
configuration. We don't seem to have done the same level of effort here.

Nicolas

> 
> > 
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > 
> > > > > >    	switch (p->type) {
> > > > > >    	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > > > > index fe6b67e83751..b6a5da79ba21 100644
> > > > > > --- a/include/uapi/linux/videodev2.h
> > > > > > +++ b/include/uapi/linux/videodev2.h
> > > > > > @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
> > > > > >    #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> > > > > >    #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> > > > > >    #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> > > > > > +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> > > > > > +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
> > > > > > 
> > > > > >    	/* Frame Size and frame rate enumeration */
> > > > > >    /*
> > > > > > --
> > > > > > 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
> > 
> _______________________________________________
> 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Nicolas Dufresne]

Le jeudi 18 juillet 2024 à 09:56 -0400, Nicolas Dufresne a écrit :
> Hi,
> 
> Le jeudi 18 juillet 2024 à 09:04 +0200, Benjamin Gaignard a écrit :
> > Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit :
> > > 
> 
> [...]
> 
> > > > > > > @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > > > > >    	int ret = check_fmt(file, p->type);
> > > > > > >    	u32 mbus_code;
> > > > > > >    	u32 cap_mask;
> > > > > > > +	u32 flags;
> > > > > > > 
> > > > > > >    	if (ret)
> > > > > > >    		return ret;
> > > > > > > @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > > > > >    		p->mbus_code = 0;
> > > > > > > 
> > > > > > >    	mbus_code = p->mbus_code;
> > > > > > > +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
> > > > > > >    	memset_after(p, 0, type);
> > > > > > >    	p->mbus_code = mbus_code;
> > > > > > > +	p->flags = flags;
> > > > > > Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
> > > > > > flags returned to userspace ? Shouldn't be drivers to set
> > > > > > V4L2_FMT_FLAG_ALL_FORMATS instead ?
> > > > > memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
> > > > > flag to drivers. Return it to userspace is a side effect but I don't that is problem
> > > > > since it set it anyway.
> > > > > 
> > > > Ok, if the expectation is that the flag is preserved through the ioctl
> > > > call, this is fine with me
> > > I might be missing something other similar features are explicitly advertised by
> > > drivers. This way, the generic layer can keep or clear the flag based of if its
> > > supported. The fact the flag persist the ioctl() or not endup having a useful
> > > semantic.
> > > 
> > > Could we do the same?
> > 
> > It is the only flag set by userspace when calling the ioctl(), all others
> > are set by the drivers.
> > I can clean it from the ioctl() structure after driver call but that won't change anything.
> 
> This does not answer my question. In other similar feature, we have an
> **internal** flag set by drivers to tell the framework that such feature is
> abled. Using that, the framework can keep or remove that flag based on if its
> supported or not. This way, userspace have a clue if the driver do have this
> support or if the returned result (in that case) is just a subset matching the
> configuration. We don't seem to have done the same level of effort here.

For the reference, you actually use that semantic in GStreamer implementation,
but the kernel code seems broken.

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7078/diffs#eb90d5495df2f085f163996014c748a36f143f76_516_527

> 
> Nicolas
> 
> > 
> > > 
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > 
> > > > > > >    	switch (p->type) {
> > > > > > >    	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > > > > > index fe6b67e83751..b6a5da79ba21 100644
> > > > > > > --- a/include/uapi/linux/videodev2.h
> > > > > > > +++ b/include/uapi/linux/videodev2.h
> > > > > > > @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
> > > > > > >    #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> > > > > > >    #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> > > > > > >    #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> > > > > > > +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> > > > > > > +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
> > > > > > > 
> > > > > > >    	/* Frame Size and frame rate enumeration */
> > > > > > >    /*
> > > > > > > --
> > > > > > > 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
> > > 
> > _______________________________________________
> > 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
> 
> _______________________________________________
> 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Benjamin Gaignard]

Le 18/07/2024 à 16:02, Nicolas Dufresne a écrit :
> Le jeudi 18 juillet 2024 à 09:56 -0400, Nicolas Dufresne a écrit :
>> Hi,
>>
>> Le jeudi 18 juillet 2024 à 09:04 +0200, Benjamin Gaignard a écrit :
>>> Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit :
>> [...]
>>
>>>>>>>> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>>>>>>>     	int ret = check_fmt(file, p->type);
>>>>>>>>     	u32 mbus_code;
>>>>>>>>     	u32 cap_mask;
>>>>>>>> +	u32 flags;
>>>>>>>>
>>>>>>>>     	if (ret)
>>>>>>>>     		return ret;
>>>>>>>> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>>>>>>>     		p->mbus_code = 0;
>>>>>>>>
>>>>>>>>     	mbus_code = p->mbus_code;
>>>>>>>> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>>>>>>>>     	memset_after(p, 0, type);
>>>>>>>>     	p->mbus_code = mbus_code;
>>>>>>>> +	p->flags = flags;
>>>>>>> Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
>>>>>>> flags returned to userspace ? Shouldn't be drivers to set
>>>>>>> V4L2_FMT_FLAG_ALL_FORMATS instead ?
>>>>>> memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
>>>>>> flag to drivers. Return it to userspace is a side effect but I don't that is problem
>>>>>> since it set it anyway.
>>>>>>
>>>>> Ok, if the expectation is that the flag is preserved through the ioctl
>>>>> call, this is fine with me
>>>> I might be missing something other similar features are explicitly advertised by
>>>> drivers. This way, the generic layer can keep or clear the flag based of if its
>>>> supported. The fact the flag persist the ioctl() or not endup having a useful
>>>> semantic.
>>>>
>>>> Could we do the same?
>>> It is the only flag set by userspace when calling the ioctl(), all others
>>> are set by the drivers.
>>> I can clean it from the ioctl() structure after driver call but that won't change anything.
>> This does not answer my question. In other similar feature, we have an
>> **internal** flag set by drivers to tell the framework that such feature is
>> abled. Using that, the framework can keep or remove that flag based on if its
>> supported or not. This way, userspace have a clue if the driver do have this
>> support or if the returned result (in that case) is just a subset matching the
>> configuration. We don't seem to have done the same level of effort here.
> For the reference, you actually use that semantic in GStreamer implementation,
> but the kernel code seems broken.
>
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7078/diffs#eb90d5495df2f085f163996014c748a36f143f76_516_527

device_caps u32 field is already almost fully used, only one 1 bit is free.
I could use it but, for me, the capability to enumerate all the formats
doesn't fit well in the existing list.

Benjamin

>
>> Nicolas
>>
>>>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>>
>>>>>>>>     	switch (p->type) {
>>>>>>>>     	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>>>>> index fe6b67e83751..b6a5da79ba21 100644
>>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>>> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>>>>>>>>     #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>>>>>>>>     #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>>>>>>>     #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>>>>>>>> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
>>>>>>>> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>>>>>>>>
>>>>>>>>     	/* Frame Size and frame rate enumeration */
>>>>>>>>     /*
>>>>>>>> --
>>>>>>>> 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
>>> _______________________________________________
>>> 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
>> _______________________________________________
>> 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Nicolas Dufresne]

Le jeudi 18 juillet 2024 à 16:43 +0200, Benjamin Gaignard a écrit :
> Le 18/07/2024 à 16:02, Nicolas Dufresne a écrit :
> > Le jeudi 18 juillet 2024 à 09:56 -0400, Nicolas Dufresne a écrit :
> > > Hi,
> > > 
> > > Le jeudi 18 juillet 2024 à 09:04 +0200, Benjamin Gaignard a écrit :
> > > > Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit :
> > > [...]
> > > 
> > > > > > > > > @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > > > > > > >     	int ret = check_fmt(file, p->type);
> > > > > > > > >     	u32 mbus_code;
> > > > > > > > >     	u32 cap_mask;
> > > > > > > > > +	u32 flags;
> > > > > > > > > 
> > > > > > > > >     	if (ret)
> > > > > > > > >     		return ret;
> > > > > > > > > @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > > > > > > > >     		p->mbus_code = 0;
> > > > > > > > > 
> > > > > > > > >     	mbus_code = p->mbus_code;
> > > > > > > > > +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
> > > > > > > > >     	memset_after(p, 0, type);
> > > > > > > > >     	p->mbus_code = mbus_code;
> > > > > > > > > +	p->flags = flags;
> > > > > > > > Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
> > > > > > > > flags returned to userspace ? Shouldn't be drivers to set
> > > > > > > > V4L2_FMT_FLAG_ALL_FORMATS instead ?
> > > > > > > memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
> > > > > > > flag to drivers. Return it to userspace is a side effect but I don't that is problem
> > > > > > > since it set it anyway.
> > > > > > > 
> > > > > > Ok, if the expectation is that the flag is preserved through the ioctl
> > > > > > call, this is fine with me
> > > > > I might be missing something other similar features are explicitly advertised by
> > > > > drivers. This way, the generic layer can keep or clear the flag based of if its
> > > > > supported. The fact the flag persist the ioctl() or not endup having a useful
> > > > > semantic.
> > > > > 
> > > > > Could we do the same?
> > > > It is the only flag set by userspace when calling the ioctl(), all others
> > > > are set by the drivers.
> > > > I can clean it from the ioctl() structure after driver call but that won't change anything.
> > > This does not answer my question. In other similar feature, we have an
> > > **internal** flag set by drivers to tell the framework that such feature is
> > > abled. Using that, the framework can keep or remove that flag based on if its
> > > supported or not. This way, userspace have a clue if the driver do have this
> > > support or if the returned result (in that case) is just a subset matching the
> > > configuration. We don't seem to have done the same level of effort here.
> > For the reference, you actually use that semantic in GStreamer implementation,
> > but the kernel code seems broken.
> > 
> > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7078/diffs#eb90d5495df2f085f163996014c748a36f143f76_516_527
> 
> device_caps u32 field is already almost fully used, only one 1 bit is free.
> I could use it but, for me, the capability to enumerate all the formats
> doesn't fit well in the existing list.

Sorry, but I will re-iterate that I don't mean to add a uAPI but an **internal**
flag between driver and framework. This way the framework knows at least.

> 
> Benjamin
> 
> > 
> > > Nicolas
> > > 
> > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > 
> > > > > > > > >     	switch (p->type) {
> > > > > > > > >     	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > > > > > > > index fe6b67e83751..b6a5da79ba21 100644
> > > > > > > > > --- a/include/uapi/linux/videodev2.h
> > > > > > > > > +++ b/include/uapi/linux/videodev2.h
> > > > > > > > > @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
> > > > > > > > >     #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> > > > > > > > >     #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> > > > > > > > >     #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> > > > > > > > > +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> > > > > > > > > +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
> > > > > > > > > 
> > > > > > > > >     	/* Frame Size and frame rate enumeration */
> > > > > > > > >     /*
> > > > > > > > > --
> > > > > > > > > 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
> > > > _______________________________________________
> > > > 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
> > > _______________________________________________
> > > 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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Sebastian Fricke]

Hey Benjamin,

typos in the subject line:

s/unconditionnaly/unconditionally/
s/pixels/pixel/

On 17.07.2024 15:14, Benjamin Gaignard wrote:
>Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.

s/pixels/pixel/

>When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
>ignore the configuration and return the hardware supported pixel
>formats for the specified queue.
>To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
>flag must be set by the drivers to highlight support of this feature
>to user space applications.
>This will permit to discover which pixel formats are supported
>without setting codec-specific information so userland can more easily
>know if the driver suits its needs well.
>The main target are stateless decoders so update the documentation
>about how to use this flag.
>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
>changes in version 4:
>- Explicitly document that the new flags are targeting mem2mem devices.
>
> .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
> .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
> .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
> drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
> include/uapi/linux/videodev2.h                        |  2 ++
> 5 files changed, 24 insertions(+)
>
>diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>index 35ed05f2695e..b0b657de910d 100644
>--- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>+++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>@@ -58,6 +58,12 @@ Querying capabilities
>      default values for these controls being used, and a returned set of formats
>      that may not be usable for the media the client is trying to decode.
>
>+   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
>+     all the supported formats without taking care of codec-dependent controls
>+     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
>+     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
>+     enumerating.
>+
> 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>    resolutions for a given format, passing desired pixel format in
>    :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
>diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>index 3adb3d205531..15bc2f59c05a 100644
>--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>@@ -234,6 +234,17 @@ 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_FORMATS``
>+      - 0x0400
>+      - Set by userland applications to enumerate all possible pixel formats
>+        without taking care of any OUTPUT or CAPTURE queue configuration.
>+        This flag is relevant only for mem2mem devices.
>+    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
>+      - 0x0800
>+      - Set by the driver to indicated that format have been enumerated because

s/indicated/indicate/

Also, either: "..that a format has been.." or "..that formats have been.."
but not "..that format have been.."

Regards,
Sebastian

>+        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
>+        been set by the userland application.
>+        This flag is relevant only for mem2mem devices.
>
> Return Value
> ============
>diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>index bdc628e8c1d6..7a3a1e9dc055 100644
>--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>@@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
>+replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>
> # V4L2 timecode types
> replace define V4L2_TC_TYPE_24FPS timecode-type
>diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>index 4c76d17b4629..5785a98b6ba2 100644
>--- a/drivers/media/v4l2-core/v4l2-ioctl.c
>+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>@@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> 	int ret = check_fmt(file, p->type);
> 	u32 mbus_code;
> 	u32 cap_mask;
>+	u32 flags;
>
> 	if (ret)
> 		return ret;
>@@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> 		p->mbus_code = 0;
>
> 	mbus_code = p->mbus_code;
>+	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
> 	memset_after(p, 0, type);
> 	p->mbus_code = mbus_code;
>+	p->flags = flags;
>
> 	switch (p->type) {
> 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>index fe6b67e83751..b6a5da79ba21 100644
>--- a/include/uapi/linux/videodev2.h
>+++ b/include/uapi/linux/videodev2.h
>@@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
> #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
> #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>+#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
>+#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>
> 	/* Frame Size and frame rate enumeration */
> /*
>-- 
>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 v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Hans Verkuil]

On 7/17/24 15:14, Benjamin Gaignard wrote:
> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
> ignore the configuration and return the hardware supported pixel
> formats for the specified queue.
> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
> flag must be set by the drivers to highlight support of this feature
> to user space applications.
> This will permit to discover which pixel formats are supported
> without setting codec-specific information so userland can more easily
> know if the driver suits its needs well.
> The main target are stateless decoders so update the documentation
> about how to use this flag.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> changes in version 4:
> - Explicitly document that the new flags are targeting mem2mem devices.
> 
>  .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
>  .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
>  include/uapi/linux/videodev2.h                        |  2 ++
>  5 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> index 35ed05f2695e..b0b657de910d 100644
> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> @@ -58,6 +58,12 @@ Querying capabilities
>       default values for these controls being used, and a returned set of formats
>       that may not be usable for the media the client is trying to decode.
>  
> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
> +     all the supported formats without taking care of codec-dependent controls
> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
> +     enumerating.
> +
>  3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>     resolutions for a given format, passing desired pixel format in
>     :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 3adb3d205531..15bc2f59c05a 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -234,6 +234,17 @@ 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_FORMATS``
> +      - 0x0400
> +      - Set by userland applications to enumerate all possible pixel formats
> +        without taking care of any OUTPUT or CAPTURE queue configuration.
> +        This flag is relevant only for mem2mem devices.
> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> +      - 0x0800
> +      - Set by the driver to indicated that format have been enumerated because
> +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
> +        been set by the userland application.
> +        This flag is relevant only for mem2mem devices.
>  
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index bdc628e8c1d6..7a3a1e9dc055 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>  
>  # V4L2 timecode types
>  replace define V4L2_TC_TYPE_24FPS timecode-type
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 4c76d17b4629..5785a98b6ba2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  	int ret = check_fmt(file, p->type);
>  	u32 mbus_code;
>  	u32 cap_mask;
> +	u32 flags;
>  
>  	if (ret)
>  		return ret;
> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  		p->mbus_code = 0;
>  
>  	mbus_code = p->mbus_code;
> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;

Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Sorry, you can't just change a read-only field (i.e. set by the driver) to
something that can be set by the application as well. That can easily cause
problems: e.g. applications that leave the flags field uninitialized.

I will try to look at alternatives tomorrow, but this approach is definitely
a no-go.

Regards,

	Hans


>  	memset_after(p, 0, type);
>  	p->mbus_code = mbus_code;
> +	p->flags = flags;
>  
>  	switch (p->type) {
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index fe6b67e83751..b6a5da79ba21 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>  
>  	/* Frame Size and frame rate enumeration */
>  /*


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

Re: [PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Hans Verkuil]

On 17/07/2024 15:14, Benjamin Gaignard wrote:
> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
> ignore the configuration and return the hardware supported pixel
> formats for the specified queue.
> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
> flag must be set by the drivers to highlight support of this feature
> to user space applications.
> This will permit to discover which pixel formats are supported
> without setting codec-specific information so userland can more easily
> know if the driver suits its needs well.
> The main target are stateless decoders so update the documentation
> about how to use this flag.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> changes in version 4:
> - Explicitly document that the new flags are targeting mem2mem devices.
> 
>  .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
>  .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
>  include/uapi/linux/videodev2.h                        |  2 ++
>  5 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> index 35ed05f2695e..b0b657de910d 100644
> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> @@ -58,6 +58,12 @@ Querying capabilities
>       default values for these controls being used, and a returned set of formats
>       that may not be usable for the media the client is trying to decode.
>  
> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
> +     all the supported formats without taking care of codec-dependent controls
> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
> +     enumerating.
> +
>  3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>     resolutions for a given format, passing desired pixel format in
>     :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 3adb3d205531..15bc2f59c05a 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -234,6 +234,17 @@ 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_FORMATS``
> +      - 0x0400
> +      - Set by userland applications to enumerate all possible pixel formats
> +        without taking care of any OUTPUT or CAPTURE queue configuration.
> +        This flag is relevant only for mem2mem devices.
> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> +      - 0x0800
> +      - Set by the driver to indicated that format have been enumerated because
> +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
> +        been set by the userland application.
> +        This flag is relevant only for mem2mem devices.
>  
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index bdc628e8c1d6..7a3a1e9dc055 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>  
>  # V4L2 timecode types
>  replace define V4L2_TC_TYPE_24FPS timecode-type
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 4c76d17b4629..5785a98b6ba2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  	int ret = check_fmt(file, p->type);
>  	u32 mbus_code;
>  	u32 cap_mask;
> +	u32 flags;
>  
>  	if (ret)
>  		return ret;
> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  		p->mbus_code = 0;
>  
>  	mbus_code = p->mbus_code;
> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>  	memset_after(p, 0, type);
>  	p->mbus_code = mbus_code;
> +	p->flags = flags;
>  
>  	switch (p->type) {
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index fe6b67e83751..b6a5da79ba21 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800

For reasons mentioned earlier, you cannot make the flags field an input to the
driver, that will almost certainly break some applications that do not zero
the flags field today, since they expect it to be set by the driver.

What probably will work is to add a flag to the index field: this would be
similar to what I do in VIDIOC_QUERYCTRL where you can OR the id field with
V4L2_CTRL_FLAG_NEXT_CTRL to modify the behavior.

It is perfectly fine to use the top bits of the index for this.

Drivers that do not support this will just fail with EINVAL, and drivers that
do support this will return a proper format.

Applications can easily test support for this by just calling ENUM_FMT with 0
ORed with the new flag: if it is supported, then it will return 0, otherwise
-EINVAL.

With this scheme I think you can also drop the proposed V4L2_FMT_FLAG_ALL_FORMATS
flag.

But as I mentioned in my reply to the cover letter of this series, I am not
convinced we really need this, and if we do, then I am not convinced about
the name of the flag.

And I would also like to know if such a feature can be used by m2m drivers
that are not codecs (e.g. scalers, csc converters), and if that would impact
the name/description of the flag.

It is currently very specific for decoders, but I prefer to see a more general
solution, not just for one specific corner case.

Regards,

	Hans

>  
>  	/* Frame Size and frame rate enumeration */
>  /*


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

Re: [PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Hans Verkuil]

On 19/07/2024 15:15, Hans Verkuil wrote:
> On 17/07/2024 15:14, Benjamin Gaignard wrote:
>> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl.
>> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must
>> ignore the configuration and return the hardware supported pixel
>> formats for the specified queue.
>> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS
>> flag must be set by the drivers to highlight support of this feature
>> to user space applications.
>> This will permit to discover which pixel formats are supported
>> without setting codec-specific information so userland can more easily
>> know if the driver suits its needs well.
>> The main target are stateless decoders so update the documentation
>> about how to use this flag.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> changes in version 4:
>> - Explicitly document that the new flags are targeting mem2mem devices.
>>
>>  .../userspace-api/media/v4l/dev-stateless-decoder.rst |  6 ++++++
>>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst       | 11 +++++++++++
>>  .../userspace-api/media/videodev2.h.rst.exceptions    |  2 ++
>>  drivers/media/v4l2-core/v4l2-ioctl.c                  |  3 +++
>>  include/uapi/linux/videodev2.h                        |  2 ++
>>  5 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>> index 35ed05f2695e..b0b657de910d 100644
>> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
>> @@ -58,6 +58,12 @@ Querying capabilities
>>       default values for these controls being used, and a returned set of formats
>>       that may not be usable for the media the client is trying to decode.
>>  
>> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
>> +     all the supported formats without taking care of codec-dependent controls
>> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
>> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
>> +     enumerating.
>> +
>>  3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported
>>     resolutions for a given format, passing desired pixel format in
>>     :c:type:`v4l2_frmsizeenum`'s ``pixel_format``.
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> index 3adb3d205531..15bc2f59c05a 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
>> @@ -234,6 +234,17 @@ 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_FORMATS``
>> +      - 0x0400
>> +      - Set by userland applications to enumerate all possible pixel formats
>> +        without taking care of any OUTPUT or CAPTURE queue configuration.
>> +        This flag is relevant only for mem2mem devices.
>> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
>> +      - 0x0800
>> +      - Set by the driver to indicated that format have been enumerated because
>> +        :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has
>> +        been set by the userland application.
>> +        This flag is relevant only for mem2mem devices.
>>  
>>  Return Value
>>  ============
>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> index bdc628e8c1d6..7a3a1e9dc055 100644
>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>> @@ -216,6 +216,8 @@ 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_FORMATS fmtdesc-flags
>> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags
>>  
>>  # V4L2 timecode types
>>  replace define V4L2_TC_TYPE_24FPS timecode-type
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 4c76d17b4629..5785a98b6ba2 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>  	int ret = check_fmt(file, p->type);
>>  	u32 mbus_code;
>>  	u32 cap_mask;
>> +	u32 flags;
>>  
>>  	if (ret)
>>  		return ret;
>> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>  		p->mbus_code = 0;
>>  
>>  	mbus_code = p->mbus_code;
>> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>>  	memset_after(p, 0, type);
>>  	p->mbus_code = mbus_code;
>> +	p->flags = flags;
>>  
>>  	switch (p->type) {
>>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index fe6b67e83751..b6a5da79ba21 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>>  #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
>> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
> 
> For reasons mentioned earlier, you cannot make the flags field an input to the
> driver, that will almost certainly break some applications that do not zero
> the flags field today, since they expect it to be set by the driver.
> 
> What probably will work is to add a flag to the index field: this would be
> similar to what I do in VIDIOC_QUERYCTRL where you can OR the id field with
> V4L2_CTRL_FLAG_NEXT_CTRL to modify the behavior.
> 
> It is perfectly fine to use the top bits of the index for this.
> 
> Drivers that do not support this will just fail with EINVAL, and drivers that
> do support this will return a proper format.
> 
> Applications can easily test support for this by just calling ENUM_FMT with 0
> ORed with the new flag: if it is supported, then it will return 0, otherwise
> -EINVAL.
> 
> With this scheme I think you can also drop the proposed V4L2_FMT_FLAG_ALL_FORMATS
> flag.

Please note that this is an addition to the uAPI, so this also implies that
there should be patches for v4l-utils (v4l2-ctl and v4l2-compliance) and
ideally patches for one or more of the media test-drivers to implement this
flag. Together with v4l2-compliance that will ensure that the new flag is
actually tested by the regression tests.

Regards,

	Hans

> 
> But as I mentioned in my reply to the cover letter of this series, I am not
> convinced we really need this, and if we do, then I am not convinced about
> the name of the flag.
> 
> And I would also like to know if such a feature can be used by m2m drivers
> that are not codecs (e.g. scalers, csc converters), and if that would impact
> the name/description of the flag.
> 
> It is currently very specific for decoders, but I prefer to see a more general
> solution, not just for one specific corner case.
> 
> Regards,
> 
> 	Hans
> 
>>  
>>  	/* Frame Size and frame rate enumeration */
>>  /*
> 
> 


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

[PATCH v4 2/2] media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag

[Benjamin Gaignard]

If V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag has been set when calling
VIDIOC_ENUM_FMT ignore depth match and returns all the
hardware supported pixels formats. In this case all set
V4L2_FMT_FLAG_ALL_FORMATS when returning the pixels formats.

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

diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index df6f2536263b..b995ad31cddc 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -201,7 +201,13 @@ 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, ignore_depth_match;
+
+	/*
+	 * If V4L2_FMT_FLAG_ALL_FORMATS flag is set, we want to enumerate all
+	 * hardware supported pixels formats
+	 */
+	ignore_depth_match = !!(f->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS);
 
 	/*
 	 * When dealing with an encoder:
@@ -222,10 +228,12 @@ 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) && !ignore_depth_match)
 			continue;
 		if (j == f->index) {
 			f->pixelformat = fmt->fourcc;
+			if (ignore_depth_match)
+				f->flags |= V4L2_FMT_FLAG_ALL_FORMATS;
 			return 0;
 		}
 		++j;
@@ -242,10 +250,12 @@ 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) && !ignore_depth_match)
 			continue;
 		if (j == f->index) {
 			f->pixelformat = fmt->fourcc;
+			if (ignore_depth_match)
+				f->flags |= V4L2_FMT_FLAG_ALL_FORMATS;
 			return 0;
 		}
 		++j;
-- 
2.43.0


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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Hans Verkuil]

On 17/07/2024 15:14, Benjamin Gaignard wrote:
> The goal of this series is to let userland applications enumerate
> all the supported pixels formats of a stateless decoder without
> setting all the possible codec-dependent control.
> That offer a simplest solution for applications to discover
> supported pixels formats and possibly let them doing smarter
> choice between stateless decoders.
> 
> An example of how it can be used in GStreamer to discover the
> supported pixels formats for stateless decoder is available here:
> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/v4l2codecs_enum_all_supported_formats?ref_type=heads

So effectively specifying this flag makes ENUM_FMT also return
formats that do not match the bit depth.

So the AV1 (for example) compressed video uses e.g. 8 bit depth, but instead of just
listing only 8 bit uncompressed pixelformats, you want to list them for any
bit depth.

But what is the point of that if the decoder can't decode 8 bit compressed to,
say, 10 bit uncompressed video?

I actually thought that this flag would just list all formats, independent
of the output format (e.g. AV1, H264, etc.), but that does not appear to be
the case? I.e., if capture pixelformat X is only available with AV1, will that still
be listed if the output pixel is set to H264?

I think you need to describe a real use-case here, and I am not convinced about
the name of the flag either.

> 
> 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 (2):
>   media: videodev2: Add flags to unconditionnaly enumerate pixels
>     formats

I.e.: it is not unconditionally, it still depends on the chosen codec.

Regards,

	Hans

>   media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag
> 
>  .../media/v4l/dev-stateless-decoder.rst          |  6 ++++++
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 11 +++++++++++
>  .../media/videodev2.h.rst.exceptions             |  2 ++
>  drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++---
>  drivers/media/v4l2-core/v4l2-ioctl.c             |  3 +++
>  include/uapi/linux/videodev2.h                   |  2 ++
>  6 files changed, 37 insertions(+), 3 deletions(-)
> 


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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Benjamin Gaignard]

Le 19/07/2024 à 14:57, Hans Verkuil a écrit :
> On 17/07/2024 15:14, Benjamin Gaignard wrote:
>> The goal of this series is to let userland applications enumerate
>> all the supported pixels formats of a stateless decoder without
>> setting all the possible codec-dependent control.
>> That offer a simplest solution for applications to discover
>> supported pixels formats and possibly let them doing smarter
>> choice between stateless decoders.
>>
>> An example of how it can be used in GStreamer to discover the
>> supported pixels formats for stateless decoder is available here:
>> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/v4l2codecs_enum_all_supported_formats?ref_type=heads
> So effectively specifying this flag makes ENUM_FMT also return
> formats that do not match the bit depth.
>
> So the AV1 (for example) compressed video uses e.g. 8 bit depth, but instead of just
> listing only 8 bit uncompressed pixelformats, you want to list them for any
> bit depth.
>
> But what is the point of that if the decoder can't decode 8 bit compressed to,
> say, 10 bit uncompressed video?

No decoder will do 8 bits to 10 bits (as far I knows).
The point is to be able to say that decoder could produce 10 bit frames without
setting a full sps/pps for each case (and for each supported codec).

>
> I actually thought that this flag would just list all formats, independent
> of the output format (e.g. AV1, H264, etc.), but that does not appear to be
> the case? I.e., if capture pixelformat X is only available with AV1, will that still
> be listed if the output pixel is set to H264?
>
> I think you need to describe a real use-case here, and I am not convinced about
> the name of the flag either.

I may have miss something but yes the goal is to list all formats independently
of the output format.
When a SoC have multiple decoders for the same codec, knowing the supported formats
is key to select the better one.
Since I will have to do more iteration, feel free to provide a better name for the
flag(s). I'm always bad for naming this kind of thing.


>
>> 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 (2):
>>    media: videodev2: Add flags to unconditionnaly enumerate pixels
>>      formats
> I.e.: it is not unconditionally, it still depends on the chosen codec.
>
> Regards,
>
> 	Hans
>
>>    media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag
>>
>>   .../media/v4l/dev-stateless-decoder.rst          |  6 ++++++
>>   .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 11 +++++++++++
>>   .../media/videodev2.h.rst.exceptions             |  2 ++
>>   drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++---
>>   drivers/media/v4l2-core/v4l2-ioctl.c             |  3 +++
>>   include/uapi/linux/videodev2.h                   |  2 ++
>>   6 files changed, 37 insertions(+), 3 deletions(-)
>>
>

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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Hans Verkuil]

On 19/07/2024 15:15, Benjamin Gaignard wrote:
> 
> Le 19/07/2024 à 14:57, Hans Verkuil a écrit :
>> On 17/07/2024 15:14, Benjamin Gaignard wrote:
>>> The goal of this series is to let userland applications enumerate
>>> all the supported pixels formats of a stateless decoder without
>>> setting all the possible codec-dependent control.
>>> That offer a simplest solution for applications to discover
>>> supported pixels formats and possibly let them doing smarter
>>> choice between stateless decoders.
>>>
>>> An example of how it can be used in GStreamer to discover the
>>> supported pixels formats for stateless decoder is available here:
>>> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/v4l2codecs_enum_all_supported_formats?ref_type=heads
>> So effectively specifying this flag makes ENUM_FMT also return
>> formats that do not match the bit depth.
>>
>> So the AV1 (for example) compressed video uses e.g. 8 bit depth, but instead of just
>> listing only 8 bit uncompressed pixelformats, you want to list them for any
>> bit depth.
>>
>> But what is the point of that if the decoder can't decode 8 bit compressed to,
>> say, 10 bit uncompressed video?
> 
> No decoder will do 8 bits to 10 bits (as far I knows).
> The point is to be able to say that decoder could produce 10 bit frames without
> setting a full sps/pps for each case (and for each supported codec).
> 
>>
>> I actually thought that this flag would just list all formats, independent
>> of the output format (e.g. AV1, H264, etc.), but that does not appear to be
>> the case? I.e., if capture pixelformat X is only available with AV1, will that still
>> be listed if the output pixel is set to H264?
>>
>> I think you need to describe a real use-case here, and I am not convinced about
>> the name of the flag either.
> 
> I may have miss something but yes the goal is to list all formats independently
> of the output format.
> When a SoC have multiple decoders for the same codec, knowing the supported formats
> is key to select the better one.
> Since I will have to do more iteration, feel free to provide a better name for the
> flag(s). I'm always bad for naming this kind of thing.

That really needs to be clarified, since in patch 1/2 it says:

+   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
+     all the supported formats without taking care of codec-dependent controls
+     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
+     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
+     enumerating.

Here it just talks about 'codec-dependent controls set on the ``OUTPUT`` queue', it
doesn't say anything about the compressed pixelformat set for the OUTPUT queue.

And patch 2/2 sets the ignore_depth_match boolean, suggesting also that it is
not listing all formats, but just ignoring a specific check.

But if you list all pixelformats without taking the OUTPUT pixelformat into
account, how do you know which pixelformat is valid for which codec?

Say that only MPEG support NV12 (just for the sake of argument), and that's
what you want to use, you have no way of knowing that NV12 is specific to MPEG,
you would have to try each codec and see if NV12 is supported for that codec.

I just don't see how this can be used in practice.

What exactly is the problem you want to solve? A real-life problem, not a theoretical
one :-)

Regards,

	Hans

> 
> 
>>
>>> 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 (2):
>>>    media: videodev2: Add flags to unconditionnaly enumerate pixels
>>>      formats
>> I.e.: it is not unconditionally, it still depends on the chosen codec.
>>
>> Regards,
>>
>>     Hans
>>
>>>    media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag
>>>
>>>   .../media/v4l/dev-stateless-decoder.rst          |  6 ++++++
>>>   .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 11 +++++++++++
>>>   .../media/videodev2.h.rst.exceptions             |  2 ++
>>>   drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++---
>>>   drivers/media/v4l2-core/v4l2-ioctl.c             |  3 +++
>>>   include/uapi/linux/videodev2.h                   |  2 ++
>>>   6 files changed, 37 insertions(+), 3 deletions(-)
>>>
>>
> 


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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Benjamin Gaignard]

Le 19/07/2024 à 15:37, Hans Verkuil a écrit :
> On 19/07/2024 15:15, Benjamin Gaignard wrote:
>> Le 19/07/2024 à 14:57, Hans Verkuil a écrit :
>>> On 17/07/2024 15:14, Benjamin Gaignard wrote:
>>>> The goal of this series is to let userland applications enumerate
>>>> all the supported pixels formats of a stateless decoder without
>>>> setting all the possible codec-dependent control.
>>>> That offer a simplest solution for applications to discover
>>>> supported pixels formats and possibly let them doing smarter
>>>> choice between stateless decoders.
>>>>
>>>> An example of how it can be used in GStreamer to discover the
>>>> supported pixels formats for stateless decoder is available here:
>>>> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/v4l2codecs_enum_all_supported_formats?ref_type=heads
>>> So effectively specifying this flag makes ENUM_FMT also return
>>> formats that do not match the bit depth.
>>>
>>> So the AV1 (for example) compressed video uses e.g. 8 bit depth, but instead of just
>>> listing only 8 bit uncompressed pixelformats, you want to list them for any
>>> bit depth.
>>>
>>> But what is the point of that if the decoder can't decode 8 bit compressed to,
>>> say, 10 bit uncompressed video?
>> No decoder will do 8 bits to 10 bits (as far I knows).
>> The point is to be able to say that decoder could produce 10 bit frames without
>> setting a full sps/pps for each case (and for each supported codec).
>>
>>> I actually thought that this flag would just list all formats, independent
>>> of the output format (e.g. AV1, H264, etc.), but that does not appear to be
>>> the case? I.e., if capture pixelformat X is only available with AV1, will that still
>>> be listed if the output pixel is set to H264?
>>>
>>> I think you need to describe a real use-case here, and I am not convinced about
>>> the name of the flag either.
>> I may have miss something but yes the goal is to list all formats independently
>> of the output format.
>> When a SoC have multiple decoders for the same codec, knowing the supported formats
>> is key to select the better one.
>> Since I will have to do more iteration, feel free to provide a better name for the
>> flag(s). I'm always bad for naming this kind of thing.
> That really needs to be clarified, since in patch 1/2 it says:
>
> +   * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate
> +     all the supported formats without taking care of codec-dependent controls
> +     set on the ``OUTPUT`` queue. To indicate that the driver has take care of this
> +     flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while
> +     enumerating.
>
> Here it just talks about 'codec-dependent controls set on the ``OUTPUT`` queue', it
> doesn't say anything about the compressed pixelformat set for the OUTPUT queue.
>
> And patch 2/2 sets the ignore_depth_match boolean, suggesting also that it is
> not listing all formats, but just ignoring a specific check.
>
> But if you list all pixelformats without taking the OUTPUT pixelformat into
> account, how do you know which pixelformat is valid for which codec?
>
> Say that only MPEG support NV12 (just for the sake of argument), and that's
> what you want to use, you have no way of knowing that NV12 is specific to MPEG,
> you would have to try each codec and see if NV12 is supported for that codec.
>
> I just don't see how this can be used in practice.
>
> What exactly is the problem you want to solve? A real-life problem, not a theoretical
> one :-)

On real-life: on a board with 2 different stateless decoders being able to detect the
one which can decode 10 bits bitstreams without testing all codec-dependent controls.

Regards,
Benjamin

>
> Regards,
>
> 	Hans
>
>>
>>>> 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 (2):
>>>>     media: videodev2: Add flags to unconditionnaly enumerate pixels
>>>>       formats
>>> I.e.: it is not unconditionally, it still depends on the chosen codec.
>>>
>>> Regards,
>>>
>>>      Hans
>>>
>>>>     media: verisilicon: Use V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag
>>>>
>>>>    .../media/v4l/dev-stateless-decoder.rst          |  6 ++++++
>>>>    .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 11 +++++++++++
>>>>    .../media/videodev2.h.rst.exceptions             |  2 ++
>>>>    drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++---
>>>>    drivers/media/v4l2-core/v4l2-ioctl.c             |  3 +++
>>>>    include/uapi/linux/videodev2.h                   |  2 ++
>>>>    6 files changed, 37 insertions(+), 3 deletions(-)
>>>>
>

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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Nicolas Dufresne]

Hi,

Le vendredi 19 juillet 2024 à 15:47 +0200, Benjamin Gaignard a écrit :
> > What exactly is the problem you want to solve? A real-life problem, not a theoretical
> > one :-)
> 
> On real-life: on a board with 2 different stateless decoders being able to detect the
> one which can decode 10 bits bitstreams without testing all codec-dependent controls.

That leans toward giving an answer for the selected bitstream format though,
since the same driver may do 10bit HEVC without 10bit AV1.

For the use case, both Chromium and GStreamer have a need to categorized
decoders so that we avoid trying to use decoder that can't do that task. More
platforms are getting multiple decoders, and we also need to take into account
the available software decoders.

Just looking at the codec specific profile is insufficient since we need two
conditions to be met.

1. The driver must support 10bit for the specific CODEC (for most codec this is
profile visible)
2. The produced 10bit color format must be supported by userspace

In today's implementation, in order to test this, we'd need to simulate a 10bit
header control, so that when enumerating the formats we get a list of 10bit
(optionally 8bit too, since some decoder can downscale colors) and finally
verify that these pixel formats are know by userspace. This is not impossible,
but very tedious, this proposal we to try and make this easier.

Nicolas


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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Jonas Karlman]

Hi,

On 2024-07-19 17:36, Nicolas Dufresne wrote:
> Hi,
> 
> Le vendredi 19 juillet 2024 à 15:47 +0200, Benjamin Gaignard a écrit :
>>> What exactly is the problem you want to solve? A real-life problem, not a theoretical
>>> one :-)
>>
>> On real-life: on a board with 2 different stateless decoders being able to detect the
>> one which can decode 10 bits bitstreams without testing all codec-dependent controls.
> 
> That leans toward giving an answer for the selected bitstream format though,
> since the same driver may do 10bit HEVC without 10bit AV1.
> 
> For the use case, both Chromium and GStreamer have a need to categorized
> decoders so that we avoid trying to use decoder that can't do that task. More
> platforms are getting multiple decoders, and we also need to take into account
> the available software decoders.
> 
> Just looking at the codec specific profile is insufficient since we need two
> conditions to be met.
> 
> 1. The driver must support 10bit for the specific CODEC (for most codec this is
> profile visible)
> 2. The produced 10bit color format must be supported by userspace
> 
> In today's implementation, in order to test this, we'd need to simulate a 10bit
> header control, so that when enumerating the formats we get a list of 10bit
> (optionally 8bit too, since some decoder can downscale colors) and finally
> verify that these pixel formats are know by userspace. This is not impossible,
> but very tedious, this proposal we to try and make this easier.

I have also been wondering what the use-case of this would be, and if it
is something to consider before a FFmpeg v4l2-request hwaccel submission.

I am guessing GStreamer may need to decide what decoder to use prior to
bitstream parsing/decoding has started?

For my re-worked FFmpeg v4l2-request hwaccel series, should hit
ffmpeg-devel list any day now, we try to probe each video device one
by one trying to identify if it will be capable to decode current stream
into a known/supported capture format [1], this typically happen when
header for first slice/frame has been parsed and is used to let driver
select its preferred/optimal capture format. The first device where all
test passes will be used and if none works FFmpeg video players will
typically fallback to use software decoding.

This type of probing may be a little bit limiting and depend too heavy
on the M2M Stateless Video Decoder Interface "It is suggested that the
driver chooses the preferred/optimal format for the current
configuration.".

Would you suggest I change how this probing is happening to make some
more clever detection of what media+video device should be used for a
specific stream with help of this new flag?

[1] https://github.com/Kwiboo/FFmpeg/blob/v4l2request-2024-v2/libavcodec/v4l2_request_probe.c#L373-L424

Regards,
Jonas

> 
> Nicolas
> 
> 


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

Re: [PATCH v4 0/2] Enumerate all pixels formats

[Nicolas Dufresne]

Hi,

Le vendredi 19 juillet 2024 à 23:59 +0200, Jonas Karlman a écrit :
> Hi,
> 
> On 2024-07-19 17:36, Nicolas Dufresne wrote:
> > Hi,
> > 
> > Le vendredi 19 juillet 2024 à 15:47 +0200, Benjamin Gaignard a écrit :
> > > > What exactly is the problem you want to solve? A real-life problem, not a theoretical
> > > > one :-)
> > > 
> > > On real-life: on a board with 2 different stateless decoders being able to detect the
> > > one which can decode 10 bits bitstreams without testing all codec-dependent controls.
> > 
> > That leans toward giving an answer for the selected bitstream format though,
> > since the same driver may do 10bit HEVC without 10bit AV1.
> > 
> > For the use case, both Chromium and GStreamer have a need to categorized
> > decoders so that we avoid trying to use decoder that can't do that task. More
> > platforms are getting multiple decoders, and we also need to take into account
> > the available software decoders.
> > 
> > Just looking at the codec specific profile is insufficient since we need two
> > conditions to be met.
> > 
> > 1. The driver must support 10bit for the specific CODEC (for most codec this is
> > profile visible)
> > 2. The produced 10bit color format must be supported by userspace
> > 
> > In today's implementation, in order to test this, we'd need to simulate a 10bit
> > header control, so that when enumerating the formats we get a list of 10bit
> > (optionally 8bit too, since some decoder can downscale colors) and finally
> > verify that these pixel formats are know by userspace. This is not impossible,
> > but very tedious, this proposal we to try and make this easier.
> 
> I have also been wondering what the use-case of this would be, and if it
> is something to consider before a FFmpeg v4l2-request hwaccel submission.
> 
> I am guessing GStreamer may need to decide what decoder to use prior to
> bitstream parsing/decoding has started?

In GStreamer, the parsing is happening before the decoder. The parser generates
capabilities that are used during the decoder selection. Though, for performance
reason (GStreamer is plugin based, and loading all the plugins all the time
during playback is too slow), we generate static information about these
decoders and cache the results. This information usually only contains
profile/level and some min/max resolution when available. But being able to
discard profile if (as an example) the produced 10bit pixel format is not
supported by GStreamer is something we want in the long run. Its not essential
of course.

We do that limitation in the plugger that we can't fallback after the first
bitstream as been passed to the decoder, but this could in theory be resolved
and have nothing to do with this proposal. The proposal is just to help
userspace code stay simple and relatively CODEC agnostic.

> 
> For my re-worked FFmpeg v4l2-request hwaccel series, should hit
> ffmpeg-devel list any day now, we try to probe each video device one
> by one trying to identify if it will be capable to decode current stream
> into a known/supported capture format [1], this typically happen when
> header for first slice/frame has been parsed and is used to let driver
> select its preferred/optimal capture format. The first device where all
> test passes will be used and if none works FFmpeg video players will
> typically fallback to use software decoding.
> 
> This type of probing may be a little bit limiting and depend too heavy
> on the M2M Stateless Video Decoder Interface "It is suggested that the
> driver chooses the preferred/optimal format for the current
> configuration.".
> 
> Would you suggest I change how this probing is happening to make some
> more clever detection of what media+video device should be used for a
> specific stream with help of this new flag?

In general, there is only 1 or 2 decoder, so this process should be fast. I
guess if we start seeing large array or decoder (e.g. array of PCIe accelerators
or similar) that could become interesting to cache the information.

I think the difference is that you have the full bitstream header available at
the time you are to decide which decoder to use, I only have a abstract summary
in GStreamer. In short, you don't have to craft anything, which is nice.

Nicolas

> 
> [1] https://github.com/Kwiboo/FFmpeg/blob/v4l2request-2024-v2/libavcodec/v4l2_request_probe.c#L373-L424
> 
> Regards,
> Jonas
> 
> > 
> > Nicolas
> > 
> > 
> 
> _______________________________________________
> 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