| Message ID | 20160914065250.15482-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote: > @@ -678,7 +678,7 @@ void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches) > &request->i915->drm.struct_mutex); > if (prev) > i915_sw_fence_await_sw_fence(&request->submit, &prev->submit, > - &request->submitq); > + &request->submitq, GFP_NOWAIT); Wrt commit message, why do we pass both here? If one was to run statistic analysis, !wq is never true if you pass &foo here. > @@ -135,6 +135,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) > + kfree(wq); This is confusing without a comment or proper flag #define. > INIT_LIST_HEAD(&wq->task_list); > - wq->flags = 0; > + wq->flags = pending; Why not make this look proper by using I915_SW_FENCE_FLAG_FOO name. > +static inline void i915_sw_fence_wait(struct i915_sw_fence *fence) > +{ > + wait_event(fence->wait, i915_sw_fence_done(fence)); > +} > + This seems to be a lost-in-rebasing hunk. Above addressed; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
On Wed, Sep 14, 2016 at 10:51:12AM +0300, Joonas Lahtinen wrote: > On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote: > > @@ -678,7 +678,7 @@ void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches) > > &request->i915->drm.struct_mutex); > > if (prev) > > i915_sw_fence_await_sw_fence(&request->submit, &prev->submit, > > - &request->submitq); > > + &request->submitq, GFP_NOWAIT); > > Wrt commit message, why do we pass both here? If one was to run > statistic analysis, !wq is never true if you pass &foo here. Only because GFP_NOWAIT was descriptive, and cleaner than say (__force gfp_t)0 > > @@ -135,6 +135,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) > > + kfree(wq); > > This is confusing without a comment or proper flag #define. > > > INIT_LIST_HEAD(&wq->task_list); > > - wq->flags = 0; > > + wq->flags = pending; > > Why not make this look proper by using I915_SW_FENCE_FLAG_FOO name. > > > +static inline void i915_sw_fence_wait(struct i915_sw_fence *fence) > > +{ > > + wait_event(fence->wait, i915_sw_fence_done(fence)); > > +} > > + > > This seems to be a lost-in-rebasing hunk. I snuck in a use along the oom path to justify it here (and avoid having to magic it out of nowhere later). -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c index 624d71bf4518..7496661b295e 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.c +++ b/drivers/gpu/drm/i915/i915_gem_request.c @@ -678,7 +678,7 @@ void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches) &request->i915->drm.struct_mutex); if (prev) i915_sw_fence_await_sw_fence(&request->submit, &prev->submit, - &request->submitq); + &request->submitq, GFP_NOWAIT); request->emitted_jiffies = jiffies; request->previous_seqno = engine->last_submitted_seqno; diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index 1e5cbc585ca2..0fe8b446ae84 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -135,6 +135,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) + kfree(wq); return 0; } @@ -194,7 +196,7 @@ static bool i915_sw_fence_check_if_after(struct i915_sw_fence *fence, int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, struct i915_sw_fence *signaler, - wait_queue_t *wq) + wait_queue_t *wq, gfp_t gfp) { unsigned long flags; int pending; @@ -206,8 +208,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 = 1; + } + 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); diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h index 373141602ca4..d221df381ec2 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.h +++ b/drivers/gpu/drm/i915/i915_sw_fence.h @@ -45,7 +45,7 @@ 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); + wait_queue_t *wq, gfp_t gfp); int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence, struct fence *dma, unsigned long timeout, @@ -62,4 +62,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_ */
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 rather than a preallocate struct. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_request.c | 2 +- drivers/gpu/drm/i915/i915_sw_fence.c | 20 ++++++++++++++++++-- drivers/gpu/drm/i915/i915_sw_fence.h | 7 ++++++- 3 files changed, 25 insertions(+), 4 deletions(-)