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

[Jani Nikula]

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

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center