Re: [PATCH v5 03/10] drm/bridge: add support for refcounted DRM bridges

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

Hello Jani,

thanks for your review.

On Tue, 31 Dec 2024 13:11:31 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Tue, 31 Dec 2024, Luca Ceresoli <luca.ceresoli@bootlin.com> 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 and the free_cb. So here
> > we add to struct drm_bridge just the refcount and free_cb fields (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.
> >
> > The function to enroll a private bridge driver data structure into
> > refcounting is based on drm_connector_init() and so called
> > drm_bridge_init() for symmetry, even though it does not initialize anything
> > except the refcounting and the funcs pointer which is needed to access
> > funcs->destroy.
> >
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >
> > ---
> >
> > This patch was added in v5.
> > ---
> >  drivers/gpu/drm/drm_bridge.c |  87 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     | 102 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 189 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index b1f0d25d55e23000521ac2ac37ee410348978ed4..6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -198,6 +198,85 @@
> >  static DEFINE_MUTEX(bridge_lock);
> >  static LIST_HEAD(bridge_list);
> >  
> > +static void drm_bridge_put_void(void *data)
> > +{
> > +	struct drm_bridge *bridge = (struct drm_bridge *)data;
> > +
> > +	drm_bridge_put(bridge);
> > +}
> > +
> > +static void drm_bridge_free(struct kref *kref)
> > +{
> > +	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> > +
> > +	DRM_DEBUG("bridge=%p\n", bridge);
> > +
> > +	WARN_ON(!bridge->funcs->destroy);  
> 
> Please don't add new DRM_DEBUG or WARN_ON where you can use the
> drm_dbg_* or drm_WARN_ON variants.

Good point. However drm_WARN_ON() cannot be used because it needs a
non-NULL struct drm_drm_device pointer which is not always available
here: in case of -EPROBE_DEFER it usually isn't. I guess I'll go for
drm_dbg_core() or drm_warn[_once](), even though none of them prints a
stack trace and I find that would be useful.

This is raising a loosely-related question about the DRM_DEBUG()s this
patch is adding, such as the one quoted above: would it make sense to
add a new drm_debug_category value for the bridge refcounting
functions? Or for bridges altogether? They are pretty different from
the core messages, and it may be useful to see only the refcounting
messages or only the core messages.

DRM_UT_BRIDGE?
DRM_UT_BRIDGE_REFCOUNT?

Luca

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