On Thu, Jul 11, 2024 at 04:28:53PM +0000, Cavitt, Jonathan wrote:

-----Original Message----- From: Intel-gfx intel-gfx-bounces@lists.freedesktop.org On Behalf Of Nitin Gote Sent: Thursday, July 11, 2024 9:32 AM To: Wilson, Chris P chris.p.wilson@intel.com; tursulin@ursulin.net; intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org; Shyti, Andi andi.shyti@intel.com; Das, Nirmoy nirmoy.das@intel.com; janusz.krzysztofik@linux.intel.com; Gote, Nitin R nitin.r.gote@intel.com; Chris Wilson chris.p.wilson@linux.intel.com; stable@vger.kernel.org Subject: [PATCH v3] drm/i915/gt: Do not consider preemption during execlists_dequeue for gen8

...

We're seeing a GPU HANG issue on a CHV platform, which was caused by bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries for gen8").

Gen8 platform has only timeslice and doesn't support a preemption mechanism as engines do not have a preemption timer and doesn't send an irq if the preemption timeout expires.

That seems to mean the original can_preempt function was inaccurately built, so fixing it here makes the most sense to me, especially if it's causing problems.

Reviewed-by: Jonathan Cavitt jonathan.cavitt@intel.com -Jonathan Cavitt

...

So, add a fix to not consider preemption during dequeuing for gen8 platforms.

v2: Simplify can_preempt() function (Tvrtko Ursulin)

v3:

• Inside need_preempt(), condition of can_preempt() is not required as simplified can_preempt() is enough. (Chris Wilson)

Fixes: bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries for gen8")

Something strange in here...

This patch is not using directly or indirectly (I915_ENGINE_HAS_PREEMPTION) the can_preempt()...

...

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11396 Suggested-by: Andi Shyti andi.shyti@intel.com Signed-off-by: Nitin Gote nitin.r.gote@intel.com Cc: Chris Wilson chris.p.wilson@linux.intel.com CC: stable@vger.kernel.org # v5.2+

---

drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 21829439e686..72090f52fb85 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -3315,11 +3315,7 @@ static void remove_from_engine(struct i915_request *rq) static bool can_preempt(struct intel_engine_cs *engine) {

• if (GRAPHICS_VER(engine->i915) > 8)

• return true;
  

• /* GPGPU on bdw requires extra w/a; not implemented */
• return engine->class != RENDER_CLASS;

• return GRAPHICS_VER(engine->i915) > 8;

} static void kick_execlists(const struct i915_request *rq, int prio) -- 2.25.1

Hi Nitin,

...

...

...

We're seeing a GPU HANG issue on a CHV platform, which was caused by bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries

for gen8").

...

...

Gen8 platform has only timeslice and doesn't support a preemption mechanism as engines do not have a preemption timer and doesn't send an irq if the preemption timeout expires.

That seems to mean the original can_preempt function was inaccurately built, so fixing it here makes the most sense to me, especially if it's causing

problems.

...

Reviewed-by: Jonathan Cavitt jonathan.cavitt@intel.com -Jonathan Cavitt

...

So, add a fix to not consider preemption during dequeuing for gen8 platforms.

v2: Simplify can_preempt() function (Tvrtko Ursulin)

v3:

• Inside need_preempt(), condition of can_preempt() is not required as simplified can_preempt() is enough. (Chris Wilson)

Fixes: bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries for gen8")

Something strange in here...

This patch is not using directly or indirectly (I915_ENGINE_HAS_PREEMPTION) the can_preempt()...

Thank you Rodrigo for the review comment. Seems like you are right. Fixes: bac24f59f454 is misleading as it's not using can_preempt(). The bug could be from the commit bac24f59f454 as mentioned in the issue But this change fixes the original implementation of can_preempt() in below commit. Fixes: 751f82b353a6 ("drm/i915/gt: Only disable preemption on gen8 render engines").

I will update the Fixes in the commit description and will send in v4.

no need to resend it, I will update it before pushing.

Reviewed-by: Andi Shyti andi.shyti@linux.intel.com

I think the first mention in the commit log is correct, though, as that's the reason where the issue was generated.

Thanks, Andi

...

...

...

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11396 Suggested-by: Andi Shyti andi.shyti@intel.com Signed-off-by: Nitin Gote nitin.r.gote@intel.com Cc: Chris Wilson chris.p.wilson@linux.intel.com CC: stable@vger.kernel.org # v5.2+

---

drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 21829439e686..72090f52fb85 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -3315,11 +3315,7 @@ static void remove_from_engine(struct i915_request *rq)

static bool can_preempt(struct intel_engine_cs *engine) {

• if (GRAPHICS_VER(engine->i915) > 8)

• return true;
  

• /* GPGPU on bdw requires extra w/a; not implemented */
• return engine->class != RENDER_CLASS;

• return GRAPHICS_VER(engine->i915) > 8;

}

static void kick_execlists(const struct i915_request *rq, int prio)

2.25.1