linux-kernel - Re: [PATCH v6 1/3] media: videodev2: Add flag to unconditionally enumerate pixel formats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c9e6ed97-bb42-465a-9bce-797adb44906f@xs4all.nl>
Date: Wed, 7 Aug 2024 10:39:54 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, mchehab@...nel.org,
 ezequiel@...guardiasur.com.ar
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v6 1/3] media: videodev2: Add flag to unconditionally
 enumerate pixel formats

On 31/07/2024 11:34, Benjamin Gaignard wrote:
> When the index is ORed with V4L2_FMTDESC_FLAG_ENUM_ALL the
> driver clears the flag and enumerate all the possible formats,
> ignoring any limitations from the current configuration.
> Drivers which do not support this flag yet always return an EINVAL.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
> change in version 6:
> - Change flag name.
> - Improve documentation.
> 
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 16 +++++++++++++++-
>  .../media/videodev2.h.rst.exceptions             |  1 +
>  include/uapi/linux/videodev2.h                   |  3 +++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 3adb3d205531..1112dc9044b2 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -85,7 +85,15 @@ the ``mbus_code`` field is handled differently:
>      * - __u32
>        - ``index``
>        - Number of the format in the enumeration, set by the application.
> -	This is in no way related to the ``pixelformat`` field.
> +        This is in no way related to the ``pixelformat`` field.
> +        When the index is ORed with ``V4L2_FMTDESC_FLAG_ENUM_ALL`` the
> +        driver clears the flag and enumerate all the possible formats,

enumerate -> enumerates

> +        ignoring any limitations from the current configuration. Drivers
> +        which do not support this flag yet always return an ``EINVAL``

Drop the 'yet'.

But this raises a question: should this flag only be supported by drivers
that can actually return different format lists depending on this flag?

Or can it be supported as well by a driver where this makes no difference?

I'm inclined to limit it to drivers that actually can return different
results. If nothing else, that will indicate to the application that this
is actually possible.

If we agree on that, then that should be documented as well.

> +        error code.
> +        Formats enumerated when using ``V4L2_FMTDESC_FLAG_ENUM_ALL`` flag
> +        shouldn't be used when calling :c:func:`VIDIOC_ENUM_FRAMESIZES`
> +        or :c:func:`VIDIOC_ENUM_FRAMEINTERVALS`.
>      * - __u32
>        - ``type``
>        - Type of the data stream, set by the application. Only these types
> @@ -234,6 +242,12 @@ the ``mbus_code`` field is handled differently:
>  	valid. The buffer consists of ``height`` lines, each having ``width``
>  	Data Units of data and the offset (in bytes) between the beginning of
>  	each two consecutive lines is ``bytesperline``.
> +    * - ``V4L2_FMTDESC_FLAG_ENUM_ALL``
> +      - 0x80000000
> +      - When the applications ORs ``index`` with ``V4L2_FMTDESC_FLAG_ENUM_ALL`` flag
> +        the driver enumerates all the possible pixel formats without taking care
> +        of any already set configuration. Drivers which do not support this flag
> +        yet, always return ``EINVAL``.

Drop 'yet'

>  
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index bdc628e8c1d6..0a9ea9686c24 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -216,6 +216,7 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags
>  replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags
>  replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags
>  replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags
> +replace define V4L2_FMTDESC_FLAG_ENUM_ALL fmtdesc-flags
>  
>  # V4L2 timecode types
>  replace define V4L2_TC_TYPE_24FPS timecode-type
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 4e91362da6da..421a30cb0c51 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -904,6 +904,9 @@ struct v4l2_fmtdesc {
>  #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>  #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>  
> +/*  Format description flag, to be ORed with the index */
> +#define V4L2_FMTDESC_FLAG_ENUM_ALL		0x80000000
> +
>  	/* Frame Size and frame rate enumeration */
>  /*
>   *	F R A M E   S I Z E   E N U M E R A T I O N

Regards,

	Hans