| Message ID | 20211130141536.891878-6-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l: subdev internal routing and streams | expand |
Hi Tomi, Thank you for the patch. 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@ideasonboard.com> > --- > 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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + v4l2_subdev_lock_state(state); > + > + return state; > +} > + > #endif
On 30/11/2021 15:15, 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@ideasonboard.com> > --- > 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. Should be: by using subdev->active_state With that change: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, hans > + * > + * 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; > + > + v4l2_subdev_lock_state(state); > + > + return state; > +} > + > #endif >
Hi Laurent, On Thu, Dec 16, 2021 at 04:34:22PM +0200, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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@ideasonboard.com> > > --- > > 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. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > + v4l2_subdev_lock_state(state); > > + > > + return state; > > +} > > + > > #endif >
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@ideasonboard.com> > > > --- > > > 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(). 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@ideasonboard.com). > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + > > > + v4l2_subdev_lock_state(state); > > > + > > > + return state; > > > +} > > > + > > > #endif
Hi Hans, On Fri, Dec 17, 2021 at 12:48:56PM +0100, Hans Verkuil wrote: > On 30/11/2021 15:15, 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@ideasonboard.com> > > --- > > 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. > > Should be: by using subdev->active_state > > With that change: > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@ideasonboard.com). > > + * > > + * 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; > > + > > + v4l2_subdev_lock_state(state); > > + > > + return state; > > +} > > + > > #endif > >
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@ideasonboard.com> > > > > --- > > > > 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@ideasonboard.com). Yes, I see this is addressed in v2.
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; + + v4l2_subdev_lock_state(state); + + return state; +} + #endif
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@ideasonboard.com> --- include/media/v4l2-subdev.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)