Re: Radeon regression in 6.6 kernel

[Luben Tuikov <ltuikov89@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 6140 bytes --]

On 2023-11-29 15:49, Alex Deucher wrote:
> On Wed, Nov 29, 2023 at 3:10 PM Alex Deucher <alexdeucher@gmail.com> 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 <alexander.deucher@amd.com>
> 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 <alexander.deucher@amd.com>
> Cc: Phillip Susi <phill@thesusis.net>
> Cc: Luben Tuikov <ltuikov89@gmail.com>
> ---
>  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@gmail.com> wrote:
>>>
>>> On Wed, Nov 29, 2023 at 11:41 AM Luben Tuikov <ltuikov89@gmail.com> wrote:
>>>>
>>>> On 2023-11-29 10:22, Alex Deucher wrote:
>>>>> On Wed, Nov 29, 2023 at 8:50 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Nov 28, 2023 at 11:45 PM Luben Tuikov <ltuikov89@gmail.com> wrote:
>>>>>>>
>>>>>>> On 2023-11-28 17:13, Alex Deucher wrote:
>>>>>>>> On Mon, Nov 27, 2023 at 6:24 PM Phillip Susi <phill@thesusis.net> wrote:
>>>>>>>>>
>>>>>>>>> Alex Deucher <alexdeucher@gmail.com> 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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]