[Intel-gfx] [RFC 3/5] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Mon, Nov 03, 2014 at 04:34:29PM +0000, Tvrtko Ursulin wrote:
> 
> On 11/03/2014 03:58 PM, Daniel Vetter wrote:
> >On Thu, Oct 30, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>Things like reliable GGTT mappings and mirrored 3d display will need to be
> >>to map the same object twice into the GGTT.
> >>
> >>Add a ggtt_view field per VMA and select the page view based on the type
> >>of the view.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> >
> >lgtm overall, some comments for polish in the crucial parts below.
> >
> >>@@ -2189,3 +2191,21 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> >>
> >>  	return vma;
> >>  }
> >>+
> >>+void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >>+			u32 flags)
> >>+{
> >>+	struct sg_table *pages;
> >>+
> >>+	if (vma->ggtt_view.get_pages)
> >>+		pages = vma->ggtt_view.get_pages(&vma->ggtt_view, vma->obj);
> >>+	else
> >>+		pages = vma->obj->pages;
> >>+
> >>+	if (pages && !IS_ERR(pages)) {
> >>+		vma->bind_vma(vma, pages, cache_level, flags);
> >>+
> >>+		if (vma->ggtt_view.put_pages)
> >>+			vma->ggtt_view.put_pages(&vma->ggtt_view);
> >>+	}
> >
> >tbh this looks a bit like overboarding generalism to me ;-) Imo
> >- drop the ->put_pages callback and just free the sg table if you have
> >   one.
> >- drop teh ->get_pages callbacks and replace it with a new
> >   i915_vma_shuffle_pages or similar you'll call for non-standard ggtt
> >   views. Two reasons: shuffle is a more accurate name than get since this
> >   version doesn't grab it's own page references any more, and with
> >   currently just one internal user for this a vtable is serious overkill.
> 
> I actually thought I will need semi-persistent view for this in the future
> patch which get_pages()/put_pages() caters for.
> 
> More precisely it would be for holding onto created pages during view
> creation across the i915_gem_gtt_prepare_object and i915_vma_bind in
> i915_gem_object_bind_to_vm.
> 
> Also, ->put_pages looks much neater to me from i915_gem_vma_destroy rather
> than leaking the knowledge of internal implementation. That is of course if
> you will allow the above justification for making the alternative view at
> least semi-persistent.

I don't see why the view needs to be semi-persistent. And it certainly
doesn't work by attaching the sg table to the vma, since then the pages
can't survive the vma. And the vma is already cached (i.e. we only ever
throw them away lazily).

And I don't hink caching it longer than the vma is useful since when we
throw away the vma that's usually a much bigger performance disaster,
often involving stalls and chasing down backing storage again. Allocating
a puny sg table and filling it again isn't a lot of work. Especially since
this is only for uncommon cases like scanout buffers or ggtt mmaps thus
far.

> As additional bonus it has the same semantics to GEM get/put_pages.

Well, this isn't the same thing. The _really_ crucial part of
get/put_pages is to grab a reference off the backing storage pages, to
make sure they don't get swapped out.

The comes prepare/finish_gtt, whose job it is to ensure that those pages
are mapped into the VT-d iommu.

Finally there's the job of shuffling the resulting dma_addr_t pages from
prepare_gtt just for the insert_entries/vma_bind functions. You're about
two layers away from get/put_pages and this shuffled sg table here is
really only needed to make insert_entries happy. And not even for the
duration of the entire gtt.
> 
> >>+}
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>index 66bc44b..cbaddda 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>@@ -109,6 +109,23 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
> >>  #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
> >>  #define GEN8_PPAT(i, x)			((uint64_t) (x) << ((i) * 8))
> >>
> >>+enum i915_ggtt_view_type {
> >>+	I915_GGTT_VIEW_NORMAL = 0,
> >>+};
> >>+
> >>+struct i915_ggtt_view {
> >>+	enum i915_ggtt_view_type type;
> >>+	unsigned int vma_id;
> >
> >Imo vma_id and ggtt_view_type are fairly redundant. If you _really_ want
> >to make this super-generic (i.e. for ppgtt) just call it gtt_view instead
> >fo ggtt_view. But I wouldn't bother.
> 
> So you suggest to imply VMA id to be equal to GGTT view type and
> VMA_ID_DEFAULT to be fixed to I915_GGTT_VIEW_NORMAL? Is it not a
> logistics/maintenance problem to sync the two then?

Well if we kill one of them completely there's no syncing required, which
is what I think we want. Having two will be a nightmare, and I don't see
any use a few years out even for non-ggtt special views.

We can always add more complexity later on if needed.

> >Removing vma_id also removes the int, which I'd ask you to replace with an
> >explicit enum anyway ;-)
> 
> Yes I left that for later. I mean, when at stage to be talking about such
> detail it is a happy place.

I'm not a fan of preemptive generalization - it tends to be the wrong one
and hurt doubly in the future since you have to remove the wrong one first
before you can add the right stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch