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
