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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
