Re: [PATCH v7 11/14] media: v4l2-subdev: Add v4l2_subdev_call_ci_state_{active,try}

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Aug 10, 2026 at 04:12:31PM +0200, Hans Verkuil wrote:
> On 07/08/2026 14:24, Sakari Ailus wrote:
> > Add v4l2_subdev_call_ci_state_active(), and
> > v4l2_subdev_call_ci_state_try() to call sub-device pad ops that
> > take struct v4l2_subdev_client_info pointer as an argument. These ops
> > cannot be called using v4l2_subdev_call_state_active() or
> > v4l2_subdev_call_state_try() as the client_info argument precedes the
> > state argument.
> 
> So if we have to jump through all these hoops just because the client_info
> pointer precedes the state pointer in the pad op argument list, wouldn't it
> be better to swap the order? For example by moving the client_info pointer
> as the last argument?
> 
> Honestly, these macros are getting really hard to follow, and I'm not sure
> it is worth it just to keep the client_info before the state pointer. Yes, that's
> the logical order, but at the price of some very hard to read defines.
> 
> Or am I missing something?

Those are exactly the points I raised in the review of v6 :-)

> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/v4l2-subdev.h | 95 +++++++++++++++++++++++++++++--------
> >  1 file changed, 74 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index b8874eeec2e5..307655ee25e0 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1964,6 +1964,22 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> >  		__result;						\
> >  	})
> >  
> > +#define __v4l2_subdev_call_drop_fourth(first, second, third, fourth, rest...) \
> > +	v4l2_subdev_call(first, second, third, ##rest)
> > +
> > +#define __v4l2_subdev_call_state_active(call, sd, o, f, args...) \
> > +	({								\
> > +		int __result;						\
> > +		struct v4l2_subdev_state *state;			\
> > +		state = v4l2_subdev_get_unlocked_active_state(sd);	\
> > +		if (state)						\
> > +			v4l2_subdev_lock_state(state);			\
> > +		__result = call(sd, o, f, NULL, state, ##args);		\
> > +		if (state)						\
> > +			v4l2_subdev_unlock_state(state);		\
> > +		__result;						\
> > +	})
> > +
> >  /**
> >   * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
> >   *				   takes state as a parameter, passing the
> > @@ -1982,22 +1998,13 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> >   * active state, lock it before calling the op and unlock it after the call.
> >   */
> >  #define v4l2_subdev_call_state_active(sd, o, f, args...)		\
> > -	({								\
> > -		int __result;						\
> > -		struct v4l2_subdev_state *state;			\
> > -		state = v4l2_subdev_get_unlocked_active_state(sd);	\
> > -		if (state)						\
> > -			v4l2_subdev_lock_state(state);			\
> > -		__result = v4l2_subdev_call(sd, o, f, state, ##args);	\
> > -		if (state)						\
> > -			v4l2_subdev_unlock_state(state);		\
> > -		__result;						\
> > -	})
> > +	__v4l2_subdev_call_state_active(__v4l2_subdev_call_drop_fourth,	\
> > +					sd, o, f, ##args)
> >  
> >  /**
> > - * v4l2_subdev_call_state_try - call an operation of a v4l2_subdev which
> > - *				takes state as a parameter, passing the
> > - *				subdev a newly allocated try state.
> > + * v4l2_subdev_call_ci_state_active - call an operation of a v4l2_subdev which
> > + *				      takes state as a parameter, passing the
> > + *				      subdev its active state.
> >   *
> >   * @sd: pointer to the &struct v4l2_subdev
> >   * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> > @@ -2007,13 +2014,14 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> >   *     each element at &struct v4l2_subdev_ops.
> >   * @args: arguments for @f.
> >   *
> > - * This is similar to v4l2_subdev_call_state_active(), except that as this
> > - * version allocates a new state, this is only usable for
> > - * V4L2_SUBDEV_FORMAT_TRY use cases.
> > - *
> > - * Note: only legacy non-MC drivers may need this macro.
> > + * This macro is just as v4l2_subdev_call_state_active(), with the exception
> > + * that it passes NULL as the client info to sub-device ops that need it
> > + * (currently pad ops set_fmt, get_selection and set_selection).
> >   */
> > -#define v4l2_subdev_call_state_try(sd, o, f, args...)                         \
> > +#define v4l2_subdev_call_ci_state_active(sd, o, f, args...)		\
> > +	__v4l2_subdev_call_state_active(v4l2_subdev_call, sd, o, f, ##args)
> > +
> > +#define __v4l2_subdev_call_state_try(call, sd, o, f, args...)		      \
> >  	({                                                                    \
> >  		int __result;                                                 \
> >  		static struct lock_class_key __key;                           \
> > @@ -2025,13 +2033,58 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> >  			__result = PTR_ERR(state);                            \
> >  		} else {                                                      \
> >  			v4l2_subdev_lock_state(state);                        \
> > -			__result = v4l2_subdev_call(sd, o, f, state, ##args); \
> > +			__result = call(sd, o, f, NULL, state, ##args);	      \
> >  			v4l2_subdev_unlock_state(state);                      \
> >  			__v4l2_subdev_state_free(state);                      \
> >  		}                                                             \
> >  		__result;                                                     \
> >  	})
> >  
> > +/**
> > + * v4l2_subdev_call_state_try - call an operation of a v4l2_subdev which
> > + *				takes state as a parameter, passing the
> > + *				subdev a newly allocated try state.
> > + *
> > + * @sd: pointer to the &struct v4l2_subdev
> > + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> > + *     Each element there groups a set of callbacks functions.
> > + * @f: callback function to be called.
> > + *     The callback functions are defined in groups, according to
> > + *     each element at &struct v4l2_subdev_ops.
> > + * @args: arguments for @f.
> > + *
> > + * This is similar to v4l2_subdev_call_state_active(), except that as this
> > + * version allocates a new state, this is only usable for
> > + * V4L2_SUBDEV_FORMAT_TRY use cases.
> > + *
> > + * Note: only legacy non-MC drivers may need this macro.
> > + */
> > +#define v4l2_subdev_call_state_try(sd, o, f, args...)			      \
> > +	__v4l2_subdev_call_state_try(__v4l2_subdev_call_drop_fourth, sd, o, f, \
> > +				     ##args);
> > +
> > +/**
> > + * v4l2_subdev_call_ci_state_try - call an operation of a v4l2_subdev which
> > + *				   takes state as a parameter, passing the
> > + *				   subdev a newly allocated try state.
> > + *
> > + * @sd: pointer to the &struct v4l2_subdev
> > + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> > + *     Each element there groups a set of callbacks functions.
> > + * @f: callback function to be called.
> > + *     The callback functions are defined in groups, according to
> > + *     each element at &struct v4l2_subdev_ops.
> > + * @args: arguments for @f.
> > + *
> > + * This is similar to v4l2_subdev_call_state_try(), with the exception
> > + * that it passes NULL as the client info to sub-device ops that need it
> > + * (currently pad ops set_fmt, get_selection and set_selection).
> > + *
> > + * Note: only legacy non-MC drivers may need this macro.
> > + */
> > +#define v4l2_subdev_call_ci_state_try(sd, o, f, args...)		     \
> > +	__v4l2_subdev_call_state_try(v4l2_subdev_call, sd, o, f, ##args);
> > +
> >  /**
> >   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
> >   *

-- 
Regards,

Laurent Pinchart