| Message ID | 20210830110116.488338-4-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:43PM +0300, Tomi Valkeinen wrote: > The subdev state is passed to functions in the media drivers, and > usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is > also given to the function in one way or another. > > One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One > could argue that the initialization of the state should be the same for > both ACTIVE and TRY cases, but unfortunately that is not the case: > > - Some drivers do also other things than just touch the state when > dealing with ACTIVE, e.g. if there is extra state outside the standard > subdev state. > - Some drivers might need to create, say, struct v4l2_subdev_format > which has 'which' field, and that needs to be filled with either ACTIVE > or TRY. > > Currently init_cfg is only called for TRY case from the v4l2 framework, > passing the TRY state. Some drivers call their own init_cfg, passing > NULL as the state, which is used to indicate ACTIVE case. > > In the future we want to pass subdev's active state from the v4l2 > framework side, so we need a solution to this. > > We could change the init_cfg() to include the TRY/ACTIVE value, which > would require changing more or less all the drivers. Instead, I have > added 'which' field to the subdev state itself, filled at state > allocation time, which only requires changes to the drivers that > allocate a state themselves. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- > drivers/media/platform/vsp1/vsp1_entity.c | 2 +- > drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- > drivers/staging/media/tegra-video/vi.c | 2 +- > include/media/v4l2-subdev.h | 7 ++++++- > 5 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index 5f4fa8c48f68..1de30d5b437f 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, > u32 width, height; > int ret; > > - sd_state = v4l2_alloc_subdev_state(sd); > + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); Shouldn't the 'which' parameters be used to decide if either ACTIVE or TRY have to be used ? this function is also used to set TRY formats, in example... Oh, maybe I got how it works, the state's which is not relevant but the v4l2_subdev_format's which is, as it will be used in the next patch to decide if the subdev's state of the file-handle's state should be passed to the ioctl. > if (IS_ERR(sd_state)) > return PTR_ERR(sd_state); > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > index e40bca254b8b..63ea5e472c33 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.c > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > * Allocate the pad configuration to store formats and selection > * rectangles. > */ > - entity->config = v4l2_alloc_subdev_state(&entity->subdev); > + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); > if (IS_ERR(entity->config)) { > media_entity_cleanup(&entity->subdev.entity); > return PTR_ERR(entity->config); > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index e1a794f69815..04ad319fb150 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > { > struct v4l2_subdev_state *state; > > - state = v4l2_alloc_subdev_state(sd); > + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); At the same time I'm not sure I get the purpose of this. Don't init_cfg() callback implementations deal with try formats themeselves ? I mean, it's not a fixed rule, they can as well initialize their default 'active' formats, but what matters is that they initialize their per-fh try states ? Shouldn't init_cfg receive the fh's state so that it can initialize it, and just in case they need to, access their subdev's state and initialize them ? I'm missing what the purpose of the flag is tbh. Thanks j > if (IS_ERR(state)) > return PTR_ERR(state); > > @@ -870,7 +870,9 @@ int v4l2_subdev_link_validate(struct media_link *link) > } > EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); > > -struct v4l2_subdev_state *v4l2_alloc_subdev_state(struct v4l2_subdev *sd) > +struct v4l2_subdev_state * > +v4l2_alloc_subdev_state(struct v4l2_subdev *sd, > + enum v4l2_subdev_format_whence which) > { > struct v4l2_subdev_state *state; > int ret; > @@ -879,6 +881,8 @@ struct v4l2_subdev_state *v4l2_alloc_subdev_state(struct v4l2_subdev *sd) > if (!state) > return ERR_PTR(-ENOMEM); > > + state->which = which; > + > if (sd->entity.num_pads) { > state->pads = kvmalloc_array(sd->entity.num_pads, > sizeof(*state->pads), > @@ -948,7 +952,7 @@ int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > { > struct v4l2_subdev_state *state; > > - state = v4l2_alloc_subdev_state(sd); > + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); > if (IS_ERR(state)) > return PTR_ERR(state); > > diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c > index a94d19e2a67c..691f5e04b0a1 100644 > --- a/drivers/staging/media/tegra-video/vi.c > +++ b/drivers/staging/media/tegra-video/vi.c > @@ -507,7 +507,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan, > if (!subdev) > return -ENODEV; > > - sd_state = v4l2_alloc_subdev_state(subdev); > + sd_state = v4l2_alloc_subdev_state(subdev, V4L2_SUBDEV_FORMAT_ACTIVE); > if (IS_ERR(sd_state)) > return PTR_ERR(sd_state); > /* > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index ecaf040ead57..5ec78ffda4f5 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -655,6 +655,7 @@ struct v4l2_subdev_pad_config { > /** > * struct v4l2_subdev_state - Used for storing subdev state information. > * > + * @which: state type (from enum v4l2_subdev_format_whence) > * @pads: &struct v4l2_subdev_pad_config array > * > * This structure only needs to be passed to the pad op if the 'which' field > @@ -662,6 +663,7 @@ struct v4l2_subdev_pad_config { > * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL. > */ > struct v4l2_subdev_state { > + u32 which; > struct v4l2_subdev_pad_config *pads; > }; > > @@ -1141,10 +1143,13 @@ int v4l2_subdev_link_validate(struct media_link *link); > * v4l2_alloc_subdev_state - allocate v4l2_subdev_state > * > * @sd: pointer to &struct v4l2_subdev for which the state is being allocated. > + * @which: configuration type for the state (from enum v4l2_subdev_format_whence) > * > * Must call v4l2_free_subdev_state() when state is no longer needed. > */ > -struct v4l2_subdev_state *v4l2_alloc_subdev_state(struct v4l2_subdev *sd); > +struct v4l2_subdev_state * > +v4l2_alloc_subdev_state(struct v4l2_subdev *sd, > + enum v4l2_subdev_format_whence which); > > /** > * v4l2_free_subdev_state - free a v4l2_subdev_state > -- > 2.25.1 >
On 13/09/2021 14:41, Jacopo Mondi wrote: > Hi Tomi, > > On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: >> The subdev state is passed to functions in the media drivers, and >> usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is >> also given to the function in one way or another. >> >> One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One >> could argue that the initialization of the state should be the same for >> both ACTIVE and TRY cases, but unfortunately that is not the case: >> >> - Some drivers do also other things than just touch the state when >> dealing with ACTIVE, e.g. if there is extra state outside the standard >> subdev state. >> - Some drivers might need to create, say, struct v4l2_subdev_format >> which has 'which' field, and that needs to be filled with either ACTIVE >> or TRY. >> >> Currently init_cfg is only called for TRY case from the v4l2 framework, >> passing the TRY state. Some drivers call their own init_cfg, passing >> NULL as the state, which is used to indicate ACTIVE case. >> >> In the future we want to pass subdev's active state from the v4l2 >> framework side, so we need a solution to this. >> >> We could change the init_cfg() to include the TRY/ACTIVE value, which >> would require changing more or less all the drivers. Instead, I have >> added 'which' field to the subdev state itself, filled at state >> allocation time, which only requires changes to the drivers that >> allocate a state themselves. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- >> drivers/media/platform/vsp1/vsp1_entity.c | 2 +- >> drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- >> drivers/staging/media/tegra-video/vi.c | 2 +- >> include/media/v4l2-subdev.h | 7 ++++++- >> 5 files changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c >> index 5f4fa8c48f68..1de30d5b437f 100644 >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c >> @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, >> u32 width, height; >> int ret; >> >> - sd_state = v4l2_alloc_subdev_state(sd); >> + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); > > Shouldn't the 'which' parameters be used to decide if either ACTIVE or > TRY have to be used ? this function is also used to set TRY formats, > in example... > > Oh, maybe I got how it works, the state's which is not > relevant but the v4l2_subdev_format's which is, as it will be used in > the next patch to decide if the subdev's state of the file-handle's > state should be passed to the ioctl. Yes. It's messy, but it's how it worked before also. The drivers can't really allocate TRY state as it must come from the core, based on the filehandle. Now as I say that, makes me wonder why even expose the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the sd parameter, and always allocate ACTIVE state, and the v4l2 core can use another way to create the TRY state. >> if (IS_ERR(sd_state)) >> return PTR_ERR(sd_state); >> >> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c >> index e40bca254b8b..63ea5e472c33 100644 >> --- a/drivers/media/platform/vsp1/vsp1_entity.c >> +++ b/drivers/media/platform/vsp1/vsp1_entity.c >> @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, >> * Allocate the pad configuration to store formats and selection >> * rectangles. >> */ >> - entity->config = v4l2_alloc_subdev_state(&entity->subdev); >> + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); >> if (IS_ERR(entity->config)) { >> media_entity_cleanup(&entity->subdev.entity); >> return PTR_ERR(entity->config); >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index e1a794f69815..04ad319fb150 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) >> { >> struct v4l2_subdev_state *state; >> >> - state = v4l2_alloc_subdev_state(sd); >> + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); > > At the same time I'm not sure I get the purpose of this. Don't > init_cfg() callback implementations deal with try formats themeselves > ? I mean, it's not a fixed rule, they can as well initialize their > default 'active' formats, but what matters is that they initialize > their per-fh try states ? That is what they do currently. init_cfg() only deals with TRY state, as that's the only "state" (i.e. pad_config) there used to be from v4l2 core's perspective. > Shouldn't init_cfg receive the fh's state so that it can initialize > it, and just in case they need to, access their subdev's state and > initialize them ? I'm missing what the purpose of the flag is tbh. Now we have (a possibility to have) state for both TRY and ACTIVE on the v4l2 core side. The active state has to be initialized also, and a logical way to do that is to use the init_cfg(). So now, for drivers that support the new active state, init_cfg() can get either TRY or ACTIVE state. And if you want to call, say, the driver's set_routing() to setup the routing in the state, you have to set the 'which' in the routing struct to a value. So somehow init_cfg needs to know if it's initializing an ACTIVE or TRY state. Tomi
Hi Tomi, On Mon, Sep 13, 2021 at 03:17:01PM +0300, Tomi Valkeinen wrote: > On 13/09/2021 14:41, Jacopo Mondi wrote: > > Hi Tomi, > > > > On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: > > > The subdev state is passed to functions in the media drivers, and > > > usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is > > > also given to the function in one way or another. > > > > > > One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One > > > could argue that the initialization of the state should be the same for > > > both ACTIVE and TRY cases, but unfortunately that is not the case: > > > > > > - Some drivers do also other things than just touch the state when > > > dealing with ACTIVE, e.g. if there is extra state outside the standard > > > subdev state. > > > - Some drivers might need to create, say, struct v4l2_subdev_format > > > which has 'which' field, and that needs to be filled with either ACTIVE > > > or TRY. > > > > > > Currently init_cfg is only called for TRY case from the v4l2 framework, > > > passing the TRY state. Some drivers call their own init_cfg, passing > > > NULL as the state, which is used to indicate ACTIVE case. > > > > > > In the future we want to pass subdev's active state from the v4l2 > > > framework side, so we need a solution to this. > > > > > > We could change the init_cfg() to include the TRY/ACTIVE value, which > > > would require changing more or less all the drivers. Instead, I have > > > added 'which' field to the subdev state itself, filled at state > > > allocation time, which only requires changes to the drivers that > > > allocate a state themselves. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > --- > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- > > > drivers/media/platform/vsp1/vsp1_entity.c | 2 +- > > > drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- > > > drivers/staging/media/tegra-video/vi.c | 2 +- > > > include/media/v4l2-subdev.h | 7 ++++++- > > > 5 files changed, 16 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > index 5f4fa8c48f68..1de30d5b437f 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, > > > u32 width, height; > > > int ret; > > > > > > - sd_state = v4l2_alloc_subdev_state(sd); > > > + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); > > > > Shouldn't the 'which' parameters be used to decide if either ACTIVE or > > TRY have to be used ? this function is also used to set TRY formats, > > in example... > > > > Oh, maybe I got how it works, the state's which is not > > relevant but the v4l2_subdev_format's which is, as it will be used in > > the next patch to decide if the subdev's state of the file-handle's > > state should be passed to the ioctl. > > Yes. It's messy, but it's how it worked before also. > > The drivers can't really allocate TRY state as it must come from the core, > based on the filehandle. Now as I say that, makes me wonder why even expose > the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the > sd parameter, and always allocate ACTIVE state, and the v4l2 core can use > another way to create the TRY state. > init_cfg() as well as other operations used to received an array of per fh's pad_configs, and the sd pointer itself. The fh's pad configs were allocated by the core, as well as the core now allocates the per-fh's state. Before the introduction of 'state', if the 'which' flags was set to TRY then information were stored/retrieved/initialized in the per-fh pad_config, otherwise the active configuration (usually stored in the driver main structure) was used. So we had a clear separation of per-fh information and the active state. The core didn't care afaict, and passed in both, then driver had to deal with them doing the right thing by inspecting the 'which' flag. The typical pattern was: static int subdev_ops(sd, pad_cfg, which) { if (which == TRY) /* Operate on config stored in pad_cfg */ else /* * Operate on config stored in subdev (and * applied to HW) */ } Or am I overlooking some cases or do you agree with my understanding so far ? Now we have a 'state' that holds the array of pad_configs and along the continuation of the series will end up holding per-pad configurations. We now also have one 'state' per file-handle, and one per-subdev. As I see this, it would be natual for drivers to receive one state without knowing where it comes from. In the next patch you instrument the core to do exactly this: inspect the which flag and pass in the 'right' state. Ofc drivers need to have access to 'which' to know if they have to apply settings to the HW or not. Looking ahead in your series I see these structures: struct v4l2_subdev_pad_config { struct v4l2_mbus_framefmt try_fmt; struct v4l2_rect try_crop; struct v4l2_rect try_compose; }; struct v4l2_subdev_stream_config { u32 pad; u32 stream; struct v4l2_mbus_framefmt fmt; struct v4l2_rect crop; struct v4l2_rect compose; }; struct v4l2_subdev_stream_configs { u32 num_configs; struct v4l2_subdev_stream_config *configs; }; All of them part of state: struct v4l2_subdev_state { struct mutex lock; u32 which; struct v4l2_subdev_pad_config *pads; struct v4l2_subdev_krouting routing; struct v4l2_subdev_stream_configs stream_configs; }; So 'state' will hold 'TRY' information (only used for 'state' instances allocated in the fh) and 'ACTIVE' ones (used for states allocated in the sd). Looking at 'v4l2_subdev_pad_config' and 'v4l2_subdev_stream_config' they seem to describe more or less the same things: fmt, crop and compose (per pad-stream in case of stream_config). I wonder if those shouldn't be unified so that: 1) Drivers receive one state: the core passes in the 'correct' one (per-fh or per-sd) as you do in next patch 2) The 'which' information is not stored in the state but it's only 'contextual' (as in a parameter to the subdev operation) so that drivers inspect it to know if they have to apply settings to hw or not 3) v4l2_subdev_pad_config can be re-used and expanded, to maintain per-pad configurations regardless if they're ACTIVE or TRY, as this only depends on where the state is stored. As I immagine it a subdev pad operation could look like: static int subdev_op(sd, pad, state, which, ...) { /* Doesn't matter if state is per-fh or the sd one. */ state->pads[pad].fmt = ....; if (which == TRY) return; /* Apply to the HW. */ } Does it make any sense to you ? I might have missed some reason why this is not possible. > > > if (IS_ERR(sd_state)) > > > return PTR_ERR(sd_state); > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > > > index e40bca254b8b..63ea5e472c33 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_entity.c > > > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > > > @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > > > * Allocate the pad configuration to store formats and selection > > > * rectangles. > > > */ > > > - entity->config = v4l2_alloc_subdev_state(&entity->subdev); > > > + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); > > > if (IS_ERR(entity->config)) { > > > media_entity_cleanup(&entity->subdev.entity); > > > return PTR_ERR(entity->config); > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index e1a794f69815..04ad319fb150 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > > > { > > > struct v4l2_subdev_state *state; > > > > > > - state = v4l2_alloc_subdev_state(sd); > > > + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); > > > > At the same time I'm not sure I get the purpose of this. Don't > > init_cfg() callback implementations deal with try formats themeselves > > ? I mean, it's not a fixed rule, they can as well initialize their > > default 'active' formats, but what matters is that they initialize > > their per-fh try states ? > > That is what they do currently. init_cfg() only deals with TRY state, as > that's the only "state" (i.e. pad_config) there used to be from v4l2 core's > perspective. > > > Shouldn't init_cfg receive the fh's state so that it can initialize > > it, and just in case they need to, access their subdev's state and > > initialize them ? I'm missing what the purpose of the flag is tbh. > > Now we have (a possibility to have) state for both TRY and ACTIVE on the > v4l2 core side. The active state has to be initialized also, and a logical > way to do that is to use the init_cfg(). The 'ACTIVE' state is stored in the subdev, to which init_cfg() has access, and it receives the 'TRY' state as a parameter. It is possible to access both states and initialize them properly if I'm not mistaken. > > So now, for drivers that support the new active state, init_cfg() can get > either TRY or ACTIVE state. And if you want to call, say, the driver's > set_routing() to setup the routing in the state, you have to set the 'which' > in the routing struct to a value. So somehow init_cfg needs to know if it's > initializing an ACTIVE or TRY state. I'm not sure I got this part. set_routing() as other ops will receive a state and 'which'. If my proposal above makes sensem where the state comes from (aka it's TRY or ACTIVE) it's not relevant for the driver. Ofc init_cfg() should have initialized both states opportunely, but that's the case today as well. Thanks j > > Tomi
On 13/09/2021 16:38, Jacopo Mondi wrote: > Hi Tomi, > > On Mon, Sep 13, 2021 at 03:17:01PM +0300, Tomi Valkeinen wrote: >> On 13/09/2021 14:41, Jacopo Mondi wrote: >>> Hi Tomi, >>> >>> On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: >>>> The subdev state is passed to functions in the media drivers, and >>>> usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is >>>> also given to the function in one way or another. >>>> >>>> One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One >>>> could argue that the initialization of the state should be the same for >>>> both ACTIVE and TRY cases, but unfortunately that is not the case: >>>> >>>> - Some drivers do also other things than just touch the state when >>>> dealing with ACTIVE, e.g. if there is extra state outside the standard >>>> subdev state. >>>> - Some drivers might need to create, say, struct v4l2_subdev_format >>>> which has 'which' field, and that needs to be filled with either ACTIVE >>>> or TRY. >>>> >>>> Currently init_cfg is only called for TRY case from the v4l2 framework, >>>> passing the TRY state. Some drivers call their own init_cfg, passing >>>> NULL as the state, which is used to indicate ACTIVE case. >>>> >>>> In the future we want to pass subdev's active state from the v4l2 >>>> framework side, so we need a solution to this. >>>> >>>> We could change the init_cfg() to include the TRY/ACTIVE value, which >>>> would require changing more or less all the drivers. Instead, I have >>>> added 'which' field to the subdev state itself, filled at state >>>> allocation time, which only requires changes to the drivers that >>>> allocate a state themselves. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> --- >>>> drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- >>>> drivers/media/platform/vsp1/vsp1_entity.c | 2 +- >>>> drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- >>>> drivers/staging/media/tegra-video/vi.c | 2 +- >>>> include/media/v4l2-subdev.h | 7 ++++++- >>>> 5 files changed, 16 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c >>>> index 5f4fa8c48f68..1de30d5b437f 100644 >>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c >>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c >>>> @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, >>>> u32 width, height; >>>> int ret; >>>> >>>> - sd_state = v4l2_alloc_subdev_state(sd); >>>> + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); >>> >>> Shouldn't the 'which' parameters be used to decide if either ACTIVE or >>> TRY have to be used ? this function is also used to set TRY formats, >>> in example... >>> >>> Oh, maybe I got how it works, the state's which is not >>> relevant but the v4l2_subdev_format's which is, as it will be used in >>> the next patch to decide if the subdev's state of the file-handle's >>> state should be passed to the ioctl. >> >> Yes. It's messy, but it's how it worked before also. >> >> The drivers can't really allocate TRY state as it must come from the core, >> based on the filehandle. Now as I say that, makes me wonder why even expose >> the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the >> sd parameter, and always allocate ACTIVE state, and the v4l2 core can use >> another way to create the TRY state. >> > > init_cfg() as well as other operations used to received an > array of per fh's pad_configs, and the sd pointer itself. The fh's pad > configs were allocated by the core, as well as the core now allocates > the per-fh's state. > > Before the introduction of 'state', if the 'which' flags was set to > TRY then information were stored/retrieved/initialized in the per-fh > pad_config, otherwise the active configuration (usually stored in the > driver main structure) was used. > > So we had a clear separation of per-fh information and the active > state. The core didn't care afaict, and passed in both, then driver had > to deal with them doing the right thing by inspecting the 'which' flag. > > The typical pattern was: > > static int subdev_ops(sd, pad_cfg, which) > { > if (which == TRY) > /* Operate on config stored in pad_cfg */ > > else > /* > * Operate on config stored in subdev (and > * applied to HW) > */ > } > > Or am I overlooking some cases or do you agree with my understanding > so far ? More or less, yes. I think there are (used to be) three kinds of ops: - Ops that get pad_cfg and 'which' in an op specific struct. E.g. set_fmt. The pad_cfg is TRY pad_config, even if 'which' == ACTIVE. - Ops that don't get pad_cfg, like s_stream. 'which' is implicitly ACTIVE. - init_cfg which gets pad_cfg, but no which (as 'which' is always implicitly TRY) So pad_cfg was TRY state. Drivers could use pad_configs internally to track ACTIVE state, but the core had no knowledge about this. > Now we have a 'state' that holds the array of pad_configs and along > the continuation of the series will end up holding per-pad > configurations. > > We now also have one 'state' per file-handle, and one > per-subdev. As I see this, it would be natual for drivers to receive > one state without knowing where it comes from. In the next patch you Note that only subdev's that explicitly support the new state code, and allocate the state, have the subdev active state. Which means only the drivers in my work branch. The "old" drivers work like they used to: they get the state (essentially repackaged pad_cfg) for TRY cases, NULL otherwise. And yes, it would be natural to just get a state, but the subdev drivers need to know if the context is TRY/ACTIVE. As you can see from the bullet list above, the driver knows this in all the other places except init_cfg. > instrument the core to do exactly this: inspect the which flag and > pass in the 'right' state. Ofc drivers need to have access to 'which' > to know if they have to apply settings to the HW or not. > > Looking ahead in your series I see these structures: > > struct v4l2_subdev_pad_config { > struct v4l2_mbus_framefmt try_fmt; > struct v4l2_rect try_crop; > struct v4l2_rect try_compose; > }; > > struct v4l2_subdev_stream_config { > u32 pad; > u32 stream; > > struct v4l2_mbus_framefmt fmt; > struct v4l2_rect crop; > struct v4l2_rect compose; > }; > > struct v4l2_subdev_stream_configs { > u32 num_configs; > struct v4l2_subdev_stream_config *configs; > }; > > All of them part of state: > > struct v4l2_subdev_state { > struct mutex lock; > u32 which; > struct v4l2_subdev_pad_config *pads; > struct v4l2_subdev_krouting routing; > struct v4l2_subdev_stream_configs stream_configs; > }; > > So 'state' will hold 'TRY' information (only used for 'state' > instances allocated in the fh) and 'ACTIVE' ones (used for states > allocated in the sd). Right. > Looking at 'v4l2_subdev_pad_config' and 'v4l2_subdev_stream_config' they > seem to describe more or less the same things: fmt, crop and compose > (per pad-stream in case of stream_config). I wonder if those shouldn't > be unified so that: > > 1) Drivers receive one state: the core passes in the 'correct' one > (per-fh or per-sd) as you do in next patch Yes. But note that "old" drivers don't have active state. > 2) The 'which' information is not stored in the state but it's only > 'contextual' (as in a parameter to the subdev operation) so that > drivers inspect it to know if they have to apply settings to hw or not Yes, except we have init_cfg... > 3) v4l2_subdev_pad_config can be re-used and expanded, to maintain per-pad > configurations regardless if they're ACTIVE or TRY, as this only depends > on where the state is stored. pad_config is a static array of per-pad configs. stream_configs is a dynamic per-stream config. stream_configs is a super-set of pad-configs, so we could drop pad_configs, but it would require changing all the drivers in non-trivial ways. v4l2_subdev_pad_config is not used or even allocated by the "new" drivers. And routing & stream_configs are not used by the "old" drivers. > As I immagine it a subdev pad operation could look like: > > static int subdev_op(sd, pad, state, which, ...) > { > /* Doesn't matter if state is per-fh or the sd one. */ > state->pads[pad].fmt = ....; > > if (which == TRY) > return; > > /* Apply to the HW. */ > } > > Does it make any sense to you ? I might have missed some reason why > this is not possible. It makes sense, but there are the buts =). I've tried to explain these in the commit messages, but it's kind of confusing. One but I haven't mentioned in the emails is that when subdev drivers call ops in other subdev drivers they pass NULL in the state. This is fine for the "old" drivers, as they expect a state only for TRY case. However, the "new" drivers unfortunately expect to get a state on both TRY and ACTIVE cases, and the only sensible way I figured out to handle this was the v4l2_subdev_validate_state() function (patch 6). So, all this could be much neater, but would require modifying all subdev drivers in non-trivial ways. I think this is something that can be done slowly in the future. >>>> if (IS_ERR(sd_state)) >>>> return PTR_ERR(sd_state); >>>> >>>> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c >>>> index e40bca254b8b..63ea5e472c33 100644 >>>> --- a/drivers/media/platform/vsp1/vsp1_entity.c >>>> +++ b/drivers/media/platform/vsp1/vsp1_entity.c >>>> @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, >>>> * Allocate the pad configuration to store formats and selection >>>> * rectangles. >>>> */ >>>> - entity->config = v4l2_alloc_subdev_state(&entity->subdev); >>>> + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); >>>> if (IS_ERR(entity->config)) { >>>> media_entity_cleanup(&entity->subdev.entity); >>>> return PTR_ERR(entity->config); >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>>> index e1a794f69815..04ad319fb150 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>> @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) >>>> { >>>> struct v4l2_subdev_state *state; >>>> >>>> - state = v4l2_alloc_subdev_state(sd); >>>> + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); >>> >>> At the same time I'm not sure I get the purpose of this. Don't >>> init_cfg() callback implementations deal with try formats themeselves >>> ? I mean, it's not a fixed rule, they can as well initialize their >>> default 'active' formats, but what matters is that they initialize >>> their per-fh try states ? >> >> That is what they do currently. init_cfg() only deals with TRY state, as >> that's the only "state" (i.e. pad_config) there used to be from v4l2 core's >> perspective. >> >>> Shouldn't init_cfg receive the fh's state so that it can initialize >>> it, and just in case they need to, access their subdev's state and >>> initialize them ? I'm missing what the purpose of the flag is tbh. >> >> Now we have (a possibility to have) state for both TRY and ACTIVE on the >> v4l2 core side. The active state has to be initialized also, and a logical >> way to do that is to use the init_cfg(). > > The 'ACTIVE' state is stored in the subdev, to which init_cfg() has > access, and it receives the 'TRY' state as a parameter. No, init_cfg gets either ACTIVE or TRY state, whichever is being allocated. For "old" drivers, ACTIVE state is never allocated so they don't get init_cfg calls for ACTIVE at all. Aaand while writing that, I realized that some drivers manually do allocate ACTIVE state temporarily, which would cause init_cfg with ACTIVE state to be called. I wonder if they explode... Need to check. > It is possible to access both states and initialize them properly if > I'm not mistaken. > >> >> So now, for drivers that support the new active state, init_cfg() can get >> either TRY or ACTIVE state. And if you want to call, say, the driver's >> set_routing() to setup the routing in the state, you have to set the 'which' >> in the routing struct to a value. So somehow init_cfg needs to know if it's >> initializing an ACTIVE or TRY state. > > I'm not sure I got this part. set_routing() as other ops will receive > a state and 'which'. If my proposal above makes sensem where the state Yes, but if it's init_cfg calling set_routing, init_cfg has to figure out the 'which' from somewhere. E.g. init_cfg from ub913 driver: static int ub913_init_cfg(struct v4l2_subdev *sd, struct v4l2_subdev_state *state) { u32 which = state->which; struct v4l2_subdev_route routes[] = { { .sink_pad = 0, .sink_stream = 0, .source_pad = 1, .source_stream = 0, .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, }, }; struct v4l2_subdev_krouting routing = { .which = which, .num_routes = ARRAY_SIZE(routes), .routes = routes, }; return ub913_set_routing(sd, state, &routing); } It uses set_routing to setup a default routing (and set_routing in turn also initializes the formats), but set_routing needs 'which'. Tomi
Hi Tomi, On Mon, Sep 13, 2021 at 05:26:45PM +0300, Tomi Valkeinen wrote: > On 13/09/2021 16:38, Jacopo Mondi wrote: > > Hi Tomi, > > > > On Mon, Sep 13, 2021 at 03:17:01PM +0300, Tomi Valkeinen wrote: > > > On 13/09/2021 14:41, Jacopo Mondi wrote: > > > > Hi Tomi, > > > > > > > > On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: > > > > > The subdev state is passed to functions in the media drivers, and > > > > > usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is > > > > > also given to the function in one way or another. > > > > > > > > > > One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One > > > > > could argue that the initialization of the state should be the same for > > > > > both ACTIVE and TRY cases, but unfortunately that is not the case: > > > > > > > > > > - Some drivers do also other things than just touch the state when > > > > > dealing with ACTIVE, e.g. if there is extra state outside the standard > > > > > subdev state. > > > > > - Some drivers might need to create, say, struct v4l2_subdev_format > > > > > which has 'which' field, and that needs to be filled with either ACTIVE > > > > > or TRY. > > > > > > > > > > Currently init_cfg is only called for TRY case from the v4l2 framework, > > > > > passing the TRY state. Some drivers call their own init_cfg, passing > > > > > NULL as the state, which is used to indicate ACTIVE case. > > > > > > > > > > In the future we want to pass subdev's active state from the v4l2 > > > > > framework side, so we need a solution to this. > > > > > > > > > > We could change the init_cfg() to include the TRY/ACTIVE value, which > > > > > would require changing more or less all the drivers. Instead, I have > > > > > added 'which' field to the subdev state itself, filled at state > > > > > allocation time, which only requires changes to the drivers that > > > > > allocate a state themselves. > > > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > > > --- > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- > > > > > drivers/media/platform/vsp1/vsp1_entity.c | 2 +- > > > > > drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- > > > > > drivers/staging/media/tegra-video/vi.c | 2 +- > > > > > include/media/v4l2-subdev.h | 7 ++++++- > > > > > 5 files changed, 16 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > index 5f4fa8c48f68..1de30d5b437f 100644 > > > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, > > > > > u32 width, height; > > > > > int ret; > > > > > > > > > > - sd_state = v4l2_alloc_subdev_state(sd); > > > > > + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); > > > > > > > > Shouldn't the 'which' parameters be used to decide if either ACTIVE or > > > > TRY have to be used ? this function is also used to set TRY formats, > > > > in example... > > > > > > > > Oh, maybe I got how it works, the state's which is not > > > > relevant but the v4l2_subdev_format's which is, as it will be used in > > > > the next patch to decide if the subdev's state of the file-handle's > > > > state should be passed to the ioctl. > > > > > > Yes. It's messy, but it's how it worked before also. > > > > > > The drivers can't really allocate TRY state as it must come from the core, > > > based on the filehandle. Now as I say that, makes me wonder why even expose > > > the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the > > > sd parameter, and always allocate ACTIVE state, and the v4l2 core can use > > > another way to create the TRY state. > > > > > > > init_cfg() as well as other operations used to received an > > array of per fh's pad_configs, and the sd pointer itself. The fh's pad > > configs were allocated by the core, as well as the core now allocates > > the per-fh's state. > > > > Before the introduction of 'state', if the 'which' flags was set to > > TRY then information were stored/retrieved/initialized in the per-fh > > pad_config, otherwise the active configuration (usually stored in the > > driver main structure) was used. > > > > So we had a clear separation of per-fh information and the active > > state. The core didn't care afaict, and passed in both, then driver had > > to deal with them doing the right thing by inspecting the 'which' flag. > > > > The typical pattern was: > > > > static int subdev_ops(sd, pad_cfg, which) > > { > > if (which == TRY) > > /* Operate on config stored in pad_cfg */ > > > > else > > /* > > * Operate on config stored in subdev (and > > * applied to HW) > > */ > > } > > > > Or am I overlooking some cases or do you agree with my understanding > > so far ? > > More or less, yes. I think there are (used to be) three kinds of ops: > > - Ops that get pad_cfg and 'which' in an op specific struct. E.g. set_fmt. > The pad_cfg is TRY pad_config, even if 'which' == ACTIVE. > > - Ops that don't get pad_cfg, like s_stream. 'which' is implicitly ACTIVE. Also note that operations like s_stream do not take a state as parameter. The driver has to fetch it from the subdev anyway (this in reply to the idea of having the active state as parameter vs retrieving it from the subdev if ACTIVE) While porting the R-Car drivers on top of this series I found myself in the need to (in the s_stream call chain) static int rcsi2_start_receiver(struct rcar_csi2 *priv) { const struct v4l2_subdev_state *state = priv->subdev.state; const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; ... /* * Configure field handling inspecting the formats of the * single sink pad streams. */ for (i = 0; i < configs->num_configs; ++i) { const struct v4l2_subdev_stream_config *config = configs->configs; if (config->pad != RCAR_CSI2_SINK) continue; if (config->fmt.field != V4L2_FIELD_ALTERNATE) continue; fld |= FLD_DET_SEL(1); fld |= FLD_FLD_EN(config->stream); /* PAL vs NTSC. */ if (config->fmt.height == 240) fld |= FLD_FLD_NUM(0); else fld |= FLD_FLD_NUM(1); } ... } Am I doing it wrong, or is this a case for the subdev to have to directly access sd->state ? > > - init_cfg which gets pad_cfg, but no which (as 'which' is always implicitly > TRY) > > So pad_cfg was TRY state. Drivers could use pad_configs internally to track > ACTIVE state, but the core had no knowledge about this. > > > Now we have a 'state' that holds the array of pad_configs and along > > the continuation of the series will end up holding per-pad > > configurations. > > > > We now also have one 'state' per file-handle, and one > > per-subdev. As I see this, it would be natual for drivers to receive > > one state without knowing where it comes from. In the next patch you > > Note that only subdev's that explicitly support the new state code, and > allocate the state, have the subdev active state. Which means only the > drivers in my work branch. > > The "old" drivers work like they used to: they get the state (essentially > repackaged pad_cfg) for TRY cases, NULL otherwise. > > And yes, it would be natural to just get a state, but the subdev drivers > need to know if the context is TRY/ACTIVE. As you can see from the bullet > list above, the driver knows this in all the other places except init_cfg. > > > instrument the core to do exactly this: inspect the which flag and > > pass in the 'right' state. Ofc drivers need to have access to 'which' > > to know if they have to apply settings to the HW or not. > > > > Looking ahead in your series I see these structures: > > > > struct v4l2_subdev_pad_config { > > struct v4l2_mbus_framefmt try_fmt; > > struct v4l2_rect try_crop; > > struct v4l2_rect try_compose; > > }; > > > > struct v4l2_subdev_stream_config { > > u32 pad; > > u32 stream; > > > > struct v4l2_mbus_framefmt fmt; > > struct v4l2_rect crop; > > struct v4l2_rect compose; > > }; > > > > struct v4l2_subdev_stream_configs { > > u32 num_configs; > > struct v4l2_subdev_stream_config *configs; > > }; > > > > All of them part of state: > > > > struct v4l2_subdev_state { > > struct mutex lock; > > u32 which; > > struct v4l2_subdev_pad_config *pads; > > struct v4l2_subdev_krouting routing; > > struct v4l2_subdev_stream_configs stream_configs; > > }; > > > > So 'state' will hold 'TRY' information (only used for 'state' > > instances allocated in the fh) and 'ACTIVE' ones (used for states > > allocated in the sd). > > Right. > > > Looking at 'v4l2_subdev_pad_config' and 'v4l2_subdev_stream_config' they > > seem to describe more or less the same things: fmt, crop and compose > > (per pad-stream in case of stream_config). I wonder if those shouldn't > > be unified so that: > > > > 1) Drivers receive one state: the core passes in the 'correct' one > > (per-fh or per-sd) as you do in next patch > > Yes. But note that "old" drivers don't have active state. > > > 2) The 'which' information is not stored in the state but it's only > > 'contextual' (as in a parameter to the subdev operation) so that > > drivers inspect it to know if they have to apply settings to hw or not > > Yes, except we have init_cfg... > > > 3) v4l2_subdev_pad_config can be re-used and expanded, to maintain per-pad > > configurations regardless if they're ACTIVE or TRY, as this only depends > > on where the state is stored. > > pad_config is a static array of per-pad configs. stream_configs is a dynamic > per-stream config. stream_configs is a super-set of pad-configs, so we could > drop pad_configs, but it would require changing all the drivers in > non-trivial ways. > > v4l2_subdev_pad_config is not used or even allocated by the "new" drivers. > And routing & stream_configs are not used by the "old" drivers. > > > As I immagine it a subdev pad operation could look like: > > > > static int subdev_op(sd, pad, state, which, ...) > > { > > /* Doesn't matter if state is per-fh or the sd one. */ > > state->pads[pad].fmt = ....; > > > > if (which == TRY) > > return; > > > > /* Apply to the HW. */ > > } > > > > Does it make any sense to you ? I might have missed some reason why > > this is not possible. > > It makes sense, but there are the buts =). I've tried to explain these in > the commit messages, but it's kind of confusing. > > One but I haven't mentioned in the emails is that when subdev drivers call > ops in other subdev drivers they pass NULL in the state. This is fine for > the "old" drivers, as they expect a state only for TRY case. However, the > "new" drivers unfortunately expect to get a state on both TRY and ACTIVE > cases, and the only sensible way I figured out to handle this was the > v4l2_subdev_validate_state() function (patch 6). > > So, all this could be much neater, but would require modifying all subdev > drivers in non-trivial ways. I think this is something that can be done > slowly in the future. > > > > > > if (IS_ERR(sd_state)) > > > > > return PTR_ERR(sd_state); > > > > > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > > > > > index e40bca254b8b..63ea5e472c33 100644 > > > > > --- a/drivers/media/platform/vsp1/vsp1_entity.c > > > > > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > > > > > @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > > > > > * Allocate the pad configuration to store formats and selection > > > > > * rectangles. > > > > > */ > > > > > - entity->config = v4l2_alloc_subdev_state(&entity->subdev); > > > > > + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); > > > > > if (IS_ERR(entity->config)) { > > > > > media_entity_cleanup(&entity->subdev.entity); > > > > > return PTR_ERR(entity->config); > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > index e1a794f69815..04ad319fb150 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > > > > > { > > > > > struct v4l2_subdev_state *state; > > > > > > > > > > - state = v4l2_alloc_subdev_state(sd); > > > > > + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); > > > > > > > > At the same time I'm not sure I get the purpose of this. Don't > > > > init_cfg() callback implementations deal with try formats themeselves > > > > ? I mean, it's not a fixed rule, they can as well initialize their > > > > default 'active' formats, but what matters is that they initialize > > > > their per-fh try states ? > > > > > > That is what they do currently. init_cfg() only deals with TRY state, as > > > that's the only "state" (i.e. pad_config) there used to be from v4l2 core's > > > perspective. > > > > > > > Shouldn't init_cfg receive the fh's state so that it can initialize > > > > it, and just in case they need to, access their subdev's state and > > > > initialize them ? I'm missing what the purpose of the flag is tbh. > > > > > > Now we have (a possibility to have) state for both TRY and ACTIVE on the > > > v4l2 core side. The active state has to be initialized also, and a logical > > > way to do that is to use the init_cfg(). > > > > The 'ACTIVE' state is stored in the subdev, to which init_cfg() has > > access, and it receives the 'TRY' state as a parameter. > > No, init_cfg gets either ACTIVE or TRY state, whichever is being allocated. > For "old" drivers, ACTIVE state is never allocated so they don't get > init_cfg calls for ACTIVE at all. > > Aaand while writing that, I realized that some drivers manually do allocate > ACTIVE state temporarily, which would cause init_cfg with ACTIVE state to be > called. I wonder if they explode... Need to check. > > > It is possible to access both states and initialize them properly if > > I'm not mistaken. > > > > > > > > So now, for drivers that support the new active state, init_cfg() can get > > > either TRY or ACTIVE state. And if you want to call, say, the driver's > > > set_routing() to setup the routing in the state, you have to set the 'which' > > > in the routing struct to a value. So somehow init_cfg needs to know if it's > > > initializing an ACTIVE or TRY state. > > > > I'm not sure I got this part. set_routing() as other ops will receive > > a state and 'which'. If my proposal above makes sensem where the state > > Yes, but if it's init_cfg calling set_routing, init_cfg has to figure out > the 'which' from somewhere. > > E.g. init_cfg from ub913 driver: > > static int ub913_init_cfg(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state) > { > u32 which = state->which; > > struct v4l2_subdev_route routes[] = { > { > .sink_pad = 0, > .sink_stream = 0, > .source_pad = 1, > .source_stream = 0, > .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > }, > }; > > struct v4l2_subdev_krouting routing = { > .which = which, > .num_routes = ARRAY_SIZE(routes), > .routes = routes, > }; > > return ub913_set_routing(sd, state, &routing); > } > > It uses set_routing to setup a default routing (and set_routing in turn also > initializes the formats), but set_routing needs 'which'. > > Tomi
Hi, On 16/09/2021 16:07, Jacopo Mondi wrote: > Also note that operations like s_stream do not take a state as > parameter. The driver has to fetch it from the subdev anyway > (this in reply to the idea of having the active state as parameter vs > retrieving it from the subdev if ACTIVE) > > While porting the R-Car drivers on top of this series I found myself > in the need to (in the s_stream call chain) > > static int rcsi2_start_receiver(struct rcar_csi2 *priv) > { > const struct v4l2_subdev_state *state = priv->subdev.state; > const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; > > ... > > /* > * Configure field handling inspecting the formats of the > * single sink pad streams. > */ > for (i = 0; i < configs->num_configs; ++i) { > const struct v4l2_subdev_stream_config *config = configs->configs; > if (config->pad != RCAR_CSI2_SINK) > continue; > > if (config->fmt.field != V4L2_FIELD_ALTERNATE) > continue; > > fld |= FLD_DET_SEL(1); > fld |= FLD_FLD_EN(config->stream); > > /* PAL vs NTSC. */ > if (config->fmt.height == 240) > fld |= FLD_FLD_NUM(0); > else > fld |= FLD_FLD_NUM(1); > } > > ... > > } > > Am I doing it wrong, or is this a case for the subdev to have to > directly access sd->state ? In s_stream path you should: state = v4l2_subdev_lock_active_state(sd); <do the work with the state> v4l2_subdev_unlock_state(state); If you already have the state, e.g. in set_fmt: state = v4l2_subdev_validate_and_lock_state(sd, state); <do the work with the state> v4l2_subdev_unlock_state(state); Accessing the stream_configs directly is fine but not that nice. I did think about some helpers, perhaps for_each_stream_config(), but I didn't add that as I didn't have the need. There's v4l2_state_get_stream_format() which can be used in many cases, but we probably need something else if you need to iterate over all the configs. Tomi
Hello, On Thu, Sep 16, 2021 at 03:07:52PM +0200, Jacopo Mondi wrote: > On Mon, Sep 13, 2021 at 05:26:45PM +0300, Tomi Valkeinen wrote: > > On 13/09/2021 16:38, Jacopo Mondi wrote: > > > On Mon, Sep 13, 2021 at 03:17:01PM +0300, Tomi Valkeinen wrote: > > > > On 13/09/2021 14:41, Jacopo Mondi wrote: > > > > > On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: > > > > > > The subdev state is passed to functions in the media drivers, and > > > > > > usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is > > > > > > also given to the function in one way or another. > > > > > > > > > > > > One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One > > > > > > could argue that the initialization of the state should be the same for > > > > > > both ACTIVE and TRY cases, but unfortunately that is not the case: > > > > > > > > > > > > - Some drivers do also other things than just touch the state when > > > > > > dealing with ACTIVE, e.g. if there is extra state outside the standard > > > > > > subdev state. > > > > > > - Some drivers might need to create, say, struct v4l2_subdev_format > > > > > > which has 'which' field, and that needs to be filled with either ACTIVE > > > > > > or TRY. > > > > > > > > > > > > Currently init_cfg is only called for TRY case from the v4l2 framework, > > > > > > passing the TRY state. Some drivers call their own init_cfg, passing > > > > > > NULL as the state, which is used to indicate ACTIVE case. > > > > > > > > > > > > In the future we want to pass subdev's active state from the v4l2 > > > > > > framework side, so we need a solution to this. > > > > > > > > > > > > We could change the init_cfg() to include the TRY/ACTIVE value, which > > > > > > would require changing more or less all the drivers. Instead, I have > > > > > > added 'which' field to the subdev state itself, filled at state > > > > > > allocation time, which only requires changes to the drivers that > > > > > > allocate a state themselves. > > > > > > > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > > > > --- > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- > > > > > > drivers/media/platform/vsp1/vsp1_entity.c | 2 +- > > > > > > drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- > > > > > > drivers/staging/media/tegra-video/vi.c | 2 +- > > > > > > include/media/v4l2-subdev.h | 7 ++++++- > > > > > > 5 files changed, 16 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > index 5f4fa8c48f68..1de30d5b437f 100644 > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > > > > @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, > > > > > > u32 width, height; > > > > > > int ret; > > > > > > > > > > > > - sd_state = v4l2_alloc_subdev_state(sd); > > > > > > + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); > > > > > > > > > > Shouldn't the 'which' parameters be used to decide if either ACTIVE or > > > > > TRY have to be used ? this function is also used to set TRY formats, > > > > > in example... > > > > > > > > > > Oh, maybe I got how it works, the state's which is not > > > > > relevant but the v4l2_subdev_format's which is, as it will be used in > > > > > the next patch to decide if the subdev's state of the file-handle's > > > > > state should be passed to the ioctl. > > > > > > > > Yes. It's messy, but it's how it worked before also. > > > > > > > > The drivers can't really allocate TRY state as it must come from the core, > > > > based on the filehandle. Now as I say that, makes me wonder why even expose > > > > the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the > > > > sd parameter, and always allocate ACTIVE state, and the v4l2 core can use > > > > another way to create the TRY state. Drivers should not allocate state manually, it should always be done by the core (when opening a file handle for the TRY states, and when initializing the subdev in the probe function for the ACTIVE state). Looking at the three drivers that call v4l2_alloc_subdev_state(), we have the vsp1 driver that uses it to allocate its own ACTIVE state at init time. This one is easy, it should be replaced with v4l2_subdev_alloc_state() (or whatever that function will end up being named, see comments to 02/36). The rcar-vin and tegra-video drivers are doing it wrong. rcar-vin has legacy code to support unfortunate design decisions (everything in the "V4L2" section in rcar-v4l2.c), and newer code that gets it right (the "V4L2 Media Controller" section in the same file). The legacy code is used on older platforms, and the newer code on newer platforms. I know that Niklas would love to get rid of the legacy code, and I'd also be happy to see it go. If that's not possible, we'll have to keep exposing v4l2_alloc_subdev_state() for this driver. Niklas, what do you think, could we drop the legacy code after all those years ? The situation in tegra-video is similar. They got it wrong. The driver is in staging though, so that's fixable. I'd propose renaming v4l2_subdev_alloc_state() to __v4l2_subdev_alloc_state() (or something that shows it's internal), documenting that it must not be used by new drivers, and adding an entry in the TODO file of the tegra-video driver to fix this. > > > init_cfg() as well as other operations used to received an > > > array of per fh's pad_configs, and the sd pointer itself. The fh's pad > > > configs were allocated by the core, as well as the core now allocates > > > the per-fh's state. > > > > > > Before the introduction of 'state', if the 'which' flags was set to > > > TRY then information were stored/retrieved/initialized in the per-fh > > > pad_config, otherwise the active configuration (usually stored in the > > > driver main structure) was used. Correct, and the active configuration is in that case stored in a driver-specific way (except in the vsp1 driver that uses the pad config structure to store it), with ad-hoc accessors and lots of manual checks in the code paths. > > > So we had a clear separation of per-fh information and the active > > > state. The core didn't care afaict, and passed in both, then driver had > > > to deal with them doing the right thing by inspecting the 'which' flag. > > > > > > The typical pattern was: > > > > > > static int subdev_ops(sd, pad_cfg, which) > > > { > > > if (which == TRY) > > > /* Operate on config stored in pad_cfg */ > > > > > > else > > > /* > > > * Operate on config stored in subdev (and > > > * applied to HW) > > > */ > > > } > > > > > > Or am I overlooking some cases or do you agree with my understanding > > > so far ? > > > > More or less, yes. I think there are (used to be) three kinds of ops: > > > > - Ops that get pad_cfg and 'which' in an op specific struct. E.g. set_fmt. > > The pad_cfg is TRY pad_config, even if 'which' == ACTIVE. And pad_cfg is ignored in those drivers when which == ACTIVE (or it should be at least, if it's not, it's a driver bug). > > - Ops that don't get pad_cfg, like s_stream. 'which' is implicitly ACTIVE. .s_stream() on a TRY configuration would be an interesting concept :-) > Also note that operations like s_stream do not take a state as > parameter. The driver has to fetch it from the subdev anyway > (this in reply to the idea of having the active state as parameter vs > retrieving it from the subdev if ACTIVE) We could pass the state as a parameter, but given that the operation always operates on the ACTIVE state by definition, I think this is redundant. > While porting the R-Car drivers on top of this series I found myself > in the need to (in the s_stream call chain) > > static int rcsi2_start_receiver(struct rcar_csi2 *priv) > { > const struct v4l2_subdev_state *state = priv->subdev.state; > const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; > > ... > > /* > * Configure field handling inspecting the formats of the > * single sink pad streams. > */ > for (i = 0; i < configs->num_configs; ++i) { > const struct v4l2_subdev_stream_config *config = configs->configs; > if (config->pad != RCAR_CSI2_SINK) > continue; > > if (config->fmt.field != V4L2_FIELD_ALTERNATE) > continue; > > fld |= FLD_DET_SEL(1); > fld |= FLD_FLD_EN(config->stream); > > /* PAL vs NTSC. */ > if (config->fmt.height == 240) > fld |= FLD_FLD_NUM(0); > else > fld |= FLD_FLD_NUM(1); > } > > ... > > } > > Am I doing it wrong, or is this a case for the subdev to have to > directly access sd->state ? (Will reply to Tomi's reply on this) > > - init_cfg which gets pad_cfg, but no which (as 'which' is always implicitly > > TRY) As proposed in 02/36, I think .init_cfg() should operate the same way regardless of whether the state is TRY or ACTIVE, so there's no need for a 'which' parameter (it would only open the door to abuses). > > So pad_cfg was TRY state. Drivers could use pad_configs internally to track > > ACTIVE state, but the core had no knowledge about this. Correct. > > > Now we have a 'state' that holds the array of pad_configs and along > > > the continuation of the series will end up holding per-pad > > > configurations. > > > > > > We now also have one 'state' per file-handle, and one > > > per-subdev. As I see this, it would be natual for drivers to receive > > > one state without knowing where it comes from. In the next patch you > > > > Note that only subdev's that explicitly support the new state code, and > > allocate the state, have the subdev active state. Which means only the > > drivers in my work branch. > > > > The "old" drivers work like they used to: they get the state (essentially > > repackaged pad_cfg) for TRY cases, NULL otherwise. > > > > And yes, it would be natural to just get a state, but the subdev drivers > > need to know if the context is TRY/ACTIVE. As you can see from the bullet > > list above, the driver knows this in all the other places except init_cfg. I agree that porting all the drivers as part of this series isn't feasible, so passing NULL for the state in case the driver hasn't explicitly opted-in seems fine to me. > > > instrument the core to do exactly this: inspect the which flag and > > > pass in the 'right' state. Ofc drivers need to have access to 'which' > > > to know if they have to apply settings to the HW or not. > > > > > > Looking ahead in your series I see these structures: > > > > > > struct v4l2_subdev_pad_config { > > > struct v4l2_mbus_framefmt try_fmt; > > > struct v4l2_rect try_crop; > > > struct v4l2_rect try_compose; > > > }; > > > > > > struct v4l2_subdev_stream_config { > > > u32 pad; > > > u32 stream; > > > > > > struct v4l2_mbus_framefmt fmt; > > > struct v4l2_rect crop; > > > struct v4l2_rect compose; > > > }; > > > > > > struct v4l2_subdev_stream_configs { > > > u32 num_configs; > > > struct v4l2_subdev_stream_config *configs; > > > }; > > > > > > All of them part of state: > > > > > > struct v4l2_subdev_state { > > > struct mutex lock; > > > u32 which; > > > struct v4l2_subdev_pad_config *pads; > > > struct v4l2_subdev_krouting routing; > > > struct v4l2_subdev_stream_configs stream_configs; > > > }; > > > > > > So 'state' will hold 'TRY' information (only used for 'state' > > > instances allocated in the fh) and 'ACTIVE' ones (used for states > > > allocated in the sd). > > > > Right. > > > > > Looking at 'v4l2_subdev_pad_config' and 'v4l2_subdev_stream_config' they > > > seem to describe more or less the same things: fmt, crop and compose > > > (per pad-stream in case of stream_config). I wonder if those shouldn't > > > be unified so that: > > > > > > 1) Drivers receive one state: the core passes in the 'correct' one > > > (per-fh or per-sd) as you do in next patch > > > > Yes. But note that "old" drivers don't have active state. > > > > > 2) The 'which' information is not stored in the state but it's only > > > 'contextual' (as in a parameter to the subdev operation) so that > > > drivers inspect it to know if they have to apply settings to hw or not > > > > Yes, except we have init_cfg... This sounds like .init_cfg() is the only blocker, which I think is promissing :-) > > > 3) v4l2_subdev_pad_config can be re-used and expanded, to maintain per-pad > > > configurations regardless if they're ACTIVE or TRY, as this only depends > > > on where the state is stored. > > > > pad_config is a static array of per-pad configs. stream_configs is a dynamic > > per-stream config. Do I understand correctly that it's both per-stream and per-pad ? I wonder if it could make sense to store it that way, with an array of per-pad configurations (v4l2_subdev_pad_config), which will in turn contain per-stream configuration. I suppose it depends on the usage patterns, which I'll understand better when reading the rest of the series. > > stream_configs is a super-set of pad-configs, so we could > > drop pad_configs, but it would require changing all the drivers in > > non-trivial ways. I like Jacopo's proposal to unify the two, but I do agree that it's not trivial. Here again I think it falls in the category of setting a long term goal and trying to go in that direction, without necessarily reaching it just yet. I wonder if we could have helper functions (maybe they're already included later in the series ?) to abstract the pad_config/stream_configs difference for drivers. This would also make further reworks easier (such as storing the per-stream configuration in v4l2_subdev_pad_config as proposed above) if we decide to address some of the changes later. > > v4l2_subdev_pad_config is not used or even allocated by the "new" drivers. > > And routing & stream_configs are not used by the "old" drivers. > > > > > As I immagine it a subdev pad operation could look like: > > > > > > static int subdev_op(sd, pad, state, which, ...) > > > { > > > /* Doesn't matter if state is per-fh or the sd one. */ > > > state->pads[pad].fmt = ....; > > > > > > if (which == TRY) > > > return; > > > > > > /* Apply to the HW. */ > > > } > > > > > > Does it make any sense to you ? I might have missed some reason why > > > this is not possible. > > > > It makes sense, but there are the buts =). I've tried to explain these in > > the commit messages, but it's kind of confusing. > > > > One but I haven't mentioned in the emails is that when subdev drivers call > > ops in other subdev drivers they pass NULL in the state. We should really really try to avoid that. s_stream() is the obvious main exception. Other subdev operations, such as .get_fmt() or .set_fmt(), shouldn't be called cross-subdevs. A subdev should only care about its own configuration, not about its neighbours. Are there blockers here ? > > This is fine for > > the "old" drivers, as they expect a state only for TRY case. However, the > > "new" drivers unfortunately expect to get a state on both TRY and ACTIVE > > cases, and the only sensible way I figured out to handle this was the > > v4l2_subdev_validate_state() function (patch 6). > > > > So, all this could be much neater, but would require modifying all subdev > > drivers in non-trivial ways. I think this is something that can be done > > slowly in the future. > > > > > > > > if (IS_ERR(sd_state)) > > > > > > return PTR_ERR(sd_state); > > > > > > > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > > > > > > index e40bca254b8b..63ea5e472c33 100644 > > > > > > --- a/drivers/media/platform/vsp1/vsp1_entity.c > > > > > > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > > > > > > @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > > > > > > * Allocate the pad configuration to store formats and selection > > > > > > * rectangles. > > > > > > */ > > > > > > - entity->config = v4l2_alloc_subdev_state(&entity->subdev); > > > > > > + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); > > > > > > if (IS_ERR(entity->config)) { > > > > > > media_entity_cleanup(&entity->subdev.entity); > > > > > > return PTR_ERR(entity->config); > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > index e1a794f69815..04ad319fb150 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > > > > > > { > > > > > > struct v4l2_subdev_state *state; > > > > > > > > > > > > - state = v4l2_alloc_subdev_state(sd); > > > > > > + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); > > > > > > > > > > At the same time I'm not sure I get the purpose of this. Don't > > > > > init_cfg() callback implementations deal with try formats themeselves > > > > > ? I mean, it's not a fixed rule, they can as well initialize their > > > > > default 'active' formats, but what matters is that they initialize > > > > > their per-fh try states ? > > > > > > > > That is what they do currently. init_cfg() only deals with TRY state, as > > > > that's the only "state" (i.e. pad_config) there used to be from v4l2 core's > > > > perspective. > > > > > > > > > Shouldn't init_cfg receive the fh's state so that it can initialize > > > > > it, and just in case they need to, access their subdev's state and > > > > > initialize them ? I'm missing what the purpose of the flag is tbh. > > > > > > > > Now we have (a possibility to have) state for both TRY and ACTIVE on the > > > > v4l2 core side. The active state has to be initialized also, and a logical > > > > way to do that is to use the init_cfg(). > > > > > > The 'ACTIVE' state is stored in the subdev, to which init_cfg() has > > > access, and it receives the 'TRY' state as a parameter. > > > > No, init_cfg gets either ACTIVE or TRY state, whichever is being allocated. > > For "old" drivers, ACTIVE state is never allocated so they don't get > > init_cfg calls for ACTIVE at all. > > > > Aaand while writing that, I realized that some drivers manually do allocate > > ACTIVE state temporarily, which would cause init_cfg with ACTIVE state to be > > called. I wonder if they explode... Need to check. Is this only rcar-vin and tegra-video (vsp1 also allocates an ACTIVE state, but it's not temporary), or are there other drivers ? > > > It is possible to access both states and initialize them properly if > > > I'm not mistaken. > > > > > > > So now, for drivers that support the new active state, init_cfg() can get > > > > either TRY or ACTIVE state. And if you want to call, say, the driver's > > > > set_routing() to setup the routing in the state, you have to set the 'which' > > > > in the routing struct to a value. So somehow init_cfg needs to know if it's > > > > initializing an ACTIVE or TRY state. > > > > > > I'm not sure I got this part. set_routing() as other ops will receive > > > a state and 'which'. If my proposal above makes sensem where the state > > > > Yes, but if it's init_cfg calling set_routing, init_cfg has to figure out > > the 'which' from somewhere. > > > > E.g. init_cfg from ub913 driver: > > > > static int ub913_init_cfg(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *state) > > { > > u32 which = state->which; > > > > struct v4l2_subdev_route routes[] = { > > { > > .sink_pad = 0, > > .sink_stream = 0, > > .source_pad = 1, > > .source_stream = 0, > > .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > > }, > > }; > > > > struct v4l2_subdev_krouting routing = { > > .which = which, > > .num_routes = ARRAY_SIZE(routes), > > .routes = routes, > > }; > > > > return ub913_set_routing(sd, state, &routing); > > } > > > > It uses set_routing to setup a default routing (and set_routing in turn also > > initializes the formats), but set_routing needs 'which'. Could we avoid such patterns, by either initializing the routing manually here, or splitting the logic of ub913_set_routing() in two functions, with common code called by ub913_set_routing() and ub913_init_cfg() ? I think calling set functions from .init_cfg() opens the door to abuses or just bugs, and I increasingly think it should be discouraged. I know I've written quite a few drivers that call .set_fmt() from .init_cfg() though, it does make things easier to some extent, but I wonder if we could do better. This leads to the question of where to initialize the hardware state, which is the part of the set functions that is only executed in the ACTIVE case. I don't think this should be done at probe time in general, but at .s_stream() time. Only drivers that allow changing formats or routing while streaming would need to perform any hardware configutation in the set functions, and I'm not even sure we have a single driver in the kernel that allows this (or, to be precise, if we have a single driver that knowingly allows this, I'm sure we have a bunch of drivers that don't prevent this situation and will crash or behave in other bad ways if userspace tried to change the configuration while streaming). Maybe we should split .set_fmt() and .set_routing() in two for these very uncommon cases, with the hardware configuration moved to separate functions ? It has the potential to simplify the set operations and make them safer. -- Regards, Laurent Pinchart
Hi Tomi, On Thu, Sep 16, 2021 at 04:24:19PM +0300, Tomi Valkeinen wrote: > On 16/09/2021 16:07, Jacopo Mondi wrote: > > > Also note that operations like s_stream do not take a state as > > parameter. The driver has to fetch it from the subdev anyway > > (this in reply to the idea of having the active state as parameter vs > > retrieving it from the subdev if ACTIVE) > > > > While porting the R-Car drivers on top of this series I found myself > > in the need to (in the s_stream call chain) > > > > static int rcsi2_start_receiver(struct rcar_csi2 *priv) > > { > > const struct v4l2_subdev_state *state = priv->subdev.state; > > const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; > > > > ... > > > > /* > > * Configure field handling inspecting the formats of the > > * single sink pad streams. > > */ > > for (i = 0; i < configs->num_configs; ++i) { > > const struct v4l2_subdev_stream_config *config = configs->configs; > > if (config->pad != RCAR_CSI2_SINK) > > continue; > > > > if (config->fmt.field != V4L2_FIELD_ALTERNATE) > > continue; > > > > fld |= FLD_DET_SEL(1); > > fld |= FLD_FLD_EN(config->stream); > > > > /* PAL vs NTSC. */ > > if (config->fmt.height == 240) > > fld |= FLD_FLD_NUM(0); > > else > > fld |= FLD_FLD_NUM(1); > > } > > > > ... > > > > } > > > > Am I doing it wrong, or is this a case for the subdev to have to > > directly access sd->state ? > > In s_stream path you should: > > state = v4l2_subdev_lock_active_state(sd); > > <do the work with the state> > > v4l2_subdev_unlock_state(state); > > If you already have the state, e.g. in set_fmt: > > state = v4l2_subdev_validate_and_lock_state(sd, state); > > <do the work with the state> > > v4l2_subdev_unlock_state(state); > > Accessing the stream_configs directly is fine but not that nice. I did > think about some helpers, perhaps for_each_stream_config(), but I didn't > add that as I didn't have the need. > > There's v4l2_state_get_stream_format() which can be used in many cases, > but we probably need something else if you need to iterate over all the > configs. I really like forcing drivers to call functions that will lock the state, at least until we can move the locks to the core (if ever). We should move the fields of v4l2_subdev that drivers are not supposed to access directly under a big PRIVATE comment. -- Regards, Laurent Pinchart
On 27/09/2021 03:46, Laurent Pinchart wrote: > Hello, > > On Thu, Sep 16, 2021 at 03:07:52PM +0200, Jacopo Mondi wrote: >> On Mon, Sep 13, 2021 at 05:26:45PM +0300, Tomi Valkeinen wrote: >>> On 13/09/2021 16:38, Jacopo Mondi wrote: >>>> On Mon, Sep 13, 2021 at 03:17:01PM +0300, Tomi Valkeinen wrote: >>>>> On 13/09/2021 14:41, Jacopo Mondi wrote: >>>>>> On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: >>>>>>> The subdev state is passed to functions in the media drivers, and >>>>>>> usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is >>>>>>> also given to the function in one way or another. >>>>>>> >>>>>>> One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One >>>>>>> could argue that the initialization of the state should be the same for >>>>>>> both ACTIVE and TRY cases, but unfortunately that is not the case: >>>>>>> >>>>>>> - Some drivers do also other things than just touch the state when >>>>>>> dealing with ACTIVE, e.g. if there is extra state outside the standard >>>>>>> subdev state. >>>>>>> - Some drivers might need to create, say, struct v4l2_subdev_format >>>>>>> which has 'which' field, and that needs to be filled with either ACTIVE >>>>>>> or TRY. >>>>>>> >>>>>>> Currently init_cfg is only called for TRY case from the v4l2 framework, >>>>>>> passing the TRY state. Some drivers call their own init_cfg, passing >>>>>>> NULL as the state, which is used to indicate ACTIVE case. >>>>>>> >>>>>>> In the future we want to pass subdev's active state from the v4l2 >>>>>>> framework side, so we need a solution to this. >>>>>>> >>>>>>> We could change the init_cfg() to include the TRY/ACTIVE value, which >>>>>>> would require changing more or less all the drivers. Instead, I have >>>>>>> added 'which' field to the subdev state itself, filled at state >>>>>>> allocation time, which only requires changes to the drivers that >>>>>>> allocate a state themselves. >>>>>>> >>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>>>>> --- >>>>>>> drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- >>>>>>> drivers/media/platform/vsp1/vsp1_entity.c | 2 +- >>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- >>>>>>> drivers/staging/media/tegra-video/vi.c | 2 +- >>>>>>> include/media/v4l2-subdev.h | 7 ++++++- >>>>>>> 5 files changed, 16 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c >>>>>>> index 5f4fa8c48f68..1de30d5b437f 100644 >>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c >>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c >>>>>>> @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, >>>>>>> u32 width, height; >>>>>>> int ret; >>>>>>> >>>>>>> - sd_state = v4l2_alloc_subdev_state(sd); >>>>>>> + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); >>>>>> >>>>>> Shouldn't the 'which' parameters be used to decide if either ACTIVE or >>>>>> TRY have to be used ? this function is also used to set TRY formats, >>>>>> in example... >>>>>> >>>>>> Oh, maybe I got how it works, the state's which is not >>>>>> relevant but the v4l2_subdev_format's which is, as it will be used in >>>>>> the next patch to decide if the subdev's state of the file-handle's >>>>>> state should be passed to the ioctl. >>>>> >>>>> Yes. It's messy, but it's how it worked before also. >>>>> >>>>> The drivers can't really allocate TRY state as it must come from the core, >>>>> based on the filehandle. Now as I say that, makes me wonder why even expose >>>>> the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the >>>>> sd parameter, and always allocate ACTIVE state, and the v4l2 core can use >>>>> another way to create the TRY state. > > Drivers should not allocate state manually, it should always be done by > the core (when opening a file handle for the TRY states, and when > initializing the subdev in the probe function for the ACTIVE state). > > Looking at the three drivers that call v4l2_alloc_subdev_state(), we > have the vsp1 driver that uses it to allocate its own ACTIVE state at > init time. This one is easy, it should be replaced with > v4l2_subdev_alloc_state() (or whatever that function will end up being > named, see comments to 02/36). I don't follow here. First you say that drivers should not allocate state. But then, I think, you say that vsp1 allocating state is fine after some change? My understanding is that vsp1 is fine. Of course, if the function it calls gets renamed we need to rename it in vsp1 too. > The rcar-vin and tegra-video drivers are doing it wrong. rcar-vin has > legacy code to support unfortunate design decisions (everything in the > "V4L2" section in rcar-v4l2.c), and newer code that gets it right (the > "V4L2 Media Controller" section in the same file). The legacy code is > used on older platforms, and the newer code on newer platforms. I know > that Niklas would love to get rid of the legacy code, and I'd also be > happy to see it go. If that's not possible, we'll have to keep exposing > v4l2_alloc_subdev_state() for this driver. Niklas, what do you think, > could we drop the legacy code after all those years ? > > The situation in tegra-video is similar. They got it wrong. The driver > is in staging though, so that's fixable. > > I'd propose renaming v4l2_subdev_alloc_state() to > __v4l2_subdev_alloc_state() (or something that shows it's internal), > documenting that it must not be used by new drivers, and adding an entry > in the TODO file of the tegra-video driver to fix this. Yes, that sounds fine. Subdev drivers allocating the state is not something that should be done in any new drivers. >>>> init_cfg() as well as other operations used to received an >>>> array of per fh's pad_configs, and the sd pointer itself. The fh's pad >>>> configs were allocated by the core, as well as the core now allocates >>>> the per-fh's state. >>>> >>>> Before the introduction of 'state', if the 'which' flags was set to >>>> TRY then information were stored/retrieved/initialized in the per-fh >>>> pad_config, otherwise the active configuration (usually stored in the >>>> driver main structure) was used. > > Correct, and the active configuration is in that case stored in a > driver-specific way (except in the vsp1 driver that uses the pad config > structure to store it), with ad-hoc accessors and lots of manual checks > in the code paths. > >>>> So we had a clear separation of per-fh information and the active >>>> state. The core didn't care afaict, and passed in both, then driver had >>>> to deal with them doing the right thing by inspecting the 'which' flag. >>>> >>>> The typical pattern was: >>>> >>>> static int subdev_ops(sd, pad_cfg, which) >>>> { >>>> if (which == TRY) >>>> /* Operate on config stored in pad_cfg */ >>>> >>>> else >>>> /* >>>> * Operate on config stored in subdev (and >>>> * applied to HW) >>>> */ >>>> } >>>> >>>> Or am I overlooking some cases or do you agree with my understanding >>>> so far ? >>> >>> More or less, yes. I think there are (used to be) three kinds of ops: >>> >>> - Ops that get pad_cfg and 'which' in an op specific struct. E.g. set_fmt. >>> The pad_cfg is TRY pad_config, even if 'which' == ACTIVE. > > And pad_cfg is ignored in those drivers when which == ACTIVE (or it > should be at least, if it's not, it's a driver bug). > >>> - Ops that don't get pad_cfg, like s_stream. 'which' is implicitly ACTIVE. > > .s_stream() on a TRY configuration would be an interesting concept :-) I think it would be a good one, though =). It could be used to validate the whole pipeline. But it would probably be rather difficult to implement with the current v4l2 framework. >> Also note that operations like s_stream do not take a state as >> parameter. The driver has to fetch it from the subdev anyway >> (this in reply to the idea of having the active state as parameter vs >> retrieving it from the subdev if ACTIVE) > > We could pass the state as a parameter, but given that the operation > always operates on the ACTIVE state by definition, I think this is > redundant. > >> While porting the R-Car drivers on top of this series I found myself >> in the need to (in the s_stream call chain) >> >> static int rcsi2_start_receiver(struct rcar_csi2 *priv) >> { >> const struct v4l2_subdev_state *state = priv->subdev.state; >> const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; >> >> ... >> >> /* >> * Configure field handling inspecting the formats of the >> * single sink pad streams. >> */ >> for (i = 0; i < configs->num_configs; ++i) { >> const struct v4l2_subdev_stream_config *config = configs->configs; >> if (config->pad != RCAR_CSI2_SINK) >> continue; >> >> if (config->fmt.field != V4L2_FIELD_ALTERNATE) >> continue; >> >> fld |= FLD_DET_SEL(1); >> fld |= FLD_FLD_EN(config->stream); >> >> /* PAL vs NTSC. */ >> if (config->fmt.height == 240) >> fld |= FLD_FLD_NUM(0); >> else >> fld |= FLD_FLD_NUM(1); >> } >> >> ... >> >> } >> >> Am I doing it wrong, or is this a case for the subdev to have to >> directly access sd->state ? > > (Will reply to Tomi's reply on this) > >>> - init_cfg which gets pad_cfg, but no which (as 'which' is always implicitly >>> TRY) > > As proposed in 02/36, I think .init_cfg() should operate the same way > regardless of whether the state is TRY or ACTIVE, so there's no need for > a 'which' parameter (it would only open the door to abuses). > >>> So pad_cfg was TRY state. Drivers could use pad_configs internally to track >>> ACTIVE state, but the core had no knowledge about this. > > Correct. > >>>> Now we have a 'state' that holds the array of pad_configs and along >>>> the continuation of the series will end up holding per-pad >>>> configurations. >>>> >>>> We now also have one 'state' per file-handle, and one >>>> per-subdev. As I see this, it would be natual for drivers to receive >>>> one state without knowing where it comes from. In the next patch you >>> >>> Note that only subdev's that explicitly support the new state code, and >>> allocate the state, have the subdev active state. Which means only the >>> drivers in my work branch. >>> >>> The "old" drivers work like they used to: they get the state (essentially >>> repackaged pad_cfg) for TRY cases, NULL otherwise. >>> >>> And yes, it would be natural to just get a state, but the subdev drivers >>> need to know if the context is TRY/ACTIVE. As you can see from the bullet >>> list above, the driver knows this in all the other places except init_cfg. > > I agree that porting all the drivers as part of this series isn't > feasible, so passing NULL for the state in case the driver hasn't > explicitly opted-in seems fine to me. > >>>> instrument the core to do exactly this: inspect the which flag and >>>> pass in the 'right' state. Ofc drivers need to have access to 'which' >>>> to know if they have to apply settings to the HW or not. >>>> >>>> Looking ahead in your series I see these structures: >>>> >>>> struct v4l2_subdev_pad_config { >>>> struct v4l2_mbus_framefmt try_fmt; >>>> struct v4l2_rect try_crop; >>>> struct v4l2_rect try_compose; >>>> }; >>>> >>>> struct v4l2_subdev_stream_config { >>>> u32 pad; >>>> u32 stream; >>>> >>>> struct v4l2_mbus_framefmt fmt; >>>> struct v4l2_rect crop; >>>> struct v4l2_rect compose; >>>> }; >>>> >>>> struct v4l2_subdev_stream_configs { >>>> u32 num_configs; >>>> struct v4l2_subdev_stream_config *configs; >>>> }; >>>> >>>> All of them part of state: >>>> >>>> struct v4l2_subdev_state { >>>> struct mutex lock; >>>> u32 which; >>>> struct v4l2_subdev_pad_config *pads; >>>> struct v4l2_subdev_krouting routing; >>>> struct v4l2_subdev_stream_configs stream_configs; >>>> }; >>>> >>>> So 'state' will hold 'TRY' information (only used for 'state' >>>> instances allocated in the fh) and 'ACTIVE' ones (used for states >>>> allocated in the sd). >>> >>> Right. >>> >>>> Looking at 'v4l2_subdev_pad_config' and 'v4l2_subdev_stream_config' they >>>> seem to describe more or less the same things: fmt, crop and compose >>>> (per pad-stream in case of stream_config). I wonder if those shouldn't >>>> be unified so that: >>>> >>>> 1) Drivers receive one state: the core passes in the 'correct' one >>>> (per-fh or per-sd) as you do in next patch >>> >>> Yes. But note that "old" drivers don't have active state. >>> >>>> 2) The 'which' information is not stored in the state but it's only >>>> 'contextual' (as in a parameter to the subdev operation) so that >>>> drivers inspect it to know if they have to apply settings to hw or not >>> >>> Yes, except we have init_cfg... > > This sounds like .init_cfg() is the only blocker, which I think is > promissing :-) > >>>> 3) v4l2_subdev_pad_config can be re-used and expanded, to maintain per-pad >>>> configurations regardless if they're ACTIVE or TRY, as this only depends >>>> on where the state is stored. >>> >>> pad_config is a static array of per-pad configs. stream_configs is a dynamic >>> per-stream config. > > Do I understand correctly that it's both per-stream and per-pad ? I > wonder if it could make sense to store it that way, with an array of > per-pad configurations (v4l2_subdev_pad_config), which will in turn > contain per-stream configuration. I suppose it depends on the usage > patterns, which I'll understand better when reading the rest of the > series. I'm not sure what you mean with "both per-stream and per-pad". It's per (stream, pad) tuple. At the moment the internal storage is essentially just an array of { pad, stream, config } items. It could also be changed as you suggest, but afaics that would require more dynamic allocations. The APIs should be designed so that the drivers don't need to care about the internal storage. >>> stream_configs is a super-set of pad-configs, so we could >>> drop pad_configs, but it would require changing all the drivers in >>> non-trivial ways. > > I like Jacopo's proposal to unify the two, but I do agree that it's not > trivial. Here again I think it falls in the category of setting a long > term goal and trying to go in that direction, without necessarily > reaching it just yet. I wonder if we could have helper functions (maybe > they're already included later in the series ?) to abstract the > pad_config/stream_configs difference for drivers. This would also make > further reworks easier (such as storing the per-stream configuration in > v4l2_subdev_pad_config as proposed above) if we decide to address some > of the changes later. If you mean do we have functions to hide the stream (i.e. stream = 0), no, I haven't added such. I have not worked towards making non-multiplexed-streams subdev drivers use the new APIs. That said, I have tried to design this series so that it is possible. And "new" and "old" drivers should be compatible with each other. >>> v4l2_subdev_pad_config is not used or even allocated by the "new" drivers. >>> And routing & stream_configs are not used by the "old" drivers. >>> >>>> As I immagine it a subdev pad operation could look like: >>>> >>>> static int subdev_op(sd, pad, state, which, ...) >>>> { >>>> /* Doesn't matter if state is per-fh or the sd one. */ >>>> state->pads[pad].fmt = ....; >>>> >>>> if (which == TRY) >>>> return; >>>> >>>> /* Apply to the HW. */ >>>> } >>>> >>>> Does it make any sense to you ? I might have missed some reason why >>>> this is not possible. >>> >>> It makes sense, but there are the buts =). I've tried to explain these in >>> the commit messages, but it's kind of confusing. >>> >>> One but I haven't mentioned in the emails is that when subdev drivers call >>> ops in other subdev drivers they pass NULL in the state. > > We should really really try to avoid that. s_stream() is the obvious > main exception. Other subdev operations, such as .get_fmt() or > .set_fmt(), shouldn't be called cross-subdevs. A subdev should only care > about its own configuration, not about its neighbours. Are there > blockers here ? There are a lot of subdevs using v4l2_subdev_call(), including uses of set_fmt. >>> This is fine for >>> the "old" drivers, as they expect a state only for TRY case. However, the >>> "new" drivers unfortunately expect to get a state on both TRY and ACTIVE >>> cases, and the only sensible way I figured out to handle this was the >>> v4l2_subdev_validate_state() function (patch 6). >>> >>> So, all this could be much neater, but would require modifying all subdev >>> drivers in non-trivial ways. I think this is something that can be done >>> slowly in the future. >>> >>>>>>> if (IS_ERR(sd_state)) >>>>>>> return PTR_ERR(sd_state); >>>>>>> >>>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c >>>>>>> index e40bca254b8b..63ea5e472c33 100644 >>>>>>> --- a/drivers/media/platform/vsp1/vsp1_entity.c >>>>>>> +++ b/drivers/media/platform/vsp1/vsp1_entity.c >>>>>>> @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, >>>>>>> * Allocate the pad configuration to store formats and selection >>>>>>> * rectangles. >>>>>>> */ >>>>>>> - entity->config = v4l2_alloc_subdev_state(&entity->subdev); >>>>>>> + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); >>>>>>> if (IS_ERR(entity->config)) { >>>>>>> media_entity_cleanup(&entity->subdev.entity); >>>>>>> return PTR_ERR(entity->config); >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> index e1a794f69815..04ad319fb150 100644 >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) >>>>>>> { >>>>>>> struct v4l2_subdev_state *state; >>>>>>> >>>>>>> - state = v4l2_alloc_subdev_state(sd); >>>>>>> + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); >>>>>> >>>>>> At the same time I'm not sure I get the purpose of this. Don't >>>>>> init_cfg() callback implementations deal with try formats themeselves >>>>>> ? I mean, it's not a fixed rule, they can as well initialize their >>>>>> default 'active' formats, but what matters is that they initialize >>>>>> their per-fh try states ? >>>>> >>>>> That is what they do currently. init_cfg() only deals with TRY state, as >>>>> that's the only "state" (i.e. pad_config) there used to be from v4l2 core's >>>>> perspective. >>>>> >>>>>> Shouldn't init_cfg receive the fh's state so that it can initialize >>>>>> it, and just in case they need to, access their subdev's state and >>>>>> initialize them ? I'm missing what the purpose of the flag is tbh. >>>>> >>>>> Now we have (a possibility to have) state for both TRY and ACTIVE on the >>>>> v4l2 core side. The active state has to be initialized also, and a logical >>>>> way to do that is to use the init_cfg(). >>>> >>>> The 'ACTIVE' state is stored in the subdev, to which init_cfg() has >>>> access, and it receives the 'TRY' state as a parameter. >>> >>> No, init_cfg gets either ACTIVE or TRY state, whichever is being allocated. >>> For "old" drivers, ACTIVE state is never allocated so they don't get >>> init_cfg calls for ACTIVE at all. >>> >>> Aaand while writing that, I realized that some drivers manually do allocate >>> ACTIVE state temporarily, which would cause init_cfg with ACTIVE state to be >>> called. I wonder if they explode... Need to check. > > Is this only rcar-vin and tegra-video (vsp1 also allocates an ACTIVE > state, but it's not temporary), or are there other drivers ? It's those three drivers. And the above is not an issue, I didn't think it to the end. Those three drivers used to call v4l2_subdev_alloc_pad_config(), which always calls init_cfg. So the situation is no different with this series. >>>> It is possible to access both states and initialize them properly if >>>> I'm not mistaken. >>>> >>>>> So now, for drivers that support the new active state, init_cfg() can get >>>>> either TRY or ACTIVE state. And if you want to call, say, the driver's >>>>> set_routing() to setup the routing in the state, you have to set the 'which' >>>>> in the routing struct to a value. So somehow init_cfg needs to know if it's >>>>> initializing an ACTIVE or TRY state. >>>> >>>> I'm not sure I got this part. set_routing() as other ops will receive >>>> a state and 'which'. If my proposal above makes sensem where the state >>> >>> Yes, but if it's init_cfg calling set_routing, init_cfg has to figure out >>> the 'which' from somewhere. >>> >>> E.g. init_cfg from ub913 driver: >>> >>> static int ub913_init_cfg(struct v4l2_subdev *sd, >>> struct v4l2_subdev_state *state) >>> { >>> u32 which = state->which; >>> >>> struct v4l2_subdev_route routes[] = { >>> { >>> .sink_pad = 0, >>> .sink_stream = 0, >>> .source_pad = 1, >>> .source_stream = 0, >>> .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, >>> }, >>> }; >>> >>> struct v4l2_subdev_krouting routing = { >>> .which = which, >>> .num_routes = ARRAY_SIZE(routes), >>> .routes = routes, >>> }; >>> >>> return ub913_set_routing(sd, state, &routing); >>> } >>> >>> It uses set_routing to setup a default routing (and set_routing in turn also >>> initializes the formats), but set_routing needs 'which'. > > Could we avoid such patterns, by either initializing the routing > manually here, or splitting the logic of ub913_set_routing() in two > functions, with common code called by ub913_set_routing() and > ub913_init_cfg() ? I think calling set functions from .init_cfg() opens > the door to abuses or just bugs, and I increasingly think it should be > discouraged. I know I've written quite a few drivers that call > .set_fmt() from .init_cfg() though, it does make things easier to some > extent, but I wonder if we could do better. It is possible, at least in theory. The problem is that the 'which' is embedded into many structs used in this context, e.g. struct v4l2_subdev_format and struct v4l2_subdev_krouting. If we don't have 'which', none of the functions using those structs is usable. But what's worse, those structs are used in the subdev state. So even if we write new helper funcs that don't need 'which', we still need it in the end when storing the data to the subdev state. The annoying thing is that I don't think we really need the 'which' in any of those functions or the state. Afaik the only real purpose of 'which' is as a parameter when the userspace calls a subdev ioctl. But getting rid of 'which' is again something I'd rather not start in the context of this series, as I believe it will be a huge task in itself. > This leads to the question of where to initialize the hardware state, > which is the part of the set functions that is only executed in the > ACTIVE case. I don't think this should be done at probe time in general, > but at .s_stream() time. Only drivers that allow changing formats or Yes, I agree. > routing while streaming would need to perform any hardware configutation > in the set functions, and I'm not even sure we have a single driver in > the kernel that allows this (or, to be precise, if we have a single > driver that knowingly allows this, I'm sure we have a bunch of drivers > that don't prevent this situation and will crash or behave in other bad > ways if userspace tried to change the configuration while streaming). > Maybe we should split .set_fmt() and .set_routing() in two for these > very uncommon cases, with the hardware configuration moved to separate > functions ? It has the potential to simplify the set operations and make > them safer. Right, that's a good point: if the driver supports changing fmt while streaming is enabled, set_fmt needs to have the 'which'. At the moment they have it, via the struct v4l2_subdev_format. If we were to remove the 'which' field (which would require separating the uAPI and kAPI facing struct v4l2_subdev_format), we would either need to pass 'which' some other way or have a new op, as you suggest. I would put this to the "later" bucket. It certainly won't be a simple change. Tomi
On 27/09/2021 03:48, Laurent Pinchart wrote: > Hi Tomi, > > On Thu, Sep 16, 2021 at 04:24:19PM +0300, Tomi Valkeinen wrote: >> On 16/09/2021 16:07, Jacopo Mondi wrote: >> >>> Also note that operations like s_stream do not take a state as >>> parameter. The driver has to fetch it from the subdev anyway >>> (this in reply to the idea of having the active state as parameter vs >>> retrieving it from the subdev if ACTIVE) >>> >>> While porting the R-Car drivers on top of this series I found myself >>> in the need to (in the s_stream call chain) >>> >>> static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>> { >>> const struct v4l2_subdev_state *state = priv->subdev.state; >>> const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; >>> >>> ... >>> >>> /* >>> * Configure field handling inspecting the formats of the >>> * single sink pad streams. >>> */ >>> for (i = 0; i < configs->num_configs; ++i) { >>> const struct v4l2_subdev_stream_config *config = configs->configs; >>> if (config->pad != RCAR_CSI2_SINK) >>> continue; >>> >>> if (config->fmt.field != V4L2_FIELD_ALTERNATE) >>> continue; >>> >>> fld |= FLD_DET_SEL(1); >>> fld |= FLD_FLD_EN(config->stream); >>> >>> /* PAL vs NTSC. */ >>> if (config->fmt.height == 240) >>> fld |= FLD_FLD_NUM(0); >>> else >>> fld |= FLD_FLD_NUM(1); >>> } >>> >>> ... >>> >>> } >>> >>> Am I doing it wrong, or is this a case for the subdev to have to >>> directly access sd->state ? >> >> In s_stream path you should: >> >> state = v4l2_subdev_lock_active_state(sd); >> >> <do the work with the state> >> >> v4l2_subdev_unlock_state(state); >> >> If you already have the state, e.g. in set_fmt: >> >> state = v4l2_subdev_validate_and_lock_state(sd, state); >> >> <do the work with the state> >> >> v4l2_subdev_unlock_state(state); >> >> Accessing the stream_configs directly is fine but not that nice. I did >> think about some helpers, perhaps for_each_stream_config(), but I didn't >> add that as I didn't have the need. >> >> There's v4l2_state_get_stream_format() which can be used in many cases, >> but we probably need something else if you need to iterate over all the >> configs. > > I really like forcing drivers to call functions that will lock the > state, at least until we can move the locks to the core (if ever). We > should move the fields of v4l2_subdev that drivers are not supposed to > access directly under a big PRIVATE comment. Can you clarify what you mean here? Did you mean you like functions that will _check_ the lock? Or did you just mean that you like v4l2_subdev_lock_state() better than mutex_lock(state->lock)? Well, in any case, I think my series does both =). I can add the private comment to subdev. In fact, at one time I did not have sd->state, but sd->_state, just to make sure no one accesses it. Tomi
Hi Tomi, On Mon, Sep 27, 2021 at 11:35:53AM +0300, Tomi Valkeinen wrote: > On 27/09/2021 03:46, Laurent Pinchart wrote: > > On Thu, Sep 16, 2021 at 03:07:52PM +0200, Jacopo Mondi wrote: > >> On Mon, Sep 13, 2021 at 05:26:45PM +0300, Tomi Valkeinen wrote: > >>> On 13/09/2021 16:38, Jacopo Mondi wrote: > >>>> On Mon, Sep 13, 2021 at 03:17:01PM +0300, Tomi Valkeinen wrote: > >>>>> On 13/09/2021 14:41, Jacopo Mondi wrote: > >>>>>> On Mon, Aug 30, 2021 at 02:00:43PM +0300, Tomi Valkeinen wrote: > >>>>>>> The subdev state is passed to functions in the media drivers, and > >>>>>>> usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is > >>>>>>> also given to the function in one way or another. > >>>>>>> > >>>>>>> One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One > >>>>>>> could argue that the initialization of the state should be the same for > >>>>>>> both ACTIVE and TRY cases, but unfortunately that is not the case: > >>>>>>> > >>>>>>> - Some drivers do also other things than just touch the state when > >>>>>>> dealing with ACTIVE, e.g. if there is extra state outside the standard > >>>>>>> subdev state. > >>>>>>> - Some drivers might need to create, say, struct v4l2_subdev_format > >>>>>>> which has 'which' field, and that needs to be filled with either ACTIVE > >>>>>>> or TRY. > >>>>>>> > >>>>>>> Currently init_cfg is only called for TRY case from the v4l2 framework, > >>>>>>> passing the TRY state. Some drivers call their own init_cfg, passing > >>>>>>> NULL as the state, which is used to indicate ACTIVE case. > >>>>>>> > >>>>>>> In the future we want to pass subdev's active state from the v4l2 > >>>>>>> framework side, so we need a solution to this. > >>>>>>> > >>>>>>> We could change the init_cfg() to include the TRY/ACTIVE value, which > >>>>>>> would require changing more or less all the drivers. Instead, I have > >>>>>>> added 'which' field to the subdev state itself, filled at state > >>>>>>> allocation time, which only requires changes to the drivers that > >>>>>>> allocate a state themselves. > >>>>>>> > >>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>>>>> --- > >>>>>>> drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- > >>>>>>> drivers/media/platform/vsp1/vsp1_entity.c | 2 +- > >>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- > >>>>>>> drivers/staging/media/tegra-video/vi.c | 2 +- > >>>>>>> include/media/v4l2-subdev.h | 7 ++++++- > >>>>>>> 5 files changed, 16 insertions(+), 7 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > >>>>>>> index 5f4fa8c48f68..1de30d5b437f 100644 > >>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > >>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > >>>>>>> @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, > >>>>>>> u32 width, height; > >>>>>>> int ret; > >>>>>>> > >>>>>>> - sd_state = v4l2_alloc_subdev_state(sd); > >>>>>>> + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); > >>>>>> > >>>>>> Shouldn't the 'which' parameters be used to decide if either ACTIVE or > >>>>>> TRY have to be used ? this function is also used to set TRY formats, > >>>>>> in example... > >>>>>> > >>>>>> Oh, maybe I got how it works, the state's which is not > >>>>>> relevant but the v4l2_subdev_format's which is, as it will be used in > >>>>>> the next patch to decide if the subdev's state of the file-handle's > >>>>>> state should be passed to the ioctl. > >>>>> > >>>>> Yes. It's messy, but it's how it worked before also. > >>>>> > >>>>> The drivers can't really allocate TRY state as it must come from the core, > >>>>> based on the filehandle. Now as I say that, makes me wonder why even expose > >>>>> the option to drivers. Maybe v4l2_alloc_subdev_state() should take just the > >>>>> sd parameter, and always allocate ACTIVE state, and the v4l2 core can use > >>>>> another way to create the TRY state. > > > > Drivers should not allocate state manually, it should always be done by > > the core (when opening a file handle for the TRY states, and when > > initializing the subdev in the probe function for the ACTIVE state). > > > > Looking at the three drivers that call v4l2_alloc_subdev_state(), we > > have the vsp1 driver that uses it to allocate its own ACTIVE state at > > init time. This one is easy, it should be replaced with > > v4l2_subdev_alloc_state() (or whatever that function will end up being > > named, see comments to 02/36). > > I don't follow here. First you say that drivers should not allocate > state. But then, I think, you say that vsp1 allocating state is fine > after some change? I mean that the vsp1 driver uses the function for its intended purpose, but this should be moved to core-allocated state (with v4l2_subdev_init_done() or however we decide to call it). The purpose matches, so it should be easy. The other two drivers allocate ACTIVE states for different purposes, which I don't think is right. > My understanding is that vsp1 is fine. Of course, if the function it > calls gets renamed we need to rename it in vsp1 too. > > > The rcar-vin and tegra-video drivers are doing it wrong. rcar-vin has > > legacy code to support unfortunate design decisions (everything in the > > "V4L2" section in rcar-v4l2.c), and newer code that gets it right (the > > "V4L2 Media Controller" section in the same file). The legacy code is > > used on older platforms, and the newer code on newer platforms. I know > > that Niklas would love to get rid of the legacy code, and I'd also be > > happy to see it go. If that's not possible, we'll have to keep exposing > > v4l2_alloc_subdev_state() for this driver. Niklas, what do you think, > > could we drop the legacy code after all those years ? > > > > The situation in tegra-video is similar. They got it wrong. The driver > > is in staging though, so that's fixable. > > > > I'd propose renaming v4l2_subdev_alloc_state() to > > __v4l2_subdev_alloc_state() (or something that shows it's internal), > > documenting that it must not be used by new drivers, and adding an entry > > in the TODO file of the tegra-video driver to fix this. > > Yes, that sounds fine. Subdev drivers allocating the state is not > something that should be done in any new drivers. > > >>>> init_cfg() as well as other operations used to received an > >>>> array of per fh's pad_configs, and the sd pointer itself. The fh's pad > >>>> configs were allocated by the core, as well as the core now allocates > >>>> the per-fh's state. > >>>> > >>>> Before the introduction of 'state', if the 'which' flags was set to > >>>> TRY then information were stored/retrieved/initialized in the per-fh > >>>> pad_config, otherwise the active configuration (usually stored in the > >>>> driver main structure) was used. > > > > Correct, and the active configuration is in that case stored in a > > driver-specific way (except in the vsp1 driver that uses the pad config > > structure to store it), with ad-hoc accessors and lots of manual checks > > in the code paths. > > > >>>> So we had a clear separation of per-fh information and the active > >>>> state. The core didn't care afaict, and passed in both, then driver had > >>>> to deal with them doing the right thing by inspecting the 'which' flag. > >>>> > >>>> The typical pattern was: > >>>> > >>>> static int subdev_ops(sd, pad_cfg, which) > >>>> { > >>>> if (which == TRY) > >>>> /* Operate on config stored in pad_cfg */ > >>>> > >>>> else > >>>> /* > >>>> * Operate on config stored in subdev (and > >>>> * applied to HW) > >>>> */ > >>>> } > >>>> > >>>> Or am I overlooking some cases or do you agree with my understanding > >>>> so far ? > >>> > >>> More or less, yes. I think there are (used to be) three kinds of ops: > >>> > >>> - Ops that get pad_cfg and 'which' in an op specific struct. E.g. set_fmt. > >>> The pad_cfg is TRY pad_config, even if 'which' == ACTIVE. > > > > And pad_cfg is ignored in those drivers when which == ACTIVE (or it > > should be at least, if it's not, it's a driver bug). > > > >>> - Ops that don't get pad_cfg, like s_stream. 'which' is implicitly ACTIVE. > > > > .s_stream() on a TRY configuration would be an interesting concept :-) > > I think it would be a good one, though =). It could be used to validate > the whole pipeline. But it would probably be rather difficult to > implement with the current v4l2 framework. > > >> Also note that operations like s_stream do not take a state as > >> parameter. The driver has to fetch it from the subdev anyway > >> (this in reply to the idea of having the active state as parameter vs > >> retrieving it from the subdev if ACTIVE) > > > > We could pass the state as a parameter, but given that the operation > > always operates on the ACTIVE state by definition, I think this is > > redundant. > > > >> While porting the R-Car drivers on top of this series I found myself > >> in the need to (in the s_stream call chain) > >> > >> static int rcsi2_start_receiver(struct rcar_csi2 *priv) > >> { > >> const struct v4l2_subdev_state *state = priv->subdev.state; > >> const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; > >> > >> ... > >> > >> /* > >> * Configure field handling inspecting the formats of the > >> * single sink pad streams. > >> */ > >> for (i = 0; i < configs->num_configs; ++i) { > >> const struct v4l2_subdev_stream_config *config = configs->configs; > >> if (config->pad != RCAR_CSI2_SINK) > >> continue; > >> > >> if (config->fmt.field != V4L2_FIELD_ALTERNATE) > >> continue; > >> > >> fld |= FLD_DET_SEL(1); > >> fld |= FLD_FLD_EN(config->stream); > >> > >> /* PAL vs NTSC. */ > >> if (config->fmt.height == 240) > >> fld |= FLD_FLD_NUM(0); > >> else > >> fld |= FLD_FLD_NUM(1); > >> } > >> > >> ... > >> > >> } > >> > >> Am I doing it wrong, or is this a case for the subdev to have to > >> directly access sd->state ? > > > > (Will reply to Tomi's reply on this) > > > >>> - init_cfg which gets pad_cfg, but no which (as 'which' is always implicitly > >>> TRY) > > > > As proposed in 02/36, I think .init_cfg() should operate the same way > > regardless of whether the state is TRY or ACTIVE, so there's no need for > > a 'which' parameter (it would only open the door to abuses). > > > >>> So pad_cfg was TRY state. Drivers could use pad_configs internally to track > >>> ACTIVE state, but the core had no knowledge about this. > > > > Correct. > > > >>>> Now we have a 'state' that holds the array of pad_configs and along > >>>> the continuation of the series will end up holding per-pad > >>>> configurations. > >>>> > >>>> We now also have one 'state' per file-handle, and one > >>>> per-subdev. As I see this, it would be natual for drivers to receive > >>>> one state without knowing where it comes from. In the next patch you > >>> > >>> Note that only subdev's that explicitly support the new state code, and > >>> allocate the state, have the subdev active state. Which means only the > >>> drivers in my work branch. > >>> > >>> The "old" drivers work like they used to: they get the state (essentially > >>> repackaged pad_cfg) for TRY cases, NULL otherwise. > >>> > >>> And yes, it would be natural to just get a state, but the subdev drivers > >>> need to know if the context is TRY/ACTIVE. As you can see from the bullet > >>> list above, the driver knows this in all the other places except init_cfg. > > > > I agree that porting all the drivers as part of this series isn't > > feasible, so passing NULL for the state in case the driver hasn't > > explicitly opted-in seems fine to me. > > > >>>> instrument the core to do exactly this: inspect the which flag and > >>>> pass in the 'right' state. Ofc drivers need to have access to 'which' > >>>> to know if they have to apply settings to the HW or not. > >>>> > >>>> Looking ahead in your series I see these structures: > >>>> > >>>> struct v4l2_subdev_pad_config { > >>>> struct v4l2_mbus_framefmt try_fmt; > >>>> struct v4l2_rect try_crop; > >>>> struct v4l2_rect try_compose; > >>>> }; > >>>> > >>>> struct v4l2_subdev_stream_config { > >>>> u32 pad; > >>>> u32 stream; > >>>> > >>>> struct v4l2_mbus_framefmt fmt; > >>>> struct v4l2_rect crop; > >>>> struct v4l2_rect compose; > >>>> }; > >>>> > >>>> struct v4l2_subdev_stream_configs { > >>>> u32 num_configs; > >>>> struct v4l2_subdev_stream_config *configs; > >>>> }; > >>>> > >>>> All of them part of state: > >>>> > >>>> struct v4l2_subdev_state { > >>>> struct mutex lock; > >>>> u32 which; > >>>> struct v4l2_subdev_pad_config *pads; > >>>> struct v4l2_subdev_krouting routing; > >>>> struct v4l2_subdev_stream_configs stream_configs; > >>>> }; > >>>> > >>>> So 'state' will hold 'TRY' information (only used for 'state' > >>>> instances allocated in the fh) and 'ACTIVE' ones (used for states > >>>> allocated in the sd). > >>> > >>> Right. > >>> > >>>> Looking at 'v4l2_subdev_pad_config' and 'v4l2_subdev_stream_config' they > >>>> seem to describe more or less the same things: fmt, crop and compose > >>>> (per pad-stream in case of stream_config). I wonder if those shouldn't > >>>> be unified so that: > >>>> > >>>> 1) Drivers receive one state: the core passes in the 'correct' one > >>>> (per-fh or per-sd) as you do in next patch > >>> > >>> Yes. But note that "old" drivers don't have active state. > >>> > >>>> 2) The 'which' information is not stored in the state but it's only > >>>> 'contextual' (as in a parameter to the subdev operation) so that > >>>> drivers inspect it to know if they have to apply settings to hw or not > >>> > >>> Yes, except we have init_cfg... > > > > This sounds like .init_cfg() is the only blocker, which I think is > > promissing :-) > > > >>>> 3) v4l2_subdev_pad_config can be re-used and expanded, to maintain per-pad > >>>> configurations regardless if they're ACTIVE or TRY, as this only depends > >>>> on where the state is stored. > >>> > >>> pad_config is a static array of per-pad configs. stream_configs is a dynamic > >>> per-stream config. > > > > Do I understand correctly that it's both per-stream and per-pad ? I > > wonder if it could make sense to store it that way, with an array of > > per-pad configurations (v4l2_subdev_pad_config), which will in turn > > contain per-stream configuration. I suppose it depends on the usage > > patterns, which I'll understand better when reading the rest of the > > series. > > I'm not sure what you mean with "both per-stream and per-pad". It's per > (stream, pad) tuple. That's what I meant, not just a configuration for each stream that traverses the subdev, but one for each pad that carries the stream, for each stream. > At the moment the internal storage is essentially just an array of > { pad, stream, config } items. It could also be changed as you suggest, > but afaics that would require more dynamic allocations. The APIs should > be designed so that the drivers don't need to care about the internal > storage. Agreed, the less drivers have to care about the storage, the better. > >>> stream_configs is a super-set of pad-configs, so we could > >>> drop pad_configs, but it would require changing all the drivers in > >>> non-trivial ways. > > > > I like Jacopo's proposal to unify the two, but I do agree that it's not > > trivial. Here again I think it falls in the category of setting a long > > term goal and trying to go in that direction, without necessarily > > reaching it just yet. I wonder if we could have helper functions (maybe > > they're already included later in the series ?) to abstract the > > pad_config/stream_configs difference for drivers. This would also make > > further reworks easier (such as storing the per-stream configuration in > > v4l2_subdev_pad_config as proposed above) if we decide to address some > > of the changes later. > > If you mean do we have functions to hide the stream (i.e. stream = 0), > no, I haven't added such. I have not worked towards making > non-multiplexed-streams subdev drivers use the new APIs. I meant helpers to access the configuration without having to care in subdev drivers about how it's actually stored. > That said, I have tried to design this series so that it is possible. > And "new" and "old" drivers should be compatible with each other. > > >>> v4l2_subdev_pad_config is not used or even allocated by the "new" drivers. > >>> And routing & stream_configs are not used by the "old" drivers. > >>> > >>>> As I immagine it a subdev pad operation could look like: > >>>> > >>>> static int subdev_op(sd, pad, state, which, ...) > >>>> { > >>>> /* Doesn't matter if state is per-fh or the sd one. */ > >>>> state->pads[pad].fmt = ....; > >>>> > >>>> if (which == TRY) > >>>> return; > >>>> > >>>> /* Apply to the HW. */ > >>>> } > >>>> > >>>> Does it make any sense to you ? I might have missed some reason why > >>>> this is not possible. > >>> > >>> It makes sense, but there are the buts =). I've tried to explain these in > >>> the commit messages, but it's kind of confusing. > >>> > >>> One but I haven't mentioned in the emails is that when subdev drivers call > >>> ops in other subdev drivers they pass NULL in the state. > > > > We should really really try to avoid that. s_stream() is the obvious > > main exception. Other subdev operations, such as .get_fmt() or > > .set_fmt(), shouldn't be called cross-subdevs. A subdev should only care > > about its own configuration, not about its neighbours. Are there > > blockers here ? > > There are a lot of subdevs using v4l2_subdev_call(), including uses of > set_fmt. It doesn't mean it's right though ;-) It's a bit different for video node drivers, they have legitimate needs to call .get_fmt() to validate the pipeline. They can also call .set_fmt() when they're not MC-based, in which case those drivers effectively fulfil the role of the V4L2 core. > >>> This is fine for > >>> the "old" drivers, as they expect a state only for TRY case. However, the > >>> "new" drivers unfortunately expect to get a state on both TRY and ACTIVE > >>> cases, and the only sensible way I figured out to handle this was the > >>> v4l2_subdev_validate_state() function (patch 6). > >>> > >>> So, all this could be much neater, but would require modifying all subdev > >>> drivers in non-trivial ways. I think this is something that can be done > >>> slowly in the future. > >>> > >>>>>>> if (IS_ERR(sd_state)) > >>>>>>> return PTR_ERR(sd_state); > >>>>>>> > >>>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > >>>>>>> index e40bca254b8b..63ea5e472c33 100644 > >>>>>>> --- a/drivers/media/platform/vsp1/vsp1_entity.c > >>>>>>> +++ b/drivers/media/platform/vsp1/vsp1_entity.c > >>>>>>> @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > >>>>>>> * Allocate the pad configuration to store formats and selection > >>>>>>> * rectangles. > >>>>>>> */ > >>>>>>> - entity->config = v4l2_alloc_subdev_state(&entity->subdev); > >>>>>>> + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); > >>>>>>> if (IS_ERR(entity->config)) { > >>>>>>> media_entity_cleanup(&entity->subdev.entity); > >>>>>>> return PTR_ERR(entity->config); > >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> index e1a794f69815..04ad319fb150 100644 > >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > >>>>>>> { > >>>>>>> struct v4l2_subdev_state *state; > >>>>>>> > >>>>>>> - state = v4l2_alloc_subdev_state(sd); > >>>>>>> + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); > >>>>>> > >>>>>> At the same time I'm not sure I get the purpose of this. Don't > >>>>>> init_cfg() callback implementations deal with try formats themeselves > >>>>>> ? I mean, it's not a fixed rule, they can as well initialize their > >>>>>> default 'active' formats, but what matters is that they initialize > >>>>>> their per-fh try states ? > >>>>> > >>>>> That is what they do currently. init_cfg() only deals with TRY state, as > >>>>> that's the only "state" (i.e. pad_config) there used to be from v4l2 core's > >>>>> perspective. > >>>>> > >>>>>> Shouldn't init_cfg receive the fh's state so that it can initialize > >>>>>> it, and just in case they need to, access their subdev's state and > >>>>>> initialize them ? I'm missing what the purpose of the flag is tbh. > >>>>> > >>>>> Now we have (a possibility to have) state for both TRY and ACTIVE on the > >>>>> v4l2 core side. The active state has to be initialized also, and a logical > >>>>> way to do that is to use the init_cfg(). > >>>> > >>>> The 'ACTIVE' state is stored in the subdev, to which init_cfg() has > >>>> access, and it receives the 'TRY' state as a parameter. > >>> > >>> No, init_cfg gets either ACTIVE or TRY state, whichever is being allocated. > >>> For "old" drivers, ACTIVE state is never allocated so they don't get > >>> init_cfg calls for ACTIVE at all. > >>> > >>> Aaand while writing that, I realized that some drivers manually do allocate > >>> ACTIVE state temporarily, which would cause init_cfg with ACTIVE state to be > >>> called. I wonder if they explode... Need to check. > > > > Is this only rcar-vin and tegra-video (vsp1 also allocates an ACTIVE > > state, but it's not temporary), or are there other drivers ? > > It's those three drivers. And the above is not an issue, I didn't think > it to the end. > > Those three drivers used to call v4l2_subdev_alloc_pad_config(), which > always calls init_cfg. So the situation is no different with this series. > > >>>> It is possible to access both states and initialize them properly if > >>>> I'm not mistaken. > >>>> > >>>>> So now, for drivers that support the new active state, init_cfg() can get > >>>>> either TRY or ACTIVE state. And if you want to call, say, the driver's > >>>>> set_routing() to setup the routing in the state, you have to set the 'which' > >>>>> in the routing struct to a value. So somehow init_cfg needs to know if it's > >>>>> initializing an ACTIVE or TRY state. > >>>> > >>>> I'm not sure I got this part. set_routing() as other ops will receive > >>>> a state and 'which'. If my proposal above makes sensem where the state > >>> > >>> Yes, but if it's init_cfg calling set_routing, init_cfg has to figure out > >>> the 'which' from somewhere. > >>> > >>> E.g. init_cfg from ub913 driver: > >>> > >>> static int ub913_init_cfg(struct v4l2_subdev *sd, > >>> struct v4l2_subdev_state *state) > >>> { > >>> u32 which = state->which; > >>> > >>> struct v4l2_subdev_route routes[] = { > >>> { > >>> .sink_pad = 0, > >>> .sink_stream = 0, > >>> .source_pad = 1, > >>> .source_stream = 0, > >>> .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > >>> }, > >>> }; > >>> > >>> struct v4l2_subdev_krouting routing = { > >>> .which = which, > >>> .num_routes = ARRAY_SIZE(routes), > >>> .routes = routes, > >>> }; > >>> > >>> return ub913_set_routing(sd, state, &routing); > >>> } > >>> > >>> It uses set_routing to setup a default routing (and set_routing in turn also > >>> initializes the formats), but set_routing needs 'which'. > > > > Could we avoid such patterns, by either initializing the routing > > manually here, or splitting the logic of ub913_set_routing() in two > > functions, with common code called by ub913_set_routing() and > > ub913_init_cfg() ? I think calling set functions from .init_cfg() opens > > the door to abuses or just bugs, and I increasingly think it should be > > discouraged. I know I've written quite a few drivers that call > > .set_fmt() from .init_cfg() though, it does make things easier to some > > extent, but I wonder if we could do better. > > It is possible, at least in theory. > > The problem is that the 'which' is embedded into many structs used in > this context, e.g. struct v4l2_subdev_format and struct > v4l2_subdev_krouting. If we don't have 'which', none of the functions > using those structs is usable. But what's worse, those structs are used > in the subdev state. So even if we write new helper funcs that don't > need 'which', we still need it in the end when storing the data to the > subdev state. > > The annoying thing is that I don't think we really need the 'which' in > any of those functions or the state. Afaik the only real purpose of > 'which' is as a parameter when the userspace calls a subdev ioctl. > > But getting rid of 'which' is again something I'd rather not start in > the context of this series, as I believe it will be a huge task in itself. It's only v4l2_subdev_krouting that has a which field as far as I can tell, the rest of the state uses v4l2_mbus_framefmt to store the format, not v4l2_subdev_format. Given that v4l2_subdev_krouting is added by this series, I'd like to try and address it. > > This leads to the question of where to initialize the hardware state, > > which is the part of the set functions that is only executed in the > > ACTIVE case. I don't think this should be done at probe time in general, > > but at .s_stream() time. Only drivers that allow changing formats or > > Yes, I agree. > > > routing while streaming would need to perform any hardware configutation > > in the set functions, and I'm not even sure we have a single driver in > > the kernel that allows this (or, to be precise, if we have a single > > driver that knowingly allows this, I'm sure we have a bunch of drivers > > that don't prevent this situation and will crash or behave in other bad > > ways if userspace tried to change the configuration while streaming). > > Maybe we should split .set_fmt() and .set_routing() in two for these > > very uncommon cases, with the hardware configuration moved to separate > > functions ? It has the potential to simplify the set operations and make > > them safer. > > Right, that's a good point: if the driver supports changing fmt while > streaming is enabled, set_fmt needs to have the 'which'. At the moment > they have it, via the struct v4l2_subdev_format. If we were to remove > the 'which' field (which would require separating the uAPI and kAPI > facing struct v4l2_subdev_format), we would either need to pass 'which' > some other way or have a new op, as you suggest. .set_fmt() + .apply_fmt() is tempting. Or .atomic_check() and .atomic_commit() ;-) > I would put this to the "later" bucket. It certainly won't be a simple > change. Agreed. -- Regards, Laurent Pinchart
Hi Tomi, On Mon, Sep 27, 2021 at 11:55:38AM +0300, Tomi Valkeinen wrote: > On 27/09/2021 03:48, Laurent Pinchart wrote: > > On Thu, Sep 16, 2021 at 04:24:19PM +0300, Tomi Valkeinen wrote: > >> On 16/09/2021 16:07, Jacopo Mondi wrote: > >> > >>> Also note that operations like s_stream do not take a state as > >>> parameter. The driver has to fetch it from the subdev anyway > >>> (this in reply to the idea of having the active state as parameter vs > >>> retrieving it from the subdev if ACTIVE) > >>> > >>> While porting the R-Car drivers on top of this series I found myself > >>> in the need to (in the s_stream call chain) > >>> > >>> static int rcsi2_start_receiver(struct rcar_csi2 *priv) > >>> { > >>> const struct v4l2_subdev_state *state = priv->subdev.state; > >>> const struct v4l2_subdev_stream_configs *configs = &state->stream_configs; > >>> > >>> ... > >>> > >>> /* > >>> * Configure field handling inspecting the formats of the > >>> * single sink pad streams. > >>> */ > >>> for (i = 0; i < configs->num_configs; ++i) { > >>> const struct v4l2_subdev_stream_config *config = configs->configs; > >>> if (config->pad != RCAR_CSI2_SINK) > >>> continue; > >>> > >>> if (config->fmt.field != V4L2_FIELD_ALTERNATE) > >>> continue; > >>> > >>> fld |= FLD_DET_SEL(1); > >>> fld |= FLD_FLD_EN(config->stream); > >>> > >>> /* PAL vs NTSC. */ > >>> if (config->fmt.height == 240) > >>> fld |= FLD_FLD_NUM(0); > >>> else > >>> fld |= FLD_FLD_NUM(1); > >>> } > >>> > >>> ... > >>> > >>> } > >>> > >>> Am I doing it wrong, or is this a case for the subdev to have to > >>> directly access sd->state ? > >> > >> In s_stream path you should: > >> > >> state = v4l2_subdev_lock_active_state(sd); > >> > >> <do the work with the state> > >> > >> v4l2_subdev_unlock_state(state); > >> > >> If you already have the state, e.g. in set_fmt: > >> > >> state = v4l2_subdev_validate_and_lock_state(sd, state); > >> > >> <do the work with the state> > >> > >> v4l2_subdev_unlock_state(state); > >> > >> Accessing the stream_configs directly is fine but not that nice. I did > >> think about some helpers, perhaps for_each_stream_config(), but I didn't > >> add that as I didn't have the need. > >> > >> There's v4l2_state_get_stream_format() which can be used in many cases, > >> but we probably need something else if you need to iterate over all the > >> configs. > > > > I really like forcing drivers to call functions that will lock the > > state, at least until we can move the locks to the core (if ever). We > > should move the fields of v4l2_subdev that drivers are not supposed to > > access directly under a big PRIVATE comment. > > Can you clarify what you mean here? Did you mean you like functions that > will _check_ the lock? Or did you just mean that you like > v4l2_subdev_lock_state() better than mutex_lock(state->lock)? I like v4l2_subdev_lock_state(). > Well, in any case, I think my series does both =). I can add the private > comment to subdev. In fact, at one time I did not have sd->state, but > sd->_state, just to make sure no one accesses it. I half recall there was a gcc attribute to mark private fields in a structure, which then generates a warning if those fields are accessed by a function that doesn't have another special attribute. Or maybe I was dreaming :-) -- Regards, Laurent Pinchart
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 5f4fa8c48f68..1de30d5b437f 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -252,7 +252,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, u32 width, height; int ret; - sd_state = v4l2_alloc_subdev_state(sd); + sd_state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); if (IS_ERR(sd_state)) return PTR_ERR(sd_state); diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index e40bca254b8b..63ea5e472c33 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -675,7 +675,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, * Allocate the pad configuration to store formats and selection * rectangles. */ - entity->config = v4l2_alloc_subdev_state(&entity->subdev); + entity->config = v4l2_alloc_subdev_state(&entity->subdev, V4L2_SUBDEV_FORMAT_ACTIVE); if (IS_ERR(entity->config)) { media_entity_cleanup(&entity->subdev.entity); return PTR_ERR(entity->config); diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index e1a794f69815..04ad319fb150 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) { struct v4l2_subdev_state *state; - state = v4l2_alloc_subdev_state(sd); + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_TRY); if (IS_ERR(state)) return PTR_ERR(state); @@ -870,7 +870,9 @@ int v4l2_subdev_link_validate(struct media_link *link) } EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); -struct v4l2_subdev_state *v4l2_alloc_subdev_state(struct v4l2_subdev *sd) +struct v4l2_subdev_state * +v4l2_alloc_subdev_state(struct v4l2_subdev *sd, + enum v4l2_subdev_format_whence which) { struct v4l2_subdev_state *state; int ret; @@ -879,6 +881,8 @@ struct v4l2_subdev_state *v4l2_alloc_subdev_state(struct v4l2_subdev *sd) if (!state) return ERR_PTR(-ENOMEM); + state->which = which; + if (sd->entity.num_pads) { state->pads = kvmalloc_array(sd->entity.num_pads, sizeof(*state->pads), @@ -948,7 +952,7 @@ int v4l2_subdev_alloc_state(struct v4l2_subdev *sd) { struct v4l2_subdev_state *state; - state = v4l2_alloc_subdev_state(sd); + state = v4l2_alloc_subdev_state(sd, V4L2_SUBDEV_FORMAT_ACTIVE); if (IS_ERR(state)) return PTR_ERR(state); diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c index a94d19e2a67c..691f5e04b0a1 100644 --- a/drivers/staging/media/tegra-video/vi.c +++ b/drivers/staging/media/tegra-video/vi.c @@ -507,7 +507,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan, if (!subdev) return -ENODEV; - sd_state = v4l2_alloc_subdev_state(subdev); + sd_state = v4l2_alloc_subdev_state(subdev, V4L2_SUBDEV_FORMAT_ACTIVE); if (IS_ERR(sd_state)) return PTR_ERR(sd_state); /* diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index ecaf040ead57..5ec78ffda4f5 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -655,6 +655,7 @@ struct v4l2_subdev_pad_config { /** * struct v4l2_subdev_state - Used for storing subdev state information. * + * @which: state type (from enum v4l2_subdev_format_whence) * @pads: &struct v4l2_subdev_pad_config array * * This structure only needs to be passed to the pad op if the 'which' field @@ -662,6 +663,7 @@ struct v4l2_subdev_pad_config { * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL. */ struct v4l2_subdev_state { + u32 which; struct v4l2_subdev_pad_config *pads; }; @@ -1141,10 +1143,13 @@ int v4l2_subdev_link_validate(struct media_link *link); * v4l2_alloc_subdev_state - allocate v4l2_subdev_state * * @sd: pointer to &struct v4l2_subdev for which the state is being allocated. + * @which: configuration type for the state (from enum v4l2_subdev_format_whence) * * Must call v4l2_free_subdev_state() when state is no longer needed. */ -struct v4l2_subdev_state *v4l2_alloc_subdev_state(struct v4l2_subdev *sd); +struct v4l2_subdev_state * +v4l2_alloc_subdev_state(struct v4l2_subdev *sd, + enum v4l2_subdev_format_whence which); /** * v4l2_free_subdev_state - free a v4l2_subdev_state
The subdev state is passed to functions in the media drivers, and usually either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY is also given to the function in one way or another. One op where this is not the case is v4l2_subdev_pad_ops.init_cfg. One could argue that the initialization of the state should be the same for both ACTIVE and TRY cases, but unfortunately that is not the case: - Some drivers do also other things than just touch the state when dealing with ACTIVE, e.g. if there is extra state outside the standard subdev state. - Some drivers might need to create, say, struct v4l2_subdev_format which has 'which' field, and that needs to be filled with either ACTIVE or TRY. Currently init_cfg is only called for TRY case from the v4l2 framework, passing the TRY state. Some drivers call their own init_cfg, passing NULL as the state, which is used to indicate ACTIVE case. In the future we want to pass subdev's active state from the v4l2 framework side, so we need a solution to this. We could change the init_cfg() to include the TRY/ACTIVE value, which would require changing more or less all the drivers. Instead, I have added 'which' field to the subdev state itself, filled at state allocation time, which only requires changes to the drivers that allocate a state themselves. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/rcar-vin/rcar-v4l2.c | 2 +- drivers/media/platform/vsp1/vsp1_entity.c | 2 +- drivers/media/v4l2-core/v4l2-subdev.c | 10 +++++++--- drivers/staging/media/tegra-video/vi.c | 2 +- include/media/v4l2-subdev.h | 7 ++++++- 5 files changed, 16 insertions(+), 7 deletions(-)