Re: [Intel-gfx] [RFC PATCH 03/97] drm/i915/gt: Move CS interrupt handler to the backend

2021-05-25 Thread Tvrtko Ursulin 



On 06/05/2021 20:13, Matthew Brost wrote:

From: Chris Wilson 

The different submission backends each have their own preferred
behaviour and interrupt setup. Let each handle their own interrupts.

This becomes more useful later as we to extract the use of auxiliary
state in the interrupt handler that is backend specific.

Signed-off-by: Matthew Brost 
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 


Same, this patch had my r-b already so I'll repeat it:

Reviewed-by: Tvrtko Ursulin 

Regards,

Tvrtko


---
  drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++
  drivers/gpu/drm/i915/gt/intel_engine_types.h  | 14 +---
  .../drm/i915/gt/intel_execlists_submission.c  | 41 ++
  drivers/gpu/drm/i915/gt/intel_gt_irq.c| 82 ++-
  drivers/gpu/drm/i915/gt/intel_gt_irq.h| 23 ++
  .../gpu/drm/i915/gt/intel_ring_submission.c   |  8 ++
  drivers/gpu/drm/i915/gt/intel_rps.c   |  2 +-
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++-
  drivers/gpu/drm/i915/i915_irq.c   | 10 ++-
  9 files changed, 124 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 0618379b68ca..828e1669f92c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -255,6 +255,11 @@ static void intel_engine_sanitize_mmio(struct 
intel_engine_cs *engine)
intel_engine_set_hwsp_writemask(engine, ~0u);
  }
  
+static void nop_irq_handler(struct intel_engine_cs *engine, u16 iir)

+{
+   GEM_DEBUG_WARN_ON(iir);
+}
+
  static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
  {
const struct engine_info *info = &intel_engines[id];
@@ -292,6 +297,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id)
engine->hw_id = info->hw_id;
engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
  
+	engine->irq_handler = nop_irq_handler;

