Re: [PATCH 1/2] drm/xe: Expose user fence from xe_sync_entry

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 14 Feb 2024, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> By allowing getting reference to user fence, we can
> control the lifetime outside of sync entries.
>
> This is needed to allow vma to track the associated
> user fence that was provided with bind ioctl.
>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_sync.c       | 63 +++++++++++++++++++++---------
>  drivers/gpu/drm/xe/xe_sync.h       |  4 ++
>  drivers/gpu/drm/xe/xe_sync_types.h | 20 +++++++---
>  3 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index aab92bee1d7cf..c3e745c7778b9 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
> @@ -19,39 +19,29 @@
>  #include "xe_macros.h"
>  #include "xe_sched_job_types.h"
>  
> -struct user_fence {
> -	struct xe_device *xe;
> -	struct kref refcount;
> -	struct dma_fence_cb cb;
> -	struct work_struct worker;
> -	struct mm_struct *mm;
> -	u64 __user *addr;
> -	u64 value;
> -};
> -
>  static void user_fence_destroy(struct kref *kref)
>  {
> -	struct user_fence *ufence = container_of(kref, struct user_fence,
> +	struct xe_user_fence *ufence = container_of(kref, struct xe_user_fence,
>  						 refcount);
>  
>  	mmdrop(ufence->mm);
>  	kfree(ufence);
>  }
>  
> -static void user_fence_get(struct user_fence *ufence)
> +static void user_fence_get(struct xe_user_fence *ufence)
>  {
>  	kref_get(&ufence->refcount);
>  }
>  
> -static void user_fence_put(struct user_fence *ufence)
> +static void user_fence_put(struct xe_user_fence *ufence)
>  {
>  	kref_put(&ufence->refcount, user_fence_destroy);
>  }
>  
> -static struct user_fence *user_fence_create(struct xe_device *xe, u64 addr,
> +static struct xe_user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>  					    u64 value)
>  {
> -	struct user_fence *ufence;
> +	struct xe_user_fence *ufence;
>  
>  	ufence = kmalloc(sizeof(*ufence), GFP_KERNEL);
>  	if (!ufence)
> @@ -69,7 +59,7 @@ static struct user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>  
>  static void user_fence_worker(struct work_struct *w)
>  {
> -	struct user_fence *ufence = container_of(w, struct user_fence, worker);
> +	struct xe_user_fence *ufence = container_of(w, struct xe_user_fence, worker);
>  
>  	if (mmget_not_zero(ufence->mm)) {
>  		kthread_use_mm(ufence->mm);
> @@ -80,10 +70,11 @@ static void user_fence_worker(struct work_struct *w)
>  	}
>  
>  	wake_up_all(&ufence->xe->ufence_wq);
> +	WRITE_ONCE(ufence->signalled, 1);
>  	user_fence_put(ufence);
>  }
>  
> -static void kick_ufence(struct user_fence *ufence, struct dma_fence *fence)
> +static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence)
>  {
>  	INIT_WORK(&ufence->worker, user_fence_worker);
>  	queue_work(ufence->xe->ordered_wq, &ufence->worker);
> @@ -92,7 +83,7 @@ static void kick_ufence(struct user_fence *ufence, struct dma_fence *fence)
>  
>  static void user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
>  {
> -	struct user_fence *ufence = container_of(cb, struct user_fence, cb);
> +	struct xe_user_fence *ufence = container_of(cb, struct xe_user_fence, cb);
>  
>  	kick_ufence(ufence, fence);
>  }
> @@ -340,3 +331,39 @@ xe_sync_in_fence_get(struct xe_sync_entry *sync, int num_sync,
>  
>  	return ERR_PTR(-ENOMEM);
>  }
> +
> +/**
> + * xe_sync_ufence_get() - Get user fence from sync
> + * @sync: input syncs
> + *
> + * Get a user fence reference from sync.
> + *
> + * Return: xe_user_fence pointer with reference
> + */
> +struct xe_user_fence *xe_sync_ufence_get(struct xe_sync_entry *sync)
> +{
> +	user_fence_get(sync->ufence);
> +
> +	return sync->ufence;
> +}
> +
> +/**
> + * xe_sync_ufence_put() - Put user fence reference
> + * @ufence: user fence reference
> + *
> + */
> +void xe_sync_ufence_put(struct xe_user_fence *ufence)
> +{
> +	user_fence_put(ufence);
> +}
> +
> +/**
> + * xe_sync_ufence_get_status() - Get user fence status
> + * @ufence: user fence
> + *
> + * Return: 1 if signalled, 0 not signalled, <0 on error
> + */
> +int xe_sync_ufence_get_status(struct xe_user_fence *ufence)
> +{
> +	return READ_ONCE(ufence->signalled);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_sync.h b/drivers/gpu/drm/xe/xe_sync.h
> index f43cdcaca6c57..0fd0d51208e62 100644
> --- a/drivers/gpu/drm/xe/xe_sync.h
> +++ b/drivers/gpu/drm/xe/xe_sync.h
> @@ -38,4 +38,8 @@ static inline bool xe_sync_is_ufence(struct xe_sync_entry *sync)
>  	return !!sync->ufence;
>  }
>  
> +struct xe_user_fence *xe_sync_ufence_get(struct xe_sync_entry *sync);
> +void xe_sync_ufence_put(struct xe_user_fence *ufence);
> +int xe_sync_ufence_get_status(struct xe_user_fence *ufence);
> +
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_sync_types.h b/drivers/gpu/drm/xe/xe_sync_types.h
> index 852db5e7884fc..6176ef9d65c72 100644
> --- a/drivers/gpu/drm/xe/xe_sync_types.h
> +++ b/drivers/gpu/drm/xe/xe_sync_types.h
> @@ -7,18 +7,28 @@
>  #define _XE_SYNC_TYPES_H_
>  
>  #include <linux/types.h>
> +#include <linux/kref.h>
> +#include <linux/dma-fence.h>
> +#include <linux/workqueue.h>
>  
> -struct drm_syncobj;
> -struct dma_fence;
> -struct dma_fence_chain;
>  struct drm_xe_sync;
> -struct user_fence;
> +
> +struct xe_user_fence {
> +	struct xe_device *xe;
> +	struct kref refcount;
> +	struct dma_fence_cb cb;
> +	struct work_struct worker;
> +	struct mm_struct *mm;
> +	u64 __user *addr;
> +	u64 value;
> +	int signalled;
> +};

Looking at the series, I think you could keep this as an opaque struct,
i.e. just forward declare struct xe_user_fence here, and leave the
definition inside xe_sync.c.

BR,
Jani.


>  
>  struct xe_sync_entry {
>  	struct drm_syncobj *syncobj;
>  	struct dma_fence *fence;
>  	struct dma_fence_chain *chain_fence;
> -	struct user_fence *ufence;
> +	struct xe_user_fence *ufence;
>  	u64 addr;
>  	u64 timeline_value;
>  	u32 type;

-- 
Jani Nikula, Intel