Re: [PATCH 4/4] drm/i915: Review the memory barriers around CPU access to buffers

[Daniel Vetter <daniel@ffwll.ch>]

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 <chris@chris-wilson.co.uk>

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 <chris@chris-wilson.co.uk>
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