[Intel-gfx] [PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c

[Paulo Zanoni]

2014-02-21 14:41 GMT-03:00 Jesse Barnes <jbarnes at virtuousgeek.org>:
> On Fri, 21 Feb 2014 13:52:23 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
>
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> These are places where we read (not write) registers while we're
>> runtime suspended.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index d90a707..34e347f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>               return 0;
>>       }
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       if (intel_fbc_enabled(dev)) {
>>               seq_puts(m, "FBC enabled\n");
>>       } else {
>> @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>               }
>>               seq_putc(m, '\n');
>>       }
>> +
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       return 0;
>>  }
>>
>> @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused)
>>               return 0;
>>       }
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE)
>>               seq_puts(m, "enabled\n");
>>       else
>>               seq_puts(m, "disabled\n");
>>
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       return 0;
>>  }
>>
>> @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>>       drm_i915_private_t *dev_priv = dev->dev_private;
>>       bool sr_enabled = false;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       if (HAS_PCH_SPLIT(dev))
>>               sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
>>       else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev))
>> @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>>       else if (IS_PINEVIEW(dev))
>>               sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
>>
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       seq_printf(m, "self-refresh: %s\n",
>>                  sr_enabled ? "enabled" : "disabled");
>>
>> @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data)
>>       if (INTEL_INFO(dev)->gen < 6)
>>               return -ENODEV;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       rdmsrl(MSR_RAPL_POWER_UNIT, power);
>>       power = (power & 0x1f00) >> 8;
>>       units = 1000000 / (1 << power); /* convert to uJ */
>>       power = I915_READ(MCH_SECP_NRG_STTS);
>>       power *= units;
>>
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       seq_printf(m, "%llu", (long long unsigned)power);
>>
>>       return 0;
>
> Looks like there are more places we need this too.. wonder if it would
> be better to put the get into some wrapper around our sysfs files...

Well, at least the deubgfs-read test from pm_pc8 doesn't complain on
my HSW and SNB. But yeah, I thought about the wrapper too. I imagine
people adding new debugfs files will always forget about the runtime
PM refcounts, so maybe the wrapper is safer. But I could do this in a
separate patch.

I also thought about wrapping the connector->detect functions.

Thanks for the reviews!

>
> But these bits look correct, if not sufficient, so:
>
> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni