On 12/06/15 19:05, Chris Wilson wrote: > On Fri, Jun 12, 2015 at 06:09:08PM +0100, Dave Gordon wrote: >> The original idea of preallocating the OLR was implemented in >> >>> 9d773091 drm/i915: Preallocate next seqno before touching the ring >> >> and the sequence of operations was to allocate the OLR, then wrap past >> the end of the ring if necessary, then wait for space if necessary. >> But subsequently intel_ring_begin() was refactored, in >> >>> 304d695 drm/i915: Flush outstanding requests before allocating new seqno >> >> to ensure that pending work that might need to be flushed used the old >> and not the newly-allocated request. This changed the sequence to wrap >> and/or wait, then allocate, although the comment still said >> /* Preallocate the olr before touching the ring */ >> which was no longer true as intel_wrap_ring_buffer() touches the ring. >> >> However, with the introduction of dynamic pinning, in >> >>> 7ba717c drm/i915/bdw: Pin the ringbuffer backing object to GGTT on-demand >> >> came the possibility that the ringbuffer might not be pinned to the GTT >> or mapped into CPU address space when intel_ring_begin() is called. It >> gets pinned when the request is allocated, so it's now important that >> this comes *before* anything that can write into the ringbuffer, in this >> case intel_wrap_ring_buffer(), as this will fault if (a) the ringbuffer >> happens not to be mapped, and (b) tail happens to be sufficiently close >> to the end of the ring to trigger wrapping. > > On the other hand, the request allocation can itself write into the > ring. This is not the right fix, that is the elimination of olr itself > and passing the request into intel_ring_begin. That way we can explicit > in our ordering into ring access. > -Chris AFAICS, request allocation can write into the ring only if it actually has to flush some *pre-existing* OLR. [Aside: it can actually trigger writing into a completely different ringbuffer, but not the one we're handling here!] The worst-case sequence is: i915_gem_request_alloc finds there's no OLR i915_gem_get_seqno finds the seqno is 0 i915_gem_init_seqno for_eash_ring do ... intel_ring_idle but no OLR, so OK It only works because i915_gem_request_alloc() allocates the request early but doesn't store it in the OLR until the end. OTOH I agree that the long-term answer is the elimination of the OLR; this is really something of a stopgap until John H's Anti-OLR patchset is merged. Although, the simplification of the wait-wrap/wait-space sequence is probably worthwhile in its own right, so if Anti-OLR gets merged first we can put the rest of the changes on top of that. It's only code inside the "if(!OLR)" section that would need to be removed. .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
