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

[Daniel Vetter]

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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch