[Intel-gfx] [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Dave Gordon]

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,

.Dave.