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 at 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 at 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
