On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote: > Hello Jani, > > On Tue, 25 Feb 2025 18:36:41 +0200 > Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > >> On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> 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@xxxxxxxxxx> >> > Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> >> >> 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
