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@xxxxxxxxxxxxxxx>
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
