diff mbox series

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

Message ID 20211124113652.22090-12-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/fbc: More FBC refactoring | expand

Commit Message

Ville Syrjälä Nov. 24, 2021, 11:36 a.m. UTC 
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.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_display_debugfs.c  |  50 +-------
 drivers/gpu/drm/i915/display/intel_fbc.c      | 110 +++++++++++++-----
 drivers/gpu/drm/i915/display/intel_fbc.h      |   4 +-
 3 files changed, 82 insertions(+), 82 deletions(-)

Comments

Jani Nikula Nov. 24, 2021, 3:43 p.m. UTC | #1 
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.


BR,
Jani.



>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../drm/i915/display/intel_display_debugfs.c  |  50 +-------
>  drivers/gpu/drm/i915/display/intel_fbc.c      | 110 +++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_fbc.h      |   4 +-
>  3 files changed, 82 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 3e456e595010..572445299b04 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -40,52 +40,6 @@ static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> -static int i915_fbc_status(struct seq_file *m, void *unused)
> -{
> -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	intel_wakeref_t wakeref;
> -
> -	if (!HAS_FBC(dev_priv))
> -		return -ENODEV;
> -
> -	wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> -	mutex_lock(&fbc->lock);
> -
> -	if (intel_fbc_is_active(fbc)) {
> -		seq_puts(m, "FBC enabled\n");
> -		seq_printf(m, "Compressing: %s\n",
> -			   yesno(intel_fbc_is_compressing(fbc)));
> -	} else {
> -		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
> -	}
> -
> -	mutex_unlock(&fbc->lock);
> -	intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
> -
> -	return 0;
> -}
> -
> -static int i915_fbc_false_color_get(void *data, u64 *val)
> -{
> -	struct drm_i915_private *dev_priv = data;
> -
> -	*val = dev_priv->fbc.false_color;
> -
> -	return 0;
> -}
> -
> -static int i915_fbc_false_color_set(void *data, u64 val)
> -{
> -	struct drm_i915_private *dev_priv = data;
> -
> -	return intel_fbc_set_false_color(&dev_priv->fbc, val);
> -}
> -
> -DEFINE_SIMPLE_ATTRIBUTE(i915_fbc_false_color_fops,
> -			i915_fbc_false_color_get, i915_fbc_false_color_set,
> -			"%llu\n");
> -
>  static int i915_ips_status(struct seq_file *m, void *unused)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -2058,7 +2012,6 @@ static const struct file_operations i915_fifo_underrun_reset_ops = {
>  
>  static const struct drm_info_list intel_display_debugfs_list[] = {
>  	{"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
> -	{"i915_fbc_status", i915_fbc_status, 0},
>  	{"i915_ips_status", i915_ips_status, 0},
>  	{"i915_sr_status", i915_sr_status, 0},
>  	{"i915_opregion", i915_opregion, 0},
> @@ -2083,7 +2036,6 @@ static const struct {
>  	{"i915_pri_wm_latency", &i915_pri_wm_latency_fops},
>  	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
>  	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
> -	{"i915_fbc_false_color", &i915_fbc_false_color_fops},
>  	{"i915_dp_test_data", &i915_displayport_test_data_fops},
>  	{"i915_dp_test_type", &i915_displayport_test_type_fops},
>  	{"i915_dp_test_active", &i915_displayport_test_active_fops},
> @@ -2110,6 +2062,8 @@ void intel_display_debugfs_register(struct drm_i915_private *i915)
>  	drm_debugfs_create_files(intel_display_debugfs_list,
>  				 ARRAY_SIZE(intel_display_debugfs_list),
>  				 minor->debugfs_root, minor);
> +
> +	intel_fbc_debugfs_register(i915);
>  }
>  
>  static int i915_panel_show(struct seq_file *m, void *data)
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 00c93040529e..ee4e3186cc9c 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -600,7 +600,7 @@ static void intel_fbc_hw_deactivate(struct intel_fbc *fbc)
>  	fbc->funcs->deactivate(fbc);
>  }
>  
> -bool intel_fbc_is_compressing(struct intel_fbc *fbc)
> +static bool intel_fbc_is_compressing(struct intel_fbc *fbc)
>  {
>  	return fbc->funcs->is_compressing(fbc);
>  }
> @@ -612,36 +612,6 @@ static void intel_fbc_nuke(struct intel_fbc *fbc)
>  	fbc->funcs->nuke(fbc);
>  }
>  
> -int intel_fbc_set_false_color(struct intel_fbc *fbc, bool enable)
> -{
> -	if (!fbc->funcs || !fbc->funcs->set_false_color)
> -		return -ENODEV;
> -
> -	mutex_lock(&fbc->lock);
> -
> -	fbc->false_color = enable;
> -
> -	fbc->funcs->set_false_color(fbc, enable);
> -
> -	mutex_unlock(&fbc->lock);
> -
> -	return 0;
> -}
> -
> -/**
> - * intel_fbc_is_active - Is FBC active?
> - * @fbc: The FBC instance
> - *
> - * This function is used to verify the current state of FBC.
> - *
> - * FIXME: This should be tracked in the plane config eventually
> - * instead of queried at runtime for most callers.
> - */
> -bool intel_fbc_is_active(struct intel_fbc *fbc)
> -{
> -	return fbc->active;
> -}
> -
>  static void intel_fbc_activate(struct intel_fbc *fbc)
>  {
>  	intel_fbc_hw_activate(fbc);
> @@ -1691,3 +1661,81 @@ void intel_fbc_init(struct drm_i915_private *i915)
>  	if (intel_fbc_hw_is_active(fbc))
>  		intel_fbc_hw_deactivate(fbc);
>  }
> +
> +static int intel_fbc_debugfs_status_show(struct seq_file *m, void *unused)
> +{
> +	struct intel_fbc *fbc = m->private;
> +	struct drm_i915_private *i915 = fbc->i915;
> +	intel_wakeref_t wakeref;
> +
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +	mutex_lock(&fbc->lock);
> +
> +	if (fbc->active) {
> +		seq_puts(m, "FBC enabled\n");
> +		seq_printf(m, "Compressing: %s\n",
> +			   yesno(intel_fbc_is_compressing(fbc)));
> +	} else {
> +		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
> +	}
> +
> +	mutex_unlock(&fbc->lock);
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(intel_fbc_debugfs_status);
> +
> +static int intel_fbc_debugfs_false_color_get(void *data, u64 *val)
> +{
> +	struct intel_fbc *fbc = data;
> +
> +	*val = fbc->false_color;
> +
> +	return 0;
> +}
> +
> +static int intel_fbc_debugfs_false_color_set(void *data, u64 val)
> +{
> +	struct intel_fbc *fbc = data;
> +
> +	mutex_lock(&fbc->lock);
> +
> +	fbc->false_color = val;
> +
> +	if (fbc->active)
> +		fbc->funcs->set_false_color(fbc, fbc->false_color);
> +
> +	mutex_unlock(&fbc->lock);
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops,
> +			intel_fbc_debugfs_false_color_get,
> +			intel_fbc_debugfs_false_color_set,
> +			"%llu\n");
> +
> +static void intel_fbc_debugfs_add(struct intel_fbc *fbc)
> +{
> +	struct drm_i915_private *i915 = fbc->i915;
> +	struct drm_minor *minor = i915->drm.primary;
> +
> +	debugfs_create_file("i915_fbc_status", 0444,
> +			    minor->debugfs_root, fbc,
> +			    &intel_fbc_debugfs_status_fops);
> +
> +	if (fbc->funcs->set_false_color)
> +		debugfs_create_file("i915_fbc_false_color", 0644,
> +				    minor->debugfs_root, fbc,
> +				    &intel_fbc_debugfs_false_color_fops);
> +}
> +
> +void intel_fbc_debugfs_register(struct drm_i915_private *i915)
> +{
> +	struct intel_fbc *fbc = &i915->fbc;
> +
> +	if (HAS_FBC(i915))
> +		intel_fbc_debugfs_add(fbc);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index 36e9e5f93bcb..0f5884f1e095 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -18,8 +18,6 @@ struct intel_fbc;
>  struct intel_plane_state;
>  
>  int intel_fbc_atomic_check(struct intel_atomic_state *state);
> -bool intel_fbc_is_active(struct intel_fbc *fbc);
> -bool intel_fbc_is_compressing(struct intel_fbc *fbc);
>  bool intel_fbc_pre_update(struct intel_atomic_state *state,
>  			  struct intel_crtc *crtc);
>  void intel_fbc_post_update(struct intel_atomic_state *state,
> @@ -37,6 +35,6 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
>  void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *i915);
>  void intel_fbc_reset_underrun(struct drm_i915_private *i915);
> -int intel_fbc_set_false_color(struct intel_fbc *fbc, bool enable);
> +void intel_fbc_debugfs_register(struct drm_i915_private *i915);
>  
>  #endif /* __INTEL_FBC_H__ */
Ville Syrjälä Nov. 25, 2021, 9:43 a.m. UTC | #2 
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.
Jani Nikula Nov. 25, 2021, 10:57 a.m. UTC | #3 
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

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.
Ville Syrjälä Nov. 25, 2021, 12:13 p.m. UTC | #4 
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.
Tvrtko Ursulin Nov. 25, 2021, 2:06 p.m. UTC | #5 
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..).

Regards,

Tvrtko
Jani Nikula Nov. 25, 2021, 2:27 p.m. UTC | #6 
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...

BR,
Jani.
Ville Syrjälä Dec. 3, 2021, 9:13 a.m. UTC | #7 
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.
Jani Nikula Dec. 3, 2021, 9:55 a.m. UTC | #8 
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?

If that's the case, my objection to adding #ifdefs just goes away.

BR,
Jani.
Ville Syrjälä Dec. 3, 2021, 10:06 a.m. UTC | #9 
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().
Jani Nikula Dec. 3, 2021, 10:47 a.m. UTC | #10 
On Fri, 03 Dec 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Dec 03, 2021 at 11:55:43AM +0200, Jani Nikula wrote:
>> 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().

\o/
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 3e456e595010..572445299b04 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -40,52 +40,6 @@  static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
 	return 0;
 }
 
-static int i915_fbc_status(struct seq_file *m, void *unused)
-{
-	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct intel_fbc *fbc = &dev_priv->fbc;
-	intel_wakeref_t wakeref;
-
-	if (!HAS_FBC(dev_priv))
-		return -ENODEV;
-
-	wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
-	mutex_lock(&fbc->lock);
-
-	if (intel_fbc_is_active(fbc)) {
-		seq_puts(m, "FBC enabled\n");
-		seq_printf(m, "Compressing: %s\n",
-			   yesno(intel_fbc_is_compressing(fbc)));
-	} else {
-		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
-	}
-
-	mutex_unlock(&fbc->lock);
-	intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
-
-	return 0;
-}
-
-static int i915_fbc_false_color_get(void *data, u64 *val)
-{
-	struct drm_i915_private *dev_priv = data;
-
-	*val = dev_priv->fbc.false_color;
-
-	return 0;
-}
-
-static int i915_fbc_false_color_set(void *data, u64 val)
-{
-	struct drm_i915_private *dev_priv = data;
-
-	return intel_fbc_set_false_color(&dev_priv->fbc, val);
-}
-
-DEFINE_SIMPLE_ATTRIBUTE(i915_fbc_false_color_fops,
-			i915_fbc_false_color_get, i915_fbc_false_color_set,
-			"%llu\n");
-
 static int i915_ips_status(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -2058,7 +2012,6 @@  static const struct file_operations i915_fifo_underrun_reset_ops = {
 
 static const struct drm_info_list intel_display_debugfs_list[] = {
 	{"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
-	{"i915_fbc_status", i915_fbc_status, 0},
 	{"i915_ips_status", i915_ips_status, 0},
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
@@ -2083,7 +2036,6 @@  static const struct {
 	{"i915_pri_wm_latency", &i915_pri_wm_latency_fops},
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
-	{"i915_fbc_false_color", &i915_fbc_false_color_fops},
 	{"i915_dp_test_data", &i915_displayport_test_data_fops},
 	{"i915_dp_test_type", &i915_displayport_test_type_fops},
 	{"i915_dp_test_active", &i915_displayport_test_active_fops},
@@ -2110,6 +2062,8 @@  void intel_display_debugfs_register(struct drm_i915_private *i915)
 	drm_debugfs_create_files(intel_display_debugfs_list,
 				 ARRAY_SIZE(intel_display_debugfs_list),
 				 minor->debugfs_root, minor);
+
+	intel_fbc_debugfs_register(i915);
 }
 
 static int i915_panel_show(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 00c93040529e..ee4e3186cc9c 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -600,7 +600,7 @@  static void intel_fbc_hw_deactivate(struct intel_fbc *fbc)
 	fbc->funcs->deactivate(fbc);
 }
 
-bool intel_fbc_is_compressing(struct intel_fbc *fbc)
+static bool intel_fbc_is_compressing(struct intel_fbc *fbc)
 {
 	return fbc->funcs->is_compressing(fbc);
 }
@@ -612,36 +612,6 @@  static void intel_fbc_nuke(struct intel_fbc *fbc)
 	fbc->funcs->nuke(fbc);
 }
 
-int intel_fbc_set_false_color(struct intel_fbc *fbc, bool enable)
-{
-	if (!fbc->funcs || !fbc->funcs->set_false_color)
-		return -ENODEV;
-
-	mutex_lock(&fbc->lock);
-
-	fbc->false_color = enable;
-
-	fbc->funcs->set_false_color(fbc, enable);
-
-	mutex_unlock(&fbc->lock);
-
-	return 0;
-}
-
-/**
- * intel_fbc_is_active - Is FBC active?
- * @fbc: The FBC instance
- *
- * This function is used to verify the current state of FBC.
- *
- * FIXME: This should be tracked in the plane config eventually
- * instead of queried at runtime for most callers.
- */
-bool intel_fbc_is_active(struct intel_fbc *fbc)
-{
-	return fbc->active;
-}
-
 static void intel_fbc_activate(struct intel_fbc *fbc)
 {
 	intel_fbc_hw_activate(fbc);
@@ -1691,3 +1661,81 @@  void intel_fbc_init(struct drm_i915_private *i915)
 	if (intel_fbc_hw_is_active(fbc))
 		intel_fbc_hw_deactivate(fbc);
 }
+
+static int intel_fbc_debugfs_status_show(struct seq_file *m, void *unused)
+{
+	struct intel_fbc *fbc = m->private;
+	struct drm_i915_private *i915 = fbc->i915;
+	intel_wakeref_t wakeref;
+
+	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
+	mutex_lock(&fbc->lock);
+
+	if (fbc->active) {
+		seq_puts(m, "FBC enabled\n");
+		seq_printf(m, "Compressing: %s\n",
+			   yesno(intel_fbc_is_compressing(fbc)));
+	} else {
+		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
+	}
+
+	mutex_unlock(&fbc->lock);
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(intel_fbc_debugfs_status);
+
+static int intel_fbc_debugfs_false_color_get(void *data, u64 *val)
+{
+	struct intel_fbc *fbc = data;
+
+	*val = fbc->false_color;
+
+	return 0;
+}
+
+static int intel_fbc_debugfs_false_color_set(void *data, u64 val)
+{
+	struct intel_fbc *fbc = data;
+
+	mutex_lock(&fbc->lock);
+
+	fbc->false_color = val;
+
+	if (fbc->active)
+		fbc->funcs->set_false_color(fbc, fbc->false_color);
+
+	mutex_unlock(&fbc->lock);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops,
+			intel_fbc_debugfs_false_color_get,
+			intel_fbc_debugfs_false_color_set,
+			"%llu\n");
+
+static void intel_fbc_debugfs_add(struct intel_fbc *fbc)
+{
+	struct drm_i915_private *i915 = fbc->i915;
+	struct drm_minor *minor = i915->drm.primary;
+
+	debugfs_create_file("i915_fbc_status", 0444,
+			    minor->debugfs_root, fbc,
+			    &intel_fbc_debugfs_status_fops);
+
+	if (fbc->funcs->set_false_color)
+		debugfs_create_file("i915_fbc_false_color", 0644,
+				    minor->debugfs_root, fbc,
+				    &intel_fbc_debugfs_false_color_fops);
+}
+
+void intel_fbc_debugfs_register(struct drm_i915_private *i915)
+{
+	struct intel_fbc *fbc = &i915->fbc;
+
+	if (HAS_FBC(i915))
+		intel_fbc_debugfs_add(fbc);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
index 36e9e5f93bcb..0f5884f1e095 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.h
+++ b/drivers/gpu/drm/i915/display/intel_fbc.h
@@ -18,8 +18,6 @@  struct intel_fbc;
 struct intel_plane_state;
 
 int intel_fbc_atomic_check(struct intel_atomic_state *state);
-bool intel_fbc_is_active(struct intel_fbc *fbc);
-bool intel_fbc_is_compressing(struct intel_fbc *fbc);
 bool intel_fbc_pre_update(struct intel_atomic_state *state,
 			  struct intel_crtc *crtc);
 void intel_fbc_post_update(struct intel_atomic_state *state,
@@ -37,6 +35,6 @@  void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
 void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *i915);
 void intel_fbc_reset_underrun(struct drm_i915_private *i915);
-int intel_fbc_set_false_color(struct intel_fbc *fbc, bool enable);
+void intel_fbc_debugfs_register(struct drm_i915_private *i915);
 
 #endif /* __INTEL_FBC_H__ */