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

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

Am 21.06.24 um 09:32 schrieb 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.

You need a hardware generation and use case which needs patching or 
inspection of IBs.

Video decoding on old SI or CIK hardware generation should probably do 
the trick.

Regards,
Christian.

>
> 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@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);
>>>       }
>>
>