On Mon, May 25, 2015 at 06:48:44PM +0100, Chris Wilson wrote: > Since the advent of mmap(wc), where we reused the same cache domain for > WC and GTT paths (oh, how I regret that double-edged advice), we need to > be extra cautious when using GTT iomap_wc internally. Since userspace maybe > modifying through the mmap(wc) and we then modify modifying through an > aliased WC path through the GTT, those writes may overlap and not be > visible to the other path. Easiest to trigger appears to be write the > batch through mmap(wc) and then attempt to perform reloc the GTT, > corruption quickly ensues. > > v2: Be stricter and do a full mb() in case we are reading through one > path and about to write through the second path. Also, apply the > barriers on transitioning into the GTT domain as well to cover the white > lie asychronous cases. > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Akash Goel <akash.goel@xxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx There are a lot more mb() and wmb() in the gem code. I count execlist, legacy rings, pwrite, set_domain and finish_gtt. Time to add a flags parameter to the set_domain ioctl so that we can untangle the domain tracking from the waiting? Add mb() all over the place in userspace around unsychronized access? Something else? -Daniel > --- > drivers/gpu/drm/i915/i915_gem.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 517c5b8100d1..f17168b2cd37 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3923,7 +3923,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) > int ret; > > if (obj->base.write_domain == I915_GEM_DOMAIN_GTT) > - return 0; > + goto out; > > ret = i915_gem_object_wait_rendering(obj, !write); > if (ret) > @@ -3943,13 +3943,6 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) > > i915_gem_object_flush_cpu_write_domain(obj); > > - /* Serialise direct access to this object with the barriers for > - * coherent writes from the GPU, by effectively invalidating the > - * GTT domain upon first access. > - */ > - if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) > - mb(); > - > old_write_domain = obj->base.write_domain; > old_read_domains = obj->base.read_domains; > > @@ -3977,6 +3970,23 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) > list_move_tail(&vma->mm_list, > &to_i915(obj->base.dev)->gtt.base.inactive_list); > > +out: > + /* > + * Userspace may be writing through mmap(wc) with > + * write_domain=GTT (or even with a white lie unsynchronized write), > + * so we need to explicitly flush before transitioning to writing > + * through the GTT, or vice versa. As we reused the GTT cache domain > + * for both paths, we must always have a memory barrier just in case. > + * > + * We need a full memory barrier in case we are reading through > + * the GTT and before we starting through the WC (or vice versa). > + * If we are only about to read through this access, we need only > + * wait for any pending writes on the other path. > + */ > + if (write) > + mb(); > + else > + wmb(); > return 0; > } > > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
