Hi Tomi,
Thank you for the patch.
On Wed, Feb 16, 2022 at 03:00:48PM +0200, Tomi Valkeinen wrote:
> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> that take a subdev state as a parameter.
We should not encourage subdev drivers to call into each other. There
may be some valid use cases for such a helper at this point, namely in
the .start_streaming() implementation in a vb2 queue, but even then, I
think it would be best to rework the pipeline start API to lock the
states of all subdevs in the pipeline (I've already improved pipeline
start on top of your streams series, the locking isn't there yet, but I
could give it a try).
On the other hand, this macro could help identifying drivers that handle
locking manually, helping reworking them once the pipeline start API
handles locking too.
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> include/media/v4l2-subdev.h | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 84c02488d53f..0db61203c8d9 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1359,6 +1359,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> __result; \
> })
>
> +/**
> + * v4l2_subdev_call_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.
> + * 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(), except that this version can only be
> + * used for ops that take a subdev state as a parameter. The macro will get the
> + * active state and 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_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_has_op - Checks if a subdev defines a certain operation.
> *
--
Regards,
Laurent Pinchart
