All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 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 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

* [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

* [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(&params, 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

* [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

* [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>
@ 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 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

* 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
       [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

* 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(&params, 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

* 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.