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

From: Hugues FRUCHET <hugues.fruchet@foss.st.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	Marek Vasut <marex@denx.de>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Hans Verkuil <hverkuil@xs4all.nl>
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 17:11:41 +0200	[thread overview]
Message-ID: <991ffe2d-fef3-0907-3f64-edcc5076762c@foss.st.com> (raw)
In-Reply-To: <b9c450b1-96d2-1ac5-0dec-04387903ebf2@ideasonboard.com>

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

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