Re: [PATCH] drm/i915: Use Write-Through cacheing for the display plane on Iris

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 30, 2013 at 08:25:56PM +0100, Chris Wilson wrote:
> Haswell GT3e has the unique feature of supporting Write-Through cacheing
> of objects within the eLLC/LLC. The purpose of this is to enable the display
> plane to remain coherent whilst objects lie resident in the eLLC/LLC - so
> that we, in theory, get the best of both worlds, perfect display and fast
> access.
> 
> However, we still need to be careful as the CPU does not see the WT when
> accessing the cache. In particular, this means that we need to flush the
> cache lines after writing to an object through the CPU, and on
> transitioning from a cached state to WT.
> 
> v2: Actually do the clflush on transition to WT, nagging by Ville.
> v3: Flush the CPU cache after writes into WT objects.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  drivers/gpu/drm/i915/i915_dma.c     |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h     |  4 +++-
>  drivers/gpu/drm/i915/i915_gem.c     | 38 ++++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++++++++-
>  include/uapi/drm/i915_drm.h         |  1 +
>  5 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 8da0b3d..75989fc 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -976,6 +976,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  	case I915_PARAM_HAS_LLC:
>  		value = HAS_LLC(dev);
>  		break;
> +	case I915_PARAM_HAS_WT:
> +		value = HAS_WT(dev);
> +		break;
>  	case I915_PARAM_HAS_ALIASING_PPGTT:
>  		value = dev_priv->mm.aliasing_ppgtt ? 1 : 0;
>  		break;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 34d2b9d..d27a82a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -452,6 +452,7 @@ enum i915_cache_level {
>  	I915_CACHE_NONE = 0,
>  	I915_CACHE_LLC,
>  	I915_CACHE_LLC_MLC, /* gen6+, in docs at least! */
> +	I915_CACHE_WT,
>  };
>  
>  typedef uint32_t gen6_gtt_pte_t;
> @@ -1344,7 +1345,7 @@ struct drm_i915_gem_object {
>  	unsigned int pending_fenced_gpu_access:1;
>  	unsigned int fenced_gpu_access:1;
>  
> -	unsigned int cache_level:2;
> +	unsigned int cache_level:3;
>  
>  	unsigned int has_aliasing_ppgtt_mapping:1;
>  	unsigned int has_global_gtt_mapping:1;
> @@ -1547,6 +1548,7 @@ struct drm_i915_file_private {
>  #define HAS_BLT(dev)            (INTEL_INFO(dev)->has_blt_ring)
>  #define HAS_VEBOX(dev)          (INTEL_INFO(dev)->has_vebox_ring)
>  #define HAS_LLC(dev)            (INTEL_INFO(dev)->has_llc)
> +#define HAS_WT(dev)            (IS_HASWELL(dev) && ((struct drm_i915_private *)(dev)->dev_private)->ellc_size)
>  #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
>  
>  #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 5)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 99362f7..1c5bcc7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3286,11 +3286,13 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj)
>  	 * snooping behaviour occurs naturally as the result of our domain
>  	 * tracking.
>  	 */
> -	if (obj->cache_level != I915_CACHE_NONE)
> +	switch (obj->cache_level) {
> +	case I915_CACHE_LLC:
> +	case I915_CACHE_LLC_MLC:
>  		return;
> +	}
>  
>  	trace_i915_gem_object_clflush(obj);
> -
>  	drm_clflush_sg(obj->pages);
>  }
>  
> @@ -3447,27 +3449,27 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		i915_gem_obj_ggtt_set_color(obj, cache_level);
>  	}
>  
> -	if (cache_level == I915_CACHE_NONE) {
> -		u32 old_read_domains, old_write_domain;
> -
> +	if (cache_level == I915_CACHE_NONE ||
> +	    cache_level == I915_CACHE_WT) {
>  		/* If we're coming from LLC cached, then we haven't
>  		 * actually been tracking whether the data is in the
>  		 * CPU cache or not, since we only allow one bit set
>  		 * in obj->write_domain and have been skipping the clflushes.
> -		 * Just set it to the CPU cache for now.
> +		 * Do them now.
>  		 */
> -		WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> -		WARN_ON(obj->base.read_domains & ~I915_GEM_DOMAIN_CPU);
> +		if (obj->pages && !obj->stolen) {
> +			u32 old_write_domain;
>  
> -		old_read_domains = obj->base.read_domains;
> -		old_write_domain = obj->base.write_domain;
> +			trace_i915_gem_object_clflush(obj);
> +			drm_clflush_sg(obj->pages);
>  
> -		obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> -		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> +			old_write_domain = obj->base.write_domain;
> +			obj->base.write_domain &= ~I915_GEM_DOMAIN_CPU;
>  
> -		trace_i915_gem_object_change_domain(obj,
> -						    old_read_domains,
> -						    old_write_domain);
> +			trace_i915_gem_object_change_domain(obj,
> +							    obj->base.read_domains,
> +							    old_write_domain);
> +		}

With the change to i915_gem_clflush_object() this hunk could be dropped,
no?

>  	}
>  
>  	obj->cache_level = cache_level;
> @@ -3565,7 +3567,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	 * of uncaching, which would allow us to flush all the LLC-cached data
>  	 * with that bit in the PTE to main memory with just one PIPE_CONTROL.
>  	 */
> -	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
> +	ret = i915_gem_object_set_cache_level(obj,
> +					      HAS_WT(obj->base.dev) ? I915_CACHE_WT : I915_CACHE_NONE);
>  	if (ret)
>  		return ret;
>  
> @@ -3638,7 +3641,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
>  
>  	/* Flush the CPU cache if it's still invalid. */
>  	if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
> -		i915_gem_clflush_object(obj);
> +		if (obj->cache_level == I915_CACHE_NONE)
> +			i915_gem_clflush_object(obj);

And what about pwrite?

I must admit that I'm getting confused by our caches again.

Based on what I've read I *think* UC GPU accesses will snoop/invalidate
the LLC and IA caches, so this kind of flush shouldn't be necessary (on
LLC platforms that is). Supposedly the same goes for the pre-copy flushes
in pread/pwrite. And the post-copy flush in pwrite is needed due to the
display engine only.

Should we maybe add a special UC cache level for display to avoid pointless
flushes on other UC buffers LLC platforms? That way userland could set most
buffers to UC via the ioctl and use MOCS to override them to LLC on IVB.

Also while looking through BSpec I noticed a slightly worrying note.
Apparently, on HSW at least, L3/not-LLC cacheable surfaces can
still evict dirty lines from L3 to LLC. The IVB flow diagrams leave me to
think IVB could behave the same way, even though it's not really spelled
out there. This would only be an issue when using MOCS since you can't
configure such a caching mode through the PTEs alone.

>  
>  		obj->base.read_domains |= I915_GEM_DOMAIN_CPU;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0522d00..072a348 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -54,6 +54,7 @@
>  					 (((bits) & 0x8) << (11 - 3)))
>  #define HSW_WB_LLC_AGE0			HSW_CACHEABILITY_CONTROL(0x3)
>  #define HSW_WB_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0xb)
> +#define HSW_WT_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0x6)
>  
>  static gen6_gtt_pte_t gen6_pte_encode(dma_addr_t addr,
>  				      enum i915_cache_level level)
> @@ -116,8 +117,16 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
>  	gen6_gtt_pte_t pte = GEN6_PTE_VALID;
>  	pte |= HSW_PTE_ADDR_ENCODE(addr);
>  
> -	if (level != I915_CACHE_NONE)
> +	switch (level) {
> +	case I915_CACHE_NONE:
> +		break;
> +	case I915_CACHE_WT:
> +		pte |= HSW_WT_ELLC_LLC_AGE0;
> +		break;
> +	default:
>  		pte |= HSW_WB_ELLC_LLC_AGE0;
> +		break;
> +	}
>  
>  	return pte;
>  }
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index e47cf00..e831292 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -338,6 +338,7 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_HAS_PINNED_BATCHES	 24
>  #define I915_PARAM_HAS_EXEC_NO_RELOC	 25
>  #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
> +#define I915_PARAM_HAS_WT     	 	 27
>  
>  typedef struct drm_i915_getparam {
>  	int param;
> -- 
> 1.8.3.2

-- 
Ville Syrjälä
Intel OTC