| Message ID | 1441309905-2744-4-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Paulo Zanoni <paulo.r.zanoni@intel.com> writes: > I added this code on 8664281b64c457705db72fc60143d03827e75ca9, on > April 12 2013. Back then, we only had support for detecting unclaimed > registers on I915_WRITE operations, and we didn't have the > i915.mmio_debug infrastructure. > > I tried to remember exactly why I added this code, and the only reason > I can think is: to help debugging. With this code, we would be able to > differentiate between the "your interrupt handler did something wrong" > and the "something bad happened before the interrupt handler" cases, > at the cost of one extra register read operation per interrupt. > > Since then, we added unclaimed register checking support for > I915_READ, we added the i915.mmio_debug infrastructure and we also > fixed most (all?) of the unclaimed register problems on HSW. Due > to this, I don't think the extra register read at every interrupt is > necesary anymore: we're probably good in terms of debugging. > > So let's kill this function in order to make sure it completely > disappears from perf. > > Notice that this only affects HSW. > As you have already posted patches for enabling unclaimed detection for skl, are you sure we want to remove this completely from interrupt path? I would think this is useful feature to have on platform enabling. How about we just bail out on intel_uncore_check_errors if mmio_debug register is zero? -Mika > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/i915_irq.c | 4 ---- > drivers/gpu/drm/i915/intel_uncore.c | 11 ----------- > 3 files changed, 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1287007..194e864 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2703,7 +2703,6 @@ extern void intel_uncore_sanitize(struct drm_device *dev); > extern void intel_uncore_early_sanitize(struct drm_device *dev, > bool restore_forcewake); > extern void intel_uncore_init(struct drm_device *dev); > -extern void intel_uncore_check_errors(struct drm_device *dev); > 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 2063279..57ec55e 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2064,10 +2064,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}. */ > - intel_uncore_check_errors(dev); > - > /* disable master interrupt before clearing iir */ > de_ier = I915_READ(DEIER); > I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 65e0ea8..8844c314 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1554,14 +1554,3 @@ bool intel_has_gpu_reset(struct drm_device *dev) > { > return intel_get_gpu_reset(dev) != NULL; > } > - > -void intel_uncore_check_errors(struct drm_device *dev) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - if (HAS_FPGA_DBG_UNCLAIMED(dev) && > - (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { > - DRM_ERROR("Unclaimed register before interrupt\n"); > - __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > - } > -} > -- > 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 1287007..194e864 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2703,7 +2703,6 @@ extern void intel_uncore_sanitize(struct drm_device *dev); extern void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake); extern void intel_uncore_init(struct drm_device *dev); -extern void intel_uncore_check_errors(struct drm_device *dev); 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 2063279..57ec55e 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2064,10 +2064,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}. */ - intel_uncore_check_errors(dev); - /* disable master interrupt before clearing iir */ de_ier = I915_READ(DEIER); I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 65e0ea8..8844c314 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1554,14 +1554,3 @@ bool intel_has_gpu_reset(struct drm_device *dev) { return intel_get_gpu_reset(dev) != NULL; } - -void intel_uncore_check_errors(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - - if (HAS_FPGA_DBG_UNCLAIMED(dev) && - (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { - DRM_ERROR("Unclaimed register before interrupt\n"); - __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); - } -}
I added this code on 8664281b64c457705db72fc60143d03827e75ca9, on April 12 2013. Back then, we only had support for detecting unclaimed registers on I915_WRITE operations, and we didn't have the i915.mmio_debug infrastructure. I tried to remember exactly why I added this code, and the only reason I can think is: to help debugging. With this code, we would be able to differentiate between the "your interrupt handler did something wrong" and the "something bad happened before the interrupt handler" cases, at the cost of one extra register read operation per interrupt. Since then, we added unclaimed register checking support for I915_READ, we added the i915.mmio_debug infrastructure and we also fixed most (all?) of the unclaimed register problems on HSW. Due to this, I don't think the extra register read at every interrupt is necesary anymore: we're probably good in terms of debugging. So let's kill this function in order to make sure it completely disappears from perf. Notice that this only affects HSW. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_irq.c | 4 ---- drivers/gpu/drm/i915/intel_uncore.c | 11 ----------- 3 files changed, 16 deletions(-)