| Message ID | 20210830110116.488338-7-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l: subdev internal routing and streams | expand |
Hi Tomi, Thank you for the patch. 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 ? 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 ? > 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@ideasonboard.com> > --- > 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 ? > + > +/** > + * v4l2_subdev_validate_and_lock_state() - Gets locked TRY or ACTIVE subdev state > + * @sd: subdevice > + * @state: subdevice state as passed to the subdev op > + * > + * This is a helper function which does the same as v4l2_subdev_validate_state > + * () except that it also locks the state. > + */ > +static inline struct v4l2_subdev_state * > +v4l2_subdev_validate_and_lock_state(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + state = state ? state : sd->state; > + > + v4l2_subdev_lock_state(state); > + > + return state; > +} > + > #endif -- Regards, Laurent Pinchart
On 27/09/2021 04:45, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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. > 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@ideasonboard.com> >> --- >> 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. Tomi
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@ideasonboard.com> > >> --- > >> 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
Hi Laurent, On 28/09/2021 10:52, Laurent Pinchart wrote: > 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 :-) I tested this, and as expected the drivers I'm using work fine without the "validate" version. From that perspective I'm fine with dropping the whole "validate" concept, and just require state to be valid for the new multiplexed-streams-enabled drivers. But the thing is, all the drivers I use are new, and not used in other hardware platforms or configurations. However, if someone ports an existing driver to multiplexed streams, either he has to be sure no other setup is using that driver, or add some kind of "validate" system. Maybe that's still ok. I don't expect people to be rushing to port the drivers to multiplexed streams =). So if there are no complaints, I'll drop the validate functions. And we can always add them back later for the few drivers that need them, if the plan goes bad... Tomi
Hi Tomi, On Wed, Sep 29, 2021 at 06:35:30PM +0300, Tomi Valkeinen wrote: > On 28/09/2021 10:52, Laurent Pinchart wrote: > > 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 :-) > > I tested this, and as expected the drivers I'm using work fine without > the "validate" version. From that perspective I'm fine with dropping the > whole "validate" concept, and just require state to be valid for the new > multiplexed-streams-enabled drivers. > > But the thing is, all the drivers I use are new, and not used in other > hardware platforms or configurations. However, if someone ports an > existing driver to multiplexed streams, either he has to be sure no > other setup is using that driver, or add some kind of "validate" system. > > Maybe that's still ok. I don't expect people to be rushing to port the > drivers to multiplexed streams =). > > So if there are no complaints, I'll drop the validate functions. And we > can always add them back later for the few drivers that need them, if > the plan goes bad... I tend to try and push for faster transitions, so I like this, but I also understand your concern. For new drivers at least, I think we shouldn't use the validate version. For existing drivers moved to the new API, it may be more difficult to enforce that. I'd like to hear what Hans and Sakari think about it, and I'll go with the majority. Can you at least document the validate function as being temporary, and not to be used for new drivers ? -- Regards, Laurent Pinchart
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; +} + +/** + * v4l2_subdev_validate_and_lock_state() - Gets locked TRY or ACTIVE subdev state + * @sd: subdevice + * @state: subdevice state as passed to the subdev op + * + * This is a helper function which does the same as v4l2_subdev_validate_state + * () except that it also locks the state. + */ +static inline struct v4l2_subdev_state * +v4l2_subdev_validate_and_lock_state(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state) +{ + state = state ? state : sd->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 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. 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@ideasonboard.com> --- include/media/v4l2-subdev.h | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)