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

[Jesse Barnes]

On Mon, 8 Apr 2013 21:24:58 +0200
Daniel Vetter <daniel at ffwll.ch> 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.
> - 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'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.
> 
> 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
> 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).

Resurrecting this again.

I'm of two minds on the API here: on the one hand, it can be nicer for
the kernel to handle this stuff if it can be done easily, and save
userspace the trouble.  But OTOH, consistent with existing page based
interfaces makes things a little less jarring...

> 
> >> - 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 think we'll need good test cases to cover things regardless.  And
yes, an mmu notifier version that doesn't require root would be more
generally useful than a root only interface (or are those items
unrelated at this point?).  Having a flag for root only operation for
clients that know what they're doing is fine though, IMO.

I think one of the nice use cases the Mesa guys have is to save an
extra copy in glReadPixels for certain types of objects, which means
non-root is a requirement.  And it seems like we could do a
glReadPixels extension that would end up being zero copy with this
interface (provided it had pixel accessor functions too to deal with
de-swizzling).

> 
> >> - 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.

Presumably the user has it mapped already through some other means, so
this shouldn't be a big limitation.  Would it simplify things a lot
and/or drop a lot of code?

> >> - 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 ;-)

Do we have a good use case for allowing the tiling and/or caching
requests?  If not, yeah I'd just say punt on it for now.

So, any chance we can land this for 3.12?  Would be nice to have a text
file or something describing usage too...

-- 
Jesse Barnes, Intel Open Source Technology Center