[PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when suspend

[Christian König]

Am 08.10.2018 um 18:30 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Deucher, Alexander
>> Sent: Tuesday, October 9, 2018 12:21 AM
>> To: Zhu, Rex <Rex.Zhu at amd.com>; Alex Deucher <alexdeucher at gmail.com>
>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>> suspend
>>
>>> -----Original Message-----
>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>> Zhu, Rex
>>> Sent: Monday, October 8, 2018 11:57 AM
>>> To: Alex Deucher <alexdeucher at gmail.com>
>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>> suspend
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alex Deucher <alexdeucher at gmail.com>
>>>> Sent: Thursday, October 4, 2018 11:35 AM
>>>> To: Zhu, Rex <Rex.Zhu at amd.com>
>>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
>>>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>> suspend
>>>>
>>>> On Wed, Oct 3, 2018 at 7:11 AM Rex Zhu <Rex.Zhu at amd.com> wrote:
>>>>> driver don't release the ucode memory when suspend. so don't need
>>>>> to allocate bo when resume back.
>>>>>
>>>>> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> index 9878212..adfeb93 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>>>> @@ -434,7 +434,7 @@ int amdgpu_ucode_init_bo(struct
>> amdgpu_device
>>>> *adev)
>>>>>                  return 0;
>>>>>          }
>>>>>
>>>>> -       if (!adev->in_gpu_reset) {
>>>>> +       if (!adev->in_gpu_reset && !adev->in_suspend) {
>>>>>                  err = amdgpu_bo_create_kernel(adev,
>>>>> adev->firmware.fw_size,
>>>> PAGE_SIZE,
>>>>>                                          amdgpu_sriov_vf(adev) ?
>>>> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
>>>>>                                          &adev->firmware.fw_buf,
>>>> Not sure if we support S3 in SR-IOV, but I think this will break it
>>>> because we'll lose vram contents and not re-init it.
>>> Confirm with SR-IOV team, S3 was not supported in SR-IOV.
>>>
>>>   But I still confused why this patch will break the suspend if in SRIOV case?
>> Pinned buffers don't get evicted so if we lose VRAM due to a gpu reset or S3,
>> the data is lost.  GTT is retained since the OS manages that.
> The gart table was unpinned when suspend.so don't need to create the bo again. we still copy the ucode to the bo.
> And in baremetal,  this function can return directly for S3.

That's irrelevant.

The whole code is buggy since amdgpu_ucode_fini_bo() will drop the BO 
independent if we are in reset or in suspend.

The correct handling here is to remove the if all together and make sure 
amdgpu_bo_create_kernel() is ALWAYS called.

Cause then it is always re-created if it isn't there already.

Alternatively we could fix up the callers of amdgpu_ucode_init_bo() and 
amdgpu_ucode_fini_bo() to be correctly balanced.

Christian.

>
> Rex
>
>
>> Alex
>>
>>> Rex
>>>
>>>> Alex
>>>>
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx