Re: [PATCH v3] drm/xe/ufence: Signal ufence immediately when possible

[Matthew Auld <matthew.auld@intel.com>]

On 18/10/2024 13:47, Nirmoy Das wrote:
> If the backing fence is signaled then signal ufence immediately.
> This should reduce load from the xe ordered_wq and also won't block
> signaling a ufence which doesn't require any serialization.
> 
> v2: fix system_wq typo
> v3: signal immediately instead of queuing in system_wq (Matt B)
> 
> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1630
> Cc: Matthew Auld <matthew.auld@intel.com>
> gc: Matthew Brost <matthew.brost@intel.com>

s/gc/Cc

> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_sync.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index c6cf227ead40..069c1e4ebea5 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
> @@ -72,10 +72,8 @@ static struct xe_user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>   	return ufence;
>   }
>   
> -static void user_fence_worker(struct work_struct *w)
> +static void signal_user_fence(struct xe_user_fence *ufence)
>   {
> -	struct xe_user_fence *ufence = container_of(w, struct xe_user_fence, worker);
> -
>   	if (mmget_not_zero(ufence->mm)) {
>   		kthread_use_mm(ufence->mm);
>   		if (copy_to_user(ufence->addr, &ufence->value, sizeof(ufence->value)))

This can end up in a CPU fault handler? There might be some locking 
issues if caller is say holding dma-resv. For example the caller in 
xe_exec which is holding dma-resv. If it can indeed hit this path, then 
we might get some splats/deadlocks, I think.

> @@ -89,6 +87,14 @@ static void user_fence_worker(struct work_struct *w)
>   	user_fence_put(ufence);
>   }
>   
> +static void user_fence_worker(struct work_struct *w)
> +{
> +	struct xe_user_fence *ufence = container_of(w, struct xe_user_fence,
> +						    worker);
> +
> +	signal_user_fence(ufence);
> +}
> +
>   static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence)
>   {
>   	INIT_WORK(&ufence->worker, user_fence_worker);
> @@ -236,7 +242,8 @@ void xe_sync_entry_signal(struct xe_sync_entry *sync, struct dma_fence *fence)
>   		err = dma_fence_add_callback(fence, &sync->ufence->cb,
>   					     user_fence_cb);
>   		if (err == -ENOENT) {
> -			kick_ufence(sync->ufence, fence);
> +			/* signal the ufence immediately if fence is already signalled */
> +			signal_user_fence(sync->ufence);
>   		} else if (err) {
>   			XE_WARN_ON("failed to add user fence");
>   			user_fence_put(sync->ufence);