| Message ID | 1464002722-4989-3-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote: > From: Christian König <christian.koenig@amd.com> > > If @signal_on_any is true the fence array signals if any fence in the array > signals, otherwise it signals when all fences in the array signal. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/fence-array.c | 23 ++++++++++++++++------- > include/linux/fence-array.h | 3 ++- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c > index bbcf79f..726e840 100644 > --- a/drivers/dma-buf/fence-array.c > +++ b/drivers/dma-buf/fence-array.c > @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb) > > if (atomic_dec_and_test(&array->num_pending)) > fence_signal(&array->base); > + fence_put(&array->base); > } > > static bool fence_array_enable_signaling(struct fence *fence) > @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence) > > for (i = 0; i < array->num_fences; ++i) { > cb[i].array = array; > + fence_get(&array->base); > if (!fence_add_callback(array->fences[i], &cb[i].cb, > - fence_array_cb_func)) > + fence_array_cb_func)) { > + fence_put(&array->base); Ok, yes, that is an artifact of reporting signaled before all cb are complete. /* As we may report that the fence is signaled before all callbacks are * complete, we need to take an additional reference count on the array * so that we do not free it too early. The core fence handling will * only hold the reference until we signal the array as complete (but * that is now insufficient). */ fence_array_signaled() should now be return atomic_read(&array->num_pending) <= 0; -Chris
On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote: > From: Christian König <christian.koenig@amd.com> > > If @signal_on_any is true the fence array signals if any fence in the array > signals, otherwise it signals when all fences in the array signal. What do you need this for? Sounds neat, but I can't come up with a use case. And that use case imo should be both in the commit message and kerneldoc. -Daniel > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/fence-array.c | 23 ++++++++++++++++------- > include/linux/fence-array.h | 3 ++- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c > index bbcf79f..726e840 100644 > --- a/drivers/dma-buf/fence-array.c > +++ b/drivers/dma-buf/fence-array.c > @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb) > > if (atomic_dec_and_test(&array->num_pending)) > fence_signal(&array->base); > + fence_put(&array->base); > } > > static bool fence_array_enable_signaling(struct fence *fence) > @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence) > > for (i = 0; i < array->num_fences; ++i) { > cb[i].array = array; > + fence_get(&array->base); > if (!fence_add_callback(array->fences[i], &cb[i].cb, > - fence_array_cb_func)) > + fence_array_cb_func)) { > + fence_put(&array->base); > if (atomic_dec_and_test(&array->num_pending)) > return false; > + } > } > > return true; > @@ -90,10 +94,11 @@ const struct fence_ops fence_array_ops = { > > /** > * fence_array_create - Create a custom fence array > - * @num_fences: [in] number of fences to add in the array > - * @fences: [in] array containing the fences > - * @context: [in] fence context to use > - * @seqno: [in] sequence number to use > + * @num_fences: [in] number of fences to add in the array > + * @fences: [in] array containing the fences > + * @context: [in] fence context to use > + * @seqno: [in] sequence number to use > + * @signal_on_any [in] signal on any fence in the array > * > * Allocate a fence_array object and initialize the base fence with fence_init(). > * In case of error it returns NULL. > @@ -101,9 +106,13 @@ const struct fence_ops fence_array_ops = { > * The caller should allocte the fences array with num_fences size > * and fill it with the fences it wants to add to the object. Ownership of this > * array is take and fence_put() is used on each fence on release. > + * > + * If @signal_on_any is true the fence array signals if any fence in the array > + * signals, otherwise it signals when all fences in the array signal. > */ > struct fence_array *fence_array_create(int num_fences, struct fence **fences, > - u64 context, unsigned seqno) > + u64 context, unsigned seqno, > + bool signal_on_any) > { > struct fence_array *array; > size_t size = sizeof(*array); > @@ -119,7 +128,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences, > context, seqno); > > array->num_fences = num_fences; > - atomic_set(&array->num_pending, num_fences); > + atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); > array->fences = fences; > > return array; > diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h > index 593ab98..86baaa4 100644 > --- a/include/linux/fence-array.h > +++ b/include/linux/fence-array.h > @@ -67,6 +67,7 @@ static inline struct fence_array *to_fence_array(struct fence *fence) > } > > struct fence_array *fence_array_create(int num_fences, struct fence **fences, > - u64 context, unsigned seqno); > + u64 context, unsigned seqno, > + bool signal_on_any); > > #endif /* __LINUX_FENCE_ARRAY_H */ > -- > 2.5.0 >
Am 23.05.2016 um 15:50 schrieb Daniel Vetter: > On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote: >> From: Christian König <christian.koenig@amd.com> >> >> If @signal_on_any is true the fence array signals if any fence in the array >> signals, otherwise it signals when all fences in the array signal. > What do you need this for? Sounds neat, but I can't come up with a use > case. And that use case imo should be both in the commit message and > kerneldoc. Good point, going to fix this when if we really use the patch. Just wanted to get opinions on this. I'm currently working on making some of the resource management in amdgpu more friendly to our GPU scheduler. Waiting for resources in the GPU scheduler is implemented as waiting for a fence to fire. So when you want to wait for any resource to become available (like in our case the VMIDs) you could make good use of something like this. Christian. > -Daniel > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/fence-array.c | 23 ++++++++++++++++------- >> include/linux/fence-array.h | 3 ++- >> 2 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c >> index bbcf79f..726e840 100644 >> --- a/drivers/dma-buf/fence-array.c >> +++ b/drivers/dma-buf/fence-array.c >> @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb) >> >> if (atomic_dec_and_test(&array->num_pending)) >> fence_signal(&array->base); >> + fence_put(&array->base); >> } >> >> static bool fence_array_enable_signaling(struct fence *fence) >> @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence) >> >> for (i = 0; i < array->num_fences; ++i) { >> cb[i].array = array; >> + fence_get(&array->base); >> if (!fence_add_callback(array->fences[i], &cb[i].cb, >> - fence_array_cb_func)) >> + fence_array_cb_func)) { >> + fence_put(&array->base); >> if (atomic_dec_and_test(&array->num_pending)) >> return false; >> + } >> } >> >> return true; >> @@ -90,10 +94,11 @@ const struct fence_ops fence_array_ops = { >> >> /** >> * fence_array_create - Create a custom fence array >> - * @num_fences: [in] number of fences to add in the array >> - * @fences: [in] array containing the fences >> - * @context: [in] fence context to use >> - * @seqno: [in] sequence number to use >> + * @num_fences: [in] number of fences to add in the array >> + * @fences: [in] array containing the fences >> + * @context: [in] fence context to use >> + * @seqno: [in] sequence number to use >> + * @signal_on_any [in] signal on any fence in the array >> * >> * Allocate a fence_array object and initialize the base fence with fence_init(). >> * In case of error it returns NULL. >> @@ -101,9 +106,13 @@ const struct fence_ops fence_array_ops = { >> * The caller should allocte the fences array with num_fences size >> * and fill it with the fences it wants to add to the object. Ownership of this >> * array is take and fence_put() is used on each fence on release. >> + * >> + * If @signal_on_any is true the fence array signals if any fence in the array >> + * signals, otherwise it signals when all fences in the array signal. >> */ >> struct fence_array *fence_array_create(int num_fences, struct fence **fences, >> - u64 context, unsigned seqno) >> + u64 context, unsigned seqno, >> + bool signal_on_any) >> { >> struct fence_array *array; >> size_t size = sizeof(*array); >> @@ -119,7 +128,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences, >> context, seqno); >> >> array->num_fences = num_fences; >> - atomic_set(&array->num_pending, num_fences); >> + atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); >> array->fences = fences; >> >> return array; >> diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h >> index 593ab98..86baaa4 100644 >> --- a/include/linux/fence-array.h >> +++ b/include/linux/fence-array.h >> @@ -67,6 +67,7 @@ static inline struct fence_array *to_fence_array(struct fence *fence) >> } >> >> struct fence_array *fence_array_create(int num_fences, struct fence **fences, >> - u64 context, unsigned seqno); >> + u64 context, unsigned seqno, >> + bool signal_on_any); >> >> #endif /* __LINUX_FENCE_ARRAY_H */ >> -- >> 2.5.0 >>
On Mon, May 23, 2016 at 03:58:41PM +0200, Christian König wrote: > Am 23.05.2016 um 15:50 schrieb Daniel Vetter: > >On Mon, May 23, 2016 at 01:25:22PM +0200, Christian König wrote: > >>From: Christian König <christian.koenig@amd.com> > >> > >>If @signal_on_any is true the fence array signals if any fence in the array > >>signals, otherwise it signals when all fences in the array signal. > >What do you need this for? Sounds neat, but I can't come up with a use > >case. And that use case imo should be both in the commit message and > >kerneldoc. > > Good point, going to fix this when if we really use the patch. Just wanted > to get opinions on this. For getting opinions on this I get a bit the impression we're slightly talking past each another. Having the actual user of these functions handy should help a lot for this discussion. > I'm currently working on making some of the resource management in amdgpu > more friendly to our GPU scheduler. > > Waiting for resources in the GPU scheduler is implemented as waiting for a > fence to fire. So when you want to wait for any resource to become available > (like in our case the VMIDs) you could make good use of something like this. Hm, ok, if you want a fence that fires when any resource becomes available of a given type becomes available that might be useful. But how do you handle the next work item that needs such a resource? Add _all_ previous fences that use it (there's not much ordering with the scheduler) and let them race, and retry if they fail? -Daniel
diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c index bbcf79f..726e840 100644 --- a/drivers/dma-buf/fence-array.c +++ b/drivers/dma-buf/fence-array.c @@ -41,6 +41,7 @@ static void fence_array_cb_func(struct fence *f, struct fence_cb *cb) if (atomic_dec_and_test(&array->num_pending)) fence_signal(&array->base); + fence_put(&array->base); } static bool fence_array_enable_signaling(struct fence *fence) @@ -51,10 +52,13 @@ static bool fence_array_enable_signaling(struct fence *fence) for (i = 0; i < array->num_fences; ++i) { cb[i].array = array; + fence_get(&array->base); if (!fence_add_callback(array->fences[i], &cb[i].cb, - fence_array_cb_func)) + fence_array_cb_func)) { + fence_put(&array->base); if (atomic_dec_and_test(&array->num_pending)) return false; + } } return true; @@ -90,10 +94,11 @@ const struct fence_ops fence_array_ops = { /** * fence_array_create - Create a custom fence array - * @num_fences: [in] number of fences to add in the array - * @fences: [in] array containing the fences - * @context: [in] fence context to use - * @seqno: [in] sequence number to use + * @num_fences: [in] number of fences to add in the array + * @fences: [in] array containing the fences + * @context: [in] fence context to use + * @seqno: [in] sequence number to use + * @signal_on_any [in] signal on any fence in the array * * Allocate a fence_array object and initialize the base fence with fence_init(). * In case of error it returns NULL. @@ -101,9 +106,13 @@ const struct fence_ops fence_array_ops = { * The caller should allocte the fences array with num_fences size * and fill it with the fences it wants to add to the object. Ownership of this * array is take and fence_put() is used on each fence on release. + * + * If @signal_on_any is true the fence array signals if any fence in the array + * signals, otherwise it signals when all fences in the array signal. */ struct fence_array *fence_array_create(int num_fences, struct fence **fences, - u64 context, unsigned seqno) + u64 context, unsigned seqno, + bool signal_on_any) { struct fence_array *array; size_t size = sizeof(*array); @@ -119,7 +128,7 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences, context, seqno); array->num_fences = num_fences; - atomic_set(&array->num_pending, num_fences); + atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); array->fences = fences; return array; diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h index 593ab98..86baaa4 100644 --- a/include/linux/fence-array.h +++ b/include/linux/fence-array.h @@ -67,6 +67,7 @@ static inline struct fence_array *to_fence_array(struct fence *fence) } struct fence_array *fence_array_create(int num_fences, struct fence **fences, - u64 context, unsigned seqno); + u64 context, unsigned seqno, + bool signal_on_any); #endif /* __LINUX_FENCE_ARRAY_H */