| Message ID | 20161007094635.28319-8-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/10/2016 10:46, Chris Wilson wrote: > In forthcoming patches, we want to be able to dynamically allocate the > wait_queue_t used whilst awaiting. This is more convenient if we extend > the i915_sw_fence_await_sw_fence() to perform the allocation for us if > we pass in a gfp mask as an alternative than a preallocated struct. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/i915_sw_fence.h | 8 ++++++++ > 2 files changed, 44 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index 1e5cbc585ca2..dfb59dced0ce 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -13,6 +13,8 @@ > > #include "i915_sw_fence.h" > > +#define I915_SW_FENCE_FLAG_ALLOC BIT(0) > + You collide with WQ_FLAG_EXCLUSIVE here, so I assume i915 will have complete control of the wq in question? > static DEFINE_SPINLOCK(i915_sw_fence_lock); > > static int __i915_sw_fence_notify(struct i915_sw_fence *fence, > @@ -135,6 +137,8 @@ static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void * > list_del(&wq->task_list); > __i915_sw_fence_complete(wq->private, key); > i915_sw_fence_put(wq->private); > + if (wq->flags & I915_SW_FENCE_FLAG_ALLOC) > + kfree(wq); > return 0; > } > > @@ -192,9 +196,9 @@ static bool i915_sw_fence_check_if_after(struct i915_sw_fence *fence, > return err; > } > > -int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > - struct i915_sw_fence *signaler, > - wait_queue_t *wq) > +static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > + struct i915_sw_fence *signaler, > + wait_queue_t *wq, gfp_t gfp) > { > unsigned long flags; > int pending; > @@ -206,8 +210,22 @@ int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > if (unlikely(i915_sw_fence_check_if_after(fence, signaler))) > return -EINVAL; > > + pending = 0; > + if (!wq) { > + wq = kmalloc(sizeof(*wq), gfp); > + if (!wq) { > + if (!gfpflags_allow_blocking(gfp)) > + return -ENOMEM; > + > + i915_sw_fence_wait(signaler); This looks like a strange API, unless I am missing something - it normally sets things up for waiting, unless the allocation fails when it actually does the wait? > + return 0; > + } > + > + pending |= I915_SW_FENCE_FLAG_ALLOC; Why is this stored in pending? Just because it is around as re-used? > + } > + > INIT_LIST_HEAD(&wq->task_list); > - wq->flags = 0; > + wq->flags = pending; > wq->func = i915_sw_fence_wake; > wq->private = i915_sw_fence_get(fence); > > @@ -226,6 +244,20 @@ int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > return pending; > } > > +int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > + struct i915_sw_fence *signaler, > + wait_queue_t *wq) > +{ > + return __i915_sw_fence_await_sw_fence(fence, signaler, wq, 0); > +} > + > +int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence, > + struct i915_sw_fence *signaler, > + gfp_t gfp) > +{ > + return __i915_sw_fence_await_sw_fence(fence, signaler, NULL, gfp); > +} > + > struct dma_fence_cb { > struct fence_cb base; > struct i915_sw_fence *fence; > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h > index 373141602ca4..5861c69f24d4 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.h > +++ b/drivers/gpu/drm/i915/i915_sw_fence.h > @@ -46,6 +46,9 @@ void i915_sw_fence_commit(struct i915_sw_fence *fence); > int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > struct i915_sw_fence *after, > wait_queue_t *wq); > +int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence, > + struct i915_sw_fence *after, > + gfp_t gfp); > int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence, > struct fence *dma, > unsigned long timeout, > @@ -62,4 +65,9 @@ static inline bool i915_sw_fence_done(const struct i915_sw_fence *fence) > return atomic_read(&fence->pending) < 0; > } > > +static inline void i915_sw_fence_wait(struct i915_sw_fence *fence) > +{ > + wait_event(fence->wait, i915_sw_fence_done(fence)); > +} > + > #endif /* _I915_SW_FENCE_H_ */ Regards, Tvrtko
On Fri, Oct 07, 2016 at 05:10:04PM +0100, Tvrtko Ursulin wrote: > > On 07/10/2016 10:46, Chris Wilson wrote: > >In forthcoming patches, we want to be able to dynamically allocate the > >wait_queue_t used whilst awaiting. This is more convenient if we extend > >the i915_sw_fence_await_sw_fence() to perform the allocation for us if > >we pass in a gfp mask as an alternative than a preallocated struct. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >--- > > drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++---- > > drivers/gpu/drm/i915/i915_sw_fence.h | 8 ++++++++ > > 2 files changed, 44 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > >index 1e5cbc585ca2..dfb59dced0ce 100644 > >--- a/drivers/gpu/drm/i915/i915_sw_fence.c > >+++ b/drivers/gpu/drm/i915/i915_sw_fence.c > >@@ -13,6 +13,8 @@ > > #include "i915_sw_fence.h" > >+#define I915_SW_FENCE_FLAG_ALLOC BIT(0) > >+ > > You collide with WQ_FLAG_EXCLUSIVE here, so I assume i915 will have > complete control of the wq in question? Hmm, we should move bit as well, but yes the flag is atm private to the wake up function for which we use a custom function. > >+ wq = kmalloc(sizeof(*wq), gfp); > >+ if (!wq) { > >+ if (!gfpflags_allow_blocking(gfp)) > >+ return -ENOMEM; > >+ > >+ i915_sw_fence_wait(signaler); > > This looks like a strange API, unless I am missing something - it > normally sets things up for waiting, unless the allocation fails > when it actually does the wait? Yes. If we blocked on the malloc and failed, we block a little more for the completion. iirc, the idea came from the async task. > > >+ return 0; > >+ } > >+ > >+ pending |= I915_SW_FENCE_FLAG_ALLOC; > > Why is this stored in pending? Just because it is around as re-used? Urm, yes. s/pending/tmp/ -Chris
On 07/10/2016 17:22, Chris Wilson wrote: > On Fri, Oct 07, 2016 at 05:10:04PM +0100, Tvrtko Ursulin wrote: >> On 07/10/2016 10:46, Chris Wilson wrote: >>> In forthcoming patches, we want to be able to dynamically allocate the >>> wait_queue_t used whilst awaiting. This is more convenient if we extend >>> the i915_sw_fence_await_sw_fence() to perform the allocation for us if >>> we pass in a gfp mask as an alternative than a preallocated struct. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++---- >>> drivers/gpu/drm/i915/i915_sw_fence.h | 8 ++++++++ >>> 2 files changed, 44 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c >>> index 1e5cbc585ca2..dfb59dced0ce 100644 >>> --- a/drivers/gpu/drm/i915/i915_sw_fence.c >>> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c >>> @@ -13,6 +13,8 @@ >>> #include "i915_sw_fence.h" >>> +#define I915_SW_FENCE_FLAG_ALLOC BIT(0) >>> + >> You collide with WQ_FLAG_EXCLUSIVE here, so I assume i915 will have >> complete control of the wq in question? > Hmm, we should move bit as well, but yes the flag is atm private to the > wake up function for which we use a custom function. > >>> + wq = kmalloc(sizeof(*wq), gfp); >>> + if (!wq) { >>> + if (!gfpflags_allow_blocking(gfp)) >>> + return -ENOMEM; >>> + >>> + i915_sw_fence_wait(signaler); >> This looks like a strange API, unless I am missing something - it >> normally sets things up for waiting, unless the allocation fails >> when it actually does the wait? > Yes. If we blocked on the malloc and failed, we block a little more for > the completion. iirc, the idea came from the async task. Still sounds strange to me. I understand it is a unlikely corner case but sounds like that sort of decision should happen in the caller. I will have to revisit the i915_sw_fence implementation and usage in more detail it seems. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index 1e5cbc585ca2..dfb59dced0ce 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -13,6 +13,8 @@ #include "i915_sw_fence.h" +#define I915_SW_FENCE_FLAG_ALLOC BIT(0) + static DEFINE_SPINLOCK(i915_sw_fence_lock); static int __i915_sw_fence_notify(struct i915_sw_fence *fence, @@ -135,6 +137,8 @@ static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void * list_del(&wq->task_list); __i915_sw_fence_complete(wq->private, key); i915_sw_fence_put(wq->private); + if (wq->flags & I915_SW_FENCE_FLAG_ALLOC) + kfree(wq); return 0; } @@ -192,9 +196,9 @@ static bool i915_sw_fence_check_if_after(struct i915_sw_fence *fence, return err; } -int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, - struct i915_sw_fence *signaler, - wait_queue_t *wq) +static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, + struct i915_sw_fence *signaler, + wait_queue_t *wq, gfp_t gfp) { unsigned long flags; int pending; @@ -206,8 +210,22 @@ int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, if (unlikely(i915_sw_fence_check_if_after(fence, signaler))) return -EINVAL; + pending = 0; + if (!wq) { + wq = kmalloc(sizeof(*wq), gfp); + if (!wq) { + if (!gfpflags_allow_blocking(gfp)) + return -ENOMEM; + + i915_sw_fence_wait(signaler); + return 0; + } + + pending |= I915_SW_FENCE_FLAG_ALLOC; + } + INIT_LIST_HEAD(&wq->task_list); - wq->flags = 0; + wq->flags = pending; wq->func = i915_sw_fence_wake; wq->private = i915_sw_fence_get(fence); @@ -226,6 +244,20 @@ int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, return pending; } +int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, + struct i915_sw_fence *signaler, + wait_queue_t *wq) +{ + return __i915_sw_fence_await_sw_fence(fence, signaler, wq, 0); +} + +int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence, + struct i915_sw_fence *signaler, + gfp_t gfp) +{ + return __i915_sw_fence_await_sw_fence(fence, signaler, NULL, gfp); +} + struct dma_fence_cb { struct fence_cb base; struct i915_sw_fence *fence; diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h index 373141602ca4..5861c69f24d4 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.h +++ b/drivers/gpu/drm/i915/i915_sw_fence.h @@ -46,6 +46,9 @@ void i915_sw_fence_commit(struct i915_sw_fence *fence); int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, struct i915_sw_fence *after, wait_queue_t *wq); +int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence, + struct i915_sw_fence *after, + gfp_t gfp); int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence, struct fence *dma, unsigned long timeout, @@ -62,4 +65,9 @@ static inline bool i915_sw_fence_done(const struct i915_sw_fence *fence) return atomic_read(&fence->pending) < 0; } +static inline void i915_sw_fence_wait(struct i915_sw_fence *fence) +{ + wait_event(fence->wait, i915_sw_fence_done(fence)); +} + #endif /* _I915_SW_FENCE_H_ */