+
engine->class = info->class;
engine->instance = info->instance;
__sprint_engine_name(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 883bafc44902..9ef349cd5cea 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -402,6 +402,7 @@ struct intel_engine_cs {
u32 irq_enable_mask; /* bitmask to enable ring interrupt */
void(*irq_enable)(struct intel_engine_cs *engine);
void(*irq_disable)(struct intel_engine_cs *engine);
+   void(*irq_handler)(struct intel_engine_cs *engine, u16 iir);
  
  	void		(*sanitize)(struct intel_engine_cs *engine);

int (*resume)(struct intel_engine_cs *engine);
@@ -481,10 +482,9 @@ struct intel_engine_cs {
  #define I915_ENGINE_HAS_PREEMPTION   BIT(2)
  #define I915_ENGINE_HAS_SEMAPHORES   BIT(3)
  #define I915_ENGINE_HAS_TIMESLICES   BIT(4)
-#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(5)
-#define I915_ENGINE_IS_VIRTUAL   BIT(6)
-#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(7)
-#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(8)
+#define I915_ENGINE_IS_VIRTUAL   BIT(5)
+#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(6)
+#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(7)
unsigned int flags;
  
  	/*

@@ -593,12 +593,6 @@ intel_engine_has_timeslices(const struct intel_engine_cs 
*engine)
return engine->flags & I915_ENGINE_HAS_TIMESLICES;
  }
  
-static inline bool

-intel_engine_needs_breadcrumb_tasklet(const struct intel_engine_cs *engine)
-{
-   return engine->flags & I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
-}
-
  static inline bool
  intel_engine_is_virtual(const struct intel_engine_cs *engine)
  {
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 9d2da5ccaef6..8db200422950 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -118,6 +118,7 @@
  #include "intel_engine_stats.h"
  #include "intel_execlists_submission.h"
  #include "intel_gt.h"
+#include "intel_gt_irq.h"
  #include "intel_gt_pm.h"
  #include "intel_gt_requests.h"
  #include "intel_lrc.h"
@@ -2384,6 +2385,45 @@ static void execlists_submission_tasklet(struct 
tasklet_struct *t)
rcu_read_unlock();
  }
  
+static void execlists_irq_handler(struct intel_engine_cs *engine, u16 iir)

+{
+   bool tasklet = false;
+
+   if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {
+   u32 eir;
+
+   /* Upper 16b are the enabling mask, rsvd for internal errors */
+   eir = ENGINE_READ(engine, RING_EIR) & GENMASK(15, 0);
+   ENGINE_TRACE(engine, "CS error: %x\n", eir);
+
+   /* Disable the error interrupt until after the reset */
+   if (likely(eir)) {
+   ENGINE

Re: [Intel-gfx] [RFC PATCH 03/97] drm/i915/gt: Move CS interrupt handler to the backend

2021-05-18 Thread Matthew Brost 
On Thu, May 06, 2021 at 12:13:17PM -0700, Matthew Brost wrote:
> From: Chris Wilson 
> 
> The different submission backends each have their own preferred
> behaviour and interrupt setup. Let each handle their own interrupts.
> 
> This becomes more useful later as we to extract the use of auxiliary
> state in the interrupt handler that is backend specific.
> 
> Signed-off-by: Matthew Brost 
> Signed-off-by: Chris Wilson 
> Cc: Tvrtko Ursulin 

Reviewed-by: Matthew Brost 

> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++
>  drivers/gpu/drm/i915/gt/intel_engine_types.h  | 14 +---
>  .../drm/i915/gt/intel_execlists_submission.c  | 41 ++
>  drivers/gpu/drm/i915/gt/intel_gt_irq.c| 82 ++-
>  drivers/gpu/drm/i915/gt/intel_gt_irq.h| 23 ++
>  .../gpu/drm/i915/gt/intel_ring_submission.c   |  8 ++
>  drivers/gpu/drm/i915/gt/intel_rps.c   |  2 +-
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++-
>  drivers/gpu/drm/i915/i915_irq.c   | 10 ++-
>  9 files changed, 124 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 0618379b68ca..828e1669f92c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -255,6 +255,11 @@ static void intel_engine_sanitize_mmio(struct 
> intel_engine_cs *engine)
>   intel_engine_set_hwsp_writemask(engine, ~0u);
>  }
>  
> +static void nop_irq_handler(struct intel_engine_cs *engine, u16 iir)
> +{
> + GEM_DEBUG_WARN_ON(iir);
> +}
> +
>  static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
>  {
>   const struct engine_info *info = &intel_engines[id];
> @@ -292,6 +297,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
> intel_engine_id id)
>   engine->hw_id = info->hw_id;
>   engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
>  
> + engine->irq_handler = nop_irq_handler;
> +
>   engine->class = info->class;
>   engine->instance = info->instance;
>   __sprint_engine_name(engine);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 883bafc44902..9ef349cd5cea 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -402,6 +402,7 @@ struct intel_engine_cs {
>   u32 irq_enable_mask; /* bitmask to enable ring interrupt */
>   void(*irq_enable)(struct intel_engine_cs *engine);
>   void(*irq_disable)(struct intel_engine_cs *engine);
> + void(*irq_handler)(struct intel_engine_cs *engine, u16 iir);
>  
>   void(*sanitize)(struct intel_engine_cs *engine);
>   int (*resume)(struct intel_engine_cs *engine);
> @@ -481,10 +482,9 @@ struct intel_engine_cs {
>  #define I915_ENGINE_HAS_PREEMPTION   BIT(2)
>  #define I915_ENGINE_HAS_SEMAPHORES   BIT(3)
>  #define I915_ENGINE_HAS_TIMESLICES   BIT(4)
> -#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(5)
> -#define I915_ENGINE_IS_VIRTUAL   BIT(6)
> -#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(7)
> -#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(8)
> +#define I915_ENGINE_IS_VIRTUAL   BIT(5)
> +#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(6)
> +#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(7)
>   unsigned int flags;
>  
>   /*
> @@ -593,12 +593,6 @@ intel_engine_has_timeslices(const struct intel_engine_cs 
> *engine)
>   return engine->flags & I915_ENGINE_HAS_TIMESLICES;
>  }
>  
> -static inline bool
> -intel_engine_needs_breadcrumb_tasklet(const struct intel_engine_cs *engine)
> -{
> - return engine->flags & I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
> -}
> -
>  static inline bool
>  intel_engine_is_virtual(const struct intel_engine_cs *engine)
>  {
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 9d2da5ccaef6..8db200422950 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -118,6 +118,7 @@
>  #include "intel_engine_stats.h"
>  #include "intel_execlists_submission.h"
>  #include "intel_gt.h"
> +#include "intel_gt_irq.h"
>  #include "intel_gt_pm.h"
>  #include "intel_gt_requests.h"
>  #include "intel_lrc.h"
> @@ -2384,6 +2385,45 @@ static void execlists_submission_tasklet(struct 
> tasklet_struct *t)
>   rcu_read_unlock();
>  }
>  
> +static void execlists_irq_handler(struct intel_engine_cs *engine, u16 iir)
> +{
> + bool tasklet = false;
> +
> + if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {
> + u32 eir;
> +
> + /* Upper 16b are the enabling mask, rsvd for internal errors */
> + eir = ENGINE_READ(engine, RING_EIR) & GENMASK(15, 0);
> + ENGINE_TRACE(engine, "CS error: %x\n", eir);
> +
> + /* D

[Intel-gfx] [RFC PATCH 03/97] drm/i915/gt: Move CS interrupt handler to the backend

2021-05-06 Thread Matthew Brost 
From: Chris Wilson 

The different submission backends each have their own preferred
behaviour and interrupt setup. Let each handle their own interrupts.

This becomes more useful later as we to extract the use of auxiliary
state in the interrupt handler that is backend specific.

Signed-off-by: Matthew Brost 
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c |  7 ++
 drivers/gpu/drm/i915/gt/intel_engine_types.h  | 14 +---
 .../drm/i915/gt/intel_execlists_submission.c  | 41 ++
 drivers/gpu/drm/i915/gt/intel_gt_irq.c| 82 ++-
 drivers/gpu/drm/i915/gt/intel_gt_irq.h| 23 ++
 .../gpu/drm/i915/gt/intel_ring_submission.c   |  8 ++
 drivers/gpu/drm/i915/gt/intel_rps.c   |  2 +-
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++-
 drivers/gpu/drm/i915/i915_irq.c   | 10 ++-
 9 files changed, 124 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 0618379b68ca..828e1669f92c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -255,6 +255,11 @@ static void intel_engine_sanitize_mmio(struct 
intel_engine_cs *engine)
intel_engine_set_hwsp_writemask(engine, ~0u);
 }
 
+static void nop_irq_handler(struct intel_engine_cs *engine, u16 iir)
+{
+   GEM_DEBUG_WARN_ON(iir);
+}
+
 static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
 {
const struct engine_info *info = &intel_engines[id];
@@ -292,6 +297,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id)
engine->hw_id = info->hw_id;
engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
 
+   engine->irq_handler = nop_irq_handler;
+
engine->class = info->class;
engine->instance = info->instance;
__sprint_engine_name(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 883bafc44902..9ef349cd5cea 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -402,6 +402,7 @@ struct intel_engine_cs {
u32 irq_enable_mask; /* bitmask to enable ring interrupt */
void(*irq_enable)(struct intel_engine_cs *engine);
void(*irq_disable)(struct intel_engine_cs *engine);
+   void(*irq_handler)(struct intel_engine_cs *engine, u16 iir);
 
void(*sanitize)(struct intel_engine_cs *engine);
int (*resume)(struct intel_engine_cs *engine);
@@ -481,10 +482,9 @@ struct intel_engine_cs {
 #define I915_ENGINE_HAS_PREEMPTION   BIT(2)
 #define I915_ENGINE_HAS_SEMAPHORES   BIT(3)
 #define I915_ENGINE_HAS_TIMESLICES   BIT(4)
-#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(5)
-#define I915_ENGINE_IS_VIRTUAL   BIT(6)
-#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(7)
-#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(8)
+#define I915_ENGINE_IS_VIRTUAL   BIT(5)
+#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(6)
+#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(7)
unsigned int flags;
 
/*
@@ -593,12 +593,6 @@ intel_engine_has_timeslices(const struct intel_engine_cs 
*engine)
return engine->flags & I915_ENGINE_HAS_TIMESLICES;
 }
 
-static inline bool
-intel_engine_needs_breadcrumb_tasklet(const struct intel_engine_cs *engine)
-{
-   return engine->flags & I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
-}
-
 static inline bool
 intel_engine_is_virtual(const struct intel_engine_cs *engine)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 9d2da5ccaef6..8db200422950 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -118,6 +118,7 @@
 #include "intel_engine_stats.h"
 #include "intel_execlists_submission.h"
 #include "intel_gt.h"
+#include "intel_gt_irq.h"
 #include "intel_gt_pm.h"
 #include "intel_gt_requests.h"
 #include "intel_lrc.h"
@@ -2384,6 +2385,45 @@ static void execlists_submission_tasklet(struct 
tasklet_struct *t)
rcu_read_unlock();
 }
 
+static void execlists_irq_handler(struct intel_engine_cs *engine, u16 iir)
+{
+   bool tasklet = false;
+
+   if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {
+   u32 eir;
+
+   /* Upper 16b are the enabling mask, rsvd for internal errors */
+   eir = ENGINE_READ(engine, RING_EIR) & GENMASK(15, 0);
+   ENGINE_TRACE(engine, "CS error: %x\n", eir);
+
+   /* Disable the error interrupt until after the reset */
+   if (likely(eir)) {
+   ENGINE_WRITE(engine, RING_EMR, ~0u);
+   ENGINE_WRITE(engine, RING_EIR, eir);
+   WRITE_ONCE(engine->execlists.error_interrupt, eir)