Re: [Intel-gfx] [PATCH] drm/i915/debugfs: move IPS debugfs to hsw_ips.c
On Thu, 02 Mar 2023, Ville Syrjälä wrote:
> On Thu, Mar 02, 2023 at 06:16:17PM +0200, Jani Nikula wrote:
>> Follow the style of placing debugfs next to the implementation.
>>
>> Signed-off-by: Jani Nikula
>
> Reviewed-by: Ville Syrjälä
Thanks!
> Some ideas for future enhancements:
> - only register when IPS is actually supported
> - make it per-crtc since IPS only exists on pipe A
> - report the software state of IPS enable vs. disable
> - there's a false color bit just like with fbc, could
> hook that up to help verify that it actually works
Yeah, I considered that, but since
#define HAS_IPS(dev_priv) (IS_HSW_ULT(dev_priv) || IS_BROADWELL(dev_priv))
it's not really high up on the list of things to do... indeed I wanted
to stow this away in hsw_ips.[ch] because of that.
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/i915/display/hsw_ips.c| 37 +++
>> drivers/gpu/drm/i915/display/hsw_ips.h| 2 +
>> .../drm/i915/display/intel_display_debugfs.c | 30 +--
>> 3 files changed, 41 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/hsw_ips.c
>> b/drivers/gpu/drm/i915/display/hsw_ips.c
>> index 83aa3800245f..2910f5d0f3e2 100644
>> --- a/drivers/gpu/drm/i915/display/hsw_ips.c
>> +++ b/drivers/gpu/drm/i915/display/hsw_ips.c
>> @@ -267,3 +267,40 @@ void hsw_ips_get_config(struct intel_crtc_state
>> *crtc_state)
>> crtc_state->ips_enabled = true;
>> }
>> }
>> +
>> +static int hsw_ips_debugfs_status_show(struct seq_file *m, void *unused)
>> +{
>> +struct drm_i915_private *i915 = m->private;
>> +intel_wakeref_t wakeref;
>> +
>> +if (!HAS_IPS(i915))
>> +return -ENODEV;
>> +
>> +wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>> +
>> +seq_printf(m, "Enabled by kernel parameter: %s\n",
>> + str_yes_no(i915->params.enable_ips));
>> +
>> +if (DISPLAY_VER(i915) >= 8) {
>> +seq_puts(m, "Currently: unknown\n");
>> +} else {
>> +if (intel_de_read(i915, IPS_CTL) & IPS_ENABLE)
>> +seq_puts(m, "Currently: enabled\n");
>> +else
>> +seq_puts(m, "Currently: disabled\n");
>> +}
>> +
>> +intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>> +
>> +return 0;
>> +}
>> +
>> +DEFINE_SHOW_ATTRIBUTE(hsw_ips_debugfs_status);
>> +
>> +void hsw_ips_debugfs_register(struct drm_i915_private *i915)
>> +{
>> +struct drm_minor *minor = i915->drm.primary;
>> +
>> +debugfs_create_file("i915_ips_status", 0444, minor->debugfs_root,
>> +i915, &hsw_ips_debugfs_status_fops);
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/hsw_ips.h
>> b/drivers/gpu/drm/i915/display/hsw_ips.h
>> index 4564dee497d7..7ed6061874f7 100644
>> --- a/drivers/gpu/drm/i915/display/hsw_ips.h
>> +++ b/drivers/gpu/drm/i915/display/hsw_ips.h
>> @@ -8,6 +8,7 @@
>>
>> #include
>>
>> +struct drm_i915_private;
>> struct intel_atomic_state;
>> struct intel_crtc;
>> struct intel_crtc_state;
>> @@ -22,5 +23,6 @@ bool hsw_crtc_state_ips_capable(const struct
>> intel_crtc_state *crtc_state);
>> int hsw_ips_compute_config(struct intel_atomic_state *state,
>> struct intel_crtc *crtc);
>> void hsw_ips_get_config(struct intel_crtc_state *crtc_state);
>> +void hsw_ips_debugfs_register(struct drm_i915_private *i915);
>>
>> #endif /* __HSW_IPS_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> index 25013f303c82..8be606bfd2b4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> @@ -8,6 +8,7 @@
>> #include
>> #include
>>
>> +#include "hsw_ips.h"
>> #include "i915_debugfs.h"
>> #include "i915_irq.h"
>> #include "i915_reg.h"
>> @@ -49,33 +50,6 @@ static int i915_frontbuffer_tracking(struct seq_file *m,
>> void *unused)
>> return 0;
>> }
>>
>> -static int i915_ips_status(struct seq_file *m, void *unused)
>> -{
>> -struct drm_i915_private *dev_priv = node_to_i915(m->private);
>> -intel_wakeref_t wakeref;
>> -
>> -if (!HAS_IPS(dev_priv))
>> -return -ENODEV;
>> -
>> -wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
>> -
>> -seq_printf(m, "Enabled by kernel parameter: %s\n",
>> - str_yes_no(dev_priv->params.enable_ips));
>> -
>> -if (DISPLAY_VER(dev_priv) >= 8) {
>> -seq_puts(m, "Currently: unknown\n");
>> -} else {
>> -if (intel_de_read(dev_priv, IPS_CTL) & IPS_ENABLE)
>> -seq_puts(m, "Currently: enabled\n");
>> -else
>> -seq_puts(m, "Currently: disabled\n");
>> -}
>> -
>> -intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
>> -
>> -return 0;
>> -}
>> -
>> static int i915_sr_status(struct seq_file *m, void *unused)
>> {
>> struct drm_i915_private *dev_pr
Re: [Intel-gfx] [PATCH] drm/i915/debugfs: move IPS debugfs to hsw_ips.c
On Thu, Mar 02, 2023 at 06:16:17PM +0200, Jani Nikula wrote:
> Follow the style of placing debugfs next to the implementation.
>
> Signed-off-by: Jani Nikula
Reviewed-by: Ville Syrjälä
Some ideas for future enhancements:
- only register when IPS is actually supported
- make it per-crtc since IPS only exists on pipe A
- report the software state of IPS enable vs. disable
- there's a false color bit just like with fbc, could
hook that up to help verify that it actually works
> ---
> drivers/gpu/drm/i915/display/hsw_ips.c| 37 +++
> drivers/gpu/drm/i915/display/hsw_ips.h| 2 +
> .../drm/i915/display/intel_display_debugfs.c | 30 +--
> 3 files changed, 41 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/hsw_ips.c
> b/drivers/gpu/drm/i915/display/hsw_ips.c
> index 83aa3800245f..2910f5d0f3e2 100644
> --- a/drivers/gpu/drm/i915/display/hsw_ips.c
> +++ b/drivers/gpu/drm/i915/display/hsw_ips.c
> @@ -267,3 +267,40 @@ void hsw_ips_get_config(struct intel_crtc_state
> *crtc_state)
> crtc_state->ips_enabled = true;
> }
> }
> +
> +static int hsw_ips_debugfs_status_show(struct seq_file *m, void *unused)
> +{
> + struct drm_i915_private *i915 = m->private;
> + intel_wakeref_t wakeref;
> +
> + if (!HAS_IPS(i915))
> + return -ENODEV;
> +
> + wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> + seq_printf(m, "Enabled by kernel parameter: %s\n",
> +str_yes_no(i915->params.enable_ips));
> +
> + if (DISPLAY_VER(i915) >= 8) {
> + seq_puts(m, "Currently: unknown\n");
> + } else {
> + if (intel_de_read(i915, IPS_CTL) & IPS_ENABLE)
> + seq_puts(m, "Currently: enabled\n");
> + else
> + seq_puts(m, "Currently: disabled\n");
> + }
> +
> + intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(hsw_ips_debugfs_status);
> +
> +void hsw_ips_debugfs_register(struct drm_i915_private *i915)
> +{
> + struct drm_minor *minor = i915->drm.primary;
> +
> + debugfs_create_file("i915_ips_status", 0444, minor->debugfs_root,
> + i915, &hsw_ips_debugfs_status_fops);
> +}
> diff --git a/drivers/gpu/drm/i915/display/hsw_ips.h
> b/drivers/gpu/drm/i915/display/hsw_ips.h
> index 4564dee497d7..7ed6061874f7 100644
> --- a/drivers/gpu/drm/i915/display/hsw_ips.h
> +++ b/drivers/gpu/drm/i915/display/hsw_ips.h
> @@ -8,6 +8,7 @@
>
> #include
>
> +struct drm_i915_private;
> struct intel_atomic_state;
> struct intel_crtc;
> struct intel_crtc_state;
> @@ -22,5 +23,6 @@ bool hsw_crtc_state_ips_capable(const struct
> intel_crtc_state *crtc_state);
> int hsw_ips_compute_config(struct intel_atomic_state *state,
> struct intel_crtc *crtc);
> void hsw_ips_get_config(struct intel_crtc_state *crtc_state);
> +void hsw_ips_debugfs_register(struct drm_i915_private *i915);
>
> #endif /* __HSW_IPS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 25013f303c82..8be606bfd2b4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -8,6 +8,7 @@
> #include
> #include
>
> +#include "hsw_ips.h"
> #include "i915_debugfs.h"
> #include "i915_irq.h"
> #include "i915_reg.h"
> @@ -49,33 +50,6 @@ static int i915_frontbuffer_tracking(struct seq_file *m,
> void *unused)
> return 0;
> }
>
> -static int i915_ips_status(struct seq_file *m, void *unused)
> -{
> - struct drm_i915_private *dev_priv = node_to_i915(m->private);
> - intel_wakeref_t wakeref;
> -
> - if (!HAS_IPS(dev_priv))
> - return -ENODEV;
> -
> - wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> -
> - seq_printf(m, "Enabled by kernel parameter: %s\n",
> -str_yes_no(dev_priv->params.enable_ips));
> -
> - if (DISPLAY_VER(dev_priv) >= 8) {
> - seq_puts(m, "Currently: unknown\n");
> - } else {
> - if (intel_de_read(dev_priv, IPS_CTL) & IPS_ENABLE)
> - seq_puts(m, "Currently: enabled\n");
> - else
> - seq_puts(m, "Currently: disabled\n");
> - }
> -
> - intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
> -
> - return 0;
> -}
> -
> static int i915_sr_status(struct seq_file *m, void *unused)
> {
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -1343,7 +1317,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_ips_status", i915_ips_status, 0},
> {"i915_sr_status", i915_sr_status, 0},
> {"i915_opregion", i915_opregion,
