[Intel-gfx] [PATCH 2/2] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level

[Chris Wilson]

On Wed, Oct 07, 2015 at 04:57:25PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 06/10/15 11:39, Chris Wilson wrote:
> >Since the remove of the pin-ioctl, we only care about not changing the
> >cache level on buffers pinned to the hardware as indicated by
> >obj->pin_display. So we can safely replace i915_gem_object_is_pinned()
> 
> i915_gem_obj_is_pinned

What? That's not the normal prefix, who named that monstrosity!

> 
> >here with a plain obj->pin_display check. During rebinding, we will check
> >sanity checks in case vma->pin_count is erroneously set.
> 
> "do sanity checks" or something.
> 
> >At the same time, we can micro-optimise GTT mmap() behaviour since we
> >only need to relinquish the mmaps before Sandybridge.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index d4a3bdf0c5b6..2b8ed7a2faab 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3629,31 +3629,34 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  {
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct i915_vma *vma, *next;
> >+	bool bound = false;
> >  	int ret = 0;
> >
> >  	if (obj->cache_level == cache_level)
> >  		goto out;
> >
> >-	if (i915_gem_obj_is_pinned(obj)) {
> >-		DRM_DEBUG("can not change the cache level of pinned objects\n");
> >-		return -EBUSY;
> >-	}
> >-
> >  	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> >+		if (!drm_mm_node_allocated(&vma->node))
> >+			continue;
> >+
> >+		if (vma->pin_count) {
> >+			DRM_DEBUG("can not change the cache level of pinned objects\n");
> >+			return -EBUSY;
> >+		}
> >+
> 
> But this is the same as i915_gem_obj_is_pinned, where is the
> obj->pin_display change commit message talks about?

Right here. The difference is that we are only iterating the vma list
once rather than 3x.

> >  		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
> >  			ret = i915_vma_unbind(vma);
> >  			if (ret)
> >  				return ret;
> >-		}
> >+		} else
> >+			bound = true;
> >  	}
> >
> >-	if (i915_gem_obj_bound_any(obj)) {
> >+	if (bound) {
> >  		ret = i915_gem_object_wait_rendering(obj, false);
> >  		if (ret)
> >  			return ret;
> >
> >-		i915_gem_object_finish_gtt(obj);
> >-
> >  		/* Before SandyBridge, you could not use tiling or fence
> >  		 * registers with snooped memory, so relinquish any fences
> >  		 * currently pointing to our region in the aperture.
> >@@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  				return ret;
> >  		}
> >
> >-		list_for_each_entry(vma, &obj->vma_list, vma_link)
> >-			if (drm_mm_node_allocated(&vma->node)) {
> >-				ret = i915_vma_bind(vma, cache_level,
> >-						    PIN_UPDATE);
> >-				if (ret)
> >-					return ret;
> >-			}
> >+		/* Access to snoopable pages through the GTT is incoherent. */
> >+		if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> >+			i915_gem_release_mmap(obj);
> 
> Don't fully understand this one - but my question is this.
> Previously userspace would lose mappings on cache level changes any
> time, after this only on !LLC when turning on caching mode. So this
> means userspace needs to know about this change and modify it's
> behavior? Or what exactly would happen in practice?

No. Userspace has no knowledge of the kernel handling the PTEs, its
mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
Otoh, we are improving the situation so even if userspace tries to avoid
set-cache-level nothing is lost.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre