Re: [PATCH] drm/i915: Only insert the mb() before updating the fence parameter

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 09, 2012 at 09:38:58AM +0100, Chris Wilson wrote:
> With a fence, we only need to insert a memory barrier around the actual
> fence alteration for CPU accesses through the GTT. Performing the
> barrier in flush-fence was inserting unnecessary and expensive barriers
> for never fenced objects.
> 
> Note removing the barriers from flush-fence, which was effectively a
> barrier before every direct access through the GTT, revealed that we
> where missing a barrier before the first access through the GTT. Lack of
> that barrier was sufficient to cause GPU hangs.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Looks good and finally puts some clear explanation and consistency behind
our mb()s. Two minor nitpicks, otherwise.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)

[snip]

> @@ -3244,6 +3254,9 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
>  
>  	i915_gem_object_flush_cpu_write_domain(obj);
>  
> +	if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0)
> +		mb();
> +

I think a comment here like we have one for all other gtt related memory
barries would be good. Another thing is the flush_gtt_write_domain uses a
wmb, whereas here we don't bother with micro-optimizing things. So I think
it'd be good to just use a mb() for that, too, if just for consistency.

Also, you know the grumpy maintainer drill: Could we exercise these
barriers with a minimal i-g-t testcase, please? Since you've managed to
kill your machine by removing them, they're no longer just there to keep
us happy, hence I'd like to have them exercised ...

Another thing that just crossed my mind: Could we lack a set of mb()s for
cpu access on llc platforms? For non-coherent platforms the mb() in the
clflush paths will do that, but on llc platforms I couldn't find anything.
And that lp bugs seems to make an excellent case for them being required
...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch