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

[Gote, Nitin R]

Hi,

> -----Original Message-----
> From: Andi Shyti <andi.shyti at linux.intel.com>
> Sent: Tuesday, July 16, 2024 3:44 AM
> To: Gote, Nitin R <nitin.r.gote at intel.com>
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; Cavitt, Jonathan
> <jonathan.cavitt at intel.com>; Wilson, Chris P <chris.p.wilson at intel.com>;
> tursulin at ursulin.net; intel-gfx at lists.freedesktop.org; dri-
> devel at lists.freedesktop.org; Das, Nirmoy <nirmoy.das at intel.com>;
> janusz.krzysztofik at linux.intel.com; Chris Wilson
> <chris.p.wilson at linux.intel.com>; stable at vger.kernel.org
> Subject: Re: [PATCH v3] drm/i915/gt: Do not consider preemption during
> execlists_dequeue for gen8
> 
> Hi,
> 
> On Fri, Jul 12, 2024 at 03:25:23PM +0200, Gote, Nitin R wrote:
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > Sent: Thursday, July 11, 2024 11:39 PM
> > > To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
> > > Cc: Gote, Nitin R <nitin.r.gote at intel.com>; Wilson, Chris P
> > > <chris.p.wilson at intel.com>; tursulin at ursulin.net; intel-
> > > gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Shyti,
> > > Andi <andi.shyti at intel.com>; Das, Nirmoy <nirmoy.das at intel.com>;
> > > janusz.krzysztofik at linux.intel.com; Chris Wilson
> > > <chris.p.wilson at linux.intel.com>; stable at vger.kernel.org
> > > 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 at lists.freedesktop.org> On
> > > > Behalf Of Nitin Gote
> > > > Sent: Thursday, July 11, 2024 9:32 AM
> > > > To: Wilson, Chris P <chris.p.wilson at intel.com>;
> > > > tursulin at ursulin.net; intel-gfx at lists.freedesktop.org
> > > > Cc: dri-devel at lists.freedesktop.org; Shyti, Andi
> > > > <andi.shyti at intel.com>; Das, Nirmoy <nirmoy.das at intel.com>;
> > > > janusz.krzysztofik at linux.intel.com; Gote, Nitin R
> > > > <nitin.r.gote at intel.com>; Chris Wilson
> > > > <chris.p.wilson at linux.intel.com>; stable at 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 at 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.
> 
> Can I reword the commit log to something similar:
> 
>     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
> commit
>     bac24f59f454 ("drm/i915/execlists: Enable coarse preemption boundaries
> for
>     Gen8").
> 
>     The Gen8 platform only supports timeslicing and doesn't have a
> preemption
>     mechanism, as its engines do not have a preemption timer.
> 
>     Commit 751f82b353a6 ("drm/i915/gt: Only disable preemption on Gen8
> render
>     engines") addressed this issue only for render engines. This patch extends
>     that fix by ensuring that preemption is not considered for all engines on
>     Gen8 platforms.
> 
>     v4:
>      - Use the correct Fixes tag (Rodrigo Vivi)
>      - Reworded commit log (Andi Shyti)
> 
>     v3:
>      - Inside need_preempt(), condition of can_preempt() is not required
>        as simplified can_preempt() is enough. (Chris Wilson)
> 
>     v2: Simplify can_preempt() function (Tvrtko Ursulin)
> 
> Andi

Sure. You can.

Thank you
- Nitin