2013/10/31 Imre Deak <imre.deak@xxxxxxxxx>:
> On Wed, 2013-10-30 at 19:40 -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
