Hi Laurent,
On Fri, Dec 17, 2021 at 07:54:38PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Fri, Dec 17, 2021 at 07:43:55PM +0200, Sakari Ailus wrote:
> > On Thu, Dec 16, 2021 at 04:34:22PM +0200, Laurent Pinchart wrote:
> > > On Tue, Nov 30, 2021 at 04:15:03PM +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 | 31 +++++++++++++++++++++++++++++++
> > > > 1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index 1810dde9c7fc..873bbe0686e3 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -1317,4 +1317,35 @@ 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->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)
> > > > +{
> > > > + state = state ? state : sd->active_state;
> > >
> > > Can we add a dev_warn() when state is NULL ? This will help speeding up
> > > the transition.
> >
> > Wouldn't this produce lots of warnings? I'd rather use dev_warn_once() to
> > avoid flooding logs.
>
> The goal is to notice the issue, to get it fixed, so I'd prefer a few
> warnings instead of a dev_warn_once().
This wouldn't be a few, but quite possibly system logs full of such lines
unless some kind of limiting factor is in place.
>
> Please note that the first 6 patches from this series have been posted
> in a new version as "[PATCH v2 0/6] v4l: subdev active state"
> (20211217135022.364954-1-tomi.valkeinen@xxxxxxxxxxxxxxxx).
Yes, I see this is addressed in v2.
--
Sakari Ailus
