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

[Andi Shyti <andi.shyti@linux.intel.com>]

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