diff mbox series

media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

Message ID 20220618222442.478285-1-marex@denx.de
State New
Headers show
Series media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc() | expand

Commit Message

Marek Vasut June 18, 2022, 10:24 p.m. UTC
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(-)

Comments

Laurent Pinchart June 20, 2022, 11:28 a.m. UTC | #1
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 ?
Marek Vasut June 20, 2022, 2:06 p.m. UTC | #2
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.

[...]
Hugues FRUCHET June 27, 2022, 9:14 a.m. UTC | #3
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.
Marek Vasut June 27, 2022, 11:30 a.m. UTC | #4
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.
Marek Vasut June 27, 2022, 1:01 p.m. UTC | #5
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.
Hugues FRUCHET June 27, 2022, 3:11 p.m. UTC | #6
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 mbox series

Patch

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,