Re: [PATCH] drm/i915: correctly order the ring init sequence

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 07 Aug 2012, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 07 Aug 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> We may only start to set up the new register values after having
>> confirmed that the ring is truely off. Otherwise the hw might lose the
>> newly written register values. This is caught later on in the init
>> sequence, when we check whether the register writes have stuck.
>
> With or without (up to you) the comment clarification discussed in IRC,

For the record,

15:14          danvet   "This must happen _after_ we've cleared the ring 
                        registers with the above sequence (the readback of the 
                        HEAD registers also enforces ordering), otherwise the 
                        hw might lose the new ring register values."


>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
>>
>> Cc: stable@vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522
>> Tested-by: Yang Guang <guang.a.yang@intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/i915/intel_ringbuffer.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index bf0195a..5b19917 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -289,8 +289,6 @@ static int init_ring_common(struct intel_ring_buffer *ring)
>>  	I915_WRITE_HEAD(ring, 0);
>>  	ring->write_tail(ring, 0);
>>  
>> -	/* Initialize the ring. */
>> -	I915_WRITE_START(ring, obj->gtt_offset);
>>  	head = I915_READ_HEAD(ring) & HEAD_ADDR;
>>  
>>  	/* G45 ring initialization fails to reset head to zero */
>> @@ -316,6 +314,10 @@ static int init_ring_common(struct intel_ring_buffer *ring)
>>  		}
>>  	}
>>  
>> +	/* Initialize the ring. This must happen _after_ we have confirmed that
>> +	 * the ring is off (with the above head == 0 check), otherwise the hw
>> +	 * might lose the new ring register values. */
>> +	I915_WRITE_START(ring, obj->gtt_offset);
>>  	I915_WRITE_CTL(ring,
>>  			((ring->size - PAGE_SIZE) & RING_NR_PAGES)
>>  			| RING_VALID);
>> -- 
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx