[Intel-gfx] [PATCH 8/8] drm/i915/icl: Use hw engine class, instance to find irq handler
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Fri Mar 16 19:37:23 UTC 2018
On 16/03/18 11:28, Michel Thierry wrote:
> On 3/16/2018 5:14 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.
>>
>> v3: rebase on top of rps intr
>> use correct class / instance limits (Michel)
>>
>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Michel Thierry <michel.thierry at intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 80
>> +++++++++++++++--------------------------
>> drivers/gpu/drm/i915/i915_reg.h | 4 ++-
>> 2 files changed, 31 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 8c4510ffe625..dd2fb2d0457f 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -413,8 +413,8 @@ static void gen6_disable_pm_irq(struct
>> drm_i915_private *dev_priv, u32 disable_m
>> }
>> 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 gen11_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>> {
>> @@ -428,7 +428,7 @@ void gen11_reset_rps_interrupts(struct
>> drm_i915_private *dev_priv)
>> */
>> dw = I915_READ_FW(GEN11_GT_INTR_DW0);
>> while (dw & BIT(GEN11_GTPM)) {
>> - gen11_gt_engine_intr(dev_priv, 0, GEN11_GTPM);
>> + gen11_gt_engine_identity(dev_priv, 0, GEN11_GTPM);
>> I915_WRITE_FW(GEN11_GT_INTR_DW0, BIT(GEN11_GTPM));
>> dw = I915_READ_FW(GEN11_GT_INTR_DW0);
>> }
>> @@ -2771,50 +2771,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 GEN11_GTPM:
>> - return gen6_rps_irq_handler(i915, 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;
>> @@ -2841,7 +2800,26 @@ 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
>> +gen11_gt_identity_handler(struct drm_i915_private * const i915,
>> + const u32 identity)
>> +{
>> + const u8 class = GEN11_INTR_ENGINE_CLASS(identity);
>> + const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity);
>> + const u16 iir = GEN11_INTR_ENGINE_MASK(identity);
>> +
>> + if (unlikely(!iir))
>> + return;
>> +
>> + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE)
>> + return gen8_cs_irq_handler(i915->engine_class[class][instance],
>> + iir);
>> +
>> + if (class == GEN11_GTPM)
>> + return gen6_rps_irq_handler(i915, iir);
>
> Hi,
>
> GEN11_GTPM should be
> [Class ID] == 'OTHER_CLASS (4)', and
> [Engine Id] == 1.
> (I'm looking at bspec 20944)
>
> So not only the GPTM check needs to be before the
> if (class <= MAX_ENGINE_CLASS),
>
> but it can't use GEN11_GTPM either.
>
> -Michel
>
I'm not fully convinced about checking against MAX_ENGINE_CLASS or
MAX_ENGINE_INSTANCE, since we do have cases that are within those values
but are still invalid (e.g. VCS1). Maybe we can just do something
simpler (that should still be safe) like:
/* OTHER_CLASS is for interrupts not comings from an engine */
if (class != OTHER_CLASS) {
engine = i915->engine_class[class][instance];
if (likely(engine))
return gen8_cs_irq_handler(...);
} else {
if (instance == 1)
return gen6_rps_irq_handler(...);
/* more cases incoming (e.g. GuC) */
}
>
>> }
>> static void
>> @@ -2866,12 +2844,10 @@ 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);
>> -
>> - if (unlikely(!iir))
>> - continue;
>> + const u32 ident = gen11_gt_engine_identity(i915,
>> + bank, bit);
>> - gen11_gt_engine_irq_handler(i915, bank, bit, iir);
>> + gen11_gt_identity_handler(i915, ident);
>> }
>> /* 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 f3cc77690124..74a8f454e8a0 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6826,7 +6826,9 @@ enum {
>> #define GEN11_INTR_IDENTITY_REG0 _MMIO(0x190060)
>> #define GEN11_INTR_IDENTITY_REG1 _MMIO(0x190064)
>> #define GEN11_INTR_DATA_VALID (1 << 31)
>> -#define GEN11_INTR_ENGINE_MASK (0xffff)
>> +#define GEN11_INTR_ENGINE_MASK(x) ((x) & 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))
>>
More information about the Intel-gfx
mailing list