[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
Mon Mar 19 15:14:08 UTC 2018
On 16/03/18 12:37, Daniele Ceraolo Spurio wrote:
>
>
> 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:
>
Thinking about this again you were right, we do need a check for
MAX_ENGINE_CLASS and MAX_ENGINE_INSTANCE to avoid overflowing the
engine_class array. However, while the MAX_ENGINE_CLASS check applies to
all cases, the MAX_ENGINE_INSTANCE one only applies for class !=
OTHER_CLASS, so we may need to split them accordingly.
Daniele
> /* 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))
>>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list