| Message ID | 1450189512-30360-4-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 15, 2015 at 04:25:09PM +0200, Mika Kuoppala wrote: > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 471f120..3a9229b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev, > > drm_atomic_state_free(state); > > + if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv)) > + DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__); DRM_DEBUG() already includes the "%s" _func__. This would seem to duplicate the DRM_DEBUG from intel_uncore_arm_unclaimed_mmio_detection() which as you point out should have been DRM_DEBUG_DRIVER() in the first place. Minor quible aside (and because this turned up late!), Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Tue, Dec 15, 2015 at 04:25:09PM +0200, Mika Kuoppala wrote: > We have done unclaimed register access check in normal > (mmio_debug=0) mode once per write. This adds probability > of finding the exact sequence where we did the bad access, but > also adds burden to each write. > > As we have mmio_debug available for more fine grained analysis, > give up accuracy of detecting correct spot at the first occurrence > by doing the one shot detection and arming of mmio_debug in hangcheck > and in modeset. This removes the write path performance burden. > > Note that in hangcheck when we arm the fine grained debug > facilities, there is no context even if the unclaimed access > is already set and thus notifying in this point has no added value. > In contrary, in display path, we add a debug message if the bit > was set on arming. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > drivers/gpu/drm/i915/i915_irq.c | 10 ++++----- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > drivers/gpu/drm/i915/intel_uncore.c | 40 +++++++++++++++++++----------------- > 4 files changed, 32 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 82c43b6..625aae9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -723,6 +723,8 @@ struct intel_uncore { > i915_reg_t reg_post; > u32 val_reset; > } fw_domain[FW_DOMAIN_ID_COUNT]; > + > + int unclaimed_mmio_check; > }; > > /* Iterate over initialised fw domains */ > @@ -2732,6 +2734,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev, > bool restore_forcewake); > extern void intel_uncore_init(struct drm_device *dev); > extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv); > +extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv); > extern void intel_uncore_fini(struct drm_device *dev); > extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore); > const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id); > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a20dc64..725a340 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2165,11 +2165,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg) > if (!intel_irqs_enabled(dev_priv)) > return IRQ_NONE; > > - /* We get interrupts on unclaimed registers, so check for this before we > - * do any I915_{READ,WRITE}. */ > - if (intel_uncore_unclaimed_mmio(dev_priv)) > - DRM_ERROR("Unclaimed register before interrupt\n"); > - > /* disable master interrupt before clearing iir */ > de_ier = I915_READ(DEIER); > I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL); > @@ -2990,6 +2985,11 @@ static void i915_hangcheck_elapsed(struct work_struct *work) > if (!i915.enable_hangcheck) > return; > > + /* Periodic arming of mmio_debug if hw detects mmio > + * access to out of range or to an unpowered block > + */ > + intel_uncore_arm_unclaimed_mmio_detection(dev_priv); It's a bit weird to have this here since it'll only detect display register accesses AFAIK. But I guess it's cheaper that adding a dedicated timer or anything for it. > + > for_each_ring(ring, dev_priv, i) { > u64 acthd; > u32 seqno; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 471f120..3a9229b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev, > > drm_atomic_state_free(state); > > + if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv)) > + DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__); > + > return 0; > } > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 34b60cb..911f189 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -629,22 +629,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, > } > } > > -static void > -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv) > -{ > - static bool mmio_debug_once = true; > - > - if (i915.mmio_debug || !mmio_debug_once) > - return; > - > - if (check_for_unclaimed_mmio(dev_priv)) { > - DRM_DEBUG("Unclaimed register detected, " > - "enabling oneshot unclaimed register reporting. " > - "Please use i915.mmio_debug=N for more information.\n"); > - i915.mmio_debug = mmio_debug_once--; > - } > -} > - > #define GEN2_READ_HEADER(x) \ > u##x val = 0; \ > assert_device_not_suspended(dev_priv); > @@ -924,7 +908,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t > gen6_gt_check_fifodbg(dev_priv); \ > } \ > hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ > - hsw_unclaimed_reg_detect(dev_priv); \ > GEN6_WRITE_FOOTER; \ > } > > @@ -959,7 +942,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool > __force_wake_get(dev_priv, FORCEWAKE_RENDER); \ > __raw_i915_write##x(dev_priv, reg, val); \ > hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ > - hsw_unclaimed_reg_detect(dev_priv); \ > GEN6_WRITE_FOOTER; \ > } > > @@ -1029,7 +1011,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \ > __force_wake_get(dev_priv, fw_engine); \ > __raw_i915_write##x(dev_priv, reg, val); \ > hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ > - hsw_unclaimed_reg_detect(dev_priv); \ > GEN6_WRITE_FOOTER; \ > } > > @@ -1249,6 +1230,8 @@ void intel_uncore_init(struct drm_device *dev) > intel_uncore_fw_domains_init(dev); > __intel_uncore_early_sanitize(dev, false); > > + dev_priv->uncore.unclaimed_mmio_check = 1; > + > switch (INTEL_INFO(dev)->gen) { > default: > case 9: > @@ -1610,3 +1593,22 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv) > { > return check_for_unclaimed_mmio(dev_priv); > } > + > +bool > +intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv) > +{ > + if (unlikely(i915.mmio_debug || > + dev_priv->uncore.unclaimed_mmio_check <= 0)) > + return false; > + > + if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) { > + DRM_DEBUG("Unclaimed register detected, " > + "enabling oneshot unclaimed register reporting. " > + "Please use i915.mmio_debug=N for more information.\n"); > + i915.mmio_debug++; > + dev_priv->uncore.unclaimed_mmio_check--; > + return true; > + } > + > + return false; > +} > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 82c43b6..625aae9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -723,6 +723,8 @@ struct intel_uncore { i915_reg_t reg_post; u32 val_reset; } fw_domain[FW_DOMAIN_ID_COUNT]; + + int unclaimed_mmio_check; }; /* Iterate over initialised fw domains */ @@ -2732,6 +2734,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake); extern void intel_uncore_init(struct drm_device *dev); extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv); +extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv); extern void intel_uncore_fini(struct drm_device *dev); extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore); const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a20dc64..725a340 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2165,11 +2165,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg) if (!intel_irqs_enabled(dev_priv)) return IRQ_NONE; - /* We get interrupts on unclaimed registers, so check for this before we - * do any I915_{READ,WRITE}. */ - if (intel_uncore_unclaimed_mmio(dev_priv)) - DRM_ERROR("Unclaimed register before interrupt\n"); - /* disable master interrupt before clearing iir */ de_ier = I915_READ(DEIER); I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL); @@ -2990,6 +2985,11 @@ static void i915_hangcheck_elapsed(struct work_struct *work) if (!i915.enable_hangcheck) return; + /* Periodic arming of mmio_debug if hw detects mmio + * access to out of range or to an unpowered block + */ + intel_uncore_arm_unclaimed_mmio_detection(dev_priv); + for_each_ring(ring, dev_priv, i) { u64 acthd; u32 seqno; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 471f120..3a9229b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev, drm_atomic_state_free(state); + if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv)) + DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__); + return 0; } diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 34b60cb..911f189 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -629,22 +629,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, } } -static void -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv) -{ - static bool mmio_debug_once = true; - - if (i915.mmio_debug || !mmio_debug_once) - return; - - if (check_for_unclaimed_mmio(dev_priv)) { - DRM_DEBUG("Unclaimed register detected, " - "enabling oneshot unclaimed register reporting. " - "Please use i915.mmio_debug=N for more information.\n"); - i915.mmio_debug = mmio_debug_once--; - } -} - #define GEN2_READ_HEADER(x) \ u##x val = 0; \ assert_device_not_suspended(dev_priv); @@ -924,7 +908,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t gen6_gt_check_fifodbg(dev_priv); \ } \ hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ - hsw_unclaimed_reg_detect(dev_priv); \ GEN6_WRITE_FOOTER; \ } @@ -959,7 +942,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool __force_wake_get(dev_priv, FORCEWAKE_RENDER); \ __raw_i915_write##x(dev_priv, reg, val); \ hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ - hsw_unclaimed_reg_detect(dev_priv); \ GEN6_WRITE_FOOTER; \ } @@ -1029,7 +1011,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \ __force_wake_get(dev_priv, fw_engine); \ __raw_i915_write##x(dev_priv, reg, val); \ hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ - hsw_unclaimed_reg_detect(dev_priv); \ GEN6_WRITE_FOOTER; \ } @@ -1249,6 +1230,8 @@ void intel_uncore_init(struct drm_device *dev) intel_uncore_fw_domains_init(dev); __intel_uncore_early_sanitize(dev, false); + dev_priv->uncore.unclaimed_mmio_check = 1; + switch (INTEL_INFO(dev)->gen) { default: case 9: @@ -1610,3 +1593,22 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv) { return check_for_unclaimed_mmio(dev_priv); } + +bool +intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv) +{ + if (unlikely(i915.mmio_debug || + dev_priv->uncore.unclaimed_mmio_check <= 0)) + return false; + + if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) { + DRM_DEBUG("Unclaimed register detected, " + "enabling oneshot unclaimed register reporting. " + "Please use i915.mmio_debug=N for more information.\n"); + i915.mmio_debug++; + dev_priv->uncore.unclaimed_mmio_check--; + return true; + } + + return false; +}
We have done unclaimed register access check in normal (mmio_debug=0) mode once per write. This adds probability of finding the exact sequence where we did the bad access, but also adds burden to each write. As we have mmio_debug available for more fine grained analysis, give up accuracy of detecting correct spot at the first occurrence by doing the one shot detection and arming of mmio_debug in hangcheck and in modeset. This removes the write path performance burden. Note that in hangcheck when we arm the fine grained debug facilities, there is no context even if the unclaimed access is already set and thus notifying in this point has no added value. In contrary, in display path, we add a debug message if the bit was set on arming. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/i915_irq.c | 10 ++++----- drivers/gpu/drm/i915/intel_display.c | 3 +++ drivers/gpu/drm/i915/intel_uncore.c | 40 +++++++++++++++++++----------------- 4 files changed, 32 insertions(+), 24 deletions(-)