RE: [PATCH v3] drm/i915/gt: Do not consider preemption during execlists_dequeue for gen8

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

 



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





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux