On Thu, Jun 29, 2017 at 01:06:01PM -0700, Rodrigo Vivi wrote: > This patch makes sense and seems the right thing to do... > > However removing the sanitize with I915_WRITE(*, val & ~mask); > doesn't give me very comfortable... > > I've seem many power well timeouts on cnl due the lack of that sanitize... > I will try to save some time here and do some experiments with cnl. Thanks for the review. You shouldn't get any timeouts due to this change. On CNL as on any other GEN9+ platform previously you got a timeout when disabling power well 1 during the display uninit sequence, which was due to DMC's own request on PW#1. After this change we'll detect this scenario and won't wait for the power well to get disabled. --Imre > > On Thu, Jun 29, 2017 at 8:37 AM, Imre Deak <imre.deak@xxxxxxxxx> wrote: > > So far in an attempt to make sure all power wells get disabled during > > display uninitialization the driver removed any secondary request bits > > (BIOS, KVMR, DEBUG) that were set for a given power well. The known > > source for these requests was DMC's request on power well 1 and the misc > > IO power well. Since DMC is inactive (DC states are disabled) at the > > point we disable these power wells, there shouldn't be any reason to > > leave them on. However there are two problems with the above > > assumption: Bspec requires that the misc IO power well stays enabled > > (without providing a reason) and there can be KVMR requests that we > > can't remove anyway (the KVMR request register is R/O). Atm, a KVMR > > request can trigger a timeout WARN when trying to disable power wells. > > > > To make the code aligned to Bspec and to get rid of the KVMR WARN, don't > > try to remove the secondary requests, only detect them and stop polling > > for the power well disabled state when any one is set. > > > > Also add a comment about the timeout values required by Bspec when > > enabling power wells and the fact that waiting for them to get disabled > > is not required by Bspec. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98564 > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 109 ++++++++++++++++++-------------- > > 1 file changed, 63 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index 1fc75e6..2fe715b 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -341,6 +341,59 @@ static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv, > > 1 << PIPE_C | 1 << PIPE_B); > > } > > > > +static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + int id = power_well->id; > > + > > + /* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */ > > + WARN_ON(intel_wait_for_register(dev_priv, > > + HSW_PWR_WELL_DRIVER, > > + SKL_POWER_WELL_STATE(id), > > + SKL_POWER_WELL_STATE(id), > > + 1)); > > +} > > + > > +static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id) > > +{ > > + u32 req_mask = SKL_POWER_WELL_REQ(id); > > + u32 ret; > > + > > + ret = I915_READ(HSW_PWR_WELL_BIOS) & req_mask ? 1 : 0; > > + ret |= I915_READ(HSW_PWR_WELL_DRIVER) & req_mask ? 2 : 0; > > + ret |= I915_READ(HSW_PWR_WELL_KVMR) & req_mask ? 4 : 0; > > + ret |= I915_READ(HSW_PWR_WELL_DEBUG) & req_mask ? 8 : 0; > > + > > + return ret; > > +} > > + > > +static void gen9_wait_for_power_well_disable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + int id = power_well->id; > > + bool disabled; > > + u32 reqs; > > + > > + /* > > + * Bspec doesn't require waiting for PWs to get disabled, but still do > > + * this for paranoia. The known cases where a PW will be forced on: > > + * - a KVMR request on any power well via the KVMR request register > > + * - a DMC request on PW1 and MISC_IO power wells via the BIOS and > > + * DEBUG request registers > > + * Skip the wait in case any of the request bits are set and print a > > + * diagnostic message. > > + */ > > + wait_for((disabled = !(I915_READ(HSW_PWR_WELL_DRIVER) & > > + SKL_POWER_WELL_STATE(id))) || > > + (reqs = gen9_power_well_requesters(dev_priv, id)), 1); > > + if (disabled) > > + return; > > + > > + DRM_DEBUG_KMS("%s forced on (bios:%d driver:%d kvmr:%d debug:%d)\n", > > + power_well->name, > > + !!(reqs & 1), !!(reqs & 2), !!(reqs & 4), !!(reqs & 8)); > > +} > > + > > static void hsw_set_power_well(struct drm_i915_private *dev_priv, > > struct i915_power_well *power_well, bool enable) > > { > > @@ -746,45 +799,6 @@ void skl_disable_dc6(struct drm_i915_private *dev_priv) > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > } > > > > -static void > > -gen9_sanitize_power_well_requests(struct drm_i915_private *dev_priv, > > - struct i915_power_well *power_well) > > -{ > > - enum skl_disp_power_wells power_well_id = power_well->id; > > - u32 val; > > - u32 mask; > > - > > - mask = SKL_POWER_WELL_REQ(power_well_id); > > - > > - val = I915_READ(HSW_PWR_WELL_KVMR); > > - if (WARN_ONCE(val & mask, "Clearing unexpected KVMR request for %s\n", > > - power_well->name)) > > - I915_WRITE(HSW_PWR_WELL_KVMR, val & ~mask); > > - > > - val = I915_READ(HSW_PWR_WELL_BIOS); > > - val |= I915_READ(HSW_PWR_WELL_DEBUG); > > - > > - if (!(val & mask)) > > - return; > > - > > - /* > > - * DMC is known to force on the request bits for power well 1 on SKL > > - * and BXT and the misc IO power well on SKL but we don't expect any > > - * other request bits to be set, so WARN for those. > > - */ > > - if (power_well_id == SKL_DISP_PW_1 || > > - (IS_GEN9_BC(dev_priv) && > > - power_well_id == SKL_DISP_PW_MISC_IO)) > > - DRM_DEBUG_DRIVER("Clearing auxiliary requests for %s forced on " > > - "by DMC\n", power_well->name); > > - else > > - WARN_ONCE(1, "Clearing unexpected auxiliary requests for %s\n", > > - power_well->name); > > - > > - I915_WRITE(HSW_PWR_WELL_BIOS, val & ~mask); > > - I915_WRITE(HSW_PWR_WELL_DEBUG, val & ~mask); > > -} > > - > > static void skl_set_power_well(struct drm_i915_private *dev_priv, > > struct i915_power_well *power_well, bool enable) > > { > > @@ -848,6 +862,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > > DRM_DEBUG_KMS("Enabling %s\n", power_well->name); > > check_fuse_status = true; > > } > > + > > + gen9_wait_for_power_well_enable(dev_priv, power_well); > > } else { > > if (enable_requested) { > > I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask); > > @@ -855,14 +871,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, > > DRM_DEBUG_KMS("Disabling %s\n", power_well->name); > > } > > > > - gen9_sanitize_power_well_requests(dev_priv, power_well); > > + gen9_wait_for_power_well_disable(dev_priv, power_well); > > } > > > > - if (wait_for(!!(I915_READ(HSW_PWR_WELL_DRIVER) & state_mask) == enable, > > - 1)) > > - DRM_ERROR("%s %s timeout\n", > > - power_well->name, enable ? "enable" : "disable"); > > - > > if (check_fuse_status) { > > if (power_well->id == SKL_DISP_PW_1) { > > if (intel_wait_for_register(dev_priv, > > @@ -2699,6 +2710,8 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv) > > /* > > * BSpec says to keep the MISC IO power well enabled here, only > > * remove our request for power well 1. > > + * Note that even though the driver's request is removed power well 1 > > + * may stay enabled after this due to DMC's own request on it. > > */ > > well = lookup_power_well(dev_priv, SKL_DISP_PW_1); > > intel_power_well_disable(dev_priv, well); > > @@ -2756,7 +2769,11 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv) > > > > /* The spec doesn't call for removing the reset handshake flag */ > > > > - /* Disable PG1 */ > > + /* > > + * Disable PW1 (PG1). > > + * Note that even though the driver's request is removed power well 1 > > + * may stay enabled after this due to DMC's own request on it. > > + */ > > mutex_lock(&power_domains->lock); > > > > well = lookup_power_well(dev_priv, SKL_DISP_PW_1); > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
