[Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl

[Daniel Vetter]

On Mon, Apr 8, 2013 at 11:48 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Mon, Apr 08, 2013 at 09:24:58PM +0200, Daniel Vetter wrote:
>> On Mon, Apr 8, 2013 at 7:40 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > On Mon, Apr 08, 2013 at 07:18:11PM +0200, Daniel Vetter wrote:
>> >> On Tue, Feb 12, 2013 at 02:17:22PM +0000, Chris Wilson wrote:
>> >> > By exporting the ability to map user address and inserting PTEs
>> >> > representing their backing pages into the GTT, we can exploit UMA in order
>> >> > to utilize normal application data as a texture source or even as a
>> >> > render target (depending upon the capabilities of the chipset). This has
>> >> > a number of uses, with zero-copy downloads to the GPU and efficient
>> >> > readback making the intermixed streaming of CPU and GPU operations
>> >> > fairly efficient. This ability has many widespread implications from
>> >> > faster rendering of client-side software rasterisers (chromium),
>> >> > mitigation of stalls due to read back (firefox) and to faster pipelining
>> >> > of texture data (such as pixel buffer objects in GL or data blobs in CL).
>> >> >
>> >> > v2: Compile with CONFIG_MMU_NOTIFIER
>> >> > v3: We can sleep while performing invalidate-range, which we can utilise
>> >> > to drop our page references prior to the kernel manipulating the vma
>> >> > (for either discard or cloning) and so protect normal users.
>> >> > v4: Only run the invalidate notifier if the range intercepts the bo.
>> >> > v5: Prevent userspace from attempting to GTT mmap non-page aligned buffers
>> >> >
>> >> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> >>
>> >> Quick bikeshed:
>> >> - Still not really in favour of the in-page gtt offset handling ... I
>> >>   still think that this is just a fancy mmap api, and it better reject
>> >>   attempts to not map anything aligned to a full page outright.
>> >
>> > Strongly disagree.
>>
>> Ok, let's dig out the beaten arguments here ;-)
>> - Imo the gtt_offset frobbery is a bit fragile, and getting this right
>> in the face of ppgtt won't make it better. And yes the only reason we
>> still have that field is that you've shot down any patch to remove it
>> citing userptr here. So "it's here already" doesn't count ;-)
>> - Userptr for i915 is an mmap interface, and that works on pages,
>> lying to userspace isn't great.
>
> No. Due to the nature of existing decades old userspace, I need to map
> byte ranges, so I do not view this as a simple equivalence to mmapping
> a bo.

See below, I need to roll this up from behind ...

>> - I don't see why userspace can't do this themselves. I've seen that
>> it makes things easier in SNA/X, but for a general purpose interface
>> that argument doesn't cut too much.
>
> I have completely opposite viewpoint: A general purpose interface is not
> a page interface, and that this interface trades a small amount of
> kernel complexity (tracking the offset_in_page) so that userspace has a
> flexible interface that matches its requirements.

mmap is the general-purpose map something into cpu address space
thingy, and it works on pages, too. So still don't buy this, and Eric
seems to agree.

But anyway, if you're convinced I'm grumpily ok with those semantics.

>> - I'm also a bit afraid that our code implicitly assumes that
>> size/offset are always page-aligned and I kinda want to avoid that we
>> have to audit for such issues from here on. We've blown up in the past
>> assuming that size > 0 already, I think we're set to blow up on this
>> one here.
>
> Now that we can distinguish between size and num_pages, there is no
> longer a need for size to be page aligned (and is currently redundant).
>
>> In any case, if you really want to stick to this I want this to be
>> explictly track in an obj->reloc_gtt_offset_adjustment or something
>
> Sure, let's call it obj->gtt_offset:12;

As long as it's something with the high 20 bits zero I'm ok. Since
with ppgtt we'll soon have tons of different ->gtt_offsets, so with
your current approach we either need to keep this offset at different
places (and I'd really really dislike to do that, see all the stuff
I've been fighting in modeset land). So I want this separate and
explicit.

>> which is very loudly yelling at people to make sure no one trips over
>> it. Tracking the adjustment in a separate field, which would only ever
>> be used in the reloc code would address all my concerns (safe for the
>> api ugliness one).
>
> And everywhere that deals in GTT addresses.

I've ignored the other cases since I don't see a use-case, but that's
a point to be address more below. So looking at other places we use
gtt address I see two major pieces
- pwrite/pread: Since we also need to deal in struct pages for cpu
access it's easiest to just add the static offset at the beginning.
Again, much clearer when this offset is explicit and stored someplace
separate.
- gtt mmaps: I have no idea how you plan to coax the mmap syscal into
cooperation here.

So addressing your point above that SNA has to deal with 25+ years of
userspace legacy: I really want this to be explicitly done in the
kernel in all the places we need it. At which point I honestly don't
understand why that special offset can't be moved into the kgem
abstraction. For normal objects it would be 0, for un-aligned userptr
objects it would be non-0. In all cases kgem would add that offset
when emitting a relocation.

Now if the argument is that this adds measurable overhead to
relocation emitting, I want this overhead to be measured to justify
adding this to the kernel.

>> >> - I915_USERPTR_UNSYNCHRONIZED ... eeek. That means that despite everyone
>> >>   having mmu notifiers enabled in their distro config, you make sure sna
>> >>   doesn't hit it. Imo not enough testing coverage ;-) Or this there
>> >>   another reason behind this than "mmu notifiers are too slow"?
>> >>
>> >>   Generally I'm a bit sloppy with going root-only for legacy X stuff (like
>> >>   scanline waits), but this here looks very much generally useful. So not
>> >>   exemption-material imo.
>> >
>> > Strongly disagree. Most of my machines do not have mmu-notifiers and
>> > would still like to benefit from userptr and I see no reason why we need
>> > to force mmu-notifiers.
>>
>> Note that I didn't shout against the mmu_notifier-less support
>> (although I'm honestly not too happy about it), what I don't like is
>> the override bit disabling the mmu_notifiers even if we have them.
>> Since that will mean that the code won't be tested through SNA, and so
>> has a good chance of being buggy. Once mesa comes around and uses it,
>> it'll nicely blow up. And one of the reason Jesse is breathing down my
>> neck to merge this is "other guys are interested in this at intel".
>
> I don't see how you can reconcile that viewpoint. In order for userspace
> to be able to use userptr without mmu-notifiers it has to be very
> careful about managing synchronisation of such pointers across vma
> events. So userptr should only be allowed to opt-in to such a precarious
> arrangement. And because it was so easy for you to shoot yourself and
> the entire system in the head by getting such synchronisation wrong, the
> suggestion was to only allow root access to the shotgun. Once it is opting
> in, behaviour of userptr should then be consistent across all
> configurations i.e. it should always avoid the mmu-notifier if userspace
> asks.

So first one thing to clear out: I think we don't need to care about
semantics around unmap, fork and friends - the kernel will hold
references to the backing storage pages in both cases. So I think both
cases are safe from userspace trying to trick the kernel.

Second thing to get out of the way: After all this discussion I think
it's ok to call fork&friends (though not unmap) as simply undefined
behaviour - the kernel might pick the old or the new address space
however it sees fit. Again, not something we need to care about. Now a
big exception would be if the behaviour around doing an early unmap
after a batchbuffer would differe between mmu-notifier based userptr
and pinning userptr. If that's indeed the case I'm seriously wondering
whether it's a good idea to merge both implementations under the same
interface. In that case I'd _much_ prefer we'd just stick with mmu
notifiers and have the same semantics everwhere.

So now the real deal: My argument for originally not merging userptr
is that pinning tons of GFP_MOVEABLE memory isn't nice. Presuming our
shrinker works (big if, I know) there shouldn't be an issue with
memory exhausting in both cases. And imo we really should keep page
migration working given how people constantly invent new uses of it.
Now I see that most of them are for servers, but:
- server-y stuff tends to trickle down
- we seem to have a server gpu product in preparation (the recent pile
of display/PCH-less patches from Ben).
So I want to keep page migration working, even when we have tons of
userptr-backed gem bos around.

Now looking and SNA userptr and this batch the result is that SNA
won't use mmu-notifier-based userptr ever. That's feels a lot like
adding a bit of code (mmu-notifier-based userptr support) to sneak in
a different feature (pinning userptr support). If mmu notifiers are
too damn slow, we need to fix that (or work around it with some
cache), not hide it by flat-out not using them.

Also, Eric's persistent ranting about root-only interfaces moved me
away from them, so now I'm even leaning a bit towards simply requiring
mmu-notifiers for userptr. It feels at least like the Right Thing ;-)

> As for testing using SNA, you can just ask SNA to only use mmu-notifier
> backed userptr and so only run when mmu-notifiers are available.

Given how good we are at testing ugly corner case (much better, but
still with gapping holes apparently), I want mmu-notifier-based
userptr to be tested on as many machines as possible. Not just mine,
once when merging, and then we have a beautiful fireworks once mesa
starts to use it since it all regressed.

>> >> - On a quick read I've seen some gtt mmap support remnants. This scares
>> >>   me, a roundabout njet! would appease. Though I think that should already
>> >>   happen with the checks we have to reject snoopable buffers?
>> >
>> > That's because there are platforms where it is theorectically possible
>> > and whilst I have no use case for it, I wanted to make it work
>> > nevertheless. I still think it is possible, but I could not see a way to
>> > do so without completely replacing the drm vm code.
>>
>> If I understand things correctly we should be able to block this
>> simply by refusing to create an mmap offset for a userptr backed
>> object.
>
> But I want to be able to support GTT mappings wherever possible. The
> basic principle of making sure every bo is first class, so that there
> are no surprises. Such as stolen, dmabuf...

I can buy that argument, but in that case I _really_ want full
test-coverage of all the exposed interfaces. userptr is imo a big
addition with high potential to expose new "interesting" corner cases,
so I want to (ab)use the opportunity to improve our testing coverage.
If that's around, I'll happily accept unrestricted userptr support.
But see below for some comments.

>> >> - I think we should reject set_cacheing requests on usersptr objects.
>> >
>> > I don't think that is strictly required, just like we should not limit
>> > the user from using set_tiling. (Though the user is never actually going
>> > to tell the kernel about such tiling...).
>>
>> Yeah, I guess we could allow people to shot their foot off. Otoh it
>> adds another dimension to the userptr interface, which we need to make
>> sure it works. Similar besides set_tiling is probably also
>> prime_export.
>>
>> The reason I'd prefer to lock things down is that we have fairly nice
>> coverage for normal gem objects wrt exercising corner-cases with igt.
>> If we don't disallow the same corner-cases with userptr, I'd prefer
>> the tests to also cover those ... Disallowing is cheaper ;-)
>
> See above: every bo should be first class...

Already mentioned above, but I do have my doubts that the in-page
offset support really does mesh with gtt (and cpu mmaps fwiw) in a
first-class manner.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch