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

[Daniel Vetter <daniel@ffwll.ch>]

While looking at barriers, I think we could be a bit more paranoid with
the barrier in intel_read_status_page and up it to a full mb() and move it
into the !lazy_coherency conditional of the various get_seqno functions.
-Daniel

On Tue, Oct 09, 2012 at 11:54:12AM +0200, Daniel Vetter wrote:
> 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

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