Hi Tomi, On Fri, Dec 17, 2021 at 01:18:35PM +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> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > --- > include/media/v4l2-subdev.h | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 80cd0ffb60d1..4bc3cc04cc6e 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -8,6 +8,7 @@ > #ifndef _V4L2_SUBDEV_H > #define _V4L2_SUBDEV_H > > +#include "linux/dev_printk.h" > #include <linux/types.h> > #include <linux/v4l2-subdev.h> > #include <media/media-entity.h> > @@ -1321,4 +1322,38 @@ 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) > +{ > + if (!state) > + dev_notice_once(sd->dev, "source subdev should be ported to new state management\n"); I agree the message is improvable. Maybe dev_notice_once(sd->dev, "The provided state is NULL. Adjusting to the subdev active state.\n"); dev_notice_once(sd->dev, "Please consid er porting your driver to the new state management API.\n); I don't mind multiple messages, other might. Anyway, as long as we warn, I'm fine with this helper! Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> Thanks j > + > + state = state ? state : sd->active_state; > + > + v4l2_subdev_lock_state(state); > + > + return state; > +} > + > #endif > -- > 2.25.1 >