Hello Maxime, Dmitry, On Mon, 10 Feb 2025 16:23:44 +0200 Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > On Mon, 10 Feb 2025 at 14:31, Maxime Ripard <mripard@xxxxxxxxxx> > wrote: > > > > On Fri, Feb 07, 2025 at 09:54:06PM +0200, Dmitry Baryshkov wrote: > > > On Fri, Feb 07, 2025 at 12:47:51PM +0100, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Thu, Feb 06, 2025 at 07:14:29PM +0100, Luca Ceresoli wrote: > > > > > DRM bridges are currently considered as a fixed element of a > > > > > DRM card, and thus their lifetime is assumed to extend for as > > > > > long as the card exists. New use cases, such as hot-pluggable > > > > > hardware with video bridges, require DRM bridges to be added > > > > > and removed to a DRM card without tearing the card down. This > > > > > is possible for connectors already (used by DP MST), so add > > > > > this possibility to DRM bridges as well. > > > > > > > > > > Implementation is based on drm_connector_init() as far as it > > > > > makes sense, and differs when it doesn't. A difference is > > > > > that bridges are not exposed to userspace, hence struct > > > > > drm_bridge does not embed a struct drm_mode_object which > > > > > would provide the refcount. Instead we add to struct > > > > > drm_bridge a refcount field (we don't need other struct > > > > > drm_mode_object fields here) and instead of using the > > > > > drm_mode_object_*() functions we reimplement from those > > > > > functions the few lines that drm_bridge needs for refcounting. > > > > > > > > > > Also add a new devm_drm_bridge_alloc() macro to allocate a > > > > > new refcounted bridge. > > > > > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> > > > > > > > > So, a couple of general comments: > > > > > > > > - I've said it a couple of times already, but I really think > > > > you're making it harder than necessary for you here. This (and > > > > only this!) should be the very first series you should be > > > > pushing. The rest can only ever work if that work goes through, > > > > and it's already hard enough as it is. So, split that patch > > > > into a series of its own, get that merged, and then we will be > > > > able to deal with panels conversion and whatever. That's even > > > > more true with panels since there's ongoing work that will make > > > > it easier for you too. So the best thing here is probably to > > > > wait. The idea you proposed was to handle the issues current panel bridge code adds to the hotplug work by adding a .destroy callback and some more devm magic. I explored the idea but even after some clarifications from you it still didn't appear clearly doable and correct to me. And even in the case it were perfectly correct and doable, it is based on adding more complexity and "magic" on top of a topic that is already hard to understand: panel_bridge lifetime. So I opted for the other way: rework panel_bridge code so its lifetime is clear and as one would expect (panel_bridge lifetime == panel lifetime). Possibly more work for me, but now it's done and it's in these patches so why waiting? This work has made the hotplug work on top of it way cleaner, and you can also see the good cleanup in the samsung-dsim driver in patch 11. I'm at work to apply the improvements suggested by Dmitry and send a new version, and I'm fine with applying more fixes as needed. And after this "basic" panel_bridge, more cleanups and rationalization can be done at any moment, in order to make the panel and panel_bridge even closer, and remove the code duplication. > > > Luca and I had a quick chat on this during FOSDEM. I really think > > > that panel (part of the) series can go in first as it fixes a > > > very well known bug _and_ allows a pretty good cleanup to a whole > > > set of drivers. > > > > I don't necessarily disagree on principle, but if you state that it > > can get first, and fixes a known problem (which one?), then it > > should be a separate, standalone, series. > > A problem of panel bridges having the wrong lifetime because of devm_ > attachment to a wrong device and so either being kept for too long or > being destroyed too early. Exactly, because panel_bridge is usually created in the consumer driver context, so the devm calls get a struct device pointer to the consumer, not the panel itself (more in patch 8). > > Ever-expanding features are bad for both the reviewers and the > > contributors, even more so when the discussion happens off-list. > > > > > With all those panel / bridge wrappers gone we should be able to > > > see a clearer picture of what individual drivers are doing. In > > > other words, which memory and which code actually hosts and uses > > > internal 'next_bridge' reference. > > > > > > > - This patch really needs to be split into several patches, > > > > something along the lines of: > > > > > > > > + Creating devm_drm_bridge_alloc() > > > > + Adding refcounting > > > > + Taking the references in all the needed places > > > > + Converting a bunch of drivers OK, I can split it. > > > The last two parts seem troublematic to me, but, I must admit, I > > > didn't spend so much time reviewing all drm_bridge usage > > > patterns. > > > > Why? the third one is already done by that patch, the fourth can > > relatively easily be done using coccinelle. > > I have doubts about cocci. It doesn't have a way to know, what is the > lifetime of the references to the reference-holding memory. Maybe I'm > missing a point there. I haven't looked into that, but I think coccinelle would be helpful in adding the needed gets, but probably not for the puts, which are usually more complex to get right. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
