Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper

Mon, 29 Apr 2024 06:51:20 -0700 

Am 29.04.24 um 15:34 schrieb Tvrtko Ursulin:


On 29/04/2024 12:02, Christian König wrote:

Am 26.04.24 um 18:43 schrieb Tvrtko Ursulin:

From: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>

Help code readability by replacing a bunch of:

bo->tbo.base.resv == vm->root.bo->tbo.base.resv

With:

amdgpu_bo_is_vm_bo(bo, vm)

No functional changes.


Ah,yes that was on my TODO list as well.


But I would have rather added this to the VM instead. In other words 
move it to amdgpu_vm.h and call it amdgpu_vm_is_bo_always_valid() or 
something like that.



I am happy to move it around as long as you are sure amdgpu_vm.h is 
the location.



For instance main API there it seems to be amdgpu_vm_bo's. At least 
all the amdgpu_bo usages do not needing anything more that the struct 
forward declared.



So if I move the helper there I either need to make it include another 
header, or move the helper out of line to amdgpu_vm.c.


Thoughts?



amdgpu_vm.c is fine as well. I just though that something so simply 
could be an inline function in the header as well.


Regards,
Christian.



Regards,

Tvrtko



Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 14 ++++++++++

  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 31 
+++++++++-------------

  3 files changed, 28 insertions(+), 19 deletions(-)


diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c

index 67c234bcf89f..32e4a9c6e805 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c

@@ -174,7 +174,7 @@ static int amdgpu_gem_object_open(struct 
drm_gem_object *obj,

          return -EPERM;
      if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID &&
-        abo->tbo.base.resv != vm->root.bo->tbo.base.resv)
+        !amdgpu_bo_is_vm_bo(abo, vm))
          return -EPERM;
      r = amdgpu_bo_reserve(abo, false);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h

index be679c42b0b8..f2bb6965cc77 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h

@@ -307,6 +307,20 @@ static inline struct amdgpu_bo 
*amdgpu_bo_shadowed(struct amdgpu_bo *bo)

      return NULL;
  }
+/**
+ * amdgpu_bo_is_vm_bo - check if the BO is VM always valid
+ *
+ * @abo: BO to be tested.
+ * @vm: VM to test against.
+ *
+ * Returns true if the BO is VM always valid.
+ */
+static inline bool amdgpu_bo_is_vm_bo(struct amdgpu_bo *bo,
+                      struct amdgpu_vm *vm)
+{
+    return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
+}
+
  bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo);

  void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 
domain);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

index 8af3f0fd3073..6d6f0e325172 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

@@ -333,7 +333,7 @@ void amdgpu_vm_bo_base_init(struct 
amdgpu_vm_bo_base *base,

      base->next = bo->vm_bo;
      bo->vm_bo = base;
-    if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv)
+    if (!amdgpu_bo_is_vm_bo(bo, vm))
          return;
      dma_resv_assert_held(vm->root.bo->tbo.base.resv);

@@ -1101,13 +1101,12 @@ static void amdgpu_vm_bo_get_memory(struct 
amdgpu_bo_va *bo_va,

       * For now ignore BOs which are currently locked and potentially
       * changing their location.
       */
-    if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv &&
-        !dma_resv_trylock(bo->tbo.base.resv))

+    if (!amdgpu_bo_is_vm_bo(bo, vm) && 
!dma_resv_trylock(bo->tbo.base.resv))

          return;
      amdgpu_bo_get_memory(bo, stats);
-    if (bo->tbo.base.resv != vm->root.bo->tbo.base.resv)
-        dma_resv_unlock(bo->tbo.base.resv);
+    if (amdgpu_bo_is_vm_bo(bo, vm))
+        dma_resv_unlock(bo->tbo.base.resv);
  }
  void amdgpu_vm_get_memory(struct amdgpu_vm *vm,

@@ -1203,8 +1202,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
*adev, struct amdgpu_bo_va *bo_va,

          uncached = false;
      }
-    if (clear || (bo && bo->tbo.base.resv ==
-              vm->root.bo->tbo.base.resv))
+    if (clear || amdgpu_bo_is_vm_bo(bo, vm))
          last_update = &vm->last_update;
      else
          last_update = &bo_va->last_pt_update;

@@ -1246,7 +1244,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
*adev, struct amdgpu_bo_va *bo_va,

       * the evicted list so that it gets validated again on the
       * next command submission.
       */
-    if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv) {
+    if (amdgpu_bo_is_vm_bo(bo, vm)) {
          uint32_t mem_type = bo->tbo.resource->mem_type;
          if (!(bo->preferred_domains &

@@ -1640,10 +1638,9 @@ static void amdgpu_vm_bo_insert_map(struct 
amdgpu_device *adev,

      if (mapping->flags & AMDGPU_PTE_PRT)
          amdgpu_vm_prt_get(adev);
-    if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv &&
-        !bo_va->base.moved) {
+    if (amdgpu_bo_is_vm_bo(bo, vm) && !bo_va->base.moved)
          amdgpu_vm_bo_moved(&bo_va->base);
-    }
+
      trace_amdgpu_vm_bo_map(bo_va, mapping);
  }

@@ -1922,8 +1919,7 @@ int amdgpu_vm_bo_clear_mappings(struct 
amdgpu_device *adev,

          if (before->flags & AMDGPU_PTE_PRT)
              amdgpu_vm_prt_get(adev);
-        if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv &&
-            !before->bo_va->base.moved)
+        if (amdgpu_bo_is_vm_bo(bo, vm) && !before->bo_va->base.moved)
amdgpu_vm_bo_moved(&before->bo_va->base);
      } else {
          kfree(before);

@@ -1937,8 +1933,7 @@ int amdgpu_vm_bo_clear_mappings(struct 
amdgpu_device *adev,

          if (after->flags & AMDGPU_PTE_PRT)
              amdgpu_vm_prt_get(adev);
-        if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv &&
-            !after->bo_va->base.moved)
+        if (amdgpu_bo_is_vm_bo(bo, vm) && !after->bo_va->base.moved)
amdgpu_vm_bo_moved(&after->bo_va->base);
      } else {
          kfree(after);
@@ -2017,7 +2012,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
      if (bo) {
          dma_resv_assert_held(bo->tbo.base.resv);
-        if (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)
+        if (amdgpu_bo_is_vm_bo(bo, vm))
              ttm_bo_set_bulk_move(&bo->tbo, NULL);
          for (base = &bo_va->base.bo->vm_bo; *base;

@@ -2111,7 +2106,7 @@ void amdgpu_vm_bo_invalidate(struct 
amdgpu_device *adev,

      for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
          struct amdgpu_vm *vm = bo_base->vm;

-        if (evicted && bo->tbo.base.resv == 
vm->root.bo->tbo.base.resv) {

+        if (evicted && amdgpu_bo_is_vm_bo(bo, vm)) {
              amdgpu_vm_bo_evicted(bo_base);
              continue;
          }

@@ -2122,7 +2117,7 @@ void amdgpu_vm_bo_invalidate(struct 
amdgpu_device *adev,

          if (bo->tbo.type == ttm_bo_type_kernel)
              amdgpu_vm_bo_relocated(bo_base);
-        else if (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)
+        else if (amdgpu_bo_is_vm_bo(bo, vm))
              amdgpu_vm_bo_moved(bo_base);
          else
              amdgpu_vm_bo_invalidated(bo_base);


























					Reply via email to