Re: [PATCH v4 25/29] media: v4l2-subdev: Move subdev client capabilities into a new struct

Jacopo Mondi
2026-04-10T13:31:38Z

Hi Sakari

On Wed, Apr 08, 2026 at 06:39:34PM +0300, Sakari Ailus wrote:
> Add struct v4l2_subdev_client_info to hold sub-device client capability
> bits that used to be stored in the client_caps field of struct
> v4l2_subdev_fh. The intent is to enable passing this struct to sub-device
> pad operation callbacks for capability information. The main reason why
> this is a new struct instead of a u64 field is that modifying the callback
> arguments requires touching almost every sub-device driver and that is
> desirable to avoid in the future, should more than the client capability bits
> need to be known to the callbacks.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 8 ++++----
> include/media/v4l2-subdev.h | 12 ++++++++++--
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 40b28e070726..eaa408832c6b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -612,7 +612,7 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
> case VIDIOC_SUBDEV_S_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval *fi = arg;
>
> - if (!(subdev_fh->client_caps &
> + if (!(subdev_fh->ci.client_caps &
> V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH))
> fi->which = V4L2_SUBDEV_FORMAT_ACTIVE;
>
> @@ -652,7 +652,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> 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 &
> + bool client_supports_streams = subdev_fh->ci.client_caps &
> V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> int rval;
>
> @@ -1119,7 +1119,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> case VIDIOC_SUBDEV_G_CLIENT_CAP: {
> struct v4l2_subdev_client_capability *client_cap = arg;
>
> - client_cap->capabilities = subdev_fh->client_caps;
> + client_cap->capabilities = subdev_fh->ci.client_caps;
>
> return 0;
> }
> @@ -1139,7 +1139,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> client_cap->capabilities &= (V4L2_SUBDEV_CLIENT_CAP_STREAMS |
> V4L2_SUBDEV_CLIENT_CAP_INTERVAL_USES_WHICH);
>
> - subdev_fh->client_caps = client_cap->capabilities;
> + subdev_fh->ci.client_caps = client_cap->capabilities;

I'm sorry for being annoying, but the discussion on v2 ended with the
question on why we can't propagate the client caps to the drivers
using the subdev state.

I understand we have the active state which is stored in the subdev
and not per-file handle. But ioctls are called on a file handle and it
seems trivial to me copy the caps from the subdev_fh to the active
state.

It would result in a much smaller set of changes.

What am I missing ?

>
> return 0;
> }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 93b672edd08e..e0861acc638d 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -734,6 +734,14 @@ struct v4l2_subdev_state {
> struct v4l2_subdev_stream_configs stream_configs;
> };
>
> +/**
> + * struct v4l2_subdev_client_info - Sub-device client information
> + * @client_caps: bitmask of ``V4L2_SUBDEV_CLIENT_CAP_*``
> + */
> +struct v4l2_subdev_client_info {
> + u64 client_caps;
> +};
> +
> /**
> * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations
> *
> @@ -1129,14 +1137,14 @@ struct v4l2_subdev {
> * @vfh: pointer to &struct v4l2_fh
> * @state: pointer to &struct v4l2_subdev_state
> * @owner: module pointer to the owner of this file handle
> - * @client_caps: bitmask of ``V4L2_SUBDEV_CLIENT_CAP_*``
> + * @ci: sub-device client info related to this file handle
> */
> struct v4l2_subdev_fh {
> struct v4l2_fh vfh;
> struct module *owner;
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> struct v4l2_subdev_state *state;
> - u64 client_caps;
> + struct v4l2_subdev_client_info ci;
> #endif
> };
>
> --
> 2.47.3
>
>