On Wed, Feb 25, 2015 at 11:17:00PM +0100, Daniel Vetter wrote: > On Fri, Feb 13, 2015 at 11:48:14AM +0000, John.C.Harrison@xxxxxxxxx wrote: > > From: John Harrison <John.C.Harrison@xxxxxxxxx> > > > > For some reason, the i915_add_request() call in > > i915_gem_execbuffer_retire_commands() was explicitly having its return code > > ignored. The _retire_commands() function itself was 'void'. Given that > > _add_request() can fail without dispatching the batch buffer, this seems odd. > > I was so convinced we've had a commit somewhere explaining this, but > apparently not. > > The deal is that after the dispatch call we have the batch commit and > there's no going back any more, which also means we can't return an error > code from the ioctl. So if you return -EIO or -ENOMEM that's a pretty bad > lie and you really have to ignore that error code. > > Again I've tried to dig up the commit for that but that was lost in the > maze of the past 5 years of changes. We've had piles of older approaches > to deal with this issue: > - Don't even emit a request, just mark objects as gpu dirty. Only when > waiting did we emit flushes and requests, which again again gave us a > context to return the error. This resulted in horrible latency since > flushes where wait too late and also all that book-keeping was not worth > it at all. Don't ask ;-) > - Emit flushes right away, but if we fail to alloc the request set the > outstanding lazy request bit. The job of the check_olr function used in > waits was to notice that and retry the allocation. > - Preallocate the request, but that still leaves the possibility that the > gpu dies. But since we've committed hangcheck will clean this up and we > can just ignore the -EIO. It's actually worse since it's not just -EIO but also -EINTR, returned by intel_ring_begin when we're thrashing the gpu a bit too badly with requests. Which means we really need to guarantee that the request is completed properly, eventually since it's not just for fatal gpu hangs. Atm that's done by only clearing outstanding_lazy_request after we've really emitted the request fully. That guarantees that even when parts of the request emission to the ringbuf fails we'll retry on the next wait if needed. A possible fix to make this infallible would be to reserve some fixed amount of ringbuf credit at request creation time and then consume it here. Of course we'd need checks to make sure we never use more ringspace than what we reserve. To avoid massive churn we could convert I915_RING_FREE_SPACE into a variable and increase it enough when allocating the request. And then reduce it again at the start of add_request. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
