| Message ID | 20240823045443.2132118-2-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace dma-fence chain with dma-fence array for media GT TLB invalidation | expand |
Am 23.08.24 um 06:54 schrieb Matthew Brost: > Useful to preallocate dma fence array and then arm in path of reclaim or > a dma fence. Exactly that was rejected before because it allows to create circle dependencies. You would need a really really good argument why that is necessary. Regards, Christian. > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/dma-buf/dma-fence-array.c | 81 ++++++++++++++++++++++--------- > include/linux/dma-fence-array.h | 7 +++ > 2 files changed, 66 insertions(+), 22 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index c74ac197d5fe..b03e0a87a5cd 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -144,36 +144,38 @@ const struct dma_fence_ops dma_fence_array_ops = { > EXPORT_SYMBOL(dma_fence_array_ops); > > /** > - * dma_fence_array_create - Create a custom fence array > + * dma_fence_array_alloc - Allocate a custom fence array > + * @num_fences: [in] number of fences to add in the array > + * > + * Return dma fence array on success, NULL on failure > + */ > +struct dma_fence_array *dma_fence_array_alloc(int num_fences) > +{ > + struct dma_fence_array *array; > + > + return kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL); > +} > +EXPORT_SYMBOL(dma_fence_array_alloc); > + > +/** > + * dma_fence_array_arm - Arm a custom fence array > + * @array: [in] dma fence array to arm > * @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 dma_fence_array object and initialize the base fence with > - * dma_fence_init(). > - * In case of error it returns NULL. > - * > - * The caller should allocate 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 taken and dma_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. > + * Implementation of @dma_fence_array_create without allocation. Useful to arm a > + * preallocated dma fence fence in the path of reclaim or dma fence signaling. > */ > -struct dma_fence_array *dma_fence_array_create(int num_fences, > - struct dma_fence **fences, > - u64 context, unsigned seqno, > - bool signal_on_any) > +void dma_fence_array_arm(struct dma_fence_array *array, > + int num_fences, > + struct dma_fence **fences, > + u64 context, unsigned seqno, > + bool signal_on_any) > { > - struct dma_fence_array *array; > - > - WARN_ON(!num_fences || !fences); > - > - array = kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL); > - if (!array) > - return NULL; > + WARN_ON(!array || !num_fences || !fences); > > array->num_fences = num_fences; > > @@ -200,6 +202,41 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > */ > while (num_fences--) > WARN_ON(dma_fence_is_container(fences[num_fences])); > +} > +EXPORT_SYMBOL(dma_fence_array_arm); > + > +/** > + * dma_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 > + * @signal_on_any: [in] signal on any fence in the array > + * > + * Allocate a dma_fence_array object and initialize the base fence with > + * dma_fence_init(). > + * In case of error it returns NULL. > + * > + * The caller should allocate 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 taken and dma_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 dma_fence_array *dma_fence_array_create(int num_fences, > + struct dma_fence **fences, > + u64 context, unsigned seqno, > + bool signal_on_any) > +{ > + struct dma_fence_array *array; > + > + array = dma_fence_array_alloc(num_fences); > + if (!array) > + return NULL; > + > + dma_fence_array_arm(array, num_fences, fences, > + context, seqno, signal_on_any); > > return array; > } > diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h > index 29c5650c1038..3466ffc4b803 100644 > --- a/include/linux/dma-fence-array.h > +++ b/include/linux/dma-fence-array.h > @@ -79,6 +79,13 @@ to_dma_fence_array(struct dma_fence *fence) > for (index = 0, fence = dma_fence_array_first(head); fence; \ > ++(index), fence = dma_fence_array_next(head, index)) > > +struct dma_fence_array *dma_fence_array_alloc(int num_fences); > +void dma_fence_array_arm(struct dma_fence_array *array, > + int num_fences, > + struct dma_fence **fences, > + u64 context, unsigned seqno, > + bool signal_on_any); > + > struct dma_fence_array *dma_fence_array_create(int num_fences, > struct dma_fence **fences, > u64 context, unsigned seqno,
On Fri, Aug 23, 2024 at 08:37:30AM +0200, Christian König wrote: > Am 23.08.24 um 06:54 schrieb Matthew Brost: > > Useful to preallocate dma fence array and then arm in path of reclaim or > > a dma fence. > > Exactly that was rejected before because it allows to create circle > dependencies. > Can you explain or do you have link to that discussion? Trying to think how this would be problematic and failing to see how it is. > You would need a really really good argument why that is necessary. > It seems quite useful when you have a code path in which you know N fences will be generated, prealloc a dma fence array, then populate at later time ensuring no failures points (malloc), and then finally install dma fence array in timeline sync obj (chain fences not allowed). It fits nicely for VM bind operations in which a device has multple TLBs and the TLB invalidation completion is a fence. I suspect Intel can't be the only device out their with multiple TLBs, does VM bind, and use timeline sync obj. Matt > Regards, > Christian. > > > > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > > Cc: Christian König <christian.koenig@amd.com> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/dma-buf/dma-fence-array.c | 81 ++++++++++++++++++++++--------- > > include/linux/dma-fence-array.h | 7 +++ > > 2 files changed, 66 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > > index c74ac197d5fe..b03e0a87a5cd 100644 > > --- a/drivers/dma-buf/dma-fence-array.c > > +++ b/drivers/dma-buf/dma-fence-array.c > > @@ -144,36 +144,38 @@ const struct dma_fence_ops dma_fence_array_ops = { > > EXPORT_SYMBOL(dma_fence_array_ops); > > /** > > - * dma_fence_array_create - Create a custom fence array > > + * dma_fence_array_alloc - Allocate a custom fence array > > + * @num_fences: [in] number of fences to add in the array > > + * > > + * Return dma fence array on success, NULL on failure > > + */ > > +struct dma_fence_array *dma_fence_array_alloc(int num_fences) > > +{ > > + struct dma_fence_array *array; > > + > > + return kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL); > > +} > > +EXPORT_SYMBOL(dma_fence_array_alloc); > > + > > +/** > > + * dma_fence_array_arm - Arm a custom fence array > > + * @array: [in] dma fence array to arm > > * @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 dma_fence_array object and initialize the base fence with > > - * dma_fence_init(). > > - * In case of error it returns NULL. > > - * > > - * The caller should allocate 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 taken and dma_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. > > + * Implementation of @dma_fence_array_create without allocation. Useful to arm a > > + * preallocated dma fence fence in the path of reclaim or dma fence signaling. > > */ > > -struct dma_fence_array *dma_fence_array_create(int num_fences, > > - struct dma_fence **fences, > > - u64 context, unsigned seqno, > > - bool signal_on_any) > > +void dma_fence_array_arm(struct dma_fence_array *array, > > + int num_fences, > > + struct dma_fence **fences, > > + u64 context, unsigned seqno, > > + bool signal_on_any) > > { > > - struct dma_fence_array *array; > > - > > - WARN_ON(!num_fences || !fences); > > - > > - array = kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL); > > - if (!array) > > - return NULL; > > + WARN_ON(!array || !num_fences || !fences); > > array->num_fences = num_fences; > > @@ -200,6 +202,41 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > > */ > > while (num_fences--) > > WARN_ON(dma_fence_is_container(fences[num_fences])); > > +} > > +EXPORT_SYMBOL(dma_fence_array_arm); > > + > > +/** > > + * dma_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 > > + * @signal_on_any: [in] signal on any fence in the array > > + * > > + * Allocate a dma_fence_array object and initialize the base fence with > > + * dma_fence_init(). > > + * In case of error it returns NULL. > > + * > > + * The caller should allocate 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 taken and dma_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 dma_fence_array *dma_fence_array_create(int num_fences, > > + struct dma_fence **fences, > > + u64 context, unsigned seqno, > > + bool signal_on_any) > > +{ > > + struct dma_fence_array *array; > > + > > + array = dma_fence_array_alloc(num_fences); > > + if (!array) > > + return NULL; > > + > > + dma_fence_array_arm(array, num_fences, fences, > > + context, seqno, signal_on_any); > > return array; > > } > > diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h > > index 29c5650c1038..3466ffc4b803 100644 > > --- a/include/linux/dma-fence-array.h > > +++ b/include/linux/dma-fence-array.h > > @@ -79,6 +79,13 @@ to_dma_fence_array(struct dma_fence *fence) > > for (index = 0, fence = dma_fence_array_first(head); fence; \ > > ++(index), fence = dma_fence_array_next(head, index)) > > +struct dma_fence_array *dma_fence_array_alloc(int num_fences); > > +void dma_fence_array_arm(struct dma_fence_array *array, > > + int num_fences, > > + struct dma_fence **fences, > > + u64 context, unsigned seqno, > > + bool signal_on_any); > > + > > struct dma_fence_array *dma_fence_array_create(int num_fences, > > struct dma_fence **fences, > > u64 context, unsigned seqno, >
On Fri, Aug 23, 2024 at 03:46:05PM +0000, Matthew Brost wrote: > On Fri, Aug 23, 2024 at 08:37:30AM +0200, Christian König wrote: > > Am 23.08.24 um 06:54 schrieb Matthew Brost: > > > Useful to preallocate dma fence array and then arm in path of reclaim or > > > a dma fence. > > > > Exactly that was rejected before because it allows to create circle > > dependencies. > > > > Can you explain or do you have link to that discussion? Trying to think > how this would be problematic and failing to see how it is. > > > You would need a really really good argument why that is necessary. > > > > It seems quite useful when you have a code path in which you know N fences > will be generated, prealloc a dma fence array, then populate at > later time ensuring no failures points (malloc), and then finally > install dma fence array in timeline sync obj (chain fences not allowed). > > It fits nicely for VM bind operations in which a device has multple > TLBs and the TLB invalidation completion is a fence. I suspect Intel > can't be the only device out their with multiple TLBs, does VM bind, and > use timeline sync obj. I think the naming you've picked is a bit confusion, since all you're splitting out is the kzalloc call. At that point the dma_fence_array isn't yet useable as a fence, so there's no issues with with circles. It's only when you call _arm that it becomes a real fence. I think just renaming _arm to _init, so that we follow the standard naming pattern for splitting _create() into kzalloc and everything else is all that's needed here? Plus updating the kernel doc to make it really clear that _alloc doesn't give you a fence, just a pile of memory. And that _init must be called with a compatible amount of fences, or it'll fail. -Sima
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index c74ac197d5fe..b03e0a87a5cd 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -144,36 +144,38 @@ const struct dma_fence_ops dma_fence_array_ops = { EXPORT_SYMBOL(dma_fence_array_ops); /** - * dma_fence_array_create - Create a custom fence array + * dma_fence_array_alloc - Allocate a custom fence array + * @num_fences: [in] number of fences to add in the array + * + * Return dma fence array on success, NULL on failure + */ +struct dma_fence_array *dma_fence_array_alloc(int num_fences) +{ + struct dma_fence_array *array; + + return kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL); +} +EXPORT_SYMBOL(dma_fence_array_alloc); + +/** + * dma_fence_array_arm - Arm a custom fence array + * @array: [in] dma fence array to arm * @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 dma_fence_array object and initialize the base fence with - * dma_fence_init(). - * In case of error it returns NULL. - * - * The caller should allocate 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 taken and dma_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. + * Implementation of @dma_fence_array_create without allocation. Useful to arm a + * preallocated dma fence fence in the path of reclaim or dma fence signaling. */ -struct dma_fence_array *dma_fence_array_create(int num_fences, - struct dma_fence **fences, - u64 context, unsigned seqno, - bool signal_on_any) +void dma_fence_array_arm(struct dma_fence_array *array, + int num_fences, + struct dma_fence **fences, + u64 context, unsigned seqno, + bool signal_on_any) { - struct dma_fence_array *array; - - WARN_ON(!num_fences || !fences); - - array = kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL); - if (!array) - return NULL; + WARN_ON(!array || !num_fences || !fences); array->num_fences = num_fences; @@ -200,6 +202,41 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, */ while (num_fences--) WARN_ON(dma_fence_is_container(fences[num_fences])); +} +EXPORT_SYMBOL(dma_fence_array_arm); + +/** + * dma_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 + * @signal_on_any: [in] signal on any fence in the array + * + * Allocate a dma_fence_array object and initialize the base fence with + * dma_fence_init(). + * In case of error it returns NULL. + * + * The caller should allocate 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 taken and dma_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 dma_fence_array *dma_fence_array_create(int num_fences, + struct dma_fence **fences, + u64 context, unsigned seqno, + bool signal_on_any) +{ + struct dma_fence_array *array; + + array = dma_fence_array_alloc(num_fences); + if (!array) + return NULL; + + dma_fence_array_arm(array, num_fences, fences, + context, seqno, signal_on_any); return array; } diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index 29c5650c1038..3466ffc4b803 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -79,6 +79,13 @@ to_dma_fence_array(struct dma_fence *fence) for (index = 0, fence = dma_fence_array_first(head); fence; \ ++(index), fence = dma_fence_array_next(head, index)) +struct dma_fence_array *dma_fence_array_alloc(int num_fences); +void dma_fence_array_arm(struct dma_fence_array *array, + int num_fences, + struct dma_fence **fences, + u64 context, unsigned seqno, + bool signal_on_any); + struct dma_fence_array *dma_fence_array_create(int num_fences, struct dma_fence **fences, u64 context, unsigned seqno,
Useful to preallocate dma fence array and then arm in path of reclaim or a dma fence. Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Christian König <christian.koenig@amd.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/dma-buf/dma-fence-array.c | 81 ++++++++++++++++++++++--------- include/linux/dma-fence-array.h | 7 +++ 2 files changed, 66 insertions(+), 22 deletions(-)