Message ID | 20200406091254.17675-2-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/5] drm/i915: Make exclusive awaits on i915_active optional | expand |
On 06/04/2020 10:12, Chris Wilson wrote: > Allow the caller to also wait upon the barriers stored in i915_active. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_active.c | 60 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_active.h | 1 + > 2 files changed, 61 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c > index d5e24be759f7..048ab9edd2c2 100644 > --- a/drivers/gpu/drm/i915/i915_active.c > +++ b/drivers/gpu/drm/i915/i915_active.c > @@ -542,6 +542,55 @@ static int __await_active(struct i915_active_fence *active, > return 0; > } > > +struct wait_barrier { > + struct wait_queue_entry base; > + struct i915_active *ref; > +}; > + > +static int > +barrier_wake(wait_queue_entry_t *wq, unsigned int mode, int flags, void *key) > +{ > + struct wait_barrier *wb = container_of(wq, typeof(*wb), base); > + > + if (i915_active_is_idle(wb->ref)) { /* shared waitqueue, must check! */ Who shares it? > + list_del(&wq->entry); > + i915_sw_fence_complete(wq->private); > + kfree(wq); > + } > + > + return 0; > +} > + > +static int __await_barrier(struct i915_active *ref, struct i915_sw_fence *fence) > +{ > + struct wait_barrier *wb; > + > + wb = kmalloc(sizeof(*wb), GFP_KERNEL); > + if (unlikely(!wb)) > + return -ENOMEM; > + > + if (!i915_active_acquire_if_busy(ref)) { > + kfree(wb); > + return 0; > + } > + > + if (!i915_sw_fence_await(fence)) { > + kfree(wb); > + i915_active_release(ref); > + return -EINVAL; > + } > + > + wb->base.flags = 0; > + wb->base.func = barrier_wake; > + wb->base.private = fence; > + wb->ref = ref; > + > + add_wait_queue(__var_waitqueue(ref), &wb->base); > + > + i915_active_release(ref); > + return 0; > +} > + > static int await_active(struct i915_active *ref, > unsigned int flags, > int (*fn)(void *arg, struct dma_fence *fence), > @@ -570,6 +619,16 @@ static int await_active(struct i915_active *ref, > return err; > } > > + if (flags & I915_ACTIVE_AWAIT_BARRIER) { > + err = flush_lazy_signals(ref); > + if (err) > + return err; > + > + err = __await_barrier(ref, arg); > + if (err) > + return err; > Could have a single set of active_acquire_if_busy/release over the previous and this new block. Not sure if that would help with any atomicity concerns, or if there are such. + } > + > return 0; > } > > @@ -582,6 +641,7 @@ int i915_request_await_active(struct i915_request *rq, > struct i915_active *ref, > unsigned int flags) > { > + GEM_BUG_ON(flags & I915_ACTIVE_AWAIT_BARRIER); Why is this an error? > return await_active(ref, flags, rq_await_fence, rq); > } > > diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h > index ffafaa78c494..cf4058150966 100644 > --- a/drivers/gpu/drm/i915/i915_active.h > +++ b/drivers/gpu/drm/i915/i915_active.h > @@ -195,6 +195,7 @@ int i915_request_await_active(struct i915_request *rq, > unsigned int flags); > #define I915_ACTIVE_AWAIT_EXCL BIT(0) > #define I915_ACTIVE_AWAIT_ACTIVE BIT(1) > +#define I915_ACTIVE_AWAIT_BARRIER BIT(2) > > int i915_active_acquire(struct i915_active *ref); > bool i915_active_acquire_if_busy(struct i915_active *ref); > Regards, Tvrtko
Quoting Tvrtko Ursulin (2020-04-06 13:06:03) > > On 06/04/2020 10:12, Chris Wilson wrote: > > Allow the caller to also wait upon the barriers stored in i915_active. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_active.c | 60 ++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_active.h | 1 + > > 2 files changed, 61 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c > > index d5e24be759f7..048ab9edd2c2 100644 > > --- a/drivers/gpu/drm/i915/i915_active.c > > +++ b/drivers/gpu/drm/i915/i915_active.c > > @@ -542,6 +542,55 @@ static int __await_active(struct i915_active_fence *active, > > return 0; > > } > > > > +struct wait_barrier { > > + struct wait_queue_entry base; > > + struct i915_active *ref; > > +}; > > + > > +static int > > +barrier_wake(wait_queue_entry_t *wq, unsigned int mode, int flags, void *key) > > +{ > > + struct wait_barrier *wb = container_of(wq, typeof(*wb), base); > > + > > + if (i915_active_is_idle(wb->ref)) { /* shared waitqueue, must check! */ > > Who shares it? __var_waitqueue(ref) => uses a one of a set of global workqueues based off hash(ref) Or we add a wait_queue_head_t to active, but we would still need to recheck as it may be reused as we are signaled. > > + if (flags & I915_ACTIVE_AWAIT_BARRIER) { > > + err = flush_lazy_signals(ref); > > + if (err) > > + return err; > > + > > + err = __await_barrier(ref, arg); > > + if (err) > > + return err; > > > > Could have a single set of active_acquire_if_busy/release over the > previous and this new block. Not sure if that would help with any > atomicity concerns, or if there are such. It would not affect correctness, it will just depend on taste. > + } > > + > > return 0; > > } > > > > @@ -582,6 +641,7 @@ int i915_request_await_active(struct i915_request *rq, > > struct i915_active *ref, > > unsigned int flags) > > { > > + GEM_BUG_ON(flags & I915_ACTIVE_AWAIT_BARRIER); > > Why is this an error? Because I'm being lazy and not hooking up the correct signaling path. Instead of signaling arg == fence, we would need &request->submit. Just messy on how to pass down the details. Maybe return await_active(ref, flags, rq_await_fence, rq, &rq->submit); and return await_active(ref, flags, sw_await_fence, fence, fence); That seems better than I was expecting. -Chris
Quoting Chris Wilson (2020-04-06 14:09:44) > Quoting Tvrtko Ursulin (2020-04-06 13:06:03) > > > > On 06/04/2020 10:12, Chris Wilson wrote: > > > Allow the caller to also wait upon the barriers stored in i915_active. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_active.c | 60 ++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_active.h | 1 + > > > 2 files changed, 61 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c > > > index d5e24be759f7..048ab9edd2c2 100644 > > > --- a/drivers/gpu/drm/i915/i915_active.c > > > +++ b/drivers/gpu/drm/i915/i915_active.c > > > @@ -542,6 +542,55 @@ static int __await_active(struct i915_active_fence *active, > > > return 0; > > > } > > > > > > +struct wait_barrier { > > > + struct wait_queue_entry base; > > > + struct i915_active *ref; > > > +}; > > > + > > > +static int > > > +barrier_wake(wait_queue_entry_t *wq, unsigned int mode, int flags, void *key) > > > +{ > > > + struct wait_barrier *wb = container_of(wq, typeof(*wb), base); > > > + > > > + if (i915_active_is_idle(wb->ref)) { /* shared waitqueue, must check! */ > > > > Who shares it? > > __var_waitqueue(ref) => uses a one of a set of global workqueues based > off hash(ref) > > Or we add a wait_queue_head_t to active, but we would still need to > recheck as it may be reused as we are signaled. > > > > + if (flags & I915_ACTIVE_AWAIT_BARRIER) { > > > + err = flush_lazy_signals(ref); > > > + if (err) > > > + return err; > > > + > > > + err = __await_barrier(ref, arg); > > > + if (err) > > > + return err; > > > > > > > Could have a single set of active_acquire_if_busy/release over the > > previous and this new block. Not sure if that would help with any > > atomicity concerns, or if there are such. > > It would not affect correctness, it will just depend on taste. Actually, flush_lazy_signals needs to be inside the active-ref, so we should rearrange the acquires. -Chris
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index d5e24be759f7..048ab9edd2c2 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -542,6 +542,55 @@ static int __await_active(struct i915_active_fence *active, return 0; } +struct wait_barrier { + struct wait_queue_entry base; + struct i915_active *ref; +}; + +static int +barrier_wake(wait_queue_entry_t *wq, unsigned int mode, int flags, void *key) +{ + struct wait_barrier *wb = container_of(wq, typeof(*wb), base); + + if (i915_active_is_idle(wb->ref)) { /* shared waitqueue, must check! */ + list_del(&wq->entry); + i915_sw_fence_complete(wq->private); + kfree(wq); + } + + return 0; +} + +static int __await_barrier(struct i915_active *ref, struct i915_sw_fence *fence) +{ + struct wait_barrier *wb; + + wb = kmalloc(sizeof(*wb), GFP_KERNEL); + if (unlikely(!wb)) + return -ENOMEM; + + if (!i915_active_acquire_if_busy(ref)) { + kfree(wb); + return 0; + } + + if (!i915_sw_fence_await(fence)) { + kfree(wb); + i915_active_release(ref); + return -EINVAL; + } + + wb->base.flags = 0; + wb->base.func = barrier_wake; + wb->base.private = fence; + wb->ref = ref; + + add_wait_queue(__var_waitqueue(ref), &wb->base); + + i915_active_release(ref); + return 0; +} + static int await_active(struct i915_active *ref, unsigned int flags, int (*fn)(void *arg, struct dma_fence *fence), @@ -570,6 +619,16 @@ static int await_active(struct i915_active *ref, return err; } + if (flags & I915_ACTIVE_AWAIT_BARRIER) { + err = flush_lazy_signals(ref); + if (err) + return err; + + err = __await_barrier(ref, arg); + if (err) + return err; + } + return 0; } @@ -582,6 +641,7 @@ int i915_request_await_active(struct i915_request *rq, struct i915_active *ref, unsigned int flags) { + GEM_BUG_ON(flags & I915_ACTIVE_AWAIT_BARRIER); return await_active(ref, flags, rq_await_fence, rq); } diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h index ffafaa78c494..cf4058150966 100644 --- a/drivers/gpu/drm/i915/i915_active.h +++ b/drivers/gpu/drm/i915/i915_active.h @@ -195,6 +195,7 @@ int i915_request_await_active(struct i915_request *rq, unsigned int flags); #define I915_ACTIVE_AWAIT_EXCL BIT(0) #define I915_ACTIVE_AWAIT_ACTIVE BIT(1) +#define I915_ACTIVE_AWAIT_BARRIER BIT(2) int i915_active_acquire(struct i915_active *ref); bool i915_active_acquire_if_busy(struct i915_active *ref);
Allow the caller to also wait upon the barriers stored in i915_active. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_active.c | 60 ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_active.h | 1 + 2 files changed, 61 insertions(+)