[Intel-gfx] [PATCH 3/3] drm/i915: Avoid unbinding due to an interrupted pin_and_fence during execbuffer

[Chris Wilson]

On Fri, 24 Aug 2012 16:46:19 +0200, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Thu, Aug 23, 2012 at 01:12:53PM +0100, Chris Wilson wrote:
> > If we need to stall in order to complete the pin_and_fence operation
> > during execbuffer reservation, there is a high likelihood that the
> > operation will be interrupted by a signal (thanks X!). In order to
> > simplify the cleanup along that error path, the object was
> > unconditionally unbound and the error propagated. However, being
> > interrupted here is far more common than I would like and so we can
> > strive to avoid the extra work by eliminating the forced unbind.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> I've merged the resend of patch 1/3, thanks a lot. But for this one here
> I've found a few tiny things to bitch about. Comments inline below.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   93 ++++++++++++----------------
> >  1 file changed, 38 insertions(+), 55 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 23aa324..0c5a433 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -331,7 +331,8 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
> >  	return ret;
> >  }
> >  
> > -#define  __EXEC_OBJECT_HAS_FENCE (1<<31)
> > +#define  __EXEC_OBJECT_HAS_PIN (1<<31)
> > +#define  __EXEC_OBJECT_HAS_FENCE (1<<30)
> >  
> >  static int
> >  need_reloc_mappable(struct drm_i915_gem_object *obj)
> > @@ -344,6 +345,7 @@ static int
> >  pin_and_fence_object(struct drm_i915_gem_object *obj,
> >  		     struct intel_ring_buffer *ring)
> >  {
> > +	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> >  	struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
> >  	bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4;
> >  	bool need_fence, need_mappable;
> > @@ -359,11 +361,13 @@ pin_and_fence_object(struct drm_i915_gem_object *obj,
> >  	if (ret)
> >  		return ret;
> >  
> > +	entry->flags |= __EXEC_OBJECT_HAS_PIN;
> > +
> >  	if (has_fenced_gpu_access) {
> >  		if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
> >  			ret = i915_gem_object_get_fence(obj);
> >  			if (ret)
> > -				goto err_unpin;
> > +				return ret;
> >  
> >  			if (i915_gem_object_pin_fence(obj))
> >  				entry->flags |= __EXEC_OBJECT_HAS_FENCE;
> > @@ -372,12 +376,35 @@ pin_and_fence_object(struct drm_i915_gem_object *obj,
> >  		}
> >  	}
> >  
> > +	/* ... and ensure ppgtt mapping exist if needed. */
> 
> Nitpick: the "... and" in this move comment looks a bit stale with the
> previously preceeding comment no longer in the same scope ;-)

Hey, I thought you were just adding a bit of drama!

> > -	list_for_each_entry_continue_reverse(obj, objects, exec_list) {
> > +	do {
> > +		unpin_and_fence_object(obj);
> > +	} while (&(obj = list_entry(obj->exec_list.prev, typeof(*obj), exec_list))->exec_list != objects);
> 
> What's the reason here for no longer using the continue_reverse macro? On
> a quick glance the new thing seems to do the same ...

Because this is actually list_for_each_entry_from_reverse(), which doesn't
yet exist. We used to unbind the current obj on error so that we could
walk backwards along the list, but now we need to start the reverse walk
from the error object.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre