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

[Tvrtko Ursulin]

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..).

Regards,

Tvrtko