Re: [PATCH] drm/i915: workaround bad DSL readout v2

From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: workaround bad DSL readout v2
Date: Tue, 15 Sep 2015 14:00:49 -0700	[thread overview]
Message-ID: <55F88701.2060602@virtuousgeek.org> (raw)
In-Reply-To: <20150910223308.GJ26517@intel.com>

On 09/10/2015 03:33 PM, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 02:57:32PM -0700, Jesse Barnes wrote:
>> On 09/10/2015 02:53 PM, Ville Syrjälä wrote:
>>> On Thu, Sep 10, 2015 at 02:38:53PM -0700, Jesse Barnes wrote:
>>>> On HSW at least (still testing other platforms, but should be harmless
>>>> elsewhere), the DSL reg reads back as 0 when read around vblank start
>>>> time.  This ends up confusing the atomic start/end checking code, since
>>>> it causes the update to appear as if it crossed a frame count boundary.
>>>> Avoid the problem by making sure we don't return scanline_offset from
>>>> the get_crtc_scanline function.  In moving the code there, I add to add
>>>> an additional delay since it could be called and have a legitimate 0
>>>> result for some time (depending on the pixel clock).
>>>>
>>>> v2: move hsw dsl read hack to get_crtc_scanline (Ville)
>>>>
>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579
>>>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>> index 90bc6c2..97e5d52 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -697,6 +697,27 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>>>>  		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
>>>>  
>>>>  	/*
>>>> +	 * On HSW, the DSL reg (0x70000) appears to return 0 if we
>>>> +	 * read it right around the start of vblank.  So try it again
>>>> +	 * so we don't accidentally end up spanning a vblank frame
>>>> +	 * increment, causing the pipe_update_end() code to squak at us.
>>>> +	 */
>>>> +	if (IS_HASWELL(dev) && !position) {
>>>> +		int i, temp;
>>>> +
>>>> +		for (i = 0; i < 100; i++) {
>>>> +			udelay(1);
>>>> +			temp = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
>>>> +				DSL_LINEMASK_GEN3;
>>>> +			if (temp != position) {
>>>> +				position = temp;
>>>> +				goto out;
>>>> +			}
>>>> +		}
>>>> +	}
>>>
>>> Hmm. Another idea. If it always happens at start of vbl, maybe have a
>>> look at the ISR. If scanline reads 0, but ISR says we're in vblank, just
>>> return vblank_start. That's assming ISR would in fact show that it's in
>>> vblank when this happens.
>>
>> Sounds a bit racy,
> 
> Not too much I think. We're under spinlock_irqsave here, and if the
> scanline should magically start to tick just after we've read it we
> would still give almost the right answer, even if we got magically
> delayed by one or two scanlines. That's assuming that it has already
> latched the double buffered registers when it goes wonky. If the
> register latching happens only when the counter starts working again,
> well then we're in deeper doodoo. But then I wouldn't expect ISR to
> indicate that we're in vblank then either. So I think you should
> definitely check what the ISR actually tells you while things are
> in the bad state.
> 
> In case the badness happens just before start of vblank, well, then
> I have no good idea how to handle it. This sort of retry loop is
> the best that comes to mind right now :(
> 
>> though I guess it wouldn't hurt.  I'm actually a
>> little dubious about this change anyway since it will mean longer delays
>> when we really are at the top of the field/frame.  You sure you don't
>> like the first one better?
> 
> The first one can't help vblank timestamps. Although I suppose that if
> the badness always cures itself before the interrupt gets raised, we
> would not hit this normally when we update the timestamp from the irq
> handler. We could hit it when updating the timestamp outside the irq
> handler though (ie. during vblank_get/put etc.).
> 

Ok so just for the record, I've tried a few things:
  - using the ISR to check the vblank and vsync status when this occurs (both are clear)
  - using the transcoder debug reg for reading the scanline (also returns 0)

so I think we're stuck with one of the two approaches already posted.  As for affecting the vblank timestamps, I think you're right that this bug shouldn't affect that.  The issue (a 0 return from the PIPEDSL read) seems to manifest just as the frame counter should be getting incremented.  By the time we actually raise an interrupt or move to the next scanline, the correct value is returned.  So even if we stuff the workaround into the pipe update start function (which should be more tolerant of delays in general) I think we'll be safe.

What do you prefer?  Do you want any changes on top of either of the previous patches given the above?

Thanks,
Jesse

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