[Intel-gfx] [PATCH] drm/i915: workaround bad DSL readout v2

[Ville Syrjälä]

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 at 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.).

-- 
Ville Syrjälä
Intel OTC