Re: [Intel-gfx] [PATCH 05/19] drm/i915/icl: Interrupt handling

[Daniele Ceraolo Spurio]

+
+static void
+gen11_gt_irq_handler(struct drm_i915_private * const i915,
+const u32 master_ctl)
+{
+   void __iomem * const regs = i915->regs;
+   unsigned int bank;
+
+   for (bank = 0; bank < 2; bank++) {
+   unsigned long intr_dw;
+   unsigned int bit;
+
+   if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
+   continue;
+
+   intr_dw = readl(regs +
+   i915_mmio_reg_offset(GEN11_GT_INTR_DW(bank)));
+


I think we should keep some kind of polling here, because there is going 
to be some delay between writing the selector and getting the correct 
value in the identity register and the CPU might also be running at a 
higher frequency than the GPU. Spec does not specify a time but implies 
that we have to wait for the valid bit to be set.



+   if (unlikely(!intr_dw))
+   DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
+
+   for_each_set_bit(bit, _dw, 32) {
+   const u16 iir = gen11_gt_engine_intr(i915, bank, bit);
+
+   if (unlikely(!iir))
+   continue;
+
+   gen11_gt_engine_irq_handler(i915, bank, bit, iir);


The identity register contains the class (bits 16-18) and instance (bits 
20-25), so we can potentially decode them and call directly:


gen11_cs_irq_handler(i915->engine_class[class][instance], iir);

instead of having a big switch. The only problem with that would be that 
GuC and PM interrupts come out as class 4 instances 0 and 1, so once we 
add support for those we'd have to do something like


if (class != OTHER_CLASS)
gen11_cs_irq_handler(...);
else {
switch (instance) {
case 0:
gen11_guc_irq_handler(...);
break;
case 1:
gen11_rps_irq_handler(...);
break;
default:
MISSING_CASE();
}
}

not sure if decoding class and instance and having a small switch wins 
in the end against the big switch, so I won't complain whichever you pick ;)


Daniele


+   }
+
+   /* Clear must be after shared has been served for engine */
+   writel(intr_dw,
+  regs + i915_mmio_reg_offset(GEN11_GT_INTR_DW(bank)));
+   }
+}
+

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/19] drm/i915/icl: Interrupt handling

[Tvrtko Ursulin]

On 15/02/2018 16:27, Mika Kuoppala wrote:

From: Tvrtko Ursulin 


I think a point has arrived when I almost don't recognize the code any 
longer so would want to relinquish the authorship if you don't mind.


Regards,

Tvrtko



v2: Rebase.

v3:
   * Remove DPF, it has been removed from SKL+.
   * Fix -internal rebase wrt. execlists interrupt handling.

v4: Rebase.

v5:
   * Updated for POR changes. (Daniele Ceraolo Spurio)
   * Merged with irq handling fixes by Daniele Ceraolo Spurio:
   * Simplify the code by using gen8_cs_irq_handler.
   * Fix interrupt handling for the upstream kernel.

v6:
   * Remove early bringup debug messages (Tvrtko)
   * Add NB about arbitrary spin wait timeout (Tvrtko)

v7 (from Paulo):
   * Don't try to write RO bits to registers.
   * Don't check for PCH types that don't exist. PCH interrupts are not
 here yet.

v9:
   * squashed in selector and shared register handling (Daniele)
   * skip writing of irq if data is not valid (Daniele)
   * use time_after32 (Chris)
   * use I915_MAX_VCS and I915_MAX_VECS (Daniele)
   * remove fake pm interrupt handling for later patch (Mika)

v10:
   * Direct processing of banks. clear banks early (Chris)
   * remove poll on valid bit, only clear valid bit (Mika)
   * use raw accessors, better naming (Chris)

Cc: Tvrtko Ursulin 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Oscar Mateo 
Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: Daniele Ceraolo Spurio 
Signed-off-by: Oscar Mateo 
Signed-off-by: Paulo Zanoni 
Signed-off-by: Mika Kuoppala 
---
  drivers/gpu/drm/i915/i915_irq.c | 222 
  drivers/gpu/drm/i915/intel_pm.c |   7 +-
  2 files changed, 228 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b886bd459acc..e3914d1e204e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -415,6 +415,9 @@ void gen6_enable_rps_interrupts(struct drm_i915_private 
*dev_priv)
if (READ_ONCE(rps->interrupts_enabled))
return;
  
+	if (WARN_ON_ONCE(IS_GEN11(dev_priv)))

+   return;
+
spin_lock_irq(_priv->irq_lock);
WARN_ON_ONCE(rps->pm_iir);
WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & 
dev_priv->pm_rps_events);
@@ -431,6 +434,9 @@ void gen6_disable_rps_interrupts(struct drm_i915_private 
*dev_priv)
if (!READ_ONCE(rps->interrupts_enabled))
return;
  
+	if (WARN_ON_ONCE(IS_GEN11(dev_priv)))

+   return;
+
spin_lock_irq(_priv->irq_lock);
rps->interrupts_enabled = false;
  
@@ -2746,6 +2752,143 @@ static void __fini_wedge(struct wedge_me *w)

 (W)->i915;  \
 __fini_wedge((W)))
  
+static __always_inline void

+gen11_cs_irq_handler(struct intel_engine_cs * const engine, const u32 iir)
+{
+   gen8_cs_irq_handler(engine, iir, 0);
+}
+
+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 gen11_cs_irq_handler(engine[RCS], iir);
+
+   case GEN11_BCS:
+   return gen11_cs_irq_handler(engine[BCS], iir);
+   }
+   case 1:
+   switch (engine_n) {
+
+   case GEN11_VCS(0):
+   return gen11_cs_irq_handler(engine[_VCS(0)], iir);
+   case GEN11_VCS(1):
+   return gen11_cs_irq_handler(engine[_VCS(1)], iir);
+   case GEN11_VCS(2):
+   return gen11_cs_irq_handler(engine[_VCS(2)], iir);
+   case GEN11_VCS(3):
+   return gen11_cs_irq_handler(engine[_VCS(3)], iir);
+
+   case GEN11_VECS(0):
+   return gen11_cs_irq_handler(engine[_VECS(0)], iir);
+   case GEN11_VECS(1):
+   return gen11_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)
+{
+   void __iomem * const regs = i915->regs;
+   u32 ident;
+
+   writel(BIT(bit),
+  regs + i915_mmio_reg_offset(GEN11_IIR_REG_SELECTOR(bank)));
+
+   ident = readl(regs 

Re: [Intel-gfx] [PATCH 05/19] drm/i915/icl: Interrupt handling

[Mika Kuoppala]

Chris Wilson  writes:

> Quoting Mika Kuoppala (2018-02-14 14:12:13)
>> From: Tvrtko Ursulin 
>> 
>> v2: Rebase.
>> 
>> v3:
>>   * Remove DPF, it has been removed from SKL+.
>>   * Fix -internal rebase wrt. execlists interrupt handling.
>> 
>> v4: Rebase.
>> 
>> v5:
>>   * Updated for POR changes. (Daniele Ceraolo Spurio)
>>   * Merged with irq handling fixes by Daniele Ceraolo Spurio:
>>   * Simplify the code by using gen8_cs_irq_handler.
>>   * Fix interrupt handling for the upstream kernel.
>> 
>> v6:
>>   * Remove early bringup debug messages (Tvrtko)
>>   * Add NB about arbitrary spin wait timeout (Tvrtko)
>> 
>> v7 (from Paulo):
>>   * Don't try to write RO bits to registers.
>>   * Don't check for PCH types that don't exist. PCH interrupts are not
>> here yet.
>> 
>> v9:
>>   * squashed in selector and shared register handling (Daniele)
>>   * skip writing of irq if data is not valid (Daniele)
>>   * use time_after32 (Chris)
>>   * use I915_MAX_VCS and I915_MAX_VECS (Daniele)
>>   * remove fake pm interrupt handling for later patch (Mika)
>> 
>> Cc: Tvrtko Ursulin 
>> Cc: Daniele Ceraolo Spurio 
>> Cc: Chris Wilson 
>> Cc: Oscar Mateo 
>> Signed-off-by: Tvrtko Ursulin 
>> Signed-off-by: Rodrigo Vivi 
>> Signed-off-by: Daniele Ceraolo Spurio 
>> Signed-off-by: Oscar Mateo 
>> Signed-off-by: Paulo Zanoni 
>> Signed-off-by: Mika Kuoppala 
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 212 
>> 
>>  drivers/gpu/drm/i915/intel_pm.c |   7 +-
>>  2 files changed, 218 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index b886bd459acc..9a2d12c8c44c 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -408,6 +408,37 @@ void gen6_reset_rps_interrupts(struct drm_i915_private 
>> *dev_priv)
>> spin_unlock_irq(_priv->irq_lock);
>>  }
>>  
>> +static int gen11_service_shared_iir(struct drm_i915_private *dev_priv,
>> +   const unsigned int bank,
>> +   const unsigned int bit)
>> +{
>> +   u64 wait_end;
>> +   u32 ident;
>> +   int irq;
>> +
>> +   I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
>> +   /*
>> +* NB: Specs do not specify how long to spin wait.
>> +* Taking 100us as an educated guess
>> +*/
>> +   wait_end = (local_clock() >> 10) + 100;
>> +   do {
>> +   ident = I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
>> +   } while (!(ident & GEN11_INTR_DATA_VALID) &&
>> +!time_after32(local_clock() >> 10, wait_end));
>
> Now you are just mixing types willy nilly :)
>
> No need for wait_end to be 64b when we are looking at a 100 interval.

I threw the poll away. We can add it later if someone objects
and/or evidence indicates that it is needed. But polling for 100us just
in case in irq handler should not be the first step.

>
>> +
>> +   if (!(ident & GEN11_INTR_DATA_VALID)) {
>> +   DRM_ERROR("INTR_IDENTITY_REG%u:%u timed out!\n", bank, bit);
>> +   return -ETIMEDOUT;
>> +   }
>> +
>> +   irq = ident & GEN11_INTR_ENGINE_MASK;
>> +
>> + I915_WRITE_FW(GEN11_INTR_IDENTITY_REG(bank), ident);

Bspec tells that valid bit write should be enough here.

>> +
>> +   return irq;
>
> return ident & GEN11_INTR_ENGINE_MASK;
>
> no need for irq, and why int return type?

I made it return zero on unvalid.

>
> Why is this gen11 specific helper so far away from the irq_handler?

Due to later patch in this series needing it, rc6 enabling
due to rps reset.

I moved those closer in both patches.

>
>> +static __always_inline void
>> +gen11_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
>> +{
>> +   gen8_cs_irq_handler(engine, iir, 0);
>> +}
>> +
>> +static void
>> +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, const u32 
>> master_ctl)
>> +{
>> +   u16 irq[2][32];
>> +   unsigned int bank, engine;
>> +
>> +   memset(irq, 0, sizeof(irq));
>> +
>> +   for (bank = 0; bank < 2; bank++) {
>> +   unsigned long tmp;
>> +   unsigned int bit;
>> +   u32 dw;
>> +   int ret;
>> +
>> +   if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
>> +   continue;
>> +
>> +   dw = I915_READ_FW(GEN11_GT_INTR_DW(bank));
>> +   if (!dw)
>> +   DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
>> +
>> +   tmp = dw;
>> +   for_each_set_bit(bit, , 32) {
>
> tmp is not required here.

reading directly now to 

Re: [Intel-gfx] [PATCH 05/19] drm/i915/icl: Interrupt handling

[Chris Wilson]

Quoting Mika Kuoppala (2018-02-14 14:12:13)
> From: Tvrtko Ursulin 
> 
> v2: Rebase.
> 
> v3:
>   * Remove DPF, it has been removed from SKL+.
>   * Fix -internal rebase wrt. execlists interrupt handling.
> 
> v4: Rebase.
> 
> v5:
>   * Updated for POR changes. (Daniele Ceraolo Spurio)
>   * Merged with irq handling fixes by Daniele Ceraolo Spurio:
>   * Simplify the code by using gen8_cs_irq_handler.
>   * Fix interrupt handling for the upstream kernel.
> 
> v6:
>   * Remove early bringup debug messages (Tvrtko)
>   * Add NB about arbitrary spin wait timeout (Tvrtko)
> 
> v7 (from Paulo):
>   * Don't try to write RO bits to registers.
>   * Don't check for PCH types that don't exist. PCH interrupts are not
> here yet.
> 
> v9:
>   * squashed in selector and shared register handling (Daniele)
>   * skip writing of irq if data is not valid (Daniele)
>   * use time_after32 (Chris)
>   * use I915_MAX_VCS and I915_MAX_VECS (Daniele)
>   * remove fake pm interrupt handling for later patch (Mika)
> 
> Cc: Tvrtko Ursulin 
> Cc: Daniele Ceraolo Spurio 
> Cc: Chris Wilson 
> Cc: Oscar Mateo 
> Signed-off-by: Tvrtko Ursulin 
> Signed-off-by: Rodrigo Vivi 
> Signed-off-by: Daniele Ceraolo Spurio 
> Signed-off-by: Oscar Mateo 
> Signed-off-by: Paulo Zanoni 
> Signed-off-by: Mika Kuoppala 
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 212 
> 
>  drivers/gpu/drm/i915/intel_pm.c |   7 +-
>  2 files changed, 218 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b886bd459acc..9a2d12c8c44c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -408,6 +408,37 @@ void gen6_reset_rps_interrupts(struct drm_i915_private 
> *dev_priv)
> spin_unlock_irq(_priv->irq_lock);
>  }
>  
> +static int gen11_service_shared_iir(struct drm_i915_private *dev_priv,
> +   const unsigned int bank,
> +   const unsigned int bit)
> +{
> +   u64 wait_end;
> +   u32 ident;
> +   int irq;
> +
> +   I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
> +   /*
> +* NB: Specs do not specify how long to spin wait.
> +* Taking 100us as an educated guess
> +*/
> +   wait_end = (local_clock() >> 10) + 100;
> +   do {
> +   ident = I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
> +   } while (!(ident & GEN11_INTR_DATA_VALID) &&
> +!time_after32(local_clock() >> 10, wait_end));

Now you are just mixing types willy nilly :)

No need for wait_end to be 64b when we are looking at a 100 interval.

> +
> +   if (!(ident & GEN11_INTR_DATA_VALID)) {
> +   DRM_ERROR("INTR_IDENTITY_REG%u:%u timed out!\n", bank, bit);
> +   return -ETIMEDOUT;
> +   }
> +
> +   irq = ident & GEN11_INTR_ENGINE_MASK;
> +
> +   I915_WRITE_FW(GEN11_INTR_IDENTITY_REG(bank), ident);
> +
> +   return irq;

return ident & GEN11_INTR_ENGINE_MASK;

no need for irq, and why int return type?

Why is this gen11 specific helper so far away from the irq_handler?

> +static __always_inline void
> +gen11_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> +{
> +   gen8_cs_irq_handler(engine, iir, 0);
> +}
> +
> +static void
> +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, const u32 master_ctl)
> +{
> +   u16 irq[2][32];
> +   unsigned int bank, engine;
> +
> +   memset(irq, 0, sizeof(irq));
> +
> +   for (bank = 0; bank < 2; bank++) {
> +   unsigned long tmp;
> +   unsigned int bit;
> +   u32 dw;
> +   int ret;
> +
> +   if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
> +   continue;
> +
> +   dw = I915_READ_FW(GEN11_GT_INTR_DW(bank));
> +   if (!dw)
> +   DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> +
> +   tmp = dw;
> +   for_each_set_bit(bit, , 32) {

tmp is not required here.

> +   ret = gen11_service_shared_iir(dev_priv, bank, bit);
> +   if (unlikely(ret < 0))
> +   continue;
> +
> +   irq[bank][bit] = ret;
> +   }
> +
> +   I915_WRITE_FW(GEN11_GT_INTR_DW(bank), dw);

If we process the banks here, we won't need to memset 128 bytes and scan
untouched cachelines!

> +   }
> +
> +   if (irq[0][GEN11_RCS0])
> +   gen11_cs_irq_handler(dev_priv->engine[RCS],
> +irq[0][GEN11_RCS0]);
> +
> +   if (irq[0][GEN11_BCS])
>