| Message ID | 20250225-drm-debugfs-show-all-bridges-v7-1-8826037ada37@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: show "all" bridges in debugfs | expand |
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
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
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.
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
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
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
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
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(-)