[PATCH v2] drm/i915: set scanout buffer as uncached on Sandybridge

[PATCH v2] drm/i915: set scanout buffer as uncached on Sandybridge

[Zhenyu Wang]

Display engine on Sandybridge is not coherent with LLC, so
try to always bind display buffer as uncached on Sandybridge.
Old AGP_USER_MEMORY is bound as uncached by default, as Sandybridge
buffer would try to live in LLC with CPU as possible, so it uses
new flag AGP_USER_UNCACHED_MEMORY for uncached binding.

This is based on review and comment from Chris Wilson.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/i915_gem.c      |   20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   10 ++++++++++
 3 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c2c19b..5948e05 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1025,6 +1025,7 @@ void i915_gem_object_unpin(struct drm_gem_object *obj);
 int i915_gem_object_unbind(struct drm_gem_object *obj);
 void i915_gem_release_mmap(struct drm_gem_object *obj);
 void i915_gem_lastclose(struct drm_device *dev);
+int i915_gem_object_enable_scanout(struct drm_gem_object *obj);
 
 /**
  * Returns true if seq1 is later than seq2.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8eb8453..78f6789 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4028,6 +4028,26 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 }
 
 int
+i915_gem_object_enable_scanout(struct drm_gem_object *obj)
+{
+	struct drm_device *dev = obj->dev;
+	struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
+	int type = IS_GEN6(dev) ? AGP_USER_UNCACHED_MEMORY : AGP_USER_MEMORY;
+
+	if (obj_priv->agp_type == type)
+		return 0;
+
+	if (obj_priv->gtt_space) {
+		int ret = i915_gem_object_unbind(obj);
+		if (ret)
+			return ret;
+	}
+
+	obj_priv->agp_type = type;
+	return 0;
+}
+
+int
 i915_gem_object_pin(struct drm_gem_object *obj, uint32_t alignment)
 {
 	struct drm_device *dev = obj->dev;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 990f065..a413db6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1461,6 +1461,10 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 		BUG();
 	}
 
+	ret = i915_gem_object_enable_scanout(obj);
+	if (ret)
+		return ret;
+
 	ret = i915_gem_object_pin(obj, alignment);
 	if (ret)
 		return ret;
@@ -5482,6 +5486,12 @@ intel_user_framebuffer_create(struct drm_device *dev,
 	if (!intel_fb)
 		return ERR_PTR(-ENOMEM);
 
+	ret = i915_gem_object_enable_scanout(obj);
+	if (ret) {
+		kfree(intel_fb);
+		return ERR_PTR(ret);
+	}
+
 	ret = intel_framebuffer_init(dev, intel_fb,
 				     mode_cmd, obj);
 	if (ret) {
-- 
1.7.1

Re: [PATCH v2] drm/i915: set scanout buffer as uncached on Sandybridge

[Chris Wilson]

On Thu, 28 Oct 2010 14:16:09 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> +	ret = i915_gem_object_enable_scanout(obj);
> +	if (ret) {
> +		kfree(intel_fb);
> +		return ERR_PTR(ret);
> +	}

We need to introduce a mutex_lock here. I wonder whether it is truly worth
it. Yes, user space often creates a fb for a new obj so we avoid the
unbind penalty on flipping to this fb. But that first rebind is a one-off
event as well, and we probably should look at how we can simply change
caching bits in the PTE on the fly.

In the short term I'll drop this hunk. I've pushed this to -staging as I'd
like to get another tested-by or reviewed-by and then send it to stable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre