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

From: Marek Vasut <marex@denx.de>
To: Hugues FRUCHET <hugues.fruchet@foss.st.com>,
	Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org,
	Alain Volmat <alain.volmat@foss.st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Amelie DELAUNAY <amelie.delaunay@foss.st.com>,
	Philippe CORNU <philippe.cornu@foss.st.com>,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()
Date: Mon, 27 Jun 2022 13:30:29 +0200	[thread overview]
Message-ID: <39657445-e5ac-bfd6-c122-b18088fa4b41@denx.de> (raw)
In-Reply-To: <5ee6c0c0-8ab0-561c-e1f6-b26e4ec438af@foss.st.com>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel