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

[Daniel Vetter]

On Mon, Nov 24, 2014 at 12:32:12PM +0000, Tvrtko Ursulin wrote:
> 
> On 11/12/2014 05:02 PM, Daniel Vetter wrote:
> >On Thu, Nov 06, 2014 at 02:39:25PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>Things like reliable GGTT mmaps and mirrored 2d-on-3d display will need
> >>to map objects into the same address space multiple times.
> >>
> >>This also means that objects now can have multiple VMA entries.
> >>
> >>Added a GGTT view concept and linked it with the VMA to distinguish betwen
> >>multiple instances per address space.
> >>
> >>New objects and GEM functions which do not take this new view as a parameter
> >>assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
> >>previous behaviour.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> >
> >Let's hope I've picked the latest version, not sure. Please either have
> >in-patch changelogs or coverletters with what's new to help confused
> >maintainers ;-)
> 
> I had a cover letter and version when it was a patch series _and_ the
> concept split into multiple VMA and GGTT view. Once only one patch remained
> from the patch series and it was redesigned to kind of merge two concepts
> into one simplified approach I did not think cover letter makes sense any
> more. Will definitely version as this design goes forward.
> 
> >Looks good overal, just a few things around the lifetime rules for sg
> >tables and related stuff that we've discussed offline. And one nit.
> >
> >I think with these addressed this is ready for detailed review and merging
> >(also applies to the -internal series).
> 
> Excellent, thank you! Just one question below:
> 
> >>-int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
> >>+int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj,
> >>+				const struct i915_ggtt_view *view)
> >>  {
> >>-	if (obj->has_dma_mapping)
> >>+	if (obj->has_dma_mapping || view->type != I915_GGTT_VIEW_NORMAL)
> >
> >This check and the one for finish_gtt are wrong. prepare/finish_gtt set up
> >the dma_addr fields in the sg table (if available also the iommu mappings
> >on big core). And that _must_ be done when the first view shows up, no
> >matter which one it is. E.g. userspace might display a rotate buffer right
> >aways without even rendering or writing into it (i.e. leaving it black).
> 
> The plan was to ensure that the "normal" view is always there first.
> Otherwise we can't "steal" DMA addresses and the other views do not have a
> proper SG table to generate DMA addresses from. So the internal code was
> doing that. Am I missing something?
> 
> >The change around finish_gtt is actually a leak since if the last view
> >around is the rotate one (which can happen if userspace drops the buffer
> >but leaves it displayed) we won't clean up (and so leak) the iommu
> >mapping.
> 
> So I somehow need to ensure finish_gtt on a "normal" view is done last,
> correct? Move it from VMA destructon to object destruction? Would this have
> any implications for the shrinker?

Normal or not is irrelevant, all we need is _one_ vma to ensure that the
dma mapping is around. Essentially this amounts to what Chris suggested,
except that we already implicitly track dma_pin_count as

dma_pin_count = 0;
for_each_entry(obj->vma_list, entry)
	dma_pin_count++;

The difference is that we only care about 0->1 and 1->0 transitions, so a
list_empty check is all we really need. And since it doesn't matter
whether we check the head node or a member of the list to check emptiness
we can just look at

list_empty(&vma->vma_list);

in the vma_bind/unbind code to decide whether we should set up or destroy
the dma mapping.

Again the type of view the vma represents (normal ggtt, ppgtt, rotate or
anything else really) is completely irrelevant. I hope that explains the
confusion here a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch