* [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
@ 2024-04-29 16:47 Tvrtko Ursulin
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-29 16:47 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian König, kernel-dev, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection
2024-04-29 16:47 [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
@ 2024-04-29 16:47 ` Tvrtko Ursulin
2024-04-30 0:31 ` Felix Kuehling
` (2 more replies)
2024-04-29 16:47 ` [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs Tvrtko Ursulin
2024-05-03 6:26 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Christian König
2 siblings, 3 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-29 16:47 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian König, kernel-dev, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
@ 2024-04-30 0:31 ` Felix Kuehling
2024-04-30 8:27 ` Tvrtko Ursulin
2024-04-30 17:16 ` [PATCH v3 " Tvrtko Ursulin
2024-05-03 6:31 ` [PATCH " Christian König
2 siblings, 1 reply; 22+ messages in thread
From: Felix Kuehling @ 2024-04-30 0:31 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx; +Cc: Christian König, kernel-dev, Tvrtko Ursulin
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;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection
2024-04-30 0:31 ` Felix Kuehling
@ 2024-04-30 8:27 ` Tvrtko Ursulin
0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-30 8:27 UTC (permalink / raw)
To: Felix Kuehling, Tvrtko Ursulin, amd-gfx; +Cc: Christian König, kernel-dev
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;
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] drm/amdgpu: Reduce mem_type to domain double indirection
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
2024-04-30 0:31 ` Felix Kuehling
@ 2024-04-30 17:16 ` Tvrtko Ursulin
2024-05-02 22:01 ` Felix Kuehling
2024-05-03 6:31 ` [PATCH " Christian König
2 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-30 17:16 UTC (permalink / raw)
To: amd-gfx; +Cc: kernel-dev, Tvrtko Ursulin, Christian König, Felix Kuehling
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v3 2/3] drm/amdgpu: Reduce mem_type to domain double indirection
2024-04-30 17:16 ` [PATCH v3 " Tvrtko Ursulin
@ 2024-05-02 22:01 ` Felix Kuehling
0 siblings, 0 replies; 22+ messages in thread
From: Felix Kuehling @ 2024-05-02 22:01 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev, Tvrtko Ursulin, Christian König
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;
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
2024-04-30 0:31 ` Felix Kuehling
2024-04-30 17:16 ` [PATCH v3 " Tvrtko Ursulin
@ 2024-05-03 6:31 ` Christian König
2 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2024-05-03 6:31 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev, Tvrtko Ursulin
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;
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs
2024-04-29 16:47 [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
@ 2024-04-29 16:47 ` Tvrtko Ursulin
2024-05-03 6:32 ` Christian König
2024-05-03 6:26 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Christian König
2 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-29 16:47 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian König, kernel-dev, Tvrtko Ursulin, Felix Kuehling
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs
2024-04-29 16:47 ` [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs Tvrtko Ursulin
@ 2024-05-03 6:32 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2024-05-03 6:32 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx
Cc: Christian König, kernel-dev, Tvrtko Ursulin, Felix Kuehling
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";
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-04-29 16:47 [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
2024-04-29 16:47 ` [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs Tvrtko Ursulin
@ 2024-05-03 6:26 ` Christian König
2024-05-03 8:23 ` Tvrtko Ursulin
2 siblings, 1 reply; 22+ messages in thread
From: Christian König @ 2024-05-03 6:26 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx; +Cc: Christian König, kernel-dev, Tvrtko Ursulin
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-03 6:26 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Christian König
@ 2024-05-03 8:23 ` Tvrtko Ursulin
2024-05-03 8:27 ` Christian König
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-05-03 8:23 UTC (permalink / raw)
To: Christian König, Tvrtko Ursulin, amd-gfx
Cc: Christian König, kernel-dev
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-03 8:23 ` Tvrtko Ursulin
@ 2024-05-03 8:27 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2024-05-03 8:27 UTC (permalink / raw)
To: Tvrtko Ursulin, Christian König, Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev
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
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
@ 2024-05-03 9:14 Tvrtko Ursulin
2024-05-06 16:26 ` Tvrtko Ursulin
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-05-03 9:14 UTC (permalink / raw)
To: amd-gfx; +Cc: kernel-dev, Tvrtko Ursulin, Christian König
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)
v3:
* Use Christian's kerneldoc.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41 ++++++++++++++++---------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
3 files changed, 29 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 4e2391c83d7c..d451ff9d5af4 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);
}
@@ -1942,7 +1940,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 {
@@ -1957,7 +1955,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 {
@@ -2037,7 +2035,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;
@@ -2131,7 +2129,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;
}
@@ -2142,7 +2140,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);
@@ -3006,3 +3004,16 @@ 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 shares the dma_resv object with the root PD and is
+ * always guaranteed to be valid inside the VM.
+ */
+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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-03 9:14 Tvrtko Ursulin
@ 2024-05-06 16:26 ` Tvrtko Ursulin
2024-05-08 5:42 ` Christian König
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-05-06 16:26 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev, Christian König
On 03/05/2024 10:14, Tvrtko Ursulin wrote:
> 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)
>
> v3:
> * Use Christian's kerneldoc.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41 ++++++++++++++++---------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
> 3 files changed, 29 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 4e2391c83d7c..d451ff9d5af4 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))
Beware the logic inversion here - luckily no one merged it yet! I will
send a respin..
Regards,
Tvrtko
> + 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);
> }
>
> @@ -1942,7 +1940,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 {
> @@ -1957,7 +1955,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 {
> @@ -2037,7 +2035,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;
> @@ -2131,7 +2129,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;
> }
> @@ -2142,7 +2140,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);
> @@ -3006,3 +3004,16 @@ 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 shares the dma_resv object with the root PD and is
> + * always guaranteed to be valid inside the VM.
> + */
> +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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-06 16:26 ` Tvrtko Ursulin
@ 2024-05-08 5:42 ` Christian König
2024-05-08 8:26 ` Tvrtko Ursulin
0 siblings, 1 reply; 22+ messages in thread
From: Christian König @ 2024-05-08 5:42 UTC (permalink / raw)
To: Tvrtko Ursulin, Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev, Christian König
Am 06.05.24 um 18:26 schrieb Tvrtko Ursulin:
>
> On 03/05/2024 10:14, Tvrtko Ursulin wrote:
>> 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)
>>
>> v3:
>> * Use Christian's kerneldoc.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41 ++++++++++++++++---------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
>> 3 files changed, 29 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 4e2391c83d7c..d451ff9d5af4 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))
>
> Beware the logic inversion here - luckily no one merged it yet! I will
> send a respin..
Let me know when that is done, I was about to merge it.
Regards,
Christian.
>
> Regards,
>
> Tvrtko
>
>> + 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);
>> }
>> @@ -1942,7 +1940,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 {
>> @@ -1957,7 +1955,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 {
>> @@ -2037,7 +2035,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;
>> @@ -2131,7 +2129,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;
>> }
>> @@ -2142,7 +2140,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);
>> @@ -3006,3 +3004,16 @@ 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 shares the dma_resv object with the root
>> PD and is
>> + * always guaranteed to be valid inside the VM.
>> + */
>> +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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-08 5:42 ` Christian König
@ 2024-05-08 8:26 ` Tvrtko Ursulin
2024-05-16 12:21 ` Tvrtko Ursulin
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-05-08 8:26 UTC (permalink / raw)
To: Christian König, Tvrtko Ursulin, amd-gfx
Cc: kernel-dev, Christian König
On 08/05/2024 06:42, Christian König wrote:
> Am 06.05.24 um 18:26 schrieb Tvrtko Ursulin:
>>
>> On 03/05/2024 10:14, Tvrtko Ursulin wrote:
>>> 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)
>>>
>>> v3:
>>> * Use Christian's kerneldoc.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41 ++++++++++++++++---------
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
>>> 3 files changed, 29 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 4e2391c83d7c..d451ff9d5af4 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))
>>
>> Beware the logic inversion here - luckily no one merged it yet! I will
>> send a respin..
>
> Let me know when that is done, I was about to merge it.
First three patches in the series
https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/
if you can pull it from there or I should resend standalone?
Please double check it though. Sorry about that I am so used to
pre-merge CI we have on the i915 preventing silly mistakes such as that one.
Regards,
Tvrtko
>
> Regards,
> Christian.
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>> + 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);
>>> }
>>> @@ -1942,7 +1940,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 {
>>> @@ -1957,7 +1955,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 {
>>> @@ -2037,7 +2035,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;
>>> @@ -2131,7 +2129,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;
>>> }
>>> @@ -2142,7 +2140,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);
>>> @@ -3006,3 +3004,16 @@ 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 shares the dma_resv object with the root
>>> PD and is
>>> + * always guaranteed to be valid inside the VM.
>>> + */
>>> +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
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-08 8:26 ` Tvrtko Ursulin
@ 2024-05-16 12:21 ` Tvrtko Ursulin
2024-05-17 11:02 ` Christian König
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-05-16 12:21 UTC (permalink / raw)
To: Christian König, Tvrtko Ursulin, amd-gfx
Cc: kernel-dev, Christian König
Hi Christian,
On 08/05/2024 09:26, Tvrtko Ursulin wrote:
> On 08/05/2024 06:42, Christian König wrote:
>> Am 06.05.24 um 18:26 schrieb Tvrtko Ursulin:
>>>
>>> On 03/05/2024 10:14, Tvrtko Ursulin wrote:
>>>> 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)
>>>>
>>>> v3:
>>>> * Use Christian's kerneldoc.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41
>>>> ++++++++++++++++---------
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
>>>> 3 files changed, 29 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 4e2391c83d7c..d451ff9d5af4 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))
>>>
>>> Beware the logic inversion here - luckily no one merged it yet! I
>>> will send a respin..
>>
>> Let me know when that is done, I was about to merge it.
>
> First three patches in the series
> https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/ if you can pull it from there or I should resend standalone?
>
> Please double check it though. Sorry about that I am so used to
> pre-merge CI we have on the i915 preventing silly mistakes such as that
> one.
Small reminder on the above since you have been out - are you okay to
pick up the first three patches from
https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/,
if they still look okay to you? Or I should re-send them standalone?
Regards,
Tvrtko
>>>> + 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);
>>>> }
>>>> @@ -1942,7 +1940,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 {
>>>> @@ -1957,7 +1955,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 {
>>>> @@ -2037,7 +2035,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;
>>>> @@ -2131,7 +2129,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;
>>>> }
>>>> @@ -2142,7 +2140,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);
>>>> @@ -3006,3 +3004,16 @@ 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 shares the dma_resv object with the root
>>>> PD and is
>>>> + * always guaranteed to be valid inside the VM.
>>>> + */
>>>> +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
>>
>>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-05-16 12:21 ` Tvrtko Ursulin
@ 2024-05-17 11:02 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2024-05-17 11:02 UTC (permalink / raw)
To: Tvrtko Ursulin, Christian König, Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev
Am 16.05.24 um 14:21 schrieb Tvrtko Ursulin:
>
> Hi Christian,
>
> On 08/05/2024 09:26, Tvrtko Ursulin wrote:
>> On 08/05/2024 06:42, Christian König wrote:
>>> Am 06.05.24 um 18:26 schrieb Tvrtko Ursulin:
>>>>
>>>> On 03/05/2024 10:14, Tvrtko Ursulin wrote:
>>>>> 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)
>>>>>
>>>>> v3:
>>>>> * Use Christian's kerneldoc.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 41
>>>>> ++++++++++++++++---------
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 ++
>>>>> 3 files changed, 29 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 4e2391c83d7c..d451ff9d5af4 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))
>>>>
>>>> Beware the logic inversion here - luckily no one merged it yet! I
>>>> will send a respin..
>>>
>>> Let me know when that is done, I was about to merge it.
>>
>> First three patches in the series
>> https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/
>> if you can pull it from there or I should resend standalone?
>>
>> Please double check it though. Sorry about that I am so used to
>> pre-merge CI we have on the i915 preventing silly mistakes such as
>> that one.
>
> Small reminder on the above since you have been out - are you okay to
> pick up the first three patches from
> https://lore.kernel.org/amd-gfx/20240506165959.50648-1-tursulin@igalia.com/,
> if they still look okay to you? Or I should re-send them standalone?
Ah, thanks for the reminder. Totally forgotten that I wanted to push
this. Just done so.
Regards,
Christian.
>
> Regards,
>
> Tvrtko
>
>>>>> + 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);
>>>>> }
>>>>> @@ -1942,7 +1940,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 {
>>>>> @@ -1957,7 +1955,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 {
>>>>> @@ -2037,7 +2035,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;
>>>>> @@ -2131,7 +2129,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;
>>>>> }
>>>>> @@ -2142,7 +2140,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);
>>>>> @@ -3006,3 +3004,16 @@ 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 shares the dma_resv object with the
>>>>> root PD and is
>>>>> + * always guaranteed to be valid inside the VM.
>>>>> + */
>>>>> +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
>>>
>>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/3] Some refactoring and maybe a memory accounting fixlet
@ 2024-04-26 16:43 Tvrtko Ursulin
2024-04-26 16:43 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-26 16:43 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian König, kernel-dev, Tvrtko Ursulin
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
As I was reading through the driver I spotted one thing which could
perhaps make it more readable (1/3), one thing which reduces some double
conversions (in principle) from TTM placement back to domain (2/3), and
also enables the last patch in the series which maybe fixes a small
memory accounting issue. But only relevant when interacting with KFD
IIUC.
Assuming the fix is real and not my confusion, please let me know if it is
all acceptable, or you would like me to re-structure the series to only
contain the fix and break off the rest for later (or never).
Tvrtko Ursulin (3):
drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
drm/amdgpu: Reduce mem_type to domain double indirection
drm/amdgpu: Fix pinned GART area accounting and fdinfo reporting
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 4 +--
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 31 ++++++++++-----------
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 14 ++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 31 +++++++++------------
5 files changed, 44 insertions(+), 38 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-04-26 16:43 [PATCH 0/3] Some refactoring and maybe a memory accounting fixlet Tvrtko Ursulin
@ 2024-04-26 16:43 ` Tvrtko Ursulin
2024-04-29 11:02 ` Christian König
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-26 16:43 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian König, kernel-dev, 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_bo_is_vm_bo(bo, vm)
No functional changes.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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);
--
2.44.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-04-26 16:43 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
@ 2024-04-29 11:02 ` Christian König
2024-04-29 13:34 ` Tvrtko Ursulin
0 siblings, 1 reply; 22+ messages in thread
From: Christian König @ 2024-04-29 11:02 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx; +Cc: Christian König, kernel-dev, Tvrtko Ursulin
Am 26.04.24 um 18:43 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_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.
Regards,
Christian.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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);
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-04-29 11:02 ` Christian König
@ 2024-04-29 13:34 ` Tvrtko Ursulin
2024-04-29 13:51 ` Christian König
0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2024-04-29 13:34 UTC (permalink / raw)
To: Christian König, Tvrtko Ursulin, amd-gfx
Cc: Christian König, kernel-dev
On 29/04/2024 12:02, Christian König wrote:
> Am 26.04.24 um 18:43 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_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?
Regards,
Tvrtko
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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);
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
2024-04-29 13:34 ` Tvrtko Ursulin
@ 2024-04-29 13:51 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2024-04-29 13:51 UTC (permalink / raw)
To: Tvrtko Ursulin, Christian König, Tvrtko Ursulin, amd-gfx; +Cc: kernel-dev
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.ursulin@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.ursulin@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);
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-05-17 11:02 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-29 16:47 [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
2024-04-29 16:47 ` [PATCH 2/3] drm/amdgpu: Reduce mem_type to domain double indirection Tvrtko Ursulin
2024-04-30 0:31 ` Felix Kuehling
2024-04-30 8:27 ` Tvrtko Ursulin
2024-04-30 17:16 ` [PATCH v3 " Tvrtko Ursulin
2024-05-02 22:01 ` Felix Kuehling
2024-05-03 6:31 ` [PATCH " Christian König
2024-04-29 16:47 ` [PATCH 3/3] drm/amdgpu: Describe preemptible objects in debugfs Tvrtko Ursulin
2024-05-03 6:32 ` Christian König
2024-05-03 6:26 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Christian König
2024-05-03 8:23 ` Tvrtko Ursulin
2024-05-03 8:27 ` Christian König
-- strict thread matches above, loose matches on Subject: below --
2024-05-03 9:14 Tvrtko Ursulin
2024-05-06 16:26 ` Tvrtko Ursulin
2024-05-08 5:42 ` Christian König
2024-05-08 8:26 ` Tvrtko Ursulin
2024-05-16 12:21 ` Tvrtko Ursulin
2024-05-17 11:02 ` Christian König
2024-04-26 16:43 [PATCH 0/3] Some refactoring and maybe a memory accounting fixlet Tvrtko Ursulin
2024-04-26 16:43 ` [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper Tvrtko Ursulin
2024-04-29 11:02 ` Christian König
2024-04-29 13:34 ` Tvrtko Ursulin
2024-04-29 13:51 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox