* [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-04 13:12 Christian König 2018-10-04 13:12 ` [PATCH 4/8] drm/ttm: allow reserving more than one shared slot v2 Christian König ` (4 more replies) 0 siblings, 5 replies; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW No need for that any more. Just replace the list when there isn't enough room any more for the additional fence. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- include/linux/reservation.h | 4 - 2 files changed, 58 insertions(+), 124 deletions(-) diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); */ int reservation_object_reserve_shared(struct reservation_object *obj) { - struct reservation_object_list *fobj, *old; - u32 max; + struct reservation_object_list *old, *new; + unsigned int i, j, k, max; old = reservation_object_get_list(obj); if (old && old->shared_max) { - if (old->shared_count < old->shared_max) { - /* perform an in-place update */ - kfree(obj->staged); - obj->staged = NULL; + if (old->shared_count < old->shared_max) return 0; - } else + else max = old->shared_max * 2; - } else - max = 4; - - /* - * resize obj->staged or allocate if it doesn't exist, - * noop if already correct size - */ - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), - GFP_KERNEL); - if (!fobj) - return -ENOMEM; - - obj->staged = fobj; - fobj->shared_max = max; - return 0; -} -EXPORT_SYMBOL(reservation_object_reserve_shared); - -static void -reservation_object_add_shared_inplace(struct reservation_object *obj, - struct reservation_object_list *fobj, - struct dma_fence *fence) -{ - struct dma_fence *signaled = NULL; - u32 i, signaled_idx; - - dma_fence_get(fence); - - preempt_disable(); - write_seqcount_begin(&obj->seq); - - for (i = 0; i < fobj->shared_count; ++i) { - struct dma_fence *old_fence; - - old_fence = rcu_dereference_protected(fobj->shared[i], - reservation_object_held(obj)); - - if (old_fence->context == fence->context) { - /* memory barrier is added by write_seqcount_begin */ - RCU_INIT_POINTER(fobj->shared[i], fence); - write_seqcount_end(&obj->seq); - preempt_enable(); - - dma_fence_put(old_fence); - return; - } - - if (!signaled && dma_fence_is_signaled(old_fence)) { - signaled = old_fence; - signaled_idx = i; - } - } - - /* - * memory barrier is added by write_seqcount_begin, - * fobj->shared_count is protected by this lock too - */ - if (signaled) { - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); } else { - BUG_ON(fobj->shared_count >= fobj->shared_max); - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); - fobj->shared_count++; + max = 4; } - write_seqcount_end(&obj->seq); - preempt_enable(); - - dma_fence_put(signaled); -} - -static void -reservation_object_add_shared_replace(struct reservation_object *obj, - struct reservation_object_list *old, - struct reservation_object_list *fobj, - struct dma_fence *fence) -{ - unsigned i, j, k; - - dma_fence_get(fence); - - if (!old) { - RCU_INIT_POINTER(fobj->shared[0], fence); - fobj->shared_count = 1; - goto done; - } + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); + if (!new) + return -ENOMEM; /* * no need to bump fence refcounts, rcu_read access @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, * references from the old struct are carried over to * the new. */ - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { - struct dma_fence *check; + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { + struct dma_fence *fence; - check = rcu_dereference_protected(old->shared[i], - reservation_object_held(obj)); - - if (check->context == fence->context || - dma_fence_is_signaled(check)) - RCU_INIT_POINTER(fobj->shared[--k], check); + fence = rcu_dereference_protected(old->shared[i], + reservation_object_held(obj)); + if (dma_fence_is_signaled(fence)) + RCU_INIT_POINTER(new->shared[--k], fence); else - RCU_INIT_POINTER(fobj->shared[j++], check); + RCU_INIT_POINTER(new->shared[j++], fence); } - fobj->shared_count = j; - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); - fobj->shared_count++; + new->shared_count = j; + new->shared_max = max; -done: preempt_disable(); write_seqcount_begin(&obj->seq); /* * RCU_INIT_POINTER can be used here, * seqcount provides the necessary barriers */ - RCU_INIT_POINTER(obj->fence, fobj); + RCU_INIT_POINTER(obj->fence, new); write_seqcount_end(&obj->seq); preempt_enable(); if (!old) - return; + return 0; /* Drop the references to the signaled fences */ - for (i = k; i < fobj->shared_max; ++i) { - struct dma_fence *f; + for (i = k; i < new->shared_max; ++i) { + struct dma_fence *fence; - f = rcu_dereference_protected(fobj->shared[i], - reservation_object_held(obj)); - dma_fence_put(f); + fence = rcu_dereference_protected(new->shared[i], + reservation_object_held(obj)); + dma_fence_put(fence); } kfree_rcu(old, rcu); + + return 0; } +EXPORT_SYMBOL(reservation_object_reserve_shared); /** * reservation_object_add_shared_fence - Add a fence to a shared slot @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, void reservation_object_add_shared_fence(struct reservation_object *obj, struct dma_fence *fence) { - struct reservation_object_list *old, *fobj = obj->staged; + struct reservation_object_list *fobj; + unsigned int i; - old = reservation_object_get_list(obj); - obj->staged = NULL; + dma_fence_get(fence); + + fobj = reservation_object_get_list(obj); - if (!fobj) - reservation_object_add_shared_inplace(obj, old, fence); - else - reservation_object_add_shared_replace(obj, old, fobj, fence); + preempt_disable(); + write_seqcount_begin(&obj->seq); + + for (i = 0; i < fobj->shared_count; ++i) { + struct dma_fence *old_fence; + + old_fence = rcu_dereference_protected(fobj->shared[i], + reservation_object_held(obj)); + if (old_fence->context == fence->context || + dma_fence_is_signaled(old_fence)) { + dma_fence_put(old_fence); + goto replace; + } + } + + BUG_ON(fobj->shared_count >= fobj->shared_max); + fobj->shared_count++; + +replace: + /* + * memory barrier is added by write_seqcount_begin, + * fobj->shared_count is protected by this lock too + */ + RCU_INIT_POINTER(fobj->shared[i], fence); + write_seqcount_end(&obj->seq); + preempt_enable(); } EXPORT_SYMBOL(reservation_object_add_shared_fence); @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, new = dma_fence_get_rcu_safe(&src->fence_excl); rcu_read_unlock(); - kfree(dst->staged); - dst->staged = NULL; - src_list = reservation_object_get_list(dst); old = reservation_object_get_excl(dst); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 --- a/include/linux/reservation.h +++ b/include/linux/reservation.h @@ -68,7 +68,6 @@ struct reservation_object_list { * @seq: sequence count for managing RCU read-side synchronization * @fence_excl: the exclusive fence, if there is one currently * @fence: list of current shared fences - * @staged: staged copy of shared fences for RCU updates */ struct reservation_object { struct ww_mutex lock; @@ -76,7 +75,6 @@ struct reservation_object { struct dma_fence __rcu *fence_excl; struct reservation_object_list __rcu *fence; - struct reservation_object_list *staged; }; #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); RCU_INIT_POINTER(obj->fence, NULL); RCU_INIT_POINTER(obj->fence_excl, NULL); - obj->staged = NULL; } /** @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) kfree(fobj); } - kfree(obj->staged); ww_mutex_destroy(&obj->lock); } -- 2.14.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 4/8] drm/ttm: allow reserving more than one shared slot v2 2018-10-04 13:12 [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Christian König @ 2018-10-04 13:12 ` Christian König [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> ` (3 subsequent siblings) 4 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx, dri-devel Let's support simultaneous submissions to multiple engines. v2: rename the field to num_shared and fix up all users Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +++++----- drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +- drivers/gpu/drm/qxl/qxl_release.c | 2 +- drivers/gpu/drm/radeon/radeon_cs.c | 4 ++-- drivers/gpu/drm/radeon/radeon_gem.c | 2 +- drivers/gpu/drm/radeon/radeon_vm.c | 4 ++-- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 12 +++++++----- drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 10 +++++----- include/drm/ttm/ttm_execbuf_util.h | 4 ++-- 14 files changed, 37 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index df0a059565f9..d0bc0312430c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -536,7 +536,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->shared = true; + entry->num_shared = 1; entry->bo = &bo->tbo; mutex_lock(&process_info->lock); if (userptr) @@ -677,7 +677,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.shared = true; + ctx->kfd_bo.tv.num_shared = 1; ctx->kfd_bo.user_pages = NULL; list_add(&ctx->kfd_bo.tv.head, &ctx->list); @@ -741,7 +741,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.shared = true; + ctx->kfd_bo.tv.num_shared = 1; ctx->kfd_bo.user_pages = NULL; list_add(&ctx->kfd_bo.tv.head, &ctx->list); @@ -1808,7 +1808,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.shared = mem->validate_list.shared; + mem->resv_list.num_shared = mem->validate_list.num_shared; } /* Reserve all BOs and page tables for validation */ @@ -2027,7 +2027,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.shared = mem->validate_list.shared; + mem->resv_list.num_shared = mem->validate_list.num_shared; } ret = ttm_eu_reserve_buffers(&ctx.ticket, &ctx.list, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c index 14d2982a47cc..b75d30ee80c6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c @@ -118,7 +118,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp, entry->priority = min(info[i].bo_priority, AMDGPU_BO_LIST_MAX_PRIORITY); entry->tv.bo = &bo->tbo; - entry->tv.shared = !bo->prime_shared_count; + entry->tv.num_shared = !bo->prime_shared_count; if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS) list->gds_obj = bo; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 3243da67db9e..4476398e5b26 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -50,7 +50,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; - p->uf_entry.tv.shared = true; + p->uf_entry.tv.num_shared = 1; p->uf_entry.user_pages = NULL; drm_gem_object_put_unlocked(gobj); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 7b3d1ebda9df..f4f00217546e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -169,7 +169,7 @@ void amdgpu_gem_object_close(struct drm_gem_object *obj, INIT_LIST_HEAD(&duplicates); tv.bo = &bo->tbo; - tv.shared = true; + tv.num_shared = 1; list_add(&tv.head, &list); amdgpu_vm_get_pd_bo(vm, &list, &vm_pd); @@ -604,7 +604,10 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, return -ENOENT; abo = gem_to_amdgpu_bo(gobj); tv.bo = &abo->tbo; - tv.shared = !!(abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID); + if (abo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) + tv.num_shared = 1; + else + tv.num_shared = 0; list_add(&tv.head, &list); } else { gobj = NULL; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c index f2f358aa0597..0ac875de0368 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c @@ -81,7 +81,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 = &adev->virt.csa_obj->tbo; - csa_tv.shared = true; + csa_tv.num_shared = 1; list_add(&csa_tv.head, &list); amdgpu_vm_get_pd_bo(vm, &list, &pd); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index bdce05183edb..218527bb0156 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -616,7 +616,7 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.base.bo->tbo; - entry->tv.shared = true; + entry->tv.num_shared = 1; entry->user_pages = NULL; list_add(&entry->tv.head, validated); } diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index a8d5457a1af9..b6ffbb857432 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -217,7 +217,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.shared = false; + entry->tv.num_shared = 0; 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 1ae31dbc61c6..f43305329939 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -178,7 +178,7 @@ 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.shared = !r->write_domain; + p->relocs[i].tv.num_shared = !r->write_domain; radeon_cs_buckets_add(&buckets, &p->relocs[i].tv.head, priority); @@ -253,7 +253,7 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p) resv = reloc->robj->tbo.resv; r = radeon_sync_resv(p->rdev, &p->ib.sync, resv, - reloc->tv.shared); + reloc->tv.num_shared); if (r) return r; } diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 27d8e7dd2d06..44617dec8183 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -552,7 +552,7 @@ static void radeon_gem_va_update_vm(struct radeon_device *rdev, INIT_LIST_HEAD(&list); tv.bo = &bo_va->bo->tbo; - tv.shared = true; + tv.num_shared = 1; 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 fed11ece0de6..31cc3ec97815 100644 --- a/drivers/gpu/drm/radeon/radeon_vm.c +++ b/drivers/gpu/drm/radeon/radeon_vm.c @@ -142,7 +142,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.shared = true; + list[0].tv.num_shared = 1; list[0].tiling_flags = 0; list_add(&list[0].tv.head, head); @@ -154,7 +154,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.shared = true; + list[idx].tv.num_shared = 1; 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 e493edb0d3e7..6a847c198b31 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -126,10 +126,11 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, } if (!ret) { - if (!entry->shared) + if (!entry->num_shared) continue; - ret = reservation_object_reserve_shared(bo->resv, 1); + ret = reservation_object_reserve_shared(bo->resv, + entry->num_shared); if (!ret) continue; } @@ -150,8 +151,9 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, } } - if (!ret && entry->shared) - ret = reservation_object_reserve_shared(bo->resv, 1); + if (!ret && entry->num_shared) + ret = reservation_object_reserve_shared(bo->resv, + entry->num_shared); if (unlikely(ret != 0)) { if (ret == -EINTR) @@ -201,7 +203,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { bo = entry->bo; - if (entry->shared) + if (entry->num_shared) reservation_object_add_shared_fence(bo->resv, fence); else reservation_object_add_excl_fence(bo->resv, fence); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c index 1f134570b759..5a76ba2fceed 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c @@ -582,7 +582,7 @@ static int vmw_bo_to_validate_list(struct vmw_sw_context *sw_context, ++sw_context->cur_val_buf; val_buf = &vval_buf->base; val_buf->bo = ttm_bo_reference(&vbo->base); - val_buf->shared = false; + val_buf->num_shared = 0; list_add_tail(&val_buf->head, &sw_context->validate_nodes); vval_buf->validate_as_mob = validate_as_mob; } @@ -4409,11 +4409,11 @@ void __vmw_execbuf_release_pinned_bo(struct vmw_private *dev_priv, INIT_LIST_HEAD(&validate_list); pinned_val.bo = ttm_bo_reference(&dev_priv->pinned_bo->base); - pinned_val.shared = false; + pinned_val.num_shared = 0; list_add_tail(&pinned_val.head, &validate_list); query_val.bo = ttm_bo_reference(&dev_priv->dummy_query_bo->base); - query_val.shared = false; + query_val.num_shared = 0; list_add_tail(&query_val.head, &validate_list); ret = ttm_eu_reserve_buffers(&ticket, &validate_list, diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index 92003ea5a219..d9109fd0123f 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -86,7 +86,7 @@ static void vmw_resource_release(struct kref *kref) struct ttm_validate_buffer val_buf; val_buf.bo = bo; - val_buf.shared = false; + val_buf.num_shared = 0; res->func->unbind(res, false, &val_buf); } res->backup_dirty = false; @@ -459,7 +459,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket, INIT_LIST_HEAD(&val_list); val_buf->bo = ttm_bo_reference(&res->backup->base); - val_buf->shared = false; + val_buf->num_shared = 0; list_add_tail(&val_buf->head, &val_list); ret = ttm_eu_reserve_buffers(ticket, &val_list, interruptible, NULL); if (unlikely(ret != 0)) @@ -562,7 +562,7 @@ static int vmw_resource_do_evict(struct ww_acquire_ctx *ticket, BUG_ON(!func->may_evict); val_buf.bo = NULL; - val_buf.shared = false; + val_buf.num_shared = 0; ret = vmw_resource_check_buffer(ticket, res, interruptible, &val_buf); if (unlikely(ret != 0)) return ret; @@ -608,7 +608,7 @@ int vmw_resource_validate(struct vmw_resource *res) return 0; val_buf.bo = NULL; - val_buf.shared = false; + val_buf.num_shared = 0; if (res->backup) val_buf.bo = &res->backup->base; do { @@ -679,7 +679,7 @@ void vmw_resource_unbind_list(struct vmw_buffer_object *vbo) struct vmw_resource *res, *next; struct ttm_validate_buffer val_buf = { .bo = &vbo->base, - .shared = false + .num_shared = 0 }; lockdep_assert_held(&vbo->base.resv->lock.base); diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index b0fdd1980034..621615fa7728 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -40,13 +40,13 @@ * * @head: list head for thread-private list. * @bo: refcounted buffer object pointer. - * @shared: should the fence be added shared? + * @num_shared: How many shared fences we want to add. */ struct ttm_validate_buffer { struct list_head head; struct ttm_buffer_object *bo; - bool shared; + unsigned int num_shared; }; /** -- 2.14.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org>]
* [PATCH 2/8] dma-buf: allow reserving more than one shared fence slot [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> @ 2018-10-04 13:12 ` Christian König 2018-10-04 13:12 ` [PATCH 3/8] dma-buf: test shared slot allocation when mutex debugging is active Christian König ` (4 subsequent siblings) 5 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Let's support simultaneous submissions to multiple engines. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/reservation.c | 13 ++++++++----- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++-- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 2 +- drivers/gpu/drm/msm/msm_gem_submit.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- drivers/gpu/drm/qxl/qxl_release.c | 2 +- drivers/gpu/drm/radeon/radeon_vm.c | 2 +- drivers/gpu/drm/ttm/ttm_bo.c | 4 ++-- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 4 ++-- drivers/gpu/drm/v3d/v3d_gem.c | 2 +- drivers/gpu/drm/vc4/vc4_gem.c | 2 +- drivers/gpu/drm/vgem/vgem_fence.c | 2 +- include/linux/reservation.h | 3 ++- 16 files changed, 28 insertions(+), 23 deletions(-) diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 5825fc336a13..5fb4fd461908 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -56,9 +56,10 @@ const char reservation_seqcount_string[] = "reservation_seqcount"; EXPORT_SYMBOL(reservation_seqcount_string); /** - * reservation_object_reserve_shared - Reserve space to add a shared - * fence to a reservation_object. + * reservation_object_reserve_shared - Reserve space to add shared fences to + * a reservation_object. * @obj: reservation object + * @num_fences: number of fences we want to add * * Should be called before reservation_object_add_shared_fence(). Must * be called with obj->lock held. @@ -66,7 +67,8 @@ EXPORT_SYMBOL(reservation_seqcount_string); * RETURNS * Zero for success, or -errno */ -int reservation_object_reserve_shared(struct reservation_object *obj) +int reservation_object_reserve_shared(struct reservation_object *obj, + unsigned int num_fences) { struct reservation_object_list *old, *new; unsigned int i, j, k, max; @@ -74,10 +76,11 @@ int reservation_object_reserve_shared(struct reservation_object *obj) old = reservation_object_get_list(obj); if (old && old->shared_max) { - if (old->shared_count < old->shared_max) + if ((old->shared_count + num_fences) <= old->shared_max) return 0; else - max = old->shared_max * 2; + max = max(old->shared_count + num_fences, + old->shared_max * 2); } else { max = 4; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 8836186eb5ef..3243da67db9e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -955,7 +955,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p) if (r) return r; - r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv); + r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1); if (r) return r; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 904014dc5915..cf768acb51dc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -640,7 +640,7 @@ int amdgpu_bo_backup_to_shadow(struct amdgpu_device *adev, bo_addr = amdgpu_bo_gpu_offset(bo); shadow_addr = amdgpu_bo_gpu_offset(bo->shadow); - r = reservation_object_reserve_shared(bo->tbo.resv); + r = reservation_object_reserve_shared(bo->tbo.resv, 1); if (r) goto err; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 6904d794d60a..bdce05183edb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -772,7 +772,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev, ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched); - r = reservation_object_reserve_shared(bo->tbo.resv); + r = reservation_object_reserve_shared(bo->tbo.resv, 1); if (r) return r; @@ -1839,7 +1839,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev, if (r) goto error_free; - r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv); + r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1); if (r) goto error_free; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 983e67f19e45..30875f8f2933 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -179,7 +179,7 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) struct reservation_object *robj = bo->obj->resv; if (!(bo->flags & ETNA_SUBMIT_BO_WRITE)) { - ret = reservation_object_reserve_shared(robj); + ret = reservation_object_reserve_shared(robj, 1); if (ret) return ret; } diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 11d834f94220..f74c856858f9 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -893,7 +893,7 @@ static void export_fence(struct i915_vma *vma, reservation_object_lock(resv, NULL); if (flags & EXEC_OBJECT_WRITE) reservation_object_add_excl_fence(resv, &rq->fence); - else if (reservation_object_reserve_shared(resv) == 0) + else if (reservation_object_reserve_shared(resv, 1) == 0) reservation_object_add_shared_fence(resv, &rq->fence); reservation_object_unlock(resv); } diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 7bd83e0afa97..acc5da791e36 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -241,7 +241,8 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) * strange place to call it. OTOH this is a * convenient can-fail point to hook it in. */ - ret = reservation_object_reserve_shared(msm_obj->resv); + ret = reservation_object_reserve_shared(msm_obj->resv, + 1); if (ret) return ret; } diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 412d49bc6e56..bd58298afe12 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -341,7 +341,7 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e int ret = 0, i; if (!exclusive) { - ret = reservation_object_reserve_shared(resv); + ret = reservation_object_reserve_shared(resv, 1); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index e37f0097f744..a8d5457a1af9 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -234,7 +234,7 @@ static int qxl_release_validate_bo(struct qxl_bo *bo) return ret; } - ret = reservation_object_reserve_shared(bo->tbo.resv); + ret = reservation_object_reserve_shared(bo->tbo.resv, 1); if (ret) return ret; diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c index 7f1a9c787bd1..fed11ece0de6 100644 --- a/drivers/gpu/drm/radeon/radeon_vm.c +++ b/drivers/gpu/drm/radeon/radeon_vm.c @@ -831,7 +831,7 @@ static int radeon_vm_update_ptes(struct radeon_device *rdev, int r; radeon_sync_resv(rdev, &ib->sync, pt->tbo.resv, true); - r = reservation_object_reserve_shared(pt->tbo.resv); + r = reservation_object_reserve_shared(pt->tbo.resv, 1); if (r) return r; diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 26b889f86670..83b4657ffb10 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -872,7 +872,7 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo, if (fence) { reservation_object_add_shared_fence(bo->resv, fence); - ret = reservation_object_reserve_shared(bo->resv); + ret = reservation_object_reserve_shared(bo->resv, 1); if (unlikely(ret)) return ret; @@ -977,7 +977,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, bool has_erestartsys = false; int i, ret; - ret = reservation_object_reserve_shared(bo->resv); + ret = reservation_object_reserve_shared(bo->resv, 1); if (unlikely(ret)) return ret; diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index e73ae0d22897..e493edb0d3e7 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -129,7 +129,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, if (!entry->shared) continue; - ret = reservation_object_reserve_shared(bo->resv); + ret = reservation_object_reserve_shared(bo->resv, 1); if (!ret) continue; } @@ -151,7 +151,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, } if (!ret && entry->shared) - ret = reservation_object_reserve_shared(bo->resv); + ret = reservation_object_reserve_shared(bo->resv, 1); if (unlikely(ret != 0)) { if (ret == -EINTR) diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 5ce24098a5fd..4a46376ff795 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -305,7 +305,7 @@ v3d_lock_bo_reservations(struct drm_device *dev, for (i = 0; i < exec->bo_count; i++) { bo = to_v3d_bo(&exec->bo[i]->base); - ret = reservation_object_reserve_shared(bo->resv); + ret = reservation_object_reserve_shared(bo->resv, 1); if (ret) { v3d_unlock_bo_reservations(dev, exec, acquire_ctx); return ret; diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c index 7910b9acedd6..5f768d886cb2 100644 --- a/drivers/gpu/drm/vc4/vc4_gem.c +++ b/drivers/gpu/drm/vc4/vc4_gem.c @@ -635,7 +635,7 @@ vc4_lock_bo_reservations(struct drm_device *dev, for (i = 0; i < exec->bo_count; i++) { bo = to_vc4_bo(&exec->bo[i]->base); - ret = reservation_object_reserve_shared(bo->resv); + ret = reservation_object_reserve_shared(bo->resv, 1); if (ret) { vc4_unlock_bo_reservations(dev, exec, acquire_ctx); return ret; diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c index b28876c222b4..f2692e5abac2 100644 --- a/drivers/gpu/drm/vgem/vgem_fence.c +++ b/drivers/gpu/drm/vgem/vgem_fence.c @@ -193,7 +193,7 @@ int vgem_fence_attach_ioctl(struct drm_device *dev, reservation_object_lock(resv, NULL); if (arg->flags & VGEM_FENCE_WRITE) reservation_object_add_excl_fence(resv, fence); - else if ((ret = reservation_object_reserve_shared(resv)) == 0) + else if ((ret = reservation_object_reserve_shared(resv, 1)) == 0) reservation_object_add_shared_fence(resv, fence); reservation_object_unlock(resv); diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 54cf6773a14c..5ddb0e143721 100644 --- a/include/linux/reservation.h +++ b/include/linux/reservation.h @@ -261,7 +261,8 @@ reservation_object_get_excl_rcu(struct reservation_object *obj) return fence; } -int reservation_object_reserve_shared(struct reservation_object *obj); +int reservation_object_reserve_shared(struct reservation_object *obj, + unsigned int num_fences); void reservation_object_add_shared_fence(struct reservation_object *obj, struct dma_fence *fence); -- 2.14.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/8] dma-buf: test shared slot allocation when mutex debugging is active [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-04 13:12 ` [PATCH 2/8] dma-buf: allow reserving more than one shared fence slot Christian König @ 2018-10-04 13:12 ` Christian König 2018-10-04 13:12 ` [PATCH 5/8] drm/amdgpu: fix using shared fence for exported BOs v2 Christian König ` (3 subsequent siblings) 5 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Set shared_max to the number of shared fences right before we release the lock. This way every attempt to add a shared fence without previously reserving a slot will cause an error. Signed-off-by: Christian König <christian.koenig@amd.com> --- include/linux/reservation.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/linux/reservation.h b/include/linux/reservation.h index 5ddb0e143721..2f0ffca35780 100644 --- a/include/linux/reservation.h +++ b/include/linux/reservation.h @@ -214,6 +214,11 @@ reservation_object_trylock(struct reservation_object *obj) static inline void reservation_object_unlock(struct reservation_object *obj) { +#ifdef CONFIG_DEBUG_MUTEXES + /* Test shared fence slot reservation */ + if (obj->fence) + obj->fence->shared_max = obj->fence->shared_count; +#endif ww_mutex_unlock(&obj->lock); } -- 2.14.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 5/8] drm/amdgpu: fix using shared fence for exported BOs v2 [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-04 13:12 ` [PATCH 2/8] dma-buf: allow reserving more than one shared fence slot Christian König 2018-10-04 13:12 ` [PATCH 3/8] dma-buf: test shared slot allocation when mutex debugging is active Christian König @ 2018-10-04 13:12 ` Christian König [not found] ` <20181004131250.2373-5-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-04 13:12 ` [PATCH 6/8] drm/amdgpu: always reserve two slots for the VM Christian König ` (2 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW It is perfectly possible that the BO list is created before the BO is exported. While at it cleanup setting shared to one instead of true. v2: add comment and simplify logic Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 13 +++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c index b75d30ee80c6..5c79da8e1150 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c @@ -118,7 +118,6 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp, entry->priority = min(info[i].bo_priority, AMDGPU_BO_LIST_MAX_PRIORITY); entry->tv.bo = &bo->tbo; - entry->tv.num_shared = !bo->prime_shared_count; if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS) list->gds_obj = bo; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 4476398e5b26..b8de56d1a866 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -598,6 +598,9 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, return r; } + amdgpu_bo_list_for_each_entry(e, p->bo_list) + e->tv.num_shared = 1; + amdgpu_bo_list_get_list(p->bo_list, &p->validated); if (p->bo_list->first_userptr != p->bo_list->num_entries) p->mn = amdgpu_mn_get(p->adev, AMDGPU_MN_TYPE_GFX); @@ -717,8 +720,14 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, gws = p->bo_list->gws_obj; oa = p->bo_list->oa_obj; - amdgpu_bo_list_for_each_entry(e, p->bo_list) - e->bo_va = amdgpu_vm_bo_find(vm, ttm_to_amdgpu_bo(e->tv.bo)); + amdgpu_bo_list_for_each_entry(e, p->bo_list) { + struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo); + + /* Make sure we use the exclusive slot for shared BOs */ + if (bo->prime_shared_count) + e->tv.num_shared = 0; + e->bo_va = amdgpu_vm_bo_find(vm, bo); + } if (gds) { p->job->gds_base = amdgpu_bo_gpu_offset(gds) >> PAGE_SHIFT; -- 2.14.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <20181004131250.2373-5-christian.koenig-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH 5/8] drm/amdgpu: fix using shared fence for exported BOs v2 [not found] ` <20181004131250.2373-5-christian.koenig-5C7GfCeVMHo@public.gmane.org> @ 2018-10-23 13:40 ` Michel Dänzer 0 siblings, 0 replies; 25+ messages in thread From: Michel Dänzer @ 2018-10-23 13:40 UTC (permalink / raw) To: Christian König Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 2018-10-04 3:12 p.m., Christian König wrote: > It is perfectly possible that the BO list is created before the BO is > exported. While at it cleanup setting shared to one instead of true. "clean up" > v2: add comment and simplify logic > > Signed-off-by: Christian König <christian.koenig@amd.com> As this is a bug fix, should it be before patch 5 and have Cc: stable? Either way, with the above fixed, Reviewed-by: Michel Dänzer <michel.daenzer@amd.com> -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 6/8] drm/amdgpu: always reserve two slots for the VM [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> ` (2 preceding siblings ...) 2018-10-04 13:12 ` [PATCH 5/8] drm/amdgpu: fix using shared fence for exported BOs v2 Christian König @ 2018-10-04 13:12 ` Christian König [not found] ` <20181004131250.2373-6-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-04 13:12 ` [PATCH 8/8] drm/ttm: drop the extra reservation for pipelined BO moves Christian König 2018-10-24 5:25 ` [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Zhang, Jerry(Junwei) 5 siblings, 1 reply; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW And drop the now superflous extra reservations. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ---- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 ++++++--------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index b8de56d1a866..ba406bd1b08f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -964,10 +964,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p) if (r) return r; - r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1); - if (r) - return r; - p->job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.base.bo); if (amdgpu_vm_debug) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 218527bb0156..1b39b0144698 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -616,7 +616,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.base.bo->tbo; - entry->tv.num_shared = 1; + /* One for the VM updates and one for the CS job */ + entry->tv.num_shared = 2; entry->user_pages = NULL; list_add(&entry->tv.head, validated); } @@ -772,10 +773,6 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev, ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched); - r = reservation_object_reserve_shared(bo->tbo.resv, 1); - if (r) - return r; - r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); if (r) goto error; @@ -1839,10 +1836,6 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev, if (r) goto error_free; - r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1); - if (r) - goto error_free; - r = amdgpu_vm_update_ptes(¶ms, start, last + 1, addr, flags); if (r) goto error_free; @@ -3023,6 +3016,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (r) goto error_free_root; + r = reservation_object_reserve_shared(root->tbo.resv, 1); + if (r) + goto error_unreserve; + r = amdgpu_vm_clear_bo(adev, vm, root, adev->vm_manager.root_level, vm->pte_support_ats); -- 2.14.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
[parent not found: <20181004131250.2373-6-christian.koenig-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH 6/8] drm/amdgpu: always reserve two slots for the VM [not found] ` <20181004131250.2373-6-christian.koenig-5C7GfCeVMHo@public.gmane.org> @ 2018-10-24 5:27 ` Zhang, Jerry(Junwei) 0 siblings, 0 replies; 25+ messages in thread From: Zhang, Jerry(Junwei) @ 2018-10-24 5:27 UTC (permalink / raw) To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 10/4/18 9:12 PM, Christian König wrote: > And drop the now superflous extra reservations. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ---- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 ++++++--------- > 2 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index b8de56d1a866..ba406bd1b08f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -964,10 +964,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p) > if (r) > return r; > > - r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1); > - if (r) > - return r; > - > p->job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.base.bo); > > if (amdgpu_vm_debug) { > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 218527bb0156..1b39b0144698 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -616,7 +616,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, > { > entry->priority = 0; > entry->tv.bo = &vm->root.base.bo->tbo; > - entry->tv.num_shared = 1; > + /* One for the VM updates and one for the CS job */ > + entry->tv.num_shared = 2; > entry->user_pages = NULL; > list_add(&entry->tv.head, validated); > } > @@ -772,10 +773,6 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev, > > ring = container_of(vm->entity.rq->sched, struct amdgpu_ring, sched); > > - r = reservation_object_reserve_shared(bo->tbo.resv, 1); > - if (r) > - return r; > - A trivial thing, this change may belong to next patch. this patch looks dropping the resv for root bo. Regards, Jerry > r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > if (r) > goto error; > @@ -1839,10 +1836,6 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev, > if (r) > goto error_free; > > - r = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1); > - if (r) > - goto error_free; > - > r = amdgpu_vm_update_ptes(¶ms, start, last + 1, addr, flags); > if (r) > goto error_free; > @@ -3023,6 +3016,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, > if (r) > goto error_free_root; > > + r = reservation_object_reserve_shared(root->tbo.resv, 1); > + if (r) > + goto error_unreserve; > + > r = amdgpu_vm_clear_bo(adev, vm, root, > adev->vm_manager.root_level, > vm->pte_support_ats); _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 8/8] drm/ttm: drop the extra reservation for pipelined BO moves [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> ` (3 preceding siblings ...) 2018-10-04 13:12 ` [PATCH 6/8] drm/amdgpu: always reserve two slots for the VM Christian König @ 2018-10-04 13:12 ` Christian König 2018-10-24 5:25 ` [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Zhang, Jerry(Junwei) 5 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW The driver is now responsible to allocate enough shared slots. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 83b4657ffb10..7bdfc1e8236d 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -858,12 +858,11 @@ EXPORT_SYMBOL(ttm_bo_mem_put); /** * Add the last move fence to the BO and reserve a new shared slot. */ -static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo, - struct ttm_mem_type_manager *man, - struct ttm_mem_reg *mem) +static void ttm_bo_add_move_fence(struct ttm_buffer_object *bo, + struct ttm_mem_type_manager *man, + struct ttm_mem_reg *mem) { struct dma_fence *fence; - int ret; spin_lock(&man->move_lock); fence = dma_fence_get(man->move); @@ -871,16 +870,9 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo, if (fence) { reservation_object_add_shared_fence(bo->resv, fence); - - ret = reservation_object_reserve_shared(bo->resv, 1); - if (unlikely(ret)) - return ret; - dma_fence_put(bo->moving); bo->moving = fence; } - - return 0; } /** @@ -908,7 +900,8 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo, return ret; } while (1); mem->mem_type = mem_type; - return ttm_bo_add_move_fence(bo, man, mem); + ttm_bo_add_move_fence(bo, man, mem); + return 0; } static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man, @@ -977,10 +970,6 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, bool has_erestartsys = false; int i, ret; - ret = reservation_object_reserve_shared(bo->resv, 1); - if (unlikely(ret)) - return ret; - mem->mm_node = NULL; for (i = 0; i < placement->num_placement; ++i) { const struct ttm_place *place = &placement->placement[i]; @@ -1016,11 +1005,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, return ret; if (mem->mm_node) { - ret = ttm_bo_add_move_fence(bo, man, mem); - if (unlikely(ret)) { - (*man->func->put_node)(man, mem); - return ret; - } + ttm_bo_add_move_fence(bo, man, mem); break; } } -- 2.14.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> ` (4 preceding siblings ...) 2018-10-04 13:12 ` [PATCH 8/8] drm/ttm: drop the extra reservation for pipelined BO moves Christian König @ 2018-10-24 5:25 ` Zhang, Jerry(Junwei) 5 siblings, 0 replies; 25+ messages in thread From: Zhang, Jerry(Junwei) @ 2018-10-24 5:25 UTC (permalink / raw) To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Patch 3, 5 is Acked-by: Junwei Zhang <Jerry.Zhang@amd.com> Others are Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com> On 10/4/18 9:12 PM, Christian König wrote: > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- > include/linux/reservation.h | 4 - > 2 files changed, 58 insertions(+), 124 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 6c95f61a32e7..5825fc336a13 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > */ > int reservation_object_reserve_shared(struct reservation_object *obj) > { > - struct reservation_object_list *fobj, *old; > - u32 max; > + struct reservation_object_list *old, *new; > + unsigned int i, j, k, max; > > old = reservation_object_get_list(obj); > > if (old && old->shared_max) { > - if (old->shared_count < old->shared_max) { > - /* perform an in-place update */ > - kfree(obj->staged); > - obj->staged = NULL; > + if (old->shared_count < old->shared_max) > return 0; > - } else > + else > max = old->shared_max * 2; > - } else > - max = 4; > - > - /* > - * resize obj->staged or allocate if it doesn't exist, > - * noop if already correct size > - */ > - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), > - GFP_KERNEL); > - if (!fobj) > - return -ENOMEM; > - > - obj->staged = fobj; > - fobj->shared_max = max; > - return 0; > -} > -EXPORT_SYMBOL(reservation_object_reserve_shared); > - > -static void > -reservation_object_add_shared_inplace(struct reservation_object *obj, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - struct dma_fence *signaled = NULL; > - u32 i, signaled_idx; > - > - dma_fence_get(fence); > - > - preempt_disable(); > - write_seqcount_begin(&obj->seq); > - > - for (i = 0; i < fobj->shared_count; ++i) { > - struct dma_fence *old_fence; > - > - old_fence = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - > - if (old_fence->context == fence->context) { > - /* memory barrier is added by write_seqcount_begin */ > - RCU_INIT_POINTER(fobj->shared[i], fence); > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(old_fence); > - return; > - } > - > - if (!signaled && dma_fence_is_signaled(old_fence)) { > - signaled = old_fence; > - signaled_idx = i; > - } > - } > - > - /* > - * memory barrier is added by write_seqcount_begin, > - * fobj->shared_count is protected by this lock too > - */ > - if (signaled) { > - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > - BUG_ON(fobj->shared_count >= fobj->shared_max); > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + max = 4; > } > > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(signaled); > -} > - > -static void > -reservation_object_add_shared_replace(struct reservation_object *obj, > - struct reservation_object_list *old, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - unsigned i, j, k; > - > - dma_fence_get(fence); > - > - if (!old) { > - RCU_INIT_POINTER(fobj->shared[0], fence); > - fobj->shared_count = 1; > - goto done; > - } > + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > + if (!new) > + return -ENOMEM; > > /* > * no need to bump fence refcounts, rcu_read access > @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > * references from the old struct are carried over to > * the new. > */ > - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { > - struct dma_fence *check; > + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { > + struct dma_fence *fence; > > - check = rcu_dereference_protected(old->shared[i], > - reservation_object_held(obj)); > - > - if (check->context == fence->context || > - dma_fence_is_signaled(check)) > - RCU_INIT_POINTER(fobj->shared[--k], check); > + fence = rcu_dereference_protected(old->shared[i], > + reservation_object_held(obj)); > + if (dma_fence_is_signaled(fence)) > + RCU_INIT_POINTER(new->shared[--k], fence); > else > - RCU_INIT_POINTER(fobj->shared[j++], check); > + RCU_INIT_POINTER(new->shared[j++], fence); > } > - fobj->shared_count = j; > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + new->shared_count = j; > + new->shared_max = max; > > -done: > preempt_disable(); > write_seqcount_begin(&obj->seq); > /* > * RCU_INIT_POINTER can be used here, > * seqcount provides the necessary barriers > */ > - RCU_INIT_POINTER(obj->fence, fobj); > + RCU_INIT_POINTER(obj->fence, new); > write_seqcount_end(&obj->seq); > preempt_enable(); > > if (!old) > - return; > + return 0; > > /* Drop the references to the signaled fences */ > - for (i = k; i < fobj->shared_max; ++i) { > - struct dma_fence *f; > + for (i = k; i < new->shared_max; ++i) { > + struct dma_fence *fence; > > - f = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - dma_fence_put(f); > + fence = rcu_dereference_protected(new->shared[i], > + reservation_object_held(obj)); > + dma_fence_put(fence); > } > kfree_rcu(old, rcu); > + > + return 0; > } > +EXPORT_SYMBOL(reservation_object_reserve_shared); > > /** > * reservation_object_add_shared_fence - Add a fence to a shared slot > @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > void reservation_object_add_shared_fence(struct reservation_object *obj, > struct dma_fence *fence) > { > - struct reservation_object_list *old, *fobj = obj->staged; > + struct reservation_object_list *fobj; > + unsigned int i; > > - old = reservation_object_get_list(obj); > - obj->staged = NULL; > + dma_fence_get(fence); > + > + fobj = reservation_object_get_list(obj); > > - if (!fobj) > - reservation_object_add_shared_inplace(obj, old, fence); > - else > - reservation_object_add_shared_replace(obj, old, fobj, fence); > + preempt_disable(); > + write_seqcount_begin(&obj->seq); > + > + for (i = 0; i < fobj->shared_count; ++i) { > + struct dma_fence *old_fence; > + > + old_fence = rcu_dereference_protected(fobj->shared[i], > + reservation_object_held(obj)); > + if (old_fence->context == fence->context || > + dma_fence_is_signaled(old_fence)) { > + dma_fence_put(old_fence); > + goto replace; > + } > + } > + > + BUG_ON(fobj->shared_count >= fobj->shared_max); > + fobj->shared_count++; > + > +replace: > + /* > + * memory barrier is added by write_seqcount_begin, > + * fobj->shared_count is protected by this lock too > + */ > + RCU_INIT_POINTER(fobj->shared[i], fence); > + write_seqcount_end(&obj->seq); > + preempt_enable(); > } > EXPORT_SYMBOL(reservation_object_add_shared_fence); > > @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, > new = dma_fence_get_rcu_safe(&src->fence_excl); > rcu_read_unlock(); > > - kfree(dst->staged); > - dst->staged = NULL; > - > src_list = reservation_object_get_list(dst); > old = reservation_object_get_excl(dst); > > diff --git a/include/linux/reservation.h b/include/linux/reservation.h > index 02166e815afb..54cf6773a14c 100644 > --- a/include/linux/reservation.h > +++ b/include/linux/reservation.h > @@ -68,7 +68,6 @@ struct reservation_object_list { > * @seq: sequence count for managing RCU read-side synchronization > * @fence_excl: the exclusive fence, if there is one currently > * @fence: list of current shared fences > - * @staged: staged copy of shared fences for RCU updates > */ > struct reservation_object { > struct ww_mutex lock; > @@ -76,7 +75,6 @@ struct reservation_object { > > struct dma_fence __rcu *fence_excl; > struct reservation_object_list __rcu *fence; > - struct reservation_object_list *staged; > }; > > #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) > @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) > __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); > RCU_INIT_POINTER(obj->fence, NULL); > RCU_INIT_POINTER(obj->fence_excl, NULL); > - obj->staged = NULL; > } > > /** > @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) > > kfree(fobj); > } > - kfree(obj->staged); > > ww_mutex_destroy(&obj->lock); > } _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 7/8] drm/amdgpu: always reserve one more shared slot for pipelined BO moves 2018-10-04 13:12 [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Christian König 2018-10-04 13:12 ` [PATCH 4/8] drm/ttm: allow reserving more than one shared slot v2 Christian König [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> @ 2018-10-04 13:12 ` Christian König 2018-10-12 8:22 ` Christian König 2018-10-25 20:15 ` Chris Wilson 4 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-04 13:12 UTC (permalink / raw) To: amd-gfx, dri-devel This allows us to drop the extra reserve in TTM. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 6 ++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index ba406bd1b08f..8edf75c76475 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -50,7 +50,8 @@ 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; - p->uf_entry.tv.num_shared = 1; + /* One for TTM and one for the CS job */ + p->uf_entry.tv.num_shared = 2; p->uf_entry.user_pages = NULL; drm_gem_object_put_unlocked(gobj); @@ -598,8 +599,9 @@ 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 = 1; + e->tv.num_shared = 2; amdgpu_bo_list_get_list(p->bo_list, &p->validated); if (p->bo_list->first_userptr != p->bo_list->num_entries) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 1b39b0144698..8c46acf893cc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -616,8 +616,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm, { entry->priority = 0; entry->tv.bo = &vm->root.base.bo->tbo; - /* One for the VM updates and one for the CS job */ - entry->tv.num_shared = 2; + /* One for the VM updates, one for TTM and one for the CS job */ + entry->tv.num_shared = 3; entry->user_pages = NULL; list_add(&entry->tv.head, validated); } -- 2.14.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> @ 2018-10-12 8:22 ` Christian König 2018-10-04 13:12 ` [PATCH 3/8] dma-buf: test shared slot allocation when mutex debugging is active Christian König ` (4 subsequent siblings) 5 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-12 8:22 UTC (permalink / raw) To: amd-gfx, dri-devel, linux-media, linaro-mm-sig Cc: Daniel Vetter, Chris Wilson Ping! Adding a few people directly and more mailing lists. Can I get an acked-by/rb for this? It's only a cleanup and not much functional change. Regards, Christian. Am 04.10.2018 um 15:12 schrieb Christian König: > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- > include/linux/reservation.h | 4 - > 2 files changed, 58 insertions(+), 124 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 6c95f61a32e7..5825fc336a13 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > */ > int reservation_object_reserve_shared(struct reservation_object *obj) > { > - struct reservation_object_list *fobj, *old; > - u32 max; > + struct reservation_object_list *old, *new; > + unsigned int i, j, k, max; > > old = reservation_object_get_list(obj); > > if (old && old->shared_max) { > - if (old->shared_count < old->shared_max) { > - /* perform an in-place update */ > - kfree(obj->staged); > - obj->staged = NULL; > + if (old->shared_count < old->shared_max) > return 0; > - } else > + else > max = old->shared_max * 2; > - } else > - max = 4; > - > - /* > - * resize obj->staged or allocate if it doesn't exist, > - * noop if already correct size > - */ > - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), > - GFP_KERNEL); > - if (!fobj) > - return -ENOMEM; > - > - obj->staged = fobj; > - fobj->shared_max = max; > - return 0; > -} > -EXPORT_SYMBOL(reservation_object_reserve_shared); > - > -static void > -reservation_object_add_shared_inplace(struct reservation_object *obj, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - struct dma_fence *signaled = NULL; > - u32 i, signaled_idx; > - > - dma_fence_get(fence); > - > - preempt_disable(); > - write_seqcount_begin(&obj->seq); > - > - for (i = 0; i < fobj->shared_count; ++i) { > - struct dma_fence *old_fence; > - > - old_fence = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - > - if (old_fence->context == fence->context) { > - /* memory barrier is added by write_seqcount_begin */ > - RCU_INIT_POINTER(fobj->shared[i], fence); > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(old_fence); > - return; > - } > - > - if (!signaled && dma_fence_is_signaled(old_fence)) { > - signaled = old_fence; > - signaled_idx = i; > - } > - } > - > - /* > - * memory barrier is added by write_seqcount_begin, > - * fobj->shared_count is protected by this lock too > - */ > - if (signaled) { > - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > - BUG_ON(fobj->shared_count >= fobj->shared_max); > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + max = 4; > } > > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(signaled); > -} > - > -static void > -reservation_object_add_shared_replace(struct reservation_object *obj, > - struct reservation_object_list *old, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - unsigned i, j, k; > - > - dma_fence_get(fence); > - > - if (!old) { > - RCU_INIT_POINTER(fobj->shared[0], fence); > - fobj->shared_count = 1; > - goto done; > - } > + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > + if (!new) > + return -ENOMEM; > > /* > * no need to bump fence refcounts, rcu_read access > @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > * references from the old struct are carried over to > * the new. > */ > - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { > - struct dma_fence *check; > + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { > + struct dma_fence *fence; > > - check = rcu_dereference_protected(old->shared[i], > - reservation_object_held(obj)); > - > - if (check->context == fence->context || > - dma_fence_is_signaled(check)) > - RCU_INIT_POINTER(fobj->shared[--k], check); > + fence = rcu_dereference_protected(old->shared[i], > + reservation_object_held(obj)); > + if (dma_fence_is_signaled(fence)) > + RCU_INIT_POINTER(new->shared[--k], fence); > else > - RCU_INIT_POINTER(fobj->shared[j++], check); > + RCU_INIT_POINTER(new->shared[j++], fence); > } > - fobj->shared_count = j; > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + new->shared_count = j; > + new->shared_max = max; > > -done: > preempt_disable(); > write_seqcount_begin(&obj->seq); > /* > * RCU_INIT_POINTER can be used here, > * seqcount provides the necessary barriers > */ > - RCU_INIT_POINTER(obj->fence, fobj); > + RCU_INIT_POINTER(obj->fence, new); > write_seqcount_end(&obj->seq); > preempt_enable(); > > if (!old) > - return; > + return 0; > > /* Drop the references to the signaled fences */ > - for (i = k; i < fobj->shared_max; ++i) { > - struct dma_fence *f; > + for (i = k; i < new->shared_max; ++i) { > + struct dma_fence *fence; > > - f = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - dma_fence_put(f); > + fence = rcu_dereference_protected(new->shared[i], > + reservation_object_held(obj)); > + dma_fence_put(fence); > } > kfree_rcu(old, rcu); > + > + return 0; > } > +EXPORT_SYMBOL(reservation_object_reserve_shared); > > /** > * reservation_object_add_shared_fence - Add a fence to a shared slot > @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > void reservation_object_add_shared_fence(struct reservation_object *obj, > struct dma_fence *fence) > { > - struct reservation_object_list *old, *fobj = obj->staged; > + struct reservation_object_list *fobj; > + unsigned int i; > > - old = reservation_object_get_list(obj); > - obj->staged = NULL; > + dma_fence_get(fence); > + > + fobj = reservation_object_get_list(obj); > > - if (!fobj) > - reservation_object_add_shared_inplace(obj, old, fence); > - else > - reservation_object_add_shared_replace(obj, old, fobj, fence); > + preempt_disable(); > + write_seqcount_begin(&obj->seq); > + > + for (i = 0; i < fobj->shared_count; ++i) { > + struct dma_fence *old_fence; > + > + old_fence = rcu_dereference_protected(fobj->shared[i], > + reservation_object_held(obj)); > + if (old_fence->context == fence->context || > + dma_fence_is_signaled(old_fence)) { > + dma_fence_put(old_fence); > + goto replace; > + } > + } > + > + BUG_ON(fobj->shared_count >= fobj->shared_max); > + fobj->shared_count++; > + > +replace: > + /* > + * memory barrier is added by write_seqcount_begin, > + * fobj->shared_count is protected by this lock too > + */ > + RCU_INIT_POINTER(fobj->shared[i], fence); > + write_seqcount_end(&obj->seq); > + preempt_enable(); > } > EXPORT_SYMBOL(reservation_object_add_shared_fence); > > @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, > new = dma_fence_get_rcu_safe(&src->fence_excl); > rcu_read_unlock(); > > - kfree(dst->staged); > - dst->staged = NULL; > - > src_list = reservation_object_get_list(dst); > old = reservation_object_get_excl(dst); > > diff --git a/include/linux/reservation.h b/include/linux/reservation.h > index 02166e815afb..54cf6773a14c 100644 > --- a/include/linux/reservation.h > +++ b/include/linux/reservation.h > @@ -68,7 +68,6 @@ struct reservation_object_list { > * @seq: sequence count for managing RCU read-side synchronization > * @fence_excl: the exclusive fence, if there is one currently > * @fence: list of current shared fences > - * @staged: staged copy of shared fences for RCU updates > */ > struct reservation_object { > struct ww_mutex lock; > @@ -76,7 +75,6 @@ struct reservation_object { > > struct dma_fence __rcu *fence_excl; > struct reservation_object_list __rcu *fence; > - struct reservation_object_list *staged; > }; > > #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) > @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) > __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); > RCU_INIT_POINTER(obj->fence, NULL); > RCU_INIT_POINTER(obj->fence_excl, NULL); > - obj->staged = NULL; > } > > /** > @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) > > kfree(fobj); > } > - kfree(obj->staged); > > ww_mutex_destroy(&obj->lock); > } ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-12 8:22 ` Christian König 0 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-12 8:22 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-media-u79uwXL29TY76Z2rM5mHXA, linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw Cc: Daniel Vetter, Chris Wilson Ping! Adding a few people directly and more mailing lists. Can I get an acked-by/rb for this? It's only a cleanup and not much functional change. Regards, Christian. Am 04.10.2018 um 15:12 schrieb Christian König: > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/reservation.c | 178 ++++++++++++++---------------------------- > include/linux/reservation.h | 4 - > 2 files changed, 58 insertions(+), 124 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 6c95f61a32e7..5825fc336a13 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > */ > int reservation_object_reserve_shared(struct reservation_object *obj) > { > - struct reservation_object_list *fobj, *old; > - u32 max; > + struct reservation_object_list *old, *new; > + unsigned int i, j, k, max; > > old = reservation_object_get_list(obj); > > if (old && old->shared_max) { > - if (old->shared_count < old->shared_max) { > - /* perform an in-place update */ > - kfree(obj->staged); > - obj->staged = NULL; > + if (old->shared_count < old->shared_max) > return 0; > - } else > + else > max = old->shared_max * 2; > - } else > - max = 4; > - > - /* > - * resize obj->staged or allocate if it doesn't exist, > - * noop if already correct size > - */ > - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), > - GFP_KERNEL); > - if (!fobj) > - return -ENOMEM; > - > - obj->staged = fobj; > - fobj->shared_max = max; > - return 0; > -} > -EXPORT_SYMBOL(reservation_object_reserve_shared); > - > -static void > -reservation_object_add_shared_inplace(struct reservation_object *obj, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - struct dma_fence *signaled = NULL; > - u32 i, signaled_idx; > - > - dma_fence_get(fence); > - > - preempt_disable(); > - write_seqcount_begin(&obj->seq); > - > - for (i = 0; i < fobj->shared_count; ++i) { > - struct dma_fence *old_fence; > - > - old_fence = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - > - if (old_fence->context == fence->context) { > - /* memory barrier is added by write_seqcount_begin */ > - RCU_INIT_POINTER(fobj->shared[i], fence); > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(old_fence); > - return; > - } > - > - if (!signaled && dma_fence_is_signaled(old_fence)) { > - signaled = old_fence; > - signaled_idx = i; > - } > - } > - > - /* > - * memory barrier is added by write_seqcount_begin, > - * fobj->shared_count is protected by this lock too > - */ > - if (signaled) { > - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > - BUG_ON(fobj->shared_count >= fobj->shared_max); > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + max = 4; > } > > - write_seqcount_end(&obj->seq); > - preempt_enable(); > - > - dma_fence_put(signaled); > -} > - > -static void > -reservation_object_add_shared_replace(struct reservation_object *obj, > - struct reservation_object_list *old, > - struct reservation_object_list *fobj, > - struct dma_fence *fence) > -{ > - unsigned i, j, k; > - > - dma_fence_get(fence); > - > - if (!old) { > - RCU_INIT_POINTER(fobj->shared[0], fence); > - fobj->shared_count = 1; > - goto done; > - } > + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > + if (!new) > + return -ENOMEM; > > /* > * no need to bump fence refcounts, rcu_read access > @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > * references from the old struct are carried over to > * the new. > */ > - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { > - struct dma_fence *check; > + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); ++i) { > + struct dma_fence *fence; > > - check = rcu_dereference_protected(old->shared[i], > - reservation_object_held(obj)); > - > - if (check->context == fence->context || > - dma_fence_is_signaled(check)) > - RCU_INIT_POINTER(fobj->shared[--k], check); > + fence = rcu_dereference_protected(old->shared[i], > + reservation_object_held(obj)); > + if (dma_fence_is_signaled(fence)) > + RCU_INIT_POINTER(new->shared[--k], fence); > else > - RCU_INIT_POINTER(fobj->shared[j++], check); > + RCU_INIT_POINTER(new->shared[j++], fence); > } > - fobj->shared_count = j; > - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > - fobj->shared_count++; > + new->shared_count = j; > + new->shared_max = max; > > -done: > preempt_disable(); > write_seqcount_begin(&obj->seq); > /* > * RCU_INIT_POINTER can be used here, > * seqcount provides the necessary barriers > */ > - RCU_INIT_POINTER(obj->fence, fobj); > + RCU_INIT_POINTER(obj->fence, new); > write_seqcount_end(&obj->seq); > preempt_enable(); > > if (!old) > - return; > + return 0; > > /* Drop the references to the signaled fences */ > - for (i = k; i < fobj->shared_max; ++i) { > - struct dma_fence *f; > + for (i = k; i < new->shared_max; ++i) { > + struct dma_fence *fence; > > - f = rcu_dereference_protected(fobj->shared[i], > - reservation_object_held(obj)); > - dma_fence_put(f); > + fence = rcu_dereference_protected(new->shared[i], > + reservation_object_held(obj)); > + dma_fence_put(fence); > } > kfree_rcu(old, rcu); > + > + return 0; > } > +EXPORT_SYMBOL(reservation_object_reserve_shared); > > /** > * reservation_object_add_shared_fence - Add a fence to a shared slot > @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct reservation_object *obj, > void reservation_object_add_shared_fence(struct reservation_object *obj, > struct dma_fence *fence) > { > - struct reservation_object_list *old, *fobj = obj->staged; > + struct reservation_object_list *fobj; > + unsigned int i; > > - old = reservation_object_get_list(obj); > - obj->staged = NULL; > + dma_fence_get(fence); > + > + fobj = reservation_object_get_list(obj); > > - if (!fobj) > - reservation_object_add_shared_inplace(obj, old, fence); > - else > - reservation_object_add_shared_replace(obj, old, fobj, fence); > + preempt_disable(); > + write_seqcount_begin(&obj->seq); > + > + for (i = 0; i < fobj->shared_count; ++i) { > + struct dma_fence *old_fence; > + > + old_fence = rcu_dereference_protected(fobj->shared[i], > + reservation_object_held(obj)); > + if (old_fence->context == fence->context || > + dma_fence_is_signaled(old_fence)) { > + dma_fence_put(old_fence); > + goto replace; > + } > + } > + > + BUG_ON(fobj->shared_count >= fobj->shared_max); > + fobj->shared_count++; > + > +replace: > + /* > + * memory barrier is added by write_seqcount_begin, > + * fobj->shared_count is protected by this lock too > + */ > + RCU_INIT_POINTER(fobj->shared[i], fence); > + write_seqcount_end(&obj->seq); > + preempt_enable(); > } > EXPORT_SYMBOL(reservation_object_add_shared_fence); > > @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct reservation_object *dst, > new = dma_fence_get_rcu_safe(&src->fence_excl); > rcu_read_unlock(); > > - kfree(dst->staged); > - dst->staged = NULL; > - > src_list = reservation_object_get_list(dst); > old = reservation_object_get_excl(dst); > > diff --git a/include/linux/reservation.h b/include/linux/reservation.h > index 02166e815afb..54cf6773a14c 100644 > --- a/include/linux/reservation.h > +++ b/include/linux/reservation.h > @@ -68,7 +68,6 @@ struct reservation_object_list { > * @seq: sequence count for managing RCU read-side synchronization > * @fence_excl: the exclusive fence, if there is one currently > * @fence: list of current shared fences > - * @staged: staged copy of shared fences for RCU updates > */ > struct reservation_object { > struct ww_mutex lock; > @@ -76,7 +75,6 @@ struct reservation_object { > > struct dma_fence __rcu *fence_excl; > struct reservation_object_list __rcu *fence; > - struct reservation_object_list *staged; > }; > > #define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base) > @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object *obj) > __seqcount_init(&obj->seq, reservation_seqcount_string, &reservation_seqcount_class); > RCU_INIT_POINTER(obj->fence, NULL); > RCU_INIT_POINTER(obj->fence_excl, NULL); > - obj->staged = NULL; > } > > /** > @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object *obj) > > kfree(fobj); > } > - kfree(obj->staged); > > ww_mutex_destroy(&obj->lock); > } _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-23 12:20 ` Christian König 0 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-23 12:20 UTC (permalink / raw) To: amd-gfx, dri-devel, linux-media, linaro-mm-sig, Huang Rui, Daenzer, Michel Cc: Daniel Vetter, Chris Wilson, Zhang, Jerry Ping once more! Adding a few more AMD people. Any comments on this? Thanks, Christian. Am 12.10.18 um 10:22 schrieb Christian König: > Ping! Adding a few people directly and more mailing lists. > > Can I get an acked-by/rb for this? It's only a cleanup and not much > functional change. > > Regards, > Christian. > > Am 04.10.2018 um 15:12 schrieb Christian König: >> No need for that any more. Just replace the list when there isn't enough >> room any more for the additional fence. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/reservation.c | 178 >> ++++++++++++++---------------------------- >> include/linux/reservation.h | 4 - >> 2 files changed, 58 insertions(+), 124 deletions(-) >> >> diff --git a/drivers/dma-buf/reservation.c >> b/drivers/dma-buf/reservation.c >> index 6c95f61a32e7..5825fc336a13 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); >> */ >> int reservation_object_reserve_shared(struct reservation_object *obj) >> { >> - struct reservation_object_list *fobj, *old; >> - u32 max; >> + struct reservation_object_list *old, *new; >> + unsigned int i, j, k, max; >> old = reservation_object_get_list(obj); >> if (old && old->shared_max) { >> - if (old->shared_count < old->shared_max) { >> - /* perform an in-place update */ >> - kfree(obj->staged); >> - obj->staged = NULL; >> + if (old->shared_count < old->shared_max) >> return 0; >> - } else >> + else >> max = old->shared_max * 2; >> - } else >> - max = 4; >> - >> - /* >> - * resize obj->staged or allocate if it doesn't exist, >> - * noop if already correct size >> - */ >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), >> - GFP_KERNEL); >> - if (!fobj) >> - return -ENOMEM; >> - >> - obj->staged = fobj; >> - fobj->shared_max = max; >> - return 0; >> -} >> -EXPORT_SYMBOL(reservation_object_reserve_shared); >> - >> -static void >> -reservation_object_add_shared_inplace(struct reservation_object *obj, >> - struct reservation_object_list *fobj, >> - struct dma_fence *fence) >> -{ >> - struct dma_fence *signaled = NULL; >> - u32 i, signaled_idx; >> - >> - dma_fence_get(fence); >> - >> - preempt_disable(); >> - write_seqcount_begin(&obj->seq); >> - >> - for (i = 0; i < fobj->shared_count; ++i) { >> - struct dma_fence *old_fence; >> - >> - old_fence = rcu_dereference_protected(fobj->shared[i], >> - reservation_object_held(obj)); >> - >> - if (old_fence->context == fence->context) { >> - /* memory barrier is added by write_seqcount_begin */ >> - RCU_INIT_POINTER(fobj->shared[i], fence); >> - write_seqcount_end(&obj->seq); >> - preempt_enable(); >> - >> - dma_fence_put(old_fence); >> - return; >> - } >> - >> - if (!signaled && dma_fence_is_signaled(old_fence)) { >> - signaled = old_fence; >> - signaled_idx = i; >> - } >> - } >> - >> - /* >> - * memory barrier is added by write_seqcount_begin, >> - * fobj->shared_count is protected by this lock too >> - */ >> - if (signaled) { >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); >> } else { >> - BUG_ON(fobj->shared_count >= fobj->shared_max); >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); >> - fobj->shared_count++; >> + max = 4; >> } >> - write_seqcount_end(&obj->seq); >> - preempt_enable(); >> - >> - dma_fence_put(signaled); >> -} >> - >> -static void >> -reservation_object_add_shared_replace(struct reservation_object *obj, >> - struct reservation_object_list *old, >> - struct reservation_object_list *fobj, >> - struct dma_fence *fence) >> -{ >> - unsigned i, j, k; >> - >> - dma_fence_get(fence); >> - >> - if (!old) { >> - RCU_INIT_POINTER(fobj->shared[0], fence); >> - fobj->shared_count = 1; >> - goto done; >> - } >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); >> + if (!new) >> + return -ENOMEM; >> /* >> * no need to bump fence refcounts, rcu_read access >> @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct >> reservation_object *obj, >> * references from the old struct are carried over to >> * the new. >> */ >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; >> ++i) { >> - struct dma_fence *check; >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); >> ++i) { >> + struct dma_fence *fence; >> - check = rcu_dereference_protected(old->shared[i], >> - reservation_object_held(obj)); >> - >> - if (check->context == fence->context || >> - dma_fence_is_signaled(check)) >> - RCU_INIT_POINTER(fobj->shared[--k], check); >> + fence = rcu_dereference_protected(old->shared[i], >> + reservation_object_held(obj)); >> + if (dma_fence_is_signaled(fence)) >> + RCU_INIT_POINTER(new->shared[--k], fence); >> else >> - RCU_INIT_POINTER(fobj->shared[j++], check); >> + RCU_INIT_POINTER(new->shared[j++], fence); >> } >> - fobj->shared_count = j; >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); >> - fobj->shared_count++; >> + new->shared_count = j; >> + new->shared_max = max; >> -done: >> preempt_disable(); >> write_seqcount_begin(&obj->seq); >> /* >> * RCU_INIT_POINTER can be used here, >> * seqcount provides the necessary barriers >> */ >> - RCU_INIT_POINTER(obj->fence, fobj); >> + RCU_INIT_POINTER(obj->fence, new); >> write_seqcount_end(&obj->seq); >> preempt_enable(); >> if (!old) >> - return; >> + return 0; >> /* Drop the references to the signaled fences */ >> - for (i = k; i < fobj->shared_max; ++i) { >> - struct dma_fence *f; >> + for (i = k; i < new->shared_max; ++i) { >> + struct dma_fence *fence; >> - f = rcu_dereference_protected(fobj->shared[i], >> - reservation_object_held(obj)); >> - dma_fence_put(f); >> + fence = rcu_dereference_protected(new->shared[i], >> + reservation_object_held(obj)); >> + dma_fence_put(fence); >> } >> kfree_rcu(old, rcu); >> + >> + return 0; >> } >> +EXPORT_SYMBOL(reservation_object_reserve_shared); >> /** >> * reservation_object_add_shared_fence - Add a fence to a shared slot >> @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct >> reservation_object *obj, >> void reservation_object_add_shared_fence(struct reservation_object >> *obj, >> struct dma_fence *fence) >> { >> - struct reservation_object_list *old, *fobj = obj->staged; >> + struct reservation_object_list *fobj; >> + unsigned int i; >> - old = reservation_object_get_list(obj); >> - obj->staged = NULL; >> + dma_fence_get(fence); >> + >> + fobj = reservation_object_get_list(obj); >> - if (!fobj) >> - reservation_object_add_shared_inplace(obj, old, fence); >> - else >> - reservation_object_add_shared_replace(obj, old, fobj, fence); >> + preempt_disable(); >> + write_seqcount_begin(&obj->seq); >> + >> + for (i = 0; i < fobj->shared_count; ++i) { >> + struct dma_fence *old_fence; >> + >> + old_fence = rcu_dereference_protected(fobj->shared[i], >> + reservation_object_held(obj)); >> + if (old_fence->context == fence->context || >> + dma_fence_is_signaled(old_fence)) { >> + dma_fence_put(old_fence); >> + goto replace; >> + } >> + } >> + >> + BUG_ON(fobj->shared_count >= fobj->shared_max); >> + fobj->shared_count++; >> + >> +replace: >> + /* >> + * memory barrier is added by write_seqcount_begin, >> + * fobj->shared_count is protected by this lock too >> + */ >> + RCU_INIT_POINTER(fobj->shared[i], fence); >> + write_seqcount_end(&obj->seq); >> + preempt_enable(); >> } >> EXPORT_SYMBOL(reservation_object_add_shared_fence); >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct >> reservation_object *dst, >> new = dma_fence_get_rcu_safe(&src->fence_excl); >> rcu_read_unlock(); >> - kfree(dst->staged); >> - dst->staged = NULL; >> - >> src_list = reservation_object_get_list(dst); >> old = reservation_object_get_excl(dst); >> diff --git a/include/linux/reservation.h b/include/linux/reservation.h >> index 02166e815afb..54cf6773a14c 100644 >> --- a/include/linux/reservation.h >> +++ b/include/linux/reservation.h >> @@ -68,7 +68,6 @@ struct reservation_object_list { >> * @seq: sequence count for managing RCU read-side synchronization >> * @fence_excl: the exclusive fence, if there is one currently >> * @fence: list of current shared fences >> - * @staged: staged copy of shared fences for RCU updates >> */ >> struct reservation_object { >> struct ww_mutex lock; >> @@ -76,7 +75,6 @@ struct reservation_object { >> struct dma_fence __rcu *fence_excl; >> struct reservation_object_list __rcu *fence; >> - struct reservation_object_list *staged; >> }; >> #define reservation_object_held(obj) >> lockdep_is_held(&(obj)->lock.base) >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object >> *obj) >> __seqcount_init(&obj->seq, reservation_seqcount_string, >> &reservation_seqcount_class); >> RCU_INIT_POINTER(obj->fence, NULL); >> RCU_INIT_POINTER(obj->fence_excl, NULL); >> - obj->staged = NULL; >> } >> /** >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object >> *obj) >> kfree(fobj); >> } >> - kfree(obj->staged); >> ww_mutex_destroy(&obj->lock); >> } > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-23 12:20 ` Christian König 0 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-23 12:20 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-media-u79uwXL29TY76Z2rM5mHXA, linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw, Huang Rui, Daenzer, Michel Cc: Zhang, Jerry, Daniel Vetter, Chris Wilson Ping once more! Adding a few more AMD people. Any comments on this? Thanks, Christian. Am 12.10.18 um 10:22 schrieb Christian König: > Ping! Adding a few people directly and more mailing lists. > > Can I get an acked-by/rb for this? It's only a cleanup and not much > functional change. > > Regards, > Christian. > > Am 04.10.2018 um 15:12 schrieb Christian König: >> No need for that any more. Just replace the list when there isn't enough >> room any more for the additional fence. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/reservation.c | 178 >> ++++++++++++++---------------------------- >> include/linux/reservation.h | 4 - >> 2 files changed, 58 insertions(+), 124 deletions(-) >> >> diff --git a/drivers/dma-buf/reservation.c >> b/drivers/dma-buf/reservation.c >> index 6c95f61a32e7..5825fc336a13 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); >> */ >> int reservation_object_reserve_shared(struct reservation_object *obj) >> { >> - struct reservation_object_list *fobj, *old; >> - u32 max; >> + struct reservation_object_list *old, *new; >> + unsigned int i, j, k, max; >> old = reservation_object_get_list(obj); >> if (old && old->shared_max) { >> - if (old->shared_count < old->shared_max) { >> - /* perform an in-place update */ >> - kfree(obj->staged); >> - obj->staged = NULL; >> + if (old->shared_count < old->shared_max) >> return 0; >> - } else >> + else >> max = old->shared_max * 2; >> - } else >> - max = 4; >> - >> - /* >> - * resize obj->staged or allocate if it doesn't exist, >> - * noop if already correct size >> - */ >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), shared[max]), >> - GFP_KERNEL); >> - if (!fobj) >> - return -ENOMEM; >> - >> - obj->staged = fobj; >> - fobj->shared_max = max; >> - return 0; >> -} >> -EXPORT_SYMBOL(reservation_object_reserve_shared); >> - >> -static void >> -reservation_object_add_shared_inplace(struct reservation_object *obj, >> - struct reservation_object_list *fobj, >> - struct dma_fence *fence) >> -{ >> - struct dma_fence *signaled = NULL; >> - u32 i, signaled_idx; >> - >> - dma_fence_get(fence); >> - >> - preempt_disable(); >> - write_seqcount_begin(&obj->seq); >> - >> - for (i = 0; i < fobj->shared_count; ++i) { >> - struct dma_fence *old_fence; >> - >> - old_fence = rcu_dereference_protected(fobj->shared[i], >> - reservation_object_held(obj)); >> - >> - if (old_fence->context == fence->context) { >> - /* memory barrier is added by write_seqcount_begin */ >> - RCU_INIT_POINTER(fobj->shared[i], fence); >> - write_seqcount_end(&obj->seq); >> - preempt_enable(); >> - >> - dma_fence_put(old_fence); >> - return; >> - } >> - >> - if (!signaled && dma_fence_is_signaled(old_fence)) { >> - signaled = old_fence; >> - signaled_idx = i; >> - } >> - } >> - >> - /* >> - * memory barrier is added by write_seqcount_begin, >> - * fobj->shared_count is protected by this lock too >> - */ >> - if (signaled) { >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); >> } else { >> - BUG_ON(fobj->shared_count >= fobj->shared_max); >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); >> - fobj->shared_count++; >> + max = 4; >> } >> - write_seqcount_end(&obj->seq); >> - preempt_enable(); >> - >> - dma_fence_put(signaled); >> -} >> - >> -static void >> -reservation_object_add_shared_replace(struct reservation_object *obj, >> - struct reservation_object_list *old, >> - struct reservation_object_list *fobj, >> - struct dma_fence *fence) >> -{ >> - unsigned i, j, k; >> - >> - dma_fence_get(fence); >> - >> - if (!old) { >> - RCU_INIT_POINTER(fobj->shared[0], fence); >> - fobj->shared_count = 1; >> - goto done; >> - } >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); >> + if (!new) >> + return -ENOMEM; >> /* >> * no need to bump fence refcounts, rcu_read access >> @@ -174,46 +92,45 @@ reservation_object_add_shared_replace(struct >> reservation_object *obj, >> * references from the old struct are carried over to >> * the new. >> */ >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; >> ++i) { >> - struct dma_fence *check; >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); >> ++i) { >> + struct dma_fence *fence; >> - check = rcu_dereference_protected(old->shared[i], >> - reservation_object_held(obj)); >> - >> - if (check->context == fence->context || >> - dma_fence_is_signaled(check)) >> - RCU_INIT_POINTER(fobj->shared[--k], check); >> + fence = rcu_dereference_protected(old->shared[i], >> + reservation_object_held(obj)); >> + if (dma_fence_is_signaled(fence)) >> + RCU_INIT_POINTER(new->shared[--k], fence); >> else >> - RCU_INIT_POINTER(fobj->shared[j++], check); >> + RCU_INIT_POINTER(new->shared[j++], fence); >> } >> - fobj->shared_count = j; >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); >> - fobj->shared_count++; >> + new->shared_count = j; >> + new->shared_max = max; >> -done: >> preempt_disable(); >> write_seqcount_begin(&obj->seq); >> /* >> * RCU_INIT_POINTER can be used here, >> * seqcount provides the necessary barriers >> */ >> - RCU_INIT_POINTER(obj->fence, fobj); >> + RCU_INIT_POINTER(obj->fence, new); >> write_seqcount_end(&obj->seq); >> preempt_enable(); >> if (!old) >> - return; >> + return 0; >> /* Drop the references to the signaled fences */ >> - for (i = k; i < fobj->shared_max; ++i) { >> - struct dma_fence *f; >> + for (i = k; i < new->shared_max; ++i) { >> + struct dma_fence *fence; >> - f = rcu_dereference_protected(fobj->shared[i], >> - reservation_object_held(obj)); >> - dma_fence_put(f); >> + fence = rcu_dereference_protected(new->shared[i], >> + reservation_object_held(obj)); >> + dma_fence_put(fence); >> } >> kfree_rcu(old, rcu); >> + >> + return 0; >> } >> +EXPORT_SYMBOL(reservation_object_reserve_shared); >> /** >> * reservation_object_add_shared_fence - Add a fence to a shared slot >> @@ -226,15 +143,39 @@ reservation_object_add_shared_replace(struct >> reservation_object *obj, >> void reservation_object_add_shared_fence(struct reservation_object >> *obj, >> struct dma_fence *fence) >> { >> - struct reservation_object_list *old, *fobj = obj->staged; >> + struct reservation_object_list *fobj; >> + unsigned int i; >> - old = reservation_object_get_list(obj); >> - obj->staged = NULL; >> + dma_fence_get(fence); >> + >> + fobj = reservation_object_get_list(obj); >> - if (!fobj) >> - reservation_object_add_shared_inplace(obj, old, fence); >> - else >> - reservation_object_add_shared_replace(obj, old, fobj, fence); >> + preempt_disable(); >> + write_seqcount_begin(&obj->seq); >> + >> + for (i = 0; i < fobj->shared_count; ++i) { >> + struct dma_fence *old_fence; >> + >> + old_fence = rcu_dereference_protected(fobj->shared[i], >> + reservation_object_held(obj)); >> + if (old_fence->context == fence->context || >> + dma_fence_is_signaled(old_fence)) { >> + dma_fence_put(old_fence); >> + goto replace; >> + } >> + } >> + >> + BUG_ON(fobj->shared_count >= fobj->shared_max); >> + fobj->shared_count++; >> + >> +replace: >> + /* >> + * memory barrier is added by write_seqcount_begin, >> + * fobj->shared_count is protected by this lock too >> + */ >> + RCU_INIT_POINTER(fobj->shared[i], fence); >> + write_seqcount_end(&obj->seq); >> + preempt_enable(); >> } >> EXPORT_SYMBOL(reservation_object_add_shared_fence); >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct >> reservation_object *dst, >> new = dma_fence_get_rcu_safe(&src->fence_excl); >> rcu_read_unlock(); >> - kfree(dst->staged); >> - dst->staged = NULL; >> - >> src_list = reservation_object_get_list(dst); >> old = reservation_object_get_excl(dst); >> diff --git a/include/linux/reservation.h b/include/linux/reservation.h >> index 02166e815afb..54cf6773a14c 100644 >> --- a/include/linux/reservation.h >> +++ b/include/linux/reservation.h >> @@ -68,7 +68,6 @@ struct reservation_object_list { >> * @seq: sequence count for managing RCU read-side synchronization >> * @fence_excl: the exclusive fence, if there is one currently >> * @fence: list of current shared fences >> - * @staged: staged copy of shared fences for RCU updates >> */ >> struct reservation_object { >> struct ww_mutex lock; >> @@ -76,7 +75,6 @@ struct reservation_object { >> struct dma_fence __rcu *fence_excl; >> struct reservation_object_list __rcu *fence; >> - struct reservation_object_list *staged; >> }; >> #define reservation_object_held(obj) >> lockdep_is_held(&(obj)->lock.base) >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object >> *obj) >> __seqcount_init(&obj->seq, reservation_seqcount_string, >> &reservation_seqcount_class); >> RCU_INIT_POINTER(obj->fence, NULL); >> RCU_INIT_POINTER(obj->fence_excl, NULL); >> - obj->staged = NULL; >> } >> /** >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object >> *obj) >> kfree(fobj); >> } >> - kfree(obj->staged); >> ww_mutex_destroy(&obj->lock); >> } > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-23 13:40 ` Michel Dänzer 0 siblings, 0 replies; 25+ messages in thread From: Michel Dänzer @ 2018-10-23 13:40 UTC (permalink / raw) To: Christian König; +Cc: amd-gfx, dri-devel, linux-media, linaro-mm-sig On 2018-10-23 2:20 p.m., Christian König wrote: > Ping once more! Adding a few more AMD people. > > Any comments on this? Patches 1 & 3 are a bit over my head I'm afraid. Patches 2, 4, 6-8 are Reviewed-by: Michel Dänzer <michel.daenzer@amd.com> -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-23 13:40 ` Michel Dänzer 0 siblings, 0 replies; 25+ messages in thread From: Michel Dänzer @ 2018-10-23 13:40 UTC (permalink / raw) To: Christian König Cc: linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-media-u79uwXL29TY76Z2rM5mHXA On 2018-10-23 2:20 p.m., Christian König wrote: > Ping once more! Adding a few more AMD people. > > Any comments on this? Patches 1 & 3 are a bit over my head I'm afraid. Patches 2, 4, 6-8 are Reviewed-by: Michel Dänzer <michel.daenzer@amd.com> -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-24 5:12 ` Huang, Ray 0 siblings, 0 replies; 25+ messages in thread From: Huang, Ray @ 2018-10-24 5:12 UTC (permalink / raw) To: Christian König, amd-gfx, dri-devel, linux-media, linaro-mm-sig, Daenzer, Michel Cc: Daniel Vetter, Chris Wilson, Zhang, Jerry Christian, I will take a look at them later. Thanks, Ray > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; > Zhang, Jerry <Jerry.Zhang@amd.com> > Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in > reservation object > > Ping once more! Adding a few more AMD people. > > Any comments on this? > > Thanks, > Christian. > > Am 12.10.18 um 10:22 schrieb Christian König: > > Ping! Adding a few people directly and more mailing lists. > > > > Can I get an acked-by/rb for this? It's only a cleanup and not much > > functional change. > > > > Regards, > > Christian. > > > > Am 04.10.2018 um 15:12 schrieb Christian König: > >> No need for that any more. Just replace the list when there isn't > >> enough room any more for the additional fence. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/reservation.c | 178 > >> ++++++++++++++---------------------------- > >> include/linux/reservation.h | 4 - > >> 2 files changed, 58 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/dma-buf/reservation.c > >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 > >> 100644 > >> --- a/drivers/dma-buf/reservation.c > >> +++ b/drivers/dma-buf/reservation.c > >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > >> */ > >> int reservation_object_reserve_shared(struct reservation_object > >> *obj) > >> { > >> - struct reservation_object_list *fobj, *old; > >> - u32 max; > >> + struct reservation_object_list *old, *new; > >> + unsigned int i, j, k, max; > >> old = reservation_object_get_list(obj); > >> if (old && old->shared_max) { > >> - if (old->shared_count < old->shared_max) { > >> - /* perform an in-place update */ > >> - kfree(obj->staged); > >> - obj->staged = NULL; > >> + if (old->shared_count < old->shared_max) > >> return 0; > >> - } else > >> + else > >> max = old->shared_max * 2; > >> - } else > >> - max = 4; > >> - > >> - /* > >> - * resize obj->staged or allocate if it doesn't exist, > >> - * noop if already correct size > >> - */ > >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), > >> shared[max]), > >> - GFP_KERNEL); > >> - if (!fobj) > >> - return -ENOMEM; > >> - > >> - obj->staged = fobj; > >> - fobj->shared_max = max; > >> - return 0; > >> -} > >> -EXPORT_SYMBOL(reservation_object_reserve_shared); > >> - > >> -static void > >> -reservation_object_add_shared_inplace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - struct dma_fence *signaled = NULL; > >> - u32 i, signaled_idx; > >> - > >> - dma_fence_get(fence); > >> - > >> - preempt_disable(); > >> - write_seqcount_begin(&obj->seq); > >> - > >> - for (i = 0; i < fobj->shared_count; ++i) { > >> - struct dma_fence *old_fence; > >> - > >> - old_fence = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (old_fence->context == fence->context) { > >> - /* memory barrier is added by write_seqcount_begin */ > >> - RCU_INIT_POINTER(fobj->shared[i], fence); > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(old_fence); > >> - return; > >> - } > >> - > >> - if (!signaled && dma_fence_is_signaled(old_fence)) { > >> - signaled = old_fence; > >> - signaled_idx = i; > >> - } > >> - } > >> - > >> - /* > >> - * memory barrier is added by write_seqcount_begin, > >> - * fobj->shared_count is protected by this lock too > >> - */ > >> - if (signaled) { > >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > >> } else { > >> - BUG_ON(fobj->shared_count >= fobj->shared_max); > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + max = 4; > >> } > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(signaled); > >> -} > >> - > >> -static void > >> -reservation_object_add_shared_replace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *old, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - unsigned i, j, k; > >> - > >> - dma_fence_get(fence); > >> - > >> - if (!old) { > >> - RCU_INIT_POINTER(fobj->shared[0], fence); > >> - fobj->shared_count = 1; > >> - goto done; > >> - } > >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > >> + if (!new) > >> + return -ENOMEM; > >> /* > >> * no need to bump fence refcounts, rcu_read access @@ -174,46 > >> +92,45 @@ reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> * references from the old struct are carried over to > >> * the new. > >> */ > >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; > >> ++i) { > >> - struct dma_fence *check; > >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); > >> ++i) { > >> + struct dma_fence *fence; > >> - check = rcu_dereference_protected(old->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (check->context == fence->context || > >> - dma_fence_is_signaled(check)) > >> - RCU_INIT_POINTER(fobj->shared[--k], check); > >> + fence = rcu_dereference_protected(old->shared[i], > >> + reservation_object_held(obj)); > >> + if (dma_fence_is_signaled(fence)) > >> + RCU_INIT_POINTER(new->shared[--k], fence); > >> else > >> - RCU_INIT_POINTER(fobj->shared[j++], check); > >> + RCU_INIT_POINTER(new->shared[j++], fence); > >> } > >> - fobj->shared_count = j; > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + new->shared_count = j; > >> + new->shared_max = max; > >> -done: > >> preempt_disable(); > >> write_seqcount_begin(&obj->seq); > >> /* > >> * RCU_INIT_POINTER can be used here, > >> * seqcount provides the necessary barriers > >> */ > >> - RCU_INIT_POINTER(obj->fence, fobj); > >> + RCU_INIT_POINTER(obj->fence, new); > >> write_seqcount_end(&obj->seq); > >> preempt_enable(); > >> if (!old) > >> - return; > >> + return 0; > >> /* Drop the references to the signaled fences */ > >> - for (i = k; i < fobj->shared_max; ++i) { > >> - struct dma_fence *f; > >> + for (i = k; i < new->shared_max; ++i) { > >> + struct dma_fence *fence; > >> - f = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - dma_fence_put(f); > >> + fence = rcu_dereference_protected(new->shared[i], > >> + reservation_object_held(obj)); > >> + dma_fence_put(fence); > >> } > >> kfree_rcu(old, rcu); > >> + > >> + return 0; > >> } > >> +EXPORT_SYMBOL(reservation_object_reserve_shared); > >> /** > >> * reservation_object_add_shared_fence - Add a fence to a shared > >> slot @@ -226,15 +143,39 @@ > >> reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> void reservation_object_add_shared_fence(struct reservation_object > >> *obj, > >> struct dma_fence *fence) > >> { > >> - struct reservation_object_list *old, *fobj = obj->staged; > >> + struct reservation_object_list *fobj; > >> + unsigned int i; > >> - old = reservation_object_get_list(obj); > >> - obj->staged = NULL; > >> + dma_fence_get(fence); > >> + > >> + fobj = reservation_object_get_list(obj); > >> - if (!fobj) > >> - reservation_object_add_shared_inplace(obj, old, fence); > >> - else > >> - reservation_object_add_shared_replace(obj, old, fobj, > >> fence); > >> + preempt_disable(); > >> + write_seqcount_begin(&obj->seq); > >> + > >> + for (i = 0; i < fobj->shared_count; ++i) { > >> + struct dma_fence *old_fence; > >> + > >> + old_fence = rcu_dereference_protected(fobj->shared[i], > >> + reservation_object_held(obj)); > >> + if (old_fence->context == fence->context || > >> + dma_fence_is_signaled(old_fence)) { > >> + dma_fence_put(old_fence); > >> + goto replace; > >> + } > >> + } > >> + > >> + BUG_ON(fobj->shared_count >= fobj->shared_max); > >> + fobj->shared_count++; > >> + > >> +replace: > >> + /* > >> + * memory barrier is added by write_seqcount_begin, > >> + * fobj->shared_count is protected by this lock too > >> + */ > >> + RCU_INIT_POINTER(fobj->shared[i], fence); > >> + write_seqcount_end(&obj->seq); > >> + preempt_enable(); > >> } > >> EXPORT_SYMBOL(reservation_object_add_shared_fence); > >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct > >> reservation_object *dst, > >> new = dma_fence_get_rcu_safe(&src->fence_excl); > >> rcu_read_unlock(); > >> - kfree(dst->staged); > >> - dst->staged = NULL; > >> - > >> src_list = reservation_object_get_list(dst); > >> old = reservation_object_get_excl(dst); > >> diff --git a/include/linux/reservation.h > >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 > >> --- a/include/linux/reservation.h > >> +++ b/include/linux/reservation.h > >> @@ -68,7 +68,6 @@ struct reservation_object_list { > >> * @seq: sequence count for managing RCU read-side synchronization > >> * @fence_excl: the exclusive fence, if there is one currently > >> * @fence: list of current shared fences > >> - * @staged: staged copy of shared fences for RCU updates > >> */ > >> struct reservation_object { > >> struct ww_mutex lock; > >> @@ -76,7 +75,6 @@ struct reservation_object { > >> struct dma_fence __rcu *fence_excl; > >> struct reservation_object_list __rcu *fence; > >> - struct reservation_object_list *staged; > >> }; > >> #define reservation_object_held(obj) > >> lockdep_is_held(&(obj)->lock.base) > >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object > >> *obj) > >> __seqcount_init(&obj->seq, reservation_seqcount_string, > >> &reservation_seqcount_class); > >> RCU_INIT_POINTER(obj->fence, NULL); > >> RCU_INIT_POINTER(obj->fence_excl, NULL); > >> - obj->staged = NULL; > >> } > >> /** > >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object > >> *obj) > >> kfree(fobj); > >> } > >> - kfree(obj->staged); > >> ww_mutex_destroy(&obj->lock); > >> } > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-24 5:12 ` Huang, Ray 0 siblings, 0 replies; 25+ messages in thread From: Huang, Ray @ 2018-10-24 5:12 UTC (permalink / raw) To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-media-u79uwXL29TY76Z2rM5mHXA, linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw, Daenzer, Michel Cc: Zhang, Jerry, Daniel Vetter, Chris Wilson Christian, I will take a look at them later. Thanks, Ray > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; > Zhang, Jerry <Jerry.Zhang@amd.com> > Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in > reservation object > > Ping once more! Adding a few more AMD people. > > Any comments on this? > > Thanks, > Christian. > > Am 12.10.18 um 10:22 schrieb Christian König: > > Ping! Adding a few people directly and more mailing lists. > > > > Can I get an acked-by/rb for this? It's only a cleanup and not much > > functional change. > > > > Regards, > > Christian. > > > > Am 04.10.2018 um 15:12 schrieb Christian König: > >> No need for that any more. Just replace the list when there isn't > >> enough room any more for the additional fence. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/reservation.c | 178 > >> ++++++++++++++---------------------------- > >> include/linux/reservation.h | 4 - > >> 2 files changed, 58 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/dma-buf/reservation.c > >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 > >> 100644 > >> --- a/drivers/dma-buf/reservation.c > >> +++ b/drivers/dma-buf/reservation.c > >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > >> */ > >> int reservation_object_reserve_shared(struct reservation_object > >> *obj) > >> { > >> - struct reservation_object_list *fobj, *old; > >> - u32 max; > >> + struct reservation_object_list *old, *new; > >> + unsigned int i, j, k, max; > >> old = reservation_object_get_list(obj); > >> if (old && old->shared_max) { > >> - if (old->shared_count < old->shared_max) { > >> - /* perform an in-place update */ > >> - kfree(obj->staged); > >> - obj->staged = NULL; > >> + if (old->shared_count < old->shared_max) > >> return 0; > >> - } else > >> + else > >> max = old->shared_max * 2; > >> - } else > >> - max = 4; > >> - > >> - /* > >> - * resize obj->staged or allocate if it doesn't exist, > >> - * noop if already correct size > >> - */ > >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), > >> shared[max]), > >> - GFP_KERNEL); > >> - if (!fobj) > >> - return -ENOMEM; > >> - > >> - obj->staged = fobj; > >> - fobj->shared_max = max; > >> - return 0; > >> -} > >> -EXPORT_SYMBOL(reservation_object_reserve_shared); > >> - > >> -static void > >> -reservation_object_add_shared_inplace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - struct dma_fence *signaled = NULL; > >> - u32 i, signaled_idx; > >> - > >> - dma_fence_get(fence); > >> - > >> - preempt_disable(); > >> - write_seqcount_begin(&obj->seq); > >> - > >> - for (i = 0; i < fobj->shared_count; ++i) { > >> - struct dma_fence *old_fence; > >> - > >> - old_fence = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (old_fence->context == fence->context) { > >> - /* memory barrier is added by write_seqcount_begin */ > >> - RCU_INIT_POINTER(fobj->shared[i], fence); > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(old_fence); > >> - return; > >> - } > >> - > >> - if (!signaled && dma_fence_is_signaled(old_fence)) { > >> - signaled = old_fence; > >> - signaled_idx = i; > >> - } > >> - } > >> - > >> - /* > >> - * memory barrier is added by write_seqcount_begin, > >> - * fobj->shared_count is protected by this lock too > >> - */ > >> - if (signaled) { > >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > >> } else { > >> - BUG_ON(fobj->shared_count >= fobj->shared_max); > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + max = 4; > >> } > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(signaled); > >> -} > >> - > >> -static void > >> -reservation_object_add_shared_replace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *old, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - unsigned i, j, k; > >> - > >> - dma_fence_get(fence); > >> - > >> - if (!old) { > >> - RCU_INIT_POINTER(fobj->shared[0], fence); > >> - fobj->shared_count = 1; > >> - goto done; > >> - } > >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > >> + if (!new) > >> + return -ENOMEM; > >> /* > >> * no need to bump fence refcounts, rcu_read access @@ -174,46 > >> +92,45 @@ reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> * references from the old struct are carried over to > >> * the new. > >> */ > >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; > >> ++i) { > >> - struct dma_fence *check; > >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); > >> ++i) { > >> + struct dma_fence *fence; > >> - check = rcu_dereference_protected(old->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (check->context == fence->context || > >> - dma_fence_is_signaled(check)) > >> - RCU_INIT_POINTER(fobj->shared[--k], check); > >> + fence = rcu_dereference_protected(old->shared[i], > >> + reservation_object_held(obj)); > >> + if (dma_fence_is_signaled(fence)) > >> + RCU_INIT_POINTER(new->shared[--k], fence); > >> else > >> - RCU_INIT_POINTER(fobj->shared[j++], check); > >> + RCU_INIT_POINTER(new->shared[j++], fence); > >> } > >> - fobj->shared_count = j; > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + new->shared_count = j; > >> + new->shared_max = max; > >> -done: > >> preempt_disable(); > >> write_seqcount_begin(&obj->seq); > >> /* > >> * RCU_INIT_POINTER can be used here, > >> * seqcount provides the necessary barriers > >> */ > >> - RCU_INIT_POINTER(obj->fence, fobj); > >> + RCU_INIT_POINTER(obj->fence, new); > >> write_seqcount_end(&obj->seq); > >> preempt_enable(); > >> if (!old) > >> - return; > >> + return 0; > >> /* Drop the references to the signaled fences */ > >> - for (i = k; i < fobj->shared_max; ++i) { > >> - struct dma_fence *f; > >> + for (i = k; i < new->shared_max; ++i) { > >> + struct dma_fence *fence; > >> - f = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - dma_fence_put(f); > >> + fence = rcu_dereference_protected(new->shared[i], > >> + reservation_object_held(obj)); > >> + dma_fence_put(fence); > >> } > >> kfree_rcu(old, rcu); > >> + > >> + return 0; > >> } > >> +EXPORT_SYMBOL(reservation_object_reserve_shared); > >> /** > >> * reservation_object_add_shared_fence - Add a fence to a shared > >> slot @@ -226,15 +143,39 @@ > >> reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> void reservation_object_add_shared_fence(struct reservation_object > >> *obj, > >> struct dma_fence *fence) > >> { > >> - struct reservation_object_list *old, *fobj = obj->staged; > >> + struct reservation_object_list *fobj; > >> + unsigned int i; > >> - old = reservation_object_get_list(obj); > >> - obj->staged = NULL; > >> + dma_fence_get(fence); > >> + > >> + fobj = reservation_object_get_list(obj); > >> - if (!fobj) > >> - reservation_object_add_shared_inplace(obj, old, fence); > >> - else > >> - reservation_object_add_shared_replace(obj, old, fobj, > >> fence); > >> + preempt_disable(); > >> + write_seqcount_begin(&obj->seq); > >> + > >> + for (i = 0; i < fobj->shared_count; ++i) { > >> + struct dma_fence *old_fence; > >> + > >> + old_fence = rcu_dereference_protected(fobj->shared[i], > >> + reservation_object_held(obj)); > >> + if (old_fence->context == fence->context || > >> + dma_fence_is_signaled(old_fence)) { > >> + dma_fence_put(old_fence); > >> + goto replace; > >> + } > >> + } > >> + > >> + BUG_ON(fobj->shared_count >= fobj->shared_max); > >> + fobj->shared_count++; > >> + > >> +replace: > >> + /* > >> + * memory barrier is added by write_seqcount_begin, > >> + * fobj->shared_count is protected by this lock too > >> + */ > >> + RCU_INIT_POINTER(fobj->shared[i], fence); > >> + write_seqcount_end(&obj->seq); > >> + preempt_enable(); > >> } > >> EXPORT_SYMBOL(reservation_object_add_shared_fence); > >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct > >> reservation_object *dst, > >> new = dma_fence_get_rcu_safe(&src->fence_excl); > >> rcu_read_unlock(); > >> - kfree(dst->staged); > >> - dst->staged = NULL; > >> - > >> src_list = reservation_object_get_list(dst); > >> old = reservation_object_get_excl(dst); > >> diff --git a/include/linux/reservation.h > >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 > >> --- a/include/linux/reservation.h > >> +++ b/include/linux/reservation.h > >> @@ -68,7 +68,6 @@ struct reservation_object_list { > >> * @seq: sequence count for managing RCU read-side synchronization > >> * @fence_excl: the exclusive fence, if there is one currently > >> * @fence: list of current shared fences > >> - * @staged: staged copy of shared fences for RCU updates > >> */ > >> struct reservation_object { > >> struct ww_mutex lock; > >> @@ -76,7 +75,6 @@ struct reservation_object { > >> struct dma_fence __rcu *fence_excl; > >> struct reservation_object_list __rcu *fence; > >> - struct reservation_object_list *staged; > >> }; > >> #define reservation_object_held(obj) > >> lockdep_is_held(&(obj)->lock.base) > >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object > >> *obj) > >> __seqcount_init(&obj->seq, reservation_seqcount_string, > >> &reservation_seqcount_class); > >> RCU_INIT_POINTER(obj->fence, NULL); > >> RCU_INIT_POINTER(obj->fence_excl, NULL); > >> - obj->staged = NULL; > >> } > >> /** > >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object > >> *obj) > >> kfree(fobj); > >> } > >> - kfree(obj->staged); > >> ww_mutex_destroy(&obj->lock); > >> } > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-24 11:10 ` Huang, Ray 0 siblings, 0 replies; 25+ messages in thread From: Huang, Ray @ 2018-10-24 11:10 UTC (permalink / raw) To: Christian König, amd-gfx, dri-devel, linux-media, linaro-mm-sig, Daenzer, Michel Cc: Daniel Vetter, Chris Wilson, Zhang, Jerry Series are Reviewed-by: Huang Rui <ray.huang@amd.com> > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; > Zhang, Jerry <Jerry.Zhang@amd.com> > Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in > reservation object > > Ping once more! Adding a few more AMD people. > > Any comments on this? > > Thanks, > Christian. > > Am 12.10.18 um 10:22 schrieb Christian König: > > Ping! Adding a few people directly and more mailing lists. > > > > Can I get an acked-by/rb for this? It's only a cleanup and not much > > functional change. > > > > Regards, > > Christian. > > > > Am 04.10.2018 um 15:12 schrieb Christian König: > >> No need for that any more. Just replace the list when there isn't > >> enough room any more for the additional fence. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/reservation.c | 178 > >> ++++++++++++++---------------------------- > >> include/linux/reservation.h | 4 - > >> 2 files changed, 58 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/dma-buf/reservation.c > >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 > >> 100644 > >> --- a/drivers/dma-buf/reservation.c > >> +++ b/drivers/dma-buf/reservation.c > >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > >> */ > >> int reservation_object_reserve_shared(struct reservation_object > >> *obj) > >> { > >> - struct reservation_object_list *fobj, *old; > >> - u32 max; > >> + struct reservation_object_list *old, *new; > >> + unsigned int i, j, k, max; > >> old = reservation_object_get_list(obj); > >> if (old && old->shared_max) { > >> - if (old->shared_count < old->shared_max) { > >> - /* perform an in-place update */ > >> - kfree(obj->staged); > >> - obj->staged = NULL; > >> + if (old->shared_count < old->shared_max) > >> return 0; > >> - } else > >> + else > >> max = old->shared_max * 2; > >> - } else > >> - max = 4; > >> - > >> - /* > >> - * resize obj->staged or allocate if it doesn't exist, > >> - * noop if already correct size > >> - */ > >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), > >> shared[max]), > >> - GFP_KERNEL); > >> - if (!fobj) > >> - return -ENOMEM; > >> - > >> - obj->staged = fobj; > >> - fobj->shared_max = max; > >> - return 0; > >> -} > >> -EXPORT_SYMBOL(reservation_object_reserve_shared); > >> - > >> -static void > >> -reservation_object_add_shared_inplace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - struct dma_fence *signaled = NULL; > >> - u32 i, signaled_idx; > >> - > >> - dma_fence_get(fence); > >> - > >> - preempt_disable(); > >> - write_seqcount_begin(&obj->seq); > >> - > >> - for (i = 0; i < fobj->shared_count; ++i) { > >> - struct dma_fence *old_fence; > >> - > >> - old_fence = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (old_fence->context == fence->context) { > >> - /* memory barrier is added by write_seqcount_begin */ > >> - RCU_INIT_POINTER(fobj->shared[i], fence); > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(old_fence); > >> - return; > >> - } > >> - > >> - if (!signaled && dma_fence_is_signaled(old_fence)) { > >> - signaled = old_fence; > >> - signaled_idx = i; > >> - } > >> - } > >> - > >> - /* > >> - * memory barrier is added by write_seqcount_begin, > >> - * fobj->shared_count is protected by this lock too > >> - */ > >> - if (signaled) { > >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > >> } else { > >> - BUG_ON(fobj->shared_count >= fobj->shared_max); > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + max = 4; > >> } > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(signaled); > >> -} > >> - > >> -static void > >> -reservation_object_add_shared_replace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *old, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - unsigned i, j, k; > >> - > >> - dma_fence_get(fence); > >> - > >> - if (!old) { > >> - RCU_INIT_POINTER(fobj->shared[0], fence); > >> - fobj->shared_count = 1; > >> - goto done; > >> - } > >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > >> + if (!new) > >> + return -ENOMEM; > >> /* > >> * no need to bump fence refcounts, rcu_read access @@ -174,46 > >> +92,45 @@ reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> * references from the old struct are carried over to > >> * the new. > >> */ > >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; > >> ++i) { > >> - struct dma_fence *check; > >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); > >> ++i) { > >> + struct dma_fence *fence; > >> - check = rcu_dereference_protected(old->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (check->context == fence->context || > >> - dma_fence_is_signaled(check)) > >> - RCU_INIT_POINTER(fobj->shared[--k], check); > >> + fence = rcu_dereference_protected(old->shared[i], > >> + reservation_object_held(obj)); > >> + if (dma_fence_is_signaled(fence)) > >> + RCU_INIT_POINTER(new->shared[--k], fence); > >> else > >> - RCU_INIT_POINTER(fobj->shared[j++], check); > >> + RCU_INIT_POINTER(new->shared[j++], fence); > >> } > >> - fobj->shared_count = j; > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + new->shared_count = j; > >> + new->shared_max = max; > >> -done: > >> preempt_disable(); > >> write_seqcount_begin(&obj->seq); > >> /* > >> * RCU_INIT_POINTER can be used here, > >> * seqcount provides the necessary barriers > >> */ > >> - RCU_INIT_POINTER(obj->fence, fobj); > >> + RCU_INIT_POINTER(obj->fence, new); > >> write_seqcount_end(&obj->seq); > >> preempt_enable(); > >> if (!old) > >> - return; > >> + return 0; > >> /* Drop the references to the signaled fences */ > >> - for (i = k; i < fobj->shared_max; ++i) { > >> - struct dma_fence *f; > >> + for (i = k; i < new->shared_max; ++i) { > >> + struct dma_fence *fence; > >> - f = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - dma_fence_put(f); > >> + fence = rcu_dereference_protected(new->shared[i], > >> + reservation_object_held(obj)); > >> + dma_fence_put(fence); > >> } > >> kfree_rcu(old, rcu); > >> + > >> + return 0; > >> } > >> +EXPORT_SYMBOL(reservation_object_reserve_shared); > >> /** > >> * reservation_object_add_shared_fence - Add a fence to a shared > >> slot @@ -226,15 +143,39 @@ > >> reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> void reservation_object_add_shared_fence(struct reservation_object > >> *obj, > >> struct dma_fence *fence) > >> { > >> - struct reservation_object_list *old, *fobj = obj->staged; > >> + struct reservation_object_list *fobj; > >> + unsigned int i; > >> - old = reservation_object_get_list(obj); > >> - obj->staged = NULL; > >> + dma_fence_get(fence); > >> + > >> + fobj = reservation_object_get_list(obj); > >> - if (!fobj) > >> - reservation_object_add_shared_inplace(obj, old, fence); > >> - else > >> - reservation_object_add_shared_replace(obj, old, fobj, > >> fence); > >> + preempt_disable(); > >> + write_seqcount_begin(&obj->seq); > >> + > >> + for (i = 0; i < fobj->shared_count; ++i) { > >> + struct dma_fence *old_fence; > >> + > >> + old_fence = rcu_dereference_protected(fobj->shared[i], > >> + reservation_object_held(obj)); > >> + if (old_fence->context == fence->context || > >> + dma_fence_is_signaled(old_fence)) { > >> + dma_fence_put(old_fence); > >> + goto replace; > >> + } > >> + } > >> + > >> + BUG_ON(fobj->shared_count >= fobj->shared_max); > >> + fobj->shared_count++; > >> + > >> +replace: > >> + /* > >> + * memory barrier is added by write_seqcount_begin, > >> + * fobj->shared_count is protected by this lock too > >> + */ > >> + RCU_INIT_POINTER(fobj->shared[i], fence); > >> + write_seqcount_end(&obj->seq); > >> + preempt_enable(); > >> } > >> EXPORT_SYMBOL(reservation_object_add_shared_fence); > >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct > >> reservation_object *dst, > >> new = dma_fence_get_rcu_safe(&src->fence_excl); > >> rcu_read_unlock(); > >> - kfree(dst->staged); > >> - dst->staged = NULL; > >> - > >> src_list = reservation_object_get_list(dst); > >> old = reservation_object_get_excl(dst); > >> diff --git a/include/linux/reservation.h > >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 > >> --- a/include/linux/reservation.h > >> +++ b/include/linux/reservation.h > >> @@ -68,7 +68,6 @@ struct reservation_object_list { > >> * @seq: sequence count for managing RCU read-side synchronization > >> * @fence_excl: the exclusive fence, if there is one currently > >> * @fence: list of current shared fences > >> - * @staged: staged copy of shared fences for RCU updates > >> */ > >> struct reservation_object { > >> struct ww_mutex lock; > >> @@ -76,7 +75,6 @@ struct reservation_object { > >> struct dma_fence __rcu *fence_excl; > >> struct reservation_object_list __rcu *fence; > >> - struct reservation_object_list *staged; > >> }; > >> #define reservation_object_held(obj) > >> lockdep_is_held(&(obj)->lock.base) > >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object > >> *obj) > >> __seqcount_init(&obj->seq, reservation_seqcount_string, > >> &reservation_seqcount_class); > >> RCU_INIT_POINTER(obj->fence, NULL); > >> RCU_INIT_POINTER(obj->fence_excl, NULL); > >> - obj->staged = NULL; > >> } > >> /** > >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object > >> *obj) > >> kfree(fobj); > >> } > >> - kfree(obj->staged); > >> ww_mutex_destroy(&obj->lock); > >> } > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object @ 2018-10-24 11:10 ` Huang, Ray 0 siblings, 0 replies; 25+ messages in thread From: Huang, Ray @ 2018-10-24 11:10 UTC (permalink / raw) To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-media-u79uwXL29TY76Z2rM5mHXA, linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw, Daenzer, Michel Cc: Zhang, Jerry, Daniel Vetter, Chris Wilson Series are Reviewed-by: Huang Rui <ray.huang@amd.com> > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Daenzer, Michel <Michel.Daenzer@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; > Zhang, Jerry <Jerry.Zhang@amd.com> > Subject: Re: [PATCH 1/8] dma-buf: remove shared fence staging in > reservation object > > Ping once more! Adding a few more AMD people. > > Any comments on this? > > Thanks, > Christian. > > Am 12.10.18 um 10:22 schrieb Christian König: > > Ping! Adding a few people directly and more mailing lists. > > > > Can I get an acked-by/rb for this? It's only a cleanup and not much > > functional change. > > > > Regards, > > Christian. > > > > Am 04.10.2018 um 15:12 schrieb Christian König: > >> No need for that any more. Just replace the list when there isn't > >> enough room any more for the additional fence. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/reservation.c | 178 > >> ++++++++++++++---------------------------- > >> include/linux/reservation.h | 4 - > >> 2 files changed, 58 insertions(+), 124 deletions(-) > >> > >> diff --git a/drivers/dma-buf/reservation.c > >> b/drivers/dma-buf/reservation.c index 6c95f61a32e7..5825fc336a13 > >> 100644 > >> --- a/drivers/dma-buf/reservation.c > >> +++ b/drivers/dma-buf/reservation.c > >> @@ -68,105 +68,23 @@ EXPORT_SYMBOL(reservation_seqcount_string); > >> */ > >> int reservation_object_reserve_shared(struct reservation_object > >> *obj) > >> { > >> - struct reservation_object_list *fobj, *old; > >> - u32 max; > >> + struct reservation_object_list *old, *new; > >> + unsigned int i, j, k, max; > >> old = reservation_object_get_list(obj); > >> if (old && old->shared_max) { > >> - if (old->shared_count < old->shared_max) { > >> - /* perform an in-place update */ > >> - kfree(obj->staged); > >> - obj->staged = NULL; > >> + if (old->shared_count < old->shared_max) > >> return 0; > >> - } else > >> + else > >> max = old->shared_max * 2; > >> - } else > >> - max = 4; > >> - > >> - /* > >> - * resize obj->staged or allocate if it doesn't exist, > >> - * noop if already correct size > >> - */ > >> - fobj = krealloc(obj->staged, offsetof(typeof(*fobj), > >> shared[max]), > >> - GFP_KERNEL); > >> - if (!fobj) > >> - return -ENOMEM; > >> - > >> - obj->staged = fobj; > >> - fobj->shared_max = max; > >> - return 0; > >> -} > >> -EXPORT_SYMBOL(reservation_object_reserve_shared); > >> - > >> -static void > >> -reservation_object_add_shared_inplace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - struct dma_fence *signaled = NULL; > >> - u32 i, signaled_idx; > >> - > >> - dma_fence_get(fence); > >> - > >> - preempt_disable(); > >> - write_seqcount_begin(&obj->seq); > >> - > >> - for (i = 0; i < fobj->shared_count; ++i) { > >> - struct dma_fence *old_fence; > >> - > >> - old_fence = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (old_fence->context == fence->context) { > >> - /* memory barrier is added by write_seqcount_begin */ > >> - RCU_INIT_POINTER(fobj->shared[i], fence); > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(old_fence); > >> - return; > >> - } > >> - > >> - if (!signaled && dma_fence_is_signaled(old_fence)) { > >> - signaled = old_fence; > >> - signaled_idx = i; > >> - } > >> - } > >> - > >> - /* > >> - * memory barrier is added by write_seqcount_begin, > >> - * fobj->shared_count is protected by this lock too > >> - */ > >> - if (signaled) { > >> - RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > >> } else { > >> - BUG_ON(fobj->shared_count >= fobj->shared_max); > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + max = 4; > >> } > >> - write_seqcount_end(&obj->seq); > >> - preempt_enable(); > >> - > >> - dma_fence_put(signaled); > >> -} > >> - > >> -static void > >> -reservation_object_add_shared_replace(struct reservation_object > >> *obj, > >> - struct reservation_object_list *old, > >> - struct reservation_object_list *fobj, > >> - struct dma_fence *fence) -{ > >> - unsigned i, j, k; > >> - > >> - dma_fence_get(fence); > >> - > >> - if (!old) { > >> - RCU_INIT_POINTER(fobj->shared[0], fence); > >> - fobj->shared_count = 1; > >> - goto done; > >> - } > >> + new = kmalloc(offsetof(typeof(*new), shared[max]), GFP_KERNEL); > >> + if (!new) > >> + return -ENOMEM; > >> /* > >> * no need to bump fence refcounts, rcu_read access @@ -174,46 > >> +92,45 @@ reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> * references from the old struct are carried over to > >> * the new. > >> */ > >> - for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; > >> ++i) { > >> - struct dma_fence *check; > >> + for (i = 0, j = 0, k = max; i < (old ? old->shared_count : 0); > >> ++i) { > >> + struct dma_fence *fence; > >> - check = rcu_dereference_protected(old->shared[i], > >> - reservation_object_held(obj)); > >> - > >> - if (check->context == fence->context || > >> - dma_fence_is_signaled(check)) > >> - RCU_INIT_POINTER(fobj->shared[--k], check); > >> + fence = rcu_dereference_protected(old->shared[i], > >> + reservation_object_held(obj)); > >> + if (dma_fence_is_signaled(fence)) > >> + RCU_INIT_POINTER(new->shared[--k], fence); > >> else > >> - RCU_INIT_POINTER(fobj->shared[j++], check); > >> + RCU_INIT_POINTER(new->shared[j++], fence); > >> } > >> - fobj->shared_count = j; > >> - RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > >> - fobj->shared_count++; > >> + new->shared_count = j; > >> + new->shared_max = max; > >> -done: > >> preempt_disable(); > >> write_seqcount_begin(&obj->seq); > >> /* > >> * RCU_INIT_POINTER can be used here, > >> * seqcount provides the necessary barriers > >> */ > >> - RCU_INIT_POINTER(obj->fence, fobj); > >> + RCU_INIT_POINTER(obj->fence, new); > >> write_seqcount_end(&obj->seq); > >> preempt_enable(); > >> if (!old) > >> - return; > >> + return 0; > >> /* Drop the references to the signaled fences */ > >> - for (i = k; i < fobj->shared_max; ++i) { > >> - struct dma_fence *f; > >> + for (i = k; i < new->shared_max; ++i) { > >> + struct dma_fence *fence; > >> - f = rcu_dereference_protected(fobj->shared[i], > >> - reservation_object_held(obj)); > >> - dma_fence_put(f); > >> + fence = rcu_dereference_protected(new->shared[i], > >> + reservation_object_held(obj)); > >> + dma_fence_put(fence); > >> } > >> kfree_rcu(old, rcu); > >> + > >> + return 0; > >> } > >> +EXPORT_SYMBOL(reservation_object_reserve_shared); > >> /** > >> * reservation_object_add_shared_fence - Add a fence to a shared > >> slot @@ -226,15 +143,39 @@ > >> reservation_object_add_shared_replace(struct > >> reservation_object *obj, > >> void reservation_object_add_shared_fence(struct reservation_object > >> *obj, > >> struct dma_fence *fence) > >> { > >> - struct reservation_object_list *old, *fobj = obj->staged; > >> + struct reservation_object_list *fobj; > >> + unsigned int i; > >> - old = reservation_object_get_list(obj); > >> - obj->staged = NULL; > >> + dma_fence_get(fence); > >> + > >> + fobj = reservation_object_get_list(obj); > >> - if (!fobj) > >> - reservation_object_add_shared_inplace(obj, old, fence); > >> - else > >> - reservation_object_add_shared_replace(obj, old, fobj, > >> fence); > >> + preempt_disable(); > >> + write_seqcount_begin(&obj->seq); > >> + > >> + for (i = 0; i < fobj->shared_count; ++i) { > >> + struct dma_fence *old_fence; > >> + > >> + old_fence = rcu_dereference_protected(fobj->shared[i], > >> + reservation_object_held(obj)); > >> + if (old_fence->context == fence->context || > >> + dma_fence_is_signaled(old_fence)) { > >> + dma_fence_put(old_fence); > >> + goto replace; > >> + } > >> + } > >> + > >> + BUG_ON(fobj->shared_count >= fobj->shared_max); > >> + fobj->shared_count++; > >> + > >> +replace: > >> + /* > >> + * memory barrier is added by write_seqcount_begin, > >> + * fobj->shared_count is protected by this lock too > >> + */ > >> + RCU_INIT_POINTER(fobj->shared[i], fence); > >> + write_seqcount_end(&obj->seq); > >> + preempt_enable(); > >> } > >> EXPORT_SYMBOL(reservation_object_add_shared_fence); > >> @@ -343,9 +284,6 @@ int reservation_object_copy_fences(struct > >> reservation_object *dst, > >> new = dma_fence_get_rcu_safe(&src->fence_excl); > >> rcu_read_unlock(); > >> - kfree(dst->staged); > >> - dst->staged = NULL; > >> - > >> src_list = reservation_object_get_list(dst); > >> old = reservation_object_get_excl(dst); > >> diff --git a/include/linux/reservation.h > >> b/include/linux/reservation.h index 02166e815afb..54cf6773a14c 100644 > >> --- a/include/linux/reservation.h > >> +++ b/include/linux/reservation.h > >> @@ -68,7 +68,6 @@ struct reservation_object_list { > >> * @seq: sequence count for managing RCU read-side synchronization > >> * @fence_excl: the exclusive fence, if there is one currently > >> * @fence: list of current shared fences > >> - * @staged: staged copy of shared fences for RCU updates > >> */ > >> struct reservation_object { > >> struct ww_mutex lock; > >> @@ -76,7 +75,6 @@ struct reservation_object { > >> struct dma_fence __rcu *fence_excl; > >> struct reservation_object_list __rcu *fence; > >> - struct reservation_object_list *staged; > >> }; > >> #define reservation_object_held(obj) > >> lockdep_is_held(&(obj)->lock.base) > >> @@ -95,7 +93,6 @@ reservation_object_init(struct reservation_object > >> *obj) > >> __seqcount_init(&obj->seq, reservation_seqcount_string, > >> &reservation_seqcount_class); > >> RCU_INIT_POINTER(obj->fence, NULL); > >> RCU_INIT_POINTER(obj->fence_excl, NULL); > >> - obj->staged = NULL; > >> } > >> /** > >> @@ -124,7 +121,6 @@ reservation_object_fini(struct reservation_object > >> *obj) > >> kfree(fobj); > >> } > >> - kfree(obj->staged); > >> ww_mutex_destroy(&obj->lock); > >> } > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object 2018-10-04 13:12 [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Christian König ` (3 preceding siblings ...) 2018-10-12 8:22 ` Christian König @ 2018-10-25 20:15 ` Chris Wilson 2018-10-25 20:20 ` Chris Wilson 4 siblings, 1 reply; 25+ messages in thread From: Chris Wilson @ 2018-10-25 20:15 UTC (permalink / raw) To: Christian König, amd-gfx, dri-devel Quoting Christian König (2018-10-04 14:12:43) > No need for that any more. Just replace the list when there isn't enough > room any more for the additional fence. Just a heads up. After this series landed, we started hitting a use-after-free when iterating the shared list. <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 -Chris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object 2018-10-25 20:15 ` Chris Wilson @ 2018-10-25 20:20 ` Chris Wilson 2018-10-25 21:21 ` Chris Wilson 0 siblings, 1 reply; 25+ messages in thread From: Chris Wilson @ 2018-10-25 20:20 UTC (permalink / raw) To: Christian König, amd-gfx, dri-devel Quoting Chris Wilson (2018-10-25 21:15:17) > Quoting Christian König (2018-10-04 14:12:43) > > No need for that any more. Just replace the list when there isn't enough > > room any more for the additional fence. > > Just a heads up. After this series landed, we started hitting a > use-after-free when iterating the shared list. > > <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI > <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 > <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 > <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] > <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 > <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 > <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 > <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 > <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 > <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 > <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b > <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 > <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 First guess would be diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 5fb4fd461908..833698a0d548 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, } BUG_ON(fobj->shared_count >= fobj->shared_max); - fobj->shared_count++; replace: /* @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, * fobj->shared_count is protected by this lock too */ RCU_INIT_POINTER(fobj->shared[i], fence); + if (i == fobj->shared_count) + fobj->shared_count++; write_seqcount_end(&obj->seq); preempt_enable(); } Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ? -Chris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object 2018-10-25 20:20 ` Chris Wilson @ 2018-10-25 21:21 ` Chris Wilson 2018-10-26 8:01 ` Christian König 0 siblings, 1 reply; 25+ messages in thread From: Chris Wilson @ 2018-10-25 21:21 UTC (permalink / raw) To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Quoting Chris Wilson (2018-10-25 21:20:21) > Quoting Chris Wilson (2018-10-25 21:15:17) > > Quoting Christian König (2018-10-04 14:12:43) > > > No need for that any more. Just replace the list when there isn't enough > > > room any more for the additional fence. > > > > Just a heads up. After this series landed, we started hitting a > > use-after-free when iterating the shared list. > > > > <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI > > <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 > > <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 > > <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] > > <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 > > <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 > > <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 > > <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 > > <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 > > <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 > > <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b > > <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 > > <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 > > First guess would be > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 5fb4fd461908..833698a0d548 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, > } > > BUG_ON(fobj->shared_count >= fobj->shared_max); > - fobj->shared_count++; > > replace: > /* > @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, > * fobj->shared_count is protected by this lock too > */ > RCU_INIT_POINTER(fobj->shared[i], fence); > + if (i == fobj->shared_count) > + fobj->shared_count++; > write_seqcount_end(&obj->seq); > preempt_enable(); > } > > Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ? Updating shared_count after setting the fence does the trick. -Chris _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object 2018-10-25 21:21 ` Chris Wilson @ 2018-10-26 8:01 ` Christian König 0 siblings, 0 replies; 25+ messages in thread From: Christian König @ 2018-10-26 8:01 UTC (permalink / raw) To: Chris Wilson, amd-gfx, dri-devel Am 25.10.18 um 23:21 schrieb Chris Wilson: > Quoting Chris Wilson (2018-10-25 21:20:21) >> Quoting Chris Wilson (2018-10-25 21:15:17) >>> Quoting Christian König (2018-10-04 14:12:43) >>>> No need for that any more. Just replace the list when there isn't enough >>>> room any more for the additional fence. >>> Just a heads up. After this series landed, we started hitting a >>> use-after-free when iterating the shared list. >>> >>> <4> [109.613162] general protection fault: 0000 [#1] PREEMPT SMP PTI >>> <4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U 4.19.0-rc8-CI-CI_DRM_5035+ #1 >>> <4> [109.613189] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011 >>> <4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915] >>> <4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00 >>> <4> [109.613283] RSP: 0018:ffffc9000044bcf8 EFLAGS: 00010246 >>> <4> [109.613292] RAX: 0000000000000000 RBX: ffffc9000044bdc0 RCX: 0000000000000001 >>> <4> [109.613302] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffffffff822474a0 >>> <4> [109.613311] RBP: ffffc9000044bd28 R08: ffff88021e158680 R09: 0000000000000001 >>> <4> [109.613321] R10: 0000000000000040 R11: 0000000000000000 R12: ffff88021e1641b8 >>> <4> [109.613331] R13: 0000000000000003 R14: ffff88021e1641b0 R15: 6b6b6b6b6b6b6b6b >>> <4> [109.613341] FS: 00007f9c9fc84980(0000) GS:ffff880227a40000(0000) knlGS:0000000000000000 >>> <4> [109.613352] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> <4> [109.613360] CR2: 00007f9c9fcb8000 CR3: 00000002247d4005 CR4: 00000000000606e0 >> First guess would be >> >> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >> index 5fb4fd461908..833698a0d548 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, >> } >> >> BUG_ON(fobj->shared_count >= fobj->shared_max); >> - fobj->shared_count++; >> >> replace: >> /* >> @@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, >> * fobj->shared_count is protected by this lock too >> */ >> RCU_INIT_POINTER(fobj->shared[i], fence); >> + if (i == fobj->shared_count) >> + fobj->shared_count++; >> write_seqcount_end(&obj->seq); >> preempt_enable(); >> } >> >> Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ? > Updating shared_count after setting the fence does the trick. Ah, crap. I can stare at the code for ages and don't see that problem. Neither did any internal testing showed any issues. Anyway your change is Reviewed-by: Christian König <christian.koenig@amd.com> Thanks for the quick fix, Christian. > -Chris _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-10-26 8:01 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-04 13:12 [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Christian König 2018-10-04 13:12 ` [PATCH 4/8] drm/ttm: allow reserving more than one shared slot v2 Christian König [not found] ` <20181004131250.2373-1-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-04 13:12 ` [PATCH 2/8] dma-buf: allow reserving more than one shared fence slot Christian König 2018-10-04 13:12 ` [PATCH 3/8] dma-buf: test shared slot allocation when mutex debugging is active Christian König 2018-10-04 13:12 ` [PATCH 5/8] drm/amdgpu: fix using shared fence for exported BOs v2 Christian König [not found] ` <20181004131250.2373-5-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-23 13:40 ` Michel Dänzer 2018-10-04 13:12 ` [PATCH 6/8] drm/amdgpu: always reserve two slots for the VM Christian König [not found] ` <20181004131250.2373-6-christian.koenig-5C7GfCeVMHo@public.gmane.org> 2018-10-24 5:27 ` Zhang, Jerry(Junwei) 2018-10-04 13:12 ` [PATCH 8/8] drm/ttm: drop the extra reservation for pipelined BO moves Christian König 2018-10-24 5:25 ` [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Zhang, Jerry(Junwei) 2018-10-04 13:12 ` [PATCH 7/8] drm/amdgpu: always reserve one more shared slot for pipelined BO moves Christian König 2018-10-12 8:22 ` [PATCH 1/8] dma-buf: remove shared fence staging in reservation object Christian König 2018-10-12 8:22 ` Christian König 2018-10-23 12:20 ` Christian König 2018-10-23 12:20 ` Christian König 2018-10-23 13:40 ` Michel Dänzer 2018-10-23 13:40 ` Michel Dänzer 2018-10-24 5:12 ` Huang, Ray 2018-10-24 5:12 ` Huang, Ray 2018-10-24 11:10 ` Huang, Ray 2018-10-24 11:10 ` Huang, Ray 2018-10-25 20:15 ` Chris Wilson 2018-10-25 20:20 ` Chris Wilson 2018-10-25 21:21 ` Chris Wilson 2018-10-26 8:01 ` Christian König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.