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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 25, 2021 at 12:57:27PM +0200, Jani Nikula wrote:
> On Thu, 25 Nov 2021, Ville Syrjälä <ville.syrjala@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@linux.intel.com> wrote:
> >> > From: Ville Syrjälä <ville.syrjala@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.

-- 
Ville Syrjälä
Intel