Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to devm_drm_bridge_alloc() API

[Liu Ying <victor.liu@nxp.com>]

On 04/30/2025, Luca Ceresoli wrote:
> Hello Liu,

Hi Luca,

> 
> On Tue, 29 Apr 2025 10:10:55 +0800
> Liu Ying <victor.liu@nxp.com> wrote:
> 
>> Hi,
>>
>> On 04/25/2025, Luca Ceresoli wrote:
>>> This is the new API for allocating DRM bridges.
>>>
>>> This driver embeds an array of channels in the main struct, and each
>>> channel embeds a drm_bridge. This prevents dynamic, refcount-based
>>> deallocation of the bridges.
>>>
>>> To make the new, dynamic bridge allocation possible:
>>>
>>>  * change the array of channels into an array of channel pointers
>>>  * allocate each channel using devm_drm_bridge_alloc()
>>>  * adapt the code wherever using the channels
>>>
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> [...]
> 
>>> @@ -345,8 +351,8 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>>>  free_child:
>>>  	of_node_put(child);
>>>  
>>> -	if (i == 1 && pc->ch[0].next_bridge)
>>> -		drm_bridge_remove(&pc->ch[0].bridge);
>>> +	if (i == 1 && pc->ch[0]->next_bridge)  
>>
>> Since this patch makes pc->ch[0] and pc->ch[1] be allocated separately,
>> pc->ch[0] could be NULL if channel0 is not available, hence a NULL pointer
>> dereference here...
> 
> See below for this.
> 
>>> +		drm_bridge_remove(&pc->ch[0]->bridge);
>>>  
>>>  	pm_runtime_disable(dev);
>>>  	return ret;
>>> @@ -359,7 +365,7 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev)
>>>  	int i;
>>>  
>>>  	for (i = 0; i < 2; i++) {
>>> -		ch = &pc->ch[i];
>>> +		ch = pc->ch[i];
>>>  
>>>  		if (!ch->is_available)  
>>
>> ...and here too.
> 
> This is indeed a bug, I should have checked the pointer for being
> non-NULL.
> 
> Looking at that more closely, I think the is_available flag can be
> entirely removed now. The allocation itself (ch != NULL) now is
> equivalent. Do you think my reasoning is correct?
> 
> Ouch! After writing the previous paragraph I realized you proposed this
> a few lines below! OK, removing is_available. :)
> 
> [...]
> 
>> On top of this patch series, this issue doesn't happen if I apply the below
>> change:
> 
> [...]
> 
>> @@ -351,7 +349,7 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>>  free_child:
>>         of_node_put(child);
>>  
>> -       if (i == 1 && pc->ch[0]->next_bridge)
>> +       if (i == 1 && pc->ch[0])
>>                 drm_bridge_remove(&pc->ch[0]->bridge);
> 
> Unrelated to this patch, but as I looked at it more in depth now, I'm
> not sure this whole logic is robust, even in the original code.
> 
> The 'i == 1' check here seems to mean "if some error happened when
> handling channel@1, that means channel@0 was successfully initialized,
> so let's clean up channel 0".
> 
> However my understanding of the bindings is that device tree is allowed
> to have the channel@1 node before the channel@0 node (or even channel@1
> without channel@0, but that's less problematic here).
> 
> In such case (channel@1 before channel@0), this would happen:
> 
>  1. alloc and init ch[1], all OK
>  2. alloc and init ch[0], an error happens
>     (e.g. of_graph_get_remote_node() fails)
> 
> So we'd reach the free_child: label, and we should call
> drm_bridge_remove() for ch[1]->bridge, but there's no code to do that.
> 
> To be robust in such a case, I think both channels need to be checked
> independently, as the status of one does not imply the status of the
> other. E.g.:
> 
>   for (i = 0; i < 2; i++)
>       if (pc->ch[i] && pc->ch[i]->next_bridge)
>           drm_bridge_remove(&pc->ch[i]->bridge);
> 
> (which is similar to what .remove() does after the changes discussed in
> this thread, and which I have queued for v3)
> 
> What's your opinion? Do you think I missed anything?

The pixel combiner DT node would be added in imx8-ss-dc{0,1}.dtsi, please
see the case for imx8-ss-dc0.dtsi introduced by an in-flight patch[1].  As
channel@{0,1} child nodes always exist(DT overlay cannot effectively delete
any of them) and channel@0 always comes first, there is no problematic case.

> 
> Thanks for taking the time to dig into this!

After looking into this patch and patch 31(though I've already provided my A-b)
more closely, I think the imx8qxp_pc and imx8{qm,qxp}_ldb main structures
should have the same life time with the embedded DRM bridges, because for
example the clk_apb clock in struct imx8qxp_pc would be accessed by the
imx8qxp_pc_bridge_mode_set DRM bridge callback.  But, IIUC, your patches extend
the life time for the embedded channel/bridge structures only, but not for the
main structures.  What do you think ?

> 
> Best regards,
> Luca
> 

[1] https://lore.kernel.org/dri-devel/20250414035028.1561475-17-victor.liu@nxp.com/

-- 
Regards,
Liu Ying