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

[Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com>]

On 7/2/2025 7:11 PM, Christian König wrote:
> 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.

Got it. I tried to add back the resource cleared after being wiped 
during eviction and verified it, and I didn't observe WARN_ON messages.

I will update the patch and send the next version.

Thanks,

Arun.

>
> 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