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@xxxxxxxxxxxxxxx> > Cc: Patrik Jakobsson <patrik.jakobsson@xxxxxxxxxxxxxxx> > Cc: Imre Deak <imre.deak@xxxxxxxxx> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> > --- > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
