[PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Hi Marek,

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 ?

Best regards,
Hugues.

On 6/27/22 13:30, Marek Vasut wrote:
> 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.