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

Laurent Pinchart
2026-04-16T17:30:20Z

On Mon, Apr 13, 2026 at 02:42:03PM +0200, Jacopo Mondi wrote:
> On Mon, Apr 13, 2026 at 11:11:33AM +0300, Sakari Ailus wrote:
> > On Fri, Apr 10, 2026 at 03:31:32PM +0200, Jacopo Mondi wrote:
> > > 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.

Storing the client caps in the state would be a hack indeed. The state
is meant to be an object that represents the state of the device.

Using it to store caps of a client means we'll need to ensure at least
that no two clients will ever use the same state at the same time (which
only applies to the active state), and that a state will always be
associated with a client when a subdev operation is called.

To ensure the former, we need to lock the active state before setting
the caps field in the state, and keep it locked to call the subdev
operation is called.

The current implementation of the subdev ioctl handling code complies
with those requirements.

For the latter, it largely depends on which operation we're talking
about. This series only adds the v4l2_subdev_client_info pointer to four
operations: .get_fmt(), .set_fmt(), .get_selection() and
.set_selection(). There are very few state-aware drivers calling the
format operations manually on a subdev managed by a different driver,
and even less calling the selection operations, and I don't expect that
list to grow.

Storing the caps in the state would be doable today, but I'm concerned
it will cause issues later and restrict us in our ability to use subdev
states. It could however be argued that introducing the new
v4l2_subdev_client_info argument could be delayed to that point in time.

All this being said, the only usage of client caps in subdev drivers
today is conversion of existing sensor drivers to the new raw camera
sensor model without breaking userspace. We have decided to propagate
the V4L2_SUBDEV_CLIENT_CAP_COMMON_RAW_SENSOR capability to drivers to
handle this, because we didn't find a way to handle it in the subdev
core, but maybe there's a better solution we overlooked ?

By the way, I don't think passing the client info pointer to the
.get_fmt() operation is needed.

> 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 ?

I think that will cause trouble. States can be accessed directly from a
client context (e.g. when servicing a subdev ioctl), but also from other
contexts (e.g. pipeline validation when starting streaming). In the
latter case no operation requiring client caps should be called. If we
store client caps in the state, I fear we'll start accessing them from
places where they shouldn't be accessed, and it would become hard to
untangle it later.

> 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.

It's still state-centric.

--
Regards,

Laurent Pinchart