From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 2/4] drm/i915: fixup seqno allocation logic for lazy_request Date: Wed, 25 Jan 2012 14:17:46 +0000 Message-ID: References: <1327496640-6735-1-git-send-email-daniel.vetter@ffwll.ch> <1327496640-6735-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id C9738A087F for ; Wed, 25 Jan 2012 06:17:59 -0800 (PST) In-Reply-To: <1327496640-6735-2-git-send-email-daniel.vetter@ffwll.ch> 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: Intel Graphics Development Cc: Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org On Wed, 25 Jan 2012 14:03:58 +0100, Daniel Vetter wrote: > Currently we reserve seqnos only when we emit the request to the ring > (by bumping dev_priv->next_seqno), but start using it much earlier for > ring->oustanding_lazy_request. When 2 threads compete for the gpu and > run on two different rings (e.g. ddx on blitter vs. compositor) > hilarity ensued, especially when we get constantly interrupted while > reserving buffers. > > Breakage seems to have been introduced in > > commit 6f392d548658a17600da7faaf8a5df25ee5f01f6 > Author: Chris Wilson > Date: Sat Aug 7 11:01:22 2010 +0100 > > drm/i915: Use a common seqno for all rings. > > This patch fixes up the seqno reservation logic by moving it into > i915_gem_next_request_seqno. The ring->add_request functions now > superflously still return the new seqno through a pointer, that will > be refactored in the next patch. > > v2: Keep i915_gem_get_seqno (but move it to i915_gem.c) to make it > clear that we only have one seqno counter for all rings. Suggested by > Chris Wilson. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45181 > Tested-by: Nicolas Kalkhof nkalkhof()at()web.de > Signed-Off-by: Daniel Vetter > --- > static int > render_ring_flush(struct intel_ring_buffer *ring, > u32 invalidate_domains, > @@ -467,7 +453,7 @@ gen6_add_request(struct intel_ring_buffer *ring, > mbox1_reg = ring->signal_mbox[0]; > mbox2_reg = ring->signal_mbox[1]; > > - *seqno = i915_gem_get_seqno(ring->dev); > + *seqno = ring->outstanding_lazy_request; In discussing this patch with Daniel, I made the mistake of reading that as i915_gem_get_next_request_seqno() instead of get_seqno(). I'd suggest the patch makes that change and hide the ugly ring->o_l_r. Then since we do i915_gem_get_next_request_seqno() both here and in the caller, it becomes much clearer that we are able to remove it. Daniel, apologies for the confusion! -Chris -- Chris Wilson, Intel Open Source Technology Centre