| Message ID | 20210830110116.488338-3-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l: subdev internal routing and streams | expand |
Hi Tomi, On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: > Add a new 'state' field to struct v4l2_subdev to which we can store the > active state of a subdev. This will place the subdev configuration into > a known place, allowing us to use the state directly from the v4l2 > framework, thus simplifying the drivers. > > We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), > which need to be used by the drivers that support subdev state in struct > v4l2_subdev. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ > include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 26a34a8e3d37..e1a794f69815 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, > v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); > } > EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); > + > +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > +{ > + struct v4l2_subdev_state *state; > + > + state = v4l2_alloc_subdev_state(sd); > + if (IS_ERR(state)) > + return PTR_ERR(state); > + > + sd->state = state; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); > + > +void v4l2_subdev_free_state(struct v4l2_subdev *sd) > +{ > + v4l2_free_subdev_state(sd->state); > + sd->state = NULL; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 8701d2e7d893..ecaf040ead57 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -898,6 +898,8 @@ struct v4l2_subdev_platform_data { > * @subdev_notifier: A sub-device notifier implicitly registered for the sub- > * device using v4l2_async_register_subdev_sensor(). > * @pdata: common part of subdevice platform data > + * @state: active state for the subdev (NULL for subdevs tracking the state > + * internally) > * > * Each instance of a subdev driver should create this struct, either > * stand-alone or embedded in a larger struct. > @@ -929,6 +931,7 @@ struct v4l2_subdev { > struct v4l2_async_notifier *notifier; > struct v4l2_async_notifier *subdev_notifier; > struct v4l2_subdev_platform_data *pdata; > + struct v4l2_subdev_state *state; Is there anything preventing state from being a struct member and only allocate the required number of v4l2_subdev_pad_config entries ? > }; > > > @@ -1217,4 +1220,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers; > void v4l2_subdev_notify_event(struct v4l2_subdev *sd, > const struct v4l2_event *ev); > > +/** > + * v4l2_subdev_alloc_state() - Allocate active subdev state for subdevice > + * @sd: The subdev for which the state is allocated > + * > + * This will allocate a subdev state and store it to > + * &struct v4l2_subdev->state. > + * > + * Must call v4l2_subdev_free_state() when the state is no longer needed. > + */ > +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd); > + > +/** > + * v4l2_subdev_free_state() - Free the active subdev state for subdevice > + * @sd: The subdevice > + * > + * This will free the subdev's state and set > + * &struct v4l2_subdev->state to NULL. > + */ > +void v4l2_subdev_free_state(struct v4l2_subdev *sd); > + > +/** > + * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice > + * @sd: The subdevice > + * > + * Return the active state for the subdevice, or NULL if the subdev does not > + * support active state. > + */ > +static inline struct v4l2_subdev_state * > +v4l2_subdev_get_active_state(struct v4l2_subdev *sd) > +{ > + return sd->state; > +} It would also make safer to access sd->state, as if a driver doesn't allocate a state but calls this function it would get back a NULL pointer. Also, the name 'active' suggests there will be a non-active state ? Thanks j > + > #endif > -- > 2.25.1 >
Hi, On 13/09/2021 13:57, Jacopo Mondi wrote: > Hi Tomi, > > On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: >> Add a new 'state' field to struct v4l2_subdev to which we can store the >> active state of a subdev. This will place the subdev configuration into >> a known place, allowing us to use the state directly from the v4l2 >> framework, thus simplifying the drivers. >> >> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), >> which need to be used by the drivers that support subdev state in struct >> v4l2_subdev. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ >> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ >> 2 files changed, 57 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 26a34a8e3d37..e1a794f69815 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, >> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); >> } >> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); >> + >> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) >> +{ >> + struct v4l2_subdev_state *state; >> + >> + state = v4l2_alloc_subdev_state(sd); >> + if (IS_ERR(state)) >> + return PTR_ERR(state); >> + >> + sd->state = state; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); >> + >> +void v4l2_subdev_free_state(struct v4l2_subdev *sd) >> +{ >> + v4l2_free_subdev_state(sd->state); >> + sd->state = NULL; >> +} >> +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 8701d2e7d893..ecaf040ead57 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -898,6 +898,8 @@ struct v4l2_subdev_platform_data { >> * @subdev_notifier: A sub-device notifier implicitly registered for the sub- >> * device using v4l2_async_register_subdev_sensor(). >> * @pdata: common part of subdevice platform data >> + * @state: active state for the subdev (NULL for subdevs tracking the state >> + * internally) >> * >> * Each instance of a subdev driver should create this struct, either >> * stand-alone or embedded in a larger struct. >> @@ -929,6 +931,7 @@ struct v4l2_subdev { >> struct v4l2_async_notifier *notifier; >> struct v4l2_async_notifier *subdev_notifier; >> struct v4l2_subdev_platform_data *pdata; >> + struct v4l2_subdev_state *state; > > Is there anything preventing state from being a struct member and only > allocate the required number of v4l2_subdev_pad_config entries ? Perhaps nothing strictly prevents it, but having the state pointer != NULL tells us that the driver has created the state. One annoyance was that I didn't find a good place to implicitly create the state based on a subdev flag. Instead the subdev driver needs to call v4l2_subdev_alloc_state() explicitly. So having the state as a pointer makes it clear that the state has been initialized. >> }; >> >> >> @@ -1217,4 +1220,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers; >> void v4l2_subdev_notify_event(struct v4l2_subdev *sd, >> const struct v4l2_event *ev); >> >> +/** >> + * v4l2_subdev_alloc_state() - Allocate active subdev state for subdevice >> + * @sd: The subdev for which the state is allocated >> + * >> + * This will allocate a subdev state and store it to >> + * &struct v4l2_subdev->state. >> + * >> + * Must call v4l2_subdev_free_state() when the state is no longer needed. >> + */ >> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd); >> + >> +/** >> + * v4l2_subdev_free_state() - Free the active subdev state for subdevice >> + * @sd: The subdevice >> + * >> + * This will free the subdev's state and set >> + * &struct v4l2_subdev->state to NULL. >> + */ >> +void v4l2_subdev_free_state(struct v4l2_subdev *sd); >> + >> +/** >> + * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice >> + * @sd: The subdevice >> + * >> + * Return the active state for the subdevice, or NULL if the subdev does not >> + * support active state. >> + */ >> +static inline struct v4l2_subdev_state * >> +v4l2_subdev_get_active_state(struct v4l2_subdev *sd) >> +{ >> + return sd->state; >> +} > > It would also make safer to access sd->state, as if a driver doesn't > allocate a state but calls this function it would get back a NULL > pointer. Would WARN_ON(!sd->state) be ok? > Also, the name 'active' suggests there will be a non-active state ? It's the V4L2_SUBDEV_FORMAT_ACTIVE vs V4L2_SUBDEV_FORMAT_TRY. For TRY we don't need getters, as the TRY state is passed directly to the relevant ops. But the drivers need to get the ACTIVE state, e.g. when starting the streaming. Tomi
Hi Tomi, On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: > Add a new 'state' field to struct v4l2_subdev to which we can store the > active state of a subdev. This will place the subdev configuration into > a known place, allowing us to use the state directly from the v4l2 > framework, thus simplifying the drivers. > > We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), > which need to be used by the drivers that support subdev state in struct > v4l2_subdev. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ > include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 26a34a8e3d37..e1a794f69815 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, > v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); > } > EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); > + > +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > +{ > + struct v4l2_subdev_state *state; > + > + state = v4l2_alloc_subdev_state(sd); So, I think this is one source of confusion about init_cfg. v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver are now supposed to allocate their state by calling v4l2_subdev_alloc_state(), in the same way as the core does for the file-handle ones. This will lead to init_cfg to be called for the 'active' (ie owned by the subdev) state, and then you need to add context to the state (by adding a 'which' field) to know what state you're dealing with. According to the init_cfg() documentation * @init_cfg: initialize the pad config to default values the op has to be called in order to initialize the per-file-handle context, not the active one. I would rather just embed 'struct v4l2_subdev_state' in 'struct v4l2_subdev', have the core going through the 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have drivers initialize their own state at probe time. If they need for some reason to access their 'active' state at init_cfg() time, they caan fish it from their subdev. If I'm not mistaken this will remove the need to have a which filed in the state, as I think the 'context' should be inferred from the 'which' argument embedded in the ops-specific structures, and not held in the state itself. Thanks j > + if (IS_ERR(state)) > + return PTR_ERR(state); > + > + sd->state = state; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); > + > +void v4l2_subdev_free_state(struct v4l2_subdev *sd) > +{ > + v4l2_free_subdev_state(sd->state); > + sd->state = NULL; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 8701d2e7d893..ecaf040ead57 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -898,6 +898,8 @@ struct v4l2_subdev_platform_data { > * @subdev_notifier: A sub-device notifier implicitly registered for the sub- > * device using v4l2_async_register_subdev_sensor(). > * @pdata: common part of subdevice platform data > + * @state: active state for the subdev (NULL for subdevs tracking the state > + * internally) > * > * Each instance of a subdev driver should create this struct, either > * stand-alone or embedded in a larger struct. > @@ -929,6 +931,7 @@ struct v4l2_subdev { > struct v4l2_async_notifier *notifier; > struct v4l2_async_notifier *subdev_notifier; > struct v4l2_subdev_platform_data *pdata; > + struct v4l2_subdev_state *state; > }; > > > @@ -1217,4 +1220,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers; > void v4l2_subdev_notify_event(struct v4l2_subdev *sd, > const struct v4l2_event *ev); > > +/** > + * v4l2_subdev_alloc_state() - Allocate active subdev state for subdevice > + * @sd: The subdev for which the state is allocated > + * > + * This will allocate a subdev state and store it to > + * &struct v4l2_subdev->state. > + * > + * Must call v4l2_subdev_free_state() when the state is no longer needed. > + */ > +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd); > + > +/** > + * v4l2_subdev_free_state() - Free the active subdev state for subdevice > + * @sd: The subdevice > + * > + * This will free the subdev's state and set > + * &struct v4l2_subdev->state to NULL. > + */ > +void v4l2_subdev_free_state(struct v4l2_subdev *sd); > + > +/** > + * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice > + * @sd: The subdevice > + * > + * Return the active state for the subdevice, or NULL if the subdev does not > + * support active state. > + */ > +static inline struct v4l2_subdev_state * > +v4l2_subdev_get_active_state(struct v4l2_subdev *sd) > +{ > + return sd->state; > +} > + > #endif > -- > 2.25.1 >
Hi, On 15/09/2021 12:44, Jacopo Mondi wrote: > Hi Tomi, > > On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: >> Add a new 'state' field to struct v4l2_subdev to which we can store the >> active state of a subdev. This will place the subdev configuration into >> a known place, allowing us to use the state directly from the v4l2 >> framework, thus simplifying the drivers. >> >> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), >> which need to be used by the drivers that support subdev state in struct >> v4l2_subdev. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ >> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ >> 2 files changed, 57 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 26a34a8e3d37..e1a794f69815 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, >> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); >> } >> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); >> + >> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) >> +{ >> + struct v4l2_subdev_state *state; >> + >> + state = v4l2_alloc_subdev_state(sd); > > So, I think this is one source of confusion about init_cfg. > > v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver > are now supposed to allocate their state by calling > v4l2_subdev_alloc_state(), in the same way as the core does for the > file-handle ones. > > This will lead to init_cfg to be called for the 'active' (ie owned by > the subdev) state, and then you need to add context to the state (by > adding a 'which' field) to know what state you're dealing with. > > According to the init_cfg() documentation > > * @init_cfg: initialize the pad config to default values > > the op has to be called in order to initialize the per-file-handle > context, not the active one. I have missed updating the documentation there =). > I would rather just embed 'struct v4l2_subdev_state' in 'struct > v4l2_subdev', have the core going through the Why would embedding the state change anything? > 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have > drivers initialize their own state at probe time. If they need for > some reason to access their 'active' state at init_cfg() time, they > caan fish it from their subdev. > > If I'm not mistaken this will remove the need to have a which filed in > the state, as I think the 'context' should be inferred from the > 'which' argument embedded in the ops-specific structures, and not held > in the state itself. I'll comment on these in the next reply, as your second mail covered the same topics. Tomi
On 16/09/2021 09:17, Tomi Valkeinen wrote: > Hi, > > On 15/09/2021 12:44, Jacopo Mondi wrote: >> Hi Tomi, >> >> On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: >>> Add a new 'state' field to struct v4l2_subdev to which we can store the >>> active state of a subdev. This will place the subdev configuration into >>> a known place, allowing us to use the state directly from the v4l2 >>> framework, thus simplifying the drivers. >>> >>> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), >>> which need to be used by the drivers that support subdev state in struct >>> v4l2_subdev. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ >>> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ >>> 2 files changed, 57 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c >>> b/drivers/media/v4l2-core/v4l2-subdev.c >>> index 26a34a8e3d37..e1a794f69815 100644 >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev >>> *sd, >>> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); >>> } >>> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); >>> + >>> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) >>> +{ >>> + struct v4l2_subdev_state *state; >>> + >>> + state = v4l2_alloc_subdev_state(sd); Replying to this again, as the second email didn't actually cover all the topics... >> So, I think this is one source of confusion about init_cfg. >> >> v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver >> are now supposed to allocate their state by calling >> v4l2_subdev_alloc_state(), in the same way as the core does for the >> file-handle ones. >> >> This will lead to init_cfg to be called for the 'active' (ie owned by >> the subdev) state, and then you need to add context to the state (by >> adding a 'which' field) to know what state you're dealing with. >> >> According to the init_cfg() documentation >> >> * @init_cfg: initialize the pad config to default values >> >> the op has to be called in order to initialize the per-file-handle >> context, not the active one. > > I have missed updating the documentation there =). The documentation above doesn't imply per-file-handle context or TRY case, afaics. It just says "initialize state to default". Unless "pad config" always means TRY, which I think it doesn't as the drivers have internally been using pad configs. But it's true that so far init_cfg has only been called for TRY case, and perhaps that's enough of a reason to keep it so. >> I would rather just embed 'struct v4l2_subdev_state' in 'struct >> v4l2_subdev', have the core going through the > > Why would embedding the state change anything? > >> 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have >> drivers initialize their own state at probe time. If they need for >> some reason to access their 'active' state at init_cfg() time, they >> caan fish it from their subdev. >> >> If I'm not mistaken this will remove the need to have a which filed in >> the state, as I think the 'context' should be inferred from the >> 'which' argument embedded in the ops-specific structures, and not held >> in the state itself. It's true that the state's which field is mainly (probably only) needed for handling the init_cfg. It could be solved in other ways too: - New subdev op to initialize active state - New subdev op which gets 'which' as a parameter, to initialize both states (state-aware drivers wouldn't need to implement the old init_cfg) - Coccinelle to change init_cfg to get the which as a parameter Without doing any deep thinking, the middle one sounds best to me. Tomi
Hi Tomi, On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote: > On 16/09/2021 09:17, Tomi Valkeinen wrote: > > Hi, > > > > On 15/09/2021 12:44, Jacopo Mondi wrote: > > > Hi Tomi, > > > > > > On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: > > > > Add a new 'state' field to struct v4l2_subdev to which we can store the > > > > active state of a subdev. This will place the subdev configuration into > > > > a known place, allowing us to use the state directly from the v4l2 > > > > framework, thus simplifying the drivers. > > > > > > > > We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), > > > > which need to be used by the drivers that support subdev state in struct > > > > v4l2_subdev. > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > > --- > > > > drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ > > > > include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ > > > > 2 files changed, 57 insertions(+) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c > > > > b/drivers/media/v4l2-core/v4l2-subdev.c > > > > index 26a34a8e3d37..e1a794f69815 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct > > > > v4l2_subdev *sd, > > > > v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); > > > > } > > > > EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); > > > > + > > > > +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > > > > +{ > > > > + struct v4l2_subdev_state *state; > > > > + > > > > + state = v4l2_alloc_subdev_state(sd); > > Replying to this again, as the second email didn't actually cover all the > topics... > > > > So, I think this is one source of confusion about init_cfg. > > > > > > v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver > > > are now supposed to allocate their state by calling > > > v4l2_subdev_alloc_state(), in the same way as the core does for the > > > file-handle ones. > > > > > > This will lead to init_cfg to be called for the 'active' (ie owned by > > > the subdev) state, and then you need to add context to the state (by > > > adding a 'which' field) to know what state you're dealing with. > > > > > > According to the init_cfg() documentation > > > > > > * @init_cfg: initialize the pad config to default values > > > > > > the op has to be called in order to initialize the per-file-handle > > > context, not the active one. > > > > I have missed updating the documentation there =). > > The documentation above doesn't imply per-file-handle context or TRY case, > afaics. It just says "initialize state to default". Unless "pad config" > always means TRY, which I think it doesn't as the drivers have internally > been using pad configs. If they do, they would have the 'active' pad_configs allocated or embedded somewhere in their driver structures, they would not receive it as parameter. Or have I missed where the core is capable of fishing the 'right' pad_configs ? I think the same should happen for state. > > But it's true that so far init_cfg has only been called for TRY case, and > perhaps that's enough of a reason to keep it so. > > > > I would rather just embed 'struct v4l2_subdev_state' in 'struct > > > v4l2_subdev', have the core going through the > > > > Why would embedding the state change anything? > > > > > 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have > > > drivers initialize their own state at probe time. If they need for > > > some reason to access their 'active' state at init_cfg() time, they > > > caan fish it from their subdev. > > > > > > If I'm not mistaken this will remove the need to have a which filed in > > > the state, as I think the 'context' should be inferred from the > > > 'which' argument embedded in the ops-specific structures, and not held > > > in the state itself. > > It's true that the state's which field is mainly (probably only) needed for > handling the init_cfg. It could be solved in other ways too: > > - New subdev op to initialize active state > - New subdev op which gets 'which' as a parameter, to initialize both states > (state-aware drivers wouldn't need to implement the old init_cfg) > - Coccinelle to change init_cfg to get the which as a parameter > > Without doing any deep thinking, the middle one sounds best to me. Isn't it simpler if you just don't call init_cfg for the 'active' state ? Driver will initialize them at probe time and that's it, then you can remove 'which' from the state (and from routing tables too if I'm not mistaken). Thanks j > > Tomi
On 16/09/2021 11:08, Jacopo Mondi wrote: > Hi Tomi, > > On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote: >> On 16/09/2021 09:17, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 15/09/2021 12:44, Jacopo Mondi wrote: >>>> Hi Tomi, >>>> >>>> On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: >>>>> Add a new 'state' field to struct v4l2_subdev to which we can store the >>>>> active state of a subdev. This will place the subdev configuration into >>>>> a known place, allowing us to use the state directly from the v4l2 >>>>> framework, thus simplifying the drivers. >>>>> >>>>> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), >>>>> which need to be used by the drivers that support subdev state in struct >>>>> v4l2_subdev. >>>>> >>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ >>>>> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ >>>>> 2 files changed, 57 insertions(+) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c >>>>> b/drivers/media/v4l2-core/v4l2-subdev.c >>>>> index 26a34a8e3d37..e1a794f69815 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>>> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct >>>>> v4l2_subdev *sd, >>>>> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); >>>>> } >>>>> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); >>>>> + >>>>> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) >>>>> +{ >>>>> + struct v4l2_subdev_state *state; >>>>> + >>>>> + state = v4l2_alloc_subdev_state(sd); >> >> Replying to this again, as the second email didn't actually cover all the >> topics... >> >>>> So, I think this is one source of confusion about init_cfg. >>>> >>>> v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver >>>> are now supposed to allocate their state by calling >>>> v4l2_subdev_alloc_state(), in the same way as the core does for the >>>> file-handle ones. >>>> >>>> This will lead to init_cfg to be called for the 'active' (ie owned by >>>> the subdev) state, and then you need to add context to the state (by >>>> adding a 'which' field) to know what state you're dealing with. >>>> >>>> According to the init_cfg() documentation >>>> >>>> * @init_cfg: initialize the pad config to default values >>>> >>>> the op has to be called in order to initialize the per-file-handle >>>> context, not the active one. >>> >>> I have missed updating the documentation there =). >> >> The documentation above doesn't imply per-file-handle context or TRY case, >> afaics. It just says "initialize state to default". Unless "pad config" >> always means TRY, which I think it doesn't as the drivers have internally >> been using pad configs. > > If they do, they would have the 'active' pad_configs allocated or > embedded somewhere in their driver structures, they would not receive > it as parameter. Or have I missed where the core is capable of fishing > the 'right' pad_configs ? I think the same should happen for state. Yes, that is correct. I was just saying that the documentation doesn't say that init_cfg is only used for the TRY case, even if that has been the case in practice. >> But it's true that so far init_cfg has only been called for TRY case, and >> perhaps that's enough of a reason to keep it so. >> >>>> I would rather just embed 'struct v4l2_subdev_state' in 'struct >>>> v4l2_subdev', have the core going through the >>> >>> Why would embedding the state change anything? >>> >>>> 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have >>>> drivers initialize their own state at probe time. If they need for >>>> some reason to access their 'active' state at init_cfg() time, they >>>> caan fish it from their subdev. >>>> >>>> If I'm not mistaken this will remove the need to have a which filed in >>>> the state, as I think the 'context' should be inferred from the >>>> 'which' argument embedded in the ops-specific structures, and not held >>>> in the state itself. >> >> It's true that the state's which field is mainly (probably only) needed for >> handling the init_cfg. It could be solved in other ways too: >> >> - New subdev op to initialize active state >> - New subdev op which gets 'which' as a parameter, to initialize both states >> (state-aware drivers wouldn't need to implement the old init_cfg) >> - Coccinelle to change init_cfg to get the which as a parameter >> >> Without doing any deep thinking, the middle one sounds best to me. > > Isn't it simpler if you just don't call init_cfg for the 'active' > state ? Driver will initialize them at probe time and that's it, then > you can remove 'which' from the state (and from routing tables too if I'm > not mistaken). Routing needs the 'which' similarly to other config structs, like v4l2_subdev_format. I did a quick implementation for the second case, which allows me to remove 'which' from the state. As for "simpler"... Both ways have pros and cons. I'm my mind this new way is simpler. Afaics, in the end (i.e. after doing the v4l2_subdev_call change), the only bigger difference is how the state-aware drivers implement their ops. With my approach, they get the correct state and that's it. With your approach, they need to use a helper function (or do it manually) to get the state based on 'which'. Tomi
Hello, On Thu, Sep 16, 2021 at 12:36:33PM +0300, Tomi Valkeinen wrote: > On 16/09/2021 11:08, Jacopo Mondi wrote: > > On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote: > >> On 16/09/2021 09:17, Tomi Valkeinen wrote: > >>> On 15/09/2021 12:44, Jacopo Mondi wrote: > >>>> On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: > >>>>> Add a new 'state' field to struct v4l2_subdev to which we can store the > >>>>> active state of a subdev. This will place the subdev configuration into > >>>>> a known place, allowing us to use the state directly from the v4l2 > >>>>> framework, thus simplifying the drivers. > >>>>> > >>>>> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), > >>>>> which need to be used by the drivers that support subdev state in struct > >>>>> v4l2_subdev. > >>>>> > >>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>>> --- > >>>>> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ > >>>>> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ > >>>>> 2 files changed, 57 insertions(+) > >>>>> > >>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> index 26a34a8e3d37..e1a794f69815 100644 > >>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct > >>>>> v4l2_subdev *sd, > >>>>> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); > >>>>> } > >>>>> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); > >>>>> + > >>>>> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > >>>>> +{ > >>>>> + struct v4l2_subdev_state *state; > >>>>> + > >>>>> + state = v4l2_alloc_subdev_state(sd); > >> > >> Replying to this again, as the second email didn't actually cover all the > >> topics... > >> > >>>> So, I think this is one source of confusion about init_cfg. > >>>> > >>>> v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver > >>>> are now supposed to allocate their state by calling > >>>> v4l2_subdev_alloc_state(), in the same way as the core does for the > >>>> file-handle ones. > >>>> > >>>> This will lead to init_cfg to be called for the 'active' (ie owned by > >>>> the subdev) state, and then you need to add context to the state (by > >>>> adding a 'which' field) to know what state you're dealing with. > >>>> > >>>> According to the init_cfg() documentation > >>>> > >>>> * @init_cfg: initialize the pad config to default values > >>>> > >>>> the op has to be called in order to initialize the per-file-handle > >>>> context, not the active one. > >>> > >>> I have missed updating the documentation there =). > >> > >> The documentation above doesn't imply per-file-handle context or TRY case, > >> afaics. It just says "initialize state to default". Unless "pad config" > >> always means TRY, which I think it doesn't as the drivers have internally > >> been using pad configs. > > > > If they do, they would have the 'active' pad_configs allocated or > > embedded somewhere in their driver structures, they would not receive > > it as parameter. Or have I missed where the core is capable of fishing > > the 'right' pad_configs ? I think the same should happen for state. > > Yes, that is correct. I was just saying that the documentation doesn't > say that init_cfg is only used for the TRY case, even if that has been > the case in practice. > > >> But it's true that so far init_cfg has only been called for TRY case, and > >> perhaps that's enough of a reason to keep it so. > >> > >>>> I would rather just embed 'struct v4l2_subdev_state' in 'struct > >>>> v4l2_subdev', have the core going through the > >>> > >>> Why would embedding the state change anything? > >>> > >>>> 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have > >>>> drivers initialize their own state at probe time. If they need for > >>>> some reason to access their 'active' state at init_cfg() time, they > >>>> caan fish it from their subdev. > >>>> > >>>> If I'm not mistaken this will remove the need to have a which filed in > >>>> the state, as I think the 'context' should be inferred from the > >>>> 'which' argument embedded in the ops-specific structures, and not held > >>>> in the state itself. > >> > >> It's true that the state's which field is mainly (probably only) needed for > >> handling the init_cfg. It could be solved in other ways too: > >> > >> - New subdev op to initialize active state > >> - New subdev op which gets 'which' as a parameter, to initialize both states > >> (state-aware drivers wouldn't need to implement the old init_cfg) > >> - Coccinelle to change init_cfg to get the which as a parameter > >> > >> Without doing any deep thinking, the middle one sounds best to me. > > > > Isn't it simpler if you just don't call init_cfg for the 'active' > > state ? Driver will initialize them at probe time and that's it, then > > you can remove 'which' from the state (and from routing tables too if I'm > > not mistaken). > > Routing needs the 'which' similarly to other config structs, like > v4l2_subdev_format. > > I did a quick implementation for the second case, which allows me to > remove 'which' from the state. > > As for "simpler"... Both ways have pros and cons. I'm my mind this new > way is simpler. > > Afaics, in the end (i.e. after doing the v4l2_subdev_call change), the > only bigger difference is how the state-aware drivers implement their > ops. With my approach, they get the correct state and that's it. With > your approach, they need to use a helper function (or do it manually) to > get the state based on 'which'. I'd like to propose a third approach (or at least I believe it's a third one). I agree with Jacopo that the state structure should not have a which field, that's a layering violation. The state is a state, regardless of whether it holds TRY or ACTIVE data. What are the current blockers that would prevent that ? However, I think .init_cfg() should be used to initialize *all* states, both TRY and ACTIVE. That will simplify driver implementation (but centralizing the initialization in a single place) and ensure that the default value for the ACTIVE state always matches the default value for the TRY state, which I think is important. When it comes to calling v4l2_subdev_alloc_state() in drivers, I also agree with Jacopo, I'm not extremely fond of it. This should be automatic, drivers shouldn't have to care about allocating the ACTIVE state. However, I also agree with Tomi that there's no real place to do this today. I think we need to restructure subdev initialization, which currently has very few rules. This could take the form, for instance, of 1. calling v4l2_subdev_init() as an early subdev init function 2. setting fields of v4l2_subdev manually 3. initializing the media_entity embedded in the subdev 4. calling a new "initialization finalization" function that will initialize the state embedded in v4l2_subdev (as opposed to allocating it dynamically as done today) That's the simplest option as it really ressembles what we do today, and would more or less only require embedded the state in v4l2_subdev and renaming v4l2_subdev_alloc_state() to v4l2_subdev_init_done() (or similar, that's likely not a great name). A more intrusive change would restructure the subdev initialization further, by handling the media_entity initialization transparently (which would require passing more arguments to v4l2_subdev_init(), probably creating a new version of that function). That's more complex and I think it's out of scope for this series, but it may still be useful to keep the long term goal in mind to try and align with the direction. -- Regards, Laurent Pinchart
On 27/09/2021 02:58, Laurent Pinchart wrote: > Hello, > > On Thu, Sep 16, 2021 at 12:36:33PM +0300, Tomi Valkeinen wrote: >> On 16/09/2021 11:08, Jacopo Mondi wrote: >>> On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote: >>>> On 16/09/2021 09:17, Tomi Valkeinen wrote: >>>>> On 15/09/2021 12:44, Jacopo Mondi wrote: >>>>>> On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: >>>>>>> Add a new 'state' field to struct v4l2_subdev to which we can store the >>>>>>> active state of a subdev. This will place the subdev configuration into >>>>>>> a known place, allowing us to use the state directly from the v4l2 >>>>>>> framework, thus simplifying the drivers. >>>>>>> >>>>>>> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), >>>>>>> which need to be used by the drivers that support subdev state in struct >>>>>>> v4l2_subdev. >>>>>>> >>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>>>>> --- >>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ >>>>>>> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ >>>>>>> 2 files changed, 57 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> index 26a34a8e3d37..e1a794f69815 100644 >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct >>>>>>> v4l2_subdev *sd, >>>>>>> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); >>>>>>> } >>>>>>> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); >>>>>>> + >>>>>>> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) >>>>>>> +{ >>>>>>> + struct v4l2_subdev_state *state; >>>>>>> + >>>>>>> + state = v4l2_alloc_subdev_state(sd); >>>> >>>> Replying to this again, as the second email didn't actually cover all the >>>> topics... >>>> >>>>>> So, I think this is one source of confusion about init_cfg. >>>>>> >>>>>> v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver >>>>>> are now supposed to allocate their state by calling >>>>>> v4l2_subdev_alloc_state(), in the same way as the core does for the >>>>>> file-handle ones. >>>>>> >>>>>> This will lead to init_cfg to be called for the 'active' (ie owned by >>>>>> the subdev) state, and then you need to add context to the state (by >>>>>> adding a 'which' field) to know what state you're dealing with. >>>>>> >>>>>> According to the init_cfg() documentation >>>>>> >>>>>> * @init_cfg: initialize the pad config to default values >>>>>> >>>>>> the op has to be called in order to initialize the per-file-handle >>>>>> context, not the active one. >>>>> >>>>> I have missed updating the documentation there =). >>>> >>>> The documentation above doesn't imply per-file-handle context or TRY case, >>>> afaics. It just says "initialize state to default". Unless "pad config" >>>> always means TRY, which I think it doesn't as the drivers have internally >>>> been using pad configs. >>> >>> If they do, they would have the 'active' pad_configs allocated or >>> embedded somewhere in their driver structures, they would not receive >>> it as parameter. Or have I missed where the core is capable of fishing >>> the 'right' pad_configs ? I think the same should happen for state. >> >> Yes, that is correct. I was just saying that the documentation doesn't >> say that init_cfg is only used for the TRY case, even if that has been >> the case in practice. >> >>>> But it's true that so far init_cfg has only been called for TRY case, and >>>> perhaps that's enough of a reason to keep it so. >>>> >>>>>> I would rather just embed 'struct v4l2_subdev_state' in 'struct >>>>>> v4l2_subdev', have the core going through the >>>>> >>>>> Why would embedding the state change anything? >>>>> >>>>>> 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have >>>>>> drivers initialize their own state at probe time. If they need for >>>>>> some reason to access their 'active' state at init_cfg() time, they >>>>>> caan fish it from their subdev. >>>>>> >>>>>> If I'm not mistaken this will remove the need to have a which filed in >>>>>> the state, as I think the 'context' should be inferred from the >>>>>> 'which' argument embedded in the ops-specific structures, and not held >>>>>> in the state itself. >>>> >>>> It's true that the state's which field is mainly (probably only) needed for >>>> handling the init_cfg. It could be solved in other ways too: >>>> >>>> - New subdev op to initialize active state >>>> - New subdev op which gets 'which' as a parameter, to initialize both states >>>> (state-aware drivers wouldn't need to implement the old init_cfg) >>>> - Coccinelle to change init_cfg to get the which as a parameter >>>> >>>> Without doing any deep thinking, the middle one sounds best to me. >>> >>> Isn't it simpler if you just don't call init_cfg for the 'active' >>> state ? Driver will initialize them at probe time and that's it, then >>> you can remove 'which' from the state (and from routing tables too if I'm >>> not mistaken). >> >> Routing needs the 'which' similarly to other config structs, like >> v4l2_subdev_format. >> >> I did a quick implementation for the second case, which allows me to >> remove 'which' from the state. >> >> As for "simpler"... Both ways have pros and cons. I'm my mind this new >> way is simpler. >> >> Afaics, in the end (i.e. after doing the v4l2_subdev_call change), the >> only bigger difference is how the state-aware drivers implement their >> ops. With my approach, they get the correct state and that's it. With >> your approach, they need to use a helper function (or do it manually) to >> get the state based on 'which'. > > I'd like to propose a third approach (or at least I believe it's a third > one). I agree with Jacopo that the state structure should not have a > which field, that's a layering violation. The state is a state, > regardless of whether it holds TRY or ACTIVE data. What are the current > blockers that would prevent that ? Oh, I agree with that too. I didn't add the 'which' field to state because I thought it's good =). It is supposed to be temporary solution. init_cfg() is the issue here. It think I had these options: - Change init_cfg() to take 'which' as a parameter - Change init_cfg() implementations to not use 'which' - Add new version for new drivers, init_cfg_state() or such, which either gets the which as a parameter or doesn't use which. - Add 'which' to state. I chose the fourth one as it's a very small change, and can be removed easily in the future when the underlying problem is solved. > However, I think .init_cfg() should be used to initialize *all* states, > both TRY and ACTIVE. That will simplify driver implementation (but > centralizing the initialization in a single place) and ensure that the > default value for the ACTIVE state always matches the default value for > the TRY state, which I think is important. I agree. > When it comes to calling v4l2_subdev_alloc_state() in drivers, I also > agree with Jacopo, I'm not extremely fond of it. This should be > automatic, drivers shouldn't have to care about allocating the ACTIVE > state. However, I also agree with Tomi that there's no real place to do > this today. I think we need to restructure subdev initialization, which > currently has very few rules. This could take the form, for instance, of I agree with your agreeings above too. > 1. calling v4l2_subdev_init() as an early subdev init function > 2. setting fields of v4l2_subdev manually > 3. initializing the media_entity embedded in the subdev > 4. calling a new "initialization finalization" function that will > initialize the state embedded in v4l2_subdev (as opposed to > allocating it dynamically as done today) Why is the allocated/embedded relevant? The drivers won't know which one is used. > That's the simplest option as it really ressembles what we do today, and > would more or less only require embedded the state in v4l2_subdev and > renaming v4l2_subdev_alloc_state() to v4l2_subdev_init_done() (or > similar, that's likely not a great name). Yes. But I don't see embedding relevant. So we could just rename v4l2_subdev_alloc_state() and we should be there. > A more intrusive change would restructure the subdev initialization > further, by handling the media_entity initialization transparently > (which would require passing more arguments to v4l2_subdev_init(), > probably creating a new version of that function). That's more complex > and I think it's out of scope for this series, but it may still be > useful to keep the long term goal in mind to try and align with the > direction. Yes, there are many things that can be improved further. But in the context of this series, I'd like to keep the improvements such that they require no changes to the existing drivers. Tomi
Hi Tomi, On Mon, Sep 27, 2021 at 10:05:12AM +0300, Tomi Valkeinen wrote: > On 27/09/2021 02:58, Laurent Pinchart wrote: > > On Thu, Sep 16, 2021 at 12:36:33PM +0300, Tomi Valkeinen wrote: > >> On 16/09/2021 11:08, Jacopo Mondi wrote: > >>> On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote: > >>>> On 16/09/2021 09:17, Tomi Valkeinen wrote: > >>>>> On 15/09/2021 12:44, Jacopo Mondi wrote: > >>>>>> On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote: > >>>>>>> Add a new 'state' field to struct v4l2_subdev to which we can store the > >>>>>>> active state of a subdev. This will place the subdev configuration into > >>>>>>> a known place, allowing us to use the state directly from the v4l2 > >>>>>>> framework, thus simplifying the drivers. > >>>>>>> > >>>>>>> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), > >>>>>>> which need to be used by the drivers that support subdev state in struct > >>>>>>> v4l2_subdev. > >>>>>>> > >>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>>>>> --- > >>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ > >>>>>>> include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ > >>>>>>> 2 files changed, 57 insertions(+) > >>>>>>> > >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> index 26a34a8e3d37..e1a794f69815 100644 > >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct > >>>>>>> v4l2_subdev *sd, > >>>>>>> v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); > >>>>>>> } > >>>>>>> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); > >>>>>>> + > >>>>>>> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > >>>>>>> +{ > >>>>>>> + struct v4l2_subdev_state *state; > >>>>>>> + > >>>>>>> + state = v4l2_alloc_subdev_state(sd); > >>>> > >>>> Replying to this again, as the second email didn't actually cover all the > >>>> topics... > >>>> > >>>>>> So, I think this is one source of confusion about init_cfg. > >>>>>> > >>>>>> v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver > >>>>>> are now supposed to allocate their state by calling > >>>>>> v4l2_subdev_alloc_state(), in the same way as the core does for the > >>>>>> file-handle ones. > >>>>>> > >>>>>> This will lead to init_cfg to be called for the 'active' (ie owned by > >>>>>> the subdev) state, and then you need to add context to the state (by > >>>>>> adding a 'which' field) to know what state you're dealing with. > >>>>>> > >>>>>> According to the init_cfg() documentation > >>>>>> > >>>>>> * @init_cfg: initialize the pad config to default values > >>>>>> > >>>>>> the op has to be called in order to initialize the per-file-handle > >>>>>> context, not the active one. > >>>>> > >>>>> I have missed updating the documentation there =). > >>>> > >>>> The documentation above doesn't imply per-file-handle context or TRY case, > >>>> afaics. It just says "initialize state to default". Unless "pad config" > >>>> always means TRY, which I think it doesn't as the drivers have internally > >>>> been using pad configs. > >>> > >>> If they do, they would have the 'active' pad_configs allocated or > >>> embedded somewhere in their driver structures, they would not receive > >>> it as parameter. Or have I missed where the core is capable of fishing > >>> the 'right' pad_configs ? I think the same should happen for state. > >> > >> Yes, that is correct. I was just saying that the documentation doesn't > >> say that init_cfg is only used for the TRY case, even if that has been > >> the case in practice. > >> > >>>> But it's true that so far init_cfg has only been called for TRY case, and > >>>> perhaps that's enough of a reason to keep it so. > >>>> > >>>>>> I would rather just embed 'struct v4l2_subdev_state' in 'struct > >>>>>> v4l2_subdev', have the core going through the > >>>>> > >>>>> Why would embedding the state change anything? > >>>>> > >>>>>> 'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have > >>>>>> drivers initialize their own state at probe time. If they need for > >>>>>> some reason to access their 'active' state at init_cfg() time, they > >>>>>> caan fish it from their subdev. > >>>>>> > >>>>>> If I'm not mistaken this will remove the need to have a which filed in > >>>>>> the state, as I think the 'context' should be inferred from the > >>>>>> 'which' argument embedded in the ops-specific structures, and not held > >>>>>> in the state itself. > >>>> > >>>> It's true that the state's which field is mainly (probably only) needed for > >>>> handling the init_cfg. It could be solved in other ways too: > >>>> > >>>> - New subdev op to initialize active state > >>>> - New subdev op which gets 'which' as a parameter, to initialize both states > >>>> (state-aware drivers wouldn't need to implement the old init_cfg) > >>>> - Coccinelle to change init_cfg to get the which as a parameter > >>>> > >>>> Without doing any deep thinking, the middle one sounds best to me. > >>> > >>> Isn't it simpler if you just don't call init_cfg for the 'active' > >>> state ? Driver will initialize them at probe time and that's it, then > >>> you can remove 'which' from the state (and from routing tables too if I'm > >>> not mistaken). > >> > >> Routing needs the 'which' similarly to other config structs, like > >> v4l2_subdev_format. > >> > >> I did a quick implementation for the second case, which allows me to > >> remove 'which' from the state. > >> > >> As for "simpler"... Both ways have pros and cons. I'm my mind this new > >> way is simpler. > >> > >> Afaics, in the end (i.e. after doing the v4l2_subdev_call change), the > >> only bigger difference is how the state-aware drivers implement their > >> ops. With my approach, they get the correct state and that's it. With > >> your approach, they need to use a helper function (or do it manually) to > >> get the state based on 'which'. > > > > I'd like to propose a third approach (or at least I believe it's a third > > one). I agree with Jacopo that the state structure should not have a > > which field, that's a layering violation. The state is a state, > > regardless of whether it holds TRY or ACTIVE data. What are the current > > blockers that would prevent that ? > > Oh, I agree with that too. I didn't add the 'which' field to state > because I thought it's good =). It is supposed to be temporary solution. > init_cfg() is the issue here. > > It think I had these options: > > - Change init_cfg() to take 'which' as a parameter > - Change init_cfg() implementations to not use 'which' > - Add new version for new drivers, init_cfg_state() or such, which > either gets the which as a parameter or doesn't use which. > - Add 'which' to state. > > I chose the fourth one as it's a very small change, and can be removed > easily in the future when the underlying problem is solved. > > > However, I think .init_cfg() should be used to initialize *all* states, > > both TRY and ACTIVE. That will simplify driver implementation (but > > centralizing the initialization in a single place) and ensure that the > > default value for the ACTIVE state always matches the default value for > > the TRY state, which I think is important. > > I agree. That's the second option from you list above, right ? As ACTIVE state support is opt-in, it seems to me that we won't need to mass-fix drivers as part of this series if we go for this option. Am I missing something ? > > When it comes to calling v4l2_subdev_alloc_state() in drivers, I also > > agree with Jacopo, I'm not extremely fond of it. This should be > > automatic, drivers shouldn't have to care about allocating the ACTIVE > > state. However, I also agree with Tomi that there's no real place to do > > this today. I think we need to restructure subdev initialization, which > > currently has very few rules. This could take the form, for instance, of > > I agree with your agreeings above too. > > > 1. calling v4l2_subdev_init() as an early subdev init function > > 2. setting fields of v4l2_subdev manually > > 3. initializing the media_entity embedded in the subdev > > 4. calling a new "initialization finalization" function that will > > initialize the state embedded in v4l2_subdev (as opposed to > > allocating it dynamically as done today) > > Why is the allocated/embedded relevant? The drivers won't know which one > is used. Embedding will lower the number of allocations. On the other hand, as replied to another patch in this series, allocating the state dynamically may be better short term, so I'm OK with that. > > That's the simplest option as it really ressembles what we do today, and > > would more or less only require embedded the state in v4l2_subdev and > > renaming v4l2_subdev_alloc_state() to v4l2_subdev_init_done() (or > > similar, that's likely not a great name). > > Yes. But I don't see embedding relevant. So we could just rename > v4l2_subdev_alloc_state() and we should be there. > > > A more intrusive change would restructure the subdev initialization > > further, by handling the media_entity initialization transparently > > (which would require passing more arguments to v4l2_subdev_init(), > > probably creating a new version of that function). That's more complex > > and I think it's out of scope for this series, but it may still be > > useful to keep the long term goal in mind to try and align with the > > direction. > > Yes, there are many things that can be improved further. But in the > context of this series, I'd like to keep the improvements such that they > require no changes to the existing drivers. I'd propose minmizing the need of such changes, but not ruling them out completely in case the gain is worth the pain. Of course it largely depends who gets the gain and who's inflicted the pain ;-) -- Regards, Laurent Pinchart
On 27/09/2021 12:39, Laurent Pinchart wrote: >>> I'd like to propose a third approach (or at least I believe it's a third >>> one). I agree with Jacopo that the state structure should not have a >>> which field, that's a layering violation. The state is a state, >>> regardless of whether it holds TRY or ACTIVE data. What are the current >>> blockers that would prevent that ? >> >> Oh, I agree with that too. I didn't add the 'which' field to state >> because I thought it's good =). It is supposed to be temporary solution. >> init_cfg() is the issue here. >> >> It think I had these options: >> >> - Change init_cfg() to take 'which' as a parameter >> - Change init_cfg() implementations to not use 'which' >> - Add new version for new drivers, init_cfg_state() or such, which >> either gets the which as a parameter or doesn't use which. >> - Add 'which' to state. >> >> I chose the fourth one as it's a very small change, and can be removed >> easily in the future when the underlying problem is solved. >> >>> However, I think .init_cfg() should be used to initialize *all* states, >>> both TRY and ACTIVE. That will simplify driver implementation (but >>> centralizing the initialization in a single place) and ensure that the >>> default value for the ACTIVE state always matches the default value for >>> the TRY state, which I think is important. >> >> I agree. > > That's the second option from you list above, right ? Yes. I'm not 100% sure yet if it's possible to get rid of which from init_cfg, but I'll try it out. > As ACTIVE state support is opt-in, it seems to me that we won't need to > mass-fix drivers as part of this series if we go for this option. Am I Yes, I think so. I'll be wiser after I've worked on this a bit =). I think the routing needs the biggest change, as the routing table contains 'which'. But routing won't affect the current drivers. However, 'which' is quite ingrained to v4l2, I won't be too surprised if I keep finding new 'whiches' while removing it from the init_cfg call paths. Tomi
On 28/09/2021 08:14, Tomi Valkeinen wrote: > Yes. I'm not 100% sure yet if it's possible to get rid of which from > init_cfg, but I'll try it out. > >> As ACTIVE state support is opt-in, it seems to me that we won't need to >> mass-fix drivers as part of this series if we go for this option. Am I > > Yes, I think so. I'll be wiser after I've worked on this a bit =). I > think the routing needs the biggest change, as the routing table > contains 'which'. But routing won't affect the current drivers. > > However, 'which' is quite ingrained to v4l2, I won't be too surprised if > I keep finding new 'whiches' while removing it from the init_cfg call > paths. It was rather easy to get rid of 'which'. I now have v4l2_subdev_routing, which is the struct used in the uAPI: struct v4l2_subdev_routing { __u32 which; __u64 routes; __u32 num_routes; __u32 reserved[5]; }; Then I have v4l2_subdev_krouting, which is used when calling the subdev set_routing op: struct v4l2_subdev_krouting { u32 which; struct v4l2_subdev_routing_table table; }; And I have v4l2_subdev_routing_table, which is used in the various helper functions, and stored in the state: struct v4l2_subdev_routing_table { unsigned int num_routes; struct v4l2_subdev_route *routes; }; As the only use of v4l2_subdev_krouting is when calling the subdev's set_routing, I think we should just drop it and pass 'which' as a parameter to subdev's set_routing(). That is a different style compared to the other ops, which have the 'which' inside the passed struct. Any opinions? Tomi
On Tue, Sep 28, 2021 at 03:33:18PM +0300, Tomi Valkeinen wrote: > On 28/09/2021 08:14, Tomi Valkeinen wrote: > > > Yes. I'm not 100% sure yet if it's possible to get rid of which from > > init_cfg, but I'll try it out. > > > >> As ACTIVE state support is opt-in, it seems to me that we won't need to > >> mass-fix drivers as part of this series if we go for this option. Am I > > > > Yes, I think so. I'll be wiser after I've worked on this a bit =). I > > think the routing needs the biggest change, as the routing table > > contains 'which'. But routing won't affect the current drivers. > > > > However, 'which' is quite ingrained to v4l2, I won't be too surprised if > > I keep finding new 'whiches' while removing it from the init_cfg call > > paths. > > It was rather easy to get rid of 'which'. Jee ! Olen iloinen :-) > I now have > v4l2_subdev_routing, which is the struct used in the uAPI: > > struct v4l2_subdev_routing { > __u32 which; > __u64 routes; > __u32 num_routes; > __u32 reserved[5]; > }; > > Then I have v4l2_subdev_krouting, which is used when calling the subdev > set_routing op: > > struct v4l2_subdev_krouting { > u32 which; > struct v4l2_subdev_routing_table table; > }; > > And I have v4l2_subdev_routing_table, which is used in the various > helper functions, and stored in the state: > > struct v4l2_subdev_routing_table { > unsigned int num_routes; > struct v4l2_subdev_route *routes; > }; > > As the only use of v4l2_subdev_krouting is when calling the subdev's > set_routing, I think we should just drop it and pass 'which' as a > parameter to subdev's set_routing(). That is a different style compared > to the other ops, which have the 'which' inside the passed struct. > > Any opinions? I don't mind, I was going to propose it when reading the above. -- Regards, Laurent Pinchart
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 26a34a8e3d37..e1a794f69815 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd, v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev); } EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event); + +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) +{ + struct v4l2_subdev_state *state; + + state = v4l2_alloc_subdev_state(sd); + if (IS_ERR(state)) + return PTR_ERR(state); + + sd->state = state; + + return 0; +} +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); + +void v4l2_subdev_free_state(struct v4l2_subdev *sd) +{ + v4l2_free_subdev_state(sd->state); + sd->state = NULL; +} +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 8701d2e7d893..ecaf040ead57 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -898,6 +898,8 @@ struct v4l2_subdev_platform_data { * @subdev_notifier: A sub-device notifier implicitly registered for the sub- * device using v4l2_async_register_subdev_sensor(). * @pdata: common part of subdevice platform data + * @state: active state for the subdev (NULL for subdevs tracking the state + * internally) * * Each instance of a subdev driver should create this struct, either * stand-alone or embedded in a larger struct. @@ -929,6 +931,7 @@ struct v4l2_subdev { struct v4l2_async_notifier *notifier; struct v4l2_async_notifier *subdev_notifier; struct v4l2_subdev_platform_data *pdata; + struct v4l2_subdev_state *state; }; @@ -1217,4 +1220,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers; void v4l2_subdev_notify_event(struct v4l2_subdev *sd, const struct v4l2_event *ev); +/** + * v4l2_subdev_alloc_state() - Allocate active subdev state for subdevice + * @sd: The subdev for which the state is allocated + * + * This will allocate a subdev state and store it to + * &struct v4l2_subdev->state. + * + * Must call v4l2_subdev_free_state() when the state is no longer needed. + */ +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd); + +/** + * v4l2_subdev_free_state() - Free the active subdev state for subdevice + * @sd: The subdevice + * + * This will free the subdev's state and set + * &struct v4l2_subdev->state to NULL. + */ +void v4l2_subdev_free_state(struct v4l2_subdev *sd); + +/** + * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice + * @sd: The subdevice + * + * Return the active state for the subdevice, or NULL if the subdev does not + * support active state. + */ +static inline struct v4l2_subdev_state * +v4l2_subdev_get_active_state(struct v4l2_subdev *sd) +{ + return sd->state; +} + #endif
Add a new 'state' field to struct v4l2_subdev to which we can store the active state of a subdev. This will place the subdev configuration into a known place, allowing us to use the state directly from the v4l2 framework, thus simplifying the drivers. We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(), which need to be used by the drivers that support subdev state in struct v4l2_subdev. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++ include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++ 2 files changed, 57 insertions(+)