Hi Tomi,
On Tue, Sep 28, 2021 at 08:02:18AM +0300, Tomi Valkeinen wrote:
> On 27/09/2021 04:45, Laurent Pinchart wrote:
> > 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 ?
>
> That would limit the use of the new drivers. E.g. the sensor driver I'm
> using works fine with CAL & co. without handling the NULL, but then the
> sensor driver couldn't be used with "old" drivers.
I'm tempted to say that would be a good thing as it would accelerate the
transition :-)
> > 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 ?
>
> The drivers I'm using only call non-state-aware ops in the other
> subdevs, so they should work fine without validate.
>
> I think it's safer to just use the validate versions for now. They're
> trivial to convert to non-validate versions later.
>
> We could just do the validate implicitly while locking the state, but it
> would make the code a bit odd:
>
> state = v4l2_subdev_lock_state(state);
>
> After the transition we want to do just:
>
> v4l2_subdev_lock_state(state);
>
> Or we could do v4l2_subdev_lock_state() with a macro, which changes the
> state variable, but... I think I'd rather have it clear and obvious with
> v4l2_subdev_validate_and_lock_state().
>
> >> 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 ?
>
> Probably not. If you need to validate, you most likely will lock the
> state right afterwards, so v4l2_subdev_validate_and_lock_state should be
> enough.
--
Regards,
Laurent Pinchart
