Re: [PATCH 24/66] drm/i915: Move aliasing_ppgtt

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Jul 01, 2013 at 09:06:20PM +0200, Daniel Vetter wrote:
> On Mon, Jul 1, 2013 at 8:52 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> >> One thing which looks a bit peculiar at the end is that struct
> >> i915_hw_ppgtt is actually used as the real ppgtt object (since it
> >> subclases i915_address space). My original plan was that we'll add a new
> >> struct i915_ppgtt {
> >>       struct i915_address_space base;
> >>       struct i915_hw_ppgtt hw_ppgtt;
> >> }
> >>
> >> To fit into your design the alising ppgtt pointer in dev_priv->gtt would
> >> then only point at a hw_ppgtt struct, not the full deal with address space
> >> and everything else around attached.
> >>
> >> Cheers, Daniel
> >
> > I don't think creating a struct i915_ppgtt is necessary or buys much. We
> > can rename i915_hw_ppgtt to i915_ppgtt, and it accomplishes the same
> > thing. Same for the i915_hw_context for that matter. I wanted to do any
> > sort of renaming after the rest of the series.
> >
> > Can you explain why we'd want to keep the hw_ppgtt, and ppgtt with the
> > track lists etc. distinct?
> 
> The difference is that for the aliasing ppgtt we don't really need a
> address space object to track object offsets since those are identical
> to the ggtt offsets. Hence we only need the hw part of the ppgtt, i.e.
> writing/clearing ptes.

I actually did attempt to make the aliasing ppgtt a real address space
(so the ggtt was the special case instead of vice versa). It proved too
much work, so I dropped it.
> 
> Full ppgtt otoh need the address space of course.
> 
> The above structure layout would allow that. It means that aliasing
> ppgtt will stick out a bit like a sore thumb but imo that's ok since
> it really _is_ a bit an odd thing. I've just noticed that you've
> partially disabled the has_aliasing_ppgtt_mapping logic in your
> branch, so I wonder a bit how aliasing ppgtt works there.

It doesn't. The intent was to completely eclipse aliasing PPGTT. If we
keep the pin interface (as discussed in the other thread), I am not
certain that is still true, though I personally have no issue forcing a
performance penalty on users that wish to use that interface.

> But I guess
> the simplest approach would be to simply keep that stuff around as-is,
> with a few twists to wrestle it into the new world: Batches would be
> executed as if they'd run in the global gtt (i.e. for eviction/binding
> purposes) with the simple difference that we'd check whether the
> global gtt has the aliasing_ppgtt pointer set up, and then write the
> ptes there. Note that unconditionally writing both ppgtt and global
> gtt ptes isn't too good, since in some virtualized enviroments
> tracking those separately is a pretty substantial win apparently.
> -Daniel

I'm still not convinced that having distinct i915_ppgtt and
i915_hw_ppgtt buys us anything.

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center