[PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[PATCH] drm/i915: set scan-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.
This fixed screen artifacts seen by using blit engine on Sandybridge.

Cc: stable team <stable@kernel.org>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3a98bea..5200ee3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -34,6 +34,7 @@
 #include "intel_bios.h"
 #include "intel_ringbuffer.h"
 #include <linux/io-mapping.h>
+#include <linux/intel-gtt.h>
 
 /* General customization:
  */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9792285..46d724f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1579,6 +1579,14 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	obj = intel_fb->obj;
 	obj_priv = to_intel_bo(obj);
 
+	/* 
+	 *  Set uncacheable for scan-out buffer on Sandybridge.
+	 *  Display engine is non-coherent with LLC, and read from
+	 *  main memory only. This ensures data is coherent with display.
+	 */
+	if (IS_GEN6(dev))
+		obj_priv->agp_type = AGP_USER_UNCACHED_MEMORY;
+
 	mutex_lock(&dev->struct_mutex);
 	ret = intel_pin_and_fence_fb_obj(dev, obj);
 	if (ret != 0) {
-- 
1.7.1

Re: [PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[Chris Wilson]

[I haven't seen my first email arrive yet...]

On Mon, 25 Oct 2010 09:42:31 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> Display engine on Sandybridge is not coherent with LLC, so
> try to always bind display buffer as uncached on Sandybridge.
> This fixed screen artifacts seen by using blit engine on Sandybridge.
[snip]

> @@ -1579,6 +1579,14 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  	obj = intel_fb->obj;
>  	obj_priv = to_intel_bo(obj);
>  
> +	/* 
> +	 *  Set uncacheable for scan-out buffer on Sandybridge.
> +	 *  Display engine is non-coherent with LLC, and read from
> +	 *  main memory only. This ensures data is coherent with display.
> +	 */
> +	if (IS_GEN6(dev))
> +		obj_priv->agp_type = AGP_USER_UNCACHED_MEMORY;
> +

[Quick recap of first email]
You can't do this here, changing the AGP type of a potentially bound
object without any locking.

Move this into intel_pin_and_fence_fb_obj(), give it a decent name and
make it robust.

int i915_gem_object_enable_scanout(struct drm_i915_gem_object *obj)
{
	int type = IS_GEN6(dev) ? AGP_USER_UNCACHED_MEMORY : AGP_USER_MEMORY;
/* These names are absolutely terrible. we should have just made
 * AGR_USER_CACHED_MEMORY the default on gen6 and kept the AGP_USER_MEMORY
 * as uncached on all generations, rather than mixing up the cacheability
 * status of common enums.
 */

	if (obj->agp_type == type)
		return 0;

	if (obj->gtt_space) {
		int ret = i915_gem_object_unbind(&obj->base);
		if (ret)
			return ret;
	}

	obj->agp_type = type;
	return 0;
}

[And the additional bit of insight...]

Also add a call from intel_user_framebuffer_create, as being the earliest
opportunity to avoid the unbind penalty. What is the performance impact of
rendering to an uncached buffer on Sandybridge? Should we only be changing
the caching status whilst pinned as the framebuffer? Rather than
unbinding/rebinding, should we introduce a function [once we have a native
GTT interface, Daniel!] to simply update the flags on the PTEs for an
object (obviously with suitable flushing).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[Zou, Nanhai]

Hi Chris,
	We only need front buffer to be uncached.
	I think that will not affect performance since GPU will seldom read back from front buffer. 
	We will measure performance on that.
Thanks
Zou Nan hai

>>-----Original Message-----
>>From: intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org
>>[mailto:intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org] On
>>Behalf Of Chris Wilson
>>Sent: 2010年10月25日 16:23
>>To: Zhenyu Wang; intel-gfx@lists.freedesktop.org
>>Cc: stable team
>>Subject: Re: [Intel-gfx] [PATCH] drm/i915: set scan-buffer as uncached on
>>Sandybridge
>>
>>[I haven't seen my first email arrive yet...]
>>
>>On Mon, 25 Oct 2010 09:42:31 +0800, Zhenyu Wang <zhenyuw@linux.intel.com>
>>wrote:
>>> Display engine on Sandybridge is not coherent with LLC, so
>>> try to always bind display buffer as uncached on Sandybridge.
>>> This fixed screen artifacts seen by using blit engine on Sandybridge.
>>[snip]
>>
>>> @@ -1579,6 +1579,14 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int
>>y,
>>>  	obj = intel_fb->obj;
>>>  	obj_priv = to_intel_bo(obj);
>>>
>>> +	/*
>>> +	 *  Set uncacheable for scan-out buffer on Sandybridge.
>>> +	 *  Display engine is non-coherent with LLC, and read from
>>> +	 *  main memory only. This ensures data is coherent with display.
>>> +	 */
>>> +	if (IS_GEN6(dev))
>>> +		obj_priv->agp_type = AGP_USER_UNCACHED_MEMORY;
>>> +
>>
>>[Quick recap of first email]
>>You can't do this here, changing the AGP type of a potentially bound
>>object without any locking.
>>
>>Move this into intel_pin_and_fence_fb_obj(), give it a decent name and
>>make it robust.
>>
>>int i915_gem_object_enable_scanout(struct drm_i915_gem_object *obj)
>>{
>>	int type = IS_GEN6(dev) ? AGP_USER_UNCACHED_MEMORY : AGP_USER_MEMORY;
>>/* These names are absolutely terrible. we should have just made
>> * AGR_USER_CACHED_MEMORY the default on gen6 and kept the AGP_USER_MEMORY
>> * as uncached on all generations, rather than mixing up the cacheability
>> * status of common enums.
>> */
>>
>>	if (obj->agp_type == type)
>>		return 0;
>>
>>	if (obj->gtt_space) {
>>		int ret = i915_gem_object_unbind(&obj->base);
>>		if (ret)
>>			return ret;
>>	}
>>
>>	obj->agp_type = type;
>>	return 0;
>>}
>>
>>[And the additional bit of insight...]
>>
>>Also add a call from intel_user_framebuffer_create, as being the earliest
>>opportunity to avoid the unbind penalty. What is the performance impact of
>>rendering to an uncached buffer on Sandybridge? Should we only be changing
>>the caching status whilst pinned as the framebuffer? Rather than
>>unbinding/rebinding, should we introduce a function [once we have a native
>>GTT interface, Daniel!] to simply update the flags on the PTEs for an
>>object (obviously with suitable flushing).
>>-Chris
>>
>>--
>>Chris Wilson, Intel Open Source Technology Centre
>>_______________________________________________
>>Intel-gfx mailing list
>>Intel-gfx@lists.freedesktop.org
>>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[Clemens Eisserer]

Hi,

>        We only need front buffer to be uncached.
>        I think that will not affect performance since GPU will seldom read back from front buffer.
>        We will measure performance on that.

At least here I have a couple of apps reading back from frontbuffer,
and performance on i945+2.13 is horrible.
No idea if it would be worse on SandyBridge if access is uncached, but
even slower would *really* hurt.

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

Re: [PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1940 bytes --]

On 2010.10.25 09:22:41 +0100, Chris Wilson wrote:
> You can't do this here, changing the AGP type of a potentially bound
> object without any locking.
> 
> Move this into intel_pin_and_fence_fb_obj(), give it a decent name and
> make it robust.

oh, right, thanks for point this out.

> 
> int i915_gem_object_enable_scanout(struct drm_i915_gem_object *obj)
> {
> 	int type = IS_GEN6(dev) ? AGP_USER_UNCACHED_MEMORY : AGP_USER_MEMORY;
> /* These names are absolutely terrible. we should have just made
>  * AGR_USER_CACHED_MEMORY the default on gen6 and kept the AGP_USER_MEMORY
>  * as uncached on all generations, rather than mixing up the cacheability
>  * status of common enums.
>  */
> 
> 	if (obj->agp_type == type)
> 		return 0;
> 
> 	if (obj->gtt_space) {
> 		int ret = i915_gem_object_unbind(&obj->base);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	obj->agp_type = type;
> 	return 0;
> }
> 
> [And the additional bit of insight...]
> 
> Also add a call from intel_user_framebuffer_create, as being the earliest
> opportunity to avoid the unbind penalty. What is the performance impact of
> rendering to an uncached buffer on Sandybridge? 

I also did another way to fix this by using new GFDT flag for each PTE.
Now every flush method for any ring has the ability to sync pages with
GFDT bit set, so scan-buffer could be set with GFDT and ensure it's got
flush out in time and synced. But we've been told uncached display buffer
setting on sandybridge is the plan to go..

> Should we only be changing
> the caching status whilst pinned as the framebuffer? Rather than
> unbinding/rebinding, should we introduce a function [once we have a native
> GTT interface, Daniel!] to simply update the flags on the PTEs for an
> object (obviously with suitable flushing).
> -Chris
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[Zou, Nanhai]

>>-----Original Message-----
>>From: intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org
>>[mailto:intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org] On
>>Behalf Of Clemens Eisserer
>>Sent: 2010年10月25日 18:31
>>To: intel-gfx
>>Subject: Re: [Intel-gfx] [PATCH] drm/i915: set scan-buffer as uncached on
>>Sandybridge
>>
>>Hi,
>>
>>>        We only need front buffer to be uncached.
>>>        I think that will not affect performance since GPU will seldom read
>>back from front buffer.
>>>        We will measure performance on that.
>>
>>At least here I have a couple of apps reading back from frontbuffer,
>>and performance on i945+2.13 is horrible.
>>No idea if it would be worse on SandyBridge if access is uncached, but
>>even slower would *really* hurt.

Are you reading back from CPU or GPU?

Thanks
Zou Nan hai

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

Re: [PATCH] drm/i915: set scan-buffer as uncached on Sandybridge

[Dave Airlie]

On Mon, Oct 25, 2010 at 11:42 AM, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> Display engine on Sandybridge is not coherent with LLC, so
> try to always bind display buffer as uncached on Sandybridge.
> This fixed screen artifacts seen by using blit engine on Sandybridge.

Also I assume you need to set this back to the other state on unbind,
for pageflipping type apps where we'd reuse the buffers.

Dave.