Re: [PATCH] drm/i915: Bug fixes to ring 'head' updating

[Dave Gordon <david.s.gordon@intel.com>]

On 18/11/14 15:00, Deepak S wrote:
> 
> On Monday 03 November 2014 06:59 PM, Dave Gordon wrote:
>> Fixes to both the LRC and the legacy ringbuffer code to correctly
>> calculate and update the available space in a ring.
>>
>> The logical ring code was updating the software ring 'head' value
>> by reading the hardware 'HEAD' register. In LRC mode, this is not
>> valid as the hardware is not necessarily executing the same context
>> that is being processed by the software. Thus reading the h/w HEAD
>> could put an unrelated (undefined, effectively random) value into
>> the s/w 'head' -- A Bad Thing for the free space calculations.
>>
>> In addition, the old code could update a ringbuffer's 'head' value
>> from the 'last_retired_head' even when the latter hadn't been recently
>> updated and therefore had a value of -1; this would also confuse the
>> freespace calculations. Now, we consume 'last_retired_head' in just
>> one place, ensuring that this confusion does not arise.
>>
>> Change-Id: Id7ce9096ed100a2882c68a54206f30b6c87e92fa
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_dma.c         |    5 ++-
>>   drivers/gpu/drm/i915/intel_lrc.c        |   36 ++++++++++-----------
>>   drivers/gpu/drm/i915/intel_ringbuffer.c |   53
>> ++++++++++++++++---------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
>>   4 files changed, 48 insertions(+), 47 deletions(-)
>>
[snip]
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index a8f72e8..1150862 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;
>> +}
>> +
>> +void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
>> +{
>> +    if (ringbuf->last_retired_head != -1) {
>> +        ringbuf->head = ringbuf->last_retired_head;
>> +        ringbuf->last_retired_head = -1;
>> +    }
>> +
>> +    ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR,
>> +                        ringbuf->tail, ringbuf->size);
>>   }
>>     int intel_ring_space(struct intel_ringbuffer *ringbuf)
>>   {
>> -    return __intel_ring_space(ringbuf->head & HEAD_ADDR,
>> -                  ringbuf->tail, ringbuf->size);
>> +    intel_ring_update_space(ringbuf);
>> +    return ringbuf->space;
>>   }
>>     bool intel_ring_stopped(struct intel_engine_cs *ring)
>> @@ -73,7 +84,7 @@ bool intel_ring_stopped(struct intel_engine_cs *ring)
>>   void __intel_ring_advance(struct intel_engine_cs *ring)
>>   {
>>       struct intel_ringbuffer *ringbuf = ring->buffer;
>> -    ringbuf->tail &= ringbuf->size - 1;
>> +    intel_ring_advance(ring);
> 
> Should this be in another patch?
> 
> Other than this other changes looks fine to me.\
> Also, are you planning to add WARN_ON if there is a mismatch with
> ring_begin & add_request?

Yes, that's another patch that I'll be sending out soon; it also checks
for various other mistakes in ring management.

Meanwhile I've decided to move the line above into that patch rather
than this one; also, I've refactored this patch to break out the two
sections that fix specific bugs in the LRC code, so I shall send out a
new version shortly.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx