From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 29/30] drm/i915: Track fence setup separately from fenced object lifetime Date: Wed, 13 Apr 2011 22:42:23 +0200 Message-ID: <20110413204222.GK3660@viiv.ffwll.ch> References: <1302640318-23165-1-git-send-email-chris@chris-wilson.co.uk> <1302640318-23165-30-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wy0-f177.google.com (mail-wy0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id A38CD9F36E for ; Wed, 13 Apr 2011 13:42:28 -0700 (PDT) Received: by wyb28 with SMTP id 28so1066582wyb.36 for ; Wed, 13 Apr 2011 13:42:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1302640318-23165-30-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: Andy Whitcroft , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Apr 12, 2011 at 09:31:57PM +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. > > Cc: Andy Whitcroft > Signed-off-by: Chris Wilson > Reviewed-by: Daniel Vetter I think that r-b is stale ;-) Still holds though for the general idea. A few nitpicks below. On general comment: I think we should get completely rid of last_fenced_ring. There should be no way an object can change rings without being at least completely flushed (or even going through the inactive list). Maybe that's for a separate patch but I'm slightly uneasy with the fact that we don't seem to systematically clear last_fenced_ring _anywhere_. > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ca14a86..1949048 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1731,6 +1731,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) > i915_gem_object_move_off_active(obj); > obj->fenced_gpu_access = false; > > + obj->last_fenced_seqno = 0; > + I think we could move that to move_off_active where last_rendering_seqno is being reset. Would be slightly more consistent. Resetting last_fenced_ring together with last_fenced_seqno probably makes sens, too. > @@ -2675,47 +2661,43 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj, > if (reg == NULL) > return -ENOSPC; > > - ret = i915_gem_object_flush_fence(obj, pipelined); > - if (ret) > - return ret; > - > - if (reg->obj) { > - struct drm_i915_gem_object *old = reg->obj; > - > + if ((old = reg->obj)) { Argh. Can you move the assignment out? > @@ -2732,7 +2714,31 @@ update: > ret = i830_write_fence_reg(obj, pipelined, regnum); > break; > } > + if (ret) > + goto err; > + > + if (pipelined) { > + reg->setup_seqno = i915_gem_next_request_seqno(pipelined); > + reg->setup_ring = pipelined; > + if (old) { > + old->last_fenced_ring = pipelined; > + old->last_fenced_seqno = reg->setup_seqno; > + } This looks superfluous. flush_fence should take care of this either directly or via flush_ring -> process_flushing_list -> move_to_active. If it's just paranoia, can this be converted to a WARN_ON? Or is this closing a gap I'm not seeing? -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48