Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

So that the driver can set some BOOKKEEP for explicit sync. Maybe some of the existing places would already make sense for that, but I targeted this for no functional changes.

Well first of all NAK to that one since it will totally break cases which need to reserve more than one fence slot.

Also as discussed with Daniel we don't want to use BOOKKEEP for implicit sync. We should instead use READ for that.

BOOKKEEP is for stuff userspace should never be aware of, e.g. like page table updates and KFD eviction fences.

Regards, Christian.

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +++++----- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 8 +++----- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +-- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 2 +- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/qxl/qxl_release.c | 2 +- drivers/gpu/drm/radeon/radeon_cs.c | 5 +++-- drivers/gpu/drm/radeon/radeon_gem.c | 2 +- drivers/gpu/drm/radeon/radeon_vm.c | 4 ++-- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 10 +++++----- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 2 +- include/drm/ttm/ttm_execbuf_util.h | 3 ++- 16 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index a4955ef76cfc..a790a089e829 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -774,7 +774,7 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem, struct amdgpu_bo *bo = mem->bo;

INIT_LIST_HEAD(&entry->head);

• entry->num_shared = 1;

• entry->usage = DMA_RESV_USAGE_READ; entry->bo = &bo->tbo; mutex_lock(&process_info->lock); if (userptr)

@@ -918,7 +918,7 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,

ctx->kfd_bo.priority = 0; ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ; list_add(&ctx->kfd_bo.tv.head, &ctx->list);

  amdgpu_vm_get_pd_bo(vm, &ctx->list, &ctx->vm_pd[0]);

@@ -981,7 +981,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,

ctx->kfd_bo.priority = 0; ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ; list_add(&ctx->kfd_bo.tv.head, &ctx->list);

  i = 0;

@@ -2218,7 +2218,7 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) validate_list.head) { list_add_tail(&mem->resv_list.head, &resv_list); mem->resv_list.bo = mem->validate_list.bo;

• mem->resv_list.num_shared = mem->validate_list.num_shared;
  

• mem->resv_list.usage = mem->validate_list.usage;
  

  }

  /* Reserve all BOs and page tables for validation */

@@ -2417,7 +2417,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)

list_add_tail(&mem->resv_list.head, &ctx.list);
mem->resv_list.bo = mem->validate_list.bo;

• mem->resv_list.num_shared = mem->validate_list.num_shared;
  

• mem->resv_list.usage = mem->validate_list.usage;
  

  }

  ret = ttm_eu_reserve_buffers(&ctx.ticket, &ctx.list,

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 60ca14afb879..2ae1c0d9d33a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -55,8 +55,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p, bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj)); p->uf_entry.priority = 0; p->uf_entry.tv.bo = &bo->tbo;

• /* One for TTM and two for the CS job */
• p->uf_entry.tv.num_shared = 3;

• p->uf_entry.tv.usage = DMA_RESV_USAGE_READ;

  drm_gem_object_put(gobj);

@@ -519,9 +518,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, return r; }

• /* One for TTM and one for the CS job */ amdgpu_bo_list_for_each_entry(e, p->bo_list)

• e->tv.num_shared = 2;
  

• e->tv.usage = DMA_RESV_USAGE_READ;
  
  

  amdgpu_bo_list_get_list(p->bo_list, &p->validated);

@@ -1261,7 +1259,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,

/* Make sure all BOs are remembered as writers */ amdgpu_bo_list_for_each_entry(e, p->bo_list)

• e->tv.num_shared = 0;
  

• e->tv.usage = DMA_RESV_USAGE_WRITE;
  
  

  ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence); mutex_unlock(&p->adev->notifier_lock);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c index c6d4d41c4393..71277257d94d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c @@ -74,7 +74,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm, INIT_LIST_HEAD(&list); INIT_LIST_HEAD(&csa_tv.head); csa_tv.bo = &bo->tbo;

• csa_tv.num_shared = 1;

• csa_tv.usage = DMA_RESV_USAGE_READ;

  list_add(&csa_tv.head, &list); amdgpu_vm_get_pd_bo(vm, &list, &pd);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 84a53758e18e..7483411229f4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -207,7 +207,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, INIT_LIST_HEAD(&duplicates);

tv.bo = &bo->tbo;

• tv.num_shared = 2;

• tv.usage = DMA_RESV_USAGE_READ; list_add(&tv.head, &list);

  amdgpu_vm_get_pd_bo(vm, &list, &vm_pd);

@@ -731,9 +731,9 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, abo = gem_to_amdgpu_bo(gobj); tv.bo = &abo->tbo; if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)

• 	tv.num_shared = 1;
  

• 	tv.usage = DMA_RESV_USAGE_READ;
  

  else

• 	tv.num_shared = 0;
  

• 	tv.usage = DMA_RESV_USAGE_WRITE;
  

  list_add(&tv.head, &list); } else { gobj = NULL;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c index 5224d9a39737..f670d8473993 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c @@ -319,7 +319,7 @@ static int amdgpu_vkms_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

tv.bo = &rbo->tbo;

• tv.num_shared = 1;

• tv.usage = DMA_RESV_USAGE_READ; list_add(&tv.head, &list);

  r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 15184153e2b9..515be19ab279 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -633,8 +633,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.bo->tbo;

• /* Two for VM updates, one for TTM and one for the CS job */
• entry->tv.num_shared = 4;

• entry->tv.usage = DMA_RESV_USAGE_READ; entry->user_pages = NULL; list_add(&entry->tv.head, validated); }

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index b3fc3e958227..af844b636778 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -1395,7 +1395,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx) vm = drm_priv_to_vm(pdd->drm_priv);

ctx->tv[gpuidx].bo = &vm->root.bo->tbo;

• ctx->tv[gpuidx].num_shared = 4;
  

• ctx->tv[gpuidx].usage = DMA_RESV_USAGE_READ;
  

  list_add(&ctx->tv[gpuidx].head, &ctx->validate_list); }

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 73423b805b54..851b7844b084 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7601,7 +7601,7 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

tv.bo = &rbo->tbo;

• tv.num_shared = 1;

• tv.usage = DMA_RESV_USAGE_READ; list_add(&tv.head, &list);

  r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 368d26da0d6a..689e35192070 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -183,7 +183,7 @@ int qxl_release_list_add(struct qxl_release *release, struct qxl_bo *bo)

qxl_bo_ref(bo); entry->tv.bo = &bo->tbo;

• entry->tv.num_shared = 0;

• entry->tv.usage = DMA_RESV_USAGE_WRITE; list_add_tail(&entry->tv.head, &release->bos); return 0; }

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 446f7bae54c4..30afe0c62dd9 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -183,7 +183,8 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) }

p->relocs[i].tv.bo = &p->relocs[i].robj->tbo;

• p->relocs[i].tv.num_shared = !r->write_domain;
  

• p->relocs[i].tv.usage =
  

• 	r->write_domain ? DMA_RESV_USAGE_WRITE : DMA_RESV_USAGE_READ;
  
  

  radeon_cs_buckets_add(&buckets, &p->relocs[i].tv.head, priority);

@@ -258,7 +259,7 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)

resv = reloc->robj->tbo.base.resv;
r = radeon_sync_resv(p->rdev, &p->ib.sync, resv,

• 		     reloc->tv.num_shared);
  

• 		     reloc->tv.usage != DMA_RESV_USAGE_WRITE);
  

  if (r) return r; }

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 8c01a7f0e027..eae47c709f5d 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -635,7 +635,7 @@ static void radeon_gem_va_update_vm(struct radeon_device *rdev, INIT_LIST_HEAD(&list);

tv.bo = &bo_va->bo->tbo;

• tv.num_shared = 1;

• tv.usage = DMA_RESV_USAGE_READ; list_add(&tv.head, &list);

  vm_bos = radeon_vm_get_bos(rdev, bo_va->vm, &list);

diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c index 987cabbf1318..702627b48dae 100644 --- a/drivers/gpu/drm/radeon/radeon_vm.c +++ b/drivers/gpu/drm/radeon/radeon_vm.c @@ -143,7 +143,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[0].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[0].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[0].tv.bo = &vm->page_directory->tbo;

• list[0].tv.num_shared = 1;

• list[0].tv.usage = DMA_RESV_USAGE_READ; list[0].tiling_flags = 0; list_add(&list[0].tv.head, head);

@@ -155,7 +155,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[idx].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].tv.bo = &list[idx].robj->tbo;

• list[idx].tv.num_shared = 1;
  

• list[idx].tv.usage = DMA_RESV_USAGE_READ;
  

  list[idx].tiling_flags = 0; list_add(&list[idx++].tv.head, head); }

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 0eb995d25df1..c39d8e5ac271 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -101,7 +101,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, continue; }

• num_fences = min(entry->num_shared, 1u);
  

• num_fences = entry->usage <= DMA_RESV_USAGE_WRITE ? 0u : 1u;
  

  if (!ret) { ret = dma_resv_reserve_fences(bo->base.resv, num_fences);

@@ -154,8 +154,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo;

• dma_resv_add_fence(bo->base.resv, fence, entry->num_shared ?
  

• 		   DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE);
  

• dma_resv_add_fence(bo->base.resv, fence, entry->usage);
  

  ttm_bo_move_to_lru_tail_unlocked(bo); dma_resv_unlock(bo->base.resv); }

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index c6d02c98a19a..58dfff7d6c76 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -130,7 +130,7 @@ static void vmw_resource_release(struct kref *kref) struct ttm_validate_buffer val_buf;

	val_buf.bo = bo;

• 	val_buf.num_shared = 0;
  

• 	val_buf.usage = DMA_RESV_USAGE_WRITE;
  res->func->unbind(res, false, &val_buf);
  

  } res->backup_dirty = false;

@@ -552,7 +552,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket, INIT_LIST_HEAD(&val_list); ttm_bo_get(&res->backup->base); val_buf->bo = &res->backup->base;

• val_buf->num_shared = 0;

• val_buf->usage = DMA_RESV_USAGE_WRITE; list_add_tail(&val_buf->head, &val_list); ret = ttm_eu_reserve_buffers(ticket, &val_list, interruptible, NULL); if (unlikely(ret != 0))

@@ -657,7 +657,7 @@ static int vmw_resource_do_evict(struct ww_acquire_ctx *ticket, BUG_ON(!func->may_evict);

val_buf.bo = NULL;

• val_buf.num_shared = 0;

• val_buf.usage = DMA_RESV_USAGE_WRITE; ret = vmw_resource_check_buffer(ticket, res, interruptible, &val_buf); if (unlikely(ret != 0)) return ret;

@@ -708,7 +708,7 @@ int vmw_resource_validate(struct vmw_resource *res, bool intr, return 0;

val_buf.bo = NULL;

• val_buf.num_shared = 0;

• val_buf.usage = DMA_RESV_USAGE_WRITE; if (res->backup) val_buf.bo = &res->backup->base; do {

@@ -777,7 +777,7 @@ void vmw_resource_unbind_list(struct vmw_buffer_object *vbo) { struct ttm_validate_buffer val_buf = { .bo = &vbo->base,

• .num_shared = 0
  

• .usage = DMA_RESV_USAGE_WRITE
  

  };

  dma_resv_assert_held(vbo->base.base.resv);

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c index f46891012be3..0476ba498321 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c @@ -288,7 +288,7 @@ int vmw_validation_add_bo(struct vmw_validation_context *ctx, val_buf->bo = ttm_bo_get_unless_zero(&vbo->base); if (!val_buf->bo) return -ESRCH;

• val_buf->num_shared = 0;
  

• val_buf->usage = DMA_RESV_USAGE_WRITE;
  

  list_add_tail(&val_buf->head, &ctx->bo_list); bo_node->as_mob = as_mob; bo_node->cpu_blit = cpu_blit;

diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index a99d7fdf2964..851961a06c27 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -31,6 +31,7 @@ #ifndef _TTM_EXECBUF_UTIL_H_ #define _TTM_EXECBUF_UTIL_H_

+#include <linux/dma-resv.h> #include <linux/list.h>

#include "ttm_bo_api.h" @@ -46,7 +47,7 @@ struct ttm_validate_buffer { struct list_head head; struct ttm_buffer_object *bo;

• unsigned int num_shared;

• enum dma_resv_usage usage; };

  /**

Am 01.06.22 um 10:11 schrieb Bas Nieuwenhuizen:

On Wed, Jun 1, 2022 at 10:02 AM Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

...

So that the driver can set some BOOKKEEP for explicit sync. Maybe some of the existing places would already make sense for that, but I targeted this for no functional changes.

Well first of all NAK to that one since it will totally break cases which need to reserve more than one fence slot.

TTM already didn't do that? From ttm_execbuf_util.c :

...

...

•         num_fences = min(entry->num_shared, 1u);
  

•         num_fences = entry->usage <= DMA_RESV_USAGE_WRITE ? 0u : 1u;
  

That's doing a min(entry->num_shared, 1u). In other words even when the driver requested to reserve no fence we at least reserve at least one.

But if the driver requested to reserve more than one then we do reserve more than one. That's rather important because both radeon and amdgpu need that for their VM updates.

This patch here completely breaks that.

There is already an drm_exec patch set from me on the dri-devel mailing list which untangles all of this and deprecates the whole ttm_exec_buf_util handling.

Regards, Christian.

...

Also as discussed with Daniel we don't want to use BOOKKEEP for implicit sync. We should instead use READ for that.

That is the plan and what we do later in the series, use BOOKKEEP for submissions that don't want to participate in implicit sync?

This refactor sets everything to READ or WRITE based on the previous num_shared value, to make sure this patch by itself is not a functional change.

...

BOOKKEEP is for stuff userspace should never be aware of, e.g. like page table updates and KFD eviction fences.

Regards, Christian.

...

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +++++----- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 8 +++----- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +-- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 2 +- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/qxl/qxl_release.c | 2 +- drivers/gpu/drm/radeon/radeon_cs.c | 5 +++-- drivers/gpu/drm/radeon/radeon_gem.c | 2 +- drivers/gpu/drm/radeon/radeon_vm.c | 4 ++-- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 10 +++++----- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 2 +- include/drm/ttm/ttm_execbuf_util.h | 3 ++- 16 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index a4955ef76cfc..a790a089e829 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -774,7 +774,7 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem, struct amdgpu_bo *bo = mem->bo;

   INIT_LIST_HEAD(&entry->head);

• entry->num_shared = 1;
  

• entry->usage = DMA_RESV_USAGE_READ;
   entry->bo = &bo->tbo;
   mutex_lock(&process_info->lock);
   if (userptr)
  

@@ -918,7 +918,7 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,

   ctx->kfd_bo.priority = 0;
   ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;
  

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ;
   list_add(&ctx->kfd_bo.tv.head, &ctx->list);
  
   amdgpu_vm_get_pd_bo(vm, &ctx->list, &ctx->vm_pd[0]);
  

@@ -981,7 +981,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,

   ctx->kfd_bo.priority = 0;
   ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;
  

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ;
   list_add(&ctx->kfd_bo.tv.head, &ctx->list);
  
   i = 0;
  

@@ -2218,7 +2218,7 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) validate_list.head) { list_add_tail(&mem->resv_list.head, &resv_list); mem->resv_list.bo = mem->validate_list.bo;

•         mem->resv_list.num_shared = mem->validate_list.num_shared;
  

•         mem->resv_list.usage = mem->validate_list.usage;
   }
  
   /* Reserve all BOs and page tables for validation */
  

@@ -2417,7 +2417,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)

           list_add_tail(&mem->resv_list.head, &ctx.list);
           mem->resv_list.bo = mem->validate_list.bo;

•         mem->resv_list.num_shared = mem->validate_list.num_shared;
  

•         mem->resv_list.usage = mem->validate_list.usage;
   }
  
   ret = ttm_eu_reserve_buffers(&ctx.ticket, &ctx.list,
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 60ca14afb879..2ae1c0d9d33a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -55,8 +55,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p, bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj)); p->uf_entry.priority = 0; p->uf_entry.tv.bo = &bo->tbo;

• /* One for TTM and two for the CS job */
  

• p->uf_entry.tv.num_shared = 3;
  

• p->uf_entry.tv.usage = DMA_RESV_USAGE_READ;
  
   drm_gem_object_put(gobj);
  
  

@@ -519,9 +518,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, return r; }

• /* One for TTM and one for the CS job */
   amdgpu_bo_list_for_each_entry(e, p->bo_list)
  

•         e->tv.num_shared = 2;
  

•         e->tv.usage = DMA_RESV_USAGE_READ;
  
   amdgpu_bo_list_get_list(p->bo_list, &p->validated);
  
  

@@ -1261,7 +1259,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,

   /* Make sure all BOs are remembered as writers */
   amdgpu_bo_list_for_each_entry(e, p->bo_list)

•         e->tv.num_shared = 0;
  

•         e->tv.usage = DMA_RESV_USAGE_WRITE;
  
   ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
   mutex_unlock(&p->adev->notifier_lock);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c index c6d4d41c4393..71277257d94d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c @@ -74,7 +74,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm, INIT_LIST_HEAD(&list); INIT_LIST_HEAD(&csa_tv.head); csa_tv.bo = &bo->tbo;

• csa_tv.num_shared = 1;
  

• csa_tv.usage = DMA_RESV_USAGE_READ;
  
   list_add(&csa_tv.head, &list);
   amdgpu_vm_get_pd_bo(vm, &list, &pd);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 84a53758e18e..7483411229f4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -207,7 +207,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, INIT_LIST_HEAD(&duplicates);

   tv.bo = &bo->tbo;

• tv.num_shared = 2;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   amdgpu_vm_get_pd_bo(vm, &list, &vm_pd);
  

@@ -731,9 +731,9 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, abo = gem_to_amdgpu_bo(gobj); tv.bo = &abo->tbo; if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)

•                 tv.num_shared = 1;
  

•                 tv.usage = DMA_RESV_USAGE_READ;
           else
  

•                 tv.num_shared = 0;
  

•                 tv.usage = DMA_RESV_USAGE_WRITE;
           list_add(&tv.head, &list);
   } else {
           gobj = NULL;
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c index 5224d9a39737..f670d8473993 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c @@ -319,7 +319,7 @@ static int amdgpu_vkms_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

   tv.bo = &rbo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 15184153e2b9..515be19ab279 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -633,8 +633,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.bo->tbo;

• /* Two for VM updates, one for TTM and one for the CS job */
  

• entry->tv.num_shared = 4;
  

• entry->tv.usage = DMA_RESV_USAGE_READ;
   entry->user_pages = NULL;
   list_add(&entry->tv.head, validated);
  

  }

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index b3fc3e958227..af844b636778 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -1395,7 +1395,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx) vm = drm_priv_to_vm(pdd->drm_priv);

           ctx->tv[gpuidx].bo = &vm->root.bo->tbo;

•         ctx->tv[gpuidx].num_shared = 4;
  

•         ctx->tv[gpuidx].usage = DMA_RESV_USAGE_READ;
           list_add(&ctx->tv[gpuidx].head, &ctx->validate_list);
   }
  
  

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 73423b805b54..851b7844b084 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7601,7 +7601,7 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

   tv.bo = &rbo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);
  

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 368d26da0d6a..689e35192070 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -183,7 +183,7 @@ int qxl_release_list_add(struct qxl_release *release, struct qxl_bo *bo)

   qxl_bo_ref(bo);
   entry->tv.bo = &bo->tbo;

• entry->tv.num_shared = 0;
  

• entry->tv.usage = DMA_RESV_USAGE_WRITE;
   list_add_tail(&entry->tv.head, &release->bos);
   return 0;
  

  }

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 446f7bae54c4..30afe0c62dd9 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -183,7 +183,8 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) }

           p->relocs[i].tv.bo = &p->relocs[i].robj->tbo;

•         p->relocs[i].tv.num_shared = !r->write_domain;
  

•         p->relocs[i].tv.usage =
  

•                 r->write_domain ? DMA_RESV_USAGE_WRITE : DMA_RESV_USAGE_READ;
  
           radeon_cs_buckets_add(&buckets, &p->relocs[i].tv.head,
                                 priority);
  

@@ -258,7 +259,7 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)

           resv = reloc->robj->tbo.base.resv;
           r = radeon_sync_resv(p->rdev, &p->ib.sync, resv,

•                              reloc->tv.num_shared);
  

•                              reloc->tv.usage != DMA_RESV_USAGE_WRITE);
           if (r)
                   return r;
   }
  

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 8c01a7f0e027..eae47c709f5d 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -635,7 +635,7 @@ static void radeon_gem_va_update_vm(struct radeon_device *rdev, INIT_LIST_HEAD(&list);

   tv.bo = &bo_va->bo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   vm_bos = radeon_vm_get_bos(rdev, bo_va->vm, &list);
  

diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c index 987cabbf1318..702627b48dae 100644 --- a/drivers/gpu/drm/radeon/radeon_vm.c +++ b/drivers/gpu/drm/radeon/radeon_vm.c @@ -143,7 +143,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[0].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[0].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[0].tv.bo = &vm->page_directory->tbo;

• list[0].tv.num_shared = 1;
  

• list[0].tv.usage = DMA_RESV_USAGE_READ;
   list[0].tiling_flags = 0;
   list_add(&list[0].tv.head, head);
  
  

@@ -155,7 +155,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[idx].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].tv.bo = &list[idx].robj->tbo;

•         list[idx].tv.num_shared = 1;
  

•         list[idx].tv.usage = DMA_RESV_USAGE_READ;
           list[idx].tiling_flags = 0;
           list_add(&list[idx++].tv.head, head);
   }
  

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 0eb995d25df1..c39d8e5ac271 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -101,7 +101,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, continue; }

•         num_fences = min(entry->num_shared, 1u);
  

•         num_fences = entry->usage <= DMA_RESV_USAGE_WRITE ? 0u : 1u;
           if (!ret) {
                   ret = dma_resv_reserve_fences(bo->base.resv,
                                                 num_fences);
  

@@ -154,8 +154,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo;

•         dma_resv_add_fence(bo->base.resv, fence, entry->num_shared ?
  

•                            DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE);
  

•         dma_resv_add_fence(bo->base.resv, fence, entry->usage);
           ttm_bo_move_to_lru_tail_unlocked(bo);
           dma_resv_unlock(bo->base.resv);
   }
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index c6d02c98a19a..58dfff7d6c76 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -130,7 +130,7 @@ static void vmw_resource_release(struct kref *kref) struct ttm_validate_buffer val_buf;

                   val_buf.bo = bo;

•                 val_buf.num_shared = 0;
  

•                 val_buf.usage = DMA_RESV_USAGE_WRITE;
                   res->func->unbind(res, false, &val_buf);
           }
           res->backup_dirty = false;
  

@@ -552,7 +552,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket, INIT_LIST_HEAD(&val_list); ttm_bo_get(&res->backup->base); val_buf->bo = &res->backup->base;

• val_buf->num_shared = 0;
  

• val_buf->usage = DMA_RESV_USAGE_WRITE;
   list_add_tail(&val_buf->head, &val_list);
   ret = ttm_eu_reserve_buffers(ticket, &val_list, interruptible, NULL);
   if (unlikely(ret != 0))
  

@@ -657,7 +657,7 @@ static int vmw_resource_do_evict(struct ww_acquire_ctx *ticket, BUG_ON(!func->may_evict);

   val_buf.bo = NULL;

• val_buf.num_shared = 0;
  

• val_buf.usage = DMA_RESV_USAGE_WRITE;
   ret = vmw_resource_check_buffer(ticket, res, interruptible, &val_buf);
   if (unlikely(ret != 0))
           return ret;
  

@@ -708,7 +708,7 @@ int vmw_resource_validate(struct vmw_resource *res, bool intr, return 0;

   val_buf.bo = NULL;

• val_buf.num_shared = 0;
  

• val_buf.usage = DMA_RESV_USAGE_WRITE;
   if (res->backup)
           val_buf.bo = &res->backup->base;
   do {
  

@@ -777,7 +777,7 @@ void vmw_resource_unbind_list(struct vmw_buffer_object *vbo) { struct ttm_validate_buffer val_buf = { .bo = &vbo->base,

•         .num_shared = 0
  

•         .usage = DMA_RESV_USAGE_WRITE
   };
  
   dma_resv_assert_held(vbo->base.base.resv);
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c index f46891012be3..0476ba498321 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c @@ -288,7 +288,7 @@ int vmw_validation_add_bo(struct vmw_validation_context *ctx, val_buf->bo = ttm_bo_get_unless_zero(&vbo->base); if (!val_buf->bo) return -ESRCH;

•         val_buf->num_shared = 0;
  

•         val_buf->usage = DMA_RESV_USAGE_WRITE;
           list_add_tail(&val_buf->head, &ctx->bo_list);
           bo_node->as_mob = as_mob;
           bo_node->cpu_blit = cpu_blit;
  

diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index a99d7fdf2964..851961a06c27 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -31,6 +31,7 @@ #ifndef _TTM_EXECBUF_UTIL_H_ #define _TTM_EXECBUF_UTIL_H_

+#include <linux/dma-resv.h> #include <linux/list.h>

#include "ttm_bo_api.h" @@ -46,7 +47,7 @@ struct ttm_validate_buffer { struct list_head head; struct ttm_buffer_object *bo;

• unsigned int num_shared;
  

• enum dma_resv_usage usage;
  

  };

  /**

Am 01.06.22 um 10:39 schrieb Bas Nieuwenhuizen:

On Wed, Jun 1, 2022 at 10:29 AM Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 10:11 schrieb Bas Nieuwenhuizen:

...

On Wed, Jun 1, 2022 at 10:02 AM Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

...

So that the driver can set some BOOKKEEP for explicit sync. Maybe some of the existing places would already make sense for that, but I targeted this for no functional changes.

Well first of all NAK to that one since it will totally break cases which need to reserve more than one fence slot.

TTM already didn't do that? From ttm_execbuf_util.c :

...

...

•         num_fences = min(entry->num_shared, 1u);
  

•         num_fences = entry->usage <= DMA_RESV_USAGE_WRITE ? 0u : 1u;
  

That's doing a min(entry->num_shared, 1u). In other words even when the driver requested to reserve no fence we at least reserve at least one.

That would be the case if it was a max, not a min. However, since it is a min, it only ever resulted in 0 or 1, behavior that we mimic based on DMA_RESV_USAGE_*.

Ah! You are working on a broken branch, that was fixed with:

commit d72dcbe9fce505228dae43bef9da8f2b707d1b3d Author: Christian König christian.koenig@amd.com Date:   Mon Apr 11 15:21:59 2022 +0200

drm/ttm: fix logic inversion in ttm_eu_reserve_buffers

That should have been max, not min.

Without that fix your branch can cause rare to debug memory corruptions.

Regards, Christian.

Nowhere else do we actually use the specific number assigned to num_shared.

...

But if the driver requested to reserve more than one then we do reserve more than one. That's rather important because both radeon and amdgpu need that for their VM updates.

This patch here completely breaks that.

There is already an drm_exec patch set from me on the dri-devel mailing list which untangles all of this and deprecates the whole ttm_exec_buf_util handling.

Can take a look at your patch, but I believe in pre-patch state this is a correct non functional change.

...

Regards, Christian.

...

...

Also as discussed with Daniel we don't want to use BOOKKEEP for implicit sync. We should instead use READ for that.

That is the plan and what we do later in the series, use BOOKKEEP for submissions that don't want to participate in implicit sync?

This refactor sets everything to READ or WRITE based on the previous num_shared value, to make sure this patch by itself is not a functional change.

...

BOOKKEEP is for stuff userspace should never be aware of, e.g. like page table updates and KFD eviction fences.

Regards, Christian.

...

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 10 +++++-----
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c            |  8 +++-----
drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c           |  2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c           |  6 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c          |  2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c            |  3 +--
drivers/gpu/drm/amd/amdkfd/kfd_svm.c              |  2 +-
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
drivers/gpu/drm/qxl/qxl_release.c                 |  2 +-
drivers/gpu/drm/radeon/radeon_cs.c                |  5 +++--
drivers/gpu/drm/radeon/radeon_gem.c               |  2 +-
drivers/gpu/drm/radeon/radeon_vm.c                |  4 ++--
drivers/gpu/drm/ttm/ttm_execbuf_util.c            |  5 ++---
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c          | 10 +++++-----
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c        |  2 +-
include/drm/ttm/ttm_execbuf_util.h                |  3 ++-
16 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index a4955ef76cfc..a790a089e829 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -774,7 +774,7 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem, struct amdgpu_bo *bo = mem->bo;

    INIT_LIST_HEAD(&entry->head);

• entry->num_shared = 1;
  

• entry->usage = DMA_RESV_USAGE_READ;
    entry->bo = &bo->tbo;
    mutex_lock(&process_info->lock);
    if (userptr)
  

@@ -918,7 +918,7 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,

    ctx->kfd_bo.priority = 0;
    ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;
  

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ;
    list_add(&ctx->kfd_bo.tv.head, &ctx->list);
  
    amdgpu_vm_get_pd_bo(vm, &ctx->list, &ctx->vm_pd[0]);
  

@@ -981,7 +981,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,

    ctx->kfd_bo.priority = 0;
    ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;
  

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ;
    list_add(&ctx->kfd_bo.tv.head, &ctx->list);
  
    i = 0;
  

@@ -2218,7 +2218,7 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) validate_list.head) { list_add_tail(&mem->resv_list.head, &resv_list); mem->resv_list.bo = mem->validate_list.bo;

•         mem->resv_list.num_shared = mem->validate_list.num_shared;
  

•         mem->resv_list.usage = mem->validate_list.usage;
    }
  
    /* Reserve all BOs and page tables for validation */
  

@@ -2417,7 +2417,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)

            list_add_tail(&mem->resv_list.head, &ctx.list);
            mem->resv_list.bo = mem->validate_list.bo;

•         mem->resv_list.num_shared = mem->validate_list.num_shared;
  

•         mem->resv_list.usage = mem->validate_list.usage;
    }
  
    ret = ttm_eu_reserve_buffers(&ctx.ticket, &ctx.list,
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 60ca14afb879..2ae1c0d9d33a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -55,8 +55,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p, bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj)); p->uf_entry.priority = 0; p->uf_entry.tv.bo = &bo->tbo;

• /* One for TTM and two for the CS job */
  

• p->uf_entry.tv.num_shared = 3;
  

• p->uf_entry.tv.usage = DMA_RESV_USAGE_READ;
  
    drm_gem_object_put(gobj);
  
  

@@ -519,9 +518,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, return r; }

• /* One for TTM and one for the CS job */
    amdgpu_bo_list_for_each_entry(e, p->bo_list)
  

•         e->tv.num_shared = 2;
  

•         e->tv.usage = DMA_RESV_USAGE_READ;
  
    amdgpu_bo_list_get_list(p->bo_list, &p->validated);
  
  

@@ -1261,7 +1259,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,

    /* Make sure all BOs are remembered as writers */
    amdgpu_bo_list_for_each_entry(e, p->bo_list)

•         e->tv.num_shared = 0;
  

•         e->tv.usage = DMA_RESV_USAGE_WRITE;
  
    ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
    mutex_unlock(&p->adev->notifier_lock);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c index c6d4d41c4393..71277257d94d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c @@ -74,7 +74,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm, INIT_LIST_HEAD(&list); INIT_LIST_HEAD(&csa_tv.head); csa_tv.bo = &bo->tbo;

• csa_tv.num_shared = 1;
  

• csa_tv.usage = DMA_RESV_USAGE_READ;
  
    list_add(&csa_tv.head, &list);
    amdgpu_vm_get_pd_bo(vm, &list, &pd);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 84a53758e18e..7483411229f4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -207,7 +207,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, INIT_LIST_HEAD(&duplicates);

    tv.bo = &bo->tbo;

• tv.num_shared = 2;
  

• tv.usage = DMA_RESV_USAGE_READ;
    list_add(&tv.head, &list);
  
    amdgpu_vm_get_pd_bo(vm, &list, &vm_pd);
  

@@ -731,9 +731,9 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, abo = gem_to_amdgpu_bo(gobj); tv.bo = &abo->tbo; if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)

•                 tv.num_shared = 1;
  

•                 tv.usage = DMA_RESV_USAGE_READ;
            else
  

•                 tv.num_shared = 0;
  

•                 tv.usage = DMA_RESV_USAGE_WRITE;
            list_add(&tv.head, &list);
    } else {
            gobj = NULL;
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c index 5224d9a39737..f670d8473993 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c @@ -319,7 +319,7 @@ static int amdgpu_vkms_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

    tv.bo = &rbo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
    list_add(&tv.head, &list);
  
    r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 15184153e2b9..515be19ab279 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -633,8 +633,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.bo->tbo;

• /* Two for VM updates, one for TTM and one for the CS job */
  

• entry->tv.num_shared = 4;
  

• entry->tv.usage = DMA_RESV_USAGE_READ;
    entry->user_pages = NULL;
    list_add(&entry->tv.head, validated);
  

  }

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index b3fc3e958227..af844b636778 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -1395,7 +1395,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx) vm = drm_priv_to_vm(pdd->drm_priv);

            ctx->tv[gpuidx].bo = &vm->root.bo->tbo;

•         ctx->tv[gpuidx].num_shared = 4;
  

•         ctx->tv[gpuidx].usage = DMA_RESV_USAGE_READ;
            list_add(&ctx->tv[gpuidx].head, &ctx->validate_list);
    }
  
  

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 73423b805b54..851b7844b084 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7601,7 +7601,7 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

    tv.bo = &rbo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
    list_add(&tv.head, &list);
  
    r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);
  

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 368d26da0d6a..689e35192070 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -183,7 +183,7 @@ int qxl_release_list_add(struct qxl_release *release, struct qxl_bo *bo)

    qxl_bo_ref(bo);
    entry->tv.bo = &bo->tbo;

• entry->tv.num_shared = 0;
  

• entry->tv.usage = DMA_RESV_USAGE_WRITE;
    list_add_tail(&entry->tv.head, &release->bos);
    return 0;
  

  }

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 446f7bae54c4..30afe0c62dd9 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -183,7 +183,8 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) }

            p->relocs[i].tv.bo = &p->relocs[i].robj->tbo;

•         p->relocs[i].tv.num_shared = !r->write_domain;
  

•         p->relocs[i].tv.usage =
  

•                 r->write_domain ? DMA_RESV_USAGE_WRITE : DMA_RESV_USAGE_READ;
  
            radeon_cs_buckets_add(&buckets, &p->relocs[i].tv.head,
                                  priority);
  

@@ -258,7 +259,7 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)

            resv = reloc->robj->tbo.base.resv;
            r = radeon_sync_resv(p->rdev, &p->ib.sync, resv,

•                              reloc->tv.num_shared);
  

•                              reloc->tv.usage != DMA_RESV_USAGE_WRITE);
            if (r)
                    return r;
    }
  

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 8c01a7f0e027..eae47c709f5d 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -635,7 +635,7 @@ static void radeon_gem_va_update_vm(struct radeon_device *rdev, INIT_LIST_HEAD(&list);

    tv.bo = &bo_va->bo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
    list_add(&tv.head, &list);
  
    vm_bos = radeon_vm_get_bos(rdev, bo_va->vm, &list);
  

diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c index 987cabbf1318..702627b48dae 100644 --- a/drivers/gpu/drm/radeon/radeon_vm.c +++ b/drivers/gpu/drm/radeon/radeon_vm.c @@ -143,7 +143,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[0].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[0].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[0].tv.bo = &vm->page_directory->tbo;

• list[0].tv.num_shared = 1;
  

• list[0].tv.usage = DMA_RESV_USAGE_READ;
    list[0].tiling_flags = 0;
    list_add(&list[0].tv.head, head);
  
  

@@ -155,7 +155,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[idx].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].tv.bo = &list[idx].robj->tbo;

•         list[idx].tv.num_shared = 1;
  

•         list[idx].tv.usage = DMA_RESV_USAGE_READ;
            list[idx].tiling_flags = 0;
            list_add(&list[idx++].tv.head, head);
    }
  

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 0eb995d25df1..c39d8e5ac271 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -101,7 +101,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, continue; }

•         num_fences = min(entry->num_shared, 1u);
  

•         num_fences = entry->usage <= DMA_RESV_USAGE_WRITE ? 0u : 1u;
            if (!ret) {
                    ret = dma_resv_reserve_fences(bo->base.resv,
                                                  num_fences);
  

@@ -154,8 +154,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo;

•         dma_resv_add_fence(bo->base.resv, fence, entry->num_shared ?
  

•                            DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE);
  

•         dma_resv_add_fence(bo->base.resv, fence, entry->usage);
            ttm_bo_move_to_lru_tail_unlocked(bo);
            dma_resv_unlock(bo->base.resv);
    }
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index c6d02c98a19a..58dfff7d6c76 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -130,7 +130,7 @@ static void vmw_resource_release(struct kref *kref) struct ttm_validate_buffer val_buf;

                    val_buf.bo = bo;

•                 val_buf.num_shared = 0;
  

•                 val_buf.usage = DMA_RESV_USAGE_WRITE;
                    res->func->unbind(res, false, &val_buf);
            }
            res->backup_dirty = false;
  

@@ -552,7 +552,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket, INIT_LIST_HEAD(&val_list); ttm_bo_get(&res->backup->base); val_buf->bo = &res->backup->base;

• val_buf->num_shared = 0;
  

• val_buf->usage = DMA_RESV_USAGE_WRITE;
    list_add_tail(&val_buf->head, &val_list);
    ret = ttm_eu_reserve_buffers(ticket, &val_list, interruptible, NULL);
    if (unlikely(ret != 0))
  

@@ -657,7 +657,7 @@ static int vmw_resource_do_evict(struct ww_acquire_ctx *ticket, BUG_ON(!func->may_evict);

    val_buf.bo = NULL;

• val_buf.num_shared = 0;
  

• val_buf.usage = DMA_RESV_USAGE_WRITE;
    ret = vmw_resource_check_buffer(ticket, res, interruptible, &val_buf);
    if (unlikely(ret != 0))
            return ret;
  

@@ -708,7 +708,7 @@ int vmw_resource_validate(struct vmw_resource *res, bool intr, return 0;

    val_buf.bo = NULL;

• val_buf.num_shared = 0;
  

• val_buf.usage = DMA_RESV_USAGE_WRITE;
    if (res->backup)
            val_buf.bo = &res->backup->base;
    do {
  

@@ -777,7 +777,7 @@ void vmw_resource_unbind_list(struct vmw_buffer_object *vbo) { struct ttm_validate_buffer val_buf = { .bo = &vbo->base,

•         .num_shared = 0
  

•         .usage = DMA_RESV_USAGE_WRITE
    };
  
    dma_resv_assert_held(vbo->base.base.resv);
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c index f46891012be3..0476ba498321 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c @@ -288,7 +288,7 @@ int vmw_validation_add_bo(struct vmw_validation_context *ctx, val_buf->bo = ttm_bo_get_unless_zero(&vbo->base); if (!val_buf->bo) return -ESRCH;

•         val_buf->num_shared = 0;
  

•         val_buf->usage = DMA_RESV_USAGE_WRITE;
            list_add_tail(&val_buf->head, &ctx->bo_list);
            bo_node->as_mob = as_mob;
            bo_node->cpu_blit = cpu_blit;
  

diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index a99d7fdf2964..851961a06c27 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -31,6 +31,7 @@ #ifndef _TTM_EXECBUF_UTIL_H_ #define _TTM_EXECBUF_UTIL_H_

+#include <linux/dma-resv.h> #include <linux/list.h>

#include "ttm_bo_api.h"

@@ -46,7 +47,7 @@ struct ttm_validate_buffer { struct list_head head; struct ttm_buffer_object *bo;

• unsigned int num_shared;
  

• enum dma_resv_usage usage;
  

  };

  /**

Am 01.06.22 um 10:41 schrieb Daniel Vetter:

On Wed, 1 Jun 2022 at 10:02, Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

...

So that the driver can set some BOOKKEEP for explicit sync. Maybe some of the existing places would already make sense for that, but I targeted this for no functional changes.

Well first of all NAK to that one since it will totally break cases which need to reserve more than one fence slot.

Quick reminder, we talked about this in the past. For many folks (not you) NAK means "fuck off" and not "this wont work for the reasons I just explained". Looks like the conversation is all on a good track in the further replies, just figured I'll drop this again as a reminder :-)

Yeah, that came to my mind as well.

But I still prefer NAK as what it means in computer since, e.g. "Not AcKnowledged", please restart from scratch.

We do need a clear indicator that the whole approach taken in a patch needs to be dropped and restarted from scratch and a NAK seems to fit that.

When I would want to tell somebody to fuck of I would clearly write that.

Christian.

Maybe do an autocomplete in your mail editor which replaces NAK with NAK (note: this means "fuck off" for many folks) so you can decide whether that's really the message you want to send out to start the morning. And in some rare case I do agree that just dropping a polite "fuck off" is the right thing to make it clear what's up ...

Cheers, Daniel

...

Also as discussed with Daniel we don't want to use BOOKKEEP for implicit sync. We should instead use READ for that.

BOOKKEEP is for stuff userspace should never be aware of, e.g. like page table updates and KFD eviction fences.

Regards, Christian.

...

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +++++----- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 8 +++----- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +-- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 2 +- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- drivers/gpu/drm/qxl/qxl_release.c | 2 +- drivers/gpu/drm/radeon/radeon_cs.c | 5 +++-- drivers/gpu/drm/radeon/radeon_gem.c | 2 +- drivers/gpu/drm/radeon/radeon_vm.c | 4 ++-- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 10 +++++----- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 2 +- include/drm/ttm/ttm_execbuf_util.h | 3 ++- 16 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index a4955ef76cfc..a790a089e829 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -774,7 +774,7 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem, struct amdgpu_bo *bo = mem->bo;

   INIT_LIST_HEAD(&entry->head);

• entry->num_shared = 1;
  

• entry->usage = DMA_RESV_USAGE_READ;
   entry->bo = &bo->tbo;
   mutex_lock(&process_info->lock);
   if (userptr)
  

@@ -918,7 +918,7 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,

   ctx->kfd_bo.priority = 0;
   ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;
  

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ;
   list_add(&ctx->kfd_bo.tv.head, &ctx->list);
  
   amdgpu_vm_get_pd_bo(vm, &ctx->list, &ctx->vm_pd[0]);
  

@@ -981,7 +981,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,

   ctx->kfd_bo.priority = 0;
   ctx->kfd_bo.tv.bo = &bo->tbo;

• ctx->kfd_bo.tv.num_shared = 1;
  

• ctx->kfd_bo.tv.usage = DMA_RESV_USAGE_READ;
   list_add(&ctx->kfd_bo.tv.head, &ctx->list);
  
   i = 0;
  

@@ -2218,7 +2218,7 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) validate_list.head) { list_add_tail(&mem->resv_list.head, &resv_list); mem->resv_list.bo = mem->validate_list.bo;

•         mem->resv_list.num_shared = mem->validate_list.num_shared;
  

•         mem->resv_list.usage = mem->validate_list.usage;
   }
  
   /* Reserve all BOs and page tables for validation */
  

@@ -2417,7 +2417,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)

           list_add_tail(&mem->resv_list.head, &ctx.list);
           mem->resv_list.bo = mem->validate_list.bo;

•         mem->resv_list.num_shared = mem->validate_list.num_shared;
  

•         mem->resv_list.usage = mem->validate_list.usage;
   }
  
   ret = ttm_eu_reserve_buffers(&ctx.ticket, &ctx.list,
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 60ca14afb879..2ae1c0d9d33a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -55,8 +55,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p, bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj)); p->uf_entry.priority = 0; p->uf_entry.tv.bo = &bo->tbo;

• /* One for TTM and two for the CS job */
  

• p->uf_entry.tv.num_shared = 3;
  

• p->uf_entry.tv.usage = DMA_RESV_USAGE_READ;
  
   drm_gem_object_put(gobj);
  
  

@@ -519,9 +518,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, return r; }

• /* One for TTM and one for the CS job */
   amdgpu_bo_list_for_each_entry(e, p->bo_list)
  

•         e->tv.num_shared = 2;
  

•         e->tv.usage = DMA_RESV_USAGE_READ;
  
   amdgpu_bo_list_get_list(p->bo_list, &p->validated);
  
  

@@ -1261,7 +1259,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,

   /* Make sure all BOs are remembered as writers */
   amdgpu_bo_list_for_each_entry(e, p->bo_list)

•         e->tv.num_shared = 0;
  

•         e->tv.usage = DMA_RESV_USAGE_WRITE;
  
   ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
   mutex_unlock(&p->adev->notifier_lock);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c index c6d4d41c4393..71277257d94d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c @@ -74,7 +74,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm, INIT_LIST_HEAD(&list); INIT_LIST_HEAD(&csa_tv.head); csa_tv.bo = &bo->tbo;

• csa_tv.num_shared = 1;
  

• csa_tv.usage = DMA_RESV_USAGE_READ;
  
   list_add(&csa_tv.head, &list);
   amdgpu_vm_get_pd_bo(vm, &list, &pd);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 84a53758e18e..7483411229f4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -207,7 +207,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, INIT_LIST_HEAD(&duplicates);

   tv.bo = &bo->tbo;

• tv.num_shared = 2;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   amdgpu_vm_get_pd_bo(vm, &list, &vm_pd);
  

@@ -731,9 +731,9 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, abo = gem_to_amdgpu_bo(gobj); tv.bo = &abo->tbo; if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)

•                 tv.num_shared = 1;
  

•                 tv.usage = DMA_RESV_USAGE_READ;
           else
  

•                 tv.num_shared = 0;
  

•                 tv.usage = DMA_RESV_USAGE_WRITE;
           list_add(&tv.head, &list);
   } else {
           gobj = NULL;
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c index 5224d9a39737..f670d8473993 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c @@ -319,7 +319,7 @@ static int amdgpu_vkms_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

   tv.bo = &rbo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 15184153e2b9..515be19ab279 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -633,8 +633,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.bo->tbo;

• /* Two for VM updates, one for TTM and one for the CS job */
  

• entry->tv.num_shared = 4;
  

• entry->tv.usage = DMA_RESV_USAGE_READ;
   entry->user_pages = NULL;
   list_add(&entry->tv.head, validated);
  

  }

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index b3fc3e958227..af844b636778 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -1395,7 +1395,7 @@ static int svm_range_reserve_bos(struct svm_validate_context *ctx) vm = drm_priv_to_vm(pdd->drm_priv);

           ctx->tv[gpuidx].bo = &vm->root.bo->tbo;

•         ctx->tv[gpuidx].num_shared = 4;
  

•         ctx->tv[gpuidx].usage = DMA_RESV_USAGE_READ;
           list_add(&ctx->tv[gpuidx].head, &ctx->validate_list);
   }
  
  

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 73423b805b54..851b7844b084 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7601,7 +7601,7 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane, INIT_LIST_HEAD(&list);

   tv.bo = &rbo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   r = ttm_eu_reserve_buffers(&ticket, &list, false, NULL);
  

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 368d26da0d6a..689e35192070 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -183,7 +183,7 @@ int qxl_release_list_add(struct qxl_release *release, struct qxl_bo *bo)

   qxl_bo_ref(bo);
   entry->tv.bo = &bo->tbo;

• entry->tv.num_shared = 0;
  

• entry->tv.usage = DMA_RESV_USAGE_WRITE;
   list_add_tail(&entry->tv.head, &release->bos);
   return 0;
  

  }

diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 446f7bae54c4..30afe0c62dd9 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -183,7 +183,8 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) }

           p->relocs[i].tv.bo = &p->relocs[i].robj->tbo;

•         p->relocs[i].tv.num_shared = !r->write_domain;
  

•         p->relocs[i].tv.usage =
  

•                 r->write_domain ? DMA_RESV_USAGE_WRITE : DMA_RESV_USAGE_READ;
  
           radeon_cs_buckets_add(&buckets, &p->relocs[i].tv.head,
                                 priority);
  

@@ -258,7 +259,7 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)

           resv = reloc->robj->tbo.base.resv;
           r = radeon_sync_resv(p->rdev, &p->ib.sync, resv,

•                              reloc->tv.num_shared);
  

•                              reloc->tv.usage != DMA_RESV_USAGE_WRITE);
           if (r)
                   return r;
   }
  

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 8c01a7f0e027..eae47c709f5d 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -635,7 +635,7 @@ static void radeon_gem_va_update_vm(struct radeon_device *rdev, INIT_LIST_HEAD(&list);

   tv.bo = &bo_va->bo->tbo;

• tv.num_shared = 1;
  

• tv.usage = DMA_RESV_USAGE_READ;
   list_add(&tv.head, &list);
  
   vm_bos = radeon_vm_get_bos(rdev, bo_va->vm, &list);
  

diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c index 987cabbf1318..702627b48dae 100644 --- a/drivers/gpu/drm/radeon/radeon_vm.c +++ b/drivers/gpu/drm/radeon/radeon_vm.c @@ -143,7 +143,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[0].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[0].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[0].tv.bo = &vm->page_directory->tbo;

• list[0].tv.num_shared = 1;
  

• list[0].tv.usage = DMA_RESV_USAGE_READ;
   list[0].tiling_flags = 0;
   list_add(&list[0].tv.head, head);
  
  

@@ -155,7 +155,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev, list[idx].preferred_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].allowed_domains = RADEON_GEM_DOMAIN_VRAM; list[idx].tv.bo = &list[idx].robj->tbo;

•         list[idx].tv.num_shared = 1;
  

•         list[idx].tv.usage = DMA_RESV_USAGE_READ;
           list[idx].tiling_flags = 0;
           list_add(&list[idx++].tv.head, head);
   }
  

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 0eb995d25df1..c39d8e5ac271 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -101,7 +101,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, continue; }

•         num_fences = min(entry->num_shared, 1u);
  

•         num_fences = entry->usage <= DMA_RESV_USAGE_WRITE ? 0u : 1u;
           if (!ret) {
                   ret = dma_resv_reserve_fences(bo->base.resv,
                                                 num_fences);
  

@@ -154,8 +154,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo;

•         dma_resv_add_fence(bo->base.resv, fence, entry->num_shared ?
  

•                            DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE);
  

•         dma_resv_add_fence(bo->base.resv, fence, entry->usage);
           ttm_bo_move_to_lru_tail_unlocked(bo);
           dma_resv_unlock(bo->base.resv);
   }
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index c6d02c98a19a..58dfff7d6c76 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -130,7 +130,7 @@ static void vmw_resource_release(struct kref *kref) struct ttm_validate_buffer val_buf;

                   val_buf.bo = bo;

•                 val_buf.num_shared = 0;
  

•                 val_buf.usage = DMA_RESV_USAGE_WRITE;
                   res->func->unbind(res, false, &val_buf);
           }
           res->backup_dirty = false;
  

@@ -552,7 +552,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket, INIT_LIST_HEAD(&val_list); ttm_bo_get(&res->backup->base); val_buf->bo = &res->backup->base;

• val_buf->num_shared = 0;
  

• val_buf->usage = DMA_RESV_USAGE_WRITE;
   list_add_tail(&val_buf->head, &val_list);
   ret = ttm_eu_reserve_buffers(ticket, &val_list, interruptible, NULL);
   if (unlikely(ret != 0))
  

@@ -657,7 +657,7 @@ static int vmw_resource_do_evict(struct ww_acquire_ctx *ticket, BUG_ON(!func->may_evict);

   val_buf.bo = NULL;

• val_buf.num_shared = 0;
  

• val_buf.usage = DMA_RESV_USAGE_WRITE;
   ret = vmw_resource_check_buffer(ticket, res, interruptible, &val_buf);
   if (unlikely(ret != 0))
           return ret;
  

@@ -708,7 +708,7 @@ int vmw_resource_validate(struct vmw_resource *res, bool intr, return 0;

   val_buf.bo = NULL;

• val_buf.num_shared = 0;
  

• val_buf.usage = DMA_RESV_USAGE_WRITE;
   if (res->backup)
           val_buf.bo = &res->backup->base;
   do {
  

@@ -777,7 +777,7 @@ void vmw_resource_unbind_list(struct vmw_buffer_object *vbo) { struct ttm_validate_buffer val_buf = { .bo = &vbo->base,

•         .num_shared = 0
  

•         .usage = DMA_RESV_USAGE_WRITE
   };
  
   dma_resv_assert_held(vbo->base.base.resv);
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c index f46891012be3..0476ba498321 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c @@ -288,7 +288,7 @@ int vmw_validation_add_bo(struct vmw_validation_context *ctx, val_buf->bo = ttm_bo_get_unless_zero(&vbo->base); if (!val_buf->bo) return -ESRCH;

•         val_buf->num_shared = 0;
  

•         val_buf->usage = DMA_RESV_USAGE_WRITE;
           list_add_tail(&val_buf->head, &ctx->bo_list);
           bo_node->as_mob = as_mob;
           bo_node->cpu_blit = cpu_blit;
  

diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index a99d7fdf2964..851961a06c27 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -31,6 +31,7 @@ #ifndef _TTM_EXECBUF_UTIL_H_ #define _TTM_EXECBUF_UTIL_H_

+#include <linux/dma-resv.h> #include <linux/list.h>

#include "ttm_bo_api.h" @@ -46,7 +47,7 @@ struct ttm_validate_buffer { struct list_head head; struct ttm_buffer_object *bo;

• unsigned int num_shared;
  

• enum dma_resv_usage usage;
  

  };

  /**

On Wed, Jun 1, 2022 at 10:03 AM Christian König christian.koenig@amd.com wrote:

Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

...

This should be okay because moves themselves use KERNEL usage and hence still sync with BOOKKEEP usage. Then any later submits still wait on any pending VM operations.

(i.e. we only made VM ops not wait on BOOKKEEP submits, not the other way around)

Well NAK again. This allows access to freed up memory and is a complete no-go.

How does this allow access to freed memory? Worst I can see is that the unmap happens earlier if the app/drivers gets the waits wrong, which wouldn't give access after the underlying BO is freed?

Regards, Christian.

...

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c index f10332e1c6c0..31bc73fd1fae 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c @@ -51,7 +51,7 @@ static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, if (!resv) return 0;

• return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, sync_mode, p->vm, true);
  

• return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, AMDGPU_SYNC_EXPLICIT, p->vm, true);
  

  }

  /**

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index 63b484dc76c5..c8d5898bea11 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c @@ -75,7 +75,7 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p, if (!resv) return 0;

• return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, sync_mode, p->vm);
  

• return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, AMDGPU_SYNC_EXPLICIT, p->vm);
  

  }

  /**

On Wed, Jun 1, 2022 at 10:40 AM Christian König christian.koenig@amd.com wrote:

Am 01.06.22 um 10:16 schrieb Bas Nieuwenhuizen:

...

On Wed, Jun 1, 2022 at 10:03 AM Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

...

This should be okay because moves themselves use KERNEL usage and hence still sync with BOOKKEEP usage. Then any later submits still wait on any pending VM operations.

(i.e. we only made VM ops not wait on BOOKKEEP submits, not the other way around)

Well NAK again. This allows access to freed up memory and is a complete no-go.

How does this allow access to freed memory? Worst I can see is that the unmap happens earlier if the app/drivers gets the waits wrong, which wouldn't give access after the underlying BO is freed?

To free up memory we need to update the PTEs and then flush those out by invalidating the TLB.

On gfx6, gfx7 and gfx8 and some broken gfx10 hw invalidating the TLB can only be done while the VMID is idle.

Only gfx9 can reliable invalidate the TLB while it is in use and even there it comes with quite some performance penalty (at TLB invalidation can take multiple seconds).

Because of this what we do in the kernel driver is to sync to everything when we unmap entries:

     if (!(flags & AMDGPU_PTE_VALID))
             sync_mode = AMDGPU_SYNC_EQ_OWNER;
     else
             sync_mode = AMDGPU_SYNC_EXPLICIT;

This acts as a barrier for freeing the memory. In other words we intentionally add a bubble which syncs everything.

I'm working for month on a concept how to do all this without causing the stalls you observer, but so far didn't came to much of a conclusion.

That might cause an unmap operation too early, but for freeing up the actual backing memory we still wait for all fences on the BO to finish first, no? In that case, since BOOKKEEP fences are still added for explicit sync, that should not be a problem, no?

(If not, that sounds like the obvious fix for making this work?)

Regards, Christian.

...

...

Regards, Christian.

...

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c index f10332e1c6c0..31bc73fd1fae 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c @@ -51,7 +51,7 @@ static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, if (!resv) return 0;

• return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, sync_mode, p->vm, true);
  

• return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, AMDGPU_SYNC_EXPLICIT, p->vm, true);
  

  }

  /**

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index 63b484dc76c5..c8d5898bea11 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c @@ -75,7 +75,7 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p, if (!resv) return 0;

• return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, sync_mode, p->vm);
  

• return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, AMDGPU_SYNC_EXPLICIT, p->vm);
  

  }

  /**

Am 01.06.22 um 10:48 schrieb Bas Nieuwenhuizen:

On Wed, Jun 1, 2022 at 10:40 AM Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 10:16 schrieb Bas Nieuwenhuizen:

...

On Wed, Jun 1, 2022 at 10:03 AM Christian König christian.koenig@amd.com wrote:

...

Am 01.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

...

This should be okay because moves themselves use KERNEL usage and hence still sync with BOOKKEEP usage. Then any later submits still wait on any pending VM operations.

(i.e. we only made VM ops not wait on BOOKKEEP submits, not the other way around)

Well NAK again. This allows access to freed up memory and is a complete no-go.

How does this allow access to freed memory? Worst I can see is that the unmap happens earlier if the app/drivers gets the waits wrong, which wouldn't give access after the underlying BO is freed?

To free up memory we need to update the PTEs and then flush those out by invalidating the TLB.

On gfx6, gfx7 and gfx8 and some broken gfx10 hw invalidating the TLB can only be done while the VMID is idle.

Only gfx9 can reliable invalidate the TLB while it is in use and even there it comes with quite some performance penalty (at TLB invalidation can take multiple seconds).

Because of this what we do in the kernel driver is to sync to everything when we unmap entries:

      if (!(flags & AMDGPU_PTE_VALID))
              sync_mode = AMDGPU_SYNC_EQ_OWNER;
      else
              sync_mode = AMDGPU_SYNC_EXPLICIT;

This acts as a barrier for freeing the memory. In other words we intentionally add a bubble which syncs everything.

I'm working for month on a concept how to do all this without causing the stalls you observer, but so far didn't came to much of a conclusion.

That might cause an unmap operation too early, but for freeing up the actual backing memory we still wait for all fences on the BO to finish first, no? In that case, since BOOKKEEP fences are still added for explicit sync, that should not be a problem, no?

(If not, that sounds like the obvious fix for making this work?)

The problem is we need to wait on fences *not* added to the buffer object.

E.g. what we currently do here while freeing memory is: 1. Update the PTEs and make that update wait for everything! 2. Add the fence of that update to the freed up BO so that this BO isn't freed before the next CS.

We might be able to fix this by adding the fences to the BO before freeing it manually, but I'm not 100% sure we can actually allocate memory for the fences in that moment.

Regards, Christian.

...

Regards, Christian.

...

...

Regards, Christian.

...

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c index f10332e1c6c0..31bc73fd1fae 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c @@ -51,7 +51,7 @@ static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, if (!resv) return 0;

• return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, sync_mode, p->vm, true);
  

• return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, AMDGPU_SYNC_EXPLICIT, p->vm, true);
  

  }

  /**

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index 63b484dc76c5..c8d5898bea11 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c @@ -75,7 +75,7 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p, if (!resv) return 0;

• return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, sync_mode, p->vm);
  

• return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, AMDGPU_SYNC_EXPLICIT, p->vm);
  

  }

  /**

Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 12:16 PM Christian König christian.koenig@amd.com wrote:

...

Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:

...

[SNIP]

...

...

I do have to fix some stuff indeed, especially for the GEM close but with that we should be able to keep the same basic approach?

Nope, not even remotely.

What we need is the following:

1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but

also all buffers we potentially need to unmap. 3. Nuking the freed list in the amdgpu_vm structure by updating freed areas directly when they are unmapped. 4. Tracking those updates inside the bo_va structure for the BO+VM combination. 5. When the bo_va structure is destroy because of closing the handle move the last clear operation over to the VM as implicit sync.

Hi Christian, isn't that a different problem though (that we're also trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync) (t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS. (t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write

...

Only when all this is done we then can resolve the dependency that the CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?

No, it's the same.

See what we do in the VM code is to artificially insert a bubble so that all VM clear operations wait for all CS operations and then use the clear fence to indicate when the backing store of the BO can be freed.

Isn't that remediated with something like the code below? At least the gem_close case should be handled with this, and the move case was already handled by the copy operation.

That is one necessary puzzle piece, yes. But you need more than that.

Especially the explicit unmap operation needs to be converted into an implicit unmap to get the TLB flush right.

I think I know all the necessary steps now, it's just tons of work to do.

Regards, Christian.

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -187,6 +187,39 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj, return 0; }

+static void dma_resv_copy(struct dma_resv *src, struct dma_resv *dst) +{

•   struct dma_resv_iter cursor;
  

•   struct dma_fence *f;
  

•   int r;
  

•   unsigned num_fences = 0;
  

•   if (src == dst)
  

•           return;
  

•   /* We assume the later loops get the same fences as the caller should
  

•    * lock the resv. */
  

•   dma_resv_for_each_fence(&cursor, src, DMA_RESV_USAGE_BOOKKEEP, f) {
  

•           ++num_fences;
  

•           dma_fence_put(f);
  

•   }
  

•   r = dma_resv_reserve_fences(dst, num_fences);
  

•   if (r) {
  

•           /* As last resort on OOM we block for the fence */
  

•           dma_resv_for_each_fence(&cursor, src,
  

DMA_RESV_USAGE_BOOKKEEP, f) {

•                   dma_fence_wait(f, false);
  

•                   dma_fence_put(f);
  

•           }
  

•   }
  

•   dma_resv_for_each_fence(&cursor, src, DMA_RESV_USAGE_BOOKKEEP, f) {
  

•           dma_resv_add_fence(dst, f, dma_resv_iter_usage(&cursor));
  

•           dma_fence_put(f);
  

•   }
  

+}

static void amdgpu_gem_object_close(struct drm_gem_object *obj, struct drm_file *file_priv) { @@ -233,6 +266,8 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, amdgpu_bo_fence(bo, fence, true); dma_fence_put(fence);

•   dma_resv_copy(vm->root.bo->tbo.base.resv, bo->tbo.base.resv);
  

out_unlock: if (unlikely(r < 0)) dev_err(adev->dev, "failed to clear page "

...

When you want to remove this bubble (which is certainly a good idea) you need to first come up with a different approach to handle the clear operations.

Regards, Christian.

...

...

Regards, Christian.

Am 03.06.22 um 14:39 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 2:08 PM Christian König christian.koenig@amd.com wrote:

...

Am 03.06.22 um 13:07 schrieb Bas Nieuwenhuizen:

...

On Fri, Jun 3, 2022 at 12:16 PM Christian König christian.koenig@amd.com wrote:

...

Am 03.06.22 um 12:08 schrieb Bas Nieuwenhuizen:

...

[SNIP]

...

> I do have to fix some stuff indeed, especially for the GEM close but > with that we should be able to keep the same basic approach? Nope, not even remotely.

What we need is the following:

1. Rolling out my drm_exec patch set, so that we can lock buffers as needed.
2. When we get a VM operation we not only lock the VM page tables, but

also all buffers we potentially need to unmap. 3. Nuking the freed list in the amdgpu_vm structure by updating freed areas directly when they are unmapped. 4. Tracking those updates inside the bo_va structure for the BO+VM combination. 5. When the bo_va structure is destroy because of closing the handle move the last clear operation over to the VM as implicit sync.

Hi Christian, isn't that a different problem though (that we're also trying to solve, but in your series)?

What this patch tries to achieve:

(t+0) CS submission setting BOOKKEEP fences (i.e. no implicit sync) (t+1) a VM operation on a BO/VM accessed by the CS.

to run concurrently. What it *doesn't* try is

(t+0) a VM operation on a BO/VM accessed by the CS. (t+1) CS submission setting BOOKKEEP fences (i.e. no implicit sync)

to run concurrently. When you write

...

Only when all this is done we then can resolve the dependency that the CS currently must wait for any clear operation on the VM.

isn't that all about the second problem?

No, it's the same.

See what we do in the VM code is to artificially insert a bubble so that all VM clear operations wait for all CS operations and then use the clear fence to indicate when the backing store of the BO can be freed.

Isn't that remediated with something like the code below? At least the gem_close case should be handled with this, and the move case was already handled by the copy operation.

That is one necessary puzzle piece, yes. But you need more than that.

Especially the explicit unmap operation needs to be converted into an implicit unmap to get the TLB flush right.

This doesn't change anything about the TLB flush though? Since all unmap -> later jobs dependencies are still implicit.

So the worst what could happen (i.f. e.g. userspace gets the waits/dependencies wrong) is

1. non-implicit CS gets submitted that touches a BO
2. VM unmap on that BO happens

2.5) the CS from 1 is still active due to missing dependencies 2.6) but any CS submission after 2 will trigger a TLB flush

Yeah, but that's exactly the bubble we try to avoid. Isn't it?

When we want to do a TLB flush the unmap operation must already be completed. Otherwise the flush is rather pointless since any access could reloads the not yet updated PTEs.

And this means that we need to artificially add a dependency on every command submission after 2 to wait until the unmap operation is completed.

Christian.

3. A TLB flush happens for a new CS
4. All CS submissions here see the TLB flush and hence the unmap

So the main problem would be the CS from step 1, but (a) if that VMFaults that is the apps own fault and (b) because we don't free the memory until (1) finishes it is not a security issue kernel-wise.

...

I think I know all the necessary steps now, it's just tons of work to do.

Regards, Christian.

...

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -187,6 +187,39 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj, return 0; }

+static void dma_resv_copy(struct dma_resv *src, struct dma_resv *dst) +{

•   struct dma_resv_iter cursor;
  

•   struct dma_fence *f;
  

•   int r;
  

•   unsigned num_fences = 0;
  

•   if (src == dst)
  

•           return;
  

•   /* We assume the later loops get the same fences as the caller should
  

•    * lock the resv. */
  

•   dma_resv_for_each_fence(&cursor, src, DMA_RESV_USAGE_BOOKKEEP, f) {
  

•           ++num_fences;
  

•           dma_fence_put(f);
  

•   }
  

•   r = dma_resv_reserve_fences(dst, num_fences);
  

•   if (r) {
  

•           /* As last resort on OOM we block for the fence */
  

•           dma_resv_for_each_fence(&cursor, src,
  

DMA_RESV_USAGE_BOOKKEEP, f) {

•                   dma_fence_wait(f, false);
  

•                   dma_fence_put(f);
  

•           }
  

•   }
  

•   dma_resv_for_each_fence(&cursor, src, DMA_RESV_USAGE_BOOKKEEP, f) {
  

•           dma_resv_add_fence(dst, f, dma_resv_iter_usage(&cursor));
  

•           dma_fence_put(f);
  

•   }
  

+}

static void amdgpu_gem_object_close(struct drm_gem_object *obj, struct drm_file *file_priv) { @@ -233,6 +266,8 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, amdgpu_bo_fence(bo, fence, true); dma_fence_put(fence);

•   dma_resv_copy(vm->root.bo->tbo.base.resv, bo->tbo.base.resv);
  

out_unlock: if (unlikely(r < 0)) dev_err(adev->dev, "failed to clear page "

...

When you want to remove this bubble (which is certainly a good idea) you need to first come up with a different approach to handle the clear operations.

Regards, Christian.

...

...

Regards, Christian.