Message-ID: <cc3944167e6b98470befd575520adb50cb8a45fa.camel@collabora.com>
Date: Wed, 17 Jan 2024 14:41:03 -0500
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, mchehab@...nel.org,
  p.zabel@...gutronix.de, hverkuil-cisco@...all.nl
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [RCF 1/2] media: videodev2: Add V4L2_FMT_FLAG_ALL_FORMATS flag

Le jeudi 11 janvier 2024 à 17:07 +0100, Benjamin Gaignard a écrit :
> Add new flag to allow enumerate all pixels formats when
> calling VIDIOC_ENUM_FMT ioctl.
> When this flag is set drivers must ignore the configuration
> and return the hardware supported pixel formats for the specified queue.
> This will permit to discover which pixels formats are supported
> without setting codec-specific information so userland can more easily
> knows if the driver suit well to what it needs.
> The main target are stateless decoders so update the documentation
> about how use this flag.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
>  .../userspace-api/media/v4l/dev-stateless-decoder.rst         | 3 +++
>  Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst     | 4 ++++
>  Documentation/userspace-api/media/videodev2.h.rst.exceptions  | 1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c                          | 2 +-
>  include/uapi/linux/videodev2.h                                | 1 +
>  5 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> index 35ed05f2695e..b7b650f1a18f 100644
> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> @@ -58,6 +58,9 @@ 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 ``V4L2_FMT_FLAG_ALL_FORMATS`` flag is set the driver must enumerate
> +     all the supported formats without taking care of codec-dependent controls.
> +
>  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 000c154b0f98..db8bc8e29a91 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -227,6 +227,10 @@ the ``mbus_code`` field is handled differently:
>  	The application can ask to configure the quantization of the capture
>  	device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with
>  	:ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set.
> +    * - ``V4L2_FMT_FLAG_ALL_FORMATS``
> +      - 0x0200
> +      - Set by userland application to enumerate all possible pixels formats
> +        without taking care of the current configuration.

This is a bit ambiguous regarding if OUTPUT queue FMT is ignored or not. From
our chat, it is ignored in this implementation. Such if I use MTK VCODEC as an
example, using that feature would return:

- MM21
- MT2T
- MT2R

At high level, the use case is to find an easy way to combine the per codec
profile information and the pixel format, since userspace can only use e.g.
10bit capability if it knows the associated pixel formats. This implementation
is already useful in my opinion, I'll try and draft a GStreamer change to use
it, as I think it will better support the idea. But it has come ceavats.

Notably, if you had a userland that implement MT2T (VP9/AV1/HEVC) but not MT2R
(H264), it would not have an easy API to figure-out. It would still have to
resort into enumerating formats for each possible codec and codec specific
compound control configuration.

An alternative is to make this enumerate "all" for the configure OUTPUT format.
This increase the precision, while still allowing generic code to be used. In
pseudo code that would be like:

for output formats
  S_FMT(OUTPUT)

  for ALL capture formats
    store(format)

Where right now we have do do:


for output formats
  S_FMT(OUTPUT)

  S_CTRL(codec_specific_ctl_config_1)
  for capture formats
    store(format)


  S_CTRL(codec_specific_ctl_config_n)
  for capture format
    store(format)
  
  ...

  S_CTRL(codec_specific_ctl_config_n)
  for capture formats
    store(format)

Where each config would demote a specific feature, like 10bit, 422, 444, film-
grain (posprocessing affect output formats). The posprocessing remains a bit
hard to figure-out in both cases though. But in practice, if I use Hantro AV1
decoder as an example, I'd get something like:

  NV15_4L4
  P010

Where NV15_4L4 is not available with filmgrain filter, but P010 is always
available. For my GStreamer use case (template caps) this wouldn't be a problem,
P010 is a well supported format and I strictly need a superset of the supported
formats.

What I'd really gain is that I don't have to do complicated enumeration logic
per codec features.

Nicolas

p.s. It would be logical to update dev-stateless-decoder doc, to mention this
enumeration option. Currently it says:


   To enumerate the set of supported raw formats, the client calls
   VIDIOC_ENUM_FMT() on the CAPTURE queue.
   
      *    The driver must return only the formats supported for the format
      currently active on the OUTPUT queue.
   
      *    Depending on the currently set OUTPUT format, the set of supported
      raw formats may depend on the value of some codec-dependent controls. The
      client is responsible for making sure that these controls are set before
      querying the CAPTURE queue. Failure to do so will result in the 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.


>  
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index 3e58aac4ef0b..42d9075b7fc2 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -215,6 +215,7 @@ replace define V4L2_FMT_FLAG_CSC_XFER_FUNC fmtdesc-flags
>  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_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 33076af4dfdb..22a93d074a5b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1544,7 +1544,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  		p->mbus_code = 0;
>  
>  	mbus_code = p->mbus_code;
> -	memset_after(p, 0, type);
> +	memset_after(p, 0, flags);

In other similar places, we still clear the flags, and only keep the allowed
bits. Maybe we should do this here too to avoid accidental flags going through ?

That should maybe be under some capability flag, so that userland knows if the
driver did implement that feature or not. If the driver didn't set that flag, we
can then clear it so that userlands not checking that flag would at least get an
enumeration response without it.

>  	p->mbus_code = mbus_code;
>  
>  	switch (p->type) {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 68e7ac178cc2..82d8c8a7fb7f 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -869,6 +869,7 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CSC_YCBCR_ENC		0x0080
>  #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0200
>  
>  	/* Frame Size and frame rate enumeration */
>  /*

Powered by blists - more mailing lists