| Message ID | 1372375867-1003-58-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Chris
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. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre 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.
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. -Chris
On Sat, Jun 29, 2013 at 8:44 AM, Chris Wilson <chris@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). Now since we've had a few funny bugs in this area already which proved to be rather hard to track down I think it's time to implement the relevant igt tests. The (currently only internal) i-g-t wiki has some information about what exactly blows up and what I think should be tested. I think it'd be a good requirement to block the real ppgtt enabling (not all the vma prep patches) until that test is ready. On that topic: Do we have other gaps in our testing, or is the current igt coverage sufficient for this massive refactoring? Ben, has anything blown up while you've developed these patches which was not caught by i-g-t? Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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@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? > > Now since we've had a few funny bugs in this area already which proved > to be rather hard to track down I think it's time to implement the > relevant igt tests. The (currently only internal) i-g-t wiki has some > information about what exactly blows up and what I think should be > tested. I think it'd be a good requirement to block the real ppgtt > enabling (not all the vma prep patches) until that test is ready. > I'll take a look on Monday and see try to start on it. I think if the test case is reasonable, then blocking the merge is fair, but I have to see what's in store before I decide whether or not to argue. > On that topic: Do we have other gaps in our testing, or is the current > igt coverage sufficient for this massive refactoring? Ben, has > anything blown up while you've developed these patches which was not > caught by i-g-t? Other than the one bug I haven't yet tracked down which I mentioned in 0000-cover-letter (I've never hit it in many IGT runs), I had one reproducible bug which was really hard to resolve. It passed all of the IGT tests, and caused some weird display corruption in UXA (it was the screenshot I posted, if you recall). It ran fine in SNA for a while, and then would hang. That wasn't even full PPGTT, it was just after the refactor. The root cause was some screwed up unbind logic where I ended up with a bogus unbind offset. In retrospect, I'm not really sure why the issue wasn't either more sever, or less, and also why IGT didn't hit it. It was the kind of bug which I don't feel is worth testing. Since addresses spaces are per fd, any test which opens multiple fds, and executes testable batches is a good test. I'm not really sure how many of them we have offhand, but we have a least a few. I think piglit on a composited desktop is one of the best focus tests we can run. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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@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. > > Now since we've had a few funny bugs in this area already which proved > > to be rather hard to track down I think it's time to implement the > > relevant igt tests. The (currently only internal) i-g-t wiki has some > > information about what exactly blows up and what I think should be > > tested. I think it'd be a good requirement to block the real ppgtt > > enabling (not all the vma prep patches) until that test is ready. > > > > I'll take a look on Monday and see try to start on it. I think if the > test case is reasonable, then blocking the merge is fair, but I have to > see what's in store before I decide whether or not to argue. > > > On that topic: Do we have other gaps in our testing, or is the current > > igt coverage sufficient for this massive refactoring? Ben, has > > anything blown up while you've developed these patches which was not > > caught by i-g-t? > > Other than the one bug I haven't yet tracked down which I mentioned in > 0000-cover-letter (I've never hit it in many IGT runs), I had one > reproducible bug which was really hard to resolve. It passed all of the > IGT tests, and caused some weird display corruption in UXA (it was the > screenshot I posted, if you recall). It ran fine in SNA for a while, and > then would hang. That wasn't even full PPGTT, it was just after the > refactor. The root cause was some screwed up unbind logic where I ended > up with a bogus unbind offset. In retrospect, I'm not really sure why > the issue wasn't either more sever, or less, and also why IGT didn't hit > it. It was the kind of bug which I don't feel is worth testing. Hm, I'm intrigued about this one, but also confused. Can you please elaborate on what exactly blows up? "unbind offset" is a new concept in my ears ... > Since addresses spaces are per fd, any test which opens multiple fds, > and executes testable batches is a good test. I'm not really sure how > many of them we have offhand, but we have a least a few. I think piglit > on a composited desktop is one of the best focus tests we can run. Yeah, switching between contexts and ppgtt is probably best done with real workloads. Worth checking though might be a very basic test with - 2 fds (so 2 address spaces) - a bunch of equally-sized objects - using them in inverse order on the two fds in the first batch (or any other trick to make sure that the two address spaces have completely different bindings) - blitting a bit of data around, alternating between the two fds Just to have a basic check for ppgtt switching. This test can probably derived quickly from one of the existing "copy stuff around" tests. drmtest helpers should also be useful. Even libdrm should keep on working if we set up two libdrm bufmgr instances. -Daniel
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@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
On Sun, Jun 30, 2013 at 1:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > 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. I was eyeing for the most minimal fix, but this is ok with me, too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sun, Jun 30, 2013 at 01:36:39PM +0200, Daniel Vetter wrote: > On Sun, Jun 30, 2013 at 1:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > 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. > > I was eyeing for the most minimal fix, but this is ok with me, too. > -Daniel > So, just fix the test?
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a4db2cc..e58584b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3746,6 +3746,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, return -EBUSY; BUG_ON(map_and_fenceable && !is_i915_ggtt(vm)); + BUG_ON(!HAS_HW_CONTEXTS(obj->base.dev) && !is_i915_ggtt(vm)); if (i915_gem_obj_bound(obj, vm)) { if ((alignment && @@ -3800,6 +3801,7 @@ int i915_gem_pin_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { + struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_pin *args = data; struct drm_i915_gem_object *obj; int ret; @@ -3808,6 +3810,11 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data, if (ret) return ret; + if (!dev_priv->hw_contexts_disabled) { + mutex_unlock(&dev->struct_mutex); + return -ENXIO; + } + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); if (&obj->base == NULL) { ret = -ENOENT;
Pin doesn't fit with PPGTT since the interface doesn't allow for the context for which we want to pin. Full PPGTT will bring a new "soft pin" interface. The semantics of which will probably take some time to iron out. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 7 +++++++ 1 file changed, 7 insertions(+)