Hi Tomi,
Thank you for the patch.
On Mon, Aug 30, 2021 at 02:00:46PM +0300, Tomi Valkeinen wrote:
> All suitable subdev ops are now passed either the TRY or the ACTIVE
> state by the v4l2 core. However, other subdrivers can still call the ops
> passing NULL as the state, implying the active case.
>
> Thus all subdev drivers supporting active state need to handle the NULL
> state case.
Do they ? Can't we mandate that the callers pass the correct state ? Do
you think that would make the transition too difficult ?
The way I understand the issue, this would only be needed to facilitate
the transition. Is there a reason why the drivers you've ported (CAL &
co.) use v4l2_subdev_validate_and_lock_state() after completing the
transition, or is the correct state always passed by the caller ?
> Additionally, the subdev drivers usually need to lock the
> state.
>
> Add two helper functions to easen the transition to centrally managed
> ACTIVE state. v4l2_subdev_validate_state() ensures that the state is not
> NULL, and v4l2_subdev_validate_and_lock_state() does the same and
> additionally locks the state.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> include/media/v4l2-subdev.h | 41 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 52a725281b23..2290b5025fc0 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1307,4 +1307,45 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> */
> void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>
> +/**
> + * v4l2_subdev_validate_state() - Gets the TRY or ACTIVE subdev state
> + * @sd: subdevice
> + * @state: subdevice state as passed to the subdev op
> + *
> + * Subdev ops used to be sometimes called with NULL as the state for ACTIVE
> + * case. Even if the v4l2 core now passes proper state for both TRY and
> + * ACTIVE cases, a subdev driver may call an op in another subdev driver,
> + * passing NULL.
> + *
> + * This function can be used as a helper to get the state also for the ACTIVE
> + * case. The subdev driver that supports ACTIVE state can use this function
> + * as the first thing in its ops, ensuring that the state variable contains
> + * either the TRY or ACTIVE state.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_validate_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + return state ? state : sd->state;
> +}
This doesn't seem to be used by the drivers you've ported, or by the
R-Car and GMSL patches from Jacopo. Do we need this function ?
> +
> +/**
> + * v4l2_subdev_validate_and_lock_state() - Gets locked TRY or ACTIVE subdev state
> + * @sd: subdevice
> + * @state: subdevice state as passed to the subdev op
> + *
> + * This is a helper function which does the same as v4l2_subdev_validate_state
> + * () except that it also locks the state.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_validate_and_lock_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + state = state ? state : sd->state;
> +
> + v4l2_subdev_lock_state(state);
> +
> + return state;
> +}
> +
> #endif
--
Regards,
Laurent Pinchart
