Re: [Intel-gfx] [PATCH 4/6] drm/i915: Add a struct dma_fence_work timeline

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 08, 2021 at 03:35:28PM +0200, Thomas Hellström wrote:
> The TTM managers and, possibly, the gtt address space managers will
> need to be able to order fences for async operation.
> Using dma_fence_is_later() for this will require that the fences we hand
> them are from a single fence context and ordered.
> 
> Introduce a struct dma_fence_work_timeline, and a function to attach
> struct dma_fence_work to such a timeline in a way that all previous
> fences attached to the timeline will be signaled when the latest
> attached struct dma_fence_work signals.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

I'm not understanding why we need this:

- if we just want to order dma_fence work, then an ordered workqueue is
  what we want. Which is why hand-rolling is better than reusing
  dma_fence_work for absolutely everything.

- if we just need to make sure the public fences signal in order, then
  it's a dma_fence_chain.

Definitely no more "it looks like it's shared code but isn't" stuff in
i915.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_sw_fence_work.c | 89 ++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_sw_fence_work.h | 58 +++++++++++++++
>  2 files changed, 145 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.c b/drivers/gpu/drm/i915/i915_sw_fence_work.c
> index 5b55cddafc9b..87cdb3158042 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence_work.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence_work.c
> @@ -5,6 +5,66 @@
>   */
>  
>  #include "i915_sw_fence_work.h"
> +#include "i915_utils.h"
> +
> +/**
> + * dma_fence_work_timeline_attach - Attach a struct dma_fence_work to a
> + * timeline.
> + * @tl: The timeline to attach to.
> + * @f: The struct dma_fence_work.
> + * @tl_cb: The i915_sw_dma_fence_cb needed to attach to the
> + * timeline. This is typically embedded into the structure that also
> + * embeds the struct dma_fence_work.
> + *
> + * This function takes a timeline reference and associates it with the
> + * struct dma_fence_work. That reference is given up when the fence
> + * signals. Furthermore it assigns a fence context and a seqno to the
> + * dma-fence, and then chains upon the previous fence of the timeline
> + * if any, to make sure that the fence signals after that fence. The
> + * @tl_cb callback structure is needed for that chaining. Finally
> + * the registered last fence of the timeline is replaced by this fence, and
> + * the timeline takes a reference on the fence, which is released when
> + * the fence signals.
> + */
> +void dma_fence_work_timeline_attach(struct dma_fence_work_timeline *tl,
> +				    struct dma_fence_work *f,
> +				    struct i915_sw_dma_fence_cb *tl_cb)
> +{
> +	struct dma_fence *await;
> +
> +	if (tl->ops->get)
> +		tl->ops->get(tl);
> +
> +	spin_lock(&tl->lock);
> +	await = tl->last_fence;
> +	tl->last_fence = dma_fence_get(&f->dma);
> +	f->dma.seqno = tl->seqno++;
> +	f->dma.context = tl->context;
> +	f->tl = tl;
> +	spin_unlock(&tl->lock);
> +
> +	if (await) {
> +		__i915_sw_fence_await_dma_fence(&f->chain, await, tl_cb);
> +		dma_fence_put(await);
> +	}
> +}
> +
> +static void dma_fence_work_timeline_detach(struct dma_fence_work *f)
> +{
> +	struct dma_fence_work_timeline *tl = f->tl;
> +	bool put = false;
> +
> +	spin_lock(&tl->lock);
> +	if (tl->last_fence == &f->dma) {
> +		put = true;
> +		tl->last_fence = NULL;
> +	}
> +	spin_unlock(&tl->lock);
> +	if (tl->ops->put)
> +		tl->ops->put(tl);
> +	if (put)
> +		dma_fence_put(&f->dma);
> +}
>  
>  static void dma_fence_work_complete(struct dma_fence_work *f)
>  {
> @@ -13,6 +73,9 @@ static void dma_fence_work_complete(struct dma_fence_work *f)
>  	if (f->ops->release)
>  		f->ops->release(f);
>  
> +	if (f->tl)
> +		dma_fence_work_timeline_detach(f);
> +
>  	dma_fence_put(&f->dma);
>  }
>  
> @@ -53,14 +116,17 @@ fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
>  
>  static const char *get_driver_name(struct dma_fence *fence)
>  {
> -	return "dma-fence";
> +	struct dma_fence_work *f = container_of(fence, typeof(*f), dma);
> +
> +	return (f->tl && f->tl->ops->name) ? f->tl->ops->name : "dma-fence";
>  }
>  
>  static const char *get_timeline_name(struct dma_fence *fence)
>  {
>  	struct dma_fence_work *f = container_of(fence, typeof(*f), dma);
>  
> -	return f->ops->name ?: "work";
> +	return (f->tl && f->tl->name) ? f->tl->name :
> +		f->ops->name ?: "work";
>  }
>  
>  static void fence_release(struct dma_fence *fence)
> @@ -84,6 +150,7 @@ void dma_fence_work_init(struct dma_fence_work *f,
>  {
>  	f->ops = ops;
>  	f->error = 0;
> +	f->tl = NULL;
>  	spin_lock_init(&f->lock);
>  	dma_fence_init(&f->dma, &fence_ops, &f->lock, 0, 0);
>  	i915_sw_fence_init(&f->chain, fence_notify);
> @@ -97,3 +164,21 @@ int dma_fence_work_chain(struct dma_fence_work *f, struct dma_fence *signal)
>  
>  	return __i915_sw_fence_await_dma_fence(&f->chain, signal, &f->cb);
>  }
> +
> +/**
> + * dma_fence_work_timeline_init - Initialize a dma_fence_work timeline
> + * @tl: The timeline to initialize,
> + * @name: The name of the timeline,
> + * @ops: The timeline operations.
> + */
> +void dma_fence_work_timeline_init(struct dma_fence_work_timeline *tl,
> +				  const char *name,
> +				  const struct dma_fence_work_timeline_ops *ops)
> +{
> +	tl->name = name;
> +	spin_lock_init(&tl->lock);
> +	tl->context = dma_fence_context_alloc(1);
> +	tl->seqno = 0;
> +	tl->last_fence = NULL;
> +	tl->ops = ops;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.h b/drivers/gpu/drm/i915/i915_sw_fence_work.h
> index caa59fb5252b..6f41ee360133 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence_work.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence_work.h
> @@ -14,6 +14,53 @@
>  #include "i915_sw_fence.h"
>  
>  struct dma_fence_work;
> +struct dma_fence_work_timeline;
> +
> +/**
> + * struct dma_fence_work_timeline_ops - Timeline operations struct
> + * @name: Timeline ops name. This field is used if the timeline itself has
> + * a NULL name. Can be set to NULL in which case a default name is used.
> + *
> + * The struct dma_fence_work_timeline is intended to be embeddable.
> + * We use the ops to get and put the parent structure.
> + */
> +struct dma_fence_work_timeline_ops {
> +	/**
> +	 * Timeline ops name. Used if the timeline itself has no name.
> +	 */
> +	const char *name;
> +
> +	/**
> +	 * put() - Put the structure embedding the timeline
> +	 * @tl: The timeline
> +	 */
> +	void (*put)(struct dma_fence_work_timeline *tl);
> +
> +	/**
> +	 * get() - Get the structure embedding the timeline
> +	 * @tl: The timeline
> +	 */
> +	void (*get)(struct dma_fence_work_timeline *tl);
> +};
> +
> +/**
> + * struct dma_fence_work_timeline - Simple timeline struct for dma_fence_work
> + * @name: The name of the timeline. May be set to NULL. Immutable
> + * @lock: Protects mutable members of the structure.
> + * @context: The timeline fence context. Immutable.
> + * @seqno: The previous seqno used. Protected by @lock.
> + * @last_fence : The previous fence of the timeline. Protected by @lock.
> + * @ops: The timeline operations struct. Immutable.
> + */
> +struct dma_fence_work_timeline {
> +	const char *name;
> +	/** Protects mutable members of the structure */
> +	spinlock_t lock;
> +	u64 context;
> +	u64 seqno;
> +	struct dma_fence *last_fence;
> +	const struct dma_fence_work_timeline_ops *ops;
> +};
>  
>  struct dma_fence_work_ops {
>  	const char *name;
> @@ -30,6 +77,9 @@ struct dma_fence_work {
>  	struct i915_sw_dma_fence_cb cb;
>  
>  	struct work_struct work;
> +
> +	struct dma_fence_work_timeline *tl;
> +
>  	const struct dma_fence_work_ops *ops;
>  };
>  
> @@ -65,4 +115,12 @@ static inline void dma_fence_work_commit_imm(struct dma_fence_work *f)
>  	dma_fence_work_commit(f);
>  }
>  
> +void dma_fence_work_timeline_attach(struct dma_fence_work_timeline *tl,
> +				    struct dma_fence_work *f,
> +				    struct i915_sw_dma_fence_cb *tl_cb);
> +
> +void dma_fence_work_timeline_init(struct dma_fence_work_timeline *tl,
> +				  const char *name,
> +				  const struct dma_fence_work_timeline_ops *ops);
> +
>  #endif /* I915_SW_FENCE_WORK_H */
> -- 
> 2.31.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch