[Intel-gfx] [PATCH 57/66] drm/i915: Disallow pin with full ppgtt

[Chris Wilson]

On Sun, Jun 30, 2013 at 01:06:47PM +0200, Daniel Vetter wrote:
> On Sat, Jun 29, 2013 at 11:56:53PM -0700, Ben Widawsky wrote:
> > On Sat, Jun 29, 2013 at 04:34:07PM +0200, Daniel Vetter wrote:
> > > On Sat, Jun 29, 2013 at 8:44 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > > > On Fri, Jun 28, 2013 at 10:43:30PM -0700, Ben Widawsky wrote:
> > > >> On Fri, Jun 28, 2013 at 09:55:27AM +0100, Chris Wilson wrote:
> > > >> > On Thu, Jun 27, 2013 at 04:30:58PM -0700, Ben Widawsky wrote:
> > > >> > > Pin doesn't fit with PPGTT since the interface doesn't allow for the
> > > >> > > context for which we want to pin.
> > > >> >
> > > >> > Nak. Pin still retains it semantics with the gtt and only applies to the
> > > >> > gtt.
> > > >>
> > > >>
> > > >> Here is the error I have on pin. I was trying to debug it previously but
> > > >> got sidetracked. I thought some combo of EXEC_GTT flag and hacks would
> > > >> make it work, but I never finished. Maybe you know offhand what I've
> > > >> messed up, and the right way to fix it?
> > > >>
> > > >> gem_pin: gem_pin.c:84: exec: Assertion `gem_exec[0].offset == offset' failed.
> > > >
> > > > Ok, that is a condition that no longer holds with full ppgtt. Now
> > > > fortunately, userspace that might depend upon that is limited to DRI1
> > > > era machines (at least in the userspace I know about) and we can just
> > > > update the test to understand that pinning and exec are two different
> > > > address spaces.
> > > >
> > > > How do you handle EXEC_OBJECT_NEEDS_GTT? As that may be an acceptable
> > > > w/a. Or just skip that portion of the test if PRAM_HAS_FULL_PPGTT. Soft
> > > > pinning should be tested separately (so that it isn't confused with
> > > > pinning to the ggtt), but that is also a viable solution to this portion
> > > > of the test.
> > > 
> > > NEEDS_GTT is only valid where we alias the global GTT and PPGTT (i.e.
> > > snb). So I think we should reject it on other platforms (or silently
> > > ignore it if userspace uses it already).
> > 
> > What do you recommend as the resolution to the failing gem_in then?
> 
> IIrc that was a test I've asked for since Chris wanted to use pin/unpin to
> work around the i830/i845 cs tlb bug in a better way, just to exercise the
> basics. I think the right option would be to reject pin on gen6+ since
> those platforms are kms-only and not one of the few kms platforms where we
> (ab)use pinning. Also all the earlier platforms don't have any (useful) hw
> ppgtt implementation. We don't need pinning on gen4/5 iirc, but still
> allowing it there increases test coverage with igt/gem_pin - now that we
> have a test we better make use of it.

I respectfully disagree. The semantics of the pin ioctl remain useful
even with the ggtt/ppgtt split, and I think barring its use forever more
is unwise. Not that pinning is a good solution, just in some cases it
may be the only solution. (It has proven useful in the past, it is
likely to do so again.) All that we need to do is note that the offset
returned by pin is ggtt and the offsets used by execbuffer are ppgtt. So
keep pin-ioctl and fix the test not to assume that pin.offset is
meaningful with execbuffer after HAS_FULL_PPGTT.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre