On Thu, 25 Nov 2021, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@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..). Frankly, I really dislike the "include code" part about selftests... BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
