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

[Daniel Vetter <daniel@ffwll.ch>]

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.

> 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.

> +		}
>  
>  		if (!pipelined) {
>  			if (reg->setup_seqno) {
> @@ -2601,31 +2616,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
>  			ret = i915_gem_object_flush_fence(obj, pipelined);
>  			if (ret)
>  				return ret;
> -		} else if (obj->tiling_changed) {
> -			if (obj->fenced_gpu_access) {
> -				if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
> -					ret = i915_gem_flush_ring(obj->ring,
> -								  0, obj->base.write_domain);
> -					if (ret)
> -						return ret;
> -				}
> -
> -				obj->fenced_gpu_access = false;
> -			}
> -		}
> -
> -		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> -			pipelined = NULL;
> -		BUG_ON(!pipelined && reg->setup_seqno);
> -
> -		if (obj->tiling_changed) {
> -			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;
>  		}
>  
>  		return 0;
> -- 
> 1.7.2.3
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48