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

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

[Tvrtko Ursulin]

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

Help code readability by replacing a bunch of:

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

With:

amdgpu_vm_is_bo_always_valid(vm, bo)

No functional changes.

v2:
 * Rename helper and move to amdgpu_vm. (Christian)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 +++++++++++++++----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 67c234bcf89f..e698d65e9508 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_vm_is_bo_always_valid(vm, abo))
 		return -EPERM;
 
 	r = amdgpu_bo_reserve(abo, false);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8af3f0fd3073..01ca4b35b369 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_vm_is_bo_always_valid(vm, bo))
 		return;
 
 	dma_resv_assert_held(vm->root.bo->tbo.base.resv);
@@ -1101,13 +1101,13 @@ 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 &&
+	if (!amdgpu_vm_is_bo_always_valid(vm, bo) &&
 	    !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_vm_is_bo_always_valid(vm, bo))
+		dma_resv_unlock(bo->tbo.base.resv);
 }
 
 void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
@@ -1203,8 +1203,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_vm_is_bo_always_valid(vm, bo))
 		last_update = &vm->last_update;
 	else
 		last_update = &bo_va->last_pt_update;
@@ -1246,7 +1245,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_vm_is_bo_always_valid(vm, bo)) {
 		uint32_t mem_type = bo->tbo.resource->mem_type;
 
 		if (!(bo->preferred_domains &
@@ -1640,10 +1639,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_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved)
 		amdgpu_vm_bo_moved(&bo_va->base);
-	}
+
 	trace_amdgpu_vm_bo_map(bo_va, mapping);
 }
 
@@ -1922,7 +1920,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 &&
+		if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
 		    !before->bo_va->base.moved)
 			amdgpu_vm_bo_moved(&before->bo_va->base);
 	} else {
@@ -1937,7 +1935,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 &&
+		if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
 		    !after->bo_va->base.moved)
 			amdgpu_vm_bo_moved(&after->bo_va->base);
 	} else {
@@ -2017,7 +2015,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_vm_is_bo_always_valid(vm, bo))
 			ttm_bo_set_bulk_move(&bo->tbo, NULL);
 
 		for (base = &bo_va->base.bo->vm_bo; *base;
@@ -2111,7 +2109,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_vm_is_bo_always_valid(vm, bo)) {
 			amdgpu_vm_bo_evicted(bo_base);
 			continue;
 		}
@@ -2122,7 +2120,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_vm_is_bo_always_valid(vm, bo))
 			amdgpu_vm_bo_moved(bo_base);
 		else
 			amdgpu_vm_bo_invalidated(bo_base);
@@ -2986,3 +2984,15 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
 	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
 }
 
+/**
+ * amdgpu_vm_is_bo_always_valid - check if the BO is VM always valid
+ *
+ * @vm: VM to test against.
+ * @abo: BO to be tested.
+ *
+ * Returns true if the BO is VM always valid.
+ */
+bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct amdgpu_bo *bo)
+{
+	return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 54d7da396de0..ec688a47dec1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -561,6 +561,8 @@ void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm *vm, struct seq_file *m);
 
 int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 
+bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct amdgpu_bo *bo);
+
 /**
  * amdgpu_vm_tlb_seq - return tlb flush sequence number
  * @vm: the amdgpu_vm structure to query
-- 
2.44.0

[PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Tvrtko Ursulin]

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

All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.

Simplify a few places in the code which convert the TTM placement into
a domain by checking against the current placement directly.

In the conversion AMDGPU_PL_PREEMPT either does not have to be handled
because amdgpu_mem_type_to_domain() cannot return that value anyway.

v2:
 * Remove AMDGPU_PL_PREEMPT handling.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Christian König <christian.koenig@amd.com> # v1
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  3 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 27 +++++++++------------
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 055ba2ea4c12..0b3b10d21952 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -165,8 +165,7 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach,
 		if (r)
 			return ERR_PTR(r);
 
-	} else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
-		     AMDGPU_GEM_DOMAIN_GTT)) {
+	} else if (bo->tbo.resource->mem_type != TTM_PL_TT) {
 		return ERR_PTR(-EBUSY);
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 8bc79924d171..eb5bd6962560 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -976,12 +976,11 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 
 	ttm_bo_pin(&bo->tbo);
 
-	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
-	if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
+	if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
 		atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
 		atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
 			     &adev->visible_pin_size);
-	} else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
+	} else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
 		atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
 	}
 
@@ -1280,7 +1279,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 {
 	uint64_t size = amdgpu_bo_size(bo);
 	struct drm_gem_object *obj;
-	unsigned int domain;
 	bool shared;
 
 	/* Abort if the BO doesn't currently have a backing store */
@@ -1290,21 +1288,20 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 	obj = &bo->tbo.base;
 	shared = drm_gem_object_is_shared_for_memory_stats(obj);
 
-	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
-	switch (domain) {
-	case AMDGPU_GEM_DOMAIN_VRAM:
+	switch (bo->tbo.resource->mem_type) {
+	case TTM_PL_VRAM:
 		stats->vram += size;
 		if (amdgpu_bo_in_cpu_visible_vram(bo))
 			stats->visible_vram += size;
 		if (shared)
 			stats->vram_shared += size;
 		break;
-	case AMDGPU_GEM_DOMAIN_GTT:
+	case TTM_PL_TT:
 		stats->gtt += size;
 		if (shared)
 			stats->gtt_shared += size;
 		break;
-	case AMDGPU_GEM_DOMAIN_CPU:
+	case TTM_PL_SYSTEM:
 	default:
 		stats->cpu += size;
 		if (shared)
@@ -1317,7 +1314,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 		if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
 			stats->requested_visible_vram += size;
 
-		if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
+		if (bo->tbo.resource->mem_type != TTM_PL_VRAM) {
 			stats->evicted_vram += size;
 			if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
 				stats->evicted_visible_vram += size;
@@ -1592,19 +1589,17 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
 	u64 size;
 
 	if (dma_resv_trylock(bo->tbo.base.resv)) {
-		unsigned int domain;
-		domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
-		switch (domain) {
-		case AMDGPU_GEM_DOMAIN_VRAM:
+		switch (bo->tbo.resource->mem_type) {
+		case TTM_PL_VRAM:
 			if (amdgpu_bo_in_cpu_visible_vram(bo))
 				placement = "VRAM VISIBLE";
 			else
 				placement = "VRAM";
 			break;
-		case AMDGPU_GEM_DOMAIN_GTT:
+		case TTM_PL_TT:
 			placement = "GTT";
 			break;
-		case AMDGPU_GEM_DOMAIN_CPU:
+		case TTM_PL_SYSTEM:
 		default:
 			placement = "CPU";
 			break;
-- 
2.44.0

[PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs

[Tvrtko Ursulin]

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

Instead of mixing them together with regular system memory objects mark
them explicitly as 'PREEMPTIBLE'.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Felix Kuehling <felix.kuehling@amd.com>
---
No idea on the name to use.. :)
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index eb5bd6962560..be6c2f5b9fcb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1599,6 +1599,9 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
 		case TTM_PL_TT:
 			placement = "GTT";
 			break;
+		case AMDGPU_PL_PREEMPT:
+			placement = "PREEMPTIBLE";
+			break;
 		case TTM_PL_SYSTEM:
 		default:
 			placement = "CPU";
-- 
2.44.0

Re: [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Felix Kuehling]

On 2024-04-29 12:47, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
> placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
> depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.
>
> Simplify a few places in the code which convert the TTM placement into
> a domain by checking against the current placement directly.
>
> In the conversion AMDGPU_PL_PREEMPT either does not have to be handled
> because amdgpu_mem_type_to_domain() cannot return that value anyway.
>
> v2:
>   * Remove AMDGPU_PL_PREEMPT handling.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Reviewed-by: Christian König <christian.koenig@amd.com> # v1
Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>

I also ran kfdtest on a multi-GPU system just to make sure this didn't 
break our multi-GPU support. BTW, I had to fix up some things when I 
tried to apply your patch to the current amd-staging-drm-next branch. 
That branch was just rebased on Linux 6.8, so maybe that's part of the 
reason.


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  3 +--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 27 +++++++++------------
>   2 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 055ba2ea4c12..0b3b10d21952 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -165,8 +165,7 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach,
>   		if (r)
>   			return ERR_PTR(r);
>   
> -	} else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
> -		     AMDGPU_GEM_DOMAIN_GTT)) {
> +	} else if (bo->tbo.resource->mem_type != TTM_PL_TT) {
>   		return ERR_PTR(-EBUSY);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 8bc79924d171..eb5bd6962560 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -976,12 +976,11 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>   
>   	ttm_bo_pin(&bo->tbo);
>   
> -	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -	if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
> +	if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
>   		atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
>   		atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
>   			     &adev->visible_pin_size);
> -	} else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> +	} else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
>   		atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
>   	}
>   
> @@ -1280,7 +1279,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   {
>   	uint64_t size = amdgpu_bo_size(bo);
>   	struct drm_gem_object *obj;
> -	unsigned int domain;
>   	bool shared;
>   
>   	/* Abort if the BO doesn't currently have a backing store */
> @@ -1290,21 +1288,20 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   	obj = &bo->tbo.base;
>   	shared = drm_gem_object_is_shared_for_memory_stats(obj);
>   
> -	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -	switch (domain) {
> -	case AMDGPU_GEM_DOMAIN_VRAM:
> +	switch (bo->tbo.resource->mem_type) {
> +	case TTM_PL_VRAM:
>   		stats->vram += size;
>   		if (amdgpu_bo_in_cpu_visible_vram(bo))
>   			stats->visible_vram += size;
>   		if (shared)
>   			stats->vram_shared += size;
>   		break;
> -	case AMDGPU_GEM_DOMAIN_GTT:
> +	case TTM_PL_TT:
>   		stats->gtt += size;
>   		if (shared)
>   			stats->gtt_shared += size;
>   		break;
> -	case AMDGPU_GEM_DOMAIN_CPU:
> +	case TTM_PL_SYSTEM:
>   	default:
>   		stats->cpu += size;
>   		if (shared)
> @@ -1317,7 +1314,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   		if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>   			stats->requested_visible_vram += size;
>   
> -		if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
> +		if (bo->tbo.resource->mem_type != TTM_PL_VRAM) {
>   			stats->evicted_vram += size;
>   			if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>   				stats->evicted_visible_vram += size;
> @@ -1592,19 +1589,17 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
>   	u64 size;
>   
>   	if (dma_resv_trylock(bo->tbo.base.resv)) {
> -		unsigned int domain;
> -		domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -		switch (domain) {
> -		case AMDGPU_GEM_DOMAIN_VRAM:
> +		switch (bo->tbo.resource->mem_type) {
> +		case TTM_PL_VRAM:
>   			if (amdgpu_bo_in_cpu_visible_vram(bo))
>   				placement = "VRAM VISIBLE";
>   			else
>   				placement = "VRAM";
>   			break;
> -		case AMDGPU_GEM_DOMAIN_GTT:
> +		case TTM_PL_TT:
>   			placement = "GTT";
>   			break;
> -		case AMDGPU_GEM_DOMAIN_CPU:
> +		case TTM_PL_SYSTEM:
>   		default:
>   			placement = "CPU";
>   			break;

Re: [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Tvrtko Ursulin]

On 30/04/2024 01:31, Felix Kuehling wrote:
> 
> On 2024-04-29 12:47, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
>> placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
>> depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.
>>
>> Simplify a few places in the code which convert the TTM placement into
>> a domain by checking against the current placement directly.
>>
>> In the conversion AMDGPU_PL_PREEMPT either does not have to be handled
>> because amdgpu_mem_type_to_domain() cannot return that value anyway.
>>
>> v2:
>>   * Remove AMDGPU_PL_PREEMPT handling.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com> # v1
> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>
> 
> I also ran kfdtest on a multi-GPU system just to make sure this didn't 
> break our multi-GPU support. BTW, I had to fix up some things when I 

Excellent thank you!

Btw important thing to stress is that I hope the r-b means not only 
patch is functionaly correct but that you guys actually agree it is an 
improvement. Because I am quite new in your code base so please apply 
strict criteria on my proposals.

> tried to apply your patch to the current amd-staging-drm-next branch. 
> That branch was just rebased on Linux 6.8, so maybe that's part of the 
> reason.

I am conditioned to work against drm-tip so maybe that is one reason, or 
also possibly because now I see I used drm-tip from more than a week ago 
as a base. :( I can rebase and re-send. So amd-staging-drm-next is the 
correct branch?

Regards,

Tvrtko

> 
> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  3 +--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 27 +++++++++------------
>>   2 files changed, 12 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> index 055ba2ea4c12..0b3b10d21952 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> @@ -165,8 +165,7 @@ static struct sg_table *amdgpu_dma_buf_map(struct 
>> dma_buf_attachment *attach,
>>           if (r)
>>               return ERR_PTR(r);
>> -    } else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
>> -             AMDGPU_GEM_DOMAIN_GTT)) {
>> +    } else if (bo->tbo.resource->mem_type != TTM_PL_TT) {
>>           return ERR_PTR(-EBUSY);
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 8bc79924d171..eb5bd6962560 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -976,12 +976,11 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo 
>> *bo, u32 domain,
>>       ttm_bo_pin(&bo->tbo);
>> -    domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> -    if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>> +    if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
>>           atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
>>           atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
>>                    &adev->visible_pin_size);
>> -    } else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
>> +    } else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
>>           atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
>>       }
>> @@ -1280,7 +1279,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>   {
>>       uint64_t size = amdgpu_bo_size(bo);
>>       struct drm_gem_object *obj;
>> -    unsigned int domain;
>>       bool shared;
>>       /* Abort if the BO doesn't currently have a backing store */
>> @@ -1290,21 +1288,20 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>       obj = &bo->tbo.base;
>>       shared = drm_gem_object_is_shared_for_memory_stats(obj);
>> -    domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> -    switch (domain) {
>> -    case AMDGPU_GEM_DOMAIN_VRAM:
>> +    switch (bo->tbo.resource->mem_type) {
>> +    case TTM_PL_VRAM:
>>           stats->vram += size;
>>           if (amdgpu_bo_in_cpu_visible_vram(bo))
>>               stats->visible_vram += size;
>>           if (shared)
>>               stats->vram_shared += size;
>>           break;
>> -    case AMDGPU_GEM_DOMAIN_GTT:
>> +    case TTM_PL_TT:
>>           stats->gtt += size;
>>           if (shared)
>>               stats->gtt_shared += size;
>>           break;
>> -    case AMDGPU_GEM_DOMAIN_CPU:
>> +    case TTM_PL_SYSTEM:
>>       default:
>>           stats->cpu += size;
>>           if (shared)
>> @@ -1317,7 +1314,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>           if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>>               stats->requested_visible_vram += size;
>> -        if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
>> +        if (bo->tbo.resource->mem_type != TTM_PL_VRAM) {
>>               stats->evicted_vram += size;
>>               if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>>                   stats->evicted_visible_vram += size;
>> @@ -1592,19 +1589,17 @@ u64 amdgpu_bo_print_info(int id, struct 
>> amdgpu_bo *bo, struct seq_file *m)
>>       u64 size;
>>       if (dma_resv_trylock(bo->tbo.base.resv)) {
>> -        unsigned int domain;
>> -        domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>> -        switch (domain) {
>> -        case AMDGPU_GEM_DOMAIN_VRAM:
>> +        switch (bo->tbo.resource->mem_type) {
>> +        case TTM_PL_VRAM:
>>               if (amdgpu_bo_in_cpu_visible_vram(bo))
>>                   placement = "VRAM VISIBLE";
>>               else
>>                   placement = "VRAM";
>>               break;
>> -        case AMDGPU_GEM_DOMAIN_GTT:
>> +        case TTM_PL_TT:
>>               placement = "GTT";
>>               break;
>> -        case AMDGPU_GEM_DOMAIN_CPU:
>> +        case TTM_PL_SYSTEM:
>>           default:
>>               placement = "CPU";
>>               break;

[PATCH v3 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Tvrtko Ursulin]

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

All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.

Simplify a few places in the code which convert the TTM placement into
a domain by checking against the current placement directly.

In the conversion AMDGPU_PL_PREEMPT either does not have to be handled
because amdgpu_mem_type_to_domain() cannot return that value anyway.

v2:
 * Remove AMDGPU_PL_PREEMPT handling.

v3:
 * Rebase.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Christian König <christian.koenig@amd.com> # v1
Reviewed-by: Felix Kuehling <felix.kuehling@amd.com> # v2
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  3 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 29 +++++++++------------
 2 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 055ba2ea4c12..0b3b10d21952 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -165,8 +165,7 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach,
 		if (r)
 			return ERR_PTR(r);
 
-	} else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
-		     AMDGPU_GEM_DOMAIN_GTT)) {
+	} else if (bo->tbo.resource->mem_type != TTM_PL_TT) {
 		return ERR_PTR(-EBUSY);
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index b2a83c802bbd..c581e4952cbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -983,12 +983,11 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 
 	ttm_bo_pin(&bo->tbo);
 
-	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
-	if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
+	if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
 		atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
 		atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
 			     &adev->visible_pin_size);
-	} else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
+	} else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
 		atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
 	}
 
@@ -1289,7 +1288,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 	struct ttm_resource *res = bo->tbo.resource;
 	uint64_t size = amdgpu_bo_size(bo);
 	struct drm_gem_object *obj;
-	unsigned int domain;
 	bool shared;
 
 	/* Abort if the BO doesn't currently have a backing store */
@@ -1299,21 +1297,20 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 	obj = &bo->tbo.base;
 	shared = drm_gem_object_is_shared_for_memory_stats(obj);
 
-	domain = amdgpu_mem_type_to_domain(res->mem_type);
-	switch (domain) {
-	case AMDGPU_GEM_DOMAIN_VRAM:
+	switch (res->mem_type) {
+	case TTM_PL_VRAM:
 		stats->vram += size;
-		if (amdgpu_res_cpu_visible(adev, bo->tbo.resource))
+		if (amdgpu_res_cpu_visible(adev, res))
 			stats->visible_vram += size;
 		if (shared)
 			stats->vram_shared += size;
 		break;
-	case AMDGPU_GEM_DOMAIN_GTT:
+	case TTM_PL_TT:
 		stats->gtt += size;
 		if (shared)
 			stats->gtt_shared += size;
 		break;
-	case AMDGPU_GEM_DOMAIN_CPU:
+	case TTM_PL_SYSTEM:
 	default:
 		stats->cpu += size;
 		if (shared)
@@ -1326,7 +1323,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 		if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
 			stats->requested_visible_vram += size;
 
-		if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
+		if (res->mem_type != TTM_PL_VRAM) {
 			stats->evicted_vram += size;
 			if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
 				stats->evicted_visible_vram += size;
@@ -1600,20 +1597,18 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
 	u64 size;
 
 	if (dma_resv_trylock(bo->tbo.base.resv)) {
-		unsigned int domain;
 
-		domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
-		switch (domain) {
-		case AMDGPU_GEM_DOMAIN_VRAM:
+		switch (bo->tbo.resource->mem_type) {
+		case TTM_PL_VRAM:
 			if (amdgpu_res_cpu_visible(adev, bo->tbo.resource))
 				placement = "VRAM VISIBLE";
 			else
 				placement = "VRAM";
 			break;
-		case AMDGPU_GEM_DOMAIN_GTT:
+		case TTM_PL_TT:
 			placement = "GTT";
 			break;
-		case AMDGPU_GEM_DOMAIN_CPU:
+		case TTM_PL_SYSTEM:
 		default:
 			placement = "CPU";
 			break;
-- 
2.44.0

Re: [PATCH v3 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Felix Kuehling]

On 2024-04-30 13:16, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
> placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
> depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.
>
> Simplify a few places in the code which convert the TTM placement into
> a domain by checking against the current placement directly.
>
> In the conversion AMDGPU_PL_PREEMPT either does not have to be handled
> because amdgpu_mem_type_to_domain() cannot return that value anyway.
>
> v2:
>   * Remove AMDGPU_PL_PREEMPT handling.
>
> v3:
>   * Rebase.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Reviewed-by: Christian König <christian.koenig@amd.com> # v1
> Reviewed-by: Felix Kuehling <felix.kuehling@amd.com> # v2

I'm waiting for Christian to review patches 1 and 3. Then I can apply 
the whole series.

Regards,
   Felix


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  3 +--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 29 +++++++++------------
>   2 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 055ba2ea4c12..0b3b10d21952 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -165,8 +165,7 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach,
>   		if (r)
>   			return ERR_PTR(r);
>   
> -	} else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
> -		     AMDGPU_GEM_DOMAIN_GTT)) {
> +	} else if (bo->tbo.resource->mem_type != TTM_PL_TT) {
>   		return ERR_PTR(-EBUSY);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index b2a83c802bbd..c581e4952cbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -983,12 +983,11 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>   
>   	ttm_bo_pin(&bo->tbo);
>   
> -	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -	if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
> +	if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
>   		atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
>   		atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
>   			     &adev->visible_pin_size);
> -	} else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> +	} else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
>   		atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
>   	}
>   
> @@ -1289,7 +1288,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   	struct ttm_resource *res = bo->tbo.resource;
>   	uint64_t size = amdgpu_bo_size(bo);
>   	struct drm_gem_object *obj;
> -	unsigned int domain;
>   	bool shared;
>   
>   	/* Abort if the BO doesn't currently have a backing store */
> @@ -1299,21 +1297,20 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   	obj = &bo->tbo.base;
>   	shared = drm_gem_object_is_shared_for_memory_stats(obj);
>   
> -	domain = amdgpu_mem_type_to_domain(res->mem_type);
> -	switch (domain) {
> -	case AMDGPU_GEM_DOMAIN_VRAM:
> +	switch (res->mem_type) {
> +	case TTM_PL_VRAM:
>   		stats->vram += size;
> -		if (amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> +		if (amdgpu_res_cpu_visible(adev, res))
>   			stats->visible_vram += size;
>   		if (shared)
>   			stats->vram_shared += size;
>   		break;
> -	case AMDGPU_GEM_DOMAIN_GTT:
> +	case TTM_PL_TT:
>   		stats->gtt += size;
>   		if (shared)
>   			stats->gtt_shared += size;
>   		break;
> -	case AMDGPU_GEM_DOMAIN_CPU:
> +	case TTM_PL_SYSTEM:
>   	default:
>   		stats->cpu += size;
>   		if (shared)
> @@ -1326,7 +1323,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   		if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>   			stats->requested_visible_vram += size;
>   
> -		if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
> +		if (res->mem_type != TTM_PL_VRAM) {
>   			stats->evicted_vram += size;
>   			if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>   				stats->evicted_visible_vram += size;
> @@ -1600,20 +1597,18 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
>   	u64 size;
>   
>   	if (dma_resv_trylock(bo->tbo.base.resv)) {
> -		unsigned int domain;
>   
> -		domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -		switch (domain) {
> -		case AMDGPU_GEM_DOMAIN_VRAM:
> +		switch (bo->tbo.resource->mem_type) {
> +		case TTM_PL_VRAM:
>   			if (amdgpu_res_cpu_visible(adev, bo->tbo.resource))
>   				placement = "VRAM VISIBLE";
>   			else
>   				placement = "VRAM";
>   			break;
> -		case AMDGPU_GEM_DOMAIN_GTT:
> +		case TTM_PL_TT:
>   			placement = "GTT";
>   			break;
> -		case AMDGPU_GEM_DOMAIN_CPU:
> +		case TTM_PL_SYSTEM:
>   		default:
>   			placement = "CPU";
>   			break;

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

[Christian König]

Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Help code readability by replacing a bunch of:
>
> bo->tbo.base.resv == vm->root.bo->tbo.base.resv
>
> With:
>
> amdgpu_vm_is_bo_always_valid(vm, bo)
>
> No functional changes.
>
> v2:
>   * Rename helper and move to amdgpu_vm. (Christian)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 +++++++++++++++----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
>   3 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 67c234bcf89f..e698d65e9508 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_vm_is_bo_always_valid(vm, abo))
>   		return -EPERM;
>   
>   	r = amdgpu_bo_reserve(abo, false);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8af3f0fd3073..01ca4b35b369 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_vm_is_bo_always_valid(vm, bo))
>   		return;
>   
>   	dma_resv_assert_held(vm->root.bo->tbo.base.resv);
> @@ -1101,13 +1101,13 @@ 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 &&
> +	if (!amdgpu_vm_is_bo_always_valid(vm, bo) &&
>   	    !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_vm_is_bo_always_valid(vm, bo))
> +		dma_resv_unlock(bo->tbo.base.resv);
>   }
>   
>   void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
> @@ -1203,8 +1203,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_vm_is_bo_always_valid(vm, bo))
>   		last_update = &vm->last_update;
>   	else
>   		last_update = &bo_va->last_pt_update;
> @@ -1246,7 +1245,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_vm_is_bo_always_valid(vm, bo)) {
>   		uint32_t mem_type = bo->tbo.resource->mem_type;
>   
>   		if (!(bo->preferred_domains &
> @@ -1640,10 +1639,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_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved)
>   		amdgpu_vm_bo_moved(&bo_va->base);
> -	}
> +
>   	trace_amdgpu_vm_bo_map(bo_va, mapping);
>   }
>   
> @@ -1922,7 +1920,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 &&
> +		if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>   		    !before->bo_va->base.moved)
>   			amdgpu_vm_bo_moved(&before->bo_va->base);
>   	} else {
> @@ -1937,7 +1935,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 &&
> +		if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>   		    !after->bo_va->base.moved)
>   			amdgpu_vm_bo_moved(&after->bo_va->base);
>   	} else {
> @@ -2017,7 +2015,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_vm_is_bo_always_valid(vm, bo))
>   			ttm_bo_set_bulk_move(&bo->tbo, NULL);
>   
>   		for (base = &bo_va->base.bo->vm_bo; *base;
> @@ -2111,7 +2109,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_vm_is_bo_always_valid(vm, bo)) {
>   			amdgpu_vm_bo_evicted(bo_base);
>   			continue;
>   		}
> @@ -2122,7 +2120,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_vm_is_bo_always_valid(vm, bo))
>   			amdgpu_vm_bo_moved(bo_base);
>   		else
>   			amdgpu_vm_bo_invalidated(bo_base);
> @@ -2986,3 +2984,15 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>   	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>   }
>   
> +/**
> + * amdgpu_vm_is_bo_always_valid - check if the BO is VM always valid
> + *
> + * @vm: VM to test against.
> + * @abo: BO to be tested.
> + *
> + * Returns true if the BO is VM always valid.

Maybe improve that a bit, e.g. something like this:

"Returns true if the BO shares the dma_resv object with the root PD and 
is always guaranteed to be valid inside the VM."

With that done the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>

Thanks,
Christian.

> + */
> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct amdgpu_bo *bo)
> +{
> +	return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 54d7da396de0..ec688a47dec1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -561,6 +561,8 @@ void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm *vm, struct seq_file *m);
>   
>   int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   
> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct amdgpu_bo *bo);
> +
>   /**
>    * amdgpu_vm_tlb_seq - return tlb flush sequence number
>    * @vm: the amdgpu_vm structure to query

Re: [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection

[Christian König]

Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> All apart from AMDGPU_GEM_DOMAIN_GTT memory domains map 1:1 to TTM
> placements. And the former be either AMDGPU_PL_PREEMPT or TTM_PL_TT,
> depending on AMDGPU_GEM_CREATE_PREEMPTIBLE.
>
> Simplify a few places in the code which convert the TTM placement into
> a domain by checking against the current placement directly.
>
> In the conversion AMDGPU_PL_PREEMPT either does not have to be handled
> because amdgpu_mem_type_to_domain() cannot return that value anyway.
>
> v2:
>   * Remove AMDGPU_PL_PREEMPT handling.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Reviewed-by: Christian König <christian.koenig@amd.com> # v1
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  3 +--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 27 +++++++++------------
>   2 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 055ba2ea4c12..0b3b10d21952 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -165,8 +165,7 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach,
>   		if (r)
>   			return ERR_PTR(r);
>   
> -	} else if (!(amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type) &
> -		     AMDGPU_GEM_DOMAIN_GTT)) {
> +	} else if (bo->tbo.resource->mem_type != TTM_PL_TT) {
>   		return ERR_PTR(-EBUSY);
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 8bc79924d171..eb5bd6962560 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -976,12 +976,11 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>   
>   	ttm_bo_pin(&bo->tbo);
>   
> -	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -	if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
> +	if (bo->tbo.resource->mem_type == TTM_PL_VRAM) {
>   		atomic64_add(amdgpu_bo_size(bo), &adev->vram_pin_size);
>   		atomic64_add(amdgpu_vram_mgr_bo_visible_size(bo),
>   			     &adev->visible_pin_size);
> -	} else if (domain == AMDGPU_GEM_DOMAIN_GTT) {
> +	} else if (bo->tbo.resource->mem_type == TTM_PL_TT) {
>   		atomic64_add(amdgpu_bo_size(bo), &adev->gart_pin_size);
>   	}
>   
> @@ -1280,7 +1279,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   {
>   	uint64_t size = amdgpu_bo_size(bo);
>   	struct drm_gem_object *obj;
> -	unsigned int domain;
>   	bool shared;
>   
>   	/* Abort if the BO doesn't currently have a backing store */
> @@ -1290,21 +1288,20 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   	obj = &bo->tbo.base;
>   	shared = drm_gem_object_is_shared_for_memory_stats(obj);
>   
> -	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -	switch (domain) {
> -	case AMDGPU_GEM_DOMAIN_VRAM:
> +	switch (bo->tbo.resource->mem_type) {
> +	case TTM_PL_VRAM:
>   		stats->vram += size;
>   		if (amdgpu_bo_in_cpu_visible_vram(bo))
>   			stats->visible_vram += size;
>   		if (shared)
>   			stats->vram_shared += size;
>   		break;
> -	case AMDGPU_GEM_DOMAIN_GTT:
> +	case TTM_PL_TT:
>   		stats->gtt += size;
>   		if (shared)
>   			stats->gtt_shared += size;
>   		break;
> -	case AMDGPU_GEM_DOMAIN_CPU:
> +	case TTM_PL_SYSTEM:
>   	default:
>   		stats->cpu += size;
>   		if (shared)
> @@ -1317,7 +1314,7 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   		if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>   			stats->requested_visible_vram += size;
>   
> -		if (domain != AMDGPU_GEM_DOMAIN_VRAM) {
> +		if (bo->tbo.resource->mem_type != TTM_PL_VRAM) {
>   			stats->evicted_vram += size;
>   			if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
>   				stats->evicted_visible_vram += size;
> @@ -1592,19 +1589,17 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
>   	u64 size;
>   
>   	if (dma_resv_trylock(bo->tbo.base.resv)) {
> -		unsigned int domain;
> -		domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> -		switch (domain) {
> -		case AMDGPU_GEM_DOMAIN_VRAM:
> +		switch (bo->tbo.resource->mem_type) {
> +		case TTM_PL_VRAM:
>   			if (amdgpu_bo_in_cpu_visible_vram(bo))
>   				placement = "VRAM VISIBLE";
>   			else
>   				placement = "VRAM";
>   			break;
> -		case AMDGPU_GEM_DOMAIN_GTT:
> +		case TTM_PL_TT:
>   			placement = "GTT";
>   			break;
> -		case AMDGPU_GEM_DOMAIN_CPU:
> +		case TTM_PL_SYSTEM:

I would still prefer a AMDGPU_PL_PREEMPT here to be able to distinct those.

On the other hand OA, GWS and GDS placements are missing as well. So 
that switch should probably be fixed.

Either way the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com> for now since it doesn't change the handling 
at all.

Regards,
Christian.

>   		default:
>   			placement = "CPU";
>   			break;

Re: [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs

[Christian König]

Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Instead of mixing them together with regular system memory objects mark
> them explicitly as 'PREEMPTIBLE'.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Felix Kuehling <felix.kuehling@amd.com>
> ---
> No idea on the name to use.. :)
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index eb5bd6962560..be6c2f5b9fcb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1599,6 +1599,9 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
>   		case TTM_PL_TT:
>   			placement = "GTT";
>   			break;
> +		case AMDGPU_PL_PREEMPT:
> +			placement = "PREEMPTIBLE";
> +			break;

While at it please ass cases for OA, GWS and GDS as well.

With that done the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>

Thanks,
Christian.

>   		case TTM_PL_SYSTEM:
>   		default:
>   			placement = "CPU";

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

[Tvrtko Ursulin]

On 03/05/2024 07:26, Christian König wrote:
> Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> Help code readability by replacing a bunch of:
>>
>> bo->tbo.base.resv == vm->root.bo->tbo.base.resv
>>
>> With:
>>
>> amdgpu_vm_is_bo_always_valid(vm, bo)
>>
>> No functional changes.
>>
>> v2:
>>   * Rename helper and move to amdgpu_vm. (Christian)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 +++++++++++++++----------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
>>   3 files changed, 28 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 67c234bcf89f..e698d65e9508 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_vm_is_bo_always_valid(vm, abo))
>>           return -EPERM;
>>       r = amdgpu_bo_reserve(abo, false);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 8af3f0fd3073..01ca4b35b369 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_vm_is_bo_always_valid(vm, bo))
>>           return;
>>       dma_resv_assert_held(vm->root.bo->tbo.base.resv);
>> @@ -1101,13 +1101,13 @@ 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 &&
>> +    if (!amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>           !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_vm_is_bo_always_valid(vm, bo))
>> +        dma_resv_unlock(bo->tbo.base.resv);
>>   }
>>   void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
>> @@ -1203,8 +1203,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_vm_is_bo_always_valid(vm, bo))
>>           last_update = &vm->last_update;
>>       else
>>           last_update = &bo_va->last_pt_update;
>> @@ -1246,7 +1245,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_vm_is_bo_always_valid(vm, bo)) {
>>           uint32_t mem_type = bo->tbo.resource->mem_type;
>>           if (!(bo->preferred_domains &
>> @@ -1640,10 +1639,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_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved)
>>           amdgpu_vm_bo_moved(&bo_va->base);
>> -    }
>> +
>>       trace_amdgpu_vm_bo_map(bo_va, mapping);
>>   }
>> @@ -1922,7 +1920,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 &&
>> +        if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>               !before->bo_va->base.moved)
>>               amdgpu_vm_bo_moved(&before->bo_va->base);
>>       } else {
>> @@ -1937,7 +1935,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 &&
>> +        if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>               !after->bo_va->base.moved)
>>               amdgpu_vm_bo_moved(&after->bo_va->base);
>>       } else {
>> @@ -2017,7 +2015,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_vm_is_bo_always_valid(vm, bo))
>>               ttm_bo_set_bulk_move(&bo->tbo, NULL);
>>           for (base = &bo_va->base.bo->vm_bo; *base;
>> @@ -2111,7 +2109,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_vm_is_bo_always_valid(vm, bo)) {
>>               amdgpu_vm_bo_evicted(bo_base);
>>               continue;
>>           }
>> @@ -2122,7 +2120,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_vm_is_bo_always_valid(vm, bo))
>>               amdgpu_vm_bo_moved(bo_base);
>>           else
>>               amdgpu_vm_bo_invalidated(bo_base);
>> @@ -2986,3 +2984,15 @@ void amdgpu_vm_update_fault_cache(struct 
>> amdgpu_device *adev,
>>       xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>   }
>> +/**
>> + * amdgpu_vm_is_bo_always_valid - check if the BO is VM always valid
>> + *
>> + * @vm: VM to test against.
>> + * @abo: BO to be tested.
>> + *
>> + * Returns true if the BO is VM always valid.
> 
> Maybe improve that a bit, e.g. something like this:
> 
> "Returns true if the BO shares the dma_resv object with the root PD and 
> is always guaranteed to be valid inside the VM."

I am only unsure if the dma_resv and root PD are too much of an 
implementation details? Or really something the API user must know? 
Considering from the uapi we have this:

/* Flag that BO is always valid in this VM */
#define AMDGPU_GEM_CREATE_VM_ALWAYS_VALID	(1 << 6)

Which is the reason I thought to keep the comment high level.

Give me a final verdict and I can change it accordingly.

Regards,

Tvrtko

> With that done the patch is Reviewed-by: Christian König 
> <christian.koenig@amd.com>
> 
> Thanks,
> Christian.
> 
>> + */
>> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct 
>> amdgpu_bo *bo)
>> +{
>> +    return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 54d7da396de0..ec688a47dec1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -561,6 +561,8 @@ void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm 
>> *vm, struct seq_file *m);
>>   int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct 
>> amdgpu_vm *vm);
>> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct 
>> amdgpu_bo *bo);
>> +
>>   /**
>>    * amdgpu_vm_tlb_seq - return tlb flush sequence number
>>    * @vm: the amdgpu_vm structure to query
> 

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

[Christian König]

Am 03.05.24 um 10:23 schrieb Tvrtko Ursulin:
>
> On 03/05/2024 07:26, Christian König wrote:
>> Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>
>>> Help code readability by replacing a bunch of:
>>>
>>> bo->tbo.base.resv == vm->root.bo->tbo.base.resv
>>>
>>> With:
>>>
>>> amdgpu_vm_is_bo_always_valid(vm, bo)
>>>
>>> No functional changes.
>>>
>>> v2:
>>>   * Rename helper and move to amdgpu_vm. (Christian)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  2 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 
>>> +++++++++++++++----------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
>>>   3 files changed, 28 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 67c234bcf89f..e698d65e9508 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_vm_is_bo_always_valid(vm, abo))
>>>           return -EPERM;
>>>       r = amdgpu_bo_reserve(abo, false);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 8af3f0fd3073..01ca4b35b369 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_vm_is_bo_always_valid(vm, bo))
>>>           return;
>>>       dma_resv_assert_held(vm->root.bo->tbo.base.resv);
>>> @@ -1101,13 +1101,13 @@ 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 &&
>>> +    if (!amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>>           !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_vm_is_bo_always_valid(vm, bo))
>>> +        dma_resv_unlock(bo->tbo.base.resv);
>>>   }
>>>   void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
>>> @@ -1203,8 +1203,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_vm_is_bo_always_valid(vm, bo))
>>>           last_update = &vm->last_update;
>>>       else
>>>           last_update = &bo_va->last_pt_update;
>>> @@ -1246,7 +1245,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_vm_is_bo_always_valid(vm, bo)) {
>>>           uint32_t mem_type = bo->tbo.resource->mem_type;
>>>           if (!(bo->preferred_domains &
>>> @@ -1640,10 +1639,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_vm_is_bo_always_valid(vm, bo) && !bo_va->base.moved)
>>>           amdgpu_vm_bo_moved(&bo_va->base);
>>> -    }
>>> +
>>>       trace_amdgpu_vm_bo_map(bo_va, mapping);
>>>   }
>>> @@ -1922,7 +1920,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 &&
>>> +        if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>>               !before->bo_va->base.moved)
>>> amdgpu_vm_bo_moved(&before->bo_va->base);
>>>       } else {
>>> @@ -1937,7 +1935,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 &&
>>> +        if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
>>>               !after->bo_va->base.moved)
>>> amdgpu_vm_bo_moved(&after->bo_va->base);
>>>       } else {
>>> @@ -2017,7 +2015,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_vm_is_bo_always_valid(vm, bo))
>>>               ttm_bo_set_bulk_move(&bo->tbo, NULL);
>>>           for (base = &bo_va->base.bo->vm_bo; *base;
>>> @@ -2111,7 +2109,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_vm_is_bo_always_valid(vm, bo)) {
>>>               amdgpu_vm_bo_evicted(bo_base);
>>>               continue;
>>>           }
>>> @@ -2122,7 +2120,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_vm_is_bo_always_valid(vm, bo))
>>>               amdgpu_vm_bo_moved(bo_base);
>>>           else
>>>               amdgpu_vm_bo_invalidated(bo_base);
>>> @@ -2986,3 +2984,15 @@ void amdgpu_vm_update_fault_cache(struct 
>>> amdgpu_device *adev,
>>>       xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>   }
>>> +/**
>>> + * amdgpu_vm_is_bo_always_valid - check if the BO is VM always valid
>>> + *
>>> + * @vm: VM to test against.
>>> + * @abo: BO to be tested.
>>> + *
>>> + * Returns true if the BO is VM always valid.
>>
>> Maybe improve that a bit, e.g. something like this:
>>
>> "Returns true if the BO shares the dma_resv object with the root PD 
>> and is always guaranteed to be valid inside the VM."
>
> I am only unsure if the dma_resv and root PD are too much of an 
> implementation details? Or really something the API user must know? 

Yeah that's exactly the reason why I wanted to add this note.

It is an implementation detail, but everybody working on the kernel code 
needs to keep that in the back of their minds.

Background is that sharing the dma_resv object has some consequences on 
synchronization for example you can't ignore.

> Considering from the uapi we have this:
>
> /* Flag that BO is always valid in this VM */
> #define AMDGPU_GEM_CREATE_VM_ALWAYS_VALID    (1 << 6)
>
> Which is the reason I thought to keep the comment high level.

It shouldn't matter for the UAPI, but we just need to keep it in mind 
inside the kernel.

Regards,
Christian.

>
> Give me a final verdict and I can change it accordingly.
>
> Regards,
>
> Tvrtko
>
>> With that done the patch is Reviewed-by: Christian König 
>> <christian.koenig@amd.com>
>>
>> Thanks,
>> Christian.
>>
>>> + */
>>> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct 
>>> amdgpu_bo *bo)
>>> +{
>>> +    return bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv;
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 54d7da396de0..ec688a47dec1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -561,6 +561,8 @@ void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm 
>>> *vm, struct seq_file *m);
>>>   int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct 
>>> amdgpu_vm *vm);
>>> +bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct 
>>> amdgpu_bo *bo);
>>> +
>>>   /**
>>>    * amdgpu_vm_tlb_seq - return tlb flush sequence number
>>>    * @vm: the amdgpu_vm structure to query
>>