Hi Tomi,
On Thu, Feb 17, 2022 at 06:05:18AM +0200, Tomi Valkeinen wrote:
> On 16/02/2022 23:38, Laurent Pinchart wrote:
> > On Wed, Feb 16, 2022 at 03:00:47PM +0200, Tomi Valkeinen wrote:
> >> All suitable subdev ops are now passed either the TRY or the ACTIVE
> >> state by the v4l2 core. However, other subdev drivers can still call the
> >> ops passing NULL as the state, implying the active case.
> >>
> >> For all current upstream drivers this doesn't matter, as they do not
> >> expect to get a valid state for ACTIVE case. But future drivers which
> >> support multiplexed streaming and routing will depend on getting a state
> >> for both active and try cases.
> >>
> >> For new drivers we can mandate that the pipelines where the drivers are
> >> used need to pass the state properly, or preferably, not call such
> >> subdev ops at all.
> >>
> >> However, if an existing subdev driver is changed to support multiplexed
> >> streams, the driver has to consider cases where its ops will be called
> >> with NULL state. The problem can easily be solved by using the
> >> v4l2_subdev_lock_and_return_state() helper, introduced here.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >> ---
> >> include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 38 insertions(+)
> >>
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 8d089a2dbd32..84c02488d53f 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> >> */
> >> void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> >>
> >> +/**
> >> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
> >> + * @sd: subdevice
> >> + * @state: subdevice state as passed to the subdev op
> >> + *
> >> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
> >> + * NULL as the state parameter, as subdevs always used to have their active
> >> + * state stored privately.
> >> + *
> >> + * However, newer state-aware subdev drivers, which store their active state in
> >> + * a common place, subdev->active_state, expect to always get a proper state as
> >> + * a parameter.
> >> + *
> >> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
> >> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
> >> + * issue by using subdev->active_state in case the passed state is NULL.
> >> + *
> >> + * This is a temporary helper function, and should be removed when we can ensure
> >> + * that all drivers pass proper state when calling other subdevs.
> >> + */
> >> +static inline struct v4l2_subdev_state *
> >> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_state *state)
> >> +{
> >> + if (state)
> >> + return state;
> >> +
> >> + dev_notice_once(sd->dev,
> >> + "The provided state is NULL. Adjusting to the subdev active state.\n"
> >> + "Please consider porting your driver to the new state management API.\n");
> >> +
> >> + state = sd->active_state;
> >> +
> >> + v4l2_subdev_lock_state(state);
> >
> > This looks strange, if the state parameter is not NULL then you don't
> > lock the state, otherwise you do. How is the caller of this function
> > expected to unlock the state ? I'm tempted to drop this helper and push
> > harder for porting drivers to the new API.
>
> Indeed, I didn't think it through. Handling this with the new locking
> model is a bit more difficult. The driver could do something like this:
>
> static int my_subdev_op(sd, _state)
> {
> state = _state ? _state : v4l2_subdev_lock_active_state();
>
> ...
>
> if (!_state)
> v4l2_subdev_unlock_state(state);
>
> return 0;
> }
>
> But hiding that behind a helper is not so easy.
>
> Perhaps it is better to drop this, and change the calling subdev drivers
> to use the v4l2_subdev_call_state_active() helper.
I think that would be better, yes.
--
Regards,
Laurent Pinchart
