[Intel-gfx] [PATCH 11/20] drm/i915/fbc: Move FBC debugfs stuff into intel_fbc.c

[Ville Syrjälä]

On Thu, Nov 25, 2021 at 04:27:18PM +0200, Jani Nikula wrote:
> On Thu, 25 Nov 2021, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
> > 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 at linux.intel.com> wrote:
> >>>> On Wed, Nov 24, 2021 at 05:43:52PM +0200, Jani Nikula wrote:
> >>>>> On Wed, 24 Nov 2021, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> >>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>>>>>
> >>>>>> 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..).
> 
> Frankly, I really dislike the "include code" part about selftests...

We seem to have gone a bit off track in the discussion here. There
is no plan to do any kind of "include code" or anything here. All
I want to do is put the debugfs stuff into the same file as the
real implementation so that a) no implementation details need to
leak outside, b) the code gets optimized away when debufs is
disabled resulting in a smaller binary. Though I don't know if
anyone seriously compiles w/o debugfs anyway.

I guess another benefit is that it's harder to forget to
update the debugfs code when making changes to the rest of the
implementation. I've lost count how many times I've forgeotten
to do that with the debugfs code living in a totally separate
file.

-- 
Ville Syrjälä
Intel