On Mon, Nov 24, 2014 at 02:32:25PM +0000, Dave Gordon wrote:
> On 24/11/14 10:04, Daniel Vetter wrote:
> > On Tue, Nov 18, 2014 at 08:07:22PM +0000, Dave Gordon wrote:
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index ae09258..a08ae65 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -52,16 +52,27 @@ intel_ring_initialized(struct intel_engine_cs *ring)
> >>
> >> int __intel_ring_space(int head, int tail, int size)
> >> {
> >> - int space = head - (tail + I915_RING_FREE_SPACE);
> >> - if (space < 0)
> >> + int space = head - tail;
> >> + if (space <= 0)
> >> space += size;
> >> - return space;
> >> + return space - I915_RING_FREE_SPACE;
> >
> > Changing the free space computation doesn't seem required, but resulting
> > in Chris&me just discussing it on irc to convince ourselves it's not
> > broken accidentally now. Can you please respin you patch with this change
> > dropped?
> >
> > In generally it's good practice to review the diff after committing a
> > patch and hunt for any unecessary changes. Imo even when the endresult
> > looks a bit ulgy (e.g. crazy ordering of static functions which requires
> > tons of predeclarations) it's better if the resulting diff looks cleaner.
> > And if things get out of hand we can always do a pure cleanup patch.
> > -Daniel
>
> This isn't an accidental change; it's to improve resilience in the case
> that head and/or tail end up with values they shouldn't have.
>
> Here's a C program to model the two different calculations in a tiny ring:
>
> #include <stdio.h>
>
> #define FREE 4
> #define SIZE 8
>
> main()
> {
> int h, t, s1, s2;
>
> printf("%s\t%s\t%s\t%s\n", "Head", "Tail", "OSpace", "NSpace");
> for (h = 0; h <= SIZE; ++h)
> {
> for (t = 0; t <= SIZE; ++t)
> {
> s1 = h - (t + FREE);
> if (s1 < 0)
> s1 += SIZE;
>
> s2 = h - t;
> if (s2 <= 0)
> s2 += SIZE;
> s2 -= FREE;
>
> printf("%2d\t%2d\t%2d\t%2d\n", h, t, s1, s2);
> }
> printf("\n");
> }
> }
>
> OSpace (s1) uses the old code, whereas NSpace (s2) is my new method.
> They agree for all well-behaved cases, but look at this snippet:
>
> Head Tail OSpace NSpace
> 6 0 2 2
> 6 1 1 1
> 6 2 0 0
> 6 3 7 -1
> 6 4 6 -2
> 6 5 5 -3
> 6 6 4 4
> 6 7 3 3
> 6 8 2 2
>
> Both the old and new code give the right answers if HEAD and TAIL have
> legitimate values. But if TAIL should somehow advance further than it
> should, and run into the reserved area, the old code might tell you that
> there's now LOTS of space available, whereas the new code returns "less
> than zero" space available.
>
> For example, the old calculation tells us that if head=6 and tail=4 then
> there are 6 slots available -- which just can't be right, as by
> definition the answer should never be more than (SIZE-RSVD). I'd much
> rather get the answer "-2" for this case, as it's then obvious that this
> really shouldn't happen.
>
> In particular, this new code would mean that the commonly-used test
> (available >= required) would immediately reject further writes into the
> ring after an overrun, giving some chance that we can recover from or at
> least diagnose the original problem; whereas allowing more writes would
> likely both confuse the h/w and destroy the evidence.
>
> It's also easier to understand, IMHO (if experienced programmers such as
> you & Chris had to discuss the old code to be confident that it was
> correct, that already suggests that it's not as clear as it could be).
>
> The used space in a ring is given by the cyclic distance from the
> consumer to the producer; conversely, the available space in a ring is
> the cyclic distance from the producer to the consumer, MINUS the amount
> reserved. The new code expresses that directly, without having to figure
> out the meaning of ADDING the reserved amount to the tail before
> subtracting it from head. In ASCII pix:
>
> +++++++++++++++++++
> + free + 0
> + free + 1
> + reserved + 2
> + reserved + 3
> (consumer) HEAD-> + used + 4
> + used + 5
> + used + 6
> + used + 7
> + used + 8
> + used + 9
> (producer) TAIL-> + free + 10
> + free + 11
> +++++++++++++++++++
>
> The sketch above shows the situation with size=12, reserved=2, TAIL=10
> and HEAD=4. Slots 4 to 9 are used (TAIL-HEAD = 10-4 = 6 slots). The
> available space is (HEAD-TAIL (+SIZE) - RSVD = 4-10+12-2 = 4 slots).
>
> +++++++++++++++++++
> + used + 0
> + used + 1
> (producer) TAIL-> + free + 2
> + free + 3
> + reserved + 4
> + reserved + 5
> (consumer) HEAD-> + used + 6
> + used + 7
> + used + 8
> + used + 9
> + used + 10
> + used + 11
> +++++++++++++++++++
>
> After TAIL has wrapped, we might have this situation with TAIL=2 and
> HEAD=6. Used space is (TAIL-HEAD (+SIZE) = 2-6+12 = 8 slots), and
> available space is (HEAD-TAIL - RSVD) = 6-2-2 = 2 slots. Straightforward
> and easy to understand :)
>
> So, I'd definitely prefer to keep this new code. We not only want to do
> the calculation in only one place, but we want to do it in the best
> possible way, with the minimum chance of propagating errors and
> confusing both h/w and developers if someone does introduce a ring
> overrun or off-by-one error in some ring-stuffing code elsewhere.
> (BTW,
Given the extensive explanation you've delivered here this _definitely_
should be in a separate patch. Rule-of-thumb: If you have multipled pieces
in your commit message/explanation and tie them together in the
explanation with an "and" you should consider splitting the patch along
the "and"s. Except when it's all really trivial stuff (e.g. going ocd over
docs or so). For the commit message please just reuse the above mail, it's
a great explanation!
And could we perhaps WARN_ON if the negative space thing happens, which
should make such an occurance a lot easier to understand?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
