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.
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. 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
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.
>
> > Changes in v6:
> > - use drm_warn, not WARN_ON (Jani Nikula)
> > - Add devm_drm_bridge_alloc() to replace drm_bridge_init() (similar to
> > drmm_encoder_alloc)
> > - Remove .destroy func: deallocation is done via the struct offset
> > computed by the devm_drm_bridge_alloc() macro
> > - use fixed free callback, as the same callback is used in all cases
> > anyway (remove free_cb, add bool is_refcounted)
> > - add drm_bridge_get/put() to drm_bridge_attach/detach() (add the bridge
> > to a list)
> > - make some DRM_DEBUG() strings more informative
> >
> > This patch was added in v5.
> > ---
> > drivers/gpu/drm/drm_bridge.c | 76 ++++++++++++++++++++++++++--
> > include/drm/drm_bridge.h | 117 +++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 189 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 1955a231378050abf1071d74a145831b425c47c5..f694b32ca59cb91c32846bc00b43360df41cc1ad 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -200,6 +200,57 @@
> > DEFINE_MUTEX(bridge_lock);
> > LIST_HEAD(bridge_list);
> >
> > +/* Internal function (for refcounted bridges) */
> > +void __drm_bridge_free(struct kref *kref)
> > +{
> > + struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> > + void *container = ((void *)bridge) - bridge->container_offset;
> > +
> > + DRM_DEBUG("bridge=%p, container=%p FREE\n", bridge, container);
>
> Pointers are not really useful to track here, since they are obfuscated
> most of the time. Using the bridge device name would probably be better
> (or removing the SHOUTING DEBUG entirely :))
bridge device name or bridge funcs (I opted for the latter for the
debugfs file)
--
With best wishes
Dmitry
