From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 3/4] drm/i915: Insert a full mb() before reading the seqno from the status page Date: Sat, 19 Jan 2013 12:02:20 +0000 Message-ID: <84c8a8$7dvdg8@orsmga001.jf.intel.com> References: <6c3329lntgg@orsmga002.jf.intel.com> <1349807080-9005-1-git-send-email-chris@chris-wilson.co.uk> <1349807080-9005-3-git-send-email-chris@chris-wilson.co.uk> <20121011124600.08005869@jbarnes-desktop> <20121019135249.7037902c@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 45DE0E5C21 for ; Sat, 19 Jan 2013 04:02:25 -0800 (PST) In-Reply-To: <20121019135249.7037902c@jbarnes-desktop> 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: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 19 Oct 2012 13:52:49 -0700, Jesse Barnes wrote: > On Fri, 19 Oct 2012 21:40:17 +0100 > Chris Wilson wrote: > > > On Thu, 11 Oct 2012 12:46:00 -0700, Jesse Barnes wrote: > > > On Tue, 9 Oct 2012 19:24:39 +0100 > > > Chris Wilson wrote: > > > > > > > Hopefully this will reduce a few of the missed IRQ warnings. > > > > > > > > Signed-off-by: Chris Wilson > > > > --- > > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 8 +++++++- > > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 -- > > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > index e069e69..133beb6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > @@ -704,14 +704,18 @@ gen6_ring_get_seqno(struct intel_ring_buffer *ring, bool lazy_coherency) > > > > /* Workaround to force correct ordering between irq and seqno writes on > > > > * ivb (and maybe also on snb) by reading from a CS register (like > > > > * ACTHD) before reading the status page. */ > > > > - if (!lazy_coherency) > > > > + if (!lazy_coherency) { > > > > intel_ring_get_active_head(ring); > > > > + mb(); > > > > + } > > > > return intel_read_status_page(ring, I915_GEM_HWS_INDEX); > > > > } > > > > > > > > static u32 > > > > ring_get_seqno(struct intel_ring_buffer *ring, bool lazy_coherency) > > > > { > > > > + if (!lazy_coherency) > > > > + mb(); > > > > return intel_read_status_page(ring, I915_GEM_HWS_INDEX); > > > > } > > > > > > > > @@ -719,6 +723,8 @@ static u32 > > > > pc_render_get_seqno(struct intel_ring_buffer *ring, bool lazy_coherency) > > > > { > > > > struct pipe_control *pc = ring->private; > > > > + if (!lazy_coherency) > > > > + mb(); > > > > return pc->cpu_page[0]; > > > > } > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > index 2ea7a31..40b252e 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > > > @@ -160,8 +160,6 @@ static inline u32 > > > > intel_read_status_page(struct intel_ring_buffer *ring, > > > > int reg) > > > > { > > > > - /* Ensure that the compiler doesn't optimize away the load. */ > > > > - barrier(); > > > > return ring->status_page.page_addr[reg]; > > > > } > > > > > > > > > > This looks a bit more voodoo-y. Theoretically an mb() on the CPU side > > > should have nothing to do with what the GPU just wrote to the status > > > page. It'll slow down the read a bit but shouldn't affect coherence at > > > all... An MMIO read from the GPU otoh should flush any stubborn DMA > > > buffers. > > > > Absolutely convinced? Aren't we here more worried about the view of the > > shared cache from any particular core and so need to treat this as an > > SMP programming problem, in which case we do need to worry about memory > > barriers around dependent reads and writes between processors? > > > > But it is definitely more voodoo... > > If it's an SMP issue, barriers won't help, we need actual > synchronization in the form of locks or something. Glad you agree. How are locks implemented? :) Irrespectively of the contentious patches Daniel thought would be a good idea, we need the first 2 to fix the mb() around fences. Poke. -Chris -- Chris Wilson, Intel Open Source Technology Centre