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

[Jacopo Mondi]

Hi Benjamin

On Mon, Jul 15, 2024 at 08:09:55AM GMT, Benjamin Gaignard wrote:
>
> Le 12/07/2024 à 18:31, Jacopo Mondi a écrit :
> > Hi Benjamin
> >
> > On Fri, Jul 12, 2024 at 12:29:59PM 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@xxxxxxxxxxxxx>
> > > ---
> > > changes in version 3:
> > > - Add a flag to inform userspace application that driver
> > >    as take care of the flag.
> > >
> > >   .../userspace-api/media/v4l/dev-stateless-decoder.rst    | 6 ++++++
> > >   .../userspace-api/media/v4l/vidioc-enum-fmt.rst          | 9 +++++++++
> > >   .../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, 22 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..0399e0fc09b3 100644
> > > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> > > @@ -234,6 +234,15 @@ 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 seems to be only relevant for codecs and m2m devices.
> >
> > For 'regular' capture (and I presume output) devices isn't the default
> > behaviour to enumerate all pixel formats, and userspace can decide to
> > restrict them by providing a media bus code (if the device reports the
> > V4L2_CAP_IO_MC capability)
>
> Yes is targeting codecs because they have two queues.

Well, m2m devices in general then

> When device have only one queue, like 'regular' capture devices,
> pixel enumeration doesn't have to take care of the other queue
> configuration because it doesn't exist.
>

This probably needs to be captured in the description here ?
We already have flags which apply only to some device types, and
that's specified in the flag description

    * - ``V4L2_FMT_FLAG_CSC_QUANTIZATION``
      - 0x0100
      - The driver allows the application to try to change the default
	quantization. This flag is relevant only for capture 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.
> > >
> > >   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
> > >
> > >
>