On Wed, Nov 29, 2023 at 10:36 PM Luben Tuikov <ltuikov89@xxxxxxxxx> wrote:
>
> On 2023-11-29 15:49, Alex Deucher wrote:
> > On Wed, Nov 29, 2023 at 3:10 PM Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
> >>
> >> Actually I think I see the problem. I'll try and send out a patch
> >> later today to test.
> >
> > Does the attached patch fix it?
>
> Thanks for the patch, Alex.
>
> Is it possible for AMD to also reproduce this issue and test this patch on a Navi23 system?
I haven't had a chance to dig into it much due to LPC and thanksgiving
and other end of year stuff.
>
> > From 96e75b5218f7a124eafa53853681eef8fe567ab8 Mon Sep 17 00:00:00 2001
> > From: Alex Deucher <alexander.deucher@xxxxxxx>
> > Date: Wed, 29 Nov 2023 15:44:25 -0500
> > Subject: [PATCH] drm/amdgpu: fix buffer funcs setting order on suspend
> >
> > We need to make disable this after the last eviction
>
> "make disable" --> "disable"
>
> > call, but before we disable the SDMA IP.
> >
> > Fixes: b70438004a14 ("drm/amdgpu: move buffer funcs setting up a level")
> > Link: https://lists.freedesktop.org/archives/amd-gfx/2023-November/101197.html
>
> Link: https://lore.kernel.org/r/87edgv4x3i.fsf@xxxxxxxxxxxxxxxx
>
> Let's link the start of the thread.
Thanks, I will update the patch.
Alex
>
> Regards,
> Luben
>
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > Cc: Phillip Susi <phill@xxxxxxxxxxxx>
> > Cc: Luben Tuikov <ltuikov89@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index b5edf40b5d03..78553e027db4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4531,8 +4531,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
> >
> > amdgpu_ras_suspend(adev);
> >
> > - amdgpu_ttm_set_buffer_funcs_status(adev, false);
> > -
> > amdgpu_device_ip_suspend_phase1(adev);
> >
> > if (!adev->in_s0ix)
> > @@ -4542,6 +4540,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
> > if (r)
> > return r;
> >
> > + amdgpu_ttm_set_buffer_funcs_status(adev, false);
> > +
> > amdgpu_fence_driver_hw_fini(adev);
> >
> > amdgpu_device_ip_suspend_phase2(adev);
>
> >
> > Alex
> >
> >>
> >> Alex
> >>
> >> On Wed, Nov 29, 2023 at 1:52 PM Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
> >>>
> >>> On Wed, Nov 29, 2023 at 11:41 AM Luben Tuikov <ltuikov89@xxxxxxxxx> wrote:
> >>>>
> >>>> On 2023-11-29 10:22, Alex Deucher wrote:
> >>>>> On Wed, Nov 29, 2023 at 8:50 AM Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On Tue, Nov 28, 2023 at 11:45 PM Luben Tuikov <ltuikov89@xxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>> On 2023-11-28 17:13, Alex Deucher wrote:
> >>>>>>>> On Mon, Nov 27, 2023 at 6:24 PM Phillip Susi <phill@xxxxxxxxxxxx> wrote:
> >>>>>>>>>
> >>>>>>>>> Alex Deucher <alexdeucher@xxxxxxxxx> writes:
> >>>>>>>>>
> >>>>>>>>>>> In that case those are the already known problems with the scheduler
> >>>>>>>>>>> changes, aren't they?
> >>>>>>>>>>
> >>>>>>>>>> Yes. Those changes went into 6.7 though, not 6.6 AFAIK. Maybe I'm
> >>>>>>>>>> misunderstanding what the original report was actually testing. If it
> >>>>>>>>>> was 6.7, then try reverting:
> >>>>>>>>>> 56e449603f0ac580700621a356d35d5716a62ce5
> >>>>>>>>>> b70438004a14f4d0f9890b3297cd66248728546c
> >>>>>>>>>
> >>>>>>>>> At some point it was suggested that I file a gitlab issue, but I took
> >>>>>>>>> this to mean it was already known and being worked on. -rc3 came out
> >>>>>>>>> today and still has the problem. Is there a known issue I could track?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> At this point, unless there are any objections, I think we should just
> >>>>>>>> revert the two patches
> >>>>>>> Uhm, no.
> >>>>>>>
> >>>>>>> Why "the two" patches?
> >>>>>>>
> >>>>>>> This email, part of this thread,
> >>>>>>>
> >>>>>>> https://lore.kernel.org/all/87r0kircdo.fsf@xxxxxxxxxxxxxxxx/
> >>>>>>>
> >>>>>>> clearly states that reverting *only* this commit,
> >>>>>>> 56e449603f0ac5 drm/sched: Convert the GPU scheduler to variable number of run-queues
> >>>>>>> *does not* mitigate the failed suspend. (Furthermore, this commit doesn't really change
> >>>>>>> anything operational, other than using an allocated array, instead of a static one, in DRM,
> >>>>>>> while the 2nd patch is solely contained within the amdgpu driver code.)
> >>>>>>>
> >>>>>>> Leaving us with only this change,
> >>>>>>> b70438004a14f4 drm/amdgpu: move buffer funcs setting up a level
> >>>>>>> to be at fault, as the kernel log attached in the linked email above shows.
> >>>>>>>
> >>>>>>> The conclusion is that only b70438004a14f4 needs reverting.
> >>>>>>
> >>>>>> b70438004a14f4 was a fix for 56e449603f0ac5. Without b70438004a14f4,
> >>>>>> 56e449603f0ac5 breaks amdgpu.
> >>>>>
> >>>>> We can try and re-enable it in the next kernel. I'm just not sure
> >>>>> we'll be able to fix this in time for 6.7 with the holidays and all
> >>>>> and I don't want to cause a lot of scheduler churn at the end of the
> >>>>> 6.7 cycle if we hold off and try and fix it. Reverting seems like the
> >>>>> best short term solution.
> >>>>
> >>>> A lot of subsequent code has come in since commit 56e449603f0ac5, as it opened
> >>>> the opportunity for a 1-to-1 relationship between an entity and a scheduler.
> >>>> (Should've always been the case, from the outset. Not sure why it was coded as
> >>>> a fixed-size array.)
> >>>>
> >>>> Given that commit 56e449603f0ac5 has nothing to do with amdgpu, and the problem
> >>>> is wholly contained in amdgpu, and no other driver has this problem, there is
> >>>> no reason to have to "churn", i.e. go back and forth in DRM, only to cover up
> >>>> an init bug in amdgpu. See the response I just sent in @this thread:
> >>>> https://lore.kernel.org/r/05007cb0-871e-4dc7-af58-1351f4ba43e2@xxxxxxxxx
> >>>>
> >>>> And it's not like this issue is unknown. I first posted about it on 2023-10-16.
> >>>>
> >>>> Ideally, amdgpu would just fix their init code.
> >>>
> >>> You can't make changes to core code that break other drivers.
> >>> Arguably 56e449603f0ac5 should not have gone in in the first place if
> >>> it broke amdgpu. b70438004a14f4 was the code to fix amdgpu's init
> >>> code, but as a side effect it seems to have broken suspend for some
> >>> users.
> >>>
> >>> Alex
