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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari

On Mon, Apr 13, 2026 at 11:11:33AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, Apr 10, 2026 at 03:31:32PM +0200, Jacopo Mondi wrote:
> > 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 ?
>
> I thought the discussion ended with the conclusion that we can't do that as
> the sub-device active state isn't bound to a file handle. :-)
>

Can you clarify why this can't happen in your opinion ?

Yes, the active state is not bound to any fh, but all ioctls
operations are issued on an fh, so it's trivial to copy the caps flags
in the active state before passing it to drivers.

If this is not preferred because it feels like an hack I understand
it, it actually is, but I would like to think in perspective here.

We want contexts, I think that's clear.

In the design I proposed the active state will be moved from the
subdev to the context (when available). A state lives in a context
which is bound to file handle. Getting the caps from a state is then a
matter of providing a helpers in the form of a chain of container_of.

The active_state which lives in the subdev will be used as a "default"
state, to support userspace application which are not context-aware.

Would it be so bad to populate a v4l2_subdev_fh * in the active state
stored in the subdev to indicate on which file handle an ioctl has
been called on and retrieve the caps from there, so that the same
(name tbd) v4l2_subdev_get_state_caps(struct v4l2_subdev_state *state)
could be used ?

Adding the caps argument to driver's ioctl handlers to me pollutes the
interface diminishing the value of the state-centric interface we are
trying to implement.

> --
> Kind regards,
>
> Sakari Ailus