| Message ID | 1383169226-16707-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2013-10-30 at 19:40 -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@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@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. --Imre > > void intel_display_power_get(struct drm_device *dev,
2013/10/31 Imre Deak <imre.deak@intel.com>: > On Wed, 2013-10-30 at 19:40 -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@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@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, >
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); + } } void intel_display_power_get(struct drm_device *dev,