[Intel-gfx] [PATCH 05/59] drm/i915: Reserve ring buffer space for i915_add_request() commands

[Chris Wilson]

On Mon, Mar 23, 2015 at 09:54:41AM +0100, Daniel Vetter wrote:
> On Fri, Mar 20, 2015 at 03:55:59PM +0000, John Harrison wrote:
> > On 20/03/2015 15:13, Daniel Vetter wrote:
> > >On Thu, Mar 19, 2015 at 12:30:10PM +0000, John.C.Harrison at Intel.com wrote:
> > >>+void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf, int size)
> > >Just a bit of interface bikeshed - I'd drop the size parameter here. It
> > >just duplicates what we tell the ring in the reservation code and the real
> > >check happens in the _end function.
> > >
> > >>+{
> > >>+	WARN_ON(size > ringbuf->reserved_size);
> > >>+	WARN_ON(ringbuf->reserved_in_use);
> > >>+
> > >>+	ringbuf->reserved_in_use = true;
> > >>+	ringbuf->reserved_tail   = ringbuf->tail;
> > >>+}
> > >>+
> > >>+void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> > >>+{
> > >>+	WARN_ON(!ringbuf->reserved_in_use);
> > >>+	WARN_ON(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size);
> > >Don't we need to handle wrap-around to make sure we do correctly check for
> > >sufficient reservation?
> > >-Daniel
> > 
> > There is nothing special to worry about for wrapping. The regular
> > intel_ring_begin() code will handle all that as before. The whole point of
> > the reserved scheme is that it is basically the same as calling
> > intel_ring_begin() with 'size + RESERVED_SIZE' everywhere. So when
> > i915_add_request() starts, it is guaranteed that an
> > 'intel_ring_begin(RESERVED_SIZE)' has been done already including any
> > necessary buffer wrapping. Thus it does not actually need to call
> > 'i_r_begin()' at all, really - it is guaranteed to succeed (as long as it
> > stays within RESERVED_SIZE total usage).
> 
> I think there's a misunderstanding. Let me try to explain again.
> 
> ringbuf->tail gets wrapped around, but the expression
> "ringbuf->reserved->tail + ringbuf->reserved_size" doesn't. And the
> comparison ">" also doesn't handle wrap around.
> 
> All taken together there won't be a spurios WARN, buf if you wrap ->tail
> and haven't reserved enough space your check wont catch this. The only
> thing you need is reserved_size+reserved_tail > RINGBUF_SIZE and your WARN
> won't fire, not even if we wrap the ring a few times.
> 
> This is even more important since the wrapping itself increases space
> requirements a bit, and so we want to be especially careful with checking
> that we reserved enough there.
> 
> Or do I miss something?

The question is why even bother to reserve space for the request
emission? If the request occupies so much ring space that the final
flush+breadcrumb+interrupt cannot fit into what remains, then we will
throw a WARN due to out of ring-space and percolate up the failure and
unwind the failed request.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre