On Thu, Mar 22, 2012 at 02:39:02PM -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.
I found this commit message to be confusing. Is it simply saying CPU
writes are snooped by the GT, but GT writes are not snooped bv the CPU?
>
> 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.
I'd like to see in the commit message why the pipe control page needs to
be uncached. The only workarounds on the top of my head don't care about
the coherency.
>
> v2: add clarifying comments and don't use the LLC flag for ioremap vs
> kmap (Daniel)
>
> Signed-off-by: Jesse Barnes <jbarnes at 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);
Bikeshed: I think ioremap_nocache is a bit better to use. It's more
self-commenting.
> + 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;
> int ret;
>
> obj = i915_gem_alloc_object(dev, 4096);
> @@ -951,7 +972,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 +980,17 @@ 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]);
> + /*
> + * 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))
> + ring->status_page.page_addr = ioremap(dev->agp->base +
> + obj->gtt_offset,
> + PAGE_SIZE);
Same bikeshed as above.
> + 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;
