Re: [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: workaround bad DSL readout in start of pipe update
Date: Thu, 10 Sep 2015 19:27:11 +0300	[thread overview]
Message-ID: <20150910162711.GP29811@intel.com> (raw)
In-Reply-To: <55F1AD13.4090707@virtuousgeek.org>

On Thu, Sep 10, 2015 at 09:17:23AM -0700, Jesse Barnes wrote:
> On 09/10/2015 09:11 AM, Ville Syrjälä wrote:
> > On Thu, Sep 10, 2015 at 08:34:22AM -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.
> >> Workaround that by avoiding updates in the first couple of scanlines.
> >> In testing, even a delay of a single microsecond is enough to give us a
> >> good DSL value again, so the millisecond we'll wait when we hit this
> >> case occasionally ought to be plenty.
> >>
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91579
> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> ---
> >>  drivers/gpu/drm/i915/intel_sprite.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index ca7e264..0c2c62f 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -113,8 +113,16 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
> >>  		 */
> >>  		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> >>  
> >> +		/*
> >> +		 * On HSW, the DSL reg (0x70000) appears to return 0 if we
> >> +		 * read it right around the start of vblank.  So skip past it
> >> +		 * so we don't accidentally end up spanning a vblank frame
> >> +		 * increment, causing the update_end() code to squak at us.
> >> +		 * (We use 2 in the comparison to account for the
> >> +		 * scanline_offset used to correct the DSL readout.)
> >> +		 */
> >>  		scanline = intel_get_crtc_scanline(crtc);
> >> -		if (scanline < min || scanline > max)
> >> +		if (scanline > 2 && (scanline < min || scanline > max))
> >>  			break;
> > 
> > And it means we'll miss a frame whenever the scanline is 0-2 even on a
> > non-broken. So I don't kike it.
> 
> We only stall 1ms in the timeout later, so we shouldn't miss a frame, we'll just queue the update in the middle of it instead, right?

Hmm. Right I forgot the 1ms timeout. Well, it's susceptible to
scheduling delays since we've re-enabled the interrupts and all, so
there's no guarantee that it's just 1ms. Also once we'll retry the loop
we'll just bail out if the timeout has already expired, so there's
really no guarantee anymore that we won't be in the danger zone when
we enter the crirical section.

> 
> > 
> > Dunno maybe something more targeted like:
> > 
> > read dsl
> > if (IS_HASWELL && scanline == crtc->scanline_offset) {
> > 	udelay(1);
> > 	read dsl again
> > }
> > in __intel_get_crtc_scanline()?
> > 
> > The udelay() is a bit unfortunate, but we'll need accurate scanline
> > informnation for the vblank timestamps too.
> 
> Yeah, hiding it in the get_crtc_scanline() might be better; it would be good to know if this affects other platforms as well though.  More testing is needed for that.
> 
> Jesse

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx