[PATCH v7 0/2] drm: show "all" bridges in debugfs

[PATCH v7 0/2] drm: show "all" bridges in debugfs

[Luca Ceresoli]

This series collects some minor improvements and fixes previously part of
[0], hence the "v7" version number.

It adds a /sys/kernel/debug/dri/bridges file showing all bridges between
drm_bridge_add() and drm_bridge_remove(), which might not be bound to any
encoder and thus not visible anywhere in debugfs.

[0] https://lore.kernel.org/dri-devel/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/

Luca

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (2):
      drm/bridge: move bridges_show logic from drm_debugfs.c
      drm/debugfs: add top-level 'bridges' file showing all added bridges

 drivers/gpu/drm/drm_bridge.c   | 53 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_debugfs.c  | 42 ++++++++++++++-------------------
 drivers/gpu/drm/drm_drv.c      |  1 +
 drivers/gpu/drm/drm_internal.h |  5 ++++
 include/drm/drm_bridge.h       | 10 ++++++++
 5 files changed, 86 insertions(+), 25 deletions(-)
---
base-commit: 09c117e66c48bfdccc41b380d6300f0a1011ec7e
change-id: 20250225-drm-debugfs-show-all-bridges-642e870b71e7

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Luca Ceresoli]

In preparation to expose more info about bridges in debugfs, which will
require more insight into drm_bridge data structures, move the bridges_show
code to drm_bridge.c.

Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v7.
---
 drivers/gpu/drm/drm_bridge.c  | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_debugfs.c | 27 ++-------------------------
 include/drm/drm_bridge.h      |  8 ++++++++
 3 files changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 241a384ebce39b4a3db58c208af27960904fc662..6e1e02c1cf443fbaa764765ad369b7a34cc03f2d 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1335,6 +1335,39 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 EXPORT_SYMBOL(of_drm_find_bridge);
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
+					     struct drm_encoder *encoder)
+{
+	struct drm_bridge *bridge;
+	unsigned int idx = 0;
+
+	drm_for_each_bridge_in_chain(encoder, bridge) {
+		drm_printf(p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
+		drm_printf(p, "\ttype: [%d] %s\n",
+			   bridge->type,
+			   drm_get_connector_type_name(bridge->type));
+
+		if (bridge->of_node)
+			drm_printf(p, "\tOF: %pOFfc\n", bridge->of_node);
+
+		drm_printf(p, "\tops: [0x%x]", bridge->ops);
+		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
+			drm_puts(p, " detect");
+		if (bridge->ops & DRM_BRIDGE_OP_EDID)
+			drm_puts(p, " edid");
+		if (bridge->ops & DRM_BRIDGE_OP_HPD)
+			drm_puts(p, " hpd");
+		if (bridge->ops & DRM_BRIDGE_OP_MODES)
+			drm_puts(p, " modes");
+		if (bridge->ops & DRM_BRIDGE_OP_HDMI)
+			drm_puts(p, " hdmi");
+		drm_puts(p, "\n");
+	}
+}
+EXPORT_SYMBOL(drm_bridge_debugfs_show_encoder_bridges);
+#endif
+
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
 MODULE_DESCRIPTION("DRM bridge infrastructure");
 MODULE_LICENSE("GPL and additional rights");
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6b2178864c7ee12db9aa1f562e106b2f604439f8..a625c0c9a9aa076af19c8ba47564dbb24ba71c9a 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -744,31 +744,8 @@ static int bridges_show(struct seq_file *m, void *data)
 {
 	struct drm_encoder *encoder = m->private;
 	struct drm_printer p = drm_seq_file_printer(m);
-	struct drm_bridge *bridge;
-	unsigned int idx = 0;
-
-	drm_for_each_bridge_in_chain(encoder, bridge) {
-		drm_printf(&p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
-		drm_printf(&p, "\ttype: [%d] %s\n",
-			   bridge->type,
-			   drm_get_connector_type_name(bridge->type));
-
-		if (bridge->of_node)
-			drm_printf(&p, "\tOF: %pOFfc\n", bridge->of_node);
-
-		drm_printf(&p, "\tops: [0x%x]", bridge->ops);
-		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
-			drm_puts(&p, " detect");
-		if (bridge->ops & DRM_BRIDGE_OP_EDID)
-			drm_puts(&p, " edid");
-		if (bridge->ops & DRM_BRIDGE_OP_HPD)
-			drm_puts(&p, " hpd");
-		if (bridge->ops & DRM_BRIDGE_OP_MODES)
-			drm_puts(&p, " modes");
-		if (bridge->ops & DRM_BRIDGE_OP_HDMI)
-			drm_puts(&p, " hdmi");
-		drm_puts(&p, "\n");
-	}
+
+	drm_bridge_debugfs_show_encoder_bridges(&p, encoder);
 
 	return 0;
 }
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 496dbbd2ad7edff7f091adfbe62de1e33ef0cf07..23c062853e7bb42d8d73af35ef7f16fb37345a37 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1108,4 +1108,12 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
 }
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
+					     struct drm_encoder *encoder);
+#else
+static inline void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
+							   struct drm_encoder *encoder) {}
+#endif
+
 #endif

-- 
2.48.1

Re: [PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Jani Nikula]

On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> In preparation to expose more info about bridges in debugfs, which will
> require more insight into drm_bridge data structures, move the bridges_show
> code to drm_bridge.c.
>
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

I hate myself for doing this on a patch that's at v7... but here goes.

Perhaps consider moving the bridges debugfs creation and fops to
drm_bridge.c instead of just adding
drm_bridge_debugfs_show_encoder_bridges().

For example, add drm_bridge_debugfs_add(struct drm_encoder *encoder),
which then contains the debugfs_create_file() call.

Interestingly, this lets you drop a lot of #ifdef CONFIG_DEBUG_FS. The
compiler optimizes the fops struct and the functions away when
debugfs_create_file() becomes a stub for CONFIG_DEBUG_FS=n. It becomes
all around cleaner.


BR,
Jani.


>
> ---
>
> This patch was added in v7.
> ---
>  drivers/gpu/drm/drm_bridge.c  | 33 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_debugfs.c | 27 ++-------------------------
>  include/drm/drm_bridge.h      |  8 ++++++++
>  3 files changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 241a384ebce39b4a3db58c208af27960904fc662..6e1e02c1cf443fbaa764765ad369b7a34cc03f2d 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1335,6 +1335,39 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  EXPORT_SYMBOL(of_drm_find_bridge);
>  #endif
>  
> +#ifdef CONFIG_DEBUG_FS
> +void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
> +					     struct drm_encoder *encoder)
> +{
> +	struct drm_bridge *bridge;
> +	unsigned int idx = 0;
> +
> +	drm_for_each_bridge_in_chain(encoder, bridge) {
> +		drm_printf(p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
> +		drm_printf(p, "\ttype: [%d] %s\n",
> +			   bridge->type,
> +			   drm_get_connector_type_name(bridge->type));
> +
> +		if (bridge->of_node)
> +			drm_printf(p, "\tOF: %pOFfc\n", bridge->of_node);
> +
> +		drm_printf(p, "\tops: [0x%x]", bridge->ops);
> +		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
> +			drm_puts(p, " detect");
> +		if (bridge->ops & DRM_BRIDGE_OP_EDID)
> +			drm_puts(p, " edid");
> +		if (bridge->ops & DRM_BRIDGE_OP_HPD)
> +			drm_puts(p, " hpd");
> +		if (bridge->ops & DRM_BRIDGE_OP_MODES)
> +			drm_puts(p, " modes");
> +		if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> +			drm_puts(p, " hdmi");
> +		drm_puts(p, "\n");
> +	}
> +}
> +EXPORT_SYMBOL(drm_bridge_debugfs_show_encoder_bridges);
> +#endif
> +
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
>  MODULE_DESCRIPTION("DRM bridge infrastructure");
>  MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b2178864c7ee12db9aa1f562e106b2f604439f8..a625c0c9a9aa076af19c8ba47564dbb24ba71c9a 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -744,31 +744,8 @@ static int bridges_show(struct seq_file *m, void *data)
>  {
>  	struct drm_encoder *encoder = m->private;
>  	struct drm_printer p = drm_seq_file_printer(m);
> -	struct drm_bridge *bridge;
> -	unsigned int idx = 0;
> -
> -	drm_for_each_bridge_in_chain(encoder, bridge) {
> -		drm_printf(&p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
> -		drm_printf(&p, "\ttype: [%d] %s\n",
> -			   bridge->type,
> -			   drm_get_connector_type_name(bridge->type));
> -
> -		if (bridge->of_node)
> -			drm_printf(&p, "\tOF: %pOFfc\n", bridge->of_node);
> -
> -		drm_printf(&p, "\tops: [0x%x]", bridge->ops);
> -		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
> -			drm_puts(&p, " detect");
> -		if (bridge->ops & DRM_BRIDGE_OP_EDID)
> -			drm_puts(&p, " edid");
> -		if (bridge->ops & DRM_BRIDGE_OP_HPD)
> -			drm_puts(&p, " hpd");
> -		if (bridge->ops & DRM_BRIDGE_OP_MODES)
> -			drm_puts(&p, " modes");
> -		if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> -			drm_puts(&p, " hdmi");
> -		drm_puts(&p, "\n");
> -	}
> +
> +	drm_bridge_debugfs_show_encoder_bridges(&p, encoder);
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 496dbbd2ad7edff7f091adfbe62de1e33ef0cf07..23c062853e7bb42d8d73af35ef7f16fb37345a37 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1108,4 +1108,12 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
>  }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_FS
> +void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
> +					     struct drm_encoder *encoder);
> +#else
> +static inline void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
> +							   struct drm_encoder *encoder) {}
> +#endif
> +
>  #endif

-- 
Jani Nikula, Intel

Re: [PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Luca Ceresoli]

Hello Jani,

On Tue, 25 Feb 2025 18:36:41 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > In preparation to expose more info about bridges in debugfs, which will
> > require more insight into drm_bridge data structures, move the bridges_show
> > code to drm_bridge.c.
> >
> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>  
> 
> I hate myself for doing this on a patch that's at v7... but here goes.

Please don't! :-) This patch is new in v7, and a different (and
definitely worse) approach was present in v6, but there was nothing
before.

> Perhaps consider moving the bridges debugfs creation and fops to
> drm_bridge.c instead of just adding
> drm_bridge_debugfs_show_encoder_bridges().
> 
> For example, add drm_bridge_debugfs_add(struct drm_encoder *encoder),
> which then contains the debugfs_create_file() call.

I think it should go in drm_encoder.c, not drm_bridge.c, right? Here we
are showing the bridges attached to an encoder, so the entry point is
each encoder.

On the other hand in patch 2 we should move the
drm_debugfs_global_add() code to drm_bridge.c, as it's showing bridges
ina encoder-independent way.

And finally drm_bridge should export the common
drm_bridge_debugfs_show_bridge() function to drm_encoder.c.

Do you think this is correct?

> Interestingly, this lets you drop a lot of #ifdef CONFIG_DEBUG_FS. The
> compiler optimizes the fops struct and the functions away when
> debugfs_create_file() becomes a stub for CONFIG_DEBUG_FS=n. It becomes
> all around cleaner.

This surely makes the idea interesting. Cleaner code is always welcome.

Luca

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

Re: [PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Jani Nikula]

On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> Hello Jani,
>
> On Tue, 25 Feb 2025 18:36:41 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>> > In preparation to expose more info about bridges in debugfs, which will
>> > require more insight into drm_bridge data structures, move the bridges_show
>> > code to drm_bridge.c.
>> >
>> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>  
>> 
>> I hate myself for doing this on a patch that's at v7... but here goes.
>
> Please don't! :-) This patch is new in v7, and a different (and
> definitely worse) approach was present in v6, but there was nothing
> before.
>
>> Perhaps consider moving the bridges debugfs creation and fops to
>> drm_bridge.c instead of just adding
>> drm_bridge_debugfs_show_encoder_bridges().
>> 
>> For example, add drm_bridge_debugfs_add(struct drm_encoder *encoder),
>> which then contains the debugfs_create_file() call.
>
> I think it should go in drm_encoder.c, not drm_bridge.c, right? Here we
> are showing the bridges attached to an encoder, so the entry point is
> each encoder.

I'm still thinking drm_bridge.c, because it's about bridges and their
details. The encoder shouldn't care about bridge implementation details.

> On the other hand in patch 2 we should move the
> drm_debugfs_global_add() code to drm_bridge.c, as it's showing bridges
> ina encoder-independent way.

Agreed on that.

> And finally drm_bridge should export the common
> drm_bridge_debugfs_show_bridge() function to drm_encoder.c.

Disagree. That will still require the EXPORT and #ifdefs around
CONFIG_DEBUG_FS.

> Do you think this is correct?
>
>> Interestingly, this lets you drop a lot of #ifdef CONFIG_DEBUG_FS. The
>> compiler optimizes the fops struct and the functions away when
>> debugfs_create_file() becomes a stub for CONFIG_DEBUG_FS=n. It becomes
>> all around cleaner.
>
> This surely makes the idea interesting. Cleaner code is always welcome.

You might find drivers/gpu/drm/i915/display/intel_display_debugfs.c
helpful (and/or confusing...). It's in a long-term flux towards the
approach of having the debugfs stuff next to the implementation.

You have intel_display_debugfs_register() for global stuff, which then
does the same for various functional blocks.

Similarly intel_connector_debugfs_add() and intel_crtc_debugfs_add() for
connector and crtc specific debugfs files.

The individual functionality specific *_register() and *_add() functions
don't have conditional compilation. But they become empty functions as
the debugfs functions become empty stubs with CONFIG_DEBUG_FS=n, and
lots of stuff just gets optimized away.

Moreover, having the debugfs next to the implementation has helped us
abstract implementation details better, and reduce exposed functions
from compilation units.

In this case, you'd be able to add more bridge specific debugfs files
later on without touching anything else than drm_bridge.c.


BR,
Jani.



-- 
Jani Nikula, Intel

Re: [PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Luca Ceresoli]

Hello Jani,

On Tue, 25 Feb 2025 20:21:50 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > Hello Jani,
> >
> > On Tue, 25 Feb 2025 18:36:41 +0200
> > Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >  
> >> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:  
> >> > In preparation to expose more info about bridges in debugfs, which will
> >> > require more insight into drm_bridge data structures, move the bridges_show
> >> > code to drm_bridge.c.
> >> >
> >> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>    
> >> 
> >> I hate myself for doing this on a patch that's at v7... but here goes.  
> >
> > Please don't! :-) This patch is new in v7, and a different (and
> > definitely worse) approach was present in v6, but there was nothing
> > before.
> >  
> >> Perhaps consider moving the bridges debugfs creation and fops to
> >> drm_bridge.c instead of just adding
> >> drm_bridge_debugfs_show_encoder_bridges().
> >> 
> >> For example, add drm_bridge_debugfs_add(struct drm_encoder *encoder),
> >> which then contains the debugfs_create_file() call.  
> >
> > I think it should go in drm_encoder.c, not drm_bridge.c, right? Here we
> > are showing the bridges attached to an encoder, so the entry point is
> > each encoder.  
> 
> I'm still thinking drm_bridge.c, because it's about bridges and their
> details. The encoder shouldn't care about bridge implementation details.

Ah, I think I now get what you mean.

Current code is:

drm_encoder_register_all()                             [drm_encoder.c]
 -> drm_debugfs_encoder_add                            [drm_debugfs.c]
   -> debugfs_create_file("bridges"...  &bridges_fops) [drm_debugfs.c]
                                    [bridges_fops is in drm_debugfs.c]

Moving the last 2 lines to drm_bridge.c and into a new function we'd
have:

drm_encoder_register_all()                             [drm_encoder.c]
 -> drm_debugfs_encoder_add [*]                        [drm_debugfs.c]
  -> drm_bridge_debugfs_add_encoder_bridges_file (NEW) [drm_bridge.c]
   -> debugfs_create_file("bridges"...  &bridges_fops) [drm_bridge.c]
                                    [bridges_fops is in drm_bridge.c]

Potentially [*] could be moved to drm_encoder.c, but that is not bridge
related and can be done as a future step.

Is this what you had in mind?

> > On the other hand in patch 2 we should move the
> > drm_debugfs_global_add() code to drm_bridge.c, as it's showing bridges
> > ina encoder-independent way.  
> 
> Agreed on that.
> 
> > And finally drm_bridge should export the common
> > drm_bridge_debugfs_show_bridge() function to drm_encoder.c.  
> 
> Disagree. That will still require the EXPORT and #ifdefs around
> CONFIG_DEBUG_FS.

With the above-sketched idea I agree we wouldn't need to export
drm_bridge_debugfs_show_bridge().

Luca

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

Re: [PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Jani Nikula]

On Wed, 26 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> Hello Jani,
>
> On Tue, 25 Feb 2025 20:21:50 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>> > Hello Jani,
>> >
>> > On Tue, 25 Feb 2025 18:36:41 +0200
>> > Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> >  
>> >> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:  
>> >> > In preparation to expose more info about bridges in debugfs, which will
>> >> > require more insight into drm_bridge data structures, move the bridges_show
>> >> > code to drm_bridge.c.
>> >> >
>> >> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> >> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>    
>> >> 
>> >> I hate myself for doing this on a patch that's at v7... but here goes.  
>> >
>> > Please don't! :-) This patch is new in v7, and a different (and
>> > definitely worse) approach was present in v6, but there was nothing
>> > before.
>> >  
>> >> Perhaps consider moving the bridges debugfs creation and fops to
>> >> drm_bridge.c instead of just adding
>> >> drm_bridge_debugfs_show_encoder_bridges().
>> >> 
>> >> For example, add drm_bridge_debugfs_add(struct drm_encoder *encoder),
>> >> which then contains the debugfs_create_file() call.  
>> >
>> > I think it should go in drm_encoder.c, not drm_bridge.c, right? Here we
>> > are showing the bridges attached to an encoder, so the entry point is
>> > each encoder.  
>> 
>> I'm still thinking drm_bridge.c, because it's about bridges and their
>> details. The encoder shouldn't care about bridge implementation details.
>
> Ah, I think I now get what you mean.
>
> Current code is:
>
> drm_encoder_register_all()                             [drm_encoder.c]
>  -> drm_debugfs_encoder_add                            [drm_debugfs.c]
>    -> debugfs_create_file("bridges"...  &bridges_fops) [drm_debugfs.c]
>                                     [bridges_fops is in drm_debugfs.c]
>
> Moving the last 2 lines to drm_bridge.c and into a new function we'd
> have:
>
> drm_encoder_register_all()                             [drm_encoder.c]
>  -> drm_debugfs_encoder_add [*]                        [drm_debugfs.c]
>   -> drm_bridge_debugfs_add_encoder_bridges_file (NEW) [drm_bridge.c]
>    -> debugfs_create_file("bridges"...  &bridges_fops) [drm_bridge.c]
>                                     [bridges_fops is in drm_bridge.c]
>
> Potentially [*] could be moved to drm_encoder.c, but that is not bridge
> related and can be done as a future step.
>
> Is this what you had in mind?

Yes!

(Though I'd give drm_bridge_debugfs_add_encoder_bridges_file() a shorter
and more generic name.)

BR,
Jani.


>
>> > On the other hand in patch 2 we should move the
>> > drm_debugfs_global_add() code to drm_bridge.c, as it's showing bridges
>> > ina encoder-independent way.  
>> 
>> Agreed on that.
>> 
>> > And finally drm_bridge should export the common
>> > drm_bridge_debugfs_show_bridge() function to drm_encoder.c.  
>> 
>> Disagree. That will still require the EXPORT and #ifdefs around
>> CONFIG_DEBUG_FS.
>
> With the above-sketched idea I agree we wouldn't need to export
> drm_bridge_debugfs_show_bridge().
>
> Luca

-- 
Jani Nikula, Intel

Re: [PATCH v7 1/2] drm/bridge: move bridges_show logic from drm_debugfs.c

[Luca Ceresoli]

Hello Jani,

On Wed, 26 Feb 2025 14:26:09 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> > Moving the last 2 lines to drm_bridge.c and into a new function we'd
> > have:
> >
> > drm_encoder_register_all()                             [drm_encoder.c]  
> >  -> drm_debugfs_encoder_add [*]                        [drm_debugfs.c]
> >   -> drm_bridge_debugfs_add_encoder_bridges_file (NEW) [drm_bridge.c]
> >    -> debugfs_create_file("bridges"...  &bridges_fops) [drm_bridge.c]  
> >                                     [bridges_fops is in drm_bridge.c]
> >
> > Potentially [*] could be moved to drm_encoder.c, but that is not bridge
> > related and can be done as a future step.
> >
> > Is this what you had in mind?  
> 
> Yes!
> 
> (Though I'd give drm_bridge_debugfs_add_encoder_bridges_file() a shorter
> and more generic name.)

Thanks, done. v8 on its way.

Luca

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

[PATCH v7 2/2] drm/debugfs: add top-level 'bridges' file showing all added bridges

[Luca Ceresoli]

The global bridges_list holding all the bridges between drm_bridge_add()
and drm_bridge_remove() cannot be inspected via debugfs. Add a file showing
it.

To avoid code duplication, move the code printing a bridge info to a common
function.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v7:
- move implementation to drm_bridge.c to avoid exporting bridge_list and
  bridge_mutex

This patch was added in v6.
---
 drivers/gpu/drm/drm_bridge.c   | 64 +++++++++++++++++++++++++++---------------
 drivers/gpu/drm/drm_debugfs.c  | 15 ++++++++++
 drivers/gpu/drm/drm_drv.c      |  1 +
 drivers/gpu/drm/drm_internal.h |  5 ++++
 include/drm/drm_bridge.h       |  2 ++
 5 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 6e1e02c1cf443fbaa764765ad369b7a34cc03f2d..c840b020cd1c9a39212ad90cca54aa93de2475cd 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1336,36 +1336,56 @@ EXPORT_SYMBOL(of_drm_find_bridge);
 #endif
 
 #ifdef CONFIG_DEBUG_FS
+static void drm_bridge_debugfs_show_bridge(struct drm_printer *p,
+					   struct drm_bridge *bridge,
+					   unsigned int idx)
+{
+	drm_printf(p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
+	drm_printf(p, "\ttype: [%d] %s\n",
+		   bridge->type,
+		   drm_get_connector_type_name(bridge->type));
+
+	if (bridge->of_node)
+		drm_printf(p, "\tOF: %pOFfc\n", bridge->of_node);
+
+	drm_printf(p, "\tops: [0x%x]", bridge->ops);
+	if (bridge->ops & DRM_BRIDGE_OP_DETECT)
+		drm_puts(p, " detect");
+	if (bridge->ops & DRM_BRIDGE_OP_EDID)
+		drm_puts(p, " edid");
+	if (bridge->ops & DRM_BRIDGE_OP_HPD)
+		drm_puts(p, " hpd");
+	if (bridge->ops & DRM_BRIDGE_OP_MODES)
+		drm_puts(p, " modes");
+	if (bridge->ops & DRM_BRIDGE_OP_HDMI)
+		drm_puts(p, " hdmi");
+	drm_puts(p, "\n");
+}
+
 void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
 					     struct drm_encoder *encoder)
 {
 	struct drm_bridge *bridge;
 	unsigned int idx = 0;
 
-	drm_for_each_bridge_in_chain(encoder, bridge) {
-		drm_printf(p, "bridge[%u]: %ps\n", idx++, bridge->funcs);
-		drm_printf(p, "\ttype: [%d] %s\n",
-			   bridge->type,
-			   drm_get_connector_type_name(bridge->type));
-
-		if (bridge->of_node)
-			drm_printf(p, "\tOF: %pOFfc\n", bridge->of_node);
-
-		drm_printf(p, "\tops: [0x%x]", bridge->ops);
-		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
-			drm_puts(p, " detect");
-		if (bridge->ops & DRM_BRIDGE_OP_EDID)
-			drm_puts(p, " edid");
-		if (bridge->ops & DRM_BRIDGE_OP_HPD)
-			drm_puts(p, " hpd");
-		if (bridge->ops & DRM_BRIDGE_OP_MODES)
-			drm_puts(p, " modes");
-		if (bridge->ops & DRM_BRIDGE_OP_HDMI)
-			drm_puts(p, " hdmi");
-		drm_puts(p, "\n");
-	}
+	drm_for_each_bridge_in_chain(encoder, bridge)
+		drm_bridge_debugfs_show_bridge(p, bridge, idx++);
 }
 EXPORT_SYMBOL(drm_bridge_debugfs_show_encoder_bridges);
+
+void drm_bridge_debugfs_show_all_bridges(struct drm_printer *p)
+{
+	struct drm_bridge *bridge;
+	unsigned int idx = 0;
+
+	mutex_lock(&bridge_lock);
+
+	list_for_each_entry(bridge, &bridge_list, list)
+		drm_bridge_debugfs_show_bridge(p, bridge, idx++);
+
+	mutex_unlock(&bridge_lock);
+}
+EXPORT_SYMBOL(drm_bridge_debugfs_show_all_bridges);
 #endif
 
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index a625c0c9a9aa076af19c8ba47564dbb24ba71c9a..74aa38ca96d23d1e7f9862ad092e72c106053fe7 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -779,3 +779,18 @@ void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
 	debugfs_remove_recursive(encoder->debugfs_entry);
 	encoder->debugfs_entry = NULL;
 }
+
+static int allbridges_show(struct seq_file *m, void *data)
+{
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	drm_bridge_debugfs_show_all_bridges(&p);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(allbridges);
+
+void drm_debugfs_global_add(struct dentry *root)
+{
+	debugfs_create_file("bridges", 0444, root, NULL, &allbridges_fops);
+}
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3cf440eee8a2ab3de134d925db8f1d2ce68062b7..9b6d7bd16ba409b6a9155a9fecbec6bfdd5ea0c2 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1120,6 +1120,7 @@ static int __init drm_core_init(void)
 	}
 
 	drm_debugfs_root = debugfs_create_dir("dri", NULL);
+	drm_debugfs_global_add(drm_debugfs_root);
 
 	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index b2b6a8e49dda46f1cb3b048ef7b28356dd3aaa4e..4c9c56b68ec1149a2def8cdcbdeda4946dbffb10 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -196,6 +196,7 @@ void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
 void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
 void drm_debugfs_encoder_add(struct drm_encoder *encoder);
 void drm_debugfs_encoder_remove(struct drm_encoder *encoder);
+void drm_debugfs_global_add(struct dentry *drm_debugfs_root);
 #else
 static inline void drm_debugfs_dev_fini(struct drm_device *dev)
 {
@@ -241,6 +242,10 @@ static inline void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
 {
 }
 
+static inline void drm_debugfs_global_add(struct dentry *drm_debugfs_root)
+{
+}
+
 #endif
 
 drm_ioctl_t drm_version;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 23c062853e7bb42d8d73af35ef7f16fb37345a37..9dd2654d1cd7212fb798432110776e5818fcb5a0 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1111,9 +1111,11 @@ static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
 #ifdef CONFIG_DEBUG_FS
 void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
 					     struct drm_encoder *encoder);
+void drm_bridge_debugfs_show_all_bridges(struct drm_printer *p);
 #else
 static inline void drm_bridge_debugfs_show_encoder_bridges(struct drm_printer *p,
 							   struct drm_encoder *encoder) {}
+static inline void drm_bridge_debugfs_show_all_bridges(struct drm_printer *p) {}
 #endif
 
 #endif

-- 
2.48.1