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) > +{ > + /* 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); Not sure it does. I don't think I've never seen a list of trapped registers anywhere. Assuming it does, it would quite surprising that it would release the trap before it actually woke up the hardware. I guess it might be possible if the registers don't actually live inside the power well (they'd have to live in power well 0 then I suppose). But that would seem to open up races all over the place where we have to touch a register also touched by the DMC, so it would seem like a fairly major problem to me. If it doesn't trap this, well, then we should just be able to access any actually trapped register, and that should kick the DMC out of DC5/6. That DC_STATE_EN seems to change on its own is still baffling me. One idea I had is that the pcu might do the save/restore for DC_STATE_EN, and then we might race with it somehow. I suppose that could happen if the pcu does some kind of speculative save of that register, and then we might end up changing it between the time it got saved and the time power well 0 gets turned off. Otherwise AFAIK the power well 0 should remain enabled as long as the machine stays out of deep pc states, which means the cpu being active should be enough. Or maybe it's the DMC that saves it when it indicates DC state readyness to the pcu, but if there's no trap we might end up changing the value after that before power well 0 gets turned off. I'm not sure how we'd go about fixing that. Anything I come up with seems to have some races in it. But of course it's really just guesswork without knowing what the the dmc and pcu are doing. We'd definitely need some way to make sure that any saved stale DC_STATE_EN value doesn't get to live past the point where we change it. One still quite racy looking idea would be to kick the DMC on both sides of the DC_STATE_EN write to lessen the chance that it would end up saving the stale value just before we update it. But since the DMC doesn't do much in the way of delayed power off, I'm not sure that would really help. And even with a delayed power off, it'd remain racy. > + > + 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); > +} > + > static void assert_can_enable_dc9(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > + const u32 dc_state = gen9_get_dc_state(dev_priv); > > WARN(!IS_BROXTON(dev), "Platform doesn't support DC9.\n"); > - WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9), > - "DC9 already programmed to be enabled.\n"); > - WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5, > - "DC5 still not disabled to enable DC9.\n"); > + WARN(dc_state & DC_STATE_EN_DC9, > + "DC9 already programmed to be enabled.\n"); > + WARN(dc_state & DC_STATE_EN_UPTO_DC5, > + "DC5 still not disabled to enable DC9.\n"); > WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on.\n"); > WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n"); > > @@ -441,10 +488,12 @@ static void assert_can_enable_dc9(struct drm_i915_private *dev_priv) > > static void assert_can_disable_dc9(struct drm_i915_private *dev_priv) > { > + const u32 dc_state = gen9_get_dc_state(dev_priv); > + > WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n"); > - WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9), > + WARN(!(dc_state & DC_STATE_EN_DC9), > "DC9 already programmed to be disabled.\n"); > - WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5, > + WARN(dc_state & DC_STATE_EN_UPTO_DC5, > "DC5 still not disabled.\n"); > > /* > @@ -491,13 +540,14 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state) > if (state & DC_STATE_EN_UPTO_DC5_DC6_MASK) > gen9_set_dc_state_debugmask_memory_up(dev_priv); > > - val = I915_READ(DC_STATE_EN); > + val = gen9_get_dc_state(dev_priv); > DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", > val & mask, state); > val &= ~mask; > val |= state; > I915_WRITE(DC_STATE_EN, val); > - POSTING_READ(DC_STATE_EN); > + > + WARN_ON((gen9_get_dc_state(dev_priv) & mask) != state); > } > > void bxt_enable_dc9(struct drm_i915_private *dev_priv) > @@ -531,13 +581,13 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv) > struct drm_device *dev = dev_priv->dev; > bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv, > SKL_DISP_PW_2); > + const u32 dc_state = gen9_get_dc_state(dev_priv); > > WARN_ONCE(!IS_SKYLAKE(dev) && !IS_KABYLAKE(dev), > "Platform doesn't support DC5.\n"); > WARN_ONCE(!HAS_RUNTIME_PM(dev), "Runtime PM not enabled.\n"); > WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n"); > - > - WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5), > + WARN_ONCE(dc_state & DC_STATE_EN_UPTO_DC5, > "DC5 already programmed to be enabled.\n"); > assert_rpm_wakelock_held(dev_priv); > > @@ -568,13 +618,14 @@ static void gen9_enable_dc5(struct drm_i915_private *dev_priv) > static void assert_can_enable_dc6(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > + const u32 dc_state = gen9_get_dc_state(dev_priv); > > WARN_ONCE(!IS_SKYLAKE(dev) && !IS_KABYLAKE(dev), > "Platform doesn't support DC6.\n"); > WARN_ONCE(!HAS_RUNTIME_PM(dev), "Runtime PM not enabled.\n"); > WARN_ONCE(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE, > "Backlight is not disabled.\n"); > - WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6), > + WARN_ONCE(dc_state & DC_STATE_EN_UPTO_DC6, > "DC6 already programmed to be enabled.\n"); > > assert_csr_loaded(dev_priv); > @@ -589,7 +640,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv) > if (dev_priv->power_domains.initializing) > return; > > - WARN_ONCE(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6), > + WARN_ONCE(!(gen9_get_dc_state(dev_priv) & DC_STATE_EN_UPTO_DC6), > "DC6 already programmed to be disabled.\n"); > } > > @@ -732,10 +783,7 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > static bool skl_power_well_enabled(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well) > { > - uint32_t mask = SKL_POWER_WELL_REQ(power_well->data) | > - SKL_POWER_WELL_STATE(power_well->data); > - > - return (I915_READ(HSW_PWR_WELL_DRIVER) & mask) == mask; > + return gen9_power_well_enabled(dev_priv, power_well->data); > } > > static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv, > @@ -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; > } > > static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv, > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
