From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 4/4] drm/i915: Review the memory barriers around CPU access to buffers Date: Thu, 11 Oct 2012 22:46:31 +0200 Message-ID: <20121011204631.GD32660@phenom.ffwll.local> References: <6c3329lntgg@orsmga002.jf.intel.com> <1349807080-9005-1-git-send-email-chris@chris-wilson.co.uk> <1349807080-9005-4-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-wi0-f177.google.com (mail-wi0-f177.google.com [209.85.212.177]) by gabe.freedesktop.org (Postfix) with ESMTP id C1C749E9BE for ; Thu, 11 Oct 2012 13:45:32 -0700 (PDT) Received: by mail-wi0-f177.google.com with SMTP id hj13so2299250wib.12 for ; Thu, 11 Oct 2012 13:45:31 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1349807080-9005-4-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: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Oct 09, 2012 at 07:24:40PM +0100, Chris Wilson wrote: > We need to treat the GPU core as a distinct processor and so apply the > same SMP memory barriers. In this case, in addition to flushing the > chipset cache, which is a no-op on LLC platforms, apply a write barrier > beforehand. And then when we invalidate the CPU cache, make sure the > memory is coherent (again this was a no-op on LLC platforms). > > Signed-off-by: Chris Wilson I think this one here deserves some love still: - the fancy new pwrite/pread code does some crazy coherency tricks behind the domain tracking code. This patch misses those. - like Jesse said: comments. - I'd still wish we'd have some i-g-t tests for this stuff ... And now my crazy new theory: We've already had some bug reports that suggested that we're not fully coherent around unbind/rebind and papered over it with: commit c501ae7f332cdaf42e31af30b72b4b66cbbb1604 Author: Chris Wilson Date: Wed Dec 14 13:57:23 2011 +0100 drm/i915: Only clear the GPU domains upon a successful finish And now we have the cpu_reloc regression from Dave Airlie which could be explained with similar rebinding penalties (if we're creative). I hope somewhat that we could explain these with the lack of proper memory barriers ... So if you can gather tested-by's with the above duct-tape reverted and these patches applied, I'd be almost as happy as with some i-g-t tests for these patches. Cheers, Daniel > --- > drivers/char/agp/intel-gtt.c | 1 + > drivers/gpu/drm/i915/i915_gem.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > index 8b0f6d19..1223128 100644 > --- a/drivers/char/agp/intel-gtt.c > +++ b/drivers/char/agp/intel-gtt.c > @@ -1706,6 +1706,7 @@ EXPORT_SYMBOL(intel_gtt_get); > > void intel_gtt_chipset_flush(void) > { > + wmb(); > if (intel_private.driver->chipset_flush) > intel_private.driver->chipset_flush(); > } > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ed8d21a..b1ebb88 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3528,6 +3528,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write) > /* Flush the CPU cache if it's still invalid. */ > if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) { > i915_gem_clflush_object(obj); > + mb(); /* in case the clflush above is optimised away */ > > obj->base.read_domains |= I915_GEM_DOMAIN_CPU; > } > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch