> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Sent: Thursday, July 11, 2024 11:39 PM
> To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>
> Cc: Gote, Nitin R <nitin.r.gote@xxxxxxxxx>; Wilson, Chris P
> <chris.p.wilson@xxxxxxxxx>; tursulin@xxxxxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Shyti, Andi
> <andi.shyti@xxxxxxxxx>; Das, Nirmoy <nirmoy.das@xxxxxxxxx>;
> janusz.krzysztofik@xxxxxxxxxxxxxxx; Chris Wilson
> <chris.p.wilson@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3] drm/i915/gt: Do not consider preemption during
> execlists_dequeue for gen8
>
> On Thu, Jul 11, 2024 at 04:28:53PM +0000, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Nitin Gote
> > Sent: Thursday, July 11, 2024 9:32 AM
> > To: Wilson, Chris P <chris.p.wilson@xxxxxxxxx>; tursulin@xxxxxxxxxxx;
> > intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Shyti, Andi
> > <andi.shyti@xxxxxxxxx>; Das, Nirmoy <nirmoy.das@xxxxxxxxx>;
> > janusz.krzysztofik@xxxxxxxxxxxxxxx; Gote, Nitin R
> > <nitin.r.gote@xxxxxxxxx>; Chris Wilson
> > <chris.p.wilson@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxx> -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.
> > > Closes:
> > > https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11396
> > > Suggested-by: Andi Shyti <andi.shyti@xxxxxxxxx>
> > > Signed-off-by: Nitin Gote <nitin.r.gote@xxxxxxxxx>
> > > Cc: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx>
> > > CC: <stable@xxxxxxxxxxxxxxx> # 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
> > >
> > >
