| Message ID | 20180312144131.20629-1-mika.kuoppala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Mika Kuoppala (2018-03-12 14:41:31) > Interrupt identity register we already read from hardware > contains engine class and instance fields. Leverage > these fields to find correct engine to handle the interrupt. > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160) > Function old new delta > gen11_irq_handler 764 604 -160 > Total: Before=1506953, After=1506793, chg -0.01% > > v2: handle class/instance overflow correctly (Mika) > > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > static void > @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, > } > > for_each_set_bit(bit, &intr_dw, 32) { > - const u16 iir = gen11_gt_engine_intr(i915, bank, bit); > + const u32 ident = gen11_gt_engine_identity(i915, > + bank, bit); > + const u16 iir = ident & GEN11_INTR_ENGINE_MASK; > + u8 class, instance; > + struct intel_engine_cs *engine; > > if (unlikely(!iir)) > continue; Now if (!ident) or actually just use u32 iir as we can pass it straight through to cs_irq_handler. > - gen11_gt_engine_irq_handler(i915, bank, bit, iir); > + class = GEN11_INTR_ENGINE_CLASS(ident); > + if (unlikely(class >= MAX_ENGINE_CLASS)) > + continue; > + > + instance = GEN11_INTR_ENGINE_INSTANCE(ident); > + if (unlikely(instance >= MAX_ENGINE_INSTANCE)) > + continue; > + > + engine = i915->engine_class[class][instance]; > + if (unlikely(!engine)) > + continue; Spurious interrupts be spurious; but we are can enable interrupts on a per-engine basis, right? -Chris
On 12/03/18 07:52, Chris Wilson wrote: > Quoting Mika Kuoppala (2018-03-12 14:41:31) >> Interrupt identity register we already read from hardware >> contains engine class and instance fields. Leverage >> these fields to find correct engine to handle the interrupt. >> >> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160) >> Function old new delta >> gen11_irq_handler 764 604 -160 >> Total: Before=1506953, After=1506793, chg -0.01% >> >> v2: handle class/instance overflow correctly (Mika) >> >> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> --- >> static void >> @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, >> } >> >> for_each_set_bit(bit, &intr_dw, 32) { >> - const u16 iir = gen11_gt_engine_intr(i915, bank, bit); >> + const u32 ident = gen11_gt_engine_identity(i915, >> + bank, bit); >> + const u16 iir = ident & GEN11_INTR_ENGINE_MASK; >> + u8 class, instance; >> + struct intel_engine_cs *engine; >> >> if (unlikely(!iir)) >> continue; > > Now if (!ident) or actually just use u32 iir as we can pass it straight > through to cs_irq_handler. Can't use (!ident) here because bit 31 (iir valid) or the class/instance bits might be set when the iir is empty (because we had a buffered irq that we actually already handled). > >> - gen11_gt_engine_irq_handler(i915, bank, bit, iir); >> + class = GEN11_INTR_ENGINE_CLASS(ident); >> + if (unlikely(class >= MAX_ENGINE_CLASS)) >> + continue; >> + >> + instance = GEN11_INTR_ENGINE_INSTANCE(ident); >> + if (unlikely(instance >= MAX_ENGINE_INSTANCE)) >> + continue; >> + >> + engine = i915->engine_class[class][instance]; >> + if (unlikely(!engine)) >> + continue; > > Spurious interrupts be spurious; but we are can enable interrupts on a > per-engine basis, right? irq enable registers are per-class for gen11. Daniele > -Chris >
Quoting Daniele Ceraolo Spurio (2018-03-12 15:09:31) > > > On 12/03/18 07:52, Chris Wilson wrote: > > Quoting Mika Kuoppala (2018-03-12 14:41:31) > >> Interrupt identity register we already read from hardware > >> contains engine class and instance fields. Leverage > >> these fields to find correct engine to handle the interrupt. > >> > >> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160) > >> Function old new delta > >> gen11_irq_handler 764 604 -160 > >> Total: Before=1506953, After=1506793, chg -0.01% > >> > >> v2: handle class/instance overflow correctly (Mika) > >> > >> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> --- > >> static void > >> @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, > >> } > >> > >> for_each_set_bit(bit, &intr_dw, 32) { > >> - const u16 iir = gen11_gt_engine_intr(i915, bank, bit); > >> + const u32 ident = gen11_gt_engine_identity(i915, > >> + bank, bit); > >> + const u16 iir = ident & GEN11_INTR_ENGINE_MASK; > >> + u8 class, instance; > >> + struct intel_engine_cs *engine; > >> > >> if (unlikely(!iir)) > >> continue; > > > > Now if (!ident) or actually just use u32 iir as we can pass it straight > > through to cs_irq_handler. > > Can't use (!ident) here because bit 31 (iir valid) or the class/instance > bits might be set when the iir is empty (because we had a buffered irq > that we actually already handled). If there's a valid bit, surely that's the one we want to be testing? If the low iir bits are 0, that's fine. The question is whether it's common enough to worry about; and I note it's marked as unlikely() so it seems like we can just let it fallthrough and do nothing. -Chris
On 12/03/18 08:23, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2018-03-12 15:09:31) >> >> >> On 12/03/18 07:52, Chris Wilson wrote: >>> Quoting Mika Kuoppala (2018-03-12 14:41:31) >>>> Interrupt identity register we already read from hardware >>>> contains engine class and instance fields. Leverage >>>> these fields to find correct engine to handle the interrupt. >>>> >>>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160) >>>> Function old new delta >>>> gen11_irq_handler 764 604 -160 >>>> Total: Before=1506953, After=1506793, chg -0.01% >>>> >>>> v2: handle class/instance overflow correctly (Mika) >>>> >>>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>>> --- >>>> static void >>>> @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, >>>> } >>>> >>>> for_each_set_bit(bit, &intr_dw, 32) { >>>> - const u16 iir = gen11_gt_engine_intr(i915, bank, bit); >>>> + const u32 ident = gen11_gt_engine_identity(i915, >>>> + bank, bit); >>>> + const u16 iir = ident & GEN11_INTR_ENGINE_MASK; >>>> + u8 class, instance; >>>> + struct intel_engine_cs *engine; >>>> >>>> if (unlikely(!iir)) >>>> continue; >>> >>> Now if (!ident) or actually just use u32 iir as we can pass it straight >>> through to cs_irq_handler. >> >> Can't use (!ident) here because bit 31 (iir valid) or the class/instance >> bits might be set when the iir is empty (because we had a buffered irq >> that we actually already handled). > > If there's a valid bit, surely that's the one we want to be testing? > We do already test the valid bit in gen11_gt_engine_intr and return zero from that function if the bit doesn't go to 1 in a reasonable time. > If the low iir bits are 0, that's fine. The question is whether it's > common enough to worry about; and I note it's marked as unlikely() so > it seems like we can just let it fallthrough and do nothing. > -Chris > Difficult to say how common it is going to be yet. If my reading of the specs is correct, double buffering is only active between reading GEN11_GT_INTR_DW and clearing it, so the iir might be empty if the second interrupt lands between reading GEN11_GT_INTR_DW and reading the iir (in which case both events will be handled in the first round). I'm ok with just falling through for now and we can revisit once we have better data. Daniele
Hi, On 3/12/2018 7:41 AM, Mika Kuoppala wrote: > Interrupt identity register we already read from hardware > contains engine class and instance fields. Leverage > these fields to find correct engine to handle the interrupt. > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160) > Function old new delta > gen11_irq_handler 764 604 -160 > Total: Before=1506953, After=1506793, chg -0.01% > > v2: handle class/instance overflow correctly (Mika) > > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 64 ++++++++++++++--------------------------- > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 23 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 828f3104488c..49816d0a380b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2733,47 +2733,9 @@ static void __fini_wedge(struct wedge_me *w) > (W)->i915; \ > __fini_wedge((W))) > > -static void > -gen11_gt_engine_irq_handler(struct drm_i915_private * const i915, > - const unsigned int bank, > - const unsigned int engine_n, > - const u16 iir) > -{ > - struct intel_engine_cs ** const engine = i915->engine; > - > - switch (bank) { > - case 0: > - switch (engine_n) { > - > - case GEN11_RCS0: > - return gen8_cs_irq_handler(engine[RCS], iir); > - > - case GEN11_BCS: > - return gen8_cs_irq_handler(engine[BCS], iir); > - } > - case 1: > - switch (engine_n) { > - > - case GEN11_VCS(0): > - return gen8_cs_irq_handler(engine[_VCS(0)], iir); > - case GEN11_VCS(1): > - return gen8_cs_irq_handler(engine[_VCS(1)], iir); > - case GEN11_VCS(2): > - return gen8_cs_irq_handler(engine[_VCS(2)], iir); > - case GEN11_VCS(3): > - return gen8_cs_irq_handler(engine[_VCS(3)], iir); > - > - case GEN11_VECS(0): > - return gen8_cs_irq_handler(engine[_VECS(0)], iir); > - case GEN11_VECS(1): > - return gen8_cs_irq_handler(engine[_VECS(1)], iir); > - } > - } > -} > - > static u32 > -gen11_gt_engine_intr(struct drm_i915_private * const i915, > - const unsigned int bank, const unsigned int bit) > +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; > @@ -2800,7 +2762,7 @@ gen11_gt_engine_intr(struct drm_i915_private * const i915, > raw_reg_write(regs, GEN11_INTR_IDENTITY_REG(bank), > GEN11_INTR_DATA_VALID); > > - return ident & GEN11_INTR_ENGINE_MASK; > + return ident; > } > > static void > @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, > } > > for_each_set_bit(bit, &intr_dw, 32) { > - const u16 iir = gen11_gt_engine_intr(i915, bank, bit); > + const u32 ident = gen11_gt_engine_identity(i915, > + bank, bit); > + const u16 iir = ident & GEN11_INTR_ENGINE_MASK; > + u8 class, instance; > + struct intel_engine_cs *engine; > > if (unlikely(!iir)) > continue; > > - gen11_gt_engine_irq_handler(i915, bank, bit, iir); > + class = GEN11_INTR_ENGINE_CLASS(ident); > + if (unlikely(class >= MAX_ENGINE_CLASS)) MAX_ENGINE_CLASS points to the last CLASS, so this should be if (unlikely(class > MAX_ENGINE_CLASS)) as you originally had in the v1 of this patch. (GuC interrupts will be reported with class = OTHER_CLASS). > + continue; > + > + instance = GEN11_INTR_ENGINE_INSTANCE(ident); > + if (unlikely(instance >= MAX_ENGINE_INSTANCE)) > + continue; > + Same here, MAX_ENGINE_INSTANCE is inclusive too. That's why we have the GEM_WARN_ON's in intel_engine_setup(). > + engine = i915->engine_class[class][instance]; > + if (unlikely(!engine)) > + continue; > + > + gen8_cs_irq_handler(engine, iir); > } > > /* Clear must be after shared has been served for engine */ > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index e6a8c0ee7df1..065825290482 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7118,6 +7118,8 @@ enum { > #define GEN11_INTR_IDENTITY_REG1 _MMIO(0x190064) > #define GEN11_INTR_DATA_VALID (1 << 31) > #define GEN11_INTR_ENGINE_MASK (0xffff) > +#define GEN11_INTR_ENGINE_CLASS(x) (((x) & GENMASK(18, 16)) >> 16) > +#define GEN11_INTR_ENGINE_INSTANCE(x) (((x) & GENMASK(25, 20)) >> 20) > > #define GEN11_INTR_IDENTITY_REG(x) _MMIO(0x190060 + (x * 4)) > >
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 828f3104488c..49816d0a380b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2733,47 +2733,9 @@ static void __fini_wedge(struct wedge_me *w) (W)->i915; \ __fini_wedge((W))) -static void -gen11_gt_engine_irq_handler(struct drm_i915_private * const i915, - const unsigned int bank, - const unsigned int engine_n, - const u16 iir) -{ - struct intel_engine_cs ** const engine = i915->engine; - - switch (bank) { - case 0: - switch (engine_n) { - - case GEN11_RCS0: - return gen8_cs_irq_handler(engine[RCS], iir); - - case GEN11_BCS: - return gen8_cs_irq_handler(engine[BCS], iir); - } - case 1: - switch (engine_n) { - - case GEN11_VCS(0): - return gen8_cs_irq_handler(engine[_VCS(0)], iir); - case GEN11_VCS(1): - return gen8_cs_irq_handler(engine[_VCS(1)], iir); - case GEN11_VCS(2): - return gen8_cs_irq_handler(engine[_VCS(2)], iir); - case GEN11_VCS(3): - return gen8_cs_irq_handler(engine[_VCS(3)], iir); - - case GEN11_VECS(0): - return gen8_cs_irq_handler(engine[_VECS(0)], iir); - case GEN11_VECS(1): - return gen8_cs_irq_handler(engine[_VECS(1)], iir); - } - } -} - static u32 -gen11_gt_engine_intr(struct drm_i915_private * const i915, - const unsigned int bank, const unsigned int bit) +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; @@ -2800,7 +2762,7 @@ gen11_gt_engine_intr(struct drm_i915_private * const i915, raw_reg_write(regs, GEN11_INTR_IDENTITY_REG(bank), GEN11_INTR_DATA_VALID); - return ident & GEN11_INTR_ENGINE_MASK; + return ident; } static void @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, } for_each_set_bit(bit, &intr_dw, 32) { - const u16 iir = gen11_gt_engine_intr(i915, bank, bit); + const u32 ident = gen11_gt_engine_identity(i915, + bank, bit); + const u16 iir = ident & GEN11_INTR_ENGINE_MASK; + u8 class, instance; + struct intel_engine_cs *engine; if (unlikely(!iir)) continue; - gen11_gt_engine_irq_handler(i915, bank, bit, iir); + class = GEN11_INTR_ENGINE_CLASS(ident); + if (unlikely(class >= MAX_ENGINE_CLASS)) + continue; + + instance = GEN11_INTR_ENGINE_INSTANCE(ident); + if (unlikely(instance >= MAX_ENGINE_INSTANCE)) + continue; + + engine = i915->engine_class[class][instance]; + if (unlikely(!engine)) + continue; + + gen8_cs_irq_handler(engine, iir); } /* Clear must be after shared has been served for engine */ diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index e6a8c0ee7df1..065825290482 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7118,6 +7118,8 @@ enum { #define GEN11_INTR_IDENTITY_REG1 _MMIO(0x190064) #define GEN11_INTR_DATA_VALID (1 << 31) #define GEN11_INTR_ENGINE_MASK (0xffff) +#define GEN11_INTR_ENGINE_CLASS(x) (((x) & GENMASK(18, 16)) >> 16) +#define GEN11_INTR_ENGINE_INSTANCE(x) (((x) & GENMASK(25, 20)) >> 20) #define GEN11_INTR_IDENTITY_REG(x) _MMIO(0x190060 + (x * 4))
Interrupt identity register we already read from hardware contains engine class and instance fields. Leverage these fields to find correct engine to handle the interrupt. add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160) Function old new delta gen11_irq_handler 764 604 -160 Total: Before=1506953, After=1506793, chg -0.01% v2: handle class/instance overflow correctly (Mika) Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 64 ++++++++++++++--------------------------- drivers/gpu/drm/i915/i915_reg.h | 2 ++ 2 files changed, 23 insertions(+), 43 deletions(-)