| Message ID | 1463605193-18040-1-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 18, 2016 at 05:59:52PM -0300, Gustavo Padovan wrote: > +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb) > +{ > + struct fence_collection_cb *f_cb; > + struct fence_collection *collection; > + > + f_cb = container_of(cb, struct fence_collection_cb, cb); > + collection = f_cb->collection; > + > + if (atomic_dec_and_test(&collection->num_pending_fences)) > + fence_signal(&collection->base); > +} > + > +static bool fence_collection_enable_signaling(struct fence *fence) > +{ > + struct fence_collection *collection = to_fence_collection(fence); > + int i; > + > + for (i = 0 ; i < collection->num_fences ; i++) { > + if (fence_add_callback(collection->fences[i].fence, > + &collection->fences[i].cb, > + collection_check_cb_func)) { > + atomic_dec(&collection->num_pending_fences); > + } > + } We don't always have a convenient means to preallocate an array of fences to use. Keeping a list of fences in addition to the array would be easier to user in many circumstances. Just means we need a struct fence_collection_entry { struct fence *fence; struct list_head link; }; int fence_collection_add(struct fence *_collection, struct fence *fence) { struct fence_collection *collection = to_fence_collection(_collection); struct fence_collection_entry *entry; entry = kmalloc(sizeof(*entry), GFP_KERNEL); if (!entry) return -ENOMEM; entry->fence = fence_get(fence); list_add_tail(&entry->link, &collection->fence_list); atomic_inc(&collection->num_pending_fences); return 0; } and a couple of list iterations as well as walking the arrays. (This fence_collection_add() needs to be documented to only be valid from the constructing thread before the fence is sealed for export/use.) -Chris
Am 19.05.2016 um 00:57 schrieb Chris Wilson: > On Wed, May 18, 2016 at 05:59:52PM -0300, Gustavo Padovan wrote: >> +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb) >> +{ >> + struct fence_collection_cb *f_cb; >> + struct fence_collection *collection; >> + >> + f_cb = container_of(cb, struct fence_collection_cb, cb); >> + collection = f_cb->collection; >> + >> + if (atomic_dec_and_test(&collection->num_pending_fences)) >> + fence_signal(&collection->base); >> +} >> + >> +static bool fence_collection_enable_signaling(struct fence *fence) >> +{ >> + struct fence_collection *collection = to_fence_collection(fence); >> + int i; >> + >> + for (i = 0 ; i < collection->num_fences ; i++) { >> + if (fence_add_callback(collection->fences[i].fence, >> + &collection->fences[i].cb, >> + collection_check_cb_func)) { >> + atomic_dec(&collection->num_pending_fences); >> + } >> + } > We don't always have a convenient means to preallocate an array of > fences to use. Keeping a list of fences in addition to the array would > be easier to user in many circumstances. I agree that there is use for such an implementation as well, but as mentioned in the last review cycle we intentionally chose an array instead of a more complex implementation here. This way the array can be passed to function like fence_wait_any_timeout() as well. I also suggested to rename it to fence_array to make that difference clear and allow for another implementation to live side by side with this. My crux at the moment is that I need both for the amdgpu driver, an array based implementation and a collection like one. Gustavo would you mind if I take your patches and work a bit on this? > > Just means we need a > > struct fence_collection_entry { > struct fence *fence; > struct list_head link; > }; > > int fence_collection_add(struct fence *_collection, > struct fence *fence) > { > struct fence_collection *collection = > to_fence_collection(_collection); > struct fence_collection_entry *entry; > > entry = kmalloc(sizeof(*entry), GFP_KERNEL); > if (!entry) > return -ENOMEM; > > entry->fence = fence_get(fence); > list_add_tail(&entry->link, &collection->fence_list); > atomic_inc(&collection->num_pending_fences); > > return 0; > } > > and a couple of list iterations as well as walking the arrays. > > (This fence_collection_add() needs to be documented to only be valid > from the constructing thread before the fence is sealed for export/use.) As suggested by Daniel as well I would prefer that the the array implementation only gets the fences as already filled array in the constructor. This is much more fail save. Christian. > -Chris >
On Thu, May 19, 2016 at 09:46:47AM +0200, Christian König wrote: > Am 19.05.2016 um 00:57 schrieb Chris Wilson: > >On Wed, May 18, 2016 at 05:59:52PM -0300, Gustavo Padovan wrote: > >>+static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb) > >>+{ > >>+ struct fence_collection_cb *f_cb; > >>+ struct fence_collection *collection; > >>+ > >>+ f_cb = container_of(cb, struct fence_collection_cb, cb); > >>+ collection = f_cb->collection; > >>+ > >>+ if (atomic_dec_and_test(&collection->num_pending_fences)) > >>+ fence_signal(&collection->base); > >>+} > >>+ > >>+static bool fence_collection_enable_signaling(struct fence *fence) > >>+{ > >>+ struct fence_collection *collection = to_fence_collection(fence); > >>+ int i; > >>+ > >>+ for (i = 0 ; i < collection->num_fences ; i++) { > >>+ if (fence_add_callback(collection->fences[i].fence, > >>+ &collection->fences[i].cb, > >>+ collection_check_cb_func)) { > >>+ atomic_dec(&collection->num_pending_fences); > >>+ } > >>+ } > >We don't always have a convenient means to preallocate an array of > >fences to use. Keeping a list of fences in addition to the array would > >be easier to user in many circumstances. > > I agree that there is use for such an implementation as well, but as > mentioned in the last review cycle we intentionally chose an array instead > of a more complex implementation here. > > This way the array can be passed to function like fence_wait_any_timeout() > as well. +1 on fence_array. > I also suggested to rename it to fence_array to make that difference clear > and allow for another implementation to live side by side with this. > > My crux at the moment is that I need both for the amdgpu driver, an array > based implementation and a collection like one. > > Gustavo would you mind if I take your patches and work a bit on this? I think the goal is to start landing the atomic fence stuff in 4.8. Probably simplest if we converge on a first iteration that I can pull into drm-misc right after 4.7-rc1. Then you can both base your respective work on top of that branch (it's a stable one, so can even base official branches on it). -Daniel
Hi Christian, 2016-05-19 Christian König <christian.koenig@amd.com>: > Am 19.05.2016 um 00:57 schrieb Chris Wilson: > > On Wed, May 18, 2016 at 05:59:52PM -0300, Gustavo Padovan wrote: > > > +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb) > > > +{ > > > + struct fence_collection_cb *f_cb; > > > + struct fence_collection *collection; > > > + > > > + f_cb = container_of(cb, struct fence_collection_cb, cb); > > > + collection = f_cb->collection; > > > + > > > + if (atomic_dec_and_test(&collection->num_pending_fences)) > > > + fence_signal(&collection->base); > > > +} > > > + > > > +static bool fence_collection_enable_signaling(struct fence *fence) > > > +{ > > > + struct fence_collection *collection = to_fence_collection(fence); > > > + int i; > > > + > > > + for (i = 0 ; i < collection->num_fences ; i++) { > > > + if (fence_add_callback(collection->fences[i].fence, > > > + &collection->fences[i].cb, > > > + collection_check_cb_func)) { > > > + atomic_dec(&collection->num_pending_fences); > > > + } > > > + } > > We don't always have a convenient means to preallocate an array of > > fences to use. Keeping a list of fences in addition to the array would > > be easier to user in many circumstances. > > I agree that there is use for such an implementation as well, but as > mentioned in the last review cycle we intentionally chose an array instead > of a more complex implementation here. > > This way the array can be passed to function like fence_wait_any_timeout() > as well. > > I also suggested to rename it to fence_array to make that difference clear > and allow for another implementation to live side by side with this. > > My crux at the moment is that I need both for the amdgpu driver, an array > based implementation and a collection like one. > > Gustavo would you mind if I take your patches and work a bit on this? Please go ahead with this and let me know if you need anything from me. Gustavo
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index d2d02fc..a8e8ecb 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,3 +1,3 @@ -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c new file mode 100644 index 0000000..432eb6b --- /dev/null +++ b/drivers/dma-buf/fence-collection.c @@ -0,0 +1,139 @@ +/* + * fence-collection: aggregate fences to be waited together + * + * Copyright (C) 2016 Collabora Ltd + * Authors: + * Gustavo Padovan <gustavo@padovan.org> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ + +#include <linux/export.h> +#include <linux/slab.h> +#include <linux/fence-collection.h> + +static const char *fence_collection_get_driver_name(struct fence *fence) +{ + struct fence_collection *collection = to_fence_collection(fence); + struct fence *f = collection->fences[0].fence; + + return f->ops->get_driver_name(fence); +} + +static const char *fence_collection_get_timeline_name(struct fence *fence) +{ + return "unbound"; +} + +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb) +{ + struct fence_collection_cb *f_cb; + struct fence_collection *collection; + + f_cb = container_of(cb, struct fence_collection_cb, cb); + collection = f_cb->collection; + + if (atomic_dec_and_test(&collection->num_pending_fences)) + fence_signal(&collection->base); +} + +static bool fence_collection_enable_signaling(struct fence *fence) +{ + struct fence_collection *collection = to_fence_collection(fence); + int i; + + for (i = 0 ; i < collection->num_fences ; i++) { + if (fence_add_callback(collection->fences[i].fence, + &collection->fences[i].cb, + collection_check_cb_func)) { + atomic_dec(&collection->num_pending_fences); + } + } + + return atomic_read(&collection->num_pending_fences); +} + +static bool fence_collection_signaled(struct fence *fence) +{ + struct fence_collection *collection = to_fence_collection(fence); + + return atomic_read(&collection->num_pending_fences) == 0; +} + +static void fence_collection_release(struct fence *fence) +{ + struct fence_collection *collection = to_fence_collection(fence); + int i; + + for (i = 0 ; i < collection->num_fences ; i++) { + fence_remove_callback(collection->fences[i].fence, + &collection->fences[i].cb); + fence_put(collection->fences[i].fence); + } + + kfree(collection->fences); + fence_free(fence); +} + +const struct fence_ops fence_collection_ops = { + .get_driver_name = fence_collection_get_driver_name, + .get_timeline_name = fence_collection_get_timeline_name, + .enable_signaling = fence_collection_enable_signaling, + .signaled = fence_collection_signaled, + .wait = fence_default_wait, + .release = fence_collection_release, +}; + +/** + * fence_collection_create - Create a custom fence collection + * is signaled + * @num_fences: [in] number of fences to add in the collection + * @cb: [in] cb array containing the fences for the collection + * + * Allocate a fence_collection and initialize the base fence with fence_init() + * and FENCE_NO_CONTEXT. It also inits all fence_collections specific fields + * and return the created fence_collection. In case of error it returns NULL. + * + * The caller should allocte the fence_collection_cb array with num_fences size + * and fill the fence fields with all the fences it wants to add to the + * collection. + * + * Collections have no context, thus fence_later() and fence_is_later() shall + * not be used with fence_collection. + * + * fence_put() on the base fence should be used to release the collection. + */ +struct fence_collection *fence_collection_create(int num_fences, + struct fence_collection_cb *cb) +{ + struct fence_collection *collection; + int i; + + collection = kzalloc(sizeof(*collection), GFP_KERNEL); + if (!collection) + return NULL; + + spin_lock_init(&collection->lock); + fence_init(&collection->base, &fence_collection_ops, &collection->lock, + FENCE_NO_CONTEXT, 0); + + collection->fences = cb; + collection->num_fences = num_fences; + atomic_set(&collection->num_pending_fences, num_fences); + + for (i = 0 ; i < collection->num_fences ; i++) { + fence_get(collection->fences[i].fence); + collection->fences[i].collection = collection; + INIT_LIST_HEAD(&collection->fences[i].cb.node); + } + + return collection; +} +EXPORT_SYMBOL(fence_collection_create); diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c index 7b05dbe..486e95c 100644 --- a/drivers/dma-buf/fence.c +++ b/drivers/dma-buf/fence.c @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit); * context or not. One device can have multiple separate contexts, * and they're used if some engine can run independently of another. */ -static atomic_t fence_context_counter = ATOMIC_INIT(0); +static atomic_t fence_context_counter = ATOMIC_INIT(1); /** * fence_context_alloc - allocate an array of fence contexts diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h new file mode 100644 index 0000000..4087622 --- /dev/null +++ b/include/linux/fence-collection.h @@ -0,0 +1,73 @@ +/* + * fence-collection: aggregates fence to be waited together + * + * Copyright (C) 2016 Collabora Ltd + * Authors: + * Gustavo Padovan <gustavo@padovan.org> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ + +#ifndef __LINUX_FENCE_COLLECTION_H +#define __LINUX_FENCE_COLLECTION_H + +#include <linux/fence.h> + +/** + * struct fence_collection_cb - callback for fence_add_callback + * @cb: the base fence_cb object + * @fence: fence we are storing in the collection + * @collection: collection the fence belongs to + * + * This struct is at the same time the storage for the fences in the + * collection and the struct we pass to fence_add_callback() + */ +struct fence_collection_cb { + struct fence_cb cb; + struct fence *fence; + struct fence_collection *collection; +}; + +/** + * struct fence_collection - fence to represent a collection of fences + * @base: fence base class + * @lock: spinlock for fence handling + * @num_pending_fences: fences in the collection not signaled yet + * @num_fences: number of fences in the collection + * @fences: ponteir to fence_collection_cb with data about the fences + */ +struct fence_collection { + struct fence base; + + spinlock_t lock; + atomic_t num_pending_fences; + int num_fences; + struct fence_collection_cb *fences; +}; + +extern const struct fence_ops fence_collection_ops; + +/** + * to_fence_collection - cast a fence to a fence_collection + * @fence: fence to cast to a fence_collection + * + * Returns NULL if the fence is not a fence_collection, + * or the fence_collection otherwise. + */ +static inline struct fence_collection *to_fence_collection(struct fence *fence) +{ + if (fence->ops != &fence_collection_ops) + return NULL; + return container_of(fence, struct fence_collection, base); +} + +struct fence_collection *fence_collection_create(int num_fences, + struct fence_collection_cb *cb); +#endif /* __LINUX_FENCE_COLLECTION_H */ diff --git a/include/linux/fence.h b/include/linux/fence.h index e7c906e..b56b31e 100644 --- a/include/linux/fence.h +++ b/include/linux/fence.h @@ -30,6 +30,9 @@ #include <linux/printk.h> #include <linux/rcupdate.h> +/* context number for fences without context, e.g: fence_collection */ +#define FENCE_NO_CONTEXT 0 + struct fence; struct fence_ops; struct fence_cb; @@ -304,6 +307,9 @@ static inline bool fence_is_later(struct fence *f1, struct fence *f2) if (WARN_ON(f1->context != f2->context)) return false; + if (WARN_ON(f1->context == FENCE_NO_CONTEXT)) + return false; + return (int)(f1->seqno - f2->seqno) > 0; } @@ -321,6 +327,9 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2) if (WARN_ON(f1->context != f2->context)) return NULL; + if (WARN_ON(f1->context == FENCE_NO_CONTEXT)) + return NULL; + /* * can't check just FENCE_FLAG_SIGNALED_BIT here, it may never have been * set if enable_signaling wasn't called, and enabling that here is