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

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

On 05/26/2025, Luca Ceresoli wrote:
> Hello Liu,

Hi Luca,

> 
> On Thu, 22 May 2025 11:01:13 +0800
> Liu Ying <victor.liu@nxp.com> wrote:
> 
>> On 05/07/2025, Luca Ceresoli wrote:
>>
>> [...]
>>
>>>> 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 ?  
>>>
>>> I see you concern, but I'm sure the change I'm introducing is not
>>> creating the problem you are concerned about.
>>>
>>> The key aspect is that my patch is merely changing the lifetime of the
>>> _allocation_ of the drm_bridge, not its usage. On drm_bridge_remove()
>>> the bridge is removed from its encoder chain and it is completely not
>>> reachable, both before and after my patch. With my patch it is not  
>>
>> drm_bridge_remove() only removes a bridge from the global bridge_list defined
>> in drm_bridge.c.  drm_bridge_detach() is the one which removes a bridge from
>> it's encoder chain.  It looks like you wrongly thought drm_bridge_remove()
>> is drm_bridge_detach().
> 
> Indeed my sentence was inaccurate, sorry about that.
> 
>> So, even if drm_bridge_remove() is called, the removed
>> bridge could still be in it's encoder chain, hence an atomic commit could still
>> access the allocated bridge(with lifetime extended) and the clock_apb clock
>> for example in struct imx8qxp_pc could also be accessed.  That's why I think
>> the main structures should have the same lifetime with the allocated bridge.
> 
> As the long-term goal is to allow bridges to be hot-removable,
> decoupling the lifetime of the various components is a necessary step.
> Definitely it will open other issues, and especially the removal during
> atomic updates. This has been discussed already, and there is a
> proposed plan to handle it.
> 
> First, here is the grand plan (mentioned in the v3 cover letter):
> 
>  1. ➜ add refcounting to DRM bridges (struct drm_bridge)
>  2. handle gracefully atomic updates during bridge removal
>  3. avoid DSI host drivers to have dangling pointers to DSI devices
>  4. finally, let bridges be removable (depends on 1+2+3)
> 
> We are now at step 1. Your concern, as I understand it, will be
> addressed at step 2. Bridges won't be removable until step 4, so the
> current changes are not introducing a misbehavior but rather preparing
> the ground with all the necessary infrastructure changes.
> 
> Step 2 was discussed in the past [0], and the idea proposed by Maxime
> is to introduce a "gone" or "unplugged" flag and drm_bridge_enter() /
> drm_bridge_exit() functions. The principle is the same as struct
> drm_device.unplugged and drm_dev_enter/exit().
> 
> In a nutshell the idea is:
> 
>  - drm_bridge.unplugged is initialized to false
>  - drm_bridge_enter() returns false if drm_bridge.unplugged == true
>  - any code holding a pointer to the bridge (including the bridge driver
>    itself) and operating on the bridge (including removal) needs to do:
>      if (drm_bridge_enter()) {
>          do something;
>          drm_bridge_exit();
>      }
>  - when the bridge is removed, the driver removal function sets
>    dev_bridge.unplugged = true
> 
> The "do something" above includes any access to device resources,
> including clocks (and clk_apb).
> 
> In other words, two pieces of code can not access the bridge structure
> at the same time. This includes bridge removal VS any atomic operations.
> 
> Do you think this addresses your concern?

Yes, drm_bridge_{enter,exit} address it.

> 
> 
> For you to have a better picture of the path, here's an additional
> clarification about drm_bridge_attach/detach() and
> drm_bridge_add/remove(). As part of step 1 of the grand plan, both of
> them will drm_bridge_get/put() the bridge, so that no bridge is freed
> if it is either in the global bridge_list or in any encoder chain.
> 
> Patches for this are already approved by Maxime [1][2]. They cannot be
> applied until all bridge drivers have been converted to the new
> devm_drm_bridge_alloc() API, so they depend on this series to be
> completely applied. We are getting pretty close: as of now the entire
> series has been applied except for this and another driver.
> 
> [0] https://lore.kernel.org/all/20250129125153.35d0487a@booty/t/#u
> [1] https://patchwork.freedesktop.org/patch/643095/
> [2] https://patchwork.freedesktop.org/patch/643096/
> 
> Best regards,
> Luca
> 

-- 
Regards,
Liu Ying