On 04/30/2025, Luca Ceresoli wrote:
> Hello Liu,
Hi Luca,
>
> On Tue, 29 Apr 2025 10:10:55 +0800
> Liu Ying <victor....@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.ceres...@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....@nxp.com/
--
Regards,
Liu Ying
