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 >From a technical standpoint, I can appreciate the pros and cons of both approaches. And I could be persuaded either way. With the maintainer hat on, I'm considering the precedent this sets, and where things start flowing after that. Do we accept all options for other files, case by case, making this a continuous bikeshedding topic, or do we want to choose an approach and start driving it everywhere? Frankly I'm not all that fond of giving people a lot of options on stuff like this, I prefer saying this is how we roll, let's stick to it. (Until we decide otherwise, and stick to something else! ;) Sooo... what would it be like if every file had their own intel_foo_debugfs_register()? Including connector specific stuff such as "i915_psr_status". Cc'd other maintainers for good measure. Let's have one debugfs bikeshedding now, and stick to it, instead of every time this pops up? BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
