On 05/26/2025, Luca Ceresoli wrote:
> Hello Liu,
Hi Luca,
>
> On Thu, 22 May 2025 11:01:13 +0800
> Liu Ying <victor.liu@xxxxxxx> 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
