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

[Chris Wilson]

On Wed, Apr 29, 2015 at 03:50:13PM +0100, Tvrtko Ursulin wrote:
> 
> On 04/27/2015 01:41 PM, 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()
> >here with a plain obj->pin_display check. During rebinding, we will check
> >sanity checks in case vma->pin_count is erroneously set.
> >
> >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 | 33 +++++++++++++++++++++------------
> >  1 file changed, 21 insertions(+), 12 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index f0a6d03e9ba5..afdb604e4005 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3768,31 +3768,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;
> >
> >  	if (obj->cache_level == cache_level)
> >  		return 0;
> >
> >-	if (i915_gem_obj_is_pinned(obj)) {
> >+	if (obj->pin_display) {
> >  		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;
> >+
> 
> Another micro-optimisation?
> 
> Side note - this was puzzling and then I realized
> i915_gem_valid_gtt_space says true when node is not allocated. It
> seems to be only concerned by node colouring - so is that one badly
> name function or I am missing something?

It's only concerned with colouring, yes. I felt like moving the two
checks together but it also acts as a sanity check in i915_vma_insert.
So it's not a micro-optimisation as such, except that I had two
conflicting uses and this was the easiest path to take.
 
> >  		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_finish_gpu(obj);
> >  		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.
> >@@ -3801,15 +3804,21 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  			ret = i915_gem_object_put_fence(obj);
> >  			if (ret)
> >  				return ret;
> >+
> >+			i915_gem_release_mmap(obj);
> >  		}
> 
> If only < gen6 we need to drop the mmap, what happens with the
> domain tracking - who removes the GTT bit from there now?

We don't care. The GTT domain bits remains valid. Access through the GTT
will remain consistent as we change the caching bits - except that on
snooped architectures access may suddenly become verboten and in those
cases we should throwaway the mmap and force the client to fault them
back in (so we can do the sanity checks in i915_gem_fault() and throw
SIGBUS to protect ourself).

To be pendantic we should split this into:


if (gen < 6)
	i915_gem_put_fence();
if (!HAS_LLC && cache_level != I915_CACHE_NONE)
	i915_gem_release_mmap(obj);

> >-		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;
> >-			}
> >+		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >+			if (!drm_mm_node_allocated(&vma->node))
> >+				continue;
> >+
> >+			if (vma->pin_count)
> >+				return -EBUSY;
> 
> Preserve DRM_DEBUG as it was before?

Oh, that can actually die. of be if (WARN_ON()) since the preposition is
that I have already checked for the only vma that can be legally pinned
at this point.

Bah, this entire patch is bogus - because of how set_cache_level is used
by pin_to_display(). Whoops. time to thow it out and start again.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre