[Intel-gfx] [PATCH 02/18] drm/i915: Allow i915_sw_fence_await_sw_fence() to allocate

[Chris Wilson]

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