[PATCH v3 0/2] Replace dma-fence chain with dma-fence array for media GT TLB invalidation

[PATCH v3 0/2] Replace dma-fence chain with dma-fence array for media GT TLB invalidation

[Matthew Brost]

Third try, requires spliting out dma fence array alloc and arm into new
functions to avoid memory allocations under notifier lock.

Matt 

Matthew Brost (2):
  dma-buf: Split out dma fence array create into alloc and arm functions
  drm/xe: Use dma-fence array for media GT TLB invalidations in PT code

 drivers/dma-buf/dma-fence-array.c | 81 ++++++++++++++++++++++---------
 drivers/gpu/drm/xe/xe_pt.c        | 34 +++++++++----
 include/linux/dma-fence-array.h   |  7 +++
 3 files changed, 90 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] dma-buf: Split out dma fence array create into alloc and arm functions

[Matthew Brost]

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(-)

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,
-- 
2.34.1

[PATCH v3 2/2] drm/xe: Use dma-fence array for media GT TLB invalidations in PT code

[Matthew Brost]

Using a chain fence is problematic as these cannot be installed in
timeout drm sync objects. Use a dma-fence-array instead at the cost of
an extra failure point.

Also fixup reserve fence count to include media GT invalidation fence.

v2:
 - Fix reserve fence count (Casey Bowman)
v3:
 - Prealloc dma fence array (CI)

Fixes: 40520283e0fd ("drm/xe: Invalidate media_gt TLBs in PT code")
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_pt.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
index 6c6714af3d5d..2e35444a85b0 100644
--- a/drivers/gpu/drm/xe/xe_pt.c
+++ b/drivers/gpu/drm/xe/xe_pt.c
@@ -3,7 +3,7 @@
  * Copyright © 2022 Intel Corporation
  */
 
-#include <linux/dma-fence-chain.h>
+#include <linux/dma-fence-array.h>
 
 #include "xe_pt.h"
 
@@ -1629,9 +1629,11 @@ xe_pt_update_ops_rfence_interval(struct xe_vm_pgtable_update_ops *pt_update_ops,
 
 static int vma_reserve_fences(struct xe_device *xe, struct xe_vma *vma)
 {
+	int shift = xe_device_get_root_tile(xe)->media_gt ? 1 : 0;
+
 	if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
 		return dma_resv_reserve_fences(xe_vma_bo(vma)->ttm.base.resv,
-					       xe->info.tile_count);
+					       xe->info.tile_count << shift);
 
 	return 0;
 }
@@ -1818,6 +1820,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
 	struct xe_vm_pgtable_update_ops *pt_update_ops =
 		&vops->pt_update_ops[tile->id];
 	struct xe_vma_op *op;
+	int shift = tile->media_gt ? 1 : 0;
 	int err;
 
 	lockdep_assert_held(&vops->vm->lock);
@@ -1826,7 +1829,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
 	xe_pt_update_ops_init(pt_update_ops);
 
 	err = dma_resv_reserve_fences(xe_vm_resv(vops->vm),
-				      tile_to_xe(tile)->info.tile_count);
+				      tile_to_xe(tile)->info.tile_count << shift);
 	if (err)
 		return err;
 
@@ -1983,7 +1986,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
 		&vops->pt_update_ops[tile->id];
 	struct dma_fence *fence;
 	struct invalidation_fence *ifence = NULL, *mfence = NULL;
-	struct dma_fence_chain *chain_fence = NULL;
+	struct dma_fence **fences = NULL;
+	struct dma_fence_array *cf = NULL;
 	struct xe_range_fence *rfence;
 	struct xe_vma_op *op;
 	int err = 0, i;
@@ -2022,8 +2026,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
 				err = -ENOMEM;
 				goto free_ifence;
 			}
-			chain_fence = dma_fence_chain_alloc();
-			if (!chain_fence) {
+			fences = kmalloc_array(2, sizeof(*fences), GFP_KERNEL);
+			if (!fences) {
+				err = -ENOMEM;
+				goto free_ifence;
+			}
+			cf = dma_fence_array_alloc(2);
+			if (!cf) {
 				err = -ENOMEM;
 				goto free_ifence;
 			}
@@ -2068,9 +2077,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
 			invalidation_fence_init(tile->media_gt, mfence, fence,
 						pt_update_ops->start,
 						pt_update_ops->last, vm->usm.asid);
-			dma_fence_chain_init(chain_fence, &ifence->base.base,
-					     &mfence->base.base, 0);
-			fence = &chain_fence->base;
+			fences[0] = &ifence->base.base;
+			fences[1] = &mfence->base.base;
+			dma_fence_array_arm(cf, 2, fences,
+					    vm->composite_fence_ctx,
+					    vm->composite_fence_seqno++,
+					    false);
+			fence = &cf->base;
 		} else {
 			fence = &ifence->base.base;
 		}
@@ -2108,7 +2121,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
 free_rfence:
 	kfree(rfence);
 free_ifence:
-	dma_fence_chain_free(chain_fence);
+	kfree(cf);
+	kfree(fences);
 	kfree(mfence);
 	kfree(ifence);
 kill_vm_tile1:
-- 
2.34.1

Re: [PATCH v3 1/2] dma-buf: Split out dma fence array create into alloc and arm functions

[Christian König]

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,

Re: [PATCH v3 2/2] drm/xe: Use dma-fence array for media GT TLB invalidations in PT code

[Christian König]

Am 23.08.24 um 06:54 schrieb Matthew Brost:
> Using a chain fence is problematic as these cannot be installed in
> timeout drm sync objects. Use a dma-fence-array instead at the cost of
> an extra failure point.

Mhm, IIRC we converted chain objects into dma-fence-arrays while 
installing them into a timeline.

Doesn't that work any more?

Regards,
Christian.

>
> Also fixup reserve fence count to include media GT invalidation fence.
>
> v2:
>   - Fix reserve fence count (Casey Bowman)
> v3:
>   - Prealloc dma fence array (CI)
>
> Fixes: 40520283e0fd ("drm/xe: Invalidate media_gt TLBs in PT code")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_pt.c | 34 ++++++++++++++++++++++++----------
>   1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 6c6714af3d5d..2e35444a85b0 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -3,7 +3,7 @@
>    * Copyright © 2022 Intel Corporation
>    */
>   
> -#include <linux/dma-fence-chain.h>
> +#include <linux/dma-fence-array.h>
>   
>   #include "xe_pt.h"
>   
> @@ -1629,9 +1629,11 @@ xe_pt_update_ops_rfence_interval(struct xe_vm_pgtable_update_ops *pt_update_ops,
>   
>   static int vma_reserve_fences(struct xe_device *xe, struct xe_vma *vma)
>   {
> +	int shift = xe_device_get_root_tile(xe)->media_gt ? 1 : 0;
> +
>   	if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
>   		return dma_resv_reserve_fences(xe_vma_bo(vma)->ttm.base.resv,
> -					       xe->info.tile_count);
> +					       xe->info.tile_count << shift);
>   
>   	return 0;
>   }
> @@ -1818,6 +1820,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
>   	struct xe_vm_pgtable_update_ops *pt_update_ops =
>   		&vops->pt_update_ops[tile->id];
>   	struct xe_vma_op *op;
> +	int shift = tile->media_gt ? 1 : 0;
>   	int err;
>   
>   	lockdep_assert_held(&vops->vm->lock);
> @@ -1826,7 +1829,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
>   	xe_pt_update_ops_init(pt_update_ops);
>   
>   	err = dma_resv_reserve_fences(xe_vm_resv(vops->vm),
> -				      tile_to_xe(tile)->info.tile_count);
> +				      tile_to_xe(tile)->info.tile_count << shift);
>   	if (err)
>   		return err;
>   
> @@ -1983,7 +1986,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>   		&vops->pt_update_ops[tile->id];
>   	struct dma_fence *fence;
>   	struct invalidation_fence *ifence = NULL, *mfence = NULL;
> -	struct dma_fence_chain *chain_fence = NULL;
> +	struct dma_fence **fences = NULL;
> +	struct dma_fence_array *cf = NULL;
>   	struct xe_range_fence *rfence;
>   	struct xe_vma_op *op;
>   	int err = 0, i;
> @@ -2022,8 +2026,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>   				err = -ENOMEM;
>   				goto free_ifence;
>   			}
> -			chain_fence = dma_fence_chain_alloc();
> -			if (!chain_fence) {
> +			fences = kmalloc_array(2, sizeof(*fences), GFP_KERNEL);
> +			if (!fences) {
> +				err = -ENOMEM;
> +				goto free_ifence;
> +			}
> +			cf = dma_fence_array_alloc(2);
> +			if (!cf) {
>   				err = -ENOMEM;
>   				goto free_ifence;
>   			}
> @@ -2068,9 +2077,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>   			invalidation_fence_init(tile->media_gt, mfence, fence,
>   						pt_update_ops->start,
>   						pt_update_ops->last, vm->usm.asid);
> -			dma_fence_chain_init(chain_fence, &ifence->base.base,
> -					     &mfence->base.base, 0);
> -			fence = &chain_fence->base;
> +			fences[0] = &ifence->base.base;
> +			fences[1] = &mfence->base.base;
> +			dma_fence_array_arm(cf, 2, fences,
> +					    vm->composite_fence_ctx,
> +					    vm->composite_fence_seqno++,
> +					    false);
> +			fence = &cf->base;
>   		} else {
>   			fence = &ifence->base.base;
>   		}
> @@ -2108,7 +2121,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>   free_rfence:
>   	kfree(rfence);
>   free_ifence:
> -	dma_fence_chain_free(chain_fence);
> +	kfree(cf);
> +	kfree(fences);
>   	kfree(mfence);
>   	kfree(ifence);
>   kill_vm_tile1:

Re: [PATCH v3 2/2] drm/xe: Use dma-fence array for media GT TLB invalidations in PT code

[Matthew Brost]

On Fri, Aug 23, 2024 at 08:40:40AM +0200, Christian König wrote:
> Am 23.08.24 um 06:54 schrieb Matthew Brost:
> > Using a chain fence is problematic as these cannot be installed in
> > timeout drm sync objects. Use a dma-fence-array instead at the cost of
> > an extra failure point.
> 
> Mhm, IIRC we converted chain objects into dma-fence-arrays while installing
> them into a timeline.
> 
> Doesn't that work any more?
> 

Thanks for the quick feedback.

As is, installing a dma-fence-chain into a timeline sync doesn't work.

The 'fence' returned from 'xe_pt_update_ops_run' is installed here [1]
as the 'fence' argument. This blows up here [2] [3]. It does suggest in
[3] to use a dma-fence-array which is what I'm doing. 

The issue with using a dma-fence array as is it adds another failure
point if dma_fence_array_create is used as is after collecting multiple
fences from TLB invalidations. Also we have lock in xe_pt_update_ops_run
which is in the path reclaim so calling dma_fence_array_create isn't
allowed under that lock.

I suppose we could drop that lock and directly wait TLB invalidation
fences if dma_fence_array_create fails but to me it makes more sense to
prealloc the dma-fence-array and populate it later. Saw your response to
my first patch about how this could be problematic, a little confused on
that so responding there too.

Matt

[1] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/gpu/drm/xe/xe_sync.c#L233
[2] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/gpu/drm/drm_syncobj.c#L349
[3] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma-buf/dma-fence-chain.c#L275

