Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
Hi Christian,
> On May 4, 2023, at 20:51, Christian König
> wrote:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 08eced097bd8..9e751f5d4aa7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -882,25 +840,13 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
>
> mutex_lock(&p->bo_list->bo_list_mutex);
>
> - /* One for TTM and one for the CS job */
> - amdgpu_bo_list_for_each_entry(e, p->bo_list)
> - e->tv.num_shared = 2;
> -
> - amdgpu_bo_list_get_list(p->bo_list, &p->validated);
> -
> - INIT_LIST_HEAD(&duplicates);
> - amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
> -
> - if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
> - list_add(&p->uf_entry.tv.head, &p->validated);
> -
> /* Get userptr backing pages. If pages are updated after registered
> * in amdgpu_gem_userptr_ioctl(), amdgpu_cs_list_validate() will do
> * amdgpu_ttm_backend_bind() to flush and invalidate new pages
> */
> amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> - struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> bool userpage_invalidated = false;
> + struct amdgpu_bo *bo = e->bo;
> int i;
>
> e->user_pages = kvmalloc_array(bo->tbo.ttm->num_pages,
> @@ -1307,20 +1281,22 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser
> *p,
> }
>
> p->fence = dma_fence_get(&leader->base.s_fence->finished);
> - list_for_each_entry(e, &p->validated, tv.head) {
> + drm_exec_for_each_locked_object(&p->exec, index, gobj) {
> +
> + ttm_bo_move_to_lru_tail_unlocked(&gem_to_amdgpu_bo(gobj)->tbo);
>
> /* Everybody except for the gang leader uses READ */
> for (i = 0; i < p->gang_size; ++i) {
> if (p->jobs[i] == leader)
> continue;
>
> - dma_resv_add_fence(e->tv.bo->base.resv,
> + dma_resv_add_fence(gobj->resv,
> &p->jobs[i]->base.s_fence->finished,
> DMA_RESV_USAGE_READ);
> }
>
> - /* The gang leader is remembered as writer */
> - e->tv.num_shared = 0;
> + /* The gang leader as remembered as writer */
> + dma_resv_add_fence(gobj->resv, p->fence, DMA_RESV_USAGE_WRITE);
> }
>
> seq = amdgpu_ctx_add_fence(p->ctx, p->entities[p->gang_leader_idx],
I believe this changes the usage of VM PDs from READ to WRITE.
Maybe we could check if a BO is PD and supply DMA_RESV_USAGE_READ in that case?
Tatsuyuki
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
Hi Chrisitan, On May 4, 2023, at 20:51, Christian König mailto:ckoenig.leichtzumer...@gmail.com>> wrote: Use the new component here as well and remove the old handling. v2: drop dupplicate handling It seems that after dropping the duplicate handling, locking of VM PDs on global BO list is basically broken everywhere, as bo->tbo.base.resv == vm->root.bo->tbo.base.resv for AMDGPU_GEM_CREATE_VM_ALWAYS_VALID Perhaps we need to bring dup handling back? Tatsuyuki
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
+Boris and +Matthew in case you want to take over this patch set.
Here are some review / testing comments, including those I posted before
to ease tracking.
On 5/4/23 20:51, Christian König wrote:
Use the new component here as well and remove the old handling.
v2: drop dupplicate handling
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 71 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 5 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 210 +---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h | 7 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 --
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 -
7 files changed, 115 insertions(+), 204 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 02b827785e39..eba3e4f01ea6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -133,6 +141,8 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
struct drm_file *filp,
list->first_userptr = first_userptr;
list->num_entries = num_entries;
+ sort(array, last_entry, sizeof(struct amdgpu_bo_list_entry),
+amdgpu_bo_list_entry_cmp, NULL);
Previously amdgpu_bo_list_get_list sorted all entries, but this one only
sorts userptr entries. I think this changes behavior?
@@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
*p,
e->user_invalidated = userpage_invalidated;
}
- r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
- &duplicates);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
- goto out_free_user_pages;
+ drm_exec_while_not_all_locked(&p->exec) {
+ r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
+ drm_exec_continue_on_contention(&p->exec);
Duplicate handling is needed for pretty much every call of
amdgpu_vm_lock_pd, as bo->tbo.base.resv == vm->root.bo->tbo.base.resv
for AMDGPU_GEM_CREATE_VM_ALWAYS_VALID.
I think Boris's suggestion of having this through a common
DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well.
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ amdgpu_bo_list_for_each_entry(e, p->bo_list) {
+ r = drm_exec_prepare_obj(&p->exec, &e->bo->tbo.base, 2);
Previously there were comments for how the fence count is calculated,
now they seem to be removed. I'd prefer if they were properly retained
as finding out who calls drm_resv_add_fence is not trivial, and wrong
reservation count can also be really hard to debug.
Likewise for amdgpu_vm_lock_pd (which was added in another commit).
+ drm_exec_break_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ e->bo_va = amdgpu_vm_bo_find(vm, e->bo);
+ e->range = NULL;
+ }
+ drm_exec_continue_on_contention(&p->exec);
+
+ if (p->uf_bo) {
+ r = drm_exec_prepare_obj(&p->exec, &p->uf_bo->tbo.base,
+2);
+ drm_exec_continue_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+ }
}
- amdgpu_bo_list_for_each_entry(e, p->bo_list) {
- struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
+ amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
+ struct mm_struct *usermm;
- e->bo_va = amdgpu_vm_bo_find(vm, bo);
+ usermm = amdgpu_ttm_tt_get_usermm(e->bo->tbo.ttm);
+ if (usermm && usermm != current->mm) {
+ r = -EPERM;
+ goto out_free_user_pages;
+ }
+
+ if (amdgpu_ttm_tt_is_userptr(e->bo->tbo.ttm) &&
+ e->user_invalidated && e->user_pages) {
+ amdgpu_bo_placement_from_domain(e->bo,
+ AMDGPU_GEM_DOMAIN_CPU);
+ r = ttm_bo_validate(&e->bo->tbo, &e->bo->placement,
+ &ctx);
+ if (r)
+ goto out_free_user_pages;
+
+ amdgpu_ttm_tt_set_user_pages(e->bo->tbo.ttm,
+e->user_pages);
+ }
+
+ kvfree(e->user_pages);
+ e->user_pages = NULL;
}
amdgpu_cs_get_threshold_for_moves(p->adev, &p->bytes_moved_threshold,
@@ -1296,9 +1271,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_pars
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
On 6/20/23 13:07, Tatsuyuki Ishi wrote:
@@ -1296,9 +1271,8 @@ static int amdgpu_cs_submit(struct
amdgpu_cs_parser *p,
*/
r = 0;
amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
- struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
-
- r |= !amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm, e->range);
+ r |= !amdgpu_ttm_tt_get_user_pages_done(e->bo->tbo.ttm,
+ e->range);
e->range = NULL;
e->range = NULL; needs to be removed, or it's causing *massive* memory
leaks.
Actually, I quoted the wrong hunk, the correct one is below.
@@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
*p,
e->user_invalidated = userpage_invalidated;
}
- r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
- &duplicates);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
- goto out_free_user_pages;
+ drm_exec_while_not_all_locked(&p->exec) {
+ r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
+ drm_exec_continue_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ amdgpu_bo_list_for_each_entry(e, p->bo_list) {
+ r = drm_exec_prepare_obj(&p->exec, &e->bo->tbo.base, 2);
+ drm_exec_break_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ e->bo_va = amdgpu_vm_bo_find(vm, e->bo);
+ e->range = NULL;
This causes the leak.
+ }
+ drm_exec_continue_on_contention(&p->exec);
+
+ if (p->uf_bo) {
+ r = drm_exec_prepare_obj(&p->exec, &p->uf_bo->tbo.base,
+2);
+ drm_exec_continue_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+ }
}
Tatsuyuki
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
Am 20.06.23 um 06:07 schrieb Tatsuyuki Ishi:
+Boris and +Matthew in case you want to take over this patch set.
Here are some review / testing comments, including those I posted
before to ease tracking.
On 5/4/23 20:51, Christian König wrote:
Use the new component here as well and remove the old handling.
v2: drop dupplicate handling
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 71 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 5 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 210 +---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h | 7 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 --
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 -
7 files changed, 115 insertions(+), 204 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 02b827785e39..eba3e4f01ea6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -133,6 +141,8 @@ int amdgpu_bo_list_create(struct amdgpu_device
*adev, struct drm_file *filp,
list->first_userptr = first_userptr;
list->num_entries = num_entries;
+ sort(array, last_entry, sizeof(struct amdgpu_bo_list_entry),
+ amdgpu_bo_list_entry_cmp, NULL);
Previously amdgpu_bo_list_get_list sorted all entries, but this one
only sorts userptr entries. I think this changes behavior?
The intention here is to sort all entries except the userptrs. Need to
double check.
@@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct
amdgpu_cs_parser *p,
e->user_invalidated = userpage_invalidated;
}
- r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
- &duplicates);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
- goto out_free_user_pages;
+ drm_exec_while_not_all_locked(&p->exec) {
+ r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
+ drm_exec_continue_on_contention(&p->exec);
Duplicate handling is needed for pretty much every call of
amdgpu_vm_lock_pd, as bo->tbo.base.resv == vm->root.bo->tbo.base.resv
for AMDGPU_GEM_CREATE_VM_ALWAYS_VALID.
Well no. AMDGPU_GEM_CREATE_VM_ALWAYS_VALID means that BOs should *not*
be part of the relocation list. So when those cause an EALREADY here
then userspace has a bug.
I think Boris's suggestion of having this through a common
DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well.
No, again. The only driver which should accept duplicates is radeon, for
all other drivers especially new ones duplicates should probably be
rejected.
We only allow this for radeon because it is already UAPI, could be that
we need to do this for amdgpu as well but I really hope we don't need this.
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ amdgpu_bo_list_for_each_entry(e, p->bo_list) {
+ r = drm_exec_prepare_obj(&p->exec, &e->bo->tbo.base, 2);
Previously there were comments for how the fence count is calculated,
now they seem to be removed. I'd prefer if they were properly retained
as finding out who calls drm_resv_add_fence is not trivial, and wrong
reservation count can also be really hard to debug.
I need to double check this, the reservation count looks incorrect in
the first place.
Likewise for amdgpu_vm_lock_pd (which was added in another commit).
+ drm_exec_break_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ e->bo_va = amdgpu_vm_bo_find(vm, e->bo);
+ e->range = NULL;
+ }
+ drm_exec_continue_on_contention(&p->exec);
+
+ if (p->uf_bo) {
+ r = drm_exec_prepare_obj(&p->exec, &p->uf_bo->tbo.base,
+ 2);
+ drm_exec_continue_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+ }
}
- amdgpu_bo_list_for_each_entry(e, p->bo_list) {
- struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
+ amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
+ struct mm_struct *usermm;
- e->bo_va = amdgpu_vm_bo_find(vm, bo);
+ usermm = amdgpu_ttm_tt_get_usermm(e->bo->tbo.ttm);
+ if (usermm && usermm != current->mm) {
+ r = -EPERM;
+ goto out_free_user_pages;
+ }
+
+ if (amdgpu_ttm_tt_is_userptr(e->bo->tbo.ttm) &&
+ e->user_invalidated && e->user_pages) {
+ amdgpu_bo_placement_from_domain(e->bo,
+ AMDGPU_GEM_DOMAIN_CPU);
+ r = ttm_bo_validate(&e->bo->tbo, &e->bo->placement,
+ &ctx);
+ if (r)
+ goto out_free_user_pages;
+
+ amdgpu_ttm_tt_set_user_pages(e->bo->tbo.ttm,
+ e->user_pages);
+ }
+
+ kvf
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
Am 20.06.23 um 06:14 schrieb Tatsuyuki Ishi:
On 6/20/23 13:07, Tatsuyuki Ishi wrote:
@@ -1296,9 +1271,8 @@ static int amdgpu_cs_submit(struct
amdgpu_cs_parser *p,
*/
r = 0;
amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
- struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
-
- r |= !amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm,
e->range);
+ r |= !amdgpu_ttm_tt_get_user_pages_done(e->bo->tbo.ttm,
+ e->range);
e->range = NULL;
e->range = NULL; needs to be removed, or it's causing *massive*
memory leaks.
Actually, I quoted the wrong hunk, the correct one is below.
Ah, yes that makes more sense. Going to take a look.
Thanks,
Christian.
@@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct
amdgpu_cs_parser *p,
e->user_invalidated = userpage_invalidated;
}
- r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
- &duplicates);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
- goto out_free_user_pages;
+ drm_exec_while_not_all_locked(&p->exec) {
+ r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
+ drm_exec_continue_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ amdgpu_bo_list_for_each_entry(e, p->bo_list) {
+ r = drm_exec_prepare_obj(&p->exec, &e->bo->tbo.base, 2);
+ drm_exec_break_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+
+ e->bo_va = amdgpu_vm_bo_find(vm, e->bo);
+ e->range = NULL;
This causes the leak.
+ }
+ drm_exec_continue_on_contention(&p->exec);
+
+ if (p->uf_bo) {
+ r = drm_exec_prepare_obj(&p->exec, &p->uf_bo->tbo.base,
+ 2);
+ drm_exec_continue_on_contention(&p->exec);
+ if (unlikely(r))
+ goto out_free_user_pages;
+ }
}
Tatsuyuki
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
On 6/20/23 17:12, Christian König wrote:
Am 20.06.23 um 06:07 schrieb Tatsuyuki Ishi:
+Boris and +Matthew in case you want to take over this patch set.
Here are some review / testing comments, including those I posted before to
ease tracking.
On 5/4/23 20:51, Christian König wrote:
Use the new component here as well and remove the old handling.
v2: drop dupplicate handling
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 71 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 5 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 210 +---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h | 7 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 22 --
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 -
7 files changed, 115 insertions(+), 204 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 02b827785e39..eba3e4f01ea6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -133,6 +141,8 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
struct drm_file *filp,
list->first_userptr = first_userptr;
list->num_entries = num_entries;
+ sort(array, last_entry, sizeof(struct amdgpu_bo_list_entry),
+ amdgpu_bo_list_entry_cmp, NULL);
Previously amdgpu_bo_list_get_list sorted all entries, but this one only sorts
userptr entries. I think this changes behavior?
The intention here is to sort all entries except the userptrs. Need to double
check.
Sorry, I mistyped. You're right that it sorts all entries except the userptrs.
The previous code seems to sort all entries including userptrs.
@@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
*p,
e->user_invalidated = userpage_invalidated;
}
- r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
- &duplicates);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
- goto out_free_user_pages;
+ drm_exec_while_not_all_locked(&p->exec) {
+ r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
+ drm_exec_continue_on_contention(&p->exec);
Duplicate handling is needed for pretty much every call of amdgpu_vm_lock_pd, as
bo->tbo.base.resv == vm->root.bo->tbo.base.resv for
AMDGPU_GEM_CREATE_VM_ALWAYS_VALID.
Well no. AMDGPU_GEM_CREATE_VM_ALWAYS_VALID means that BOs should *not* be part
of the relocation list. So when those cause an EALREADY here then userspace has
a bug.
Sounds fair, lemme check how RADV is handling this again.
Tatsuyuki
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
On Tue, 20 Jun 2023 10:12:13 +0200 Christian König wrote: > > I think Boris's suggestion of having this through a common > > DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well. > > No, again. The only driver which should accept duplicates is radeon, for > all other drivers especially new ones duplicates should probably be > rejected. > > We only allow this for radeon because it is already UAPI, could be that > we need to do this for amdgpu as well but I really hope we don't need this. Just want to describe the use case we have: we support submission in batch (several jobs passed to the submit ioctl) with a submit-all-or-nothing model: if any of the job description is passed wrong args or causes an allocation error, we fail the whole group. In the submission path, we want to prepare GEMs for all jobs. That means adding enough fence slots for the number job finished fences. Given not all jobs will access the same set of BOs, I thought I could use duplicates support to make my life easier, because otherwise I have to collect all BOs upfront, store them in a temporary array, and keep track of the number of fence slots needed for each of them. I guess the other option would be to over-estimate the number of slots and make it equal to num_jobs for all BOs.
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
Am 20.06.23 um 10:28 schrieb Boris Brezillon: On Tue, 20 Jun 2023 10:12:13 +0200 Christian König wrote: I think Boris's suggestion of having this through a common DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well. No, again. The only driver which should accept duplicates is radeon, for all other drivers especially new ones duplicates should probably be rejected. We only allow this for radeon because it is already UAPI, could be that we need to do this for amdgpu as well but I really hope we don't need this. Just want to describe the use case we have: we support submission in batch (several jobs passed to the submit ioctl) with a submit-all-or-nothing model: if any of the job description is passed wrong args or causes an allocation error, we fail the whole group. In the submission path, we want to prepare GEMs for all jobs. That means adding enough fence slots for the number job finished fences. Given not all jobs will access the same set of BOs, I thought I could use duplicates support to make my life easier, because otherwise I have to collect all BOs upfront, store them in a temporary array, and keep track of the number of fence slots needed for each of them. I guess the other option would be to over-estimate the number of slots and make it equal to num_jobs for all BOs. Sounds pretty much what amdgpu is doing as well, but question is why don't you give just one list of BOs? Do you really want to add the fences that fine grained? For radeon it turned out that we just had stupid userspace which sometimes mentioned a BO in the list twice. On the other hand over estimating the number of fences needed is perfectly fine as well, that is rounded up to the next kvmalloc size or even next page size anyway. So IIRC and you have <510 fences you either get 14, 30, 62, 126, 254 and above 510 you should get it rounded to the next 512. Regards, Christian.
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
On 6/20/23 17:16, Tatsuyuki Ishi wrote:
On 6/20/23 17:12, Christian König wrote:
Am 20.06.23 um 06:07 schrieb Tatsuyuki Ishi:
@@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
*p,
e->user_invalidated = userpage_invalidated;
}
- r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
- &duplicates);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
- goto out_free_user_pages;
+ drm_exec_while_not_all_locked(&p->exec) {
+ r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
+ drm_exec_continue_on_contention(&p->exec);
Duplicate handling is needed for pretty much every call of amdgpu_vm_lock_pd, as
bo->tbo.base.resv == vm->root.bo->tbo.base.resv for
AMDGPU_GEM_CREATE_VM_ALWAYS_VALID.
Well no. AMDGPU_GEM_CREATE_VM_ALWAYS_VALID means that BOs should *not* be part
of the relocation list. So when those cause an EALREADY here then userspace has
a bug.
Sounds fair, lemme check how RADV is handling this again.
I checked again and relocation list was actually fine, but other places were
not. For example amdgpu_gem_object_close
locks both bo->tbo.base.resv and vm->root.bo->tbo.base.resv (PD) on its own.
This was the easily debuggable case since it caused an error log but some other
BO operations on ALWAYS_VALID
is also presumably broken due to the same reason.
Tatsuyuki
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
On Tue, 20 Jun 2023 10:44:26 +0200 Christian König wrote: > Am 20.06.23 um 10:28 schrieb Boris Brezillon: > > On Tue, 20 Jun 2023 10:12:13 +0200 > > Christian König wrote: > > > >>> I think Boris's suggestion of having this through a common > >>> DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well. > >> No, again. The only driver which should accept duplicates is radeon, for > >> all other drivers especially new ones duplicates should probably be > >> rejected. > >> > >> We only allow this for radeon because it is already UAPI, could be that > >> we need to do this for amdgpu as well but I really hope we don't need > >> this. > > Just want to describe the use case we have: we support submission in > > batch (several jobs passed to the submit ioctl) with a > > submit-all-or-nothing model: if any of the job description is passed > > wrong args or causes an allocation error, we fail the whole group. In > > the submission path, we want to prepare GEMs for all jobs. That means > > adding enough fence slots for the number job finished fences. Given not > > all jobs will access the same set of BOs, I thought I could use > > duplicates support to make my life easier, because otherwise I have to > > collect all BOs upfront, store them in a temporary array, and keep > > track of the number of fence slots needed for each of them. I guess > > the other option would be to over-estimate the number of slots and make > > it equal to num_jobs for all BOs. > > Sounds pretty much what amdgpu is doing as well, but question is why > don't you give just one list of BOs? Do you really want to add the > fences that fine grained? Actually, we don't give a list of BOs at all, we pass a VM, and lock all BOs attached to the VM (similar to what Xe does). And, as all other drivers being submitted recently, we use explicit sync, so most of those VM BOs, except for the imported/exported ones, will be given a BOOKKEEP fence. The reason we need support for duplicates is because we also have implicit BOs (like the HWRT object that's shared by the geometry/fragment queues to pass data around), and those can be passed to multiple jobs in a given batch and require special synchronization (geometry job writes to them, fragment job reads from them, so we have a reader/writer sync to express). I can of course de-duplicate upfront, by parsing jobs and creating an array of BOs that need to be acquired over the whole submission, but that's still one extra-step I'd prefer to avoid, given the dma_resv framework allows us to figure it out at lock time. I can also just deal with the EALREADY case in the driver directly, it's not like it's super complicated anyway, just thought other drivers would fall in the same situation, that's all. > > For radeon it turned out that we just had stupid userspace which > sometimes mentioned a BO in the list twice. Okay, that's not the same thing, indeed. > > On the other hand over estimating the number of fences needed is > perfectly fine as well, that is rounded up to the next kvmalloc size or > even next page size anyway. Yeah, actually over-provisioning is not the most annoying part. Iterating over jobs to collect 'meta'-BOs is, so if I can just rely on EALREADY to detect that case and fallback to reserving an extra slot in that situation, I'd prefer that.
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
Am 20.06.23 um 11:09 schrieb Boris Brezillon: On Tue, 20 Jun 2023 10:44:26 +0200 Christian König wrote: Am 20.06.23 um 10:28 schrieb Boris Brezillon: On Tue, 20 Jun 2023 10:12:13 +0200 Christian König wrote: I think Boris's suggestion of having this through a common DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well. No, again. The only driver which should accept duplicates is radeon, for all other drivers especially new ones duplicates should probably be rejected. We only allow this for radeon because it is already UAPI, could be that we need to do this for amdgpu as well but I really hope we don't need this. Just want to describe the use case we have: we support submission in batch (several jobs passed to the submit ioctl) with a submit-all-or-nothing model: if any of the job description is passed wrong args or causes an allocation error, we fail the whole group. In the submission path, we want to prepare GEMs for all jobs. That means adding enough fence slots for the number job finished fences. Given not all jobs will access the same set of BOs, I thought I could use duplicates support to make my life easier, because otherwise I have to collect all BOs upfront, store them in a temporary array, and keep track of the number of fence slots needed for each of them. I guess the other option would be to over-estimate the number of slots and make it equal to num_jobs for all BOs. Sounds pretty much what amdgpu is doing as well, but question is why don't you give just one list of BOs? Do you really want to add the fences that fine grained? Actually, we don't give a list of BOs at all, we pass a VM, and lock all BOs attached to the VM (similar to what Xe does). And, as all other drivers being submitted recently, we use explicit sync, so most of those VM BOs, except for the imported/exported ones, will be given a BOOKKEEP fence. The reason we need support for duplicates is because we also have implicit BOs (like the HWRT object that's shared by the geometry/fragment queues to pass data around), and those can be passed to multiple jobs in a given batch and require special synchronization (geometry job writes to them, fragment job reads from them, so we have a reader/writer sync to express). I can of course de-duplicate upfront, by parsing jobs and creating an array of BOs that need to be acquired over the whole submission, but that's still one extra-step I'd prefer to avoid, given the dma_resv framework allows us to figure it out at lock time. I can also just deal with the EALREADY case in the driver directly, it's not like it's super complicated anyway, just thought other drivers would fall in the same situation, that's all. Well as long as you just need to ignore EALREADY, that should be trivial and doable. What radeon needs is to keep EALREADY BOs in a separate container because we need to double check their properties to not break the UAPI. I strongly think that this shouldn't be needed by any other driver. Going to add a flag to ignore EALREADY which can be set during exec init. Regards, Christian. For radeon it turned out that we just had stupid userspace which sometimes mentioned a BO in the list twice. Okay, that's not the same thing, indeed. On the other hand over estimating the number of fences needed is perfectly fine as well, that is rounded up to the next kvmalloc size or even next page size anyway. Yeah, actually over-provisioning is not the most annoying part. Iterating over jobs to collect 'meta'-BOs is, so if I can just rely on EALREADY to detect that case and fallback to reserving an extra slot in that situation, I'd prefer that.
Re: [PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2
On Tue, 20 Jun 2023 11:14:51 +0200 Christian König wrote: > Am 20.06.23 um 11:09 schrieb Boris Brezillon: > > On Tue, 20 Jun 2023 10:44:26 +0200 > > Christian König wrote: > > > >> Am 20.06.23 um 10:28 schrieb Boris Brezillon: > >>> On Tue, 20 Jun 2023 10:12:13 +0200 > >>> Christian König wrote: > >>> > > I think Boris's suggestion of having this through a common > > DRM_EXEC_FLAG_ALLOW_DUPLICATES flag fits well. > No, again. The only driver which should accept duplicates is radeon, for > all other drivers especially new ones duplicates should probably be > rejected. > > We only allow this for radeon because it is already UAPI, could be that > we need to do this for amdgpu as well but I really hope we don't need > this. > >>> Just want to describe the use case we have: we support submission in > >>> batch (several jobs passed to the submit ioctl) with a > >>> submit-all-or-nothing model: if any of the job description is passed > >>> wrong args or causes an allocation error, we fail the whole group. In > >>> the submission path, we want to prepare GEMs for all jobs. That means > >>> adding enough fence slots for the number job finished fences. Given not > >>> all jobs will access the same set of BOs, I thought I could use > >>> duplicates support to make my life easier, because otherwise I have to > >>> collect all BOs upfront, store them in a temporary array, and keep > >>> track of the number of fence slots needed for each of them. I guess > >>> the other option would be to over-estimate the number of slots and make > >>> it equal to num_jobs for all BOs. > >> Sounds pretty much what amdgpu is doing as well, but question is why > >> don't you give just one list of BOs? Do you really want to add the > >> fences that fine grained? > > Actually, we don't give a list of BOs at all, we pass a VM, and lock > > all BOs attached to the VM (similar to what Xe does). And, as all other > > drivers being submitted recently, we use explicit sync, so most of > > those VM BOs, except for the imported/exported ones, will be given a > > BOOKKEEP fence. > > > > The reason we need support for duplicates is because we also have > > implicit BOs (like the HWRT object that's shared by the > > geometry/fragment queues to pass data around), and those can be passed > > to multiple jobs in a given batch and require special synchronization > > (geometry job writes to them, fragment job reads from them, so we have > > a reader/writer sync to express). I can of course de-duplicate upfront, > > by parsing jobs and creating an array of BOs that need to be acquired > > over the whole submission, but that's still one extra-step I'd prefer > > to avoid, given the dma_resv framework allows us to figure it out at > > lock time. I can also just deal with the EALREADY case in the driver > > directly, it's not like it's super complicated anyway, just thought > > other drivers would fall in the same situation, that's all. > > Well as long as you just need to ignore EALREADY, that should be trivial > and doable. Oh, yeah, that's all I need really. We probably don't want to add the GEM object a second time in the array though, hence the goto reserve_fences in my proposal when EALREADY is returned. > > What radeon needs is to keep EALREADY BOs in a separate container > because we need to double check their properties to not break the UAPI. > > I strongly think that this shouldn't be needed by any other driver. > > Going to add a flag to ignore EALREADY which can be set during exec init. Thanks!
