| Message ID | 20170411101340.31994-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/04/2017 11:13, Chris Wilson wrote: > We acquire the forcewake and use I915_READ_FW instead for the atomic > wait within intel_uncore_wait_for_register. However, this still leaves > us vulnerable to concurrent mmio access to the register, which can cause > system hangs on gen7. The protection is to acquire uncore.lock around > each register, so lets add it back. > > v2: Wrap __intel_wait_for_register_fw() to re-use its atomic wait_for > loop and spare adding another for ourselves. > v3: Add might_sleep() annotation > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 53c8457869f6..f43121700f83 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1661,14 +1661,22 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv, > u32 value, > unsigned int timeout_ms) > { > - > unsigned fw = > intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ); > int ret; > > - intel_uncore_forcewake_get(dev_priv, fw); > - ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2); > - intel_uncore_forcewake_put(dev_priv, fw); > + might_sleep(); > + > + spin_lock_irq(&dev_priv->uncore.lock); > + intel_uncore_forcewake_get__locked(dev_priv, fw); > + > + ret = __intel_wait_for_register_fw(dev_priv, > + reg, mask, value, > + 2, 0, NULL); > + > + intel_uncore_forcewake_put__locked(dev_priv, fw); > + spin_unlock_irq(&dev_priv->uncore.lock); > + > if (ret) > ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value, > timeout_ms); > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 53c8457869f6..f43121700f83 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1661,14 +1661,22 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv, u32 value, unsigned int timeout_ms) { - unsigned fw = intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ); int ret; - intel_uncore_forcewake_get(dev_priv, fw); - ret = wait_for_us((I915_READ_FW(reg) & mask) == value, 2); - intel_uncore_forcewake_put(dev_priv, fw); + might_sleep(); + + spin_lock_irq(&dev_priv->uncore.lock); + intel_uncore_forcewake_get__locked(dev_priv, fw); + + ret = __intel_wait_for_register_fw(dev_priv, + reg, mask, value, + 2, 0, NULL); + + intel_uncore_forcewake_put__locked(dev_priv, fw); + spin_unlock_irq(&dev_priv->uncore.lock); + if (ret) ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value, timeout_ms);
We acquire the forcewake and use I915_READ_FW instead for the atomic wait within intel_uncore_wait_for_register. However, this still leaves us vulnerable to concurrent mmio access to the register, which can cause system hangs on gen7. The protection is to acquire uncore.lock around each register, so lets add it back. v2: Wrap __intel_wait_for_register_fw() to re-use its atomic wait_for loop and spare adding another for ourselves. v3: Add might_sleep() annotation Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_uncore.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)