From: Hans Verkuil <hverkuil@xs4all.nl>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
"Helen Koike" <helen.koike@collabora.com>,
"Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
linux-media@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org,
Hans Verkuil <hverkuil-cisco@xs4all.nl>
Subject: Re: [PATCH v8 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices
Date: Thu, 23 Apr 2020 12:38:49 +0200 [thread overview]
Message-ID: <d234f2bf-9bda-e073-ad6f-392f6b5d0cdd@xs4all.nl> (raw)
In-Reply-To: <20200421135743.1381930-4-niklas.soderlund+renesas@ragnatech.se>
On 21/04/2020 15:57, Niklas Söderlund wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> node. For MC-centric devices, its behaviour has always been ill-defined,
> with drivers implementing one of the following behaviours:
>
> - No support for VIDIOC_ENUM_FMT at all
> - Enumerating all formats supported by the video node, regardless of the
> configuration of the pipeline
> - Enumerating formats supported by the video node for the active
> configuration of the connected subdevice
>
> The first behaviour is obviously useless for applications. The second
> behaviour provides the most information, but doesn't offer a way to find
> what formats are compatible with a given pipeline configuration. The
> third behaviour fixes that, but with the drawback that applications
> can't enumerate all supported formats anymore, and have to modify the
> active configuration of the pipeline to enumerate formats.
>
> The situation is messy as none of the implemented behaviours are ideal,
> and userspace can't predict what will happen as the behaviour is
> driver-specific.
>
> To fix this, let's extend the VIDIOC_ENUM_FMT with a missing capability:
> enumerating pixel formats for a given media bus code. The media bus code
> is passed through the v4l2_fmtdesc structure in a new mbus_code field
> (repurposed from the reserved fields). With this capability in place,
> applications can enumerate pixel formats for a given media bus code
> without modifying the active configuration of the device.
>
> The current behaviour of the ioctl is preserved when the new mbus_code
> field is set to 0, ensuring compatibility with existing userspace. The
> API extension is documented as mandatory for MC-centric devices (as
> advertised through the V4L2_CAP_IO_MC capability), allowing applications
> and compliance tools to easily determine the availability of the
> VIDIOC_ENUM_FMT extension.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v7
> - Update documentation.
> ---
> .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 16 +++++++++++++---
> drivers/media/v4l2-core/v4l2-ioctl.c | 13 +++++++++++--
> include/uapi/linux/videodev2.h | 3 ++-
> 3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 7e3142e11d77d9c0..8dc8a73c320dda98 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -48,10 +48,20 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
> formats in preference order, where preferred formats are returned before
> (that is, with lower ``index`` value) less-preferred formats.
>
> -.. note::
> +If the driver doesn't advertise the ``V4L2_CAP_IO_MC``
> +:ref:`device-capabilities <capability>`, applications shall initialize the
That's :ref:`capability <device-capabilities>`
> +``mbus_code`` field to zero. Drivers shall enumerate all image formats. The
> +enumerated formats may depend on the active input or output of the device.
>
> - After switching input or output the list of enumerated image
> - formats may be different.
> +If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`device-capabilities
> +<capability>`, applications may initialize the ``mbus_code`` to a valid
Ditto.
> +:ref:`v4l2_mbus_pixelcode <media bus format code>`. If the ``mbus_code` field
:ref:`media bus format code <v4l2-mbus-pixelcode>`
``mbus_code` -> ``mbus_code``
> +is not zero, drivers shall restrict enumeration to only the image formats that
> +can produce (for video output devices) or be produced from (for video capture
> +devices) that media bus code. Regardless of the value of the ``mbus_code``
> +field, the enumerated image formats shall not depend on the active
> +configuration of the video device or device pipeline. Enumeration shall
> +otherwise operate as previously described.
The new field isn't documented, vidioc-enum-fmt.rst still shows 'reserved[4]' in
the struct v4l2_fmtdesc description.
>
>
> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index afd1f427df557f71..3e7b99fa415222c6 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -264,12 +264,13 @@ static void v4l_print_fmtdesc(const void *arg, bool write_only)
> {
> const struct v4l2_fmtdesc *p = arg;
>
> - pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, description='%.*s'\n",
> + pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, mbus_code=0x%04x, description='%.*s'\n",
> p->index, prt_names(p->type, v4l2_type_names),
> p->flags, (p->pixelformat & 0xff),
> (p->pixelformat >> 8) & 0xff,
> (p->pixelformat >> 16) & 0xff,
> (p->pixelformat >> 24) & 0xff,
> + p->mbus_code,
> (int)sizeof(p->description), p->description);
> }
>
> @@ -1472,12 +1473,20 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> struct video_device *vdev = video_devdata(file);
> struct v4l2_fmtdesc *p = arg;
> int ret = check_fmt(file, p->type);
> + u32 mbus_code;
> u32 cap_mask;
>
> if (ret)
> return ret;
> ret = -EINVAL;
>
> + if (p->mbus_code && !(vdev->device_caps & V4L2_CAP_IO_MC))
> + return -EINVAL;
I suggest changing this to:
if (!(vdev->device_caps & V4L2_CAP_IO_MC))
p->mbus_code = 0;
This is much more robust for existing userspace since they may not have
properly zeroed the reserved array. Also, mbus_code only makes sense in
combination with CAP_IO_MC, so zeroing it if CAP_IO_MC isn't set makes sense.
The documentation probably needs to be updated to reflect this change as well.
I'll post a patch for v4l2-compliance as well.
Regards,
Hans
> +
> + mbus_code = p->mbus_code;
> + CLEAR_AFTER_FIELD(p, type);
> + p->mbus_code = mbus_code;
> +
> switch (p->type) {
> case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> @@ -2757,7 +2766,7 @@ DEFINE_V4L_STUB_FUNC(dv_timings_cap)
>
> static const struct v4l2_ioctl_info v4l2_ioctls[] = {
> IOCTL_INFO(VIDIOC_QUERYCAP, v4l_querycap, v4l_print_querycap, 0),
> - IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, INFO_FL_CLEAR(v4l2_fmtdesc, type)),
> + IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, 0),
> IOCTL_INFO(VIDIOC_G_FMT, v4l_g_fmt, v4l_print_format, 0),
> IOCTL_INFO(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO),
> IOCTL_INFO(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE),
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index b18f3f7cde31c2e4..c3a1cf1c507f5506 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -784,7 +784,8 @@ struct v4l2_fmtdesc {
> __u32 flags;
> __u8 description[32]; /* Description string */
> __u32 pixelformat; /* Format fourcc */
> - __u32 reserved[4];
> + __u32 mbus_code; /* Media bus code */
> + __u32 reserved[3];
> };
>
> #define V4L2_FMT_FLAG_COMPRESSED 0x0001
>
next prev parent reply other threads:[~2020-04-23 10:38 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-21 13:57 [PATCH v8 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
2020-04-21 13:57 ` [PATCH v8 1/6] " Niklas Söderlund
2020-04-21 13:57 ` [PATCH v8 2/6] media: pci: Fill v4l2_fmtdesc with designated initializers Niklas Söderlund
2020-04-21 13:57 ` [PATCH v8 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices Niklas Söderlund
2020-04-22 17:26 ` Sakari Ailus
2020-04-23 10:38 ` Hans Verkuil [this message]
2020-04-24 13:43 ` [PATCH v8.1 " Laurent Pinchart
2020-04-29 16:27 ` Mauro Carvalho Chehab
2020-04-29 16:38 ` Laurent Pinchart
2020-04-29 18:01 ` Mauro Carvalho Chehab
2020-04-29 18:50 ` Laurent Pinchart
2020-04-29 19:46 ` Mauro Carvalho Chehab
2020-04-29 20:08 ` Sakari Ailus
2020-05-05 14:27 ` Hans Verkuil
2020-05-06 9:53 ` Mauro Carvalho Chehab
2020-05-06 9:51 ` Mauro Carvalho Chehab
2020-04-21 13:57 ` [PATCH v8 4/6] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
2020-04-21 13:57 ` [PATCH v8 5/6] staging/intel-ipu3: " Niklas Söderlund
2020-04-21 13:57 ` [PATCH v8 6/6] vimc: " Niklas Söderlund
2020-04-22 17:22 ` [PATCH v8 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Laurent Pinchart
2020-04-23 9:46 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d234f2bf-9bda-e073-ad6f-392f6b5d0cdd@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=helen.koike@collabora.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).