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

["Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>]

Am 08.10.2018 um 19:58 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Tuesday, October 9, 2018 1:32 AM
>> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Alex Deucher <alexdeucher@gmail.com>
>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>> Subject: Re: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>> suspend
>>
>> 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@amd.com>; Alex Deucher
>> <alexdeucher@gmail.com>
>>>> Cc: amd-gfx list <amd-gfx@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@lists.freedesktop.org> On Behalf Of
>>>>> Zhu, Rex
>>>>> Sent: Monday, October 8, 2018 11:57 AM
>>>>> To: Alex Deucher <alexdeucher@gmail.com>
>>>>> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
>>>>> Subject: RE: [PATCH 1/5] drm/amdgpu: Don't reallocate ucode bo when
>>>>> suspend
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Alex Deucher <alexdeucher@gmail.com>
>>>>>> Sent: Thursday, October 4, 2018 11:35 AM
>>>>>> To: Zhu, Rex <Rex.Zhu@amd.com>
>>>>>> Cc: amd-gfx list <amd-gfx@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@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@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.
> We don't call amdgpu_ucode_fini_bo when suspend/sriov_reset.

Yeah and exactly that's the bug which should be fixed instead.

Christian.

>
> Rex
>
>> 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@lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx