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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxx>

   	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@xxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to kernel-leave@xxxxxxxxxxxxxxxxxxxxx
This list is managed by https://mailman.collabora.com





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux