On 25/11/2021 12:13, Ville Syrjälä wrote:
On Thu, Nov 25, 2021 at 12:57:27PM +0200, Jani Nikula wrote:
On Thu, 25 Nov 2021, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Wed, Nov 24, 2021 at 05:43:52PM +0200, Jani Nikula wrote:
On Wed, 24 Nov 2021, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
In order to encapsulate FBC harder let's just move the debugfs
stuff into intel_fbc.c.
Mmmh, I've kind of moved towards a split where i915_debugfs.c and
intel_display_debugfs.c have all the debugfs boilerplate, while the
implementation files have the guts with struct drm_i915_private *i915
(or something more specific) and struct seq_file *m passed in.
In some ways the split is arbitrary, but I kind of find the debugfs
boilerplate a distraction in the implementation files, and we also skip
building the debugfs files completely for CONFIG_DEBUG_FS=n. I don't
think I'd want to add #ifdefs on that spread around either.
If we want to keep the debugfs in a separate file then we'll have to
expose the guts of the FBC implementation in intel_fbc.h (or some other
header) just for that, or we add a whole bunch of otherwise useless
functions that pretend to provide some higher level of abstraction.
Not really a fan of either of those options.
Obviously I'm in favour of hiding the guts, no question about it. I'm
also very much in favour of moving the details out of our *debugfs.c
files. It's just a question of where to draw the line, and which side of
the line the debugfs boilerplate lands.
Which leaves us either your approach in the patch at hand, or adding the
fbc helper functions for debugfs, which would be something like:
intel_fbc_get_status
intel_fbc_get_false_color
intel_fbc_set_false_color
So I guess you're suggesting that just the DEFINE_ATTRIBUTE
and debugfs_create_file() stuff should remain in
intel_display_debugfs.c?
Not sure that approach has any benefits whatsoever. The get/set
functions will need to be non-static and they'll get included in
the binary whether or not debugfs is enabled or not (unless you
lto it perhaps). If everything is in intel_fbc.c all that stuff
just gets optimized out entirely when not needed.
Also then I couldn't do this sort of stuff:
if (fbc->funcs->set_false_color)
debugfs_create_file(...)
because that requires knowledge only available to intel_fbc.c.
I'd need to add some kind of intel_fbc_has_false_color() thing
just for that.
Not guaranteeing I captured all the nuances here but how about an
approach similar to selftests? That is, have a separate file for debugfs
registration and bits (each "module" explicitly registers as in Ville's
patch), and have the owning "module" include the debugfs part at the end
of it. That way no exports, or defining too much API, would be needed.
And not needing common debugfs code to know the guts of any module.
Benefit of not compiling any of it when !CONFIG_DEBUG_FS is kept (or
gained, not even sure any more..).
Regards,
Tvrtko
