Re: [PATCH 07/21] 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>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/21] drm/i915: Track fence setup separately from fenced object lifetime
Date: Sat, 16 Apr 2011 15:20:42 +0200	[thread overview]
Message-ID: <20110416132041.GD3498@viiv.ffwll.ch> (raw)
In-Reply-To: <1302945465-32115-8-git-send-email-chris@chris-wilson.co.uk>

On Sat, Apr 16, 2011 at 10:17:31AM +0100, 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).
> 
> A large amount of the change is to delay updating the information about
> the fence on bo until after we successfully write, or queue the write to,
> the register. This avoids the complication of undoing a partial change
> should we fail in pipelining the change.

Reordering the accounting and consistently using setup_seqno/ring to track
fence changes looks correct.

I still see a few minor issues with the pipelined fencing code that should
be addressed in later patches:
- setup_seqno/ring is not being cleanup up anywhere and might go stale.
  The hack I've used in one of my stabs at this was to loop over all
  fences at the end of retire_requests.
- The semantics of last_fenced_seqno/ring are a bit hairy. I think ideal
  would be if the following would always hold:
  * an object may never change it's ring if last_fenced_seqno != 0. A call
    to flush fence is required to change the ring.
  * if last_fenced_seqno != 0, then always last_rendering_seqno != 0 (i.e.
    if the execbuffer fails we still move all objects with pipelined
    fencing setups to the active list).
  These two combined should give:
    last_fenced_ring != NULL implies last_fenced_ring == last_rendering_ring
  allowing us to simplify a few things.
- The obj->fenced_gpu_access optimization got killed in a previous patch.
  We could resurrect that by clearing it in process_flushing_list. With
  that change (and perhaps some movement of the assignement of
  fenced_gpu_access) we could consolidate the last_fenced_seqno/ring
  tracking.

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