Message-ID: <87y0zsme7i.fsf@intel.com>
Date: Fri, 03 Jan 2025 11:36:17 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Simona Vetter <simona@...ll.ch>, Inki Dae <inki.dae@...sung.com>, Jagan
 Teki <jagan@...rulasolutions.com>, Marek Szyprowski
 <m.szyprowski@...sung.com>, Catalin Marinas <catalin.marinas@....com>,
 Will
 Deacon <will@...nel.org>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
 <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Daniel Thompson <danielt@...nel.org>,
 Andrzej Hajda <andrzej.hajda@...el.com>, Jonathan Corbet <corbet@....net>,
 Paul Kocialkowski <contact@...lk.fr>, Maxime Ripard <mripard@...nel.org>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 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>, Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
 <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Hervé Codina
 <herve.codina@...tlin.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, Paul
 Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH v5 03/10] drm/bridge: add support for refcounted DRM
 bridges

On Thu, 02 Jan 2025, Luca Ceresoli <luca.ceresoli@...tlin.com> wrote:
> Hello Jani,
>
> thanks for your review.
>
> On Tue, 31 Dec 2024 13:11:31 +0200
> Jani Nikula <jani.nikula@...ux.intel.com> wrote:
>
>> On Tue, 31 Dec 2024, Luca Ceresoli <luca.ceresoli@...tlin.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@...tlin.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.

drm_dbg_* can handle NULL drm device; maybe drm_WARN* should be modified
to do so as well?

> 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?

IMO the biggest benefit of new categories would be for very noisy
logging that you really only want enabled when debugging a specific
issue. Otherwise, the hard part about adding new categories is their
adoption.

For example, we now have DRM_UT_DP, but it's only haphazardly used here
and there. I don't really see a lot of point in having that separated
from DRM_UT_KMS. When would you want one but not the other? How would
you go about converting some KMS to DP logging, and why? What's special
about DP, why don't we have an HDMI category? Etc.

OTOH, DRM_UT_DP is also used for DP AUX transfer debug logging. I think
that would've been a good category on its own: Do you want noisy logging
about DPCD access or not? But not used for anything else.

Oh, having written the above, I looked up a18b21929453 ("drm/dp_helper:
Add DP aux channel tracing"). DRM_UT_DP *was* intended only for DP AUX
message tracing, but its naming unfortunately suggests a broader
category, and here we are.


BR,
Jani.


-- 
Jani Nikula, Intel

Powered by blists - more mailing lists