Re: [PATCH 5/6] drm/i915/gt: Serialize GRDOM access between multiple engine resets

On 15/06/2022 16:27, Mauro Carvalho Chehab wrote:

From: Chris Wilson chris.p.wilson@intel.com

Don't allow two engines to be reset in parallel, as they would both try to select a reset bit (and send requests to common registers) and wait on that register, at the same time. Serialize control of the reset requests/acks using the uncore->lock, which will also ensure that no other GT state changes at the same time as the actual reset.

Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Ah okay I get it, the fixes tag was applied indiscriminately to the whole series. :) It definitely does not belong in this patch.

Otherwise LGTM:

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com

Regards,

Tvrtko

Reported-by: Mika Kuoppala mika.kuoppala@linux.intel.com Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Mika Kuoppala mika.kuoppala@linux.intel.com Cc: Andi Shyti andi.shyti@intel.com Cc: stable@vger.kernel.org Acked-by: Thomas Hellström thomas.hellstrom@linux.intel.com Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org

---

See [PATCH 0/6] at: https://lore.kernel.org/all/cover.1655306128.git.mchehab@kernel.org/

drivers/gpu/drm/i915/gt/intel_reset.c | 37 ++++++++++++++++++++------- 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index a5338c3fde7a..c68d36fb5bbd 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -300,9 +300,9 @@ static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask) return err; } -static int gen6_reset_engines(struct intel_gt *gt,

• 	      intel_engine_mask_t engine_mask,
  

• 	      unsigned int retry)
  

+static int __gen6_reset_engines(struct intel_gt *gt,

• 		intel_engine_mask_t engine_mask,
  

• 		unsigned int retry)
  

  { struct intel_engine_cs *engine; u32 hw_mask;

@@ -321,6 +321,20 @@ static int gen6_reset_engines(struct intel_gt *gt, return gen6_hw_domain_reset(gt, hw_mask); } +static int gen6_reset_engines(struct intel_gt *gt,

• 	      intel_engine_mask_t engine_mask,
  

• 	      unsigned int retry)
  

+{

• unsigned long flags;
• int ret;
• spin_lock_irqsave(&gt->uncore->lock, flags);
• ret = __gen6_reset_engines(gt, engine_mask, retry);
• spin_unlock_irqrestore(&gt->uncore->lock, flags);
• return ret;

+}

• static struct intel_engine_cs *find_sfc_paired_vecs_engine(struct intel_engine_cs *engine) { int vecs_id;

@@ -487,9 +501,9 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine) rmw_clear_fw(uncore, sfc_lock.lock_reg, sfc_lock.lock_bit); } -static int gen11_reset_engines(struct intel_gt *gt,

• 	       intel_engine_mask_t engine_mask,
  

• 	       unsigned int retry)
  

+static int __gen11_reset_engines(struct intel_gt *gt,

• 		 intel_engine_mask_t engine_mask,
  

• 		 unsigned int retry)
  

  { struct intel_engine_cs *engine; intel_engine_mask_t tmp;

@@ -583,8 +597,11 @@ static int gen8_reset_engines(struct intel_gt *gt, struct intel_engine_cs *engine; const bool reset_non_ready = retry >= 1; intel_engine_mask_t tmp;

• unsigned long flags; int ret;

• spin_lock_irqsave(&gt->uncore->lock, flags);
• for_each_engine_masked(engine, gt, engine_mask, tmp) { ret = gen8_engine_reset_prepare(engine); if (ret && !reset_non_ready)

@@ -612,17 +629,19 @@ static int gen8_reset_engines(struct intel_gt *gt, * This is best effort, so ignore any error from the initial reset. */ if (IS_DG2(gt->i915) && engine_mask == ALL_ENGINES)

• gen11_reset_engines(gt, gt->info.engine_mask, 0);
  

• __gen11_reset_engines(gt, gt->info.engine_mask, 0);
  

if (GRAPHICS_VER(gt->i915) >= 11)

• ret = gen11_reset_engines(gt, engine_mask, retry);
  

• ret = __gen11_reset_engines(gt, engine_mask, retry);
  

  else

• ret = gen6_reset_engines(gt, engine_mask, retry);
  

• ret = __gen6_reset_engines(gt, engine_mask, retry);
  

skip_reset: for_each_engine_masked(engine, gt, engine_mask, tmp) gen8_engine_reset_cancel(engine);

• spin_unlock_irqrestore(&gt->uncore->lock, flags);
• return ret; }