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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
