Re: [PATCH 1/6] drm/i915: Update dma_fence_work

On 10/13/21 14:41, Daniel Vetter wrote:

On Fri, Oct 08, 2021 at 03:35:25PM +0200, Thomas Hellström wrote:

...

Move the release callback to after fence signaling to align with what's done for upcoming VM_BIND user-fence signaling.

Finally call the work callback regardless of whether we have a fence error or not and update the existing callbacks accordingly. We will need this to intercept the error for failsafe migration.

Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com

I think before we make this thing more complex we really should either move this into dma-buf/ as a proper thing, or just open-code.

Minimally at least any new async dma_fence worker needs to have dma_fence_begin/end_signalling annotations, or we're just digging a grave here.

I'm also not seeing the point in building everything on top of this, for many cases just an open-coded work_struct should be a lot simpler. It's just more to clean up later on, that part is for sure. -Daniel

Yes, I mentioned to Matthew, I'm going to respin this based on our previous discussions.

Forgot to mention on the ML.

/Thomas

...

---

drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 5 +++ drivers/gpu/drm/i915/i915_sw_fence_work.c | 36 ++++++++++----------- drivers/gpu/drm/i915/i915_sw_fence_work.h | 1 + drivers/gpu/drm/i915/i915_vma.c | 12 +++++-- 4 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c index f0435c6feb68..2143ebaf5b6f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c @@ -28,6 +28,11 @@ static void clflush_work(struct dma_fence_work *base) { struct clflush *clflush = container_of(base, typeof(*clflush), base);

• if (base->error) {

• dma_fence_set_error(&base->dma, base->error);
  

• return;
  

• }
• __do_clflush(clflush->obj); }

diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.c b/drivers/gpu/drm/i915/i915_sw_fence_work.c index 5b33ef23d54c..5b55cddafc9b 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence_work.c +++ b/drivers/gpu/drm/i915/i915_sw_fence_work.c @@ -6,21 +6,24 @@

#include "i915_sw_fence_work.h"

-static void fence_complete(struct dma_fence_work *f) +static void dma_fence_work_complete(struct dma_fence_work *f) {

• dma_fence_signal(&f->dma);
• if (f->ops->release) f->ops->release(f);

• dma_fence_signal(&f->dma);

• dma_fence_put(&f->dma); }

-static void fence_work(struct work_struct *work) +static void dma_fence_work_work(struct work_struct *work) { struct dma_fence_work *f = container_of(work, typeof(*f), work);

• f->ops->work(f);

• if (f->ops->work)

• f->ops->work(f);
  
  

• fence_complete(f);
• dma_fence_put(&f->dma);

• dma_fence_work_complete(f); }

  static int __i915_sw_fence_call

@@ -31,17 +34,13 @@ fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state) switch (state) { case FENCE_COMPLETE: if (fence->error)

• 	dma_fence_set_error(&f->dma, fence->error);
  

• if (!f->dma.error) {
  

• 	dma_fence_get(&f->dma);
  

• 	if (test_bit(DMA_FENCE_WORK_IMM, &f->dma.flags))
  

• 		fence_work(&f->work);
  

• 	else
  

• 		queue_work(system_unbound_wq, &f->work);
  

• } else {
  

• 	fence_complete(f);
  

• }
  

• 	cmpxchg(&f->error, 0, fence->error);
  

• dma_fence_get(&f->dma);
  

• if (test_bit(DMA_FENCE_WORK_IMM, &f->dma.flags))
  

• 	dma_fence_work_work(&f->work);
  

• else
  

• 	queue_work(system_unbound_wq, &f->work);
  

  break;

  case FENCE_FREE:

@@ -84,10 +83,11 @@ void dma_fence_work_init(struct dma_fence_work *f, const struct dma_fence_work_ops *ops) { f->ops = ops;

• f->error = 0; spin_lock_init(&f->lock); dma_fence_init(&f->dma, &fence_ops, &f->lock, 0, 0); i915_sw_fence_init(&f->chain, fence_notify);

• INIT_WORK(&f->work, fence_work);

• INIT_WORK(&f->work, dma_fence_work_work); }

  int dma_fence_work_chain(struct dma_fence_work *f, struct dma_fence *signal)

diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.h b/drivers/gpu/drm/i915/i915_sw_fence_work.h index d56806918d13..caa59fb5252b 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence_work.h +++ b/drivers/gpu/drm/i915/i915_sw_fence_work.h @@ -24,6 +24,7 @@ struct dma_fence_work_ops { struct dma_fence_work { struct dma_fence dma; spinlock_t lock;

• int error;

  struct i915_sw_fence chain; struct i915_sw_dma_fence_cb cb;

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 4b7fc4647e46..5123ac28ad9a 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -301,6 +301,11 @@ static void __vma_bind(struct dma_fence_work *work) struct i915_vma_work *vw = container_of(work, typeof(*vw), base); struct i915_vma *vma = vw->vma;

• if (work->error) {

• dma_fence_set_error(&work->dma, work->error);
  

• return;
  

• }
• vma->ops->bind_vma(vw->vm, &vw->stash, vma, vw->cache_level, vw->flags); }

@@ -333,7 +338,7 @@ struct i915_vma_work *i915_vma_work(void) return NULL;

dma_fence_work_init(&vw->base, &bind_ops);

• vw->base.dma.error = -EAGAIN; /* disable the worker by default */

• vw->base.error = -EAGAIN; /* disable the worker by default */

  return vw; }

@@ -416,6 +421,9 @@ int i915_vma_bind(struct i915_vma *vma, * part of the obj->resv->excl_fence as it only affects * execution and not content or object's backing store lifetime. */

• work->base.error = 0; /* enable the queue_work() */
  

• prev = i915_active_set_exclusive(&vma->active, &work->base.dma); if (prev) { __i915_sw_fence_await_dma_fence(&work->base.chain,

@@ -424,8 +432,6 @@ int i915_vma_bind(struct i915_vma *vma, dma_fence_put(prev); }

• work->base.dma.error = 0; /* enable the queue_work() */
  

• if (vma->obj) { __i915_gem_object_pin_pages(vma->obj); work->pinned = i915_gem_object_get(vma->obj);

-- 2.31.1