Re: [PATCH 05/10] drm/i915: Disable 'get seqno' workaround for VLV

From: Dave Gordon <david.s.gordon@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>, John.C.Harrison@Intel.com
Cc: Intel-GFX@Lists.FreeDesktop.Org
Subject: Re: [PATCH 05/10] drm/i915: Disable 'get seqno' workaround for VLV
Date: Wed, 10 Dec 2014 17:11:09 +0000	[thread overview]
Message-ID: <54887EAD.4020807@intel.com> (raw)
In-Reply-To: <20141210104224.GQ27182@phenom.ffwll.local>

On 10/12/14 10:42, Daniel Vetter wrote:
> On Tue, Dec 09, 2014 at 12:59:08PM +0000, John.C.Harrison@Intel.com wrote:
>> From: Dave Gordon <david.s.gordon@intel.com>
>>
>> There is a workaround for a hardware bug when reading the seqno from the status
>> page. The bug does not exist on VLV however, the workaround was still being
>> applied.
> 
> Given how much trouble the missed seqno fun cause us I'd like more
> justification. What kind of (stress)-testing has been done here? And
> you're sure you've never seen the little dmesg notice that the kernel
> switched to polling?
> -Daniel

This was necessary during VLV scheduler/preemption work, where we
stressed the command streamer and IRQ path probably more than any of the
usual tests, since we were looking for out-of-sequence anomalies.

The comments in gen6_ring_get_seqno() say it's to fix a bug on ivb/snb,
and nothing in the BSpec suggests it's needed on later chips.

I think the problem was really in the fact that the workaround
implemented in gen6_ring_get_seqno() was reading a nonshadowed register,
therefore triggering forcewake activity, which was pretty buggy; so we
took out the gen6-specific code before finding that there were still
other problems with forcewake.

[Aside]
The original intention appears to have been to generate a single I/O
READ cycle, forcing all pending inbound DMA so be flushed to memory, so
that the subsequent memory read would see the latest value. But now, it
doesn't just generate a single cycle but a whole flurry of writes and
polling reads, because reading ACTHD requires forcewake (unless that was
wrong, and has now been fixed). So should gen6_ring_get_seqno() instead
read a register that doesn't require forcewake? I wouldn't object to
using it in perpetuity if it really only read one register!
[/aside]

.Dave.

>> Change-Id: Ic781fdb31e1f794ce1fa8a6d0d5ee379756c5db6
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ringbuffer.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 3887f1a..f990ce4 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -2367,7 +2367,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>>  		ring->irq_get = gen6_ring_get_irq;
>>  		ring->irq_put = gen6_ring_put_irq;
>>  		ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
>> -		ring->get_seqno = gen6_ring_get_seqno;
>> +		if (IS_VALLEYVIEW(dev))
>> +			ring->get_seqno = ring_get_seqno;
>> +		else
>> +			ring->get_seqno = gen6_ring_get_seqno;
>>  		ring->set_seqno = ring_set_seqno;
>>  		if (i915_semaphore_is_enabled(dev)) {
>>  			ring->semaphore.sync_to = gen6_ring_sync;
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> 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