| Message ID | 20180920143350.29249-11-mika.kuoppala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ICL interrupt handling improvements | expand |
On 20/09/18 07:33, Mika Kuoppala wrote: > If we ack the identities immediately after they have been > handled, it should unblock next interrupt accumulation > in the gathering register earlier. It also allows us to > remove time based polling of valid bit. If we don't get a valid > sample now, we will likely get a valid sample on next interrupt, > which will be generated due to skipping the ack. The downside > is that we will have as many ack writes as there were identities > handled. > I'm not sure this is going to work. It looks like if we skip the clearing of the identity register we need to skip the clearing of INTR_DW as well because the HW doesn't seem to set that again. If we clear it, when the interrupt gets re-triggered the driver won't be able to handle it and so it'll keep being re-triggered in a loop. I've just confirmed this behavior on the current code (i.e. without this series applied) by simply pretending the ident was not valid for the first interrupt: [ 728.765749] [drm:gen11_gt_engine_identity [i915]] *ERROR* INTR_IDENTITY_REG0:15 0x00000000 not valid! [ 728.765800] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank! [ 728.765865] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank! [ 728.766050] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank! [... more of the same ...] [ 782.395257] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank! [ 782.395293] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank! I haven't checked in detail all the other patches in the series yet so not sure if any of them can mitigate this issue. Daniele > Leave small retry loop for safety and with debugs to see if we > ever encounter read with valid not set. > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++++--------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 27116e3f21af..beb9fe4abf1f 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2965,22 +2965,18 @@ gen11_gt_engine_identity(struct drm_i915_private * const i915, > const unsigned int bank, const unsigned int bit) > { > void __iomem * const regs = i915->regs; > - u32 timeout_ts; > - u32 ident; > + u32 ident, retry = 0; > > lockdep_assert_held(&i915->irq_lock); > > raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank), BIT(bit)); > > - /* > - * NB: Specs do not specify how long to spin wait, > - * so we do ~100us as an educated guess. > - */ > - timeout_ts = (local_clock() >> 10) + 100; > do { > ident = raw_reg_read(regs, GEN11_INTR_IDENTITY_REG(bank)); > - } while (!(ident & GEN11_INTR_DATA_VALID) && > - !time_after32(local_clock() >> 10, timeout_ts)); > + } while (!(ident & GEN11_INTR_DATA_VALID) && ++retry <= 10); > + > + if (unlikely(GEM_SHOW_DEBUG() && retry)) > + WARN_ONCE(1, "INTR_IDENTITY took %u reads to settle\n", retry); > > if (unlikely(!(ident & GEN11_INTR_DATA_VALID))) { > DRM_ERROR("INTR_IDENTITY_REG%u:%u 0x%08x not valid!\n", > @@ -3031,9 +3027,6 @@ gen11_gt_identity_handler(struct drm_i915_private * const i915, > const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); > const u16 intr = GEN11_INTR_ENGINE_INTR(identity); > > - if (unlikely(!intr)) > - return; > - > if (class <= COPY_ENGINE_CLASS) > return gen11_engine_irq_handler(i915, class, instance, intr); > > @@ -3065,11 +3058,14 @@ gen11_gt_bank_handler(struct drm_i915_private * const i915, > const u32 ident = gen11_gt_engine_identity(i915, > bank, bit); > > + if (unlikely(!ident)) > + continue; > + > gen11_gt_identity_handler(i915, ident); > - } > > - /* Clear must be after shared has been served for engine */ > - raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw); > + /* Clear must be after shared has been served for engine */ > + raw_reg_write(regs, GEN11_GT_INTR_DW(bank), BIT(bit)); > + } > } > > static void >
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 27116e3f21af..beb9fe4abf1f 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2965,22 +2965,18 @@ gen11_gt_engine_identity(struct drm_i915_private * const i915, const unsigned int bank, const unsigned int bit) { void __iomem * const regs = i915->regs; - u32 timeout_ts; - u32 ident; + u32 ident, retry = 0; lockdep_assert_held(&i915->irq_lock); raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank), BIT(bit)); - /* - * NB: Specs do not specify how long to spin wait, - * so we do ~100us as an educated guess. - */ - timeout_ts = (local_clock() >> 10) + 100; do { ident = raw_reg_read(regs, GEN11_INTR_IDENTITY_REG(bank)); - } while (!(ident & GEN11_INTR_DATA_VALID) && - !time_after32(local_clock() >> 10, timeout_ts)); + } while (!(ident & GEN11_INTR_DATA_VALID) && ++retry <= 10); + + if (unlikely(GEM_SHOW_DEBUG() && retry)) + WARN_ONCE(1, "INTR_IDENTITY took %u reads to settle\n", retry); if (unlikely(!(ident & GEN11_INTR_DATA_VALID))) { DRM_ERROR("INTR_IDENTITY_REG%u:%u 0x%08x not valid!\n", @@ -3031,9 +3027,6 @@ gen11_gt_identity_handler(struct drm_i915_private * const i915, const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); const u16 intr = GEN11_INTR_ENGINE_INTR(identity); - if (unlikely(!intr)) - return; - if (class <= COPY_ENGINE_CLASS) return gen11_engine_irq_handler(i915, class, instance, intr); @@ -3065,11 +3058,14 @@ gen11_gt_bank_handler(struct drm_i915_private * const i915, const u32 ident = gen11_gt_engine_identity(i915, bank, bit); + if (unlikely(!ident)) + continue; + gen11_gt_identity_handler(i915, ident); - } - /* Clear must be after shared has been served for engine */ - raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw); + /* Clear must be after shared has been served for engine */ + raw_reg_write(regs, GEN11_GT_INTR_DW(bank), BIT(bit)); + } } static void
If we ack the identities immediately after they have been handled, it should unblock next interrupt accumulation in the gathering register earlier. It also allows us to remove time based polling of valid bit. If we don't get a valid sample now, we will likely get a valid sample on next interrupt, which will be generated due to skipping the ack. The downside is that we will have as many ack writes as there were identities handled. Leave small retry loop for safety and with debugs to see if we ever encounter read with valid not set. Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)