drm/i915/gen11: Preempt-to-idle support in execlists.

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h          |  2 ++
 drivers/gpu/drm/i915/i915_pci.c          |  3 ++-
 drivers/gpu/drm/i915/intel_device_info.h |  1 +
 drivers/gpu/drm/i915/intel_lrc.c         | 45 +++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_lrc.h         |  1 +
 5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private *dev_priv)
                ((dev_priv)->info.has_logical_ring_elsq)
 #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
 
 #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
        GEN(11), \
        .ddb_size = 2048, \
        .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
 
 static const struct intel_device_info intel_icelake_11_info = {
        GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
        func(has_logical_ring_contexts); \
        func(has_logical_ring_elsq); \
        func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
        func(has_overlay); \
        func(has_pooled_eu); \
        func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
 #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
 #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
 #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
 
 #define GEN8_CTX_STATUS_COMPLETED_MASK \
         (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c +5 -0

+                prio > max(rq_prio(last), 0);
 }
 
 /**

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
        execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
 }
 
+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+

drivers/gpu/drm/i915/intel_lrc.c +1 -0

                                execlists_set_active(execlists,
                                                     EXECLISTS_ACTIVE_HWACK);
+
                        if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
                                execlists_clear_active(execlists,
                                                       EXECLISTS_ACTIVE_HWACK);

@@ -976,8 +1004,13 @@ static void execlists_submission_tasklet(unsigned long data)
                        /* We should never get a COMPLETED | IDLE_ACTIVE! */
                        GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 71 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
  drivers/gpu/drm/i915/i915_pci.c          |  3 ++-
  drivers/gpu/drm/i915/intel_device_info.h |  1 +
  drivers/gpu/drm/i915/intel_lrc.c         | 45 +++++++++++++++++++++++++++-----
  drivers/gpu/drm/i915/intel_lrc.h         |  1 +
  5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private *dev_priv)
                 ((dev_priv)->info.has_logical_ring_elsq)
  #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                 ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
 
  #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
         GEN(11), \
         .ddb_size = 2048, \
         .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
 
  static const struct intel_device_info intel_icelake_11_info = {
         GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
         func(has_logical_ring_contexts); \
         func(has_logical_ring_elsq); \
         func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
         func(has_overlay); \
         func(has_pooled_eu); \
         func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
  #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
  #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
  #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
 

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct intel_engine_cs *engine, · line 188

Quoted 9 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

  #define GEN8_CTX_STATUS_COMPLETED_MASK \
          (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
                                 const struct i915_request *last,
                                 int prio)
  {
-       return engine->i915->preempt_context && prio > max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

But at any rate, making this an engine->flag would eliminate one pointer dance.

Quoted 9 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
  }
 
  /**

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
         execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
  }
 

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -0

+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +10 -0

+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);
+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

Quoted 5 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -962,10 +987,13 @@ static void execlists_submission_tasklet(unsigned long data)
                                   status, buf[2*head + 1],
                                   execlists->active);
 

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 993

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -2

-                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-                                     GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or preempted to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+                                       GEN8_CTX_STATUS_PREEMPTED)) &&
+                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

                                 execlists_set_active(execlists,
                                                      EXECLISTS_ACTIVE_HWACK);
+
                         if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
                                 execlists_clear_active(execlists,
                                                        EXECLISTS_ACTIVE_HWACK);

@@ -976,8 +1004,13 @@ static void execlists_submission_tasklet(unsigned long data)
                         /* We should never get a COMPLETED | IDLE_ACTIVE! */
                         GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 1014

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +7 -2

-                       if (status & GEN8_CTX_STATUS_COMPLETE &&
-                           buf[2*head + 1] == execlists->preempt_complete_status) {
+                       /*
+                        * Check if preempted to real idle, either directly or
+                        * the preemptive context already finished executing
+                        */
+                       if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) ||
+                           (status & GEN8_CTX_STATUS_COMPLETE &&
+                           buf[2*head + 1] == execlists->preempt_complete_status)) {
                                 GEM_TRACE("%s preempt-idle\n", engine->name);

Hmm. I was hoping that we would be able to engineer a single check to cover all sins. Might have been overly optimistic, but I can dream. -Chris

Quoted 72 lines

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 71 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
  drivers/gpu/drm/i915/i915_pci.c          |  3 ++-
  drivers/gpu/drm/i915/intel_device_info.h |  1 +
  drivers/gpu/drm/i915/intel_lrc.c         | 45 +++++++++++++++++++++++++++-----
  drivers/gpu/drm/i915/intel_lrc.h         |  1 +
  5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private *dev_priv)
                 ((dev_priv)->info.has_logical_ring_elsq)
  #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                 ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
 
  #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
         GEN(11), \
         .ddb_size = 2048, \
         .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
 
  static const struct intel_device_info intel_icelake_11_info = {
         GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
         func(has_logical_ring_contexts); \
         func(has_logical_ring_elsq); \
         func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
         func(has_overlay); \
         func(has_pooled_eu); \
         func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
  #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
  #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
  #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
 

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct intel_engine_cs *engine, · line 188

Quoted 9 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

  #define GEN8_CTX_STATUS_COMPLETED_MASK \
          (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
                                 const struct i915_request *last,
                                 int prio)
  {
-       return engine->i915->preempt_context && prio > max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

Yes.. I had mixed feelings about changing needs_preempt_context() now, as that would mean adding a temporary condition on GuC until the GuC preemption is merged.
I will add the conditions and disable the allocation in v2 of the patch.

Quoted 2 lines

But at any rate, making this an engine->flag would eliminate one pointer dance.

Could be an interesting idea for a separate patch.

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
  }
 
  /**

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
         execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
  }
 

Quoted 16 lines

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -0

+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +10 -0

+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);
+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

My arguments for separate function are:
- better code readability
- keeping the symmetry between execlist and GuC flow - GuC preemption patches will introduce separate function as well
- only 4 lines of the function would be common
- the name inject_preempt_context() wouldn't match the new purpose, so renaming would be needed
- reduced self-documenting code due to two separate preempt methods not having distinct names

That's all, I don't have any future plans for it. If you want me to merge the two, let me know.

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -962,10 +987,13 @@ static void execlists_submission_tasklet(unsigned long data)
                                   status, buf[2*head + 1],
                                   execlists->active);
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 993

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -2

-                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-                                     GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or preempted to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+                                       GEN8_CTX_STATUS_PREEMPTED)) &&
+                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

I will check if lack of the change affects test results. Personally, I would keep this change, even if only for allowing simple definition of what HWACK flag means.

Quoting Lis, Tomasz (2018-03-28 17:06:58)

Quoted 84 lines · Chris Wilson

Quoted 82 lines

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 74 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
   drivers/gpu/drm/i915/i915_drv.h          |  2 ++
   drivers/gpu/drm/i915/i915_pci.c          |  3 ++-
   drivers/gpu/drm/i915/intel_device_info.h |  1 +
   drivers/gpu/drm/i915/intel_lrc.c         | 45 +++++++++++++++++++++++++++-----
   drivers/gpu/drm/i915/intel_lrc.h         |  1 +
   5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private *dev_priv)
                  ((dev_priv)->info.has_logical_ring_elsq)
   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                  ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
 
   #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
          GEN(11), \
          .ddb_size = 2048, \
          .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
 
   static const struct intel_device_info intel_icelake_11_info = {
          GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
          func(has_logical_ring_contexts); \
          func(has_logical_ring_elsq); \
          func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
          func(has_overlay); \
          func(has_pooled_eu); \
          func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
   #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
   #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
   #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
 
   #define GEN8_CTX_STATUS_COMPLETED_MASK \
           (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct intel_engine_cs *engine, · line 188

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
                                  const struct i915_request *last,
                                  int prio)
   {
-       return engine->i915->preempt_context && prio > max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

Quoted 7 lines

Yes.. I had mixed feelings about changing needs_preempt_context() now, as that would mean adding a temporary condition on GuC until the GuC preemption is merged.
I will add the conditions and disable the allocation in v2 of the patch.

Quoted 2 lines

But at any rate, making this an engine->flag would eliminate one pointer dance.

Could be an interesting idea for a separate patch.

To land first ;)

Quoted 34 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
   }
 
   /**

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
          execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
   }
 

Quoted 16 lines

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -0

+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +10 -0

+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);
+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

My arguments for separate function are:
- better code readability
- keeping the symmetry between execlist and GuC flow - GuC preemption patches will introduce separate function as well
- only 4 lines of the function would be common
- the name inject_preempt_context() wouldn't match the new purpose, so renaming would be needed
- reduced self-documenting code due to two separate preempt methods not having distinct names

Quoted 2 lines

That's all, I don't have any future plans for it. If you want me to merge the two, let me know.

The problem that I am worrying about is that we will duplicate bunch of other code, the actual ELS[PQ] write is the smaller portion. Plus we already have the branch on something much more pleasant.

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -962,10 +987,13 @@ static void execlists_submission_tasklet(unsigned long data)
                                    status, buf[2*head + 1],
                                    execlists->active);
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 993

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -2

-                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-                                     GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or preempted to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+                                       GEN8_CTX_STATUS_PREEMPTED)) &&
+                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

Quoted 3 lines

I will check if lack of the change affects test results. Personally, I would keep this change, even if only for allowing simple definition of what HWACK flag means.

The simple definition is the opposite one, imo. We set the flag after we get the corresponding response from HW; any preemption or activate event must follow the most recent ELSP write. So that will include the preemption event following the preempt-idle write.

Then on deciding that the HW is idle, we apply the complication such that execlists->active == 0. (That rule is what breaks the pattern.) -Chris

Quoted 86 lines · Chris Wilson

Quoting Lis, Tomasz (2018-03-28 17:06:58)

Quoted 85 lines · Chris Wilson

Quoted 83 lines

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 73 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
   drivers/gpu/drm/i915/i915_drv.h          |  2 ++ drivers/gpu/drm/i915/i915_pci.c          |  3 ++- drivers/gpu/drm/i915/intel_device_info.h |  1 + drivers/gpu/drm/i915/intel_lrc.c         | 45
+++++++++++++++++++++++++++-----
   drivers/gpu/drm/i915/intel_lrc.h         |  1 +

   5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
                   ((dev_priv)->info.has_logical_ring_elsq)
    #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                   ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
    #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
           GEN(11), \
           .ddb_size = 2048, \
           .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
    static const struct intel_device_info intel_icelake_11_info = {
           GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
           func(has_logical_ring_contexts); \
           func(has_logical_ring_elsq); \
           func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
           func(has_overlay); \
           func(has_pooled_eu); \
           func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
    #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
    #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
    #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
    #define GEN8_CTX_STATUS_COMPLETED_MASK \
            (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct · line 190

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct 
 intel_engine_cs *engine,
                                   const struct i915_request *last,
                                   int prio)
    {
-       return engine->i915->preempt_context && prio > 
 max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

Quoted 8 lines

Quoted 7 lines

Yes.. I had mixed feelings about changing needs_preempt_context() now, as that would mean adding a temporary condition on GuC until the GuC preemption is merged.
I will add the conditions and disable the allocation in v2 of the patch.

Quoted 2 lines

But at any rate, making this an engine->flag would eliminate one pointer dance.

Could be an interesting idea for a separate patch.

To land first ;)

:)
Sure, I can do that.

Quoted 33 lines

Quoted 33 lines

Quoted 15 lines

Quoted 15 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
    }

   /**

drivers/gpu/drm/i915/intel_lrc.c +8 -0

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct 
 intel_engine_cs *engine)
           execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
    }
+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+
+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +6 -0

+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);
+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

My arguments for separate function are:
- better code readability
- keeping the symmetry between execlist and GuC flow - GuC preemption patches will introduce separate function as well
- only 4 lines of the function would be common
- the name inject_preempt_context() wouldn't match the new purpose, so renaming would be needed
- reduced self-documenting code due to two separate preempt methods not having distinct names

Quoted 5 lines

Quoted 2 lines

That's all, I don't have any future plans for it. If you want me to merge the two, let me know.

The problem that I am worrying about is that we will duplicate bunch of other code, the actual ELS[PQ] write is the smaller portion. Plus we already have the branch on something much more pleasant.

I see. I don't know any details there, so I'm not able to weigh that.
Just let me know whether this possible duplication outweights the arguments I provided, and I will merge these functions. I'm not overly attached to my solution.

Quoted 22 lines

Quoted 14 lines

Quoted 14 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -2

@@ -962,10 +987,13 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                                     status, buf[2*head + 1],
                                     execlists->active);
-                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-                                     GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or preempted 

Quoted 4 lines

to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE | +                                       GEN8_CTX_STATUS_PREEMPTED)) && +                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

Quoted 3 lines

I will check if lack of the change affects test results. Personally, I would keep this change, even if only for allowing simple definition of what HWACK flag means.

The simple definition is the opposite one, imo. We set the flag after we get the corresponding response from HW; any preemption or activate event must follow the most recent ELSP write. So that will include the preemption event following the preempt-idle write.

Quoted 3 lines

Then on deciding that the HW is idle, we apply the complication such that execlists->active == 0. (That rule is what breaks the pattern.) -Chris

Ok, I will remove this unnecessary condition.
I tested this and lack of it doesn't seem to affect the results. (I'll be out next week; expect v2 when I'm back) -Tomasz

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 165 lines · Tomasz

Quoted 86 lines · Chris Wilson

Quoting Lis, Tomasz (2018-03-28 17:06:58)

Quoted 85 lines · Chris Wilson

Quoted 83 lines

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 73 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
   drivers/gpu/drm/i915/i915_drv.h          |  2 ++ drivers/gpu/drm/i915/i915_pci.c          |  3 ++- drivers/gpu/drm/i915/intel_device_info.h |  1 + drivers/gpu/drm/i915/intel_lrc.c         | 45
+++++++++++++++++++++++++++-----
   drivers/gpu/drm/i915/intel_lrc.h         |  1 +

   5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
 ((dev_priv)->info.has_logical_ring_elsq)
    #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
 ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+ ((dev_priv)->info.has_hw_preempt_to_idle)
    #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
           GEN(11), \
           .ddb_size = 2048, \
           .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
    static const struct intel_device_info intel_icelake_11_info = {
           GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
           func(has_logical_ring_contexts); \
           func(has_logical_ring_elsq); \
           func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
           func(has_overlay); \
           func(has_pooled_eu); \
           func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
    #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
    #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
    #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
    #define GEN8_CTX_STATUS_COMPLETED_MASK \
            (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct · line 190

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct 
 intel_engine_cs *engine,
                                   const struct i915_request *last,
                                   int prio)
    {
-       return engine->i915->preempt_context && prio > 
 max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

Quoted 8 lines

Quoted 7 lines

Yes.. I had mixed feelings about changing needs_preempt_context() now, as that would mean adding a temporary condition on GuC until the GuC preemption is merged.
I will add the conditions and disable the allocation in v2 of the patch.

Quoted 2 lines

But at any rate, making this an engine->flag would eliminate one pointer dance.

Could be an interesting idea for a separate patch.

To land first ;)

:)
Sure, I can do that.

Quoted 35 lines

Quoted 35 lines

Quoted 16 lines

Quoted 16 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
    }

   /**

drivers/gpu/drm/i915/intel_lrc.c +8 -0

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct 
 intel_engine_cs *engine)
 execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
    }
+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = 
 &engine->execlists;
+
+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +6 -0

+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);
+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

My arguments for separate function are:
- better code readability
- keeping the symmetry between execlist and GuC flow - GuC preemption patches will introduce separate function as well
- only 4 lines of the function would be common
- the name inject_preempt_context() wouldn't match the new purpose, so renaming would be needed
- reduced self-documenting code due to two separate preempt methods not
having distinct names

Quoted 5 lines

Quoted 2 lines

That's all, I don't have any future plans for it. If you want me to merge the two, let me know.

The problem that I am worrying about is that we will duplicate bunch of other code, the actual ELS[PQ] write is the smaller portion. Plus we already have the branch on something much more pleasant.

I see. I don't know any details there, so I'm not able to weigh that.
Just let me know whether this possible duplication outweights the arguments I provided, and I will merge these functions. I'm not overly attached to my solution.

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -2

@@ -962,10 +987,13 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                                     status, buf[2*head + 1],
 execlists->active);
-                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
- GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or 

Quoted 5 lines

preempted to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+ GEN8_CTX_STATUS_PREEMPTED)) &&
+                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

Quoted 3 lines

I will check if lack of the change affects test results. Personally, I would keep this change, even if only for allowing simple definition of what HWACK flag means.

The simple definition is the opposite one, imo. We set the flag after we get the corresponding response from HW; any preemption or activate event must follow the most recent ELSP write. So that will include the preemption event following the preempt-idle write.

Quoted 8 lines

Quoted 3 lines

Then on deciding that the HW is idle, we apply the complication such that execlists->active == 0. (That rule is what breaks the pattern.) -Chris

Ok, I will remove this unnecessary condition.
I tested this and lack of it doesn't seem to affect the results. (I'll be out next week; expect v2 when I'm back) -Tomasz

Do we have any test to cover a preempt to idle on already idle HW (which is the case we cover with this flag here)? If not maybe we cold add a selftest for that.

Daniele

Quoted 5 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 74 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
  drivers/gpu/drm/i915/i915_pci.c          |  3 ++-
  drivers/gpu/drm/i915/intel_device_info.h |  1 +
  drivers/gpu/drm/i915/intel_lrc.c         | 45 +++++++++++++++++++++++++++-----
  drivers/gpu/drm/i915/intel_lrc.h         |  1 +
  5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private *dev_priv)
                 ((dev_priv)->info.has_logical_ring_elsq)
  #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                 ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
 
  #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
         GEN(11), \
         .ddb_size = 2048, \
         .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
 
  static const struct intel_device_info intel_icelake_11_info = {
         GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
         func(has_logical_ring_contexts); \
         func(has_logical_ring_elsq); \
         func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
         func(has_overlay); \
         func(has_pooled_eu); \
         func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
  #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
  #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
  #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
 
  #define GEN8_CTX_STATUS_COMPLETED_MASK \
          (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct intel_engine_cs *engine, · line 188

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
                                 const struct i915_request *last,
                                 int prio)
  {
-       return engine->i915->preempt_context && prio > max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

But at any rate, making this an engine->flag would eliminate one pointer dance.

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
  }
 
  /**

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
         execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
  }
 

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 551

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +11 -0

+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+
+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 3 lines

drivers/gpu/drm/i915/intel_lrc.c +3 -0

+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -962,10 +987,13 @@ static void execlists_submission_tasklet(unsigned long data)
                                   status, buf[2*head + 1],
                                   execlists->active);
 

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 993

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -2

-                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-                                     GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or preempted to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+                                       GEN8_CTX_STATUS_PREEMPTED)) &&
+                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

                                 execlists_set_active(execlists,
                                                      EXECLISTS_ACTIVE_HWACK);
+
                         if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
                                 execlists_clear_active(execlists,
                                                        EXECLISTS_ACTIVE_HWACK);

@@ -976,8 +1004,13 @@ static void execlists_submission_tasklet(unsigned long data)
                         /* We should never get a COMPLETED | IDLE_ACTIVE! */
                         GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 1014

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +7 -2

-                       if (status & GEN8_CTX_STATUS_COMPLETE &&
-                           buf[2*head + 1] == execlists->preempt_complete_status) {
+                       /*
+                        * Check if preempted to real idle, either directly or
+                        * the preemptive context already finished executing
+                        */
+                       if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) ||
+                           (status & GEN8_CTX_STATUS_COMPLETE &&
+                           buf[2*head + 1] == execlists->preempt_complete_status)) {
                                 GEM_TRACE("%s preempt-idle\n", engine->name);

Hmm. I was hoping that we would be able to engineer a single check to cover all sins. Might have been overly optimistic, but I can dream. -Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 85 lines · Chris Wilson

Quoting Tomasz Lis (2018-03-27 16:17:59)

Quoted 73 lines

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |  2 ++ drivers/gpu/drm/i915/i915_pci.c          |  3 ++- drivers/gpu/drm/i915/intel_device_info.h |  1 + drivers/gpu/drm/i915/intel_lrc.c         | 45
+++++++++++++++++++++++++++-----
  drivers/gpu/drm/i915/intel_lrc.h         |  1 +

  5 files changed, 45 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 800230b..c32580b 100644
@@ -2514,6 +2514,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
                  ((dev_priv)->info.has_logical_ring_elsq)
   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
 ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
     #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

  diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 4364922..66b6700 100644

drivers/gpu/drm/i915/i915_pci.c +2 -1

@@ -595,7 +595,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
          GEN(11), \
          .ddb_size = 2048, \
          .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
     static const struct intel_device_info intel_icelake_11_info = {
          GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
          func(has_logical_ring_contexts); \
          func(has_logical_ring_elsq); \
          func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
          func(has_overlay); \
          func(has_pooled_eu); \
          func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index ba7f783..1a22de4 100644
@@ -153,6 +153,7 @@
   #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
   #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
   #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
     #define GEN8_CTX_STATUS_COMPLETED_MASK \
           (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c static inline bool need_preempt(const struct · line 190

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -183,7 +184,9 @@ static inline bool need_preempt(const struct 
 intel_engine_cs *engine,
                                  const struct i915_request *last,
                                  int prio)
   {
-       return engine->i915->preempt_context && prio > 
 max(rq_prio(last), 0);
+       return (engine->i915->preempt_context ||
+               HAS_HW_PREEMPT_TO_IDLE(engine->i915)) &&

Well, you haven't actually disabled allocating the preempt_context so...

Quoted 3 lines

But at any rate, making this an engine->flag would eliminate one pointer dance.

Can't we re-use I915_SCHEDULER_CAP_PREEMPTION in engine->i915->caps.scheduler? That btw like here to be set if i915->preempt_context || HAS_HW_PREEMPT_TO_IDLE(i915)

Quoted 13 lines

Quoted 13 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+                prio > max(rq_prio(last), 0);
   }

    /**

drivers/gpu/drm/i915/intel_lrc.c +4 -0

@@ -535,6 +538,25 @@ static void inject_preempt_context(struct 
 intel_engine_cs *engine)
          execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
   }
   +static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+
+       GEM_TRACE("%s\n", engine->name);

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 551

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +6 -0

+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg != NULL);

Shouldn't this check be the other way around?

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void inject_preempt_context(struct intel_engine_cs *engine) · line 554

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +3 -0

+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);

Future plans? Because just inserting the branch into the setter of inject_preempt_context() resolves a lot of conflicts with other work.

Quoted 14 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -962,10 +987,13 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                                    status, buf[2*head + 1],
                                    execlists->active);

  -                       if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
- GEN8_CTX_STATUS_PREEMPTED))
+                       /* Check if switched to active or preempted to active */
+                       if ((status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+ GEN8_CTX_STATUS_PREEMPTED)) &&
+                           !(status & GEN11_CTX_STATUS_PREEMPT_IDLE))

Setting HWACK here is harmless as it gets cleared again. Unless, there is some oddity in the code flow.

There is actually some oddity, but it is more on the HW side. A preempt to idle can potentially land on an already idle HW, in which case GEN8_CTX_STATUS_ACTIVE_IDLE is not set and GEN8_CTX_STATUS_IDLE_ACTIVE is set instead. In this case without this check on GEN11_CTX_STATUS_PREEMPT_IDLE we would set the HWACK here but we wouldn't call the clear below. Not sure if we end up clearing the flag elsewhere, but that doesn't look too nice IMHO.

BTW, the relevant CSB bits coming out in the 2 preempt to idle cases are as follows:

preempt active HW:
GEN11_CTX_STATUS_PREEMPT_IDLE | GEN8_CTX_STATUS_ACTIVE_IDLE | GEN8_CTX_STATUS_PREEMPTED

Preempt idle HW:
GEN11_CTX_STATUS_PREEMPT_IDLE | GEN8_CTX_STATUS_IDLE_ACTIVE

Daniele

Quoted 27 lines

Quoted 26 lines

execlists_set_active(execlists, EXECLISTS_ACTIVE_HWACK);
+
                         if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -976,8 +1004,13 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                          /* We should never get a COMPLETED | IDLE_ACTIVE! */
                          GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);

  -                       if (status & GEN8_CTX_STATUS_COMPLETE && -                           buf[2*head + 1] == execlists->preempt_complete_status) {
+                       /*
+                        * Check if preempted to real idle, either directly or
+                        * the preemptive context already finished executing
+                        */
+                       if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) || +                           (status & GEN8_CTX_STATUS_COMPLETE && +                           buf[2*head + 1] == execlists->preempt_complete_status)) {
                                 GEM_TRACE("%s preempt-idle\n", engine->name);

Quoted 8 lines

Hmm. I was hoping that we would be able to engineer a single check to cover all sins. Might have been overly optimistic, but I can dream. -Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when
preempt-to-idle is supported. (Chris)
Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h          |  2 ++
 drivers/gpu/drm/i915/i915_gem_context.c  |  4 ++-
 drivers/gpu/drm/i915/i915_pci.c          |  3 +-
 drivers/gpu/drm/i915/intel_device_info.h |  1 +
 drivers/gpu/drm/i915/intel_lrc.c         | 47 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_lrc.h         |  1 +
 6 files changed, 51 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 0286911..f445340 100644
@@ -2518,6 +2518,8 @@ intel_info(const struct drm_i915_private *dev_priv)
                ((dev_priv)->info.has_logical_ring_elsq)
 #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
                ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+               ((dev_priv)->info.has_hw_preempt_to_idle)
 
 #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

index 74435af..d65f469 100644

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

 int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
        GEN(11), \
        .ddb_size = 2048, \
        .has_csr = 0, \
-       .has_logical_ring_elsq = 1
+       .has_logical_ring_elsq = 1, \
+       .has_hw_preempt_to_idle = 1
 
 static const struct intel_device_info intel_icelake_11_info = {
        GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
        func(has_logical_ring_contexts); \
        func(has_logical_ring_elsq); \
        func(has_logical_ring_preemption); \
+       func(has_hw_preempt_to_idle); \
        func(has_overlay); \
        func(has_pooled_eu); \
        func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +20 -0

index 029901a..4c94488 100644
@@ -154,6 +154,7 @@
 #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
 #define GEN8_CTX_STATUS_COMPLETE       (1 << 4)
 #define GEN8_CTX_STATUS_LITE_RESTORE   (1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE  (1 << 29)
 
 #define GEN8_CTX_STATUS_COMPLETED_MASK \
         (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -552,6 +553,25 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
        execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
 }
 
+static void gen11_preempt_to_idle(struct intel_engine_cs *engine)
+{
+       struct intel_engine_execlists *execlists = &engine->execlists;
+
+       GEM_TRACE("%s\n", engine->name);
+
+       /*
+        * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+        * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+        */
+       GEM_BUG_ON(execlists->ctrl_reg == NULL);
+
+       /* trigger preemption to idle */
+       writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);
+
+       execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);
+       execlists_set_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
+}
+
 static void execlists_dequeue(struct intel_engine_cs *engine)
 {
        struct intel_engine_execlists * const execlists = &engine->execlists;

drivers/gpu/drm/i915/intel_lrc.c +1 -1

                        goto unlock;
                }
 

@@ -1010,7 +1033,15 @@ static void execlists_submission_tasklet(unsigned long data)
                                      GEN8_CTX_STATUS_PREEMPTED))
                                execlists_set_active(execlists,
                                                     EXECLISTS_ACTIVE_HWACK);
-                       if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+

drivers/gpu/drm/i915/intel_lrc.c +7 -2

@@ -1020,8 +1051,13 @@ static void execlists_submission_tasklet(unsigned long data)
                        /* We should never get a COMPLETED | IDLE_ACTIVE! */
                        GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 
-                       if (status & GEN8_CTX_STATUS_COMPLETE &&
-                           buf[2*head + 1] == execlists->preempt_complete_status) {
+                       /*
+                        * Check if preempted to real idle, either directly or
+                        * the preemptive context already finished executing
+                        */
+                       if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) ||
+                           (status & GEN8_CTX_STATUS_COMPLETE &&
+                           buf[2*head + 1] == execlists->preempt_complete_status)) {
                                GEM_TRACE("%s preempt-idle\n", engine->name);
 
                                execlists_cancel_port_requests(execlists);

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +5 -1

@@ -1010,7 +1033,15 @@ static void execlists_submission_tasklet(unsigned long data)
                                       GEN8_CTX_STATUS_PREEMPTED))
                                 execlists_set_active(execlists,
                                                      EXECLISTS_ACTIVE_HWACK);
-                       if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+
+                       /*
+                        * Check if switched to idle or preempted to idle.
+                        * The STATUS_IDLE_ACTIVE flag is really used to mark
+                        * preemtion from idle to idle, this is not a mistake.

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 1046

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +4 -0

+                        */
+                       if ((status & GEN8_CTX_STATUS_ACTIVE_IDLE) ||
+                           ((status & GEN8_CTX_STATUS_IDLE_ACTIVE) &&
+                            (status & GEN11_CTX_STATUS_PREEMPT_IDLE)))
                                 execlists_clear_active(execlists,
                                                        EXECLISTS_ACTIVE_HWACK);

But still pointless, no?

Quoted 19 lines

drivers/gpu/drm/i915/intel_lrc.c +7 -2

@@ -1020,8 +1051,13 @@ static void execlists_submission_tasklet(unsigned long data)
                         /* We should never get a COMPLETED | IDLE_ACTIVE! */
                         GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 
-                       if (status & GEN8_CTX_STATUS_COMPLETE &&
-                           buf[2*head + 1] == execlists->preempt_complete_status) {
+                       /*
+                        * Check if preempted to real idle, either directly or
+                        * the preemptive context already finished executing
+                        */
+                       if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) ||
+                           (status & GEN8_CTX_STATUS_COMPLETE &&
+                           buf[2*head + 1] == execlists->preempt_complete_status)) {
                                 GEM_TRACE("%s preempt-idle\n", engine->name);
 
                                 execlists_cancel_port_requests(execlists);

@@ -2157,7 +2193,8 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
         engine->unpark = NULL;
 

drivers/gpu/drm/i915/intel_lrc.c static void execlists_set_default_submission(struct intel_engine_cs *engine) · line 2198

Quoted 5 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

         engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-       if (engine->i915->preempt_context)
+       if (engine->i915->preempt_context ||
+           HAS_HW_PREEMPT_TO_IDLE(engine->i915))
                 engine->flags |= I915_ENGINE_HAS_PREEMPTION;

-Chris

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when
preempt-to-idle is supported. (Chris)
Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

v3: Fixed needs_preempt_context() change. (Chris)
Merged preemption trigger functions to one. (Chris) Fixed context state to not assume COMPLETED_MASK after preemption, since idle-to-idle case will not have it set.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |   2 +
  drivers/gpu/drm/i915/i915_gem_context.c  |   5 +-
  drivers/gpu/drm/i915/i915_pci.c          |   3 +-
  drivers/gpu/drm/i915/intel_device_info.h |   1 +
  drivers/gpu/drm/i915/intel_lrc.c         | 115 ++++++++++++++++++++++---------
  drivers/gpu/drm/i915/intel_lrc.h         |   1 +
  6 files changed, 92 insertions(+), 35 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 57fb3aa..6e9647b 100644
@@ -2535,6 +2535,8 @@ intel_info(const struct drm_i915_private *dev_priv)
  		((dev_priv)->info.has_logical_ring_elsq)
  #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
  		((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+		((dev_priv)->info.has_hw_preempt_to_idle)
 
  #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

index 33f8a4b..bdac129 100644

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

  }
 
  int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info intel_cannonlake_info = {
  	GEN(11), \
  	.ddb_size = 2048, \
  	.has_csr = 0, \
-	.has_logical_ring_elsq = 1
+	.has_logical_ring_elsq = 1, \
+	.has_hw_preempt_to_idle = 1
 
  static const struct intel_device_info intel_icelake_11_info = {
  	GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
  	func(has_logical_ring_contexts); \
  	func(has_logical_ring_elsq); \
  	func(has_logical_ring_preemption); \
+	func(has_hw_preempt_to_idle); \
  	func(has_overlay); \
  	func(has_pooled_eu); \
  	func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index 29dcf34..8fe6795 100644
@@ -154,6 +154,7 @@
  #define GEN8_CTX_STATUS_ACTIVE_IDLE	(1 << 3)
  #define GEN8_CTX_STATUS_COMPLETE	(1 << 4)
  #define GEN8_CTX_STATUS_LITE_RESTORE	(1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE	(1 << 29)
 
  #define GEN8_CTX_STATUS_COMPLETED_MASK \
  	 (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c +0 -6

  {
  	struct intel_engine_execlists *execlists = &engine->execlists;
-	struct intel_context *ce =
-		to_intel_context(engine->i915->preempt_context, engine);
-	unsigned int n;
 
-	GEM_BUG_ON(execlists->preempt_complete_status !=
-		   upper_32_bits(ce->lrc_desc));
-	GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &

drivers/gpu/drm/i915/intel_lrc.c +33 -16

-	/*
-	 * Switch to our empty preempt context so
-	 * the state of the GPU is known (idle).
-	 */
-	GEM_TRACE("%s\n", engine->name);
-	for (n = execlists_num_ports(execlists); --n; )
-		write_desc(execlists, 0, n);
+		/*
+		 * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+		 * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+		 */
+		GEM_BUG_ON(execlists->ctrl_reg == NULL);
 
-	write_desc(execlists, ce->lrc_desc, n);
+		/* trigger preemption to idle */
+		writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);
+	} else {
+		struct intel_context *ce =
+			to_intel_context(engine->i915->preempt_context, engine);
+		unsigned int n;
 
-	/* we need to manually load the submit queue */
-	if (execlists->ctrl_reg)
-		writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+		GEM_BUG_ON(execlists->preempt_complete_status !=
+			   upper_32_bits(ce->lrc_desc));
+		GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+		       _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+					  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+		       _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+					  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));
+
+		/*
+		 * Switch to our empty preempt context so
+		 * the state of the GPU is known (idle).
+		 */
+		GEM_TRACE("%s\n", engine->name);
+		for (n = execlists_num_ports(execlists); --n; )
+			write_desc(execlists, 0, n);
+
+		write_desc(execlists, ce->lrc_desc, n);
+
+		/* we need to manually load the submit queue */
+		if (execlists->ctrl_reg)
+			writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+	}
 
  	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
  	execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);

@@ -1045,22 +1064,51 @@ static void execlists_submission_tasklet(unsigned long data)
  				  status, buf[2*head + 1],
  				  execlists->active);
 
-			if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-				      GEN8_CTX_STATUS_PREEMPTED))
-				execlists_set_active(execlists,
-						     EXECLISTS_ACTIVE_HWACK);
-			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)

drivers/gpu/drm/i915/intel_lrc.c +26 -5

+				if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+					      GEN8_CTX_STATUS_PREEMPTED))
+					execlists_set_active(execlists,
+						       EXECLISTS_ACTIVE_HWACK);
+
+				if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+					execlists_clear_active(execlists,
+						       EXECLISTS_ACTIVE_HWACK);
 
-			/* We should never get a COMPLETED | IDLE_ACTIVE! */
-			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
+				if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
+					continue;
 
-			if (status & GEN8_CTX_STATUS_COMPLETE &&
-			    buf[2*head + 1] == execlists->preempt_complete_status) {
+				/*
+				 * We should never get a
+				 * COMPLETED | IDLE_ACTIVE!
+				 */
+				GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
+			}
+
+			/*
+			 * Check if preempted to real idle, either directly or
+			 * the preemptive context already finished executing
+			 */
+			if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) ||
+			    (status & GEN8_CTX_STATUS_COMPLETE &&
+			    buf[2*head + 1] == execlists->preempt_complete_status)) {
  				GEM_TRACE("%s preempt-idle\n", engine->name);
 
  				execlists_cancel_port_requests(execlists);

@@ -2217,7 +2265,8 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
  	engine->unpark = NULL;
 
  	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-	if (engine->i915->preempt_context)
+	if (engine->i915->preempt_context ||
+	    HAS_HW_PREEMPT_TO_IDLE(engine->i915))
  		engine->flags |= I915_ENGINE_HAS_PREEMPTION;
 
  	engine->i915->caps.scheduler =

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 4ec7d8d..b1083ac 100644

Quoted 54 lines · Tomasz Lis

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when preempt-to-idle is supported. (Chris)
     Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
     Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

v3: Fixed needs_preempt_context() change. (Chris)
     Merged preemption trigger functions to one. (Chris) Fixed context state to not assume COMPLETED_MASK after preemption, since idle-to-idle case will not have it set.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |   2 + drivers/gpu/drm/i915/i915_gem_context.c  |   5 +- drivers/gpu/drm/i915/i915_pci.c          |   3 +- drivers/gpu/drm/i915/intel_device_info.h |   1 + drivers/gpu/drm/i915/intel_lrc.c         | 115
++++++++++++++++++++++---------
  drivers/gpu/drm/i915/intel_lrc.h         |   1 +

  6 files changed, 92 insertions(+), 35 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 57fb3aa..6e9647b 100644
@@ -2535,6 +2535,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
           ((dev_priv)->info.has_logical_ring_elsq)
   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
           ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+        ((dev_priv)->info.has_hw_preempt_to_idle)
     #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 33f8a4b..bdac129 100644

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -454,7 +454,10 @@ destroy_kernel_context(struct i915_gem_context 
 **ctxp)
     static bool needs_preempt_context(struct drm_i915_private *i915)

drivers/gpu/drm/i915/i915_gem_context.c destroy_kernel_context(struct i915_gem_context **ctxp) · line 460

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +4 -1

   {
-    return HAS_LOGICAL_RING_PREEMPTION(i915);
+    return HAS_LOGICAL_RING_PREEMPTION(i915) &&
+           (!HAS_HW_PREEMPT_TO_IDLE(i915) ||
+        (HAS_HW_PREEMPT_TO_IDLE(i915) &&
+        !USES_GUC_SUBMISSION(i915)));

Why do we keep the preempt context for !USES_GUC_SUBMISSION(i915) even if HAS_HW_PREEMPT_TO_IDLE(i915)? After this patch we shouldn't need it anymore, right?

Quoted 37 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

   }
     int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
       GEN(11), \
       .ddb_size = 2048, \
       .has_csr = 0, \
-    .has_logical_ring_elsq = 1
+    .has_logical_ring_elsq = 1, \
+    .has_hw_preempt_to_idle = 1
     static const struct intel_device_info intel_icelake_11_info = {
       GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
       func(has_logical_ring_contexts); \
       func(has_logical_ring_elsq); \
       func(has_logical_ring_preemption); \
+    func(has_hw_preempt_to_idle); \
       func(has_overlay); \
       func(has_pooled_eu); \
       func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 29dcf34..8fe6795 100644
@@ -154,6 +154,7 @@
   #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
   #define GEN8_CTX_STATUS_COMPLETE    (1 << 4)
   #define GEN8_CTX_STATUS_LITE_RESTORE    (1 << 15)

drivers/gpu/drm/i915/intel_lrc.c static void port_assign(struct execlist_port · line 528

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+#define GEN11_CTX_STATUS_PREEMPT_IDLE    (1 << 29)
     #define GEN8_CTX_STATUS_COMPLETED_MASK \
        (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -526,31 +527,49 @@ static void port_assign(struct execlist_port 
 *port, struct i915_request *rq)
   static void inject_preempt_context(struct intel_engine_cs *engine)

For gen11+ we don't inject a preempt context anymore, maybe we can rename this function to something like "inject_preempt()".

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -3

   {
       struct intel_engine_execlists *execlists = &engine->execlists;
-    struct intel_context *ce =
-        to_intel_context(engine->i915->preempt_context, engine);
-    unsigned int n;

  -    GEM_BUG_ON(execlists->preempt_complete_status != -           upper_32_bits(ce->lrc_desc));
-    GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | -                       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=

Quoted 8 lines

- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | -                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)); +    if (HAS_HW_PREEMPT_TO_IDLE(engine->i915)) {
+        /*
+         * If we have hardware preempt-to-idle, we do not need to +         * inject any job to the hardware. We only set a flag. +         */
+        GEM_TRACE("%s\n", engine->name);

This trace is in both conditional branches, might be cleaner to just put it before the if statement.

Quoted 60 lines

  -    /*
-     * Switch to our empty preempt context so
-     * the state of the GPU is known (idle).
-     */
-    GEM_TRACE("%s\n", engine->name);
-    for (n = execlists_num_ports(execlists); --n; )
-        write_desc(execlists, 0, n);
+        /*
+         * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also +         * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set +         */
+        GEM_BUG_ON(execlists->ctrl_reg == NULL);
  -    write_desc(execlists, ce->lrc_desc, n);
+        /* trigger preemption to idle */
+        writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg); +    } else {
+        struct intel_context *ce =
+ to_intel_context(engine->i915->preempt_context, engine); +        unsigned int n;
  -    /* we need to manually load the submit queue */
-    if (execlists->ctrl_reg)
-        writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+        GEM_BUG_ON(execlists->preempt_complete_status != +               upper_32_bits(ce->lrc_desc));
+        GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));

drivers/gpu/drm/i915/intel_lrc.c +15 -0

+
+        /*
+         * Switch to our empty preempt context so
+         * the state of the GPU is known (idle).
+         */
+        GEM_TRACE("%s\n", engine->name);
+        for (n = execlists_num_ports(execlists); --n; )
+            write_desc(execlists, 0, n);
+
+        write_desc(execlists, ce->lrc_desc, n);
+
+        /* we need to manually load the submit queue */
+        if (execlists->ctrl_reg)
+            writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+    }

        execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK); execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -1045,22 +1064,51 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                     status, buf[2*head + 1],
                     execlists->active);

  -            if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | -                      GEN8_CTX_STATUS_PREEMPTED))
-                execlists_set_active(execlists,
-                             EXECLISTS_ACTIVE_HWACK); -            if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) +            /*
+             * Check if preempted from idle to idle directly. +             * The STATUS_IDLE_ACTIVE flag is used to mark +             * such transition.

Quoted 6 lines

+             */
+            if ((status & GEN8_CTX_STATUS_IDLE_ACTIVE) && +                 (status & GEN11_CTX_STATUS_PREEMPT_IDLE)) { +
                  execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);

EXECLISTS_ACTIVE_HWACK should be already clear here (we clear it both when we inject the pre-emption and on the previous A->I CSB event), so there should be no need to clear it.

Quoted 11 lines

  -            if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) -                continue;
+                /*
+                 * We could not have COMPLETED anything
+                 * if we were idle before preemption.
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_COMPLETED_MASK); +            }
+
+            else {

nitpick: formatting is wrong here.

Daniele

Quoted 46 lines

+                if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | +                          GEN8_CTX_STATUS_PREEMPTED))
+                    execlists_set_active(execlists,
+                               EXECLISTS_ACTIVE_HWACK); +
+                if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) +                    execlists_clear_active(execlists,
+                               EXECLISTS_ACTIVE_HWACK); -            /* We should never get a COMPLETED | IDLE_ACTIVE! */ -            GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); +                if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) +                    continue;
  -            if (status & GEN8_CTX_STATUS_COMPLETE && -                buf[2*head + 1] == execlists->preempt_complete_status) {
+                /*
+                 * We should never get a
+                 * COMPLETED | IDLE_ACTIVE!
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); +            }
+
+            /*
+             * Check if preempted to real idle, either directly or +             * the preemptive context already finished executing +             */
+            if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) || +                (status & GEN8_CTX_STATUS_COMPLETE &&
+                buf[2*head + 1] == execlists->preempt_complete_status)) {
                  GEM_TRACE("%s preempt-idle\n", engine->name); execlists_cancel_port_requests(execlists);

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -2217,7 +2265,8 @@ static void 
 execlists_set_default_submission(struct intel_engine_cs *engine)
       engine->unpark = NULL;
         engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-    if (engine->i915->preempt_context)
+    if (engine->i915->preempt_context ||
+        HAS_HW_PREEMPT_TO_IDLE(engine->i915))
           engine->flags |= I915_ENGINE_HAS_PREEMPTION;

        engine->i915->caps.scheduler =

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 4ec7d8d..b1083ac 100644
@@ -45,6 +45,7 @@

drivers/gpu/drm/i915/intel_lrc.h line 51

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.h +2 -0

   #define RING_EXECLIST_SQ_CONTENTS(engine) _MMIO((engine)->mmio_base 
+ 0x510)
   #define RING_EXECLIST_CONTROL(engine) _MMIO((engine)->mmio_base + 0x550)
   #define      EL_CTRL_LOAD                (1 << 0)
+#define      EL_CTRL_PREEMPT_TO_IDLE        (1 << 1)
     /* The docs specify that the write pointer wraps around after 5h, "After status
    * is written out to the last available status QW at offset 5h, this pointer

Quoted 57 lines · Daniele Ceraolo Spurio

Quoted 54 lines · Tomasz Lis

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when preempt-to-idle is supported. (Chris)
     Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
     Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

v3: Fixed needs_preempt_context() change. (Chris)
     Merged preemption trigger functions to one. (Chris) Fixed context state to not assume COMPLETED_MASK after preemption, since idle-to-idle case will not have it set.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |   2 + drivers/gpu/drm/i915/i915_gem_context.c  |   5 +- drivers/gpu/drm/i915/i915_pci.c          |   3 +- drivers/gpu/drm/i915/intel_device_info.h |   1 + drivers/gpu/drm/i915/intel_lrc.c         | 115
++++++++++++++++++++++---------
  drivers/gpu/drm/i915/intel_lrc.h         |   1 +

  6 files changed, 92 insertions(+), 35 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 57fb3aa..6e9647b 100644
@@ -2535,6 +2535,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
           ((dev_priv)->info.has_logical_ring_elsq)
   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
           ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+        ((dev_priv)->info.has_hw_preempt_to_idle)
     #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 33f8a4b..bdac129 100644

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

@@ -454,7 +454,10 @@ destroy_kernel_context(struct i915_gem_context 
 **ctxp)
     static bool needs_preempt_context(struct drm_i915_private *i915)

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c destroy_kernel_context(struct i915_gem_context **ctxp) · line 460

Quoted 6 lines

drivers/gpu/drm/i915/i915_gem_context.c +4 -1

   {
-    return HAS_LOGICAL_RING_PREEMPTION(i915);
+    return HAS_LOGICAL_RING_PREEMPTION(i915) &&
+           (!HAS_HW_PREEMPT_TO_IDLE(i915) ||
+        (HAS_HW_PREEMPT_TO_IDLE(i915) &&
+        !USES_GUC_SUBMISSION(i915)));

Why do we keep the preempt context for !USES_GUC_SUBMISSION(i915) even if HAS_HW_PREEMPT_TO_IDLE(i915)? After this patch we shouldn't need it anymore, right?

The patch only provides gen11 way for the non-GuC submission. This is why the condition is so convoluted - preempt_context is still needed if we use GuC. This will be simplified after GuC paches are added.

Quoted 37 lines

Quoted 37 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

   }
     int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
       GEN(11), \
       .ddb_size = 2048, \
       .has_csr = 0, \
-    .has_logical_ring_elsq = 1
+    .has_logical_ring_elsq = 1, \
+    .has_hw_preempt_to_idle = 1
     static const struct intel_device_info intel_icelake_11_info = {
       GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
       func(has_logical_ring_contexts); \
       func(has_logical_ring_elsq); \
       func(has_logical_ring_preemption); \
+    func(has_hw_preempt_to_idle); \
       func(has_overlay); \
       func(has_pooled_eu); \
       func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 29dcf34..8fe6795 100644
@@ -154,6 +154,7 @@
   #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
   #define GEN8_CTX_STATUS_COMPLETE    (1 << 4)
   #define GEN8_CTX_STATUS_LITE_RESTORE    (1 << 15)

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void port_assign(struct execlist_port · line 528

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+#define GEN11_CTX_STATUS_PREEMPT_IDLE    (1 << 29)
     #define GEN8_CTX_STATUS_COMPLETED_MASK \
        (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -526,31 +527,49 @@ static void port_assign(struct execlist_port 
 *port, struct i915_request *rq)
   static void inject_preempt_context(struct intel_engine_cs *engine)

For gen11+ we don't inject a preempt context anymore, maybe we can rename this function to something like "inject_preempt()".

My initial approach was to just add a second function. Merging the changes to inject_preempt_context() was requested by Chris; as I understand it is to minimize refactoring in other work in progress.

Quoted 20 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -3

   {
       struct intel_engine_execlists *execlists = &engine->execlists;
-    struct intel_context *ce =
-        to_intel_context(engine->i915->preempt_context, engine);
-    unsigned int n;

  -    GEM_BUG_ON(execlists->preempt_complete_status != -           upper_32_bits(ce->lrc_desc));
-    GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | -                       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=

Quoted 8 lines

- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | -                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)); +    if (HAS_HW_PREEMPT_TO_IDLE(engine->i915)) {
+        /*
+         * If we have hardware preempt-to-idle, we do not need to +         * inject any job to the hardware. We only set a flag. +         */
+        GEM_TRACE("%s\n", engine->name);

Quoted 1 line

This trace is in both conditional branches, might be cleaner to just put it before the if statement.

True, I did not differentiated the messages. Will put before.

Quoted 60 lines

Quoted 60 lines

  -    /*
-     * Switch to our empty preempt context so
-     * the state of the GPU is known (idle).
-     */
-    GEM_TRACE("%s\n", engine->name);
-    for (n = execlists_num_ports(execlists); --n; )
-        write_desc(execlists, 0, n);
+        /*
+         * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also +         * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set +         */
+        GEM_BUG_ON(execlists->ctrl_reg == NULL);
  -    write_desc(execlists, ce->lrc_desc, n);
+        /* trigger preemption to idle */
+        writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg); +    } else {
+        struct intel_context *ce =
+ to_intel_context(engine->i915->preempt_context, engine); +        unsigned int n;
  -    /* we need to manually load the submit queue */
-    if (execlists->ctrl_reg)
-        writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+        GEM_BUG_ON(execlists->preempt_complete_status != +               upper_32_bits(ce->lrc_desc));
+        GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));

drivers/gpu/drm/i915/intel_lrc.c +15 -0

+
+        /*
+         * Switch to our empty preempt context so
+         * the state of the GPU is known (idle).
+         */
+        GEM_TRACE("%s\n", engine->name);
+        for (n = execlists_num_ports(execlists); --n; )
+            write_desc(execlists, 0, n);
+
+        write_desc(execlists, ce->lrc_desc, n);
+
+        /* we need to manually load the submit queue */
+        if (execlists->ctrl_reg)
+            writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+    }

        execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK); execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -1045,22 +1064,51 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                     status, buf[2*head + 1],
                     execlists->active);

  -            if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | -                      GEN8_CTX_STATUS_PREEMPTED))
-                execlists_set_active(execlists,
-                             EXECLISTS_ACTIVE_HWACK); -            if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) +            /*
+             * Check if preempted from idle to idle directly. +             * The STATUS_IDLE_ACTIVE flag is used to mark +             * such transition.

Quoted 8 lines

Quoted 6 lines

+             */
+            if ((status & GEN8_CTX_STATUS_IDLE_ACTIVE) && +                 (status & GEN11_CTX_STATUS_PREEMPT_IDLE)) { +
                  execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);

EXECLISTS_ACTIVE_HWACK should be already clear here (we clear it both when we inject the pre-emption and on the previous A->I CSB event), so there should be no need to clear it.

This is a complex case; optimizations here may lead to errors later.
But I agree - since this block is only entered on idle-to-idle preemption, and setting the flag can only happen when hardware is not idle, we should never see the ACTIVE_HWACK flag set here. I will change it to GEM_BUG_ON(), unless I will get any errors in testing that.

Quoted 12 lines

Quoted 11 lines

  -            if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) -                continue;
+                /*
+                 * We could not have COMPLETED anything
+                 * if we were idle before preemption.
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_COMPLETED_MASK); +            }
+
+            else {

Quoted 1 line

nitpick: formatting is wrong here.

ack.

Quoted 49 lines

Daniele

Quoted 46 lines

+                if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | +                          GEN8_CTX_STATUS_PREEMPTED))
+                    execlists_set_active(execlists,
+                               EXECLISTS_ACTIVE_HWACK); +
+                if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) +                    execlists_clear_active(execlists,
+                               EXECLISTS_ACTIVE_HWACK); -            /* We should never get a COMPLETED | IDLE_ACTIVE! */ -            GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); +                if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) +                    continue;
  -            if (status & GEN8_CTX_STATUS_COMPLETE && -                buf[2*head + 1] == execlists->preempt_complete_status) {
+                /*
+                 * We should never get a
+                 * COMPLETED | IDLE_ACTIVE!
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); +            }
+
+            /*
+             * Check if preempted to real idle, either directly or +             * the preemptive context already finished executing +             */
+            if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) || +                (status & GEN8_CTX_STATUS_COMPLETE &&
+                buf[2*head + 1] == execlists->preempt_complete_status)) {
                  GEM_TRACE("%s preempt-idle\n", engine->name); execlists_cancel_port_requests(execlists);

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -2217,7 +2265,8 @@ static void 
 execlists_set_default_submission(struct intel_engine_cs *engine)
       engine->unpark = NULL;
         engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-    if (engine->i915->preempt_context)
+    if (engine->i915->preempt_context ||
+        HAS_HW_PREEMPT_TO_IDLE(engine->i915))
           engine->flags |= I915_ENGINE_HAS_PREEMPTION;

        engine->i915->caps.scheduler =

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 4ec7d8d..b1083ac 100644
@@ -45,6 +45,7 @@

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.h line 51

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.h +2 -0

   #define RING_EXECLIST_SQ_CONTENTS(engine) _MMIO((engine)->mmio_base 
+ 0x510)
   #define RING_EXECLIST_CONTROL(engine) _MMIO((engine)->mmio_base + 0x550)
   #define      EL_CTRL_LOAD                (1 << 0)
+#define      EL_CTRL_PREEMPT_TO_IDLE        (1 << 1)
     /* The docs specify that the write pointer wraps around after 5h, "After status
    * is written out to the last available status QW at offset 5h, this pointer

Quoted 68 lines · Tomasz

Quoted 65 lines · Daniele Ceraolo Spurio

Quoted 52 lines · Tomasz Lis

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when preempt-to-idle is supported. (Chris)
     Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
     Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

v3: Fixed needs_preempt_context() change. (Chris)
     Merged preemption trigger functions to one. (Chris) Fixed context state to not assume COMPLETED_MASK after preemption, since idle-to-idle case will not have it set.

Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |   2 + drivers/gpu/drm/i915/i915_gem_context.c  |   5 +- drivers/gpu/drm/i915/i915_pci.c          |   3 +- drivers/gpu/drm/i915/intel_device_info.h |   1 + drivers/gpu/drm/i915/intel_lrc.c         | 115
++++++++++++++++++++++---------
  drivers/gpu/drm/i915/intel_lrc.h         |   1 +

  6 files changed, 92 insertions(+), 35 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 57fb3aa..6e9647b 100644
@@ -2535,6 +2535,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
           ((dev_priv)->info.has_logical_ring_elsq)
   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
           ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+        ((dev_priv)->info.has_hw_preempt_to_idle)
     #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 33f8a4b..bdac129 100644

drivers/gpu/drm/i915/i915_gem_context.c destroy_kernel_context(struct i915_gem_context · line 460

Quoted 8 lines

drivers/gpu/drm/i915/i915_gem_context.c +4 -1

@@ -454,7 +454,10 @@ destroy_kernel_context(struct i915_gem_context 
 **ctxp)
     static bool needs_preempt_context(struct drm_i915_private *i915)
   {
-    return HAS_LOGICAL_RING_PREEMPTION(i915);
+    return HAS_LOGICAL_RING_PREEMPTION(i915) &&
+           (!HAS_HW_PREEMPT_TO_IDLE(i915) ||
+        (HAS_HW_PREEMPT_TO_IDLE(i915) &&
+        !USES_GUC_SUBMISSION(i915)));

Why do we keep the preempt context for !USES_GUC_SUBMISSION(i915) even if HAS_HW_PREEMPT_TO_IDLE(i915)? After this patch we shouldn't need it anymore, right?

Quoted 2 lines

The patch only provides gen11 way for the non-GuC submission. This is why the condition is so convoluted - preempt_context is still needed if we use GuC. This will be simplified after GuC paches are added.

mmm I think this check is the other way around because it returns true when HAS_HW_PREEMPT_TO_IDLE for !USES_GUC_SUBMISSION, so when GuC is not in use.

BTW, GuC does not support using the preempt context on platforms that have HW supported preempt-to-idle, so there is no need to keep the preempt context around for GuC.

Quoted 136 lines

Quoted 37 lines

Quoted 37 lines

drivers/gpu/drm/i915/i915_gem_context.c +0 -0

   }
     int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 4364922..66b6700 100644
@@ -595,7 +595,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
       GEN(11), \
       .ddb_size = 2048, \
       .has_csr = 0, \
-    .has_logical_ring_elsq = 1
+    .has_logical_ring_elsq = 1, \
+    .has_hw_preempt_to_idle = 1
     static const struct intel_device_info intel_icelake_11_info = {
       GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
       func(has_logical_ring_contexts); \
       func(has_logical_ring_elsq); \
       func(has_logical_ring_preemption); \
+    func(has_hw_preempt_to_idle); \
       func(has_overlay); \
       func(has_pooled_eu); \
       func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 29dcf34..8fe6795 100644
@@ -154,6 +154,7 @@
   #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
   #define GEN8_CTX_STATUS_COMPLETE    (1 << 4)
   #define GEN8_CTX_STATUS_LITE_RESTORE    (1 << 15)

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c static void port_assign(struct execlist_port · line 528

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+#define GEN11_CTX_STATUS_PREEMPT_IDLE    (1 << 29)
     #define GEN8_CTX_STATUS_COMPLETED_MASK \
        (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -526,31 +527,49 @@ static void port_assign(struct execlist_port 
 *port, struct i915_request *rq)
   static void inject_preempt_context(struct intel_engine_cs *engine)

For gen11+ we don't inject a preempt context anymore, maybe we can rename this function to something like "inject_preempt()".

My initial approach was to just add a second function. Merging the changes to inject_preempt_context() was requested by Chris; as I understand it is to minimize refactoring in other work in progress.

Quoted 20 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -3

   {
       struct intel_engine_execlists *execlists = &engine->execlists;
-    struct intel_context *ce =
- to_intel_context(engine->i915->preempt_context, engine);
-    unsigned int n;

  -    GEM_BUG_ON(execlists->preempt_complete_status != -           upper_32_bits(ce->lrc_desc));
-    GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | -                       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=

Quoted 8 lines

- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | -                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)); +    if (HAS_HW_PREEMPT_TO_IDLE(engine->i915)) {
+        /*
+         * If we have hardware preempt-to-idle, we do not need to +         * inject any job to the hardware. We only set a flag. +         */
+        GEM_TRACE("%s\n", engine->name);

Quoted 1 line

This trace is in both conditional branches, might be cleaner to just put it before the if statement.

True, I did not differentiated the messages. Will put before.

Quoted 68 lines

Quoted 58 lines

  -    /*
-     * Switch to our empty preempt context so
-     * the state of the GPU is known (idle).
-     */
-    GEM_TRACE("%s\n", engine->name);
-    for (n = execlists_num_ports(execlists); --n; )
-        write_desc(execlists, 0, n);
+        /*
+         * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also +         * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set +         */
+        GEM_BUG_ON(execlists->ctrl_reg == NULL);
  -    write_desc(execlists, ce->lrc_desc, n);
+        /* trigger preemption to idle */
+        writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg); +    } else {
+        struct intel_context *ce =
+ to_intel_context(engine->i915->preempt_context, engine); +        unsigned int n;
  -    /* we need to manually load the submit queue */
-    if (execlists->ctrl_reg)
-        writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+        GEM_BUG_ON(execlists->preempt_complete_status != +               upper_32_bits(ce->lrc_desc));
+ GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));

drivers/gpu/drm/i915/intel_lrc.c +15 -0

+
+        /*
+         * Switch to our empty preempt context so
+         * the state of the GPU is known (idle).
+         */
+        GEM_TRACE("%s\n", engine->name);
+        for (n = execlists_num_ports(execlists); --n; )
+            write_desc(execlists, 0, n);
+
+        write_desc(execlists, ce->lrc_desc, n);
+
+        /* we need to manually load the submit queue */
+        if (execlists->ctrl_reg)
+            writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+    }

        execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK); execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -1045,22 +1064,51 @@ static void 
 execlists_submission_tasklet(unsigned long data)
                     status, buf[2*head + 1],
                     execlists->active);

  -            if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | -                      GEN8_CTX_STATUS_PREEMPTED))
-                execlists_set_active(execlists,
-                             EXECLISTS_ACTIVE_HWACK); -            if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) +            /*
+             * Check if preempted from idle to idle directly.

Quoted 8 lines

+             * The STATUS_IDLE_ACTIVE flag is used to mark +             * such transition.
+             */
+            if ((status & GEN8_CTX_STATUS_IDLE_ACTIVE) && +                 (status & GEN11_CTX_STATUS_PREEMPT_IDLE)) { +
                  execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);

EXECLISTS_ACTIVE_HWACK should be already clear here (we clear it both when we inject the pre-emption and on the previous A->I CSB event), so there should be no need to clear it.

Quoted 4 lines

This is a complex case; optimizations here may lead to errors later.
But I agree - since this block is only entered on idle-to-idle preemption, and setting the flag can only happen when hardware is not idle, we should never see the ACTIVE_HWACK flag set here. I will change it to GEM_BUG_ON(), unless I will get any errors in testing that.

I'm not sure we actually need to care at all about EXECLISTS_ACTIVE_HWACK here. From what I can see that is only used to make sure we don't submit while the execlists HW is loading the current submission. In this case however we're sure no submissions are occurring because EXECLISTS_ACTIVE_PREEMPT is set, so we're already guarded.

Daniele

Quoted 71 lines

Quoted 12 lines

Quoted 11 lines

  -            if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) -                continue;
+                /*
+                 * We could not have COMPLETED anything
+                 * if we were idle before preemption.
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_COMPLETED_MASK); +            }
+
+            else {

Quoted 1 line

nitpick: formatting is wrong here.

ack.

Quoted 56 lines

Daniele

Quoted 53 lines

+                if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | +                          GEN8_CTX_STATUS_PREEMPTED))
+                    execlists_set_active(execlists,
+                               EXECLISTS_ACTIVE_HWACK); +
+                if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) +                    execlists_clear_active(execlists,
+                               EXECLISTS_ACTIVE_HWACK); -            /* We should never get a COMPLETED | IDLE_ACTIVE! */ -            GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); +                if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) +                    continue;
  -            if (status & GEN8_CTX_STATUS_COMPLETE && -                buf[2*head + 1] == execlists->preempt_complete_status) {
+                /*
+                 * We should never get a
+                 * COMPLETED | IDLE_ACTIVE!
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); +            }
+
+            /*
+             * Check if preempted to real idle, either directly or +             * the preemptive context already finished executing +             */
+            if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) || +                (status & GEN8_CTX_STATUS_COMPLETE &&
+                buf[2*head + 1] == execlists->preempt_complete_status)) {
                  GEM_TRACE("%s preempt-idle\n", engine->name); execlists_cancel_port_requests(execlists);

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -2217,7 +2265,8 @@ static void 
 execlists_set_default_submission(struct intel_engine_cs *engine)
       engine->unpark = NULL;
         engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-    if (engine->i915->preempt_context)
+    if (engine->i915->preempt_context ||
+        HAS_HW_PREEMPT_TO_IDLE(engine->i915))
           engine->flags |= I915_ENGINE_HAS_PREEMPTION;

        engine->i915->caps.scheduler =

drivers/gpu/drm/i915/intel_lrc.h +1 -0

index 4ec7d8d..b1083ac 100644
@@ -45,6 +45,7 @@
   #define RING_EXECLIST_SQ_CONTENTS(engine) _MMIO((engine)->mmio_base + 0x510)
   #define RING_EXECLIST_CONTROL(engine) _MMIO((engine)->mmio_base + 0x550)
   #define      EL_CTRL_LOAD                (1 << 0)
+#define      EL_CTRL_PREEMPT_TO_IDLE        (1 << 1)
     /* The docs specify that the write pointer wraps around after 5h, "After status
    * is written out to the last available status QW at offset 5h, this pointer
 

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when
preempt-to-idle is supported. (Chris)
Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

v3: Fixed needs_preempt_context() change. (Chris)
Merged preemption trigger functions to one. (Chris) Fixed conyext state tonot assume COMPLETED_MASK after preemption, since idle-to-idle case will not have it set.

v4: Simplified needs_preempt_context() change. (Daniele)
Removed clearing HWACK flag in idle-to-idle preempt. (Daniele)

Cc: Joonas Lahtinen <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Daniele Ceraolo Spurio <[email protected]>
Cc: Michal Winiarski <[email protected]>
Cc: Mika Kuoppala <[email protected]>
Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |   2 +
  drivers/gpu/drm/i915/i915_gem_context.c  |   3 +-
  drivers/gpu/drm/i915/i915_pci.c          |   3 +-
  drivers/gpu/drm/i915/intel_device_info.h |   1 +
  drivers/gpu/drm/i915/intel_lrc.c         | 113 +++++++++++++++++++++----------
  drivers/gpu/drm/i915/intel_lrc.h         |   1 +
  6 files changed, 86 insertions(+), 37 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 487922f..35eddf7 100644
@@ -2534,6 +2534,8 @@ intel_info(const struct drm_i915_private *dev_priv)
  		((dev_priv)->info.has_logical_ring_elsq)
  #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
  		((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+		((dev_priv)->info.has_hw_preempt_to_idle)
 
  #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
 

drivers/gpu/drm/i915/i915_gem_context.c +2 -1

index 45393f6..341a5ff 100644
@@ -455,7 +455,8 @@ destroy_kernel_context(struct i915_gem_context **ctxp)
 
  static bool needs_preempt_context(struct drm_i915_private *i915)
  {
-	return HAS_LOGICAL_RING_PREEMPTION(i915);
+	return HAS_LOGICAL_RING_PREEMPTION(i915) &&
+	       !HAS_HW_PREEMPT_TO_IDLE(i915);
  }
 
  int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 97a91e6a..ee09926 100644
@@ -593,7 +593,8 @@ static const struct intel_device_info intel_cannonlake_info = {
  	GEN(11), \
  	.ddb_size = 2048, \
  	.has_csr = 0, \
-	.has_logical_ring_elsq = 1
+	.has_logical_ring_elsq = 1, \
+	.has_hw_preempt_to_idle = 1
 
  static const struct intel_device_info intel_icelake_11_info = {
  	GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
  	func(has_logical_ring_contexts); \
  	func(has_logical_ring_elsq); \
  	func(has_logical_ring_preemption); \
+	func(has_hw_preempt_to_idle); \
  	func(has_overlay); \
  	func(has_pooled_eu); \
  	func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +1 -0

index 8a6058b..f95cb37 100644
@@ -154,6 +154,7 @@
  #define GEN8_CTX_STATUS_ACTIVE_IDLE	(1 << 3)
  #define GEN8_CTX_STATUS_COMPLETE	(1 << 4)
  #define GEN8_CTX_STATUS_LITE_RESTORE	(1 << 15)
+#define GEN11_CTX_STATUS_PREEMPT_IDLE	(1 << 29)
 
  #define GEN8_CTX_STATUS_COMPLETED_MASK \
  	 (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

drivers/gpu/drm/i915/intel_lrc.c +36 -27

  {
  	struct intel_engine_execlists *execlists = &engine->execlists;
-	struct intel_context *ce =
-		to_intel_context(engine->i915->preempt_context, engine);
-	unsigned int n;
-
-	GEM_BUG_ON(execlists->preempt_complete_status !=
-		   upper_32_bits(ce->lrc_desc));
-	GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
-		    _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
-				       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
-		   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
-				      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));
 
-	/*
-	 * Switch to our empty preempt context so
-	 * the state of the GPU is known (idle).
-	 */
  	GEM_TRACE("%s\n", engine->name);
-	for (n = execlists_num_ports(execlists); --n; )
-		write_desc(execlists, 0, n);
+	if (HAS_HW_PREEMPT_TO_IDLE(engine->i915)) {
+		/*
+		 * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also
+		 * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set
+		 */
+		GEM_BUG_ON(execlists->ctrl_reg == NULL);
 
-	write_desc(execlists, ce->lrc_desc, n);
+		/*
+		 * If we have hardware preempt-to-idle, we do not need to
+		 * inject any job to the hardware. We only set a flag.
+		 */
+		writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg);
+	} else {
+		struct intel_context *ce =
+			to_intel_context(engine->i915->preempt_context, engine);
+		unsigned int n;
 
-	/* we need to manually load the submit queue */
-	if (execlists->ctrl_reg)
-		writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+		GEM_BUG_ON(execlists->preempt_complete_status !=
+			   upper_32_bits(ce->lrc_desc));
+		GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+			    _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+					       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+			   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+					      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));
+
+		/*
+		 * Switch to our empty preempt context so
+		 * the state of the GPU is known (idle).
+		 */
+		for (n = execlists_num_ports(execlists); --n; )
+			write_desc(execlists, 0, n);
+
+		write_desc(execlists, ce->lrc_desc, n);
+
+		/* we need to manually load the submit queue */
+		if (execlists->ctrl_reg)
+			writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+	}
 
  	execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);
  	execlists_set_active(execlists, EXECLISTS_ACTIVE_PREEMPT);

@@ -1031,22 +1047,48 @@ static void process_csb(struct intel_engine_cs *engine)
  				  status, buf[2*head + 1],
  				  execlists->active);
 
-			if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
-				      GEN8_CTX_STATUS_PREEMPTED))
-				execlists_set_active(execlists,
-						     EXECLISTS_ACTIVE_HWACK);
-			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
-				execlists_clear_active(execlists,

drivers/gpu/drm/i915/intel_lrc.c +32 -7

+				GEM_BUG_ON(execlists_is_active(execlists,
+						      EXECLISTS_ACTIVE_HWACK));
 
-			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
-				continue;
+				/*
+				 * We could not have COMPLETED anything
+				 * if we were idle before preemption.
+				 */
+				GEM_BUG_ON(status & GEN8_CTX_STATUS_COMPLETED_MASK);
+			} else {
+				if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+					      GEN8_CTX_STATUS_PREEMPTED))
+					execlists_set_active(execlists,
+							     EXECLISTS_ACTIVE_HWACK);
+
+				if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+					execlists_clear_active(execlists,
+							       EXECLISTS_ACTIVE_HWACK);
+
+				if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
+					continue;
+
+				/* We should never get a COMPLETED | IDLE_ACTIVE! */
+				GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
+			}
 
-			/* We should never get a COMPLETED | IDLE_ACTIVE! */
-			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 
-			if (status & GEN8_CTX_STATUS_COMPLETE &&
-			    buf[2*head + 1] == execlists->preempt_complete_status) {
+			/*
+			 * Check if preempted to real idle, either directly or
+			 * the preemptive context already finished executing
+			 */
+			if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) ||
+			    (status & GEN8_CTX_STATUS_COMPLETE &&
+			    buf[2*head + 1] == execlists->preempt_complete_status)) {
  				GEM_TRACE("%s preempt-idle\n", engine->name);
  				complete_preempt_context(execlists);
  				continue;

@@ -2337,7 +2379,8 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
  	engine->unpark = NULL;
 
  	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-	if (engine->i915->preempt_context)
+	if (engine->i915->preempt_context ||
+	    HAS_HW_PREEMPT_TO_IDLE(engine->i915))
  		engine->flags |= I915_ENGINE_HAS_PREEMPTION;
 
  	engine->i915->caps.scheduler =

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 1593194..3249e9b 100644

Quoted 102 lines · Tomasz Lis

The patch adds support of preempt-to-idle requesting by setting a proper bit within Execlist Control Register, and receiving preemption result from Context Status Buffer.

Preemption in previous gens required a special batch buffer to be executed, so the Command Streamer never preempted to idle directly. In Icelake it is possible, as there is a hardware mechanism to inform the kernel about status of the preemption request.

This patch does not cover using the new preemption mechanism when GuC is active.

v2: Added needs_preempt_context() change so that it is not created when preempt-to-idle is supported. (Chris)
     Updated setting HWACK flag so that it is cleared after preempt-to-dle. (Chris, Daniele)
     Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)

v3: Fixed needs_preempt_context() change. (Chris)
     Merged preemption trigger functions to one. (Chris) Fixed conyext state tonot assume COMPLETED_MASK after preemption, since idle-to-idle case will not have it set.

v4: Simplified needs_preempt_context() change. (Daniele) Removed clearing HWACK flag in idle-to-idle preempt. (Daniele)

Cc: Joonas Lahtinen <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Daniele Ceraolo Spurio <[email protected]>
Cc: Michal Winiarski <[email protected]>
Cc: Mika Kuoppala <[email protected]>
Bspec: 18922

Signed-off-by Tomasz Lis <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h          |   2 + drivers/gpu/drm/i915/i915_gem_context.c  |   3 +- drivers/gpu/drm/i915/i915_pci.c          |   3 +- drivers/gpu/drm/i915/intel_device_info.h |   1 + drivers/gpu/drm/i915/intel_lrc.c         | 113
+++++++++++++++++++++----------
  drivers/gpu/drm/i915/intel_lrc.h         |   1 +

  6 files changed, 86 insertions(+), 37 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 487922f..35eddf7 100644
@@ -2534,6 +2534,8 @@ intel_info(const struct drm_i915_private 
 *dev_priv)
           ((dev_priv)->info.has_logical_ring_elsq)
   #define HAS_LOGICAL_RING_PREEMPTION(dev_priv) \
           ((dev_priv)->info.has_logical_ring_preemption)
+#define HAS_HW_PREEMPT_TO_IDLE(dev_priv) \
+        ((dev_priv)->info.has_hw_preempt_to_idle)
     #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)

  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 45393f6..341a5ff 100644

drivers/gpu/drm/i915/i915_gem_context.c +2 -1

@@ -455,7 +455,8 @@ destroy_kernel_context(struct i915_gem_context 
 **ctxp)
     static bool needs_preempt_context(struct drm_i915_private *i915)
   {
-    return HAS_LOGICAL_RING_PREEMPTION(i915);
+    return HAS_LOGICAL_RING_PREEMPTION(i915) &&
+           !HAS_HW_PREEMPT_TO_IDLE(i915);
   }
     int i915_gem_contexts_init(struct drm_i915_private *dev_priv)

drivers/gpu/drm/i915/i915_pci.c +2 -1

index 97a91e6a..ee09926 100644
@@ -593,7 +593,8 @@ static const struct intel_device_info 
 intel_cannonlake_info = {
       GEN(11), \
       .ddb_size = 2048, \
       .has_csr = 0, \
-    .has_logical_ring_elsq = 1
+    .has_logical_ring_elsq = 1, \
+    .has_hw_preempt_to_idle = 1
     static const struct intel_device_info intel_icelake_11_info = {
       GEN11_FEATURES,

drivers/gpu/drm/i915/intel_device_info.h +1 -0

index 933e316..4eb97b5 100644
@@ -98,6 +98,7 @@ enum intel_platform {
       func(has_logical_ring_contexts); \
       func(has_logical_ring_elsq); \
       func(has_logical_ring_preemption); \
+    func(has_hw_preempt_to_idle); \
       func(has_overlay); \
       func(has_pooled_eu); \
       func(has_psr); \

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 8a6058b..f95cb37 100644
@@ -154,6 +154,7 @@
   #define GEN8_CTX_STATUS_ACTIVE_IDLE    (1 << 3)
   #define GEN8_CTX_STATUS_COMPLETE    (1 << 4)
   #define GEN8_CTX_STATUS_LITE_RESTORE    (1 << 15)

drivers/gpu/drm/i915/intel_lrc.c static void port_assign(struct execlist_port · line 524

Quoted 6 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+#define GEN11_CTX_STATUS_PREEMPT_IDLE    (1 << 29)
     #define GEN8_CTX_STATUS_COMPLETED_MASK \
        (GEN8_CTX_STATUS_COMPLETE | GEN8_CTX_STATUS_PREEMPTED)

@@ -522,31 +523,46 @@ static void port_assign(struct execlist_port 
 *port, struct i915_request *rq)
   static void inject_preempt_context(struct intel_engine_cs *engine)

continuing the discussion from the previous patch, I still think that we should rename this function now that it doesn't inject a context on some gens. A new function name should be relatively trivial to handle from other patch series hitting the area (compared to having a second function).

Quoted 77 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -11

   {
       struct intel_engine_execlists *execlists = &engine->execlists;
-    struct intel_context *ce =
-        to_intel_context(engine->i915->preempt_context, engine);
-    unsigned int n;
-
-    GEM_BUG_ON(execlists->preempt_complete_status !=
-           upper_32_bits(ce->lrc_desc));
-    GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
-                       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
- _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
-                      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));

  -    /*
-     * Switch to our empty preempt context so
-     * the state of the GPU is known (idle).
-     */
      GEM_TRACE("%s\n", engine->name);
-    for (n = execlists_num_ports(execlists); --n; )
-        write_desc(execlists, 0, n);
+    if (HAS_HW_PREEMPT_TO_IDLE(engine->i915)) {
+        /*
+         * hardware which HAS_HW_PREEMPT_TO_IDLE(), always also +         * HAS_LOGICAL_RING_ELSQ(), so we can assume ctrl_reg is set +         */
+        GEM_BUG_ON(execlists->ctrl_reg == NULL);
  -    write_desc(execlists, ce->lrc_desc, n);
+        /*
+         * If we have hardware preempt-to-idle, we do not need to +         * inject any job to the hardware. We only set a flag. +         */
+        writel(EL_CTRL_PREEMPT_TO_IDLE, execlists->ctrl_reg); +    } else {
+        struct intel_context *ce =
+ to_intel_context(engine->i915->preempt_context, engine); +        unsigned int n;
  -    /* we need to manually load the submit queue */
-    if (execlists->ctrl_reg)
-        writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+        GEM_BUG_ON(execlists->preempt_complete_status != +               upper_32_bits(ce->lrc_desc));
+        GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                           CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | +                          CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));

drivers/gpu/drm/i915/intel_lrc.c +14 -0

+
+        /*
+         * Switch to our empty preempt context so
+         * the state of the GPU is known (idle).
+         */
+        for (n = execlists_num_ports(execlists); --n; )
+            write_desc(execlists, 0, n);
+
+        write_desc(execlists, ce->lrc_desc, n);
+
+        /* we need to manually load the submit queue */
+        if (execlists->ctrl_reg)
+            writel(EL_CTRL_LOAD, execlists->ctrl_reg);
+    }

        execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK); execlists_set_active(execlists, EXECLISTS_ACTIVE_PREEMPT);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -1031,22 +1047,48 @@ static void process_csb(struct 
 intel_engine_cs *engine)
                     status, buf[2*head + 1],
                     execlists->active);

  -            if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | -                      GEN8_CTX_STATUS_PREEMPTED))
-                execlists_set_active(execlists,
-                             EXECLISTS_ACTIVE_HWACK); -            if (status & GEN8_CTX_STATUS_ACTIVE_IDLE) -                execlists_clear_active(execlists,
-                               EXECLISTS_ACTIVE_HWACK); +            /*
+             * Check if preempted from idle to idle directly. +             * The STATUS_IDLE_ACTIVE flag is used to mark

Quoted 6 lines

+             * such transition.
+             */
+            if ((status & GEN8_CTX_STATUS_IDLE_ACTIVE) && +                 (status & GEN11_CTX_STATUS_PREEMPT_IDLE)) { +
+                /* Cannot be waiting for HWACK while HW is idle */

This comment does not match the check, since if the EXECLISTS_ACTIVE_HWACK is set it means we've received the hw ack, not that we're waiting for it. Personally I would just remove the BUG_ON since we don't really care about the value of HWACK as long as EXECLISTS_ACTIVE_PREEMPT is set, as the latter ensures us we're not going to submit work until the whole preempt process is complete. A BUG_ON for EXECLISTS_ACTIVE_PREEMPT is already in complete_preempt_context so we're covered on that side.

With the 2 minor comments addressed:

Reviewed-by Daniele Ceraolo Spurio <[email protected]>

Daniele

Quoted 55 lines

+ GEM_BUG_ON(execlists_is_active(execlists,
+                              EXECLISTS_ACTIVE_HWACK));
  -            if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK)) -                continue;
+                /*
+                 * We could not have COMPLETED anything
+                 * if we were idle before preemption.
+                 */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_COMPLETED_MASK); +            } else {
+                if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE | +                          GEN8_CTX_STATUS_PREEMPTED))
+                    execlists_set_active(execlists,
+                                 EXECLISTS_ACTIVE_HWACK);

drivers/gpu/drm/i915/intel_lrc.c +11 -0

+
+                if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+                    execlists_clear_active(execlists,
+                                   EXECLISTS_ACTIVE_HWACK);
+
+                if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
+                    continue;
+
+                /* We should never get a COMPLETED | IDLE_ACTIVE! */
+                GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
+            }

  -            /* We should never get a COMPLETED | IDLE_ACTIVE! */ -            GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE); -            if (status & GEN8_CTX_STATUS_COMPLETE && -                buf[2*head + 1] == execlists->preempt_complete_status) {
+            /*
+             * Check if preempted to real idle, either directly or +             * the preemptive context already finished executing +             */
+            if ((status & GEN11_CTX_STATUS_PREEMPT_IDLE) || +                (status & GEN8_CTX_STATUS_COMPLETE &&
+                buf[2*head + 1] == execlists->preempt_complete_status)) {
                  GEM_TRACE("%s preempt-idle\n", engine->name); complete_preempt_context(execlists); continue;

drivers/gpu/drm/i915/intel_lrc.c +2 -1

@@ -2337,7 +2379,8 @@ static void 
 execlists_set_default_submission(struct intel_engine_cs *engine)
       engine->unpark = NULL;
         engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-    if (engine->i915->preempt_context)
+    if (engine->i915->preempt_context ||
+        HAS_HW_PREEMPT_TO_IDLE(engine->i915))
           engine->flags |= I915_ENGINE_HAS_PREEMPTION;

        engine->i915->caps.scheduler =

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index 1593194..3249e9b 100644
@@ -45,6 +45,7 @@