Re: [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Andy Whitcroft <apw@canonical.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/i915: Track fence setup separately from fenced object lifetime
Date: Sat, 19 Mar 2011 23:35:55 +0100	[thread overview]
Message-ID: <20110319223554.GB16343@viiv.ffwll.ch> (raw)
In-Reply-To: <1300487719-26578-2-git-send-email-chris@chris-wilson.co.uk>

On Fri, Mar 18, 2011 at 10:35:16PM +0000, Chris Wilson wrote:
> This fixes a bookkeeping error causing an OOPS whilst waiting for an
> object to finish using a fence. Now we can simply wait for the fence to
> be written independent of the objects currently inhabiting it (past,
> present and future).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ah, this patch addresses my update label comment ;) Otherwise I think
tracking fences more as independent objects definitely results in clearer
semantics. A tiny nitpick below.

> @@ -2647,9 +2638,13 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
>  			pipelined = NULL;
>  
>  		old->fence_reg = I915_FENCE_REG_NONE;
> -		old->last_fenced_ring = pipelined;
> -		old->last_fenced_seqno =
> -			pipelined ? i915_gem_next_request_seqno(pipelined) : 0;
> +		old->last_fenced_seqno = 0;
> +		if (pipelined) {
> +			old->last_fenced_seqno =
> +				i915_gem_next_request_seqno(pipelined);
> +			i915_gem_object_move_to_active(old, pipelined,
> +						       old->last_fenced_seqno);
> +		}
>  
>  		drm_gem_object_unreference(&old->base);
>  	} else if (obj->last_fenced_seqno == 0)

This special case of the last_fenced_seqno tracking slightly annoys me. I
_think_ the flush_ring in flush_fence does already take care of this (but
I'm not too shure, and this is definitely the safe option).
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48