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

[Samuel Zhang <guoqzhan@amd.com>]

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.

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,