On Tue, Jun 23, 2015 at 04:43:24PM +0100, John Harrison wrote: > On 23/06/2015 14:24, Daniel Vetter wrote: > >On Tue, Jun 23, 2015 at 12:38:01PM +0100, John Harrison wrote: > >>On 22/06/2015 21:12, Daniel Vetter wrote: > >>>On Fri, Jun 19, 2015 at 05:34:12PM +0100, John.C.Harrison@xxxxxxxxx wrote: > >>>>From: John Harrison <John.C.Harrison@xxxxxxxxx> > >>>> > >>>>It is a bad idea for i915_add_request() to fail. The work will already have been > >>>>send to the ring and will be processed, but there will not be any tracking or > >>>>management of that work. > >>>> > >>>>The only way the add request call can fail is if it can't write its epilogue > >>>>commands to the ring (cache flushing, seqno updates, interrupt signalling). The > >>>>reasons for that are mostly down to running out of ring buffer space and the > >>>>problems associated with trying to get some more. This patch prevents that > >>>>situation from happening in the first place. > >>>> > >>>>When a request is created, it marks sufficient space as reserved for the > >>>>epilogue commands. Thus guaranteeing that by the time the epilogue is written, > >>>>there will be plenty of space for it. Note that a ring_begin() call is required > >>>>to actually reserve the space (and do any potential waiting). However, that is > >>>>not currently done at request creation time. This is because the ring_begin() > >>>>code can allocate a request. Hence calling begin() from the request allocation > >>>>code would lead to infinite recursion! Later patches in this series remove the > >>>>need for begin() to do the allocate. At that point, it becomes safe for the > >>>>allocate to call begin() and really reserve the space. > >>>> > >>>>Until then, there is a potential for insufficient space to be available at the > >>>>point of calling i915_add_request(). However, that would only be in the case > >>>>where the request was created and immediately submitted without ever calling > >>>>ring_begin() and adding any work to that request. Which should never happen. And > >>>>even if it does, and if that request happens to fall down the tiny window of > >>>>opportunity for failing due to being out of ring space then does it really > >>>>matter because the request wasn't doing anything in the first place? > >>>> > >>>>v2: Updated the 'reserved space too small' warning to include the offending > >>>>sizes. Added a 'cancel' operation to clean up when a request is abandoned. Added > >>>>re-initialisation of tracking state after a buffer wrap to keep the sanity > >>>>checks accurate. > >>>> > >>>>v3: Incremented the reserved size to accommodate Ironlake (after finally > >>>>managing to run on an ILK system). Also fixed missing wrap code in LRC mode. > >>>> > >>>>v4: Added extra comment and removed duplicate WARN (feedback from Tomas). > >>>> > >>>>v5: Re-write of wrap handling to prevent unnecessary early wraps (feedback from > >>>>Daniel Vetter). > >>>This didn't actually implement what I suggested (wrapping is the worst > >>>case, hence skipping the check for that is breaking the sanity check) and > >>>so changed the patch from "correct, but a bit fragile" to broken. I've > >>>merged the previous version instead. > >>>-Daniel > >>I'm confused. I thought your main issue was the early wrapping not the > >>sanity check. The check is to ensure that the reservation is large enough to > >>cover all the commands written during request submission. That should not be > >>affected by whether a wrap occurs or not. Wrapping does not magically add an > >>extra bunch of dwords to the emit_request() call. Whereas making the check > >>work with the wrap condition requires adding in extra tracking state of > >>exactly where the wrap occurred. That is extra code that only exists to > >>catch something in the very rare case which should already have been caught > >>in the very common case. I.e. if your reserved size is too small then you > >>will hit the warning on every batch buffer submission. > >The problem is that if you allow a wrap in the reserve size then the > >ringspace requirements are bigger than if you don't wrap. And since the > >add request is split up into many intel_ring_begin that's possible. Hence > >if you allow wrapping in the reserved space, then the most important case > >for the debug check is to make sure that it catches any kind of > >reservation overflow while wrapping. The not-wrapped case is probably the > >boring one. > > > >And indeed eventually we should overflow since according to your comment > >the worst case add request on ilk is 136 dwords. And the largest > >intel_ring_begin in there is 32 dwords, which means at most we'll throw > >away 31 dwords when wrapping. Which means the 160 dwords of reservation > >are not enough since we'd need 167 dwords of space for the worst case. But > >since the space_end debug check was a no-op for the wrapped case you won't > >catch this one. > > The minimum reservation size in this case is still only 136. The prepare > code checks for the 32 words actually requested and wraps if necessary. It > then checks for 136+32 words of space. If that would cause a wrap it will > then add on the amount of space actually left in the ring and wait for that > bigger total. That guarantees that it has waited for the 136 at the start of > the ring. The caller is then free to fill in the 32 words and there is still > guaranteed to be a minimum of 136 words available (with or without wrapping) > before any further wait for space is necessary. Thus the add_request() code > is safe from fear of failure irrespective of where any wrap might occur. > > > > > >Wrt keeping track of wrapping while the reservation is in use, the > >following should do that without any need of additional tracking: > > > > > > int used_size = ringbuf->tail - ringbuf->reserved_tail; > > > > if (used_size < 0) > > used_size += ringbuf->size; > > > > WARN(used_size < ringbuf->reserved_size, > > "request reserved size too small: %d vs %d!\n", > > used_size, ringbuf->reserved_size); > > > >I was mistaken that you can reuse __intel_ring_space (since that has > >slightly different requirements), but this gives you a nicely localized > >check for reservation overflow which works even when you wrap. Ofc it > >won't work if an add_request is bigger than the entire ring, but that's > >impossible anyway since we can at most reserve ringbuf->size - > >I915_RING_FREE_SPACE. > The problem with the above calculation is that it includes the wasted space > at the end of the ring. Thus it will complain the reserved size was too > small when in fact it was just fine. Ok I again misunderstood your patch a bit since it didn't quite do what I expect, and I stand corrected that v5 works too. But I still seem to fail to get my main concern across. I'll see whether I can whip up a patch as a short demonstration, maybe that helps to unconfuse this dicussion. For now I think we're covered with either v4 or v5 so sticking with either is ok with me. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
