[PATCH v2 1/2] drm/i915: Add a workaround for HSW scanline counter weirdness

[PATCH v2 1/2] drm/i915: Add a workaround for HSW scanline counter weirdness

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On HSW the scanline counter seems to behave differently depending on
the output type. eDP on port A does what you would expect an the normal
+1 fixup is sufficient to cover it. But on HDMI outputs we seem to need
a +2 fixup. Just assume we always need the +2 fixup and accept the
slight inaccuracy on eDP.

This fixes a regression introduced in:
 commit 8072bfa6045a264d3913102a35fab125b06603a2
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Mon Oct 28 21:22:52 2013 +0200

    drm/radeon: Move the early vblank IRQ fixup to radeon_get_crtc_scanoutpos()

That commit removed the heuristic that tried to fix up the timestamps
for vblank interrupts that fire a bit too early. Since then the vblank
timestamp code would treat some vblank interrupts as spurious since the
scanline counter would indicate that vblank_start wasn't reached yet.
That in turn lead to incorrect vblank event sequence numbers being
reported to userspace, which lead to unsteady framerate in applications
such as XBMC which uses them for timing purposes.

v2: Remeber to call ilk_pipe_in_vblank_locked() on HSW too (Mika)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75725
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9fec711..d4c952d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -702,7 +702,28 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		else
 			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
