[PATCH 1/2] drm/amdgpu: Unmap BO memory before calling amdgpu_bo_unref()

[Thomas Zimmermann]

Hi

Am 20.06.24 um 17:50 schrieb Christian König:
> Am 20.06.24 um 16:44 schrieb Thomas Zimmermann:
>> Prepares for using ttm_bo_vmap() and ttm_bo_vunmap() in amdgpu. Both
>> require the caller to hold the GEM reservation lock, which is not the
>> case while releasing a buffer object. Hence, push a possible call to
>> unmap out from the buffer-object release code. Warn if a buffer object
>> with mapped pages is supposed to be released.
>
> Yeah, I've looked into this a while ago as well and that unfortunately 
> won't work like this.
>
> Amdgpu also uses ttm_bo_kmap() on user allocations, so the 
> amdgpu_bo_kunmap() in amdgpu_bo_destroy() is a must have.

Is there a testcase (igt-gpu-tools ?) that runs this code?  I've tested 
these patches by booting and running a 3d game under X11. But I didn't 
expect that to fully cover all cases.

Best regards
Thomas

>
> On the other hand I'm pretty sure that calling ttm_bo_vunmap() without 
> holding the reservation lock is ok in this situation.
>
> After all it's guaranteed that nobody else is having a reference to 
> the BO any more.
>
> Regards,
> Christian.
>
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index a1b7438c43dc8..d58b11ea0ead5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -58,7 +58,12 @@ static void amdgpu_bo_destroy(struct 
>> ttm_buffer_object *tbo)
>>   {
>>       struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
>>   -    amdgpu_bo_kunmap(bo);
>> +    /*
>> +     * BO memory pages should be unmapped at this point. Call
>> +     * amdgpu_bo_kunmap() before releasing the BO.
>> +     */
>> +    if (drm_WARN_ON_ONCE(bo->tbo.base.dev, bo->kmap.bo))
>> +        amdgpu_bo_kunmap(bo);
>>         if (bo->tbo.base.import_attach)
>>           drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg);
>> @@ -450,9 +455,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, 
>> u64 *gpu_addr,
>> WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend);
>>         if (likely(amdgpu_bo_reserve(*bo, true) == 0)) {
>> -        if (cpu_addr)
>> -            amdgpu_bo_kunmap(*bo);
>> -
>> +        amdgpu_bo_kunmap(*bo);
>>           amdgpu_bo_unpin(*bo);
>>           amdgpu_bo_unreserve(*bo);
>>       }
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)