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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

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@xxxxxxxxxxxxxxxxxx>
---
  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?

  		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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux