On Mon, Mar 26, 2012 at 11:34:21AM -0700, Ben Widawsky wrote:
> 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.
Afaik we've cleared this up in our mtg yesterday:
- Full coherent gtt mappings work, they simply moved the bit around (we
need to set bit2 instead of bit1 like on snb/ivb).
- It sounds like all the gpu functions can handle coherent memory, like
on snb/ivb. But because there's no shared cache between the gpu and the
cpu you don't gain anything, but only lose due to the required snoop
traffic.
- Because there's no last level cache it also means that when the gpu does
a write and snoops the cpu cache, it essentially means the cpu
completely drops it's cacheline and has to go back to main memory
(instead of l3 like it does on llc platforms). Gpu reads snoop the cpu
cache corectly.
I hope this clears up the confusion around coherency on vlv. Let Jesse
only needs to check with a real piece of silicon whether that's true ;-)
-Daniel
>
> >
> > 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;
> _______________________________________________
> 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
