Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

Chris Wilson
2016-04-18T15:22:48Z

On Mon, Apr 18, 2016 at 04:08:55PM +0100, Tvrtko Ursulin wrote:
> On 18/04/16 15:54, Chris Wilson wrote:
> >@@ -3378,6 +3378,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
> > ret = i915_gem_object_put_fence(obj);
> > if (ret)
> > return ret;
> >+
> >+ i915_vma_iounmap(vma);
> > }
> >
> > trace_i915_vma_unbind(vma);
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >index b3af2e808b49..e185dff358e1 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> > return obj->base.size;
> > }
> > }
> >+
> >+void *i915_vma_pin_iomap(struct i915_vma *vma)
> >+{
> >+ void *ptr;
>
> Just realized we should mark the pointers with __iomem here.

The problem is that we need to lose the annotation at some point as we
mix access via ring->virtual_start. For correctness, we should report
__iomem here and discard it later.

> >+/**
> >+ * i915_vma_iounmap - unmaps the VMA
> >+ * @vma: VMA to unmap
> >+ *
> >+ * Unmaps the previously iomapped VMA using iounmap.
> >+ *
> >+ * Users of i915_vma_pin_iomap() should not manually unmap by calling this
> >+ * function but should call i915_vma_unpin_iomap() instead.
> >+ *
> >+ * Callers must hold the struct_mutex.
> >+ */
> >+static inline void i915_vma_iounmap(struct i915_vma *vma)
> >+{
> >+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
> >+ GEM_BUG_ON(vma->pin_count);
> >+
> >+ if (vma->iomap == NULL)
> >+ return;
> >+
> >+ io_mapping_unmap(vma->iomap);
> >+ vma->iomap = NULL;
> >+}
>
> It would be best to hide this near the unbind code. Alternatively
> maybe put a louder warning in the kerneldoc? Just so it is a bit
> more distinguishable from the GEM exported API for this feature.

I had the same thought, then decided to stick with the
i915_vma_iounmap() call in i915_vma_unbind(). Maybe __i915_vma_iounmap?
-Chris

--
Chris Wilson, Intel Open Source Technology Centre