[Intel-gfx] [PATCH] drm/i915/gen9: Probe power well 1 status on dc status query

[Chris Wilson]

On Tue, Jan 26, 2016 at 05:45:31PM +0200, Mika Kuoppala wrote:
> There has been cases where we read DC_STATE and get something that we
> did not write there. As DMC owns power well 1, this could be that DMC
> snoops DC_STATE accesses and needs to wake up power well 1 up to serve
> the access. But the waking up power well 1 takes time and we might end up
> reading during unfinished well wakeup.
> 
> When we want the dc status, wake up DMC and make sure that DMC has
> properly woken up well 1 before we read for the final value.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=93768
> Suggested-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> Cc: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 86 ++++++++++++++++++++++++++-------
>  1 file changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index bbca527184d0..83c24e73cb88 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -418,15 +418,62 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |	\
>  	BIT(POWER_DOMAIN_INIT))
>  
> +
> +static bool __gen9_power_well_enabled(struct drm_i915_private *dev_priv,
> +				      const enum skl_disp_power_wells pw,
> +				      const bool req)
> +{
> +	uint32_t mask = SKL_POWER_WELL_STATE(pw);
> +
> +	if (req)
> +		mask |= SKL_POWER_WELL_REQ(pw);
> +
> +	return (I915_READ(HSW_PWR_WELL_DRIVER) & mask) == mask;
> +}
> +
> +static bool gen9_power_well_enabled(struct drm_i915_private *dev_priv,
> +				    const enum skl_disp_power_wells pw)
> +{
> +	return __gen9_power_well_enabled(dev_priv, pw, true);
> +}
> +
> +static bool gen9_power_well_enabled_noreq(struct drm_i915_private *dev_priv,
> +					  const enum skl_disp_power_wells pw)
> +{
> +	return __gen9_power_well_enabled(dev_priv, pw, false);
> +}
> +

> +static bool gen9_pw1_enabled(struct drm_i915_private *dev_priv)

Jumping around between conventions for function names?

> +{
> +	/* DMC owns the pw1. Don't check for request */
> +	return gen9_power_well_enabled_noreq(dev_priv, SKL_DISP_PW_1);
> +}
> +
> +static u32 gen9_get_dc_state(struct drm_i915_private *dev_priv)
> +{
> +	if (gen9_pw1_enabled(dev_priv))
> +		return I915_READ(DC_STATE_EN);
> +
> +	/* DMC should snoop this and wakeup */
> +	I915_READ(DC_STATE_EN);
> +
> +	if (wait_for(gen9_pw1_enabled(dev_priv), 1))
> +		DRM_ERROR("pw1 enabling timeout 0x%x\n",
> +			  I915_READ(HSW_PWR_WELL_DRIVER));

	if (!gen9_powerwell_1_enabled(dev_priv)) {
		/* DMC should snoop this and wakeup */
		I915_READ(DC_STATE_EN);

		/* And wait for it to wakeup */
		if (wait_for(gen9_pw1_enabled(dev_priv), 1))
			DRM_ERROR("pw1 enabling timeout 0x%x\n",
				  I915_READ(HSW_PWR_WELL_DRIVER));
	}

> +	return I915_READ(DC_STATE_EN);
> +}

Reads a bit better

> @@ -762,7 +810,11 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>  static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> -	return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
> +	if (gen9_pw1_enabled(dev_priv))
> +		return (I915_READ(DC_STATE_EN) &
> +			DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
> +
> +	return true;

/* Please explain this!
if (!gen9_powerwell_1_enabled(dev_priv))
	return true;

return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;

Again looks a little easier on the eyes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre