Re: [PATCH 02/12] drm/i915/guc: Don't call ring_is_idle in GuC submission

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 19/07/2022 01:09, John Harrison wrote:
On 7/18/2022 05:26, Tvrtko Ursulin wrote:

On 13/07/2022 00:31, John.C.Harrison@xxxxxxxxx wrote:
From: Matthew Brost <matthew.brost@xxxxxxxxx>

The engine registers really shouldn't be touched during GuC submission
as the GuC owns the registers. Don't call ring_is_idle and tie

Touch being just read and it is somehow harmful?
The registers are meaningless when GuC is controlling the submission. The i915 driver has no knowledge of what context is or is not executing on any given engine at any given time. So reading reading the ring registers is incorrect - it can lead to bad assumptions about what state the hardware is in.

Same is actually true with the execlists backend. The code in ring_is_idle is not concerning itself with which context is running or not. Just that the head/tail/ctl appear idle.

Problem/motivation appears to be on a higher than simply ring registers.

I am not claiming it makes sense with Guc and that it has to remain but just suggesting for as a minimum clearer commit message.

intel_engine_is_idle strictly to the engine pm.

Strictly seems wrong - it is just ring_is_idle check that is replaced and not the whole implementation of intel_engine_is_idle.

Because intel_engine_is_idle tied to the engine pm, retire requests
before checking intel_engines_are_idle in gt_drop_caches, and lastly
Why is re-ordering important? I at least can't understand it. I hope it's not working around IGT failures.
If requests are physically completed but not retired then they will be holding unnecessary PM references. So we need to flush those out before checking for idle.

And if they are not as someone passes in DROP_RESET_ACTIVE? They will not retire and code still enters intel_engines_are_idle so that has to work, no? Something does not align for me still.

increase the timeout in gt_drop_caches for the intel_engines_are_idle
check.

Same here - why?
@Matthew Brost - do you recall which particular tests were hitting an issue? I'm guessing gem_ctx_create? I believe that's the one that creates and destroys thousands of contexts. That is much slower with GuC (GuC communication required) than with execlists (i915 internal state change only).

And if that is a logically separate change please split the patch up.

Regards,

Tvrtko


John.




Regards,

Tvrtko

Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
---
  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 +++++++++++++
  drivers/gpu/drm/i915/i915_debugfs.c       |  6 +++---
  drivers/gpu/drm/i915/i915_drv.h           |  2 +-
  3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 283870c659911..959a7c92e8f4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1602,6 +1602,9 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
  {
      bool idle = true;
  +    /* GuC submission shouldn't access HEAD & TAIL via MMIO */
+    GEM_BUG_ON(intel_engine_uses_guc(engine));
+
      if (I915_SELFTEST_ONLY(!engine->mmio_base))
          return true;
  @@ -1668,6 +1671,16 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
      if (!i915_sched_engine_is_empty(engine->sched_engine))
          return false;
  +    /*
+     * We shouldn't touch engine registers with GuC submission as the GuC +     * owns the registers. Let's tie the idle to engine pm, at worst this +     * function sometimes will falsely report non-idle when idle during the
+     * delay to retire requests or with virtual engines and a request
+     * running on another instance within the same class / submit mask.
+     */
+    if (intel_engine_uses_guc(engine))
+        return false;
+
      /* Ring stopped? */
      return ring_is_idle(engine);
  }
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 94e5c29d2ee3a..ee5334840e9cb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -654,13 +654,13 @@ gt_drop_caches(struct intel_gt *gt, u64 val)
  {
      int ret;
  +    if (val & DROP_RETIRE || val & DROP_RESET_ACTIVE)
+        intel_gt_retire_requests(gt);
+
      if (val & DROP_RESET_ACTIVE &&
          wait_for(intel_engines_are_idle(gt), I915_IDLE_ENGINES_TIMEOUT))
          intel_gt_set_wedged(gt);
  -    if (val & DROP_RETIRE)
-        intel_gt_retire_requests(gt);
-
      if (val & (DROP_IDLE | DROP_ACTIVE)) {
          ret = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
          if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c22f29c3faa0e..53c7474dde495 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -278,7 +278,7 @@ struct i915_gem_mm {
      u32 shrink_count;
  };
  -#define I915_IDLE_ENGINES_TIMEOUT (200) /* in ms */
+#define I915_IDLE_ENGINES_TIMEOUT (500) /* in ms */
    unsigned long i915_fence_context_timeout(const struct drm_i915_private *i915,
                       u64 context);




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux