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 Fri, Dec 03, 2021 at 11:55:43AM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Nov 25, 2021 at 04:27:18PM +0200, Jani Nikula wrote:
> >> On Thu, 25 Nov 2021, Tvrtko Ursulin <tvrtko.ursulin@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@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.
> >> >
> >> > 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.
> 
> Yeah, let's un-stall this.
> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> 
> on the change here, better abstractions and smaller interfaces being the
> main rationale for it.
> 
> I think an insteresting question is, with all the debugfs stuff being
> static in intel_fbc.c, is the compiler actually smart enough to optimize
> the static code and data away when CONFIG_DEBUG_FS=n, even without
> #ifdefs? Or is that something you're already claiming above?

Yes it all disappeared from the binary when I tried it.
Only thing left was an empty intel_fbc_debugfs_register().

-- 
Ville Syrjälä
Intel