Re: [Intel-gfx] [PATCH 06/20] drm/i915/icl: Ringbuffer interrupt handling

[Daniele Ceraolo Spurio]

On 13/02/18 08:37, Mika Kuoppala wrote:

From: Tvrtko Ursulin 

Since it is not possible to mask individual engine instances
and they are all permanently unmasked we do not need to do
anything for engine interrupt management.

v2: Rebase.
v3: Remove gen 11 extra check in logical_render_ring_init.
v4: Rebase fixes.
v5: Rebase/refactor.
v6: Rebase.
v7: Rebase.

Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Rodrigo Vivi 
Cc: Daniele Ceraolo Spurio 
---
  drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 
  drivers/gpu/drm/i915/intel_lrc.c | 11 +--
  drivers/gpu/drm/i915/intel_ringbuffer.h  |  5 +
  3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c 
b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index b955f7d7bd0f..69c727f97eb5 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -167,18 +167,22 @@ static void irq_enable(struct intel_engine_cs *engine)
 */
set_bit(ENGINE_IRQ_BREADCRUMB, >irq_posted);
  
-	/* Caller disables interrupts */

-   spin_lock(>i915->irq_lock);
-   engine->irq_enable(engine);
-   spin_unlock(>i915->irq_lock);
+   if (engine->irq_enable) {
+   /* Caller disables interrupts */
+   spin_lock(>i915->irq_lock);
+   engine->irq_enable(engine);
+   spin_unlock(>i915->irq_lock);
+   }
  }
  
  static void irq_disable(struct intel_engine_cs *engine)

  {
-   /* Caller disables interrupts */
-   spin_lock(>i915->irq_lock);
-   engine->irq_disable(engine);
-   spin_unlock(>i915->irq_lock);
+   if (engine->irq_disable) {
+   /* Caller disables interrupts */
+   spin_lock(>i915->irq_lock);
+   engine->irq_disable(engine);
+   spin_unlock(>i915->irq_lock);
+   }
  }
  
  void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c2c8380a0121..da396c46b345 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1989,8 +1989,15 @@ logical_ring_default_vfuncs(struct intel_engine_cs 
*engine)
  
  	engine->set_default_submission = execlists_set_default_submission;
  
-	engine->irq_enable = gen8_logical_ring_enable_irq;

-   engine->irq_disable = gen8_logical_ring_disable_irq;
+   if (INTEL_GEN(engine->i915) < 11) {
+   engine->irq_enable = gen8_logical_ring_enable_irq;
+   engine->irq_disable = gen8_logical_ring_disable_irq;
+   } else {
+   /*
+* On Gen11 interrupts are permanently unmasked and there
+* are no per-engine instance mask bits.
+*/


As mentioned on the previous review, the masks exist but we can't use 
them easily, because according to the docs we need to clear them to 
allow C6 entry. My guess is that it is actually the possible pending 
interrupts behind the mask that can keep the device awake more than the 
masks value themselves, but the docs just say to clear the registers. 
Maybe a possible solution would be to track idleness per-engine and 
unmask the interrupts once all requests have completed on the specific 
engine? Anyway, while we're still in early enabling I'd just update this 
comment and the commit message to reflect that we don't use the mask to 
allow C6 entry and we can eventually come back to refine this at a later 
point if we see that we're generating too many interrupts.


With the updated comment and commit message:

Reviewed-by: Daniele Ceraolo Spurio 

Daniele


+   }
engine->emit_bb_start = gen8_emit_bb_start;
  }
  
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h

index f743351c441f..caed64bb02da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -393,6 +393,11 @@ struct intel_engine_cs {
  
  	u32 irq_keep_mask; /* always keep these interrupts */

u32 irq_enable_mask; /* bitmask to enable ring interrupt */
+
+   /*
+* irq_enable and irq_disable do not have to be provided for
+* an engine. In other words they can be NULL.
+*/
void(*irq_enable)(struct intel_engine_cs *engine);
void(*irq_disable)(struct intel_engine_cs *engine);
  


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

[Intel-gfx] [PATCH 06/20] drm/i915/icl: Ringbuffer interrupt handling

[Mika Kuoppala]

From: Tvrtko Ursulin 

Since it is not possible to mask individual engine instances
and they are all permanently unmasked we do not need to do
anything for engine interrupt management.

v2: Rebase.
v3: Remove gen 11 extra check in logical_render_ring_init.
v4: Rebase fixes.
v5: Rebase/refactor.
v6: Rebase.
v7: Rebase.

Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Rodrigo Vivi 
Cc: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 
 drivers/gpu/drm/i915/intel_lrc.c | 11 +--
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  5 +
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c 
b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index b955f7d7bd0f..69c727f97eb5 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -167,18 +167,22 @@ static void irq_enable(struct intel_engine_cs *engine)
 */
set_bit(ENGINE_IRQ_BREADCRUMB, >irq_posted);
 
-   /* Caller disables interrupts */
-   spin_lock(>i915->irq_lock);
-   engine->irq_enable(engine);
-   spin_unlock(>i915->irq_lock);
+   if (engine->irq_enable) {
+   /* Caller disables interrupts */
+   spin_lock(>i915->irq_lock);
+   engine->irq_enable(engine);
+   spin_unlock(>i915->irq_lock);
+   }
 }
 
 static void irq_disable(struct intel_engine_cs *engine)
 {
-   /* Caller disables interrupts */
-   spin_lock(>i915->irq_lock);
-   engine->irq_disable(engine);
-   spin_unlock(>i915->irq_lock);
+   if (engine->irq_disable) {
+   /* Caller disables interrupts */
+   spin_lock(>i915->irq_lock);
+   engine->irq_disable(engine);
+   spin_unlock(>i915->irq_lock);
+   }
 }
 
 void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c2c8380a0121..da396c46b345 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1989,8 +1989,15 @@ logical_ring_default_vfuncs(struct intel_engine_cs 
*engine)
 
engine->set_default_submission = execlists_set_default_submission;
 
-   engine->irq_enable = gen8_logical_ring_enable_irq;
-   engine->irq_disable = gen8_logical_ring_disable_irq;
+   if (INTEL_GEN(engine->i915) < 11) {
+   engine->irq_enable = gen8_logical_ring_enable_irq;
+   engine->irq_disable = gen8_logical_ring_disable_irq;
+   } else {
+   /*
+* On Gen11 interrupts are permanently unmasked and there
+* are no per-engine instance mask bits.
+*/
+   }
engine->emit_bb_start = gen8_emit_bb_start;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f743351c441f..caed64bb02da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -393,6 +393,11 @@ struct intel_engine_cs {
 
u32 irq_keep_mask; /* always keep these interrupts */
u32 irq_enable_mask; /* bitmask to enable ring interrupt */
+
+   /*
+* irq_enable and irq_disable do not have to be provided for
+* an engine. In other words they can be NULL.
+*/
void(*irq_enable)(struct intel_engine_cs *engine);
void(*irq_disable)(struct intel_engine_cs *engine);
 
-- 
2.14.1

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