2022-07-04 08:10:42

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH v3 0/2] Fix TLB invalidate issues with Broadwell

i915 selftest hangcheck is causing the i915 driver timeouts, as reported
by Intel CI bot:

http://gfx-ci.fi.intel.com/cibuglog-ng/issuefilterassoc/24297?query_key=42a999f48fa6ecce068bc8126c069be7c31153b4

When such test runs, the only output is:

[ 68.811639] i915: Performing live selftests with st_random_seed=0xe138eac7 st_timeout=500
[ 68.811792] i915: Running hangcheck
[ 68.811859] i915: Running intel_hangcheck_live_selftests/igt_hang_sanitycheck
[ 68.816910] i915 0000:00:02.0: [drm] Cannot find any crtc or sizes
[ 68.841597] i915: Running intel_hangcheck_live_selftests/igt_reset_nop
[ 69.346347] igt_reset_nop: 80 resets
[ 69.362695] i915: Running intel_hangcheck_live_selftests/igt_reset_nop_engine
[ 69.863559] igt_reset_nop_engine(rcs0): 709 resets
[ 70.364924] igt_reset_nop_engine(bcs0): 903 resets
[ 70.866005] igt_reset_nop_engine(vcs0): 659 resets
[ 71.367934] igt_reset_nop_engine(vcs1): 549 resets
[ 71.869259] igt_reset_nop_engine(vecs0): 553 resets
[ 71.882592] i915: Running intel_hangcheck_live_selftests/igt_reset_idle_engine
[ 72.383554] rcs0: Completed 16605 idle resets
[ 72.884599] bcs0: Completed 18641 idle resets
[ 73.385592] vcs0: Completed 17517 idle resets
[ 73.886658] vcs1: Completed 15474 idle resets
[ 74.387600] vecs0: Completed 17983 idle resets
[ 74.387667] i915: Running intel_hangcheck_live_selftests/igt_reset_active_engine
[ 74.889017] rcs0: Completed 747 active resets
[ 75.174240] intel_engine_reset(bcs0) failed, err:-110
[ 75.174301] bcs0: Completed 525 active resets

After that, the machine just silently hangs.

Bisecting the issue, the patch that introduced the regression is:

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

Reverting it fix the issues, but introduce other problems, as TLB
won't be invalidated anymore. So, instead, let's fix the root cause.

It turns that the TLB flush logic ends conflicting with i915 reset,
which is called during selftest hangcheck. So, the TLB cache should
be serialized together with i915 reset.

Tested on an Intel NUC5i7RYB with an i7-5557U Broadwell CPU.

v3:
- Removed the logic that would check if the engine is awake before doing
TLB flush invalidation as backporting PM logic up to Kernel 4.x could be
too painful. After getting this one merged, I'll submit a separate patch
with the PM awake logic.

v2:

- Reduced to bare minimum fixes, as this shoud be backported deeply
into stable.


Chris Wilson (2):
drm/i915/gt: Serialize GRDOM access between multiple engine resets
drm/i915/gt: Serialize TLB invalidates with GT resets

drivers/gpu/drm/i915/gt/intel_gt.c | 15 ++++++++++-
drivers/gpu/drm/i915/gt/intel_reset.c | 37 ++++++++++++++++++++-------
2 files changed, 42 insertions(+), 10 deletions(-)

--
2.36.1



2022-07-04 08:10:43

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH v3 1/2] drm/i915/gt: Serialize GRDOM access between multiple engine resets

From: Chris Wilson <[email protected]>

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.

Cc: [email protected] # Up to 4.4
Reported-by: Mika Kuoppala <[email protected]>
Signed-off-by: Chris Wilson <[email protected]>
Cc: Mika Kuoppala <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Acked-by: Thomas Hellström <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH v3 0/2] at: https://lore.kernel.org/all/[email protected]/

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;
}

--
2.36.1

2022-07-04 08:35:18

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH v3 2/2] drm/i915/gt: Serialize TLB invalidates with GT resets

From: Chris Wilson <[email protected]>

Avoid trying to invalidate the TLB in the middle of performing an
engine reset, as this may result in the reset timing out. Currently,
the TLB invalidate is only serialised by its own mutex, forgoing the
uncore lock, but we can take the uncore->lock as well to serialise
the mmio access, thereby serialising with the GDRST.

Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with
i915 selftest/hangcheck.

Cc: [email protected] # Up to 4.4
Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
Reported-by: Mauro Carvalho Chehab <[email protected]>
Tested-by: Mauro Carvalho Chehab <[email protected]>
Reviewed-by: Mauro Carvalho Chehab <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Thomas Hellström <[email protected]>
Cc: Andi Shyti <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH v3 0/2] at: https://lore.kernel.org/all/[email protected]/

drivers/gpu/drm/i915/gt/intel_gt.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 8da3314bb6bf..68c2b0d8f187 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -952,6 +952,20 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
mutex_lock(&gt->tlb_invalidate_lock);
intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);

+ spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
+
+ for_each_engine(engine, gt, id) {
+ struct reg_and_bit rb;
+
+ rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
+ if (!i915_mmio_reg_offset(rb.reg))
+ continue;
+
+ intel_uncore_write_fw(uncore, rb.reg, rb.bit);
+ }
+
+ spin_unlock_irq(&uncore->lock);
+
for_each_engine(engine, gt, id) {
/*
* HW architecture suggest typical invalidation time at 40us,
@@ -966,7 +980,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
if (!i915_mmio_reg_offset(rb.reg))
continue;

- intel_uncore_write_fw(uncore, rb.reg, rb.bit);
if (__intel_wait_for_register_fw(uncore,
rb.reg, rb.bit, 0,
timeout_us, timeout_ms,
--
2.36.1

2022-07-06 11:09:27

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] drm/i915/gt: Serialize GRDOM access between multiple engine resets

Hi Mauro and Chris,

On Mon, Jul 04, 2022 at 09:09:28AM +0100, Mauro Carvalho Chehab wrote:
> From: Chris Wilson <[email protected]>
>
> 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.
>
> Cc: [email protected] # Up to 4.4
> Reported-by: Mika Kuoppala <[email protected]>
> Signed-off-by: Chris Wilson <[email protected]>
> Cc: Mika Kuoppala <[email protected]>
> Reviewed-by: Andi Shyti <[email protected]>
> Acked-by: Thomas Hellstr?m <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>

sorry for the delay but I wanted to understand what has been
agreed between you and Tvrtko about the Cc'ing the stable list.

Anyway, I confirm my review here.

Andi

> ---
>
> To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH v3 0/2] at: https://lore.kernel.org/all/[email protected]/
>
> 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;
> }
>
> --
> 2.36.1

2022-07-06 11:38:52

by Andi Shyti

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] drm/i915/gt: Serialize TLB invalidates with GT resets

Hi Mauro and Chris,

On Mon, Jul 04, 2022 at 09:09:29AM +0100, Mauro Carvalho Chehab wrote:
> From: Chris Wilson <[email protected]>
>
> Avoid trying to invalidate the TLB in the middle of performing an
> engine reset, as this may result in the reset timing out. Currently,
> the TLB invalidate is only serialised by its own mutex, forgoing the
> uncore lock, but we can take the uncore->lock as well to serialise
> the mmio access, thereby serialising with the GDRST.
>
> Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with
> i915 selftest/hangcheck.
>
> Cc: [email protected] # Up to 4.4
> Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
> Reported-by: Mauro Carvalho Chehab <[email protected]>
> Tested-by: Mauro Carvalho Chehab <[email protected]>
> Reviewed-by: Mauro Carvalho Chehab <[email protected]>
> Cc: Chris Wilson <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: Thomas Hellstr?m <[email protected]>
> Cc: Andi Shyti <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
>
> To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH v3 0/2] at: https://lore.kernel.org/all/[email protected]/
>
> drivers/gpu/drm/i915/gt/intel_gt.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 8da3314bb6bf..68c2b0d8f187 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -952,6 +952,20 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
> mutex_lock(&gt->tlb_invalidate_lock);
> intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>
> + spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
> +
> + for_each_engine(engine, gt, id) {
> + struct reg_and_bit rb;
> +
> + rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
> + if (!i915_mmio_reg_offset(rb.reg))
> + continue;
> +
> + intel_uncore_write_fw(uncore, rb.reg, rb.bit);
> + }
> +
> + spin_unlock_irq(&uncore->lock);
> +

looks good,

Reviewed-by: Andi Shyti <[email protected]>

Thanks,
Andi

> for_each_engine(engine, gt, id) {
> /*
> * HW architecture suggest typical invalidation time at 40us,
> @@ -966,7 +980,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
> if (!i915_mmio_reg_offset(rb.reg))
> continue;
>
> - intel_uncore_write_fw(uncore, rb.reg, rb.bit);
> if (__intel_wait_for_register_fw(uncore,
> rb.reg, rb.bit, 0,
> timeout_us, timeout_ms,
> --
> 2.36.1

2022-07-07 21:51:11

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/gt: Serialize GRDOM access between multiple engine resets

On 04.07.2022 10:09, Mauro Carvalho Chehab wrote:
> From: Chris Wilson <[email protected]>
>
> 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.
>
> Cc: [email protected] # Up to 4.4
> Reported-by: Mika Kuoppala <[email protected]>
> Signed-off-by: Chris Wilson <[email protected]>
> Cc: Mika Kuoppala <[email protected]>
> Reviewed-by: Andi Shyti <[email protected]>
> Acked-by: Thomas Hellström <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>

Reviewed-by: Andrzej Hajda <[email protected]>

Regards
Andrzej

> ---
>
> To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH v3 0/2] at: https://lore.kernel.org/all/[email protected]/
>
> 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;
> }
>

2022-07-07 22:10:39

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/gt: Serialize TLB invalidates with GT resets

On 04.07.2022 10:09, Mauro Carvalho Chehab wrote:
> From: Chris Wilson <[email protected]>
>
> Avoid trying to invalidate the TLB in the middle of performing an
> engine reset, as this may result in the reset timing out. Currently,
> the TLB invalidate is only serialised by its own mutex, forgoing the
> uncore lock, but we can take the uncore->lock as well to serialise
> the mmio access, thereby serialising with the GDRST.
>
> Tested on a NUC5i7RYB, BIOS RYBDWi35.86A.0380.2019.0517.1530 with
> i915 selftest/hangcheck.
>
> Cc: [email protected] # Up to 4.4
> Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
> Reported-by: Mauro Carvalho Chehab <[email protected]>
> Tested-by: Mauro Carvalho Chehab <[email protected]>
> Reviewed-by: Mauro Carvalho Chehab <[email protected]>
> Cc: Chris Wilson <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: Thomas Hellström <[email protected]>
> Cc: Andi Shyti <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>

Reviewed-by: Andrzej Hajda <[email protected]>

Regards
Andrzej

> ---
>
> To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH v3 0/2] at: https://lore.kernel.org/all/[email protected]/
>
> drivers/gpu/drm/i915/gt/intel_gt.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 8da3314bb6bf..68c2b0d8f187 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -952,6 +952,20 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
> mutex_lock(&gt->tlb_invalidate_lock);
> intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>
> + spin_lock_irq(&uncore->lock); /* serialise invalidate with GT reset */
> +
> + for_each_engine(engine, gt, id) {
> + struct reg_and_bit rb;
> +
> + rb = get_reg_and_bit(engine, regs == gen8_regs, regs, num);
> + if (!i915_mmio_reg_offset(rb.reg))
> + continue;
> +
> + intel_uncore_write_fw(uncore, rb.reg, rb.bit);
> + }
> +
> + spin_unlock_irq(&uncore->lock);
> +
> for_each_engine(engine, gt, id) {
> /*
> * HW architecture suggest typical invalidation time at 40us,
> @@ -966,7 +980,6 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
> if (!i915_mmio_reg_offset(rb.reg))
> continue;
>
> - intel_uncore_write_fw(uncore, rb.reg, rb.bit);
> if (__intel_wait_for_register_fw(uncore,
> rb.reg, rb.bit, 0,
> timeout_us, timeout_ms,