[Intel-gfx] [PATCH 3/4] drm/i915: get a PC8 reference when enabling the power well

[Paulo Zanoni]

2013/10/31 Imre Deak <imre.deak at intel.com>:
> On Wed, 2013-10-30 at 19:40 -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> In the current code, at haswell_modeset_global_resources, first we
>> decide if we want to enable/disable the power well, then we decide if
>> we want to enable/disable PC8. On the case where we're enabling PC8
>> this works fine, but on the case where we disable PC8 due to a non-eDP
>> monitor being enabled, we first enable the power well and then disable
>> PC8. Although wrong, this doesn't seem to be causing any problems now,
>> and we don't even see anything in dmesg. But the patches for runtime
>> D3 turn this problem into a real bug, so we need to fix it.
>>
>> This fixes the "modeset-non-lpsp" test from both "pc8" and
>> "runtime_pm" tests from intel-gpu-tools.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index a0c907f..5b50083 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5547,6 +5547,8 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>>       bool is_enabled, enable_requested;
>>       uint32_t tmp;
>>
>> +     WARN_ON(dev_priv->pc8.enabled);
>> +
>>       tmp = I915_READ(HSW_PWR_WELL_DRIVER);
>>       is_enabled = tmp & HSW_PWR_WELL_STATE_ENABLED;
>>       enable_requested = tmp & HSW_PWR_WELL_ENABLE_REQUEST;
>> @@ -5591,16 +5593,24 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>>  static void __intel_power_well_get(struct drm_device *dev,
>>                                  struct i915_power_well *power_well)
>>  {
>> -     if (!power_well->count++)
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     if (!power_well->count++) {
>> +             hsw_disable_package_c8(dev_priv);
>>               __intel_set_power_well(dev, true);
>> +     }
>>  }
>>
>>  static void __intel_power_well_put(struct drm_device *dev,
>>                                  struct i915_power_well *power_well)
>>  {
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>>       WARN_ON(!power_well->count);
>> -     if (!--power_well->count)
>> +     if (!--power_well->count) {
>>               __intel_set_power_well(dev, false);
>> +             hsw_enable_package_c8(dev_priv);
>> +     }
>>  }
>
> It would be better to add these __intel_set_power_well(), which has all
> the HSW specific bits already.

The problem is that __intel_set_power_well() is also called by
intel_power_domains_resume(), which is just used to set the power well
on the state we expect it to already be (without changing the
refcounts). So in the current code I can safely assume that the get
and put calls are balanced, while if I move the code to
__intel_set_power_well() I will lose this guarantee and have to work
around it.

Besides, it should be safe to call these functions on non-HSW
platforms since they have a check. And in the future, when I merge PC8
and D3 as a single feature, this will be a call to D3 get, which will
be needed on all platforms anyway.

>
> --Imre
>
>>
>>  void intel_display_power_get(struct drm_device *dev,
>



-- 
Paulo Zanoni