| Message ID | 1444132698-20280-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 06/10/15 12:58, Chris Wilson wrote: > As paranoia, we want to ensure that the CPU's PTEs have been revoked for > the object before we return from i915_gem_release_mmap(). This allows us > to rely on there being no outstanding memory accesses and guarantees > serialisation of the code against concurrent access just by calling > i915_gem_release_mmap(). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 2b8ed7a2faab..642644f12295 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1877,11 +1877,21 @@ out: > void > i915_gem_release_mmap(struct drm_i915_gem_object *obj) > { > + /* Serialisation between user GTT access and our code depends upon > + * revoking the CPU's PTE whilst the mutex is held. The next user > + * pagefault then has to wait until we release the mutex. > + */ > + lockdep_assert_held(&obj->base.dev->struct_mutex); > + > if (!obj->fault_mappable) > return; > > drm_vma_node_unmap(&obj->base.vma_node, > obj->base.dev->anon_inode->i_mapping); > + > + /* Ensure that the CPU's PTE are revoked before we return */ > + mb(); > + smp_mb() or smp_wmb() would not suffice? Is it needed on uniprocessor? Regards, Tvrtko
On Tue, Oct 06, 2015 at 03:40:02PM +0100, Tvrtko Ursulin wrote: > > > Hi, > > On 06/10/15 12:58, Chris Wilson wrote: > >As paranoia, we want to ensure that the CPU's PTEs have been revoked for > >the object before we return from i915_gem_release_mmap(). This allows us > >to rely on there being no outstanding memory accesses and guarantees > >serialisation of the code against concurrent access just by calling > >i915_gem_release_mmap(). > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index 2b8ed7a2faab..642644f12295 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -1877,11 +1877,21 @@ out: > > void > > i915_gem_release_mmap(struct drm_i915_gem_object *obj) > > { > >+ /* Serialisation between user GTT access and our code depends upon > >+ * revoking the CPU's PTE whilst the mutex is held. The next user > >+ * pagefault then has to wait until we release the mutex. > >+ */ > >+ lockdep_assert_held(&obj->base.dev->struct_mutex); > >+ > > if (!obj->fault_mappable) > > return; > > > > drm_vma_node_unmap(&obj->base.vma_node, > > obj->base.dev->anon_inode->i_mapping); > >+ > >+ /* Ensure that the CPU's PTE are revoked before we return */ > >+ mb(); > >+ > > smp_mb() or smp_wmb() would not suffice? Is it needed on uniprocessor? Correct, smp_mb() would not suffice as we are serialised accessing through a mmio channel with the PTE writes. A wmb() may suffice though, but that actually changed code :) -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 2b8ed7a2faab..642644f12295 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1877,11 +1877,21 @@ out: void i915_gem_release_mmap(struct drm_i915_gem_object *obj) { + /* Serialisation between user GTT access and our code depends upon + * revoking the CPU's PTE whilst the mutex is held. The next user + * pagefault then has to wait until we release the mutex. + */ + lockdep_assert_held(&obj->base.dev->struct_mutex); + if (!obj->fault_mappable) return; drm_vma_node_unmap(&obj->base.vma_node, obj->base.dev->anon_inode->i_mapping); + + /* Ensure that the CPU's PTE are revoked before we return */ + mb(); + obj->fault_mappable = false; } @@ -3168,9 +3178,6 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) return; - /* Wait for any direct GTT access to complete */ - mb(); - old_read_domains = obj->base.read_domains; old_write_domain = obj->base.write_domain;
As paranoia, we want to ensure that the CPU's PTEs have been revoked for the object before we return from i915_gem_release_mmap(). This allows us to rely on there being no outstanding memory accesses and guarantees serialisation of the code against concurrent access just by calling i915_gem_release_mmap(). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)