Re: [PATCH 1/2] drm/xe: fix unbalanced rpm put() with fence_fini()

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Oct 08, 2024 at 11:47:24AM +0100, Matthew Auld wrote:
> Currently we can call fence_fini() twice if something goes wrong when
> sending the GuC CT for the tlb request, since we signal the fence and
> return an error, leading to the caller also calling fini() on the error
> path in the case of stack version of the flow, which leads to an extra
> rpm put() which might later cause device to enter suspend when it
> shouldn't. It looks like we can just drop the fini() call since the
> fence signaller side will already call this for us.
> 
> There are known mysterious splats with device going to sleep even with
> an rpm ref, and this could be one candidate.
> 
> Fixes: 0a382f9bc5dc ("drm/xe: Hold a PM ref when GT TLB invalidations are inflight")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 26 ++++++++-------------
>  drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h |  1 -
>  drivers/gpu/drm/xe/xe_vm.c                  |  8 ++-----
>  3 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> index 98616de0c5bb..3eca8d680533 100644
> --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> @@ -37,6 +37,12 @@ static long tlb_timeout_jiffies(struct xe_gt *gt)
>  	return hw_tlb_timeout + 2 * delay;
>  }
>  
> +static void xe_gt_tlb_invalidation_fence_fini(struct xe_gt_tlb_invalidation_fence *fence)
> +{

To catch a double call of this:

if (WARN_ON_ONCE(!fence->gt))
	return;

Everything else LGTM. With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +	xe_pm_runtime_put(gt_to_xe(fence->gt));
> +	fence->gt = NULL; /* fini() should be called once */
> +}
> +
>  static void
>  __invalidation_fence_signal(struct xe_device *xe, struct xe_gt_tlb_invalidation_fence *fence)
>  {
> @@ -204,7 +210,7 @@ static int send_tlb_invalidation(struct xe_guc *guc,
>  						   tlb_timeout_jiffies(gt));
>  		}
>  		spin_unlock_irq(&gt->tlb_invalidation.pending_lock);
> -	} else if (ret < 0) {
> +	} else {
>  		__invalidation_fence_signal(xe, fence);
>  	}
>  	if (!ret) {
> @@ -267,10 +273,8 @@ int xe_gt_tlb_invalidation_ggtt(struct xe_gt *gt)
>  
>  		xe_gt_tlb_invalidation_fence_init(gt, &fence, true);
>  		ret = xe_gt_tlb_invalidation_guc(gt, &fence);
> -		if (ret < 0) {
> -			xe_gt_tlb_invalidation_fence_fini(&fence);
> +		if (ret)
>  			return ret;
> -		}
>  
>  		xe_gt_tlb_invalidation_fence_wait(&fence);
>  	} else if (xe_device_uc_enabled(xe) && !xe_device_wedged(xe)) {
> @@ -498,7 +502,8 @@ static const struct dma_fence_ops invalidation_fence_ops = {
>   * @stack: fence is stack variable
>   *
>   * Initialize TLB invalidation fence for use. xe_gt_tlb_invalidation_fence_fini
> - * must be called if fence is not signaled.
> + * will be automatically called when fence is signalled (all fences must signal),
> + * even on error.
>   */
>  void xe_gt_tlb_invalidation_fence_init(struct xe_gt *gt,
>  				       struct xe_gt_tlb_invalidation_fence *fence,
> @@ -518,14 +523,3 @@ void xe_gt_tlb_invalidation_fence_init(struct xe_gt *gt,
>  		dma_fence_get(&fence->base);
>  	fence->gt = gt;
>  }
> -
> -/**
> - * xe_gt_tlb_invalidation_fence_fini - Finalize TLB invalidation fence
> - * @fence: TLB invalidation fence to finalize
> - *
> - * Drop PM ref which fence took durinig init.
> - */
> -void xe_gt_tlb_invalidation_fence_fini(struct xe_gt_tlb_invalidation_fence *fence)
> -{
> -	xe_pm_runtime_put(gt_to_xe(fence->gt));
> -}
> diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> index a84065fa324c..f430d5797af7 100644
> --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> @@ -28,7 +28,6 @@ int xe_guc_tlb_invalidation_done_handler(struct xe_guc *guc, u32 *msg, u32 len);
>  void xe_gt_tlb_invalidation_fence_init(struct xe_gt *gt,
>  				       struct xe_gt_tlb_invalidation_fence *fence,
>  				       bool stack);
> -void xe_gt_tlb_invalidation_fence_fini(struct xe_gt_tlb_invalidation_fence *fence);
>  
>  static inline void
>  xe_gt_tlb_invalidation_fence_wait(struct xe_gt_tlb_invalidation_fence *fence)
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index ce9dca4d4e87..c99380271de6 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3199,10 +3199,8 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
>  
>  			ret = xe_gt_tlb_invalidation_vma(tile->primary_gt,
>  							 &fence[fence_id], vma);
> -			if (ret < 0) {
> -				xe_gt_tlb_invalidation_fence_fini(&fence[fence_id]);
> +			if (ret)
>  				goto wait;
> -			}
>  			++fence_id;
>  
>  			if (!tile->media_gt)
> @@ -3214,10 +3212,8 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
>  
>  			ret = xe_gt_tlb_invalidation_vma(tile->media_gt,
>  							 &fence[fence_id], vma);
> -			if (ret < 0) {
> -				xe_gt_tlb_invalidation_fence_fini(&fence[fence_id]);
> +			if (ret)
>  				goto wait;
> -			}
>  			++fence_id;
>  		}
>  	}
> -- 
> 2.46.2
>