Hi,
> -----Original Message-----
> From: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Sent: Tuesday, July 16, 2024 3:44 AM
> To: Gote, Nitin R <nitin.r.gote@xxxxxxxxx>
> Cc: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Cavitt, Jonathan
> <jonathan.cavitt@xxxxxxxxx>; Wilson, Chris P <chris.p.wilson@xxxxxxxxx>;
> tursulin@xxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; 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
>
> Hi,
>
> On Fri, Jul 12, 2024 at 03:25:23PM +0200, Gote, Nitin R wrote:
> > > -----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.
>
> 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
