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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux