On Fri, 2013-11-22 at 21:04 +0200, Imre Deak wrote:
> On Fri, 2013-11-22 at 15:32 -0200, Paulo Zanoni wrote:
> > 2013/11/14 Imre Deak <imre.deak@xxxxxxxxx>:
> > > Add a debugfs entry showing the use-count for all power domains of each
> > > power well.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > 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?
>
> In the first version I had the POWER_DOMAIN_* values redefined to be
> bitmasks, where this wouldn't have worked.
>
> > 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.
>
> I think this is mostly a matter of taste, having a switch table allows
> for mistypings for example. But I don't mind rewriting it if that's a
> more standard way in the driver.
>
> > > +
> > > + 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.
>
> It belongs to the power well. For instance in theory we could have a
> power domain that needs more than a single power well to be on.
Actually you were right. Even though I want to show the refcount for a
given domain for each power well, this refcount will always be the same
for each power well. So I'll move domain_count to power_domains as you
suggested.
--Imre
>
> > > +#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);
>
> Ok, will add this.
>
> --Imre
>
> >
> > > + 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
