[Intel-gfx] [PATCH 17/25] drm/i915: ValleyView cacheability is different

[Daniel Vetter]

On Wed, Mar 21, 2012 at 12:48:38PM -0700, Jesse Barnes 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.
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |    2 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   35 ++++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e4fa294..a636703 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -255,6 +255,7 @@ static const struct intel_device_info intel_valleyview_m_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.is_valleyview = 1,
> +	.has_llc = 0,
>  };
>  
>  static const struct intel_device_info intel_valleyview_d_info = {
> @@ -264,6 +265,7 @@ static const struct intel_device_info intel_valleyview_d_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.is_valleyview = 1,
> +	.has_llc = 0,

Usually we don't set feature bits to 0, please drop these 2 hunks.

>  };
>  
>  static const struct pci_device_id pciidlist[] = {		/* aka */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ca3972f..f52abc4 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;

Nope, that's not gonna work. Afaik we have 3 kinds of snoopable bus access
from the gpu:
- llc, i.e. snoopable access for all render operations, support if
  intel_info->has_llc == 1
- snoopable access to untiled mem with the blitter, supported on all
  generations (down to mighty old i81x)
- snoopable access to the hw status page

Please clear up the confusion here. Below you also use the IS_VLV macro,
that seems more appropriate. Also I'm wondering whether this is supposed
to be fixed in future silicon revisions, if so please mark this as a w/a
that can be reaped as soon as we don't use this early silicon for testing
any more.
-Daniel


>  	int ret;
>  
>  	if (ring->private)
> @@ -335,14 +337,19 @@ 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]);
> +	if (!HAS_LLC(ring->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 +371,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 +941,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 +957,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;
>  	int ret;
>  
>  	obj = i915_gem_alloc_object(dev, 4096);
> @@ -951,7 +967,7 @@ static int init_status_page(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 != 0) {
> @@ -959,7 +975,12 @@ static int init_status_page(struct intel_ring_buffer *ring)
>  	}
>  
>  	ring->status_page.gfx_addr = obj->gtt_offset;
> -	ring->status_page.page_addr = kmap(obj->pages[0]);
> +	if (!HAS_LLC(ring->dev))
> +		ring->status_page.page_addr = ioremap(dev->agp->base +
> +						      obj->gtt_offset,
> +						      PAGE_SIZE);
> +	else
> +		ring->status_page.page_addr = kmap(obj->pages[0]);
>  	if (ring->status_page.page_addr == NULL) {
>  		memset(&dev_priv->hws_map, 0, sizeof(dev_priv->hws_map));
>  		goto err_unpin;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48