On 2023-11-29 22:36, Luben Tuikov wrote: > On 2023-11-29 15:49, Alex Deucher wrote: >> On Wed, Nov 29, 2023 at 3:10 PM Alex Deucher 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? > >> From 96e75b5218f7a124eafa53853681eef8fe567ab8 Mon Sep 17 00:00:00 2001 >> From: Alex Deucher >> 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@vps.thesusis.net > > Let's link the start of the thread. > > Regards, > Luben > >> Signed-off-by: Alex Deucher >> Cc: Phillip Susi >> Cc: Luben Tuikov >> --- >> 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); >> + If you're moving this past phase 1, there's another instance in amdgpu_device_ip_suspend(), which may need to be moved down. Regards, Luben >> 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 wrote: >>>> >>>> On Wed, Nov 29, 2023 at 11:41 AM Luben Tuikov wrote: >>>>> >>>>> On 2023-11-29 10:22, Alex Deucher wrote: >>>>>> On Wed, Nov 29, 2023 at 8:50 AM Alex Deucher wrote: >>>>>>> >>>>>>> On Tue, Nov 28, 2023 at 11:45 PM Luben Tuikov wrote: >>>>>>>> >>>>>>>> On 2023-11-28 17:13, Alex Deucher wrote: >>>>>>>>> On Mon, Nov 27, 2023 at 6:24 PM Phillip Susi wrote: >>>>>>>>>> >>>>>>>>>> Alex Deucher 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@vps.thesusis.net/ >>>>>>>> >>>>>>>> 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@gmail.com >>>>> >>>>> 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