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

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, Apr 13, 2016 at 01:30:39PM +0100, Tvrtko Ursulin wrote:
> As long as it remains hidden in here, otherwise is a bit heavy and
> rude (BUG_ON), or weak as API (if converted to GEM_BUG_ON and return
> pointer undocumented). And if it stays here BUG_ON is redundant.
> Hm.. leaning towards the direct container_of below to bypass all
> these considerations.

Reasonable. I do value the shorthand to_ggtt() though. Ok, the shorthand
can wait until we have more use cases.

> >  static int
> >  i915_get_ggtt_vma_pages(struct i915_vma *vma);
> >
> >@@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> >  		return obj->base.size;
> >  	}
> >  }
> >+
> >+void *i915_vma_iomap(struct i915_vma *vma)
> 
> Kerneldoc! :)

Sorry. Should have double checked with your patch.

> >+{
> >+	if (WARN_ON(!vma->obj->map_and_fenceable))
> >+		return ERR_PTR(-ENODEV);
> >+
> >+	BUG_ON(!vma->is_ggtt);
> >+	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> 
> Maybe aggregate the two into a single WARN_ON and return -EINVAL ?
> Since we easily can sounds better.
> 
> >+
> >+	if (vma->iomap == NULL) {
> >+		struct i915_ggtt *ggtt = to_ggtt(vma->vm);
> >+		void *ptr;
> >+
> >+		ptr = io_mapping_map_wc(ggtt->mappable,
> >+					vma->node.start,
> >+					vma->node.size);
> >+		if (ptr == NULL) {
> >+			int ret;
> >+
> >+			/* Too many areas already allocated? */
> >+			ret = i915_gem_evict_vm(vma->vm, true);
> 
> I don't know much about the iomapping bussiness. Can we exhaust the
> address space similar to vmap and would then interfere with our own
> (or other?) call sites doing plain io_mapping_map* calls?

It's vmap underneath. My thinking here was that if we couldn't allocate
a new ioremap_wc, the obvious candidates were to reap everything inside
the GGTT i.e. inactive ioremap_wc.

But the question is, do we want to reap vma->iomaps upon vmap_purge.
Yes, we probably should - simplest will be to do a similar forced GGTT
eviction from vmap_purge and refine from there.

> >+	vma = i915_gem_obj_to_ggtt(obj);
> >+
> >  	/* setup aperture base/size for vesafb takeover */
> >  	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> >  	info->apertures->ranges[0].size = ggtt->mappable_end;
> >
> >-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> >-	info->fix.smem_len = size;
> >+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> >+	info->fix.smem_len = vma->node.size;
> >
> >-	info->screen_base =
> >-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> >-			   size);
> >-	if (!info->screen_base) {
> >+	info->screen_base = i915_vma_iomap(vma);
> 
> We are slowly establishing that ERR_PTR should not be stored in
> structure members but I suppose here we can allow it since the
> structure is freed if it fails.

I know, I tell everyone else off for doing it like this as well.
Practice what you preach!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx