Re: [RFC PATCH 1/5] drm/ttm: Refactor num_shared into usage.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Am 01.06.22 um 10:41 schrieb Daniel Vetter:

On Wed, 1 Jun 2022 at 10:02, Christian König <christian.koenig@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
---
   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;
   };

   /**
[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux