mbox series

[0/4] fixes for Adreno A5Xx preemption

Message ID 20240711100038.268803-1-vladimir.lypak@gmail.com (mailing list archive)
Headers show
Series fixes for Adreno A5Xx preemption | expand

Message

Vladimir Lypak July 11, 2024, 10 a.m. UTC
There are several issues with preemption on Adreno A5XX GPUs which
render system unusable if more than one priority level is used. Those
issues include persistent GPU faults and hangs, full UI lockups with
idling GPU.

---
Vladimir Lypak (4):
  drm/msm/a5xx: disable preemption in submits by default
  drm/msm/a5xx: properly clear preemption records on resume
  drm/msm/a5xx: fix races in preemption evaluation stage
  drm/msm/a5xx: workaround early ring-buffer emptiness check

 drivers/gpu/drm/msm/adreno/a5xx_gpu.c     | 18 ++++++++++----
 drivers/gpu/drm/msm/adreno/a5xx_gpu.h     | 12 ++++++---
 drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 30 ++++++++++++++++++++---
 3 files changed, 47 insertions(+), 13 deletions(-)
---
base-commit: 523b23f0bee3014a7a752c9bb9f5c54f0eddae88

Comments

Connor Abbott July 17, 2024, 9:40 a.m. UTC | #1
On Thu, Jul 11, 2024 at 11:10 AM Vladimir Lypak
<vladimir.lypak@gmail.com> wrote:
>
> There are several issues with preemption on Adreno A5XX GPUs which
> render system unusable if more than one priority level is used. Those
> issues include persistent GPU faults and hangs, full UI lockups with
> idling GPU.
>
> ---
> Vladimir Lypak (4):
>   drm/msm/a5xx: disable preemption in submits by default
>   drm/msm/a5xx: properly clear preemption records on resume
>   drm/msm/a5xx: fix races in preemption evaluation stage
>   drm/msm/a5xx: workaround early ring-buffer emptiness check
>
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     | 18 ++++++++++----
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.h     | 12 ++++++---
>  drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 30 ++++++++++++++++++++---
>  3 files changed, 47 insertions(+), 13 deletions(-)
> ---
> base-commit: 523b23f0bee3014a7a752c9bb9f5c54f0eddae88
> --
> 2.45.2
>

Hi Vladimir,

While looking at preemption on a7xx, where the overall logic is pretty
much the same, and I've been seeing the same "soft lockups". However
even after porting patch 3, it turns out that's not enough because
there's a different race. The sequence of events is something like
this:

1. Medium-prio app A submits to ring 2.
2. Ring 0 retires its last job and we start a preemption to ring 2.
3. High-prio app B submits to ring 0. It sees the preemption from step
2 ongoing and doesn't write the WTPR register or try to preempt.
4. The preemption finishes and we update WPTR.
5. App A's submit retires. We try to preempt, but the submit and
ring->cur write from step 3 happened on a different CPU and the write
hasn't landed yet so we skip it.

It's a bit tricky because write reordering is involved, but this seems
to be what's happening - everything except my speculation about the
delayed write to ring->cur being the problem comes straight from a
trace of this happening.

Rob suggested on IRC that we make retire handling happen on the same
workqueue as submissions, so that preempt_trigger is always
serialized, which IIUC would also make patch 3 unnecessary. What do
you think?

Best regards,

Connor
Vladimir Lypak July 17, 2024, 4:31 p.m. UTC | #2
On Wed, Jul 17, 2024 at 10:40:26AM +0100, Connor Abbott wrote:
> On Thu, Jul 11, 2024 at 11:10 AM Vladimir Lypak
> <vladimir.lypak@gmail.com> wrote:
> >
> > There are several issues with preemption on Adreno A5XX GPUs which
> > render system unusable if more than one priority level is used. Those
> > issues include persistent GPU faults and hangs, full UI lockups with
> > idling GPU.
> >
> > ---
> > Vladimir Lypak (4):
> >   drm/msm/a5xx: disable preemption in submits by default
> >   drm/msm/a5xx: properly clear preemption records on resume
> >   drm/msm/a5xx: fix races in preemption evaluation stage
> >   drm/msm/a5xx: workaround early ring-buffer emptiness check
> >
> >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     | 18 ++++++++++----
> >  drivers/gpu/drm/msm/adreno/a5xx_gpu.h     | 12 ++++++---
> >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 30 ++++++++++++++++++++---
> >  3 files changed, 47 insertions(+), 13 deletions(-)
> > ---
> > base-commit: 523b23f0bee3014a7a752c9bb9f5c54f0eddae88
> > --
> > 2.45.2
> >
> 
> Hi Vladimir,

Hi Connor!

> 
> While looking at preemption on a7xx, where the overall logic is pretty
> much the same, and I've been seeing the same "soft lockups". However
> even after porting patch 3, it turns out that's not enough because
> there's a different race. The sequence of events is something like
> this:
> 
> 1. Medium-prio app A submits to ring 2.
> 2. Ring 0 retires its last job and we start a preemption to ring 2.
> 3. High-prio app B submits to ring 0. It sees the preemption from step
> 2 ongoing and doesn't write the WTPR register or try to preempt.
> 4. The preemption finishes and we update WPTR.
At this point with patch 3 applied it should switch to ring 0 right away
because of trigger call in the end of a5xx_preempt_irq. Didn't you
forget it? Downstream has such call too. Even though it makes preemption
a little more aggressive it doesn't work without it.

> 5. App A's submit retires. We try to preempt, but the submit and
> ring->cur write from step 3 happened on a different CPU and the write
> hasn't landed yet so we skip it.

I don't think this is possible on modern CPUs. Could it be that retire
IRQ appeared earlier (while it was switching from 0 to 2) and you are
looking at msm_gpu_submit_retired trace event which is called from
retire work later.

> 
> It's a bit tricky because write reordering is involved, but this seems
> to be what's happening - everything except my speculation about the
> delayed write to ring->cur being the problem comes straight from a
> trace of this happening.
> 
> Rob suggested on IRC that we make retire handling happen on the same
> workqueue as submissions, so that preempt_trigger is always
> serialized, which IIUC would also make patch 3 unnecessary. What do
> you think?

In this patch series i have tried to do least amount of changes so it
could be easily back-ported. It isn't pretty but it works reliably for
me. Otherwise it would be fine to just disable preemption like it's done
on LTS before 5.4 and rework preemption in new kernel releases.

Kind regards,

Vladimir

> 
> Best regards,
> 
> Connor
Connor Abbott July 17, 2024, 5:52 p.m. UTC | #3
On Wed, Jul 17, 2024 at 5:33 PM Vladimir Lypak <vladimir.lypak@gmail.com> wrote:
>
> On Wed, Jul 17, 2024 at 10:40:26AM +0100, Connor Abbott wrote:
> > On Thu, Jul 11, 2024 at 11:10 AM Vladimir Lypak
> > <vladimir.lypak@gmail.com> wrote:
> > >
> > > There are several issues with preemption on Adreno A5XX GPUs which
> > > render system unusable if more than one priority level is used. Those
> > > issues include persistent GPU faults and hangs, full UI lockups with
> > > idling GPU.
> > >
> > > ---
> > > Vladimir Lypak (4):
> > >   drm/msm/a5xx: disable preemption in submits by default
> > >   drm/msm/a5xx: properly clear preemption records on resume
> > >   drm/msm/a5xx: fix races in preemption evaluation stage
> > >   drm/msm/a5xx: workaround early ring-buffer emptiness check
> > >
> > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     | 18 ++++++++++----
> > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.h     | 12 ++++++---
> > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 30 ++++++++++++++++++++---
> > >  3 files changed, 47 insertions(+), 13 deletions(-)
> > > ---
> > > base-commit: 523b23f0bee3014a7a752c9bb9f5c54f0eddae88
> > > --
> > > 2.45.2
> > >
> >
> > Hi Vladimir,
>
> Hi Connor!
>
> >
> > While looking at preemption on a7xx, where the overall logic is pretty
> > much the same, and I've been seeing the same "soft lockups". However
> > even after porting patch 3, it turns out that's not enough because
> > there's a different race. The sequence of events is something like
> > this:
> >
> > 1. Medium-prio app A submits to ring 2.
> > 2. Ring 0 retires its last job and we start a preemption to ring 2.
> > 3. High-prio app B submits to ring 0. It sees the preemption from step
> > 2 ongoing and doesn't write the WTPR register or try to preempt.
> > 4. The preemption finishes and we update WPTR.
> At this point with patch 3 applied it should switch to ring 0 right away
> because of trigger call in the end of a5xx_preempt_irq. Didn't you
> forget it? Downstream has such call too. Even though it makes preemption
> a little more aggressive it doesn't work without it.

Yes, I didn't apply that bit because it didn't seem necessary to fix
the original issue you described and it seemed like just an
optimization to make preemption more aggressive, however it does seem
to fix the issue. I can't verify that the issue you describe (the
retire IRQ arriving before preemption IRQ) is what's actually
happening because adding a tracepoint on retire seems to change the
timing enough so that the lockup doesn't happen, though. So I guess
I'll just have to assume that's what it was.

Given how subtle this is, enough that I missed it, maybe it's worth a
comment and an extra commit.

Also, I forgot to mention that while I was reading this over I found
another (theoretical) race - we could flush a submit in between
calling update_wptr() and set_preempt_state(PREEMPT_NONE) in
a5xx_preempt_irq() and never update wptr. I would fix it by renaming
PREEMPT_ABORT to PREEMPT_FINISH and pulling out the ABORT ->
update_wptr() -> NONE sequence in a5xx_preempt_trigger() into a
separate function that also gets called in a5xx_preempt_irq().

Connor

>
> > 5. App A's submit retires. We try to preempt, but the submit and
> > ring->cur write from step 3 happened on a different CPU and the write
> > hasn't landed yet so we skip it.
>
> I don't think this is possible on modern CPUs. Could it be that retire
> IRQ appeared earlier (while it was switching from 0 to 2) and you are
> looking at msm_gpu_submit_retired trace event which is called from
> retire work later.
>
> >
> > It's a bit tricky because write reordering is involved, but this seems
> > to be what's happening - everything except my speculation about the
> > delayed write to ring->cur being the problem comes straight from a
> > trace of this happening.
> >
> > Rob suggested on IRC that we make retire handling happen on the same
> > workqueue as submissions, so that preempt_trigger is always
> > serialized, which IIUC would also make patch 3 unnecessary. What do
> > you think?
>
> In this patch series i have tried to do least amount of changes so it
> could be easily back-ported. It isn't pretty but it works reliably for
> me. Otherwise it would be fine to just disable preemption like it's done
> on LTS before 5.4 and rework preemption in new kernel releases.
>
> Kind regards,
>
> Vladimir
>
> >
> > Best regards,
> >
> > Connor