Re: [PATCH 03/19] drm/i915: get a PC8 reference when enabling the power well

Daniel Vetter
2013-12-10T21:28:59Z

On Wed, Nov 27, 2013 at 05:59:22PM -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" subtest from the "pm_pc8" test from
> intel-gpu-tools.
>
> v2: - Rebase (i915_disable_power_well).
> v3: - More reabase.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 432ec13..1cb02b1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5677,6 +5677,8 @@ static void hsw_set_power_well(struct drm_device *dev,
> unsigned long irqflags;
> 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;
> @@ -5736,17 +5738,26 @@ 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)
> {
> - if (!power_well->count++ && power_well->set)
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (!power_well->count++ && power_well->set) {
> + hsw_disable_package_c8(dev_priv);
> power_well->set(dev, power_well, true);

Just to reiterate what we've discussed: Once this stuff has settled we
need to push the hsw_*_pc8 out of this common code into the
power_well->set/get functions. Merged the patch for now.
-Daniel

> + }
> }
>
> 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 && power_well->set && i915_disable_power_well)
> + if (!--power_well->count && power_well->set &&
> + i915_disable_power_well) {
> power_well->set(dev, power_well, false);
> + hsw_enable_package_c8(dev_priv);
> + }
> }
>
> void intel_display_power_get(struct drm_device *dev,
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch