Re: [PATCH 20/26] drm/i915: ValleyView has limited cacheability

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 22 Mar 2012 14:39:02 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> The GT can snoop CPU writes, but doesn't snoop into the CPU cache when
> it does writes, so we can't use the cache bits the same way.
> 
> So map the status and pipe control pages as uncached on ValleyView, and
> only set the pages to cached if we're on a supported platform.
> 
> v2: add clarifying comments and don't use the LLC flag for ioremap vs
>     kmap (Daniel)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   45 ++++++++++++++++++++++++++-----
>  1 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ca3972f..9b26c9d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -319,6 +319,8 @@ init_pipe_control(struct intel_ring_buffer *ring)
>  {
>  	struct pipe_control *pc;
>  	struct drm_i915_gem_object *obj;
> +	int cache_level = HAS_LLC(ring->dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
> +	struct drm_device *dev;
>  	int ret;
>  
>  	if (ring->private)
> @@ -335,14 +337,24 @@ init_pipe_control(struct intel_ring_buffer *ring)
>  		goto err;
>  	}
>  
> -	i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> +	i915_gem_object_set_cache_level(obj, cache_level);
>  
>  	ret = i915_gem_object_pin(obj, 4096, true);
>  	if (ret)
>  		goto err_unref;
> -
> +	dev = obj->base.dev;
>  	pc->gtt_offset = obj->gtt_offset;
> -	pc->cpu_page =  kmap(obj->pages[0]);
> +	/*
> +	 * On ValleyView, only CPU writes followed by GPU reads are snooped,
> +	 * not GPU writes followed by CPU reads.  So we need to map status
> +	 * pages as uncached.
> +	 */
> +	if (IS_VALLEYVIEW(dev))
> +		pc->cpu_page = ioremap(dev->agp->base +
> +				       obj->gtt_offset,
> +				       PAGE_SIZE);
> +	else
> +		pc->cpu_page =  kmap(obj->pages[0]);
>  	if (pc->cpu_page == NULL)
>  		goto err_unpin;
>  
> @@ -364,12 +376,17 @@ cleanup_pipe_control(struct intel_ring_buffer *ring)
>  {
>  	struct pipe_control *pc = ring->private;
>  	struct drm_i915_gem_object *obj;
> +	struct drm_device *dev;
>  
>  	if (!ring->private)
>  		return;
>  
>  	obj = pc->obj;
> -	kunmap(obj->pages[0]);
> +	dev = obj->base.dev;
> +	if (IS_VALLEYVIEW(dev))
> +		iounmap(pc->cpu_page);
> +	else
> +		kunmap(obj->pages[0]);
>  	i915_gem_object_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
>  
> @@ -929,7 +946,10 @@ static void cleanup_status_page(struct intel_ring_buffer *ring)
>  	if (obj == NULL)
>  		return;
>  
> -	kunmap(obj->pages[0]);
> +	if (IS_VALLEYVIEW(dev_priv->dev))
> +		iounmap(ring->status_page.page_addr);
> +	else
> +		kunmap(obj->pages[0]);
>  	i915_gem_object_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
>  	ring->status_page.obj = NULL;
> @@ -942,6 +962,7 @@ static int init_status_page(struct intel_ring_buffer *ring)
>  	struct drm_device *dev = ring->dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
> +	int cache_level = HAS_LLC(ring->dev) ? I915_CACHE_LLC : I915_CACHE_NONE;

So yeah earlier chipsets where this was ok but that don't set .has_llc
will be affected; I'll just change this to IS_VALLEYVIEW.

But on the plus side, if we applied this they probably wouldn't see
"missed irq" messages (if that ever happened on those chipsets). :)

-- 
Jesse Barnes, Intel Open Source Technology Center