Re: [PATCH 1/1] drm/amdgpu: move bo_va ref counting to internal funcs

2022-01-13 Thread Das, Nirmoy 



On 1/13/2022 1:12 PM, Christian König wrote:



Am 13.01.22 um 13:06 schrieb Nirmoy Das:

GEM code should not deal with struct amdgpu_bo_va's ref_count.
Move ref counting to amdgpu_vm.c.

Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 38 +++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
  3 files changed, 42 insertions(+), 9 deletions(-)

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

index 4a11a2f4fa73..691f0a879c90 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -176,12 +176,9 @@ static int amdgpu_gem_object_open(struct 
drm_gem_object *obj,

  if (r)
  return r;
  -    bo_va = amdgpu_vm_bo_find(vm, abo);
-    if (!bo_va) {
-    bo_va = amdgpu_vm_bo_add(adev, vm, abo);
-    } else {
-    ++bo_va->ref_count;
-    }
+    if (!amdgpu_vm_bo_get(vm, abo))
+    amdgpu_vm_bo_add(adev, vm, abo);
+
  amdgpu_bo_unreserve(abo);
  return 0;
  }
@@ -218,7 +215,7 @@ static void amdgpu_gem_object_close(struct 
drm_gem_object *obj,

  return;
  }
  bo_va = amdgpu_vm_bo_find(vm, bo);
-    if (!bo_va || --bo_va->ref_count)
+    if (!bo_va)
  goto out_unlock;
    amdgpu_vm_bo_rmv(adev, bo_va);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

index b23cb463b106..9d60de6a6697 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1290,16 +1290,49 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct 
amdgpu_vm *vm,

 struct amdgpu_bo *bo)
  {
  struct amdgpu_vm_bo_base *base;
+    struct amdgpu_bo_va *bo_va = NULL;
    for (base = bo->vm_bo; base; base = base->next) {
  if (base->vm != vm)
  continue;
  -    return container_of(base, struct amdgpu_bo_va, base);
+    bo_va = container_of(base, struct amdgpu_bo_va, base);
  }
-    return NULL;
+
+    if (bo_va && bo_va->ref_count <= 0)
+    return NULL;
+
+    return bo_va;
  }
  +/**
+ * amdgpu_vm_bo_get - find the bo_va for a specific vm & bo and 
increase

+ * the ref_count
+ *
+ * @vm: requested vm
+ * @bo: requested buffer object
+ *
+ * Find @bo inside the requested vm.
+ * Search inside the @bos vm list for the requested vm
+ * Returns the found bo_va with +1 ref_count or NULL if none is found
+ *
+ * Object has to be reserved!
+ *
+ * Returns:
+ * Found bo_va or NULL.
+ */
+struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
+  struct amdgpu_bo *bo)
+{
+    struct amdgpu_bo_va *bo_va = amdgpu_vm_bo_find(vm, bo);
+
+    if (bo_va)
+    ++bo_va->ref_count;
+
+    return bo_va;
+}
+
+
  /**
   * amdgpu_vm_map_gart - Resolve gart mapping of addr
   *
@@ -2704,6 +2737,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
  if (bo && bo_va->is_xgmi)
  amdgpu_xgmi_set_pstate(adev, AMDGPU_XGMI_PSTATE_MIN);
  +    --bo_va->ref_count;
  kfree(bo_va);


That here won't work, you are removing and freeing the bo_va even if 
the refcount is not zero yet.


I suggest to have a matching amdgpu_vm_bo_put() function instead.



Right, let me resend v2.


Thanks,

Nirmoy



Regards,
Christian.


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

index 85fcfb8c5efd..6d936fb1b934 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -415,6 +415,8 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device 
*adev,
  uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t 
addr);

  struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 struct amdgpu_bo *bo);
+struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
+   struct amdgpu_bo *bo);
  struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
    struct amdgpu_vm *vm,
    struct amdgpu_bo *bo);

Re: [PATCH 1/1] drm/amdgpu: move bo_va ref counting to internal funcs

2022-01-13 Thread Christian König 




Am 13.01.22 um 13:06 schrieb Nirmoy Das:

GEM code should not deal with struct amdgpu_bo_va's ref_count.
Move ref counting to amdgpu_vm.c.

Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 38 +++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
  3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 4a11a2f4fa73..691f0a879c90 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -176,12 +176,9 @@ static int amdgpu_gem_object_open(struct drm_gem_object 
*obj,
if (r)
return r;
  
-	bo_va = amdgpu_vm_bo_find(vm, abo);

-   if (!bo_va) {
-   bo_va = amdgpu_vm_bo_add(adev, vm, abo);
-   } else {
-   ++bo_va->ref_count;
-   }
+   if (!amdgpu_vm_bo_get(vm, abo))
+   amdgpu_vm_bo_add(adev, vm, abo);
+
amdgpu_bo_unreserve(abo);
return 0;
  }
@@ -218,7 +215,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object 
*obj,
return;
}
bo_va = amdgpu_vm_bo_find(vm, bo);
-   if (!bo_va || --bo_va->ref_count)
+   if (!bo_va)
goto out_unlock;
  
  	amdgpu_vm_bo_rmv(adev, bo_va);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b23cb463b106..9d60de6a6697 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1290,16 +1290,49 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm 
*vm,
   struct amdgpu_bo *bo)
  {
struct amdgpu_vm_bo_base *base;
+   struct amdgpu_bo_va *bo_va = NULL;
  
  	for (base = bo->vm_bo; base; base = base->next) {

if (base->vm != vm)
continue;
  
-		return container_of(base, struct amdgpu_bo_va, base);

+   bo_va = container_of(base, struct amdgpu_bo_va, base);
}
-   return NULL;
+
+   if (bo_va && bo_va->ref_count <= 0)
+   return NULL;
+
+   return bo_va;
  }
  
+/**

+ * amdgpu_vm_bo_get - find the bo_va for a specific vm & bo and increase
+ * the ref_count
+ *
+ * @vm: requested vm
+ * @bo: requested buffer object
+ *
+ * Find @bo inside the requested vm.
+ * Search inside the @bos vm list for the requested vm
+ * Returns the found bo_va with +1 ref_count or NULL if none is found
+ *
+ * Object has to be reserved!
+ *
+ * Returns:
+ * Found bo_va or NULL.
+ */
+struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
+ struct amdgpu_bo *bo)
+{
+   struct amdgpu_bo_va *bo_va = amdgpu_vm_bo_find(vm, bo);
+
+   if (bo_va)
+   ++bo_va->ref_count;
+
+   return bo_va;
+}
+
+
  /**
   * amdgpu_vm_map_gart - Resolve gart mapping of addr
   *
@@ -2704,6 +2737,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
if (bo && bo_va->is_xgmi)
amdgpu_xgmi_set_pstate(adev, AMDGPU_XGMI_PSTATE_MIN);
  
+	--bo_va->ref_count;

kfree(bo_va);


That here won't work, you are removing and freeing the bo_va even if the 
refcount is not zero yet.


I suggest to have a matching amdgpu_vm_bo_put() function instead.

Regards,
Christian.


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

index 85fcfb8c5efd..6d936fb1b934 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -415,6 +415,8 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
  uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
  struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
   struct amdgpu_bo *bo);
+struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
+  struct amdgpu_bo *bo);
  struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
  struct amdgpu_vm *vm,
  struct amdgpu_bo *bo);

[PATCH 1/1] drm/amdgpu: move bo_va ref counting to internal funcs

2022-01-13 Thread Nirmoy Das 
GEM code should not deal with struct amdgpu_bo_va's ref_count.
Move ref counting to amdgpu_vm.c.

Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 38 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 4a11a2f4fa73..691f0a879c90 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -176,12 +176,9 @@ static int amdgpu_gem_object_open(struct drm_gem_object 
*obj,
if (r)
return r;
 
-   bo_va = amdgpu_vm_bo_find(vm, abo);
-   if (!bo_va) {
-   bo_va = amdgpu_vm_bo_add(adev, vm, abo);
-   } else {
-   ++bo_va->ref_count;
-   }
+   if (!amdgpu_vm_bo_get(vm, abo))
+   amdgpu_vm_bo_add(adev, vm, abo);
+
amdgpu_bo_unreserve(abo);
return 0;
 }
@@ -218,7 +215,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object 
*obj,
return;
}
bo_va = amdgpu_vm_bo_find(vm, bo);
-   if (!bo_va || --bo_va->ref_count)
+   if (!bo_va)
goto out_unlock;
 
amdgpu_vm_bo_rmv(adev, bo_va);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b23cb463b106..9d60de6a6697 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1290,16 +1290,49 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm 
*vm,
   struct amdgpu_bo *bo)
 {
struct amdgpu_vm_bo_base *base;
+   struct amdgpu_bo_va *bo_va = NULL;
 
for (base = bo->vm_bo; base; base = base->next) {
if (base->vm != vm)
continue;
 
-   return container_of(base, struct amdgpu_bo_va, base);
+   bo_va = container_of(base, struct amdgpu_bo_va, base);
}
-   return NULL;
+
+   if (bo_va && bo_va->ref_count <= 0)
+   return NULL;
+
+   return bo_va;
 }
 
+/**
+ * amdgpu_vm_bo_get - find the bo_va for a specific vm & bo and increase
+ * the ref_count
+ *
+ * @vm: requested vm
+ * @bo: requested buffer object
+ *
+ * Find @bo inside the requested vm.
+ * Search inside the @bos vm list for the requested vm
+ * Returns the found bo_va with +1 ref_count or NULL if none is found
+ *
+ * Object has to be reserved!
+ *
+ * Returns:
+ * Found bo_va or NULL.
+ */
+struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
+ struct amdgpu_bo *bo)
+{
+   struct amdgpu_bo_va *bo_va = amdgpu_vm_bo_find(vm, bo);
+
+   if (bo_va)
+   ++bo_va->ref_count;
+
+   return bo_va;
+}
+
+
 /**
  * amdgpu_vm_map_gart - Resolve gart mapping of addr
  *
@@ -2704,6 +2737,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
if (bo && bo_va->is_xgmi)
amdgpu_xgmi_set_pstate(adev, AMDGPU_XGMI_PSTATE_MIN);
 
+   --bo_va->ref_count;
kfree(bo_va);
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 85fcfb8c5efd..6d936fb1b934 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -415,6 +415,8 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
 struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
   struct amdgpu_bo *bo);
+struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
+  struct amdgpu_bo *bo);
 struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
  struct amdgpu_vm *vm,
  struct amdgpu_bo *bo);
-- 
2.33.1