Re: [PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

["Timur Kristóf" <timur.kristof@gmail.com>]

On 2026. július 14., kedd 22:41:33 közép-európai nyári idő Alex Deucher wrote:
> On Tue, Jul 14, 2026 at 4:14 PM Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> > On 13/07/2026 13:58, Timur Kristóf wrote:
> > > The gfx_v7_0_cp_compute_resume() function should only return
> > > success when all compute rings are actually functional.
> > > This will be especially important for soft reset which needs
> > > this to know whether the reset was successful.
> > > 
> > > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > > ---
> > > 
> > >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index
> > > 9c4b3ac27e1f..a1a9f3fc4567 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > @@ -3019,12 +3019,14 @@ static int gfx_v7_0_cp_compute_resume(struct
> > > amdgpu_device *adev)> > 
> > >       gfx_v7_0_cp_compute_enable(adev, true);
> > > 
> > > +     r = 0;
> > > +
> > > 
> > >       for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> > >       
> > >               ring = &adev->gfx.compute_ring[i];
> > > 
> > > -             amdgpu_ring_test_helper(ring);
> > > +             r |= amdgpu_ring_test_helper(ring);
> > > 
> > >       }
> > > 
> > > -     return 0;
> > > +     return r;
> > > 
> > >   }
> > >   
> > >   static void gfx_v7_0_cp_enable(struct amdgpu_device *adev, bool
> > >   enable)
> > 
> > Gfx8 and 9 (did not look further) do not do it like that. Should they?
> > Or there is more work there to be done first?

I actually added the same code for GFX8 in the previous series.
In gfx_v8_0_cp_test_all_rings() it checks that all rings are functional, and 
it calls that from gfx_v8_0_cp_resume().

I think it would be a good idea to do this for newer GPUs as well, but I 
haven't yet touched the code for those.

> > 
> > I actually might like this because maybe it gets us closer to removing
> > the ring->sched.ready hack but what I am just not sure if the idea was
> > to allow driver to function with some non-functional rings after resume.
> > Under the premise that if they initialized during init, then after
> > resume they must too, or if they don't, it is a transient glitch. I
> > don't know.. I am being imaginative here thinking about silly driver
> > workarounds for weird hardware glitches. It is much more likely this was
> > just an oversight and it is completely fine to to error out.
> > 
> > I have to defer to Alex and Christian on this one.
> 
> The reason for not checking the errors was because compute queue
> failure was not seen as fatal.  There are a lot of compute queues
> (relative to other engines), so if something happened, it seemed
> better to just continue in a degraded mode with fewer compute queues
> than to fail to resume in general.

We had a conversation about a similar topic (it was about UVD), where Alex 
said that in general we should prefer not to handle degraded functionality in 
amdgpu. I think the same principle should apply here.

1. My main problem with handling degraded functionality here is that I have 
never seen any issue where just some compute queues fail to initialize after 
boot or after suspend/resume. That means we can't meaningfully test that 
scenario, so we can't trust any code we write to handle that either.
2. It would be very tedious to keep track of which queues didn't work in the 
first place vs. which are those that don't work because of a bug in the soft 
reset code. I consider the soft reset as failed if not all queues work 
correctly.

Considering the above, I vote that we should just expect all queues to work 
correctly at initialization and after a recovery. What do you guys think?

Thanks & best regards,
Timur