Re: [PATCH 1/2] drm/xe: Take a reference in xe_exec_queue_last_fence_get()

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

On 2024-02-01 01:48, Matthew Brost wrote:
> Take a reference in xe_exec_queue_last_fence_get(). Also fix a reference
> counting underflow bug VM bind and unbind.
> 
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_exec_queue.c | 8 ++++++--
>   drivers/gpu/drm/xe/xe_migrate.c    | 5 ++++-
>   drivers/gpu/drm/xe/xe_sched_job.c  | 1 -
>   drivers/gpu/drm/xe/xe_sync.c       | 2 --
>   drivers/gpu/drm/xe/xe_vm.c         | 1 +
>   5 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index c0b7434e78f1..2976635be4d3 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -976,20 +976,24 @@ void xe_exec_queue_last_fence_put_unlocked(struct xe_exec_queue *q)
>    * @q: The exec queue
>    * @vm: The VM the engine does a bind or exec for
>    *
> - * Get last fence, does not take a ref
> + * Get last fence, takes a ref
>    *
>    * Returns: last fence if not signaled, dma fence stub if signaled
>    */
>   struct dma_fence *xe_exec_queue_last_fence_get(struct xe_exec_queue *q,
>   					       struct xe_vm *vm)
>   {
> +	struct dma_fence *fence;
> +
>   	xe_exec_queue_last_fence_lockdep_assert(q, vm);
>   
>   	if (q->last_fence &&
>   	    test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &q->last_fence->flags))
>   		xe_exec_queue_last_fence_put(q, vm);
>   
> -	return q->last_fence ? q->last_fence : dma_fence_get_stub();
> +	fence = q->last_fence ? q->last_fence : dma_fence_get_stub();
> +	dma_fence_get(fence);
This should be:
	fence = q->last_fence ? dma_fence_get(q->last_fence) :
		dma_fence_get_stub();

Otherwise we increase refcount on stub twice.
Even though it's harmless as it's statically allocated and never 
expected to be freed, we probably shouldn't.

With that fixed for this patch
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> +	return fence;
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 9ab004871f9a..894e36c28f32 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -1214,8 +1214,11 @@ static bool no_in_syncs(struct xe_vm *vm, struct xe_exec_queue *q,
>   	}
>   	if (q) {
>   		fence = xe_exec_queue_last_fence_get(q, vm);
> -		if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +		if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +			dma_fence_put(fence);
>   			return false;
> +		}
> +		dma_fence_put(fence);
>   	}
>   
>   	return true;
> diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c
> index cde1407867db..8151ddafb940 100644
> --- a/drivers/gpu/drm/xe/xe_sched_job.c
> +++ b/drivers/gpu/drm/xe/xe_sched_job.c
> @@ -274,7 +274,6 @@ int xe_sched_job_last_fence_add_dep(struct xe_sched_job *job, struct xe_vm *vm)
>   	struct dma_fence *fence;
>   
>   	fence = xe_exec_queue_last_fence_get(job->q, vm);
> -	dma_fence_get(fence);
>   
>   	return drm_sched_job_add_dependency(&job->drm, fence);
>   }
> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index e4c220cf9115..aab92bee1d7c 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
> @@ -307,7 +307,6 @@ xe_sync_in_fence_get(struct xe_sync_entry *sync, int num_sync,
>   	/* Easy case... */
>   	if (!num_in_fence) {
>   		fence = xe_exec_queue_last_fence_get(q, vm);
> -		dma_fence_get(fence);
>   		return fence;
>   	}
>   
> @@ -322,7 +321,6 @@ xe_sync_in_fence_get(struct xe_sync_entry *sync, int num_sync,
>   		}
>   	}
>   	fences[current_fence++] = xe_exec_queue_last_fence_get(q, vm);
> -	dma_fence_get(fences[current_fence - 1]);
>   	cf = dma_fence_array_create(num_in_fence, fences,
>   				    vm->composite_fence_ctx,
>   				    vm->composite_fence_seqno++,
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 8576535c4b6a..e55161136490 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1959,6 +1959,7 @@ static int xe_vm_prefetch(struct xe_vm *vm, struct xe_vma *vma,
>   					xe_exec_queue_last_fence_get(wait_exec_queue, vm);
>   
>   				xe_sync_entry_signal(&syncs[i], NULL, fence);
> +				dma_fence_put(fence);
>   			}
>   		}
>