Christian, I will take a look at them later. Thanks, Ray > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@xxxxxxxxx] > Sent: Tuesday, October 23, 2018 8:20 PM > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux- > media@xxxxxxxxxxxxxxx; linaro-mm-sig@xxxxxxxxxxxxxxxx; Huang, Ray > <Ray.Huang@xxxxxxx>; Daenzer, Michel <Michel.Daenzer@xxxxxxx> > Cc: Daniel Vetter <daniel@xxxxxxxx>; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; > Zhang, Jerry <Jerry.Zhang@xxxxxxx> > 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@xxxxxxx> > >> --- > >> 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); > >> } > >
