Message-ID: <20250507161311.6e434f2f@booty>
Date: Wed, 7 May 2025 16:13:11 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Liu Ying <victor.liu@....com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Andrzej Hajda
 <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
 Robert Foss <rfoss@...nel.org>, Laurent Pinchart
 <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>,
 Jernej Skrabec <jernej.skrabec@...il.com>, Jagan Teki
 <jagan@...rulasolutions.com>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
 <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Douglas Anderson
 <dianders@...omium.org>, Chun-Kuang Hu <chunkuang.hu@...nel.org>, Krzysztof
 Kozlowski <krzk@...nel.org>, Anusha Srivatsa <asrivats@...hat.com>, Paul
 Kocialkowski <paulk@...-base.io>, Dmitry Baryshkov <lumag@...nel.org>, Hui
 Pu <Hui.Pu@...ealthcare.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, dri-devel@...ts.freedesktop.org,
 asahi@...ts.linux.dev, linux-kernel@...r.kernel.org,
 chrome-platform@...ts.linux.dev, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, linux-renesas-soc@...r.kernel.org,
 platform-driver-x86@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org,
 linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to
 devm_drm_bridge_alloc() API

Hello Liu,

On Wed, 7 May 2025 18:16:28 +0800
Liu Ying <victor.liu@....com> 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
> >>> freed immediately, but it's just a piece of "wasted" memory that is
> >>> still allocated until elsewhere in the kernel there are pointers to it,
> >>> to avoid use-after-free.
> >>>
> >>> With this explanation, do you think my patch is correct (after fixing
> >>> the bug we already discussed of course)?    
> >>
> >> I tend to say your patch is not correct because we'll eventually make sure
> >> that removing a bridge module is safe when doing atomic commit,  
> > 
> > I think your sentence can be rephrased as "your patch is correct with
> > the current code base where bridges are not (yet) removable, but there
> > will be a problem when they start to actually be removable".
> > 
> > Is my understanding correct? If it is, I agree on that sentence.  
> 
> Nope, I meant your patch should align the life times of the main structures
> and the DRM bridges, for the sake of the kinda long term goal - remove bridge
> driver module safely when doing atomic commit.

Again, I don't think there is any bug introduced by this patch (once
the NULL ptr deref bug we already discussed is fixed). No bridge can be
removed as of now, with or without this patch.

You concern that this patch would make things more complex in the
future, when bridges will actually become removable and they could be
during atomic updates. But about this...

> > The work to have removable bridges is massive and non-trivial, so it
> > will need to be tackled in steps. The grand plan [0] is:
> > 
> >  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. finish the hotplug bridge work, removing the "always-disconnected"
> >     connector, moving code to the core and potentially removing the
> >     hotplug-bridge itself (this needs to be clarified as points 1-3 are
> >     developed)  
> 
> I'm busy with internal things these days and cannot look into the grand
> plan and steps closely, sorry about that.

...I'll wait until you have time to look into that more closely. There
is just no way to understand this whole topic without some dedicated
attention, which takes time unavoidably.

In the meanwhile I am going to send v3 soon with the known bug fixed,
so the best version is available to continue this discussion.

> > I am at step 1 right now. Removal during atomic updates is step 2,
> > ideas about how to implement that are already being discussed [1],
> > there's a practical plan proposed by Maxime with the goal of reaching
> > removable bridges without breaking things along the path.
> > 
> > [0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/
> > [1] https://lore.kernel.org/all/20250106-vigorous-talented-viper-fa49d9@houat/
> >   
> >> which means
> >> the main structures should have the same life time with the DRM bridges.  
> > 
> > The word "lifetime" mean two things for bridges:
> > 
> >  * the time span during which memory is allocated for a struct
> >    drm_bridge (along with the embedding struct)  
> 
> Note that with your patch set the imx8*-ldb drivers and this bridge driver
> won't allocate the DRM bridge along with the embedding struct.

By "embedding struct" I mean the struct imx8qxp_pc_channel that embeds
the struct drm_bridge. Sorry, I realize my wording was ambiguous.

> This makes
> me worry, because maybe these drivers are the only "special" ones in this
> patch set and I don't want them to be "special" after your patch set is
> applied.

Luca

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

Powered by blists - more mailing lists