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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Liu,

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?


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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com