Hi Tomi,
Thank you for the patch.
On Thu, Mar 23, 2023 at 03:58:35PM +0200, Tomi Valkeinen wrote:
> Add new ioctls to set and get subdev client capabilities. Client in this
> context means the userspace application which opens the subdev device
> node. The client capabilities are stored in the file handle of the
> opened subdev device node, and the client must set the capabilities for
> each opened subdev.
>
> For now we only add a single flag, V4L2_SUBDEV_CLIENT_CAP_STREAMS, which
> indicates that the client is streams-aware.
>
> The reason for needing such a flag is as follows:
>
> Many structs passed via ioctls, e.g. struct v4l2_subdev_format, contain
> reserved fields (usually a single array field). These reserved fields
> can be used to extend the ioctl. The userspace is required to zero the
> reserved fields.
>
> We recently added a new 'stream' field to many of these structs, and the
> space for the field was taken from these reserved arrays. The assumption
> was that these new 'stream' fields are always initialized to zero if the
> userspace does not use them. This was a mistake, as, as mentioned above,
> the userspace is required to zero the _reserved_ fields. In other words,
> there is no requirement to zero this new stream field, and if the
> userspace doesn't use the field (which is the case for all userspace
> applications at the moment), the field may contain random data.
>
> This shows that the way the reserved fields are defined in v4l2 is, in
> my opinion, somewhat broken, but there is nothing to do about that.
>
> To fix this issue we need a way for the userspace to tell the kernel
> that the userspace has indeed set the 'stream' field, and it's fine for
> the kernel to access it. This is achieved with the new ioctl, which the
> userspace should usually use right after opening the subdev device node.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> Interdiff against v2:
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> index 0c4ff938f63d..20f12a1cc0f7 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> @@ -37,20 +37,29 @@ Description
> ===========
>
> These ioctls are used to get and set the client (the application using the
> -subdevice ioctls) capabilities. By default no client capabilities are set.
> +subdevice ioctls) capabilities. The client capabilities are stored in the file
> +handle of the opened subdev device node, and the client must set the
> +capabilities for each opened subdev separately.
> +
> +By default no client capabilities are set when a subdev device node is opened.
>
> The purpose of the client capabilities are to inform the kernel of the behavior
> of the client, mainly related to maintaining compatibility with different
> kernel and userspace versions.
>
> -The ``VIDIOC_SUBDEV_S_CLIENT_CAP`` will modify the struct
> -:c:type:`v4l2_subdev_client_capability` to reflect the capabilities that were
> -accepted. A common case for the kernel not accepting a capability is that the
> -kernel is older than the headers the userspace uses, and thus the capability is
> -unknown to the kernel.
> +The ``VIDIOC_SUBDEV_G_CLIENT_CAP`` ioctl returns the current client capabilities
> +associated with the file handle ``fd``.
>
> -The ``VIDIOC_SUBDEV_S_CLIENT_CAP`` will replace all the previously set
> -capabilities.
> +The ``VIDIOC_SUBDEV_S_CLIENT_CAP`` ioctl sets client capabilities for the file
> +handle ``fd``. The new capabilities fully replace the current capabilities, the
> +ioctl can therefore also be used to remove capabilities that have previously
> +been set.
> +
> +``VIDIOC_SUBDEV_S_CLIENT_CAP`` modifies the struct
> +:c:type:`v4l2_subdev_client_capability` to reflect the capabilities that have
> +been accepted. A common case for the kernel not accepting a capability is that
> +the kernel is older than the headers the userspace uses, and thus the capability
> +is unknown to the kernel.
>
> .. flat-table:: Client Capabilities
> :header-rows: 1
>
> .../userspace-api/media/v4l/user-func.rst | 1 +
> .../media/v4l/vidioc-subdev-g-client-cap.rst | 83 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-subdev.c | 63 ++++++++++++++
> include/media/v4l2-subdev.h | 1 +
> include/uapi/linux/v4l2-subdev.h | 21 +++++
> 5 files changed, 169 insertions(+)
> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 228c1521f190..15ff0bf7bbe6 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -72,6 +72,7 @@ Function Reference
> vidioc-subdev-g-frame-interval
> vidioc-subdev-g-routing
> vidioc-subdev-g-selection
> + vidioc-subdev-g-client-cap
> vidioc-subdev-querycap
> vidioc-subscribe-event
> func-mmap
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> new file mode 100644
> index 000000000000..20f12a1cc0f7
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst
> @@ -0,0 +1,83 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_SUBDEV_G_CLIENT_CAP:
> +
> +************************************************************
> +ioctl VIDIOC_SUBDEV_G_CLIENT_CAP, VIDIOC_SUBDEV_S_CLIENT_CAP
> +************************************************************
> +
> +Name
> +====
> +
> +VIDIOC_SUBDEV_G_CLIENT_CAP - VIDIOC_SUBDEV_S_CLIENT_CAP - Get or set client
> +capabilities.
> +
> +Synopsis
> +========
> +
> +.. c:macro:: VIDIOC_SUBDEV_G_CLIENT_CAP
> +
> +``int ioctl(int fd, VIDIOC_SUBDEV_G_CLIENT_CAP, struct v4l2_subdev_client_capability *argp)``
> +
> +.. c:macro:: VIDIOC_SUBDEV_S_CLIENT_CAP
> +
> +``int ioctl(int fd, VIDIOC_SUBDEV_S_CLIENT_CAP, struct v4l2_subdev_client_capability *argp)``
> +
> +Arguments
> +=========
> +
> +``fd``
> + File descriptor returned by :ref:`open() <func-open>`.
> +
> +``argp``
> + Pointer to struct :c:type:`v4l2_subdev_client_capability`.
> +
> +Description
> +===========
> +
> +These ioctls are used to get and set the client (the application using the
> +subdevice ioctls) capabilities. The client capabilities are stored in the file
> +handle of the opened subdev device node, and the client must set the
> +capabilities for each opened subdev separately.
> +
> +By default no client capabilities are set when a subdev device node is opened.
> +
> +The purpose of the client capabilities are to inform the kernel of the behavior
> +of the client, mainly related to maintaining compatibility with different
> +kernel and userspace versions.
> +
> +The ``VIDIOC_SUBDEV_G_CLIENT_CAP`` ioctl returns the current client capabilities
> +associated with the file handle ``fd``.
> +
> +The ``VIDIOC_SUBDEV_S_CLIENT_CAP`` ioctl sets client capabilities for the file
> +handle ``fd``. The new capabilities fully replace the current capabilities, the
> +ioctl can therefore also be used to remove capabilities that have previously
> +been set.
> +
> +``VIDIOC_SUBDEV_S_CLIENT_CAP`` modifies the struct
> +:c:type:`v4l2_subdev_client_capability` to reflect the capabilities that have
> +been accepted. A common case for the kernel not accepting a capability is that
> +the kernel is older than the headers the userspace uses, and thus the capability
> +is unknown to the kernel.
> +
> +.. flat-table:: Client Capabilities
> + :header-rows: 1
> +
> + * - Capability
> + - Description
> + * - ``V4L2_SUBDEV_CLIENT_CAP_STREAMS``
> + - The client is aware of streams. Setting this flag enables the use
> + of 'stream' fields (referring to the stream number) with various
> + ioctls. If this is not set (which is the default), the 'stream' fields
> + will be forced to 0 by the kernel.
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> +
> +ENOIOCTLCMD
> + The kernel does not support this ioctl.
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index dff1d9be7841..9d47f4800ea8 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -498,8 +498,11 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> struct video_device *vdev = video_devdata(file);
> struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> struct v4l2_fh *vfh = file->private_data;
> + struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
> bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
> bool streams_subdev = sd->flags & V4L2_SUBDEV_FL_STREAMS;
> + bool client_supports_streams = subdev_fh->client_caps &
> + V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> int rval;
>
> switch (cmd) {
> @@ -624,6 +627,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_G_FMT: {
> struct v4l2_subdev_format *format = arg;
>
> + if (!client_supports_streams)
> + format->stream = 0;
> +
> memset(format->reserved, 0, sizeof(format->reserved));
> memset(format->format.reserved, 0, sizeof(format->format.reserved));
> return v4l2_subdev_call(sd, pad, get_fmt, state, format);
> @@ -635,6 +641,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> if (format->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
> return -EPERM;
>
> + if (!client_supports_streams)
> + format->stream = 0;
> +
> memset(format->reserved, 0, sizeof(format->reserved));
> memset(format->format.reserved, 0, sizeof(format->format.reserved));
> return v4l2_subdev_call(sd, pad, set_fmt, state, format);
> @@ -644,6 +653,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> struct v4l2_subdev_crop *crop = arg;
> struct v4l2_subdev_selection sel;
>
> + if (!client_supports_streams)
> + crop->stream = 0;
> +
> memset(crop->reserved, 0, sizeof(crop->reserved));
> memset(&sel, 0, sizeof(sel));
> sel.which = crop->which;
> @@ -665,6 +677,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> if (crop->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
> return -EPERM;
>
> + if (!client_supports_streams)
> + crop->stream = 0;
> +
> memset(crop->reserved, 0, sizeof(crop->reserved));
> memset(&sel, 0, sizeof(sel));
> sel.which = crop->which;
> @@ -683,6 +698,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {
> struct v4l2_subdev_mbus_code_enum *code = arg;
>
> + if (!client_supports_streams)
> + code->stream = 0;
> +
> memset(code->reserved, 0, sizeof(code->reserved));
> return v4l2_subdev_call(sd, pad, enum_mbus_code, state,
> code);
> @@ -691,6 +709,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: {
> struct v4l2_subdev_frame_size_enum *fse = arg;
>
> + if (!client_supports_streams)
> + fse->stream = 0;
> +
> memset(fse->reserved, 0, sizeof(fse->reserved));
> return v4l2_subdev_call(sd, pad, enum_frame_size, state,
> fse);
> @@ -699,6 +720,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_G_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval *fi = arg;
>
> + if (!client_supports_streams)
> + fi->stream = 0;
> +
> memset(fi->reserved, 0, sizeof(fi->reserved));
> return v4l2_subdev_call(sd, video, g_frame_interval, arg);
> }
> @@ -709,6 +733,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> if (ro_subdev)
> return -EPERM;
>
> + if (!client_supports_streams)
> + fi->stream = 0;
> +
> memset(fi->reserved, 0, sizeof(fi->reserved));
> return v4l2_subdev_call(sd, video, s_frame_interval, arg);
> }
> @@ -716,6 +743,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval_enum *fie = arg;
>
> + if (!client_supports_streams)
> + fie->stream = 0;
> +
> memset(fie->reserved, 0, sizeof(fie->reserved));
> return v4l2_subdev_call(sd, pad, enum_frame_interval, state,
> fie);
> @@ -724,6 +754,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_G_SELECTION: {
> struct v4l2_subdev_selection *sel = arg;
>
> + if (!client_supports_streams)
> + sel->stream = 0;
> +
> memset(sel->reserved, 0, sizeof(sel->reserved));
> return v4l2_subdev_call(
> sd, pad, get_selection, state, sel);
> @@ -735,6 +768,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> if (sel->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
> return -EPERM;
>
> + if (!client_supports_streams)
> + sel->stream = 0;
> +
> memset(sel->reserved, 0, sizeof(sel->reserved));
> return v4l2_subdev_call(
> sd, pad, set_selection, state, sel);
> @@ -876,6 +912,33 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> routing->which, &krouting);
> }
>
> + case VIDIOC_SUBDEV_G_CLIENT_CAP: {
> + struct v4l2_subdev_client_capability *client_cap = arg;
> +
> + client_cap->capabilities = subdev_fh->client_caps;
> +
> + return 0;
> + }
> +
> + case VIDIOC_SUBDEV_S_CLIENT_CAP: {
> + struct v4l2_subdev_client_capability *client_cap = arg;
> +
> + /*
> + * Clear V4L2_SUBDEV_CLIENT_CAP_STREAMS if streams API is not
> + * enabled. Remove this when streams API is no longer
> + * experimental.
> + */
> + if (!v4l2_subdev_enable_streams_api)
> + client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> +
> + /* Filter out unsupported capabilities */
> + client_cap->capabilities &= V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> +
> + subdev_fh->client_caps = client_cap->capabilities;
> +
> + return 0;
> + }
> +
> default:
> return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 17773be4a4ee..b5bb5b802929 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1121,6 +1121,7 @@ struct v4l2_subdev_fh {
> struct module *owner;
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> struct v4l2_subdev_state *state;
> + u64 client_caps;
> #endif
> };
>
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 654d659de835..4a195b68f28f 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -233,6 +233,24 @@ struct v4l2_subdev_routing {
> __u32 reserved[6];
> };
>
> +/*
> + * The client is aware of streams. Setting this flag enables the use of 'stream'
> + * fields (referring to the stream number) with various ioctls. If this is not
> + * set (which is the default), the 'stream' fields will be forced to 0 by the
> + * kernel.
> + */
> + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0)
> +
> +/**
> + * struct v4l2_subdev_client_capability - Capabilities of the client accessing
> + * the subdev
> + *
> + * @capabilities: A bitmask of V4L2_SUBDEV_CLIENT_CAP_* flags.
> + */
> +struct v4l2_subdev_client_capability {
> + __u64 capabilities;
> +};
> +
> /* Backwards compatibility define --- to be removed */
> #define v4l2_subdev_edid v4l2_edid
>
> @@ -250,6 +268,9 @@ struct v4l2_subdev_routing {
> #define VIDIOC_SUBDEV_S_SELECTION _IOWR('V', 62, struct v4l2_subdev_selection)
> #define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing)
> #define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing)
> +#define VIDIOC_SUBDEV_G_CLIENT_CAP _IOR('V', 101, struct v4l2_subdev_client_capability)
> +#define VIDIOC_SUBDEV_S_CLIENT_CAP _IOWR('V', 102, struct v4l2_subdev_client_capability)
> +
> /* The following ioctls are identical to the ioctls in videodev2.h */
> #define VIDIOC_SUBDEV_G_STD _IOR('V', 23, v4l2_std_id)
> #define VIDIOC_SUBDEV_S_STD _IOW('V', 24, v4l2_std_id)
--
Regards,
Laurent Pinchart
