[Intel-gfx] [PATCH v2 8/8] drm/i915: add a debugfs entry for power domain info

Paulo Zanoni przanoni at gmail.com
Fri Nov 22 18:32:48 CET 2013


2013/11/14 Imre Deak <imre.deak at intel.com>:
> Add a debugfs entry showing the use-count for all power domains of each
> power well.
>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |  4 +++
>  drivers/gpu/drm/i915/intel_pm.c     | 16 ++++++---
>  3 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6875b7a..a6555cd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1844,6 +1844,74 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static const char *power_domain_str(enum intel_display_power_domain domain)
> +{
> +#define PWRD(p)        { POWER_DOMAIN_ ## p, # p }
> +       static struct {
> +               enum intel_display_power_domain domain;
> +               const char *name;
> +       } table[] = {
> +               PWRD(PIPE_A),
> +               PWRD(PIPE_B),
> +               PWRD(PIPE_C),
> +               PWRD(PIPE_A_PANEL_FITTER),
> +               PWRD(PIPE_B_PANEL_FITTER),
> +               PWRD(PIPE_C_PANEL_FITTER),
> +               PWRD(TRANSCODER_A),
> +               PWRD(TRANSCODER_B),
> +               PWRD(TRANSCODER_C),
> +               PWRD(TRANSCODER_EDP),
> +               PWRD(VGA),
> +               PWRD(AUDIO),
> +               PWRD(INIT),
> +       };
> +#undef PWRD
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(table); i++)
> +               if (table[i].domain == domain)
> +                       return table[i].name;

Why not just "return table[domain].name" instead of the 3 lines above?

Our driver has a few of these functions returning strings for enums
and they usually just contain a "switch" statement on the enum,
without using complicated macros. I tend to prefer the simple things.


> +
> +       WARN_ON(1);
> +
> +       return "?";
> +}
> +
> +static int i915_power_domain_info(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       int i;
> +
> +       mutex_lock(&power_domains->lock);
> +
> +       seq_printf(m, "%-25s %s\n", "Power well/domain", "Use count");
> +       for (i = 0; i < power_domains->power_well_count; i++) {
> +               struct i915_power_well *power_well;
> +               enum intel_display_power_domain power_domain;
> +
> +               power_well = &power_domains->power_wells[i];
> +               seq_printf(m, "%-25s %d\n", power_well->name,
> +                          power_well->count);
> +
> +               for (power_domain = 0; power_domain < POWER_DOMAIN_NUM;
> +                    power_domain++) {
> +                       if (!(BIT(power_domain) & power_well->domains))
> +                               continue;
> +
> +                       seq_printf(m, "  %-23s %d\n",
> +                                  power_domain_str(power_domain),
> +                                  power_well->domain_count[power_domain]);
> +               }
> +       }
> +
> +       mutex_unlock(&power_domains->lock);
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -3076,6 +3144,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_edp_psr_status", i915_edp_psr_status, 0},
>         {"i915_energy_uJ", i915_energy_uJ, 0},
>         {"i915_pc8_status", i915_pc8_status, 0},
> +       {"i915_power_domain_info", i915_power_domain_info, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 06f47bf..194e39f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -950,6 +950,10 @@ struct i915_power_well {
>         /* power well enable/disable usage count */
>         int count;
>         unsigned long domains;
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +       /* usage count for each power domain in the domains mask */
> +       int domain_count[POWER_DOMAIN_NUM];

Shouldn't we track this in struct power_domains, since when we get a
domain we don't get it for a specific power well, but just "get the
domain"? I think it makes more sense.


> +#endif
>         void *data;
>         void (*set)(struct drm_device *dev, struct i915_power_well *power_well,
>                     bool enable);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d252453..99210c1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5726,17 +5726,25 @@ static void hsw_set_power_well(struct drm_device *dev,
>  }
>
>  static void __intel_power_well_get(struct drm_device *dev,
> -                                  struct i915_power_well *power_well)
> +                                  struct i915_power_well *power_well,
> +                                  enum intel_display_power_domain domain)
>  {
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +       power_well->domain_count[domain]++;
> +#endif
>         if (!power_well->count++ && power_well->set)
>                 power_well->set(dev, power_well, true);
>  }
>
>  static void __intel_power_well_put(struct drm_device *dev,
> -                                  struct i915_power_well *power_well)
> +                                  struct i915_power_well *power_well,
> +                                  enum intel_display_power_domain domain)
>  {
>         WARN_ON(!power_well->count);
>
> +#if IS_ENABLED(CONFIG_DEBUG_FS)

WARN_ON(power_well->domain_count[domain] == 0);


> +       power_well->domain_count[domain]--;
> +#endif
>         if (!--power_well->count && power_well->set && i915_disable_power_well)
>                 power_well->set(dev, power_well, false);
>  }
> @@ -5753,7 +5761,7 @@ void intel_display_power_get(struct drm_device *dev,
>
>         mutex_lock(&power_domains->lock);
>         for_each_power_well(i, power_well, BIT(domain), power_domains)
> -               __intel_power_well_get(dev, power_well);
> +               __intel_power_well_get(dev, power_well, domain);
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5769,7 +5777,7 @@ void intel_display_power_put(struct drm_device *dev,
>
>         mutex_lock(&power_domains->lock);
>         for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> -               __intel_power_well_put(dev, power_well);
> +               __intel_power_well_put(dev, power_well, domain);
>         mutex_unlock(&power_domains->lock);
>  }
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni



More information about the Intel-gfx mailing list