Re: [PATCH v2 2/2] drm/xe: Don't issue TLB invalidations for VMAs if using execlists

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Feb 21, 2024 at 07:59:06PM -0800, Matthew Brost wrote:
> TLB invalidations for VMAs are currently only implemented with the GuC
> enabled. Do not issue TLB invalidations if using execlists. A longer
> term fix would be up the TLB invalidation layer to be execlist aware.
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pt.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 7f54bc3e389d..c4de13bcfe85 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -1256,9 +1256,11 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
>  	 */
>  	if ((rebind && !xe_vm_in_lr_mode(vm) && !vm->batch_invalidate_tlb) ||
>  	    (!rebind && xe_vm_has_scratch(vm) && xe_vm_in_preempt_fence_mode(vm))) {
> -		ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
> -		if (!ifence)
> -			return ERR_PTR(-ENOMEM);
> +		if (!vm->xe->info.force_execlist) {
> +			ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
> +			if (!ifence)
> +				return ERR_PTR(-ENOMEM);
> +		}
>  	}
>  
>  	rfence = kzalloc(sizeof(*rfence), GFP_KERNEL);
> @@ -1574,7 +1576,7 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queu
>  	struct xe_vm *vm = xe_vma_vm(vma);
>  	u32 num_entries;
>  	struct dma_fence *fence = NULL;
> -	struct invalidation_fence *ifence;
> +	struct invalidation_fence *ifence = NULL;
>  	struct xe_range_fence *rfence;
>  
>  	LLIST_HEAD(deferred);
> @@ -1593,9 +1595,11 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queu
>  	xe_pt_calc_rfence_interval(vma, &unbind_pt_update, entries,
>  				   num_entries);
>  
> -	ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
> -	if (!ifence)
> -		return ERR_PTR(-ENOMEM);
> +	if (!vm->xe->info.force_execlist) {
> +		ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
> +		if (!ifence)
> +			return ERR_PTR(-ENOMEM);
> +	}
>  
>  	rfence = kzalloc(sizeof(*rfence), GFP_KERNEL);
>  	if (!rfence) {
> @@ -1625,13 +1629,15 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queu
>  			dma_fence_wait(fence, false);
>  
>  		/* TLB invalidation must be done before signaling unbind */
> -		err = invalidation_fence_init(tile->primary_gt, ifence, fence, vma);
> -		if (err) {
> -			dma_fence_put(fence);
> -			kfree(ifence);
> -			return ERR_PTR(err);
> +		if (ifence) {

I'm not so sure about the indirections of the conditions here...

probably deserves some refactor with earlier return or different functions?

But anyway, let's unbreak things first while we think on a better
organization/separation.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +			err = invalidation_fence_init(tile->primary_gt, ifence, fence, vma);
> +			if (err) {
> +				dma_fence_put(fence);
> +				kfree(ifence);
> +				return ERR_PTR(err);
> +			}
> +			fence = &ifence->base.base;
>  		}
> -		fence = &ifence->base.base;
>  
>  		/* add shared fence now for pagetable delayed destroy */
>  		dma_resv_add_fence(xe_vm_resv(vm), fence,
> -- 
> 2.34.1
>