Re: [Intel-gfx] [PATCH 8/8] drm/i915/icl: Use hw engine class, instance to find irq handler

2018-03-19 Thread Daniele Ceraolo Spurio 



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 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Michel Thierry 
Signed-off-by: Mika Kuoppala 
---
  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

Re: [Intel-gfx] [PATCH 8/8] drm/i915/icl: Use hw engine class, instance to find irq handler

2018-03-16 Thread Daniele Ceraolo Spurio 



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 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Michel Thierry 
Signed-off-by: Mika Kuoppala 
---
  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

Re: [Intel-gfx] [PATCH 8/8] drm/i915/icl: Use hw engine class, instance to find irq handler

2018-03-16 Thread Michel Thierry 

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 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Michel Thierry 
Signed-off-by: Mika Kuoppala 
---
  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

[Intel-gfx] [PATCH 8/8] drm/i915/icl: Use hw engine class, instance to find irq handler

2018-03-16 Thread Mika Kuoppala 
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 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Michel Thierry 
Signed-off-by: Mika Kuoppala 
---
 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);
 }
 
 static void
@@ -2866,12 +2844,10 @@ gen11_gt_irq_handler(struct drm_i915_private * const 
i915,
}
 
for_each_set_bit(bit, _dw, 32) {
-   const u16 iir =