[Intel-gfx] [PATCH 40/55] drm/i915: Refactor blocking waits

[Chris Wilson]

On Wed, Jul 27, 2016 at 09:04:03AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> > Tidy up the for loops that handle waiting for read/write vs read-only
> > access.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 158 +++++++++++++++++++---------------------
> >  1 file changed, 75 insertions(+), 83 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 3f6b69dcaccb..2d86a0c3f295 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1339,6 +1339,23 @@ put_rpm:
> >  	return ret;
> >  }
> >  
> > +static void
> > +i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
> > +			       struct drm_i915_gem_request *req)
> > +{
> > +	int idx = req->engine->id;
> > +
> > +	if (i915_gem_active_peek(&obj->last_read[idx],
> > +				 &obj->base.dev->struct_mutex) == req)
> > +		i915_gem_object_retire__read(obj, idx);
> > +	else if (i915_gem_active_peek(&obj->last_write,
> > +				      &obj->base.dev->struct_mutex) == req)
> > +		i915_gem_object_retire__write(obj);
> 
> If these functions will use same mutex (be it different than
> struct_mutex) in all invocations, I'd make an alias for it.
> 
> > +
> > +	if (!i915_reset_in_progress(&req->i915->gpu_error))
> > +		i915_gem_request_retire_upto(req);
> > +}
> > +
> >  /**
> >   * Ensures that all rendering to the object has completed and the object is
> >   * safe to unbind from the GTT or access from the CPU.
> > @@ -1349,39 +1366,34 @@ int
> >  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
> >  			       bool readonly)
> >  {
> > -	struct drm_i915_gem_request *request;
> >  	struct reservation_object *resv;
> > -	int ret, i;
> > +	struct i915_gem_active *active;
> > +	unsigned long active_mask;
> > +	int idx, ret;
> >  
> 
> We could do early exit here based on the active_mask, like with the
> nonblocking version.

Nope. Because here we have to worry about third parties, we can't exit
early.
 
> > -	if (readonly) {
> > -		request = i915_gem_active_peek(&obj->last_write,
> > -					       &obj->base.dev->struct_mutex);
> > -		if (request) {
> > -			ret = i915_wait_request(request);
> > -			if (ret)
> > -				return ret;
> > +	lockdep_assert_held(&obj->base.dev->struct_mutex);
> >  
> > -			i = request->engine->id;
> > -			if (i915_gem_active_peek(&obj->last_read[i],
> > -						 &obj->base.dev->struct_mutex) == request)
> > -				i915_gem_object_retire__read(obj, i);
> > -			else
> > -				i915_gem_object_retire__write(obj);
> > -		}
> > +	if (!readonly) {
> 
> Not sure why not just swap and keep this if (readonly)...

Consistency. The others did !readonly, and this was the odd one out.
> 
> > +		active = obj->last_read;
> > +		active_mask = obj->active;
> >  	} else {
> > -		for (i = 0; i < I915_NUM_ENGINES; i++) {
> > -			request = i915_gem_active_peek(&obj->last_read[i],
> > -						       &obj->base.dev->struct_mutex);
> > -			if (!request)
> > -				continue;
> > +		active_mask = 1;
> 
> Wouldn't we have RENDER_RING define for this and other instances?

?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre