Re: [PATCH 1/3] drm/amdgpu: move GTT to SHM after eviction for hibernation

["Christian König" <christian.koenig@amd.com>]

On 02.07.25 09:28, Samuel Zhang wrote:
> 
> On 2025/7/1 16:22, Christian König wrote:
>> On 01.07.25 10:18, Zhang, GuoQing (Sam) wrote:
>>> [AMD Official Use Only - AMD Internal Distribution Only]
>>>
>>>
>>> Hi Christian,
>>>
>>>  
>>> Thank you for the feedback.
>>>
>>>  
>>> For “return ret < 0 ? ret : 0;”, it is equivalent to “return ret;” since ret is always <= 0 after the loop.
>> No it isn't.
>>
>> ttm_global_swapout() returns the number of pages swapped out and only a negative error code if something went wrong.
> 
> 
> /**
>  * move GTT BOs to shmem for hibernation.
>  *
>  * returns 0 on success, negative on failure.
>  */
> int ttm_device_prepare_hibernation(void)
> {
>     struct ttm_operation_ctx ctx = {
>         .interruptible = false,
>         .no_wait_gpu = false,
>         .force_alloc = true
>     };
>     int ret;
> 
>     do {
>         ret = ttm_global_swapout(&ctx, GFP_KERNEL);
>     } while (ret > 0);
>     return ret;
> }
> 
> This is the new code version.
> If ttm_global_swapout() return positive number, the while loop will continue to the next iteration.
> The while loop stops only when ttm_global_swapout() returns 0 or negative number. In both case, the new function can just return the ret.

Ok, now I at least got what you wanted to do. But that is not really what I had in mind and isn't really good coding style.

Please use ttm_device_swapout() instead of ttm_global_swapout(), apart from that we can probably keep it that way.

Regards,
Christian.

> 
> The ret values printed in the do while loop:
> [   53.745892] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 512
> [   53.950975] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 35840
> [   53.951713] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 9
> [   67.712196] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 2187264
> [   67.713726] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 512
> [   67.759212] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 32768
> [   67.761946] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1024
> [   67.762685] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 85
> [   67.763518] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 175
> [   67.767318] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 2367
> [   67.767942] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1
> [   67.768499] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1
> [   67.769054] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1
> ...
> [   67.783554] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1
> [   67.785755] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1
> [   67.788607] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 1
> [   67.789906] [TTM DEVICE] ttm_device_prepare_hibernation:164 ret 0
> 
> 
> Regards
> Sam
> 
> 
> 
>>
>> And it's probably not a good idea to return that from the new function.
>>
>> Regards,
>> Christian.
>>
>>>  
>>> For all other comments, I will revise the patch accordingly in v2.
>>>
>>>  
>>> Regards
>>>
>>> Sam
>>>
>>>  
>>>  
>>> *From: *Koenig, Christian <Christian.Koenig@amd.com>
>>> *Date: *Monday, June 30, 2025 at 19:54
>>> *To: *Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, rafael@kernel.org <rafael@kernel.org>, len.brown@intel.com <len.brown@intel.com>, pavel@kernel.org <pavel@kernel.org>, Deucher, Alexander <Alexander.Deucher@amd.com>, Limonciello, Mario <Mario.Limonciello@amd.com>, Lazar, Lijo <Lijo.Lazar@amd.com>
>>> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Ma, Qing (Mark) <Qing.Ma@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>, dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>, linux-pm@vger.kernel.org <linux-pm@vger.kernel.org>, linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
>>> *Subject: *Re: [PATCH 1/3] drm/amdgpu: move GTT to SHM after eviction for hibernation
>>>
>>> On 30.06.25 12:41, Samuel Zhang wrote:
>>>> When hibernate with data center dGPUs, huge number of VRAM BOs evicted
>>>> to GTT and takes too much system memory. This will cause hibernation
>>>> fail due to insufficient memory for creating the hibernation image.
>>>>
>>>> Move GTT BOs to shmem in KMD, then shmem to swap disk in kernel
>>>> hibernation code to make room for hibernation image.
>>> This should probably be two patches, one for TTM and then an amdgpu patch to forward the event.
>>>
>>>> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 ++++++++++++-
>>>>    drivers/gpu/drm/ttm/ttm_resource.c      | 18 ++++++++++++++++++
>>>>    include/drm/ttm/ttm_resource.h          |  1 +
>>>>    3 files changed, 31 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 4d57269c9ca8..5aede907a591 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -2889,6 +2889,7 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>>>>    int amdgpu_ttm_evict_resources(struct amdgpu_device *adev, int mem_type)
>>>>    {
>>>>          struct ttm_resource_manager *man;
>>>> +     int r;
>>>>             switch (mem_type) {
>>>>          case TTM_PL_VRAM:
>>>> @@ -2903,7 +2904,17 @@ int amdgpu_ttm_evict_resources(struct amdgpu_device *adev, int mem_type)
>>>>                  return -EINVAL;
>>>>          }
>>>>    -     return ttm_resource_manager_evict_all(&adev->mman.bdev, man);
>>>> +     r = ttm_resource_manager_evict_all(&adev->mman.bdev, man);
>>>> +     if (r) {
>>>> +             DRM_ERROR("Failed to evict memory type %d\n", mem_type);
>>>> +             return r;
>>>> +     }
>>>> +     if (adev->in_s4 && mem_type == TTM_PL_VRAM) {
>>>> +             r = ttm_resource_manager_swapout();
>>>> +             if (r)
>>>> +                     DRM_ERROR("Failed to swap out, %d\n", r);
>>>> +     }
>>>> +     return r;
>>>>    }
>>>>       #if defined(CONFIG_DEBUG_FS)
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> index fd41b56e2c66..07b1f5a5afc2 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_resource.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> @@ -534,6 +534,24 @@ void ttm_resource_manager_init(struct ttm_resource_manager *man,
>>>>    }
>>>>    EXPORT_SYMBOL(ttm_resource_manager_init);
>>>>    +int ttm_resource_manager_swapout(void)
>>> This needs documentation, better placement and a better name.
>>>
>>> First of all put it into ttm_device.c instead of the resource manager.
>>>
>>> Then call it something like ttm_device_prepare_hibernation or similar.
>>>
>>>
>>>> +{
>>>> +     struct ttm_operation_ctx ctx = {
>>>> +             .interruptible = false,
>>>> +             .no_wait_gpu = false,
>>>> +             .force_alloc = true
>>>> +     };
>>>> +     int ret;
>>>> +
>>>> +     while (true) {
>>> Make that:
>>>
>>> do {
>>>          ret = ...
>>> } while (ret > 0);
>>>
>>>> +             ret = ttm_global_swapout(&ctx, GFP_KERNEL);
>>>> +             if (ret <= 0)
>>>> +                     break;
>>>> +     }
>>>> +     return ret;
>>> It's rather pointless to return the number of swapped out pages.
>>>
>>> Make that "return ret < 0 ? ret : 0;
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +}
>>>> +EXPORT_SYMBOL(ttm_resource_manager_swapout);
>>>> +
>>>>    /*
>>>>     * ttm_resource_manager_evict_all
>>>>     *
>>>> diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
>>>> index b873be9597e2..46181758068e 100644
>>>> --- a/include/drm/ttm/ttm_resource.h
>>>> +++ b/include/drm/ttm/ttm_resource.h
>>>> @@ -463,6 +463,7 @@ void ttm_resource_manager_init(struct ttm_resource_manager *man,
>>>>       int ttm_resource_manager_evict_all(struct ttm_device *bdev,
>>>>                                     struct ttm_resource_manager *man);
>>>> +int ttm_resource_manager_swapout(void);
>>>>       uint64_t ttm_resource_manager_usage(struct ttm_resource_manager *man);
>>>>    void ttm_resource_manager_debug(struct ttm_resource_manager *man,