Re: [PATCH 1/2] drm/amdgpu: Unmap BO memory before calling amdgpu_bo_unref()
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
---
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()
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
---
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()
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
---
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);
}
[PATCH 1/2] drm/amdgpu: Unmap BO memory before calling amdgpu_bo_unref()
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
---
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
