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

[Chris Wilson <chris@chris-wilson.co.uk>]

[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