Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
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 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 Cc: Christian König Reviewed-by: Christian König --- 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-tursu...@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-tursu...@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 = >last_update; else last_update = _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(_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
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
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 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 Cc: Christian König Reviewed-by: Christian König --- 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-tursu...@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-tursu...@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 = >last_update; else last_update = _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(_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(>bo_va->base); } else { @@ -1957,7 +1955,7 @@ int
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
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 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 Cc: Christian König Reviewed-by: Christian König --- 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-tursu...@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 = >last_update; else last_update = _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(_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(>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) &&
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
Am 06.05.24 um 18:26 schrieb Tvrtko Ursulin: On 03/05/2024 10:14, Tvrtko Ursulin wrote: From: Tvrtko Ursulin 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 Cc: Christian König Reviewed-by: Christian König --- 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 = >last_update; else last_update = _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(_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(>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(>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(>tbo, NULL);
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
On 03/05/2024 10:14, Tvrtko Ursulin wrote: From: Tvrtko Ursulin 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 Cc: Christian König Reviewed-by: Christian König --- 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 = >last_update; else last_update = _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(_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(>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(>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 ==
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
On 03/05/2024 07:26, Christian König wrote: Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin: From: Tvrtko Ursulin 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 Cc: Christian König --- 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 = >last_update; else last_update = _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(_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(>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(>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(>tbo, NULL); for (base = _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
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
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 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 Cc: Christian König --- 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 = >last_update; else last_update = _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(_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(>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(>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(>tbo, NULL); for (base = _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 =
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
Am 29.04.24 um 18:47 schrieb Tvrtko Ursulin: From: Tvrtko Ursulin 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 Cc: Christian König --- 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 = >last_update; else last_update = _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(_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(>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(>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(>tbo, NULL); for (base = _va->base.bo->vm_bo; *base; @@
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
On 29/04/2024 12:02, Christian König wrote: Am 26.04.24 um 18:43 schrieb Tvrtko Ursulin: From: Tvrtko Ursulin 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 --- 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 = >last_update; else last_update = _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(_va->base); - } + trace_amdgpu_vm_bo_map(bo_va,
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
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 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 --- 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 = >last_update; else last_update = _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 ==
Re: [PATCH 1/3] drm/amdgpu: Add amdgpu_bo_is_vm_bo helper
Am 26.04.24 um 18:43 schrieb Tvrtko Ursulin: From: Tvrtko Ursulin 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 --- 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 = >last_update; else last_update = _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(_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 &&