> Regards,
> Christian.
> 
> > 
> > Also fixup reserve fence count to include media GT invalidation fence.
> > 
> > v2:
> >   - Fix reserve fence count (Casey Bowman)
> > v3:
> >   - Prealloc dma fence array (CI)
> > 
> > Fixes: 40520283e0fd ("drm/xe: Invalidate media_gt TLBs in PT code")
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_pt.c | 34 ++++++++++++++++++++++++----------
> >   1 file changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > index 6c6714af3d5d..2e35444a85b0 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > @@ -3,7 +3,7 @@
> >    * Copyright © 2022 Intel Corporation
> >    */
> > -#include <linux/dma-fence-chain.h>
> > +#include <linux/dma-fence-array.h>
> >   #include "xe_pt.h"
> > @@ -1629,9 +1629,11 @@ xe_pt_update_ops_rfence_interval(struct xe_vm_pgtable_update_ops *pt_update_ops,
> >   static int vma_reserve_fences(struct xe_device *xe, struct xe_vma *vma)
> >   {
> > +	int shift = xe_device_get_root_tile(xe)->media_gt ? 1 : 0;
> > +
> >   	if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
> >   		return dma_resv_reserve_fences(xe_vma_bo(vma)->ttm.base.resv,
> > -					       xe->info.tile_count);
> > +					       xe->info.tile_count << shift);
> >   	return 0;
> >   }
> > @@ -1818,6 +1820,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
> >   	struct xe_vm_pgtable_update_ops *pt_update_ops =
> >   		&vops->pt_update_ops[tile->id];
> >   	struct xe_vma_op *op;
> > +	int shift = tile->media_gt ? 1 : 0;
> >   	int err;
> >   	lockdep_assert_held(&vops->vm->lock);
> > @@ -1826,7 +1829,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
> >   	xe_pt_update_ops_init(pt_update_ops);
> >   	err = dma_resv_reserve_fences(xe_vm_resv(vops->vm),
> > -				      tile_to_xe(tile)->info.tile_count);
> > +				      tile_to_xe(tile)->info.tile_count << shift);
> >   	if (err)
> >   		return err;
> > @@ -1983,7 +1986,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> >   		&vops->pt_update_ops[tile->id];
> >   	struct dma_fence *fence;
> >   	struct invalidation_fence *ifence = NULL, *mfence = NULL;
> > -	struct dma_fence_chain *chain_fence = NULL;
> > +	struct dma_fence **fences = NULL;
> > +	struct dma_fence_array *cf = NULL;
> >   	struct xe_range_fence *rfence;
> >   	struct xe_vma_op *op;
> >   	int err = 0, i;
> > @@ -2022,8 +2026,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> >   				err = -ENOMEM;
> >   				goto free_ifence;
> >   			}
> > -			chain_fence = dma_fence_chain_alloc();
> > -			if (!chain_fence) {
> > +			fences = kmalloc_array(2, sizeof(*fences), GFP_KERNEL);
> > +			if (!fences) {
> > +				err = -ENOMEM;
> > +				goto free_ifence;
> > +			}
> > +			cf = dma_fence_array_alloc(2);
> > +			if (!cf) {
> >   				err = -ENOMEM;
> >   				goto free_ifence;
> >   			}
> > @@ -2068,9 +2077,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> >   			invalidation_fence_init(tile->media_gt, mfence, fence,
> >   						pt_update_ops->start,
> >   						pt_update_ops->last, vm->usm.asid);
> > -			dma_fence_chain_init(chain_fence, &ifence->base.base,
> > -					     &mfence->base.base, 0);
> > -			fence = &chain_fence->base;
> > +			fences[0] = &ifence->base.base;
> > +			fences[1] = &mfence->base.base;
> > +			dma_fence_array_arm(cf, 2, fences,
> > +					    vm->composite_fence_ctx,
> > +					    vm->composite_fence_seqno++,
> > +					    false);
> > +			fence = &cf->base;
> >   		} else {
> >   			fence = &ifence->base.base;
> >   		}
> > @@ -2108,7 +2121,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> >   free_rfence:
> >   	kfree(rfence);
> >   free_ifence:
> > -	dma_fence_chain_free(chain_fence);
> > +	kfree(cf);
> > +	kfree(fences);
> >   	kfree(mfence);
> >   	kfree(ifence);
> >   kill_vm_tile1:
> 

Re: [PATCH v3 1/2] dma-buf: Split out dma fence array create into alloc and arm functions

[Matthew Brost]

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,
> 

Re: [PATCH v3 2/2] drm/xe: Use dma-fence array for media GT TLB invalidations in PT code

[Christian König]

Am 23.08.24 um 17:38 schrieb Matthew Brost:
> On Fri, Aug 23, 2024 at 08:40:40AM +0200, Christian König wrote:
>> Am 23.08.24 um 06:54 schrieb Matthew Brost:
>>> Using a chain fence is problematic as these cannot be installed in
>>> timeout drm sync objects. Use a dma-fence-array instead at the cost of
>>> an extra failure point.
>> Mhm, IIRC we converted chain objects into dma-fence-arrays while installing
>> them into a timeline.
>>
>> Doesn't that work any more?
>>
> Thanks for the quick feedback.
>
> As is, installing a dma-fence-chain into a timeline sync doesn't work.
>
> The 'fence' returned from 'xe_pt_update_ops_run' is installed here [1]
> as the 'fence' argument. This blows up here [2] [3]. It does suggest in
> [3] to use a dma-fence-array which is what I'm doing.

Ah, that makes it more clear. You are not using some IOCTL to install 
the fences into a timeline but rather want to do this at the end of your 
submission IOCTL, right?

> The issue with using a dma-fence array as is it adds another failure
> point if dma_fence_array_create is used as is after collecting multiple
> fences from TLB invalidations. Also we have lock in xe_pt_update_ops_run
> which is in the path reclaim so calling dma_fence_array_create isn't
> allowed under that lock.

Ok that is a rather good argument for this.

Just tow comments I've seen on the code:
1. Please rename dma_fence_array_arm() into dma_fence_array_init()
2. Please drop WARN_ON(!array, a NULL array will result in a NULL 
pointer de-reference and crash anyway.

> I suppose we could drop that lock and directly wait TLB invalidation
> fences if dma_fence_array_create fails but to me it makes more sense to
> prealloc the dma-fence-array and populate it later. Saw your response to
> my first patch about how this could be problematic, a little confused on
> that so responding there too.

Yeah people came up with the crazy idea to insert dma_fence_array 
objects into other dma_fence_array's resulting in overwriting the kernel 
stack when you free this construct finally.

Additional to that Sima pointed out during the initial review of this 
code that we should make sure that no circles can happen with a dma_fence.

But we now have a warning when somebody tries to add a container to a 
dma_fence_array object so that should probably be fine.

Regards,
Christian.

>
> Matt
>
> [1] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/gpu/drm/xe/xe_sync.c#L233
> [2] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/gpu/drm/drm_syncobj.c#L349
> [3] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma-buf/dma-fence-chain.c#L275
>
>> Regards,
>> Christian.
>>
>>> Also fixup reserve fence count to include media GT invalidation fence.
>>>
>>> v2:
>>>    - Fix reserve fence count (Casey Bowman)
>>> v3:
>>>    - Prealloc dma fence array (CI)
>>>
>>> Fixes: 40520283e0fd ("drm/xe: Invalidate media_gt TLBs in PT code")
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/xe_pt.c | 34 ++++++++++++++++++++++++----------
>>>    1 file changed, 24 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>>> index 6c6714af3d5d..2e35444a85b0 100644
>>> --- a/drivers/gpu/drm/xe/xe_pt.c
>>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>>> @@ -3,7 +3,7 @@
>>>     * Copyright © 2022 Intel Corporation
>>>     */
>>> -#include <linux/dma-fence-chain.h>
>>> +#include <linux/dma-fence-array.h>
>>>    #include "xe_pt.h"
>>> @@ -1629,9 +1629,11 @@ xe_pt_update_ops_rfence_interval(struct xe_vm_pgtable_update_ops *pt_update_ops,
>>>    static int vma_reserve_fences(struct xe_device *xe, struct xe_vma *vma)
>>>    {
>>> +	int shift = xe_device_get_root_tile(xe)->media_gt ? 1 : 0;
>>> +
>>>    	if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
>>>    		return dma_resv_reserve_fences(xe_vma_bo(vma)->ttm.base.resv,
>>> -					       xe->info.tile_count);
>>> +					       xe->info.tile_count << shift);
>>>    	return 0;
>>>    }
>>> @@ -1818,6 +1820,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
>>>    	struct xe_vm_pgtable_update_ops *pt_update_ops =
>>>    		&vops->pt_update_ops[tile->id];
>>>    	struct xe_vma_op *op;
>>> +	int shift = tile->media_gt ? 1 : 0;
>>>    	int err;
>>>    	lockdep_assert_held(&vops->vm->lock);
>>> @@ -1826,7 +1829,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
>>>    	xe_pt_update_ops_init(pt_update_ops);
>>>    	err = dma_resv_reserve_fences(xe_vm_resv(vops->vm),
>>> -				      tile_to_xe(tile)->info.tile_count);
>>> +				      tile_to_xe(tile)->info.tile_count << shift);
>>>    	if (err)
>>>    		return err;
>>> @@ -1983,7 +1986,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>>>    		&vops->pt_update_ops[tile->id];
>>>    	struct dma_fence *fence;
>>>    	struct invalidation_fence *ifence = NULL, *mfence = NULL;
>>> -	struct dma_fence_chain *chain_fence = NULL;
>>> +	struct dma_fence **fences = NULL;
>>> +	struct dma_fence_array *cf = NULL;
>>>    	struct xe_range_fence *rfence;
>>>    	struct xe_vma_op *op;
>>>    	int err = 0, i;
>>> @@ -2022,8 +2026,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>>>    				err = -ENOMEM;
>>>    				goto free_ifence;
>>>    			}
>>> -			chain_fence = dma_fence_chain_alloc();
>>> -			if (!chain_fence) {
>>> +			fences = kmalloc_array(2, sizeof(*fences), GFP_KERNEL);
>>> +			if (!fences) {
>>> +				err = -ENOMEM;
>>> +				goto free_ifence;
>>> +			}
>>> +			cf = dma_fence_array_alloc(2);
>>> +			if (!cf) {
>>>    				err = -ENOMEM;
>>>    				goto free_ifence;
>>>    			}
>>> @@ -2068,9 +2077,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>>>    			invalidation_fence_init(tile->media_gt, mfence, fence,
>>>    						pt_update_ops->start,
>>>    						pt_update_ops->last, vm->usm.asid);
>>> -			dma_fence_chain_init(chain_fence, &ifence->base.base,
>>> -					     &mfence->base.base, 0);
>>> -			fence = &chain_fence->base;
>>> +			fences[0] = &ifence->base.base;
>>> +			fences[1] = &mfence->base.base;
>>> +			dma_fence_array_arm(cf, 2, fences,
>>> +					    vm->composite_fence_ctx,
>>> +					    vm->composite_fence_seqno++,
>>> +					    false);
>>> +			fence = &cf->base;
>>>    		} else {
>>>    			fence = &ifence->base.base;
>>>    		}
>>> @@ -2108,7 +2121,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
>>>    free_rfence:
>>>    	kfree(rfence);
>>>    free_ifence:
>>> -	dma_fence_chain_free(chain_fence);
>>> +	kfree(cf);
>>> +	kfree(fences);
>>>    	kfree(mfence);
>>>    	kfree(ifence);
>>>    kill_vm_tile1:

Re: [PATCH v3 2/2] drm/xe: Use dma-fence array for media GT TLB invalidations in PT code

[Matthew Brost]

On Mon, Aug 26, 2024 at 09:43:40AM +0200, Christian König wrote:
> Am 23.08.24 um 17:38 schrieb Matthew Brost:
> > On Fri, Aug 23, 2024 at 08:40:40AM +0200, Christian König wrote:
> > > Am 23.08.24 um 06:54 schrieb Matthew Brost:
> > > > Using a chain fence is problematic as these cannot be installed in
> > > > timeout drm sync objects. Use a dma-fence-array instead at the cost of
> > > > an extra failure point.
> > > Mhm, IIRC we converted chain objects into dma-fence-arrays while installing
> > > them into a timeline.
> > > 
> > > Doesn't that work any more?
> > > 
> > Thanks for the quick feedback.
> > 
> > As is, installing a dma-fence-chain into a timeline sync doesn't work.
> > 
> > The 'fence' returned from 'xe_pt_update_ops_run' is installed here [1]
> > as the 'fence' argument. This blows up here [2] [3]. It does suggest in
> > [3] to use a dma-fence-array which is what I'm doing.
> 
> Ah, that makes it more clear. You are not using some IOCTL to install the
> fences into a timeline but rather want to do this at the end of your
> submission IOCTL, right?
> 

Bind IOCTL, but correct. Submission and bind IOCTLs in Xe are
conceptually the same wrt to syncs.

> > The issue with using a dma-fence array as is it adds another failure
> > point if dma_fence_array_create is used as is after collecting multiple
> > fences from TLB invalidations. Also we have lock in xe_pt_update_ops_run
> > which is in the path reclaim so calling dma_fence_array_create isn't
> > allowed under that lock.
> 
> Ok that is a rather good argument for this.
> 
> Just tow comments I've seen on the code:
> 1. Please rename dma_fence_array_arm() into dma_fence_array_init()
> 2. Please drop WARN_ON(!array, a NULL array will result in a NULL pointer
> de-reference and crash anyway.
> 

Will do.

> > I suppose we could drop that lock and directly wait TLB invalidation
> > fences if dma_fence_array_create fails but to me it makes more sense to
> > prealloc the dma-fence-array and populate it later. Saw your response to
> > my first patch about how this could be problematic, a little confused on
> > that so responding there too.
> 
> Yeah people came up with the crazy idea to insert dma_fence_array objects
> into other dma_fence_array's resulting in overwriting the kernel stack when
> you free this construct finally.
> 
> Additional to that Sima pointed out during the initial review of this code
> that we should make sure that no circles can happen with a dma_fence.
>

Ah, yes. I could see how that could be an issue.
 
> But we now have a warning when somebody tries to add a container to a
> dma_fence_array object so that should probably be fine.
>

See the warn and agree this should protect against this type of problem
code.

Matt

> Regards,
> Christian.
> 
> > 
> > Matt
> > 
> > [1] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/gpu/drm/xe/xe_sync.c#L233
> > [2] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/gpu/drm/drm_syncobj.c#L349
> > [3] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma-buf/dma-fence-chain.c#L275
> > 
> > > Regards,
> > > Christian.
> > > 
> > > > Also fixup reserve fence count to include media GT invalidation fence.
> > > > 
> > > > v2:
> > > >    - Fix reserve fence count (Casey Bowman)
> > > > v3:
> > > >    - Prealloc dma fence array (CI)
> > > > 
> > > > Fixes: 40520283e0fd ("drm/xe: Invalidate media_gt TLBs in PT code")
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > >    drivers/gpu/drm/xe/xe_pt.c | 34 ++++++++++++++++++++++++----------
> > > >    1 file changed, 24 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > > > index 6c6714af3d5d..2e35444a85b0 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pt.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > > > @@ -3,7 +3,7 @@
> > > >     * Copyright © 2022 Intel Corporation
> > > >     */
> > > > -#include <linux/dma-fence-chain.h>
> > > > +#include <linux/dma-fence-array.h>
> > > >    #include "xe_pt.h"
> > > > @@ -1629,9 +1629,11 @@ xe_pt_update_ops_rfence_interval(struct xe_vm_pgtable_update_ops *pt_update_ops,
> > > >    static int vma_reserve_fences(struct xe_device *xe, struct xe_vma *vma)
> > > >    {
> > > > +	int shift = xe_device_get_root_tile(xe)->media_gt ? 1 : 0;
> > > > +
> > > >    	if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
> > > >    		return dma_resv_reserve_fences(xe_vma_bo(vma)->ttm.base.resv,
> > > > -					       xe->info.tile_count);
> > > > +					       xe->info.tile_count << shift);
> > > >    	return 0;
> > > >    }
> > > > @@ -1818,6 +1820,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
> > > >    	struct xe_vm_pgtable_update_ops *pt_update_ops =
> > > >    		&vops->pt_update_ops[tile->id];
> > > >    	struct xe_vma_op *op;
> > > > +	int shift = tile->media_gt ? 1 : 0;
> > > >    	int err;
> > > >    	lockdep_assert_held(&vops->vm->lock);
> > > > @@ -1826,7 +1829,7 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
> > > >    	xe_pt_update_ops_init(pt_update_ops);
> > > >    	err = dma_resv_reserve_fences(xe_vm_resv(vops->vm),
> > > > -				      tile_to_xe(tile)->info.tile_count);
> > > > +				      tile_to_xe(tile)->info.tile_count << shift);
> > > >    	if (err)
> > > >    		return err;
> > > > @@ -1983,7 +1986,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> > > >    		&vops->pt_update_ops[tile->id];
> > > >    	struct dma_fence *fence;
> > > >    	struct invalidation_fence *ifence = NULL, *mfence = NULL;
> > > > -	struct dma_fence_chain *chain_fence = NULL;
> > > > +	struct dma_fence **fences = NULL;
> > > > +	struct dma_fence_array *cf = NULL;
> > > >    	struct xe_range_fence *rfence;
> > > >    	struct xe_vma_op *op;
> > > >    	int err = 0, i;
> > > > @@ -2022,8 +2026,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> > > >    				err = -ENOMEM;
> > > >    				goto free_ifence;
> > > >    			}
> > > > -			chain_fence = dma_fence_chain_alloc();
> > > > -			if (!chain_fence) {
> > > > +			fences = kmalloc_array(2, sizeof(*fences), GFP_KERNEL);
> > > > +			if (!fences) {
> > > > +				err = -ENOMEM;
> > > > +				goto free_ifence;
> > > > +			}
> > > > +			cf = dma_fence_array_alloc(2);
> > > > +			if (!cf) {
> > > >    				err = -ENOMEM;
> > > >    				goto free_ifence;
> > > >    			}
> > > > @@ -2068,9 +2077,13 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> > > >    			invalidation_fence_init(tile->media_gt, mfence, fence,
> > > >    						pt_update_ops->start,
> > > >    						pt_update_ops->last, vm->usm.asid);
> > > > -			dma_fence_chain_init(chain_fence, &ifence->base.base,
> > > > -					     &mfence->base.base, 0);
> > > > -			fence = &chain_fence->base;
> > > > +			fences[0] = &ifence->base.base;
> > > > +			fences[1] = &mfence->base.base;
> > > > +			dma_fence_array_arm(cf, 2, fences,
> > > > +					    vm->composite_fence_ctx,
> > > > +					    vm->composite_fence_seqno++,
> > > > +					    false);
> > > > +			fence = &cf->base;
> > > >    		} else {
> > > >    			fence = &ifence->base.base;
> > > >    		}
> > > > @@ -2108,7 +2121,8 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> > > >    free_rfence:
> > > >    	kfree(rfence);
> > > >    free_ifence:
> > > > -	dma_fence_chain_free(chain_fence);
> > > > +	kfree(cf);
> > > > +	kfree(fences);
> > > >    	kfree(mfence);
> > > >    	kfree(ifence);
> > > >    kill_vm_tile1:
> 

Re: [PATCH v3 1/2] dma-buf: Split out dma fence array create into alloc and arm functions

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch