| Message ID | 20220618222442.478285-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc() | expand |
On Mon, Jun 20, 2022 at 12:44:02PM +0300, Tomi Valkeinen wrote: > On 19/06/2022 02:16, Laurent Pinchart wrote: > > Hi Marek, > > > > CC'ing Tomi to get his opinion. > > > > On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote: > >> Any local subdev state should be allocated and free'd using > >> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which > >> takes care of calling .init_cfg() subdev op. Without this, > >> subdev internal state might be uninitialized by the time > >> any other subdev op is called. > > Does this fix a bug you have? Wasn't this broken even before the active > state, as init_cfg was not called? > > In any case, I think we have to do something like this, as the source > subdev might depend on a valid subdev state. > > It's not very nice to have the drivers using __v4l2_subdev_state_alloc, > though. But if non-MC drivers are not going away, You know my opinion on this :-) We shouldn't have any new user of __v4l2_subdev_state_alloc() in new drivers, but fixing issues in existing drivers is a valid use case. I'd like if the dcmi driver could be converted to be MC-centric, but that will likely not happen. > and if they are going > to be calling ops in other subdevs with V4L2_SUBDEV_FORMAT_TRY, they > need to pass a valid subdev state... > > I don't see a better way right away, so I think this is fine. > > Do we need a v4l2_subdev_call_state_try(), similar to > v4l2_subdev_call_state_active()? It would handle allocating the state, > calling the op and freeing the state. I could imagine the user trying multiple operations on the same state, but maybe a single call is a common enough use case to implement a dedicated helper ?
On 6/20/22 11:44, Tomi Valkeinen wrote: > Hi, Hello all, >> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote: >>> Any local subdev state should be allocated and free'd using >>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which >>> takes care of calling .init_cfg() subdev op. Without this, >>> subdev internal state might be uninitialized by the time >>> any other subdev op is called. > > Does this fix a bug you have? Yes > Wasn't this broken even before the active > state, as init_cfg was not called? Yes, this was always broken. I suspect nobody tested this mode of operation before. In my case, I have this DCMI driver connected directly to MT9P006 sensor. > In any case, I think we have to do something like this, as the source > subdev might depend on a valid subdev state. Right. [...]
Hi Marek, On 6/20/22 16:06, Marek Vasut wrote: > On 6/20/22 11:44, Tomi Valkeinen wrote: >> Hi, > > Hello all, > >>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote: >>>> Any local subdev state should be allocated and free'd using >>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which >>>> takes care of calling .init_cfg() subdev op. Without this, >>>> subdev internal state might be uninitialized by the time >>>> any other subdev op is called. >> >> Does this fix a bug you have? > > Yes Which bug did you encounter exactly ? This is strange that we have not yet encounter any problems around that through our tests campaigns... or we have to enforce with a new test, so better to know what your problem was exactly. > >> Wasn't this broken even before the active state, as init_cfg was not >> called? > > Yes, this was always broken. I suspect nobody tested this mode of > operation before. In my case, I have this DCMI driver connected directly > to MT9P006 sensor. As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor. I don't see the difference with our OV5640 used in parallel mode which is a mainline config on our side, so one more time I wonder what the problem was. > >> In any case, I think we have to do something like this, as the source >> subdev might depend on a valid subdev state. > > Right. > > [...] BR, Hugues.
On 6/27/22 11:14, Hugues FRUCHET wrote: > Hi Marek, Hi, >>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote: >>>>> Any local subdev state should be allocated and free'd using >>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which >>>>> takes care of calling .init_cfg() subdev op. Without this, >>>>> subdev internal state might be uninitialized by the time >>>>> any other subdev op is called. >>> >>> Does this fix a bug you have? >> >> Yes > > Which bug did you encounter exactly ? The DCMI driver does set_fmt subdev call on the sensor driver instance. The mt9p031 sensor driver set_fmt depends on crop rectangle to be initialized _before_ set_fmt subdev call is made. Currently this initialization is done in open callback, which is too late, so when the DCMI does set_fmt subdev call, it operates on uninitialized private data. There is patch to mt9p031 driver which move the initialization to the right place in .init_cfg: [PATCH v2] media: mt9p031: Move open subdev op init code into init_cfg However, the .init_cfg is not called by DCMI right now. For that to be called in the right place, __v4l2_subdev_state_alloc() must be added, hence this patch. You won't trigger the problem on OV5640 because that one driver does not implement .init_cfg v4l2_subdev_ops . > This is strange that we have not yet encounter any problems around that > through our tests campaigns... or we have to enforce with a new test, so > better to know what your problem was exactly. You need a sensor driver which implements struct v4l2_subdev_ops .init_cfg and then have something in set_fmt depend on the initialization done in the .init_cfg callback . Then you would see the problem. >>> Wasn't this broken even before the active state, as init_cfg was not >>> called? >> >> Yes, this was always broken. I suspect nobody tested this mode of >> operation before. In my case, I have this DCMI driver connected >> directly to MT9P006 sensor. > > As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor. > I don't see the difference with our OV5640 used in parallel mode which > is a mainline config on our side, so one more time I wonder what the > problem was. See above.
On 6/27/22 14:53, Hugues FRUCHET wrote: > Hi Marek, Hi, > Thanks for explanation, I understand now. > > Please note that dcmi is not atmel-isi.c has same code structure, hence > same problem: > > static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f, > struct v4l2_subdev_state pad_state = { > .pads = &pad_cfg > }; > [...] > ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt, > > > Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME": > > drivers/media/platform/renesas/vsp1/vsp1_entity.c > /* > * FIXME: Drop this call, drivers are not supposed to use > * __v4l2_subdev_state_alloc(). > */ > entity->config = __v4l2_subdev_state_alloc(&entity->subdev, > "vsp1:config->lock", &key); > > > drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c > /* > * FIXME: Drop this call, drivers are not supposed to use > * __v4l2_subdev_state_alloc(). > */ > sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key); > > > So I wonder about introducing this new change in dcmi while it is marked > as "FIXME" in other camera interface drivers ? This is probably something Tomi/Laurent can answer better. It should be OK for this driver as far as I understand the discussion in this thread.
Thanks Tomi for details, OK for me with a FIXME on top, for the sake of uniformity with other drivers. BR, Hugues. On 6/27/22 15:30, Tomi Valkeinen wrote: > On 27/06/2022 16:01, Marek Vasut wrote: >> On 6/27/22 14:53, Hugues FRUCHET wrote: >>> Hi Marek, >> >> Hi, >> >>> Thanks for explanation, I understand now. >>> >>> Please note that dcmi is not atmel-isi.c has same code structure, >>> hence same problem: >>> >>> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f, >>> struct v4l2_subdev_state pad_state = { >>> .pads = &pad_cfg >>> }; >>> [...] >>> ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt, >>> >>> >>> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME": >>> >>> drivers/media/platform/renesas/vsp1/vsp1_entity.c >>> /* >>> * FIXME: Drop this call, drivers are not supposed to use >>> * __v4l2_subdev_state_alloc(). >>> */ >>> entity->config = __v4l2_subdev_state_alloc(&entity->subdev, >>> "vsp1:config->lock", &key); >>> >>> >>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c >>> /* >>> * FIXME: Drop this call, drivers are not supposed to use >>> * __v4l2_subdev_state_alloc(). >>> */ >>> sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key); >>> >>> >>> So I wonder about introducing this new change in dcmi while it is >>> marked as "FIXME" in other camera interface drivers ? >> >> This is probably something Tomi/Laurent can answer better. It should >> be OK for this driver as far as I understand the discussion in this >> thread. > > Yes and no. We shouldn't use __ funcs in the drivers. > __v4l2_subdev_state_alloc() calls exist in the current drivers as it > wasn't trivial to change the driver to do it otherwise while adding the > subdev state feature. > > If I recall right, the other users (at least some of them) were storing > internal state in the state, not passing it forward. And, of course, the > drivers were themselves interested in the state stored there. > > Here, we only need to allocate the state so that the driver is able to > call set_fmt on another subdev, so it's a bit different case. > > Anyway, I think it's _not_ ok to add __v4l2_subdev_state_alloc() without > a FIXME comment. However, I think it's ok to add a helper func, similar > to v4l2_subdev_call_state_active(), which in turn uses > __v4l2_subdev_state_alloc. > > However, if we end up in a situation where we think it's "normal" for > drivers to call __v4l2_subdev_state_alloc, we need to rename it and drop > the two underscores. But I think we're not there yet (and hopefully never). > > Tomi
diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c index ec54b5ecfd544..ef795c12fb233 100644 --- a/drivers/media/platform/st/stm32/stm32-dcmi.c +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c @@ -996,22 +996,26 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f, struct dcmi_framesize *sd_framesize) { const struct dcmi_format *sd_fmt; + static struct lock_class_key key; struct dcmi_framesize sd_fsize; struct v4l2_pix_format *pix = &f->fmt.pix; - struct v4l2_subdev_pad_config pad_cfg; - struct v4l2_subdev_state pad_state = { - .pads = &pad_cfg - }; + struct v4l2_subdev_state *sd_state; struct v4l2_subdev_format format = { .which = V4L2_SUBDEV_FORMAT_TRY, }; bool do_crop; int ret; + sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key); + if (IS_ERR(sd_state)) + return PTR_ERR(sd_state); + sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat); if (!sd_fmt) { - if (!dcmi->num_of_sd_formats) - return -ENODATA; + if (!dcmi->num_of_sd_formats) { + ret = -ENODATA; + goto done; + } sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1]; pix->pixelformat = sd_fmt->fourcc; @@ -1036,10 +1040,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f, } v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code); - ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, - &pad_state, &format); + ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format); if (ret < 0) - return ret; + goto done; /* Update pix regarding to what sensor can do */ v4l2_fill_pix_format(pix, &format.format); @@ -1079,7 +1082,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f, if (sd_framesize) *sd_framesize = sd_fsize; - return 0; +done: + __v4l2_subdev_state_free(sd_state); + return ret; } static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f) @@ -1183,31 +1188,33 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi, struct v4l2_pix_format *pix) { const struct dcmi_format *sd_fmt; + static struct lock_class_key key; + struct v4l2_subdev_state *sd_state; struct v4l2_subdev_format format = { .which = V4L2_SUBDEV_FORMAT_TRY, }; - struct v4l2_subdev_pad_config pad_cfg; - struct v4l2_subdev_state pad_state = { - .pads = &pad_cfg - }; int ret; + sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key); + if (IS_ERR(sd_state)) + return PTR_ERR(sd_state); + sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat); if (!sd_fmt) { - if (!dcmi->num_of_sd_formats) - return -ENODATA; + if (!dcmi->num_of_sd_formats) { + ret = -ENODATA; + goto done; + } sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1]; pix->pixelformat = sd_fmt->fourcc; } v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code); - ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, - &pad_state, &format); - if (ret < 0) - return ret; - - return 0; + ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format); +done: + __v4l2_subdev_state_free(sd_state); + return ret; } static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,
Any local subdev state should be allocated and free'd using __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which takes care of calling .init_cfg() subdev op. Without this, subdev internal state might be uninitialized by the time any other subdev op is called. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alain Volmat <alain.volmat@foss.st.com> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Philippe CORNU <philippe.cornu@foss.st.com> Cc: linux-stm32@st-md-mailman.stormreply.com Cc: linux-arm-kernel@lists.infradead.org --- drivers/media/platform/st/stm32/stm32-dcmi.c | 51 +++++++++++--------- 1 file changed, 29 insertions(+), 22 deletions(-)