-		if (HAS_PCH_SPLIT(dev)) {
+		if (HAS_DDI(dev)) {
+			/*
+			 * On HSW HDMI outputs there seems to be a 2 line
+			 * difference, whereas eDP has the normal 1 line
+			 * difference that earlier platforms have. External
+			 * DP is unknown. For now just check for the 2 line
+			 * difference case on all output types on HSW+.
+			 *
+			 * This might misinterpret the scanline counter being
+			 * one line too far along on eDP, but that's less
+			 * dangerous than the alternative since that would lead
+			 * the vblank timestamp code astray when it sees a
+			 * scanline count before vblank_start during a vblank
+			 * interrupt.
+			 */
+			in_vbl = ilk_pipe_in_vblank_locked(dev, pipe);
+			if ((in_vbl && (position == vbl_start - 2 ||
+					position == vbl_start - 1)) ||
+			    (!in_vbl && (position == vbl_end - 2 ||
+					 position == vbl_end - 1)))
+				position = (position + 2) % vtotal;
+		} else if (HAS_PCH_SPLIT(dev)) {
 			/*
 			 * The scanline counter increments at the leading edge
 			 * of hsync, ie. it completely misses the active portion
-- 
1.8.3.2

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

[PATCH 2/2] drm/i915: Fix scanline counter fixup on BDW

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The display interrupts changed on BDW, so the current ILK-HSW specific
code in ilk_pipe_in_vblank_locked() doesn't work there. Add the required
bits for BDW, and while at it, change the existing code to use nicer
looking vblank status bit macros.

Also remove the now stale __raw_i915_read16() definition which was
left over from the failed gen2 ISR experiment.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73962
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d4c952d..ec9b8a4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -618,33 +618,25 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
 
 /* raw reads, only for fast reads of display block, no need for forcewake etc. */
 #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
-#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
 
 static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t status;
-
-	if (INTEL_INFO(dev)->gen < 7) {
-		status = pipe == PIPE_A ?
-			DE_PIPEA_VBLANK :
-			DE_PIPEB_VBLANK;
+	int reg;
+
+	if (INTEL_INFO(dev)->gen >= 8) {
+		status = GEN8_PIPE_VBLANK;
+		reg = GEN8_DE_PIPE_ISR(pipe);
+	} else if (INTEL_INFO(dev)->gen >= 7) {
+		status = DE_PIPE_VBLANK_IVB(pipe);
+		reg = DEISR;
 	} else {
-		switch (pipe) {
-		default:
-		case PIPE_A:
-			status = DE_PIPEA_VBLANK_IVB;
-			break;
-		case PIPE_B:
-			status = DE_PIPEB_VBLANK_IVB;
-			break;
-		case PIPE_C:
-			status = DE_PIPEC_VBLANK_IVB;
-			break;
-		}
+		status = DE_PIPE_VBLANK(pipe);
+		reg = DEISR;
 	}
 
-	return __raw_i915_read32(dev_priv, DEISR) & status;
+	return __raw_i915_read32(dev_priv, reg) & status;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-- 
1.8.3.2

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

Re: [PATCH 2/2] drm/i915: Fix scanline counter fixup on BDW

[Daniel Vetter]

On Tue, Mar 11, 2014 at 12:58:46PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The display interrupts changed on BDW, so the current ILK-HSW specific
> code in ilk_pipe_in_vblank_locked() doesn't work there. Add the required
> bits for BDW, and while at it, change the existing code to use nicer
> looking vblank status bit macros.
> 
> Also remove the now stale __raw_i915_read16() definition which was
> left over from the failed gen2 ISR experiment.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73962
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d4c952d..ec9b8a4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -618,33 +618,25 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
>  
>  /* raw reads, only for fast reads of display block, no need for forcewake etc. */

fast reads in context with mmio transactions sounds like a fairly crazy
oxymoron. And our I915_READ functions should already dtrt wrt not doing
the forcewake dance for display registers, otherwise I'll consider it a
bug.

Care to throw a patch on top to remove these guys completely?

Thanks, Daniel

>  #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
> -#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
>  
>  static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t status;
> -
> -	if (INTEL_INFO(dev)->gen < 7) {
> -		status = pipe == PIPE_A ?
> -			DE_PIPEA_VBLANK :
> -			DE_PIPEB_VBLANK;
> +	int reg;
> +
> +	if (INTEL_INFO(dev)->gen >= 8) {
> +		status = GEN8_PIPE_VBLANK;
> +		reg = GEN8_DE_PIPE_ISR(pipe);
> +	} else if (INTEL_INFO(dev)->gen >= 7) {
> +		status = DE_PIPE_VBLANK_IVB(pipe);
> +		reg = DEISR;
>  	} else {
> -		switch (pipe) {
> -		default:
> -		case PIPE_A:
> -			status = DE_PIPEA_VBLANK_IVB;
> -			break;
> -		case PIPE_B:
> -			status = DE_PIPEB_VBLANK_IVB;
> -			break;
> -		case PIPE_C:
> -			status = DE_PIPEC_VBLANK_IVB;
> -			break;
> -		}
> +		status = DE_PIPE_VBLANK(pipe);
> +		reg = DEISR;
>  	}
>  
> -	return __raw_i915_read32(dev_priv, DEISR) & status;
> +	return __raw_i915_read32(dev_priv, reg) & status;
>  }
>  
>  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Fix scanline counter fixup on BDW

[Ville Syrjälä]

On Tue, Mar 11, 2014 at 12:38:10PM +0100, Daniel Vetter wrote:
> On Tue, Mar 11, 2014 at 12:58:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The display interrupts changed on BDW, so the current ILK-HSW specific
> > code in ilk_pipe_in_vblank_locked() doesn't work there. Add the required
> > bits for BDW, and while at it, change the existing code to use nicer
> > looking vblank status bit macros.
> > 
> > Also remove the now stale __raw_i915_read16() definition which was
> > left over from the failed gen2 ISR experiment.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73962
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++-------------------
> >  1 file changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index d4c952d..ec9b8a4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -618,33 +618,25 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
> >  
> >  /* raw reads, only for fast reads of display block, no need for forcewake etc. */
> 
> fast reads in context with mmio transactions sounds like a fairly crazy
> oxymoron. And our I915_READ functions should already dtrt wrt not doing
> the forcewake dance for display registers, otherwise I'll consider it a
> bug.
> 
> Care to throw a patch on top to remove these guys completely?

We could have other junk in there like unclaimed register access
checks which could slow this down a bit. Although it seems we
don't do unclaimed register checks for reads currently.

I'm going to kill the ISR read anyway in the atomic sprite series,
so that'll get us back to just one mmio read per call, and so even
if we did a few extra mmio accesses for unclaimed register access
checks, it doesn't feel too expensive to me.

So yes, I could throw on an extra patch to do what you ask, but I'd
rather do that after the atomic sprite series is applied. These
patches are meant for -fixes anyway, so I want to keep them fairly
minimal.

> Thanks, Daniel
> 
> >  #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
> > -#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
> >  
> >  static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t status;
> > -
> > -	if (INTEL_INFO(dev)->gen < 7) {
> > -		status = pipe == PIPE_A ?
> > -			DE_PIPEA_VBLANK :
> > -			DE_PIPEB_VBLANK;
> > +	int reg;
> > +
> > +	if (INTEL_INFO(dev)->gen >= 8) {
> > +		status = GEN8_PIPE_VBLANK;
> > +		reg = GEN8_DE_PIPE_ISR(pipe);
> > +	} else if (INTEL_INFO(dev)->gen >= 7) {
> > +		status = DE_PIPE_VBLANK_IVB(pipe);
> > +		reg = DEISR;
> >  	} else {
> > -		switch (pipe) {
> > -		default:
> > -		case PIPE_A:
> > -			status = DE_PIPEA_VBLANK_IVB;
> > -			break;
> > -		case PIPE_B:
> > -			status = DE_PIPEB_VBLANK_IVB;
> > -			break;
> > -		case PIPE_C:
> > -			status = DE_PIPEC_VBLANK_IVB;
> > -			break;
> > -		}
> > +		status = DE_PIPE_VBLANK(pipe);
> > +		reg = DEISR;
> >  	}
> >  
> > -	return __raw_i915_read32(dev_priv, DEISR) & status;
> > +	return __raw_i915_read32(dev_priv, reg) & status;
> >  }
> >  
> >  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> > -- 
> > 1.8.3.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Fix scanline counter fixup on BDW

[Mika Kuoppala]

ville.syrjala@linux.intel.com writes:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The display interrupts changed on BDW, so the current ILK-HSW specific
> code in ilk_pipe_in_vblank_locked() doesn't work there. Add the required
> bits for BDW, and while at it, change the existing code to use nicer
> looking vblank status bit macros.
>
> Also remove the now stale __raw_i915_read16() definition which was
> left over from the failed gen2 ISR experiment.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73962
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Both patches,
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

With regards to changing raw reads to I915_READ:
I would rather keep the raw_reads here to
keep the same locking behaviour, for now. Referring to this
comment in i915_get_crtc_scanoutpos:

/*
 * Lock uncore.lock, as we will do multiple timing critical raw
 * register reads, potentially with preemption disabled, so the
 * following code must not block on uncore.lock.
 */

-Mika

>  drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d4c952d..ec9b8a4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -618,33 +618,25 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
>  
>  /* raw reads, only for fast reads of display block, no need for forcewake etc. */
>  #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
> -#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
>  
>  static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t status;
> -
> -	if (INTEL_INFO(dev)->gen < 7) {
> -		status = pipe == PIPE_A ?
> -			DE_PIPEA_VBLANK :
> -			DE_PIPEB_VBLANK;
> +	int reg;
> +
> +	if (INTEL_INFO(dev)->gen >= 8) {
> +		status = GEN8_PIPE_VBLANK;
> +		reg = GEN8_DE_PIPE_ISR(pipe);
> +	} else if (INTEL_INFO(dev)->gen >= 7) {
> +		status = DE_PIPE_VBLANK_IVB(pipe);
> +		reg = DEISR;
>  	} else {
> -		switch (pipe) {
> -		default:
> -		case PIPE_A:
> -			status = DE_PIPEA_VBLANK_IVB;
> -			break;
> -		case PIPE_B:
> -			status = DE_PIPEB_VBLANK_IVB;
> -			break;
> -		case PIPE_C:
> -			status = DE_PIPEC_VBLANK_IVB;
> -			break;
> -		}
> +		status = DE_PIPE_VBLANK(pipe);
> +		reg = DEISR;
>  	}
>  
> -	return __raw_i915_read32(dev_priv, DEISR) & status;
> +	return __raw_i915_read32(dev_priv, reg) & status;
>  }
>  
>  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> -- 
> 1.8.3.2
>
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915: Fix scanline counter fixup on BDW

[Jani Nikula]

On Tue, 11 Mar 2014, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> ville.syrjala@linux.intel.com writes:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> The display interrupts changed on BDW, so the current ILK-HSW specific
>> code in ilk_pipe_in_vblank_locked() doesn't work there. Add the required
>> bits for BDW, and while at it, change the existing code to use nicer
>> looking vblank status bit macros.
>>
>> Also remove the now stale __raw_i915_read16() definition which was
>> left over from the failed gen2 ISR experiment.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73962
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Both patches,
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Both pushed to -fixes, thanks for the patches and review.

Jani.



>
> With regards to changing raw reads to I915_READ:
> I would rather keep the raw_reads here to
> keep the same locking behaviour, for now. Referring to this
> comment in i915_get_crtc_scanoutpos:
>
> /*
>  * Lock uncore.lock, as we will do multiple timing critical raw
>  * register reads, potentially with preemption disabled, so the
>  * following code must not block on uncore.lock.
>  */
>
> -Mika
>
>>  drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++-------------------
>>  1 file changed, 11 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index d4c952d..ec9b8a4 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -618,33 +618,25 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
>>  
>>  /* raw reads, only for fast reads of display block, no need for forcewake etc. */
>>  #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
>> -#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
>>  
>>  static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	uint32_t status;
>> -
>> -	if (INTEL_INFO(dev)->gen < 7) {
>> -		status = pipe == PIPE_A ?
>> -			DE_PIPEA_VBLANK :
>> -			DE_PIPEB_VBLANK;
>> +	int reg;
>> +
>> +	if (INTEL_INFO(dev)->gen >= 8) {
>> +		status = GEN8_PIPE_VBLANK;
>> +		reg = GEN8_DE_PIPE_ISR(pipe);
>> +	} else if (INTEL_INFO(dev)->gen >= 7) {
>> +		status = DE_PIPE_VBLANK_IVB(pipe);
>> +		reg = DEISR;
>>  	} else {
>> -		switch (pipe) {
>> -		default:
>> -		case PIPE_A:
>> -			status = DE_PIPEA_VBLANK_IVB;
>> -			break;
>> -		case PIPE_B:
>> -			status = DE_PIPEB_VBLANK_IVB;
>> -			break;
>> -		case PIPE_C:
>> -			status = DE_PIPEC_VBLANK_IVB;
>> -			break;
>> -		}
>> +		status = DE_PIPE_VBLANK(pipe);
>> +		reg = DEISR;
>>  	}
>>  
>> -	return __raw_i915_read32(dev_priv, DEISR) & status;
>> +	return __raw_i915_read32(dev_priv, reg) & status;
>>  }
>>  
>>  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
>> -- 
>> 1.8.3.2
>>
>> _______________________________________________
>> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx