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

Message ID 1441899263-12986-1-git-send-email-jbarnes@virtuousgeek.org
State New
Headers show

Commit Message

Jesse Barnes Sept. 10, 2015, 3:34 p.m. UTC
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(-)

Comments

Ville Syrjälä Sept. 10, 2015, 4:11 p.m. UTC | #1
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.

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.

>  
>  		if (timeout <= 0) {
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jesse Barnes Sept. 10, 2015, 4:17 p.m. UTC | #2
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?

> 
> 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ä Sept. 10, 2015, 4:27 p.m. UTC | #3
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

Patch
diff mbox

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;
 
 		if (timeout <= 0) {