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

[Jani Nikula <jani.nikula@linux.intel.com>]

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