| Message ID | 20211008133530.664509-2-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Failsafe migration blits | expand |
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 > --- > 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 >
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 >>
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);
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> --- 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(-)