[RFC PATCH 0/2] drm/amdgpu: Convert to ttm_bo_vmap()

[RFC PATCH 0/2] drm/amdgpu: Convert to ttm_bo_vmap()

[Thomas Zimmermann]

Convert amdgpu to use ttm_bo_vmap() instead of ttm_bo_kmap(). The
latter is deprecated. Passing ttm_bo_vmap()'s locking validation 
requires to remove the implicit unmap from the BO release path.

Smoke-tested with an Radeon RX 460.

There's similar patchset for xe at [1], which requires additional
changes.

[1] https://lore.kernel.org/dri-devel/20240614133556.11378-1-tzimmermann@suse.de/

Thomas Zimmermann (2):
  drm/amdgpu: Unmap BO memory before calling amdgpu_bo_unref()
  drm/amdgpu: Convert to ttm_bo_vmap() et al

 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 25 ++++++++++++++--------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  4 +++-
 2 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.45.2

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

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

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

[PATCH 2/2] drm/amdgpu: Convert to ttm_bo_vmap() et al

[Thomas Zimmermann]

Replace each call to ttm_bo_kmap() with a call to ttm_bo_vmap().
Same for ttm_bo_kunmap() and ttm_bo_vunmap(). There's now one less
driver depending on the deprecated ttm_bo_kmap().

Also allows for dropping struct ttm_bo_kmap_obj in favor of struct
iosys_map, which is the preferred representation of BO memory mappings.
Manual type conversion in amdgpu_bo_kptr() is required to make the
returned pointer usable within amdgpu. In a follow-up patch, amdgpu
should be convert to use struct iosys_map directly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 16 ++++++++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  4 +++-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d58b11ea0ead5..baa60e25c13e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -62,7 +62,7 @@ static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo)
 	 * 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))
+	if (drm_WARN_ON_ONCE(bo->tbo.base.dev, !iosys_map_is_null(&bo->map)))
 		amdgpu_bo_kunmap(bo);
 
 	if (bo->tbo.base.import_attach)
@@ -802,7 +802,7 @@ int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr)
 		return 0;
 	}
 
-	r = ttm_bo_kmap(&bo->tbo, 0, PFN_UP(bo->tbo.base.size), &bo->kmap);
+	r = ttm_bo_vmap(&bo->tbo, &bo->map);
 	if (r)
 		return r;
 
@@ -823,9 +823,12 @@ int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr)
  */
 void *amdgpu_bo_kptr(struct amdgpu_bo *bo)
 {
-	bool is_iomem;
+	if (iosys_map_is_null(&bo->map))
+		return NULL;
+	if (bo->map.is_iomem)
+		return (void __force *)bo->map.vaddr_iomem;
 
-	return ttm_kmap_obj_virtual(&bo->kmap, &is_iomem);
+	return bo->map.vaddr;
 }
 
 /**
@@ -836,8 +839,9 @@ void *amdgpu_bo_kptr(struct amdgpu_bo *bo)
  */
 void amdgpu_bo_kunmap(struct amdgpu_bo *bo)
 {
-	if (bo->kmap.bo)
-		ttm_bo_kunmap(&bo->kmap);
+	if (iosys_map_is_null(&bo->map))
+		return;
+	ttm_bo_vunmap(&bo->tbo, &bo->map);
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index bc42ccbde659a..553a92303339f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -28,6 +28,8 @@
 #ifndef __AMDGPU_OBJECT_H__
 #define __AMDGPU_OBJECT_H__
 
+#include <linux/iosys-map.h>
+
 #include <drm/amdgpu_drm.h>
 #include "amdgpu.h"
 #include "amdgpu_res_cursor.h"
@@ -99,7 +101,7 @@ struct amdgpu_bo {
 	struct ttm_place		placements[AMDGPU_BO_MAX_PLACEMENTS];
 	struct ttm_placement		placement;
 	struct ttm_buffer_object	tbo;
-	struct ttm_bo_kmap_obj		kmap;
+	struct iosys_map		map;
 	u64				flags;
 	/* per VM structure for page tables and with virtual addresses */
 	struct amdgpu_vm_bo_base	*vm_bo;
-- 
2.45.2

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

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

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

Re: [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@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)

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

[Christian König]

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