[01/10] drm/i915: Release mmaps on partial ggtt vma unbind
diff mbox

Message ID 1450110229-30450-2-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä Dec. 14, 2015, 4:23 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When a partial ggtt vma gets evicted, we need to zap any CPU
mapping to said vma as well. Currently we zap the mappings
only when the normal gtt vma gets evicted, but for partial
vmas we leave behind stale CPU mappins. And so, if something
else gets bound into the same gtt address range, any
userspace access into the relevant virtual addresses will
go astray.

I didn't find anything really suitable in the mm code to zap
just the needed mappings (we'd need to know the right CPU
side mm and vma etc.), so let's just call i915_gem_release_mmap()
for now.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Testcase: igt/gem_mmap_gtt
Fixes: c5ad54c ("drm/i915: Use partial view in mmap fault handler")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Chris Wilson Dec. 14, 2015, 5:01 p.m. UTC | #1
On Mon, Dec 14, 2015 at 06:23:40PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When a partial ggtt vma gets evicted, we need to zap any CPU
> mapping to said vma as well. Currently we zap the mappings
> only when the normal gtt vma gets evicted, but for partial
> vmas we leave behind stale CPU mappins. And so, if something
> else gets bound into the same gtt address range, any
> userspace access into the relevant virtual addresses will
> go astray.
> 
> I didn't find anything really suitable in the mm code to zap
> just the needed mappings (we'd need to know the right CPU
> side mm and vma etc.), so let's just call i915_gem_release_mmap()
> for now.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Testcase: igt/gem_mmap_gtt
> Fixes: c5ad54c ("drm/i915: Use partial view in mmap fault handler")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I fixed this by tracking vma->map_and_fenceable instead. We still kill
the entire object though. Note this is not enough to completely fix the
test case, so perhaps we wait until we can merge the series to do the
full fix?
-Chris
Ville Syrjälä Dec. 14, 2015, 5:26 p.m. UTC | #2
On Mon, Dec 14, 2015 at 05:01:45PM +0000, Chris Wilson wrote:
> On Mon, Dec 14, 2015 at 06:23:40PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When a partial ggtt vma gets evicted, we need to zap any CPU
> > mapping to said vma as well. Currently we zap the mappings
> > only when the normal gtt vma gets evicted, but for partial
> > vmas we leave behind stale CPU mappins. And so, if something
> > else gets bound into the same gtt address range, any
> > userspace access into the relevant virtual addresses will
> > go astray.
> > 
> > I didn't find anything really suitable in the mm code to zap
> > just the needed mappings (we'd need to know the right CPU
> > side mm and vma etc.), so let's just call i915_gem_release_mmap()
> > for now.
> > 
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Testcase: igt/gem_mmap_gtt
> > Fixes: c5ad54c ("drm/i915: Use partial view in mmap fault handler")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I fixed this by tracking vma->map_and_fenceable instead. We still kill
> the entire object though. Note this is not enough to completely fix the
> test case, so perhaps we wait until we can merge the series to do the
> full fix?

Sure, if you have something better in the pipeline I'm happy to drop
this.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 66b170598ae6..c29b929f796c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3264,6 +3264,9 @@  static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
 		ret = i915_gem_object_put_fence(obj);
 		if (ret)
 			return ret;
+	} else if (i915_is_ggtt(vma->vm) &&
+		   vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL) {
+		i915_gem_release_mmap(obj);
 	}
 
 	trace_i915_vma_unbind(vma);