[Intel-gfx] [PATCH 2/2] drm/i915: Rework order of operations in {__intel, logical}_ring_prepare()

[Chris Wilson]

On Fri, Jun 12, 2015 at 07:54:17PM +0100, Dave Gordon wrote:
> 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:

You forget that ultimately (or rather should have been in the design for
execlists once the shortcomings of the ad hoc method were apparent) 
equest allocation will also be responsible for context management (since
they are one and the same).

> 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.

See my patches a year ago for a more complete cleanup.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre