Re: [PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

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

On 02.07.25 13:58, Arunpravin Paneer Selvam wrote:
> Hi Christian,
> 
> On 7/2/2025 1:27 PM, Christian König wrote:
>> On 01.07.25 21:08, Arunpravin Paneer Selvam wrote:
>>> Set the dirty bit when the memory resource is not cleared
>>> during BO release.
>>>
>>> v2(Christian):
>>>   - Drop the cleared flag set to false.
>>>   - Improve the amdgpu_vram_mgr_set_clear_state() function.
>>>
>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 1 -
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++-
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 9c5df35f05b7..86eb6d47dcc5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -409,7 +409,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>>>  		if (r) {
>>>  			goto error;
>>>  		} else if (wipe_fence) {
>>> -			amdgpu_vram_mgr_set_cleared(bo->resource);
>> Mhm, that looks incorrect to me.
>>
>> Why don't we consider the resource cleared after it go wiped during eviction?
> 
> Modifying the resource flag here doesn't go into effect until we call the drm_buddy_free_list() in amdgpu_vram_mgr_del(). This BO will be cleared once again after executing amdgpu_bo_release_notify(). With the new implementation, there's a chance that changing the resource flag the second time would cause the WARN_ON to occur. Hence I removed the resource cleared function call in amdgpu_move_blit. Thanks, Arun.

Something fishy is going on that we don't fully understand.

What happens here is that we move from VRAM to GTT, clear the VRAM BO after the move and set the flag.

When the BO is destroyed the it is backed by GTT and not VRAM any more, so no clear operation and no flag setting is performed.

It looks more like we forget to clear the flag in some cases.

Regards,
Christian.

> 
>> Regards,
>> Christian.
>>
>>>  			dma_fence_put(fence);
>>>  			fence = wipe_fence;
>>>  		}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>> index b256cbc2bc27..2c88d5fd87da 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>> @@ -66,7 +66,10 @@ to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
>>>  
>>>  static inline void amdgpu_vram_mgr_set_cleared(struct ttm_resource *res)
>>>  {
>>> -	to_amdgpu_vram_mgr_resource(res)->flags |= DRM_BUDDY_CLEARED;
>>> +	struct amdgpu_vram_mgr_resource *ares = to_amdgpu_vram_mgr_resource(res);
>>> +
>>> +	WARN_ON(ares->flags & DRM_BUDDY_CLEARED);
>>> +	ares->flags |= DRM_BUDDY_CLEARED;
>>>  }
>>>  
>>>  #endif