Re: [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing

[Chris Wilson <chris@chris-wilson.co.uk>]

On Sat, 19 Mar 2011 23:17:56 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 18, 2011 at 08:02:10AM +0000, Chris Wilson wrote:
> > ... even though it was disabled. A mistake in the handling of fence reuse
> > caused us to skip the vital delay of waiting for the object to finish
> > rendering before changing the register.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Nice catch and good simplification of the code-flow. One nitpick about a
> possible further cleanup below.

Hints for further cleanups are always appreciated. :)

> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d4bf061..c5dfb59 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
> >  		reg = &dev_priv->fence_regs[obj->fence_reg];
> >  		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
> >  
> > -		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> > -			pipelined = NULL;
> > +		if (obj->tiling_changed) {
> > +			ret = i915_gem_object_flush_fence(obj, pipelined);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> > +				pipelined = NULL;
> > +
> > +			if (pipelined) {
> > +				reg->setup_seqno =
> > +					i915_gem_next_request_seqno(pipelined);
> > +				obj->last_fenced_seqno = reg->setup_seqno;
> > +				obj->last_fenced_ring = pipelined;
> > +			}
> > +
> > +			goto update;
> 
> I think we could move the update label 3 lines up, which would make the
> above if(pipelined) clause unnecessary. Maybe even drop the goto and
> extract the tail of get_fence as a function of its own.

Right, I did this in a later patch. I want to keep this as a simple
rearrangement of the code since that is the minimal fix and improve upon
that with further patches. Sound good?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre