Re: [PATCH 00/66] [v1] Full PPGTT minus soft pin

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jul 01, 2013 at 03:36:13PM -0700, Ben Widawsky wrote:
> On Mon, Jul 01, 2013 at 11:39:30PM +0200, Daniel Vetter wrote:
> > Hi Ben
> > 
> > So first things first: I rather like what the code looks like overall at
> > the end. I've done a light read-through (by far not a full review) and
> > besides a few bikesheds (all mentioned by mail already) the big thing is
> > the 1:1 context:ppgtt address space relationship.
> > 
> > We've discussed this at length in private irc and agreed that we need to
> > changes this two a n:1 relationship, so I'll just reiterate the reasons
> > for that on the list:
> > 
> > - Current userspace expects that different contexts created on the same fd
> >   all use the same address space (since there's really only one). So if we
> >   want to no add a new ABI (and for testing I really think we want to
> >   enable ppgtt on current unchanged userspace) we must keep that promise.
> >   Hence we need to be able to point the different contexts created on an
> >   fd all at the same (per-fd) address space.
> > 
> >   If we want we could later on extend this and allow a context to have a
> >   private address space all of its own.
> > 
> > - HW contexts are pretty much the execution/scheduling primitive the hw
> >   provides us with. On current platforms that's a bit a stretch, but it's
> >   much clearer on future platforms.
> > 
> >   The equivalent concept on the cpu would be threads, and history
> >   unanimously established that having multiple threads in the same process
> >   is a useful concept. So I think allowing the same N:1 context:ppgtt
> >   relation on gpus is sound. Of course that does not preclude other ways
> >   to share individual buffers, which we already support with
> >   flink/dma_buf.
> > 
> > With that big issue resolved there's only the bikesheds left. I'm not
> > really worried about those, and in any case we already have some good
> > discussions going on about them.
> 
> I've discussed this with the Mesa team, and I believe this is what they
> want. I'll highlight the important bit for TL;DR people:
> >   Hence we need to be able to point the different contexts created on an
> >   fd all at the same (per-fd) address space.
> 
> If one wants a new address space, they will have to open a new fd.

Yeah, that's the gist of it.

> > So merge plan:
> > 
> > Since the context<->ppgtt relation needs to be revamped I'd like to punt
> > on patches in that area at first and concentrate on merging the
> > address_space/vma conversion and all the prep work leading to that first.
> 
> The main idea [ack, or nak] is the ppgtt becomes *ppgtt, and the context
> will refcount it on context creation/destrution - that way all the
> existing tricky context refcounting should still work, and the last
> context to down ref a ppgtt will destroy it in it's handler.

Yep, that's what I have in mind.

> > We're already discussing some of the details, so I won't repeat that here
> > again. I think for the overall approach I wouldn't bother with rebasing -
> > shuffling around such a massive&invasive patch series is a major pain and
> > rather error-prone.
> > 
> > Instead I'd just freeze the current branch as a known-working reference
> > point and cherry-pick individual subseries out of it. Also some patches
> > will need to be redone, but thanks to the benefit of hindsight I hope that
> > v2 will have much less churn. Again I've tossed around a few ideas in
> > replies to individual patches.
> > 
> > For prep work I see a few pieces:
> > 
> > - drm_mm related prep work, especially drm_mm.c core changes. I think
> >   simply reordering all the relevant patches and resubmitting them (with
> >   cc: dri-devel) is all we need to get those in (minus the oddball last
> >   minute bikeshed).
> > 
> > - Small static inline functions to ease the pain of the conversion. I
> >   think those need to be redone with as much foreshadowing as possible (so
> >   that later patches don't suffer from overblown diff sizes). Maybe we
> >   should also do the lookup-helpers upfront.
> > 
> > - Other prep work like killing obj->gtt_offset and stuff I've missed (but
> >   which doesn't touch the context<->ppgtt relation).
> 
> I think this might be mergeable now. Did you try and have conflicts?

It's mergeable but imo it makes much more sense to add the gtt_space access
helpers first and then embed the drm_mm_node. Same for killing gtt_offset.
Atm your patch ordering for those three things is the wrong way round,
resulting in needless amounts of diff churn. I should have replied to that
patch with this comment, but maybe I've failed ...

> > - I think we can also merge as much of the hw interfacing code as possible
> >   up-front (or in paralle), e.g. converting the pde loading to LRI.
> 
> I was thinking this as well. I wasn't sure how you'd feel about the
> idea. I'd really like that. This should also have no conflicts at
> present (that I'm aware of).

Yeah, if you can prep a subseries of cherry-pick hw interface prep stuff I
can merge it right away. Should also make reviewing stuff a bit easier if
it's split out from the main thing.

> > Then we can slurp all the address_space/vma conversion patches. Imo the
> > important part is to fledge out the commit message with the hindsight
> > insights and explain a bit why certain stuff gets moved and why other
> > stuff should stay where it currently is. We also need to review whether
> > bisecting isn't broken anywhere, but since we don't yet add a real ppgtt I
> > don't expect any issues.
> > 
> > Once that's all in we can revisit the context vs. ppgtt question and
> > figure out how to make it work. I expect that we need to refcount ppgtt
> > address spaces. But if we keep the per-fd contexts around (imo a sane idea
> > anyway) we should get by when just the contexts hold references onto the
> > ppgtt object. Since the context is guaranteed to stay around until the
> > last object is inactive again that should ensure that the ppgtt address
> > space stays around for long enough, too. And it would avoid the ugliness
> > of adding more tricky active refcounting on top of the context/obj
> > refcounting we already have.
> > 
> > Comments?
> 
> The high level plan sounds totally fine to me, I still have some issues
> with how to break up the huge changes in the VMA/VM conversion since
> many interfaces need to change, and that gets messy. In the end I like
> what I did where I split out the 6 or 7 major changes for review. Would
> you be okay with that again? Maybe if all goes well, it won't be a
> problem.

The current vma patches have some scary looking warnings about bisecting,
which need to be addressed. One trick could be to simply embed the vma
object temporarily into the gem_object until everything is moved into the
new place. That way the logic stays the same, we only have the code churn
to deal with (like sprinkling vma arguments instead of obj arguments over
tons of functions). Even the list handling code could be implemented while
the (single) vma is still embedded.

Then the actual behaviour change of a free-standing vma would reduce a lot
(and that change is the tricky one for bisecting I'd guess).

This approach for vmas would mirror your current approach for growing the
address_space struct by piecewise moving stuff from dev_priv->mm to the
(global gtt) address space struct.

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