[PATCH 1/7] drm/i915: Optimize pipe irq handling on bdw

[PATCH 1/7] drm/i915: Optimize pipe irq handling on bdw

[Daniel Vetter]

We have a per-pipe bit in the master irq control register, so use it.
This allows us to drop the masks for aggregate interrupt bits and be a
bit more explicit in the code. It also removes one indentation level.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++++++---------------------
 drivers/gpu/drm/i915/i915_reg.h |  5 +----
 2 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 54338cf72feb..c04fbbf0acf7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1749,6 +1749,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	u32 master_ctl;
 	irqreturn_t ret = IRQ_NONE;
 	uint32_t tmp = 0;
+	enum pipe pipe;
 
 	atomic_inc(&dev_priv->irq_received);
 
@@ -1777,31 +1778,28 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 		}
 	}
 
-	if (master_ctl & GEN8_DE_IRQS) {
-		int de_ret = 0;
-		int pipe;
-		for_each_pipe(pipe) {
-			uint32_t pipe_iir;
-
-			pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-			if (pipe_iir & GEN8_PIPE_VBLANK)
-				drm_handle_vblank(dev, pipe);
+	for_each_pipe(pipe) {
+		uint32_t pipe_iir;
 
-			if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
-				intel_prepare_page_flip(dev, pipe);
-				intel_finish_page_flip_plane(dev, pipe);
-			}
+		if (!(master_ctl & GEN8_DE_PIPE_IRQ(pipe)))
+			continue;
 
-			if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
-				DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);
+		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
+		if (pipe_iir & GEN8_PIPE_VBLANK)
+			drm_handle_vblank(dev, pipe);
 
-			if (pipe_iir) {
-				de_ret++;
-				ret = IRQ_HANDLED;
-				I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
-			}
+		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
+			intel_prepare_page_flip(dev, pipe);
+			intel_finish_page_flip_plane(dev, pipe);
 		}
-		if (!de_ret)
+
+		if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
+			DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);
+
+		if (pipe_iir) {
+			ret = IRQ_HANDLED;
+			I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
+		} else
 			DRM_ERROR("The master control interrupt lied (DE PIPE)!\n");
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2b9e66c0c6cf..f150edaa64ca 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4031,15 +4031,12 @@
 #define  GEN8_DE_PIPE_C_IRQ		(1<<18)
 #define  GEN8_DE_PIPE_B_IRQ		(1<<17)
 #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
+#define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+pipe))
 #define  GEN8_GT_VECS_IRQ		(1<<6)
 #define  GEN8_GT_VCS2_IRQ		(1<<3)
 #define  GEN8_GT_VCS1_IRQ		(1<<2)
 #define  GEN8_GT_BCS_IRQ		(1<<1)
 #define  GEN8_GT_RCS_IRQ		(1<<0)
-/* Lazy definition */
-#define  GEN8_GT_IRQS			0x000000ff
-#define  GEN8_DE_IRQS			0x01ff0000
-#define  GEN8_RSVD_IRQS			0xB700ff00
 
 #define GEN8_GT_ISR(which) (0x44300 + (0x10 * (which)))
 #define GEN8_GT_IMR(which) (0x44304 + (0x10 * (which)))
-- 
1.8.4.rc3

[PATCH 2/7] drm/i915: Fix up the bdw pipe interrupt enable lists

[Daniel Vetter]

- Pipe underrun can't just be enabled, we need some support code like
  on ilk-hsw to make this happen. So drop it for now.
- CRC error is a special mode of the CRC hardware that we don't use,
  so again drop it. Real CRC support for bdw will be added later.
- All the other error bits are about faults, so rename the #define and
  adjust the output.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++++----
 drivers/gpu/drm/i915/i915_reg.h |  9 ++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c04fbbf0acf7..370c9281a256 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1793,8 +1793,11 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			intel_finish_page_flip_plane(dev, pipe);
 		}
 
-		if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
-			DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);
+		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
+			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
+				  'A' + pipe,
+				  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
+		}
 
 		if (pipe_iir) {
 			ret = IRQ_HANDLED;
@@ -2863,9 +2866,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
-				   GEN8_PIPE_SCAN_LINE_EVENT |
 				   GEN8_PIPE_VBLANK |
-				   GEN8_DE_PIPE_IRQ_ERRORS;
+				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
 	int pipe;
 	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
 	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_enables;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f150edaa64ca..9e7588345017 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4064,11 +4064,10 @@
 #define  GEN8_PIPE_SCAN_LINE_EVENT	(1 << 2)
 #define  GEN8_PIPE_VSYNC		(1 << 1)
 #define  GEN8_PIPE_VBLANK		(1 << 0)
-#define GEN8_DE_PIPE_IRQ_ERRORS	(GEN8_PIPE_UNDERRUN | \
-				 GEN8_PIPE_CDCLK_CRC_ERROR | \
-				 GEN8_PIPE_CURSOR_FAULT | \
-				 GEN8_PIPE_SPRITE_FAULT | \
-				 GEN8_PIPE_PRIMARY_FAULT)
+#define GEN8_DE_PIPE_IRQ_FAULT_ERRORS \
+	(GEN8_PIPE_CURSOR_FAULT | \
+	 GEN8_PIPE_SPRITE_FAULT | \
+	 GEN8_PIPE_PRIMARY_FAULT)
 
 #define GEN8_DE_PORT_ISR 0x44440
 #define GEN8_DE_PORT_IMR 0x44444
-- 
1.8.4.rc3

[PATCH] drm/i915: Fix up the bdw pipe interrupt enable lists

[Daniel Vetter]

- Pipe underrun can't just be enabled, we need some support code like
  on ilk-hsw to make this happen. So drop it for now.
- CRC error is a special mode of the CRC hardware that we don't use,
  so again drop it. Real CRC support for bdw will be added later.
- All the other error bits are about faults, so rename the #define and
  adjust the output.

v2: Use pipe_name as pointed out by Ville. Ville's comment was on a
previous patch, but it was easier to squash in here.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++++----
 drivers/gpu/drm/i915/i915_reg.h |  9 ++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c04fbbf0acf7..e1bfc85d1789 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1793,8 +1793,11 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			intel_finish_page_flip_plane(dev, pipe);
 		}
 
-		if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
-			DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);
+		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
+			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
+				  pipe_name(pipe),
+				  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
+		}
 
 		if (pipe_iir) {
 			ret = IRQ_HANDLED;
@@ -2863,9 +2866,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
-				   GEN8_PIPE_SCAN_LINE_EVENT |
 				   GEN8_PIPE_VBLANK |
-				   GEN8_DE_PIPE_IRQ_ERRORS;
+				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
 	int pipe;
 	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
 	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_enables;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f150edaa64ca..9e7588345017 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4064,11 +4064,10 @@
 #define  GEN8_PIPE_SCAN_LINE_EVENT	(1 << 2)
 #define  GEN8_PIPE_VSYNC		(1 << 1)
 #define  GEN8_PIPE_VBLANK		(1 << 0)
-#define GEN8_DE_PIPE_IRQ_ERRORS	(GEN8_PIPE_UNDERRUN | \
-				 GEN8_PIPE_CDCLK_CRC_ERROR | \
-				 GEN8_PIPE_CURSOR_FAULT | \
-				 GEN8_PIPE_SPRITE_FAULT | \
-				 GEN8_PIPE_PRIMARY_FAULT)
+#define GEN8_DE_PIPE_IRQ_FAULT_ERRORS \
+	(GEN8_PIPE_CURSOR_FAULT | \
+	 GEN8_PIPE_SPRITE_FAULT | \
+	 GEN8_PIPE_PRIMARY_FAULT)
 
 #define GEN8_DE_PORT_ISR 0x44440
 #define GEN8_DE_PORT_IMR 0x44444
-- 
1.8.4.rc3

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

Re: [PATCH] drm/i915: Fix up the bdw pipe interrupt enable lists

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 02:49:24PM +0100, Daniel Vetter wrote:
> - Pipe underrun can't just be enabled, we need some support code like
>   on ilk-hsw to make this happen. So drop it for now.
> - CRC error is a special mode of the CRC hardware that we don't use,
>   so again drop it. Real CRC support for bdw will be added later.
> - All the other error bits are about faults, so rename the #define and
>   adjust the output.
> 
> v2: Use pipe_name as pointed out by Ville. Ville's comment was on a
> previous patch, but it was easier to squash in here.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++----
>  drivers/gpu/drm/i915/i915_reg.h |  9 ++++-----
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c04fbbf0acf7..e1bfc85d1789 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1793,8 +1793,11 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  			intel_finish_page_flip_plane(dev, pipe);
>  		}
>  
> -		if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
> -			DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);
> +		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
> +			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
> +				  pipe_name(pipe),
> +				  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
> +		}
>  
>  		if (pipe_iir) {
>  			ret = IRQ_HANDLED;
> @@ -2863,9 +2866,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
> -				   GEN8_PIPE_SCAN_LINE_EVENT |
>  				   GEN8_PIPE_VBLANK |
> -				   GEN8_DE_PIPE_IRQ_ERRORS;
> +				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
>  	int pipe;
>  	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
>  	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_enables;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f150edaa64ca..9e7588345017 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4064,11 +4064,10 @@
>  #define  GEN8_PIPE_SCAN_LINE_EVENT	(1 << 2)
>  #define  GEN8_PIPE_VSYNC		(1 << 1)
>  #define  GEN8_PIPE_VBLANK		(1 << 0)
> -#define GEN8_DE_PIPE_IRQ_ERRORS	(GEN8_PIPE_UNDERRUN | \
> -				 GEN8_PIPE_CDCLK_CRC_ERROR | \
> -				 GEN8_PIPE_CURSOR_FAULT | \
> -				 GEN8_PIPE_SPRITE_FAULT | \
> -				 GEN8_PIPE_PRIMARY_FAULT)
> +#define GEN8_DE_PIPE_IRQ_FAULT_ERRORS \
> +	(GEN8_PIPE_CURSOR_FAULT | \
> +	 GEN8_PIPE_SPRITE_FAULT | \
> +	 GEN8_PIPE_PRIMARY_FAULT)
>  
>  #define GEN8_DE_PORT_ISR 0x44440
>  #define GEN8_DE_PORT_IMR 0x44444
> -- 
> 1.8.4.rc3

-- 
Ville Syrjälä
Intel OTC

[PATCH 3/7] drm/i915: Wire up port A aux channel

[Daniel Vetter]

Useful for dp aux to work better. Also stop enabling the port A
hotplug event - eDP panels are expected to fire that interupt and
we're not really ready to deal with them. This is consistent with how
we handle port A on ilk-hsw.

The more important bit is that we must delay the enabling of hotplug
interrupts until all the encoders are fully set up. But we need irq
support earlier than that, hence hotplug interrupts can only be
enabled in the ->hpd_irq_setup callback.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 19 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h |  3 ++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 370c9281a256..686afe0a98f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1778,6 +1778,21 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 		}
 	}
 
+	if (master_ctl & GEN8_DE_PORT_IRQ) {
+		tmp = I915_READ(GEN8_DE_PORT_IIR);
+		if (tmp & GEN8_AUX_CHANNEL_A_HOTPLUG)
+			dp_aux_irq_handler(dev);
+		else if (tmp)
+			DRM_ERROR("Unexpected DE Port interrupt\n");
+		else
+			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
+
+		if (tmp) {
+			I915_WRITE(GEN8_DE_PORT_IIR, tmp);
+			ret = IRQ_HANDLED;
+		}
+	}
+
 	for_each_pipe(pipe) {
 		uint32_t pipe_iir;
 
@@ -2883,8 +2898,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	}
 	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
-	I915_WRITE(GEN8_DE_PORT_IMR, ~_PORT_DP_A_HOTPLUG);
-	I915_WRITE(GEN8_DE_PORT_IER, _PORT_DP_A_HOTPLUG);
+	I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A_HOTPLUG);
+	I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A_HOTPLUG);
 	POSTING_READ(GEN8_DE_PORT_IER);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9e7588345017..be0d98ae97f7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4073,7 +4073,8 @@
 #define GEN8_DE_PORT_IMR 0x44444
 #define GEN8_DE_PORT_IIR 0x44448
 #define GEN8_DE_PORT_IER 0x4444c
-#define  _PORT_DP_A_HOTPLUG		(1 << 3)
+#define  GEN8_PORT_DP_A_HOTPLUG		(1 << 3)
+#define  GEN8_AUX_CHANNEL_A_HOTPLUG	(1 << 0)
 
 #define GEN8_DE_MISC_ISR 0x44460
 #define GEN8_DE_MISC_IMR 0x44464
-- 
1.8.4.rc3

Re: [PATCH 3/7] drm/i915: Wire up port A aux channel

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 11:05:42AM +0100, Daniel Vetter wrote:
> Useful for dp aux to work better. Also stop enabling the port A
> hotplug event - eDP panels are expected to fire that interupt and
> we're not really ready to deal with them. This is consistent with how
> we handle port A on ilk-hsw.
> 
> The more important bit is that we must delay the enabling of hotplug
> interrupts until all the encoders are fully set up. But we need irq
> support earlier than that, hence hotplug interrupts can only be
> enabled in the ->hpd_irq_setup callback.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 19 +++++++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h |  3 ++-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 370c9281a256..686afe0a98f1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1778,6 +1778,21 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  		}
>  	}
>  
> +	if (master_ctl & GEN8_DE_PORT_IRQ) {
> +		tmp = I915_READ(GEN8_DE_PORT_IIR);
> +		if (tmp & GEN8_AUX_CHANNEL_A_HOTPLUG)
> +			dp_aux_irq_handler(dev);
> +		else if (tmp)
> +			DRM_ERROR("Unexpected DE Port interrupt\n");
> +		else
> +			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
> +
> +		if (tmp) {
> +			I915_WRITE(GEN8_DE_PORT_IIR, tmp);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
>  	for_each_pipe(pipe) {
>  		uint32_t pipe_iir;
>  
> @@ -2883,8 +2898,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	}
>  	POSTING_READ(GEN8_DE_PIPE_ISR(0));
>  
> -	I915_WRITE(GEN8_DE_PORT_IMR, ~_PORT_DP_A_HOTPLUG);
> -	I915_WRITE(GEN8_DE_PORT_IER, _PORT_DP_A_HOTPLUG);
> +	I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A_HOTPLUG);
> +	I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A_HOTPLUG);
>  	POSTING_READ(GEN8_DE_PORT_IER);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9e7588345017..be0d98ae97f7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4073,7 +4073,8 @@
>  #define GEN8_DE_PORT_IMR 0x44444
>  #define GEN8_DE_PORT_IIR 0x44448
>  #define GEN8_DE_PORT_IER 0x4444c
> -#define  _PORT_DP_A_HOTPLUG		(1 << 3)
> +#define  GEN8_PORT_DP_A_HOTPLUG		(1 << 3)
> +#define  GEN8_AUX_CHANNEL_A_HOTPLUG	(1 << 0)
                              ^^^^^^^^

That's not a hotplug interrupt.

Otherwise looks good:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  #define GEN8_DE_MISC_ISR 0x44460
>  #define GEN8_DE_MISC_IMR 0x44464
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm/i915: Wire up port A aux channel

[Daniel Vetter]

Useful for dp aux to work better. Also stop enabling the port A
hotplug event - eDP panels are expected to fire that interupt and
we're not really ready to deal with them. This is consistent with how
we handle port A on ilk-hsw.

The more important bit is that we must delay the enabling of hotplug
interrupts until all the encoders are fully set up. But we need irq
support earlier than that, hence hotplug interrupts can only be
enabled in the ->hpd_irq_setup callback.

v2: Drop the _HOTPLUG, it isn't (Ville).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 19 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h |  3 ++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e1bfc85d1789..9304ce3e4649 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1778,6 +1778,21 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 		}
 	}
 
+	if (master_ctl & GEN8_DE_PORT_IRQ) {
+		tmp = I915_READ(GEN8_DE_PORT_IIR);
+		if (tmp & GEN8_AUX_CHANNEL_A)
+			dp_aux_irq_handler(dev);
+		else if (tmp)
+			DRM_ERROR("Unexpected DE Port interrupt\n");
+		else
+			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
+
+		if (tmp) {
+			I915_WRITE(GEN8_DE_PORT_IIR, tmp);
+			ret = IRQ_HANDLED;
+		}
+	}
+
 	for_each_pipe(pipe) {
 		uint32_t pipe_iir;
 
@@ -2883,8 +2898,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	}
 	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
-	I915_WRITE(GEN8_DE_PORT_IMR, ~_PORT_DP_A_HOTPLUG);
-	I915_WRITE(GEN8_DE_PORT_IER, _PORT_DP_A_HOTPLUG);
+	I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A);
+	I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A);
 	POSTING_READ(GEN8_DE_PORT_IER);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9e7588345017..fe8cb4cc0296 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4073,7 +4073,8 @@
 #define GEN8_DE_PORT_IMR 0x44444
 #define GEN8_DE_PORT_IIR 0x44448
 #define GEN8_DE_PORT_IER 0x4444c
-#define  _PORT_DP_A_HOTPLUG		(1 << 3)
+#define  GEN8_PORT_DP_A_HOTPLUG		(1 << 3)
+#define  GEN8_AUX_CHANNEL_A		(1 << 0)
 
 #define GEN8_DE_MISC_ISR 0x44460
 #define GEN8_DE_MISC_IMR 0x44464
-- 
1.8.4.rc3

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

Re: [PATCH] drm/i915: Wire up port A aux channel

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 02:49:55PM +0100, Daniel Vetter wrote:
> Useful for dp aux to work better. Also stop enabling the port A
> hotplug event - eDP panels are expected to fire that interupt and
> we're not really ready to deal with them. This is consistent with how
> we handle port A on ilk-hsw.
> 
> The more important bit is that we must delay the enabling of hotplug
> interrupts until all the encoders are fully set up. But we need irq
> support earlier than that, hence hotplug interrupts can only be
> enabled in the ->hpd_irq_setup callback.
> 
> v2: Drop the _HOTPLUG, it isn't (Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 19 +++++++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h |  3 ++-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e1bfc85d1789..9304ce3e4649 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1778,6 +1778,21 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  		}
>  	}
>  
> +	if (master_ctl & GEN8_DE_PORT_IRQ) {
> +		tmp = I915_READ(GEN8_DE_PORT_IIR);
> +		if (tmp & GEN8_AUX_CHANNEL_A)
> +			dp_aux_irq_handler(dev);
> +		else if (tmp)
> +			DRM_ERROR("Unexpected DE Port interrupt\n");
> +		else
> +			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
> +
> +		if (tmp) {
> +			I915_WRITE(GEN8_DE_PORT_IIR, tmp);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
>  	for_each_pipe(pipe) {
>  		uint32_t pipe_iir;
>  
> @@ -2883,8 +2898,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	}
>  	POSTING_READ(GEN8_DE_PIPE_ISR(0));
>  
> -	I915_WRITE(GEN8_DE_PORT_IMR, ~_PORT_DP_A_HOTPLUG);
> -	I915_WRITE(GEN8_DE_PORT_IER, _PORT_DP_A_HOTPLUG);
> +	I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A);
> +	I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A);
>  	POSTING_READ(GEN8_DE_PORT_IER);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9e7588345017..fe8cb4cc0296 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4073,7 +4073,8 @@
>  #define GEN8_DE_PORT_IMR 0x44444
>  #define GEN8_DE_PORT_IIR 0x44448
>  #define GEN8_DE_PORT_IER 0x4444c
> -#define  _PORT_DP_A_HOTPLUG		(1 << 3)
> +#define  GEN8_PORT_DP_A_HOTPLUG		(1 << 3)
> +#define  GEN8_AUX_CHANNEL_A		(1 << 0)
>  
>  #define GEN8_DE_MISC_ISR 0x44460
>  #define GEN8_DE_MISC_IMR 0x44464
> -- 
> 1.8.4.rc3

-- 
Ville Syrjälä
Intel OTC

[PATCH 4/7] drm/i915: Wire up PCH interrupts for bdw

[Daniel Vetter]

Gives us hotplug, gmbus, dp aux and south errors (underrun
reporting!).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 686afe0a98f1..510a16a96393 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1821,6 +1821,22 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			DRM_ERROR("The master control interrupt lied (DE PIPE)!\n");
 	}
 
+	if (!HAS_PCH_NOP(dev) && master_ctl & GEN8_DE_PCH_IRQ) {
+		/*
+		 * FIXME(BDW): Assume for now that the new interrupt handling
+		 * scheme also closed the SDE interrupt handling race we've seen
+		 * on older pch-split platforms. But this needs testing.
+		 */
+		u32 pch_iir = I915_READ(SDEIIR);
+
+		cpt_irq_handler(dev, pch_iir);
+
+		if (pch_iir) {
+			I915_WRITE(SDEIIR, pch_iir);
+			ret = IRQ_HANDLED;
+		}
+	}
+
 	I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-- 
1.8.4.rc3

[PATCH 5/7] drm/i915: Wire up pipe CRC support for bdw

[Daniel Vetter]

The layout of the CRC registers is the same as on hsw, only the
interrupt handling has changed a bit. So trivial to wire up, yay!

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 510a16a96393..d2d678f72486 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1808,6 +1808,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			intel_finish_page_flip_plane(dev, pipe);
 		}
 
+		if (pipe_iir & GEN8_PIPE_CDCLK_CRC_DONE)
+			hsw_pipe_crc_irq_handler(dev, pipe);
+
 		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
 			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
 				  'A' + pipe,
@@ -2898,6 +2901,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
 				   GEN8_PIPE_VBLANK |
+				   GEN8_PIPE_CDCLK_CRC_DONE |
 				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
 	int pipe;
 	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
-- 
1.8.4.rc3

[PATCH 6/7] drm/i915: Optimize gen8_enable|disable_vblank functions

[Daniel Vetter]

Let's cache the IMR value like on other platforms. This is needed to
implement the underrun reporting since then we'll have two places that
change the same register at runtime.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d2d678f72486..51966feee5d2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2203,17 +2203,14 @@ static int gen8_enable_vblank(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
-	uint32_t imr;
 
 	if (!i915_pipe_enabled(dev, pipe))
 		return -EINVAL;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	imr = I915_READ(GEN8_DE_PIPE_IMR(pipe));
-	if ((imr & GEN8_PIPE_VBLANK) == 1) {
-		I915_WRITE(GEN8_DE_PIPE_IMR(pipe), imr & ~GEN8_PIPE_VBLANK);
-		POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
-	}
+	dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_VBLANK;
+	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
+	POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 	return 0;
 }
@@ -2270,17 +2267,14 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
-	uint32_t imr;
 
 	if (!i915_pipe_enabled(dev, pipe))
 		return;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	imr = I915_READ(GEN8_DE_PIPE_IMR(pipe));
-	if ((imr & GEN8_PIPE_VBLANK) == 0) {
-		I915_WRITE(GEN8_DE_PIPE_IMR(pipe), imr | GEN8_PIPE_VBLANK);
-		POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
-	}
+	dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_VBLANK;
+	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
+	POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-- 
1.8.4.rc3

Re: [PATCH 6/7] drm/i915: Optimize gen8_enable|disable_vblank functions

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 11:05:45AM +0100, Daniel Vetter wrote:
> Let's cache the IMR value like on other platforms. This is needed to
> implement the underrun reporting since then we'll have two places that
> change the same register at runtime.

This looks OK, so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

But I think gen8_de_irq_postinstall() isn't quite right. It'll already
enable and unmask the vblank irqs, even though it should just enable
them, but leave them masked.

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d2d678f72486..51966feee5d2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2203,17 +2203,14 @@ static int gen8_enable_vblank(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
> -	uint32_t imr;
>  
>  	if (!i915_pipe_enabled(dev, pipe))
>  		return -EINVAL;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	imr = I915_READ(GEN8_DE_PIPE_IMR(pipe));
> -	if ((imr & GEN8_PIPE_VBLANK) == 1) {
> -		I915_WRITE(GEN8_DE_PIPE_IMR(pipe), imr & ~GEN8_PIPE_VBLANK);
> -		POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
> -	}
> +	dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_VBLANK;
> +	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> +	POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  	return 0;
>  }
> @@ -2270,17 +2267,14 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
> -	uint32_t imr;
>  
>  	if (!i915_pipe_enabled(dev, pipe))
>  		return;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	imr = I915_READ(GEN8_DE_PIPE_IMR(pipe));
> -	if ((imr & GEN8_PIPE_VBLANK) == 0) {
> -		I915_WRITE(GEN8_DE_PIPE_IMR(pipe), imr | GEN8_PIPE_VBLANK);
> -		POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
> -	}
> +	dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_VBLANK;
> +	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> +	POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/2] drm/i915: Mask the vblank interrupt on bdw by default

[Daniel Vetter]

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bf71e352fd74..1ce5722c2462 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2917,15 +2917,15 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
-				   GEN8_PIPE_VBLANK |
-				   GEN8_PIPE_CDCLK_CRC_DONE |
-				   GEN8_PIPE_FIFO_UNDERRUN |
-				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
+	uint32_t de_pipe_masked = GEN8_PIPE_FLIP_DONE |
+		GEN8_PIPE_CDCLK_CRC_DONE |
+		GEN8_PIPE_FIFO_UNDERRUN |
+		GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
+	uint32_t de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK;
 	int pipe;
-	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
-	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_enables;
-	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_enables;
+	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_masked;
+	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_masked;
+	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
 
 	for_each_pipe(pipe) {
 		u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-- 
1.8.4.rc3

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

[PATCH 2/2] drm/i915/bdw: Take render error interrupt out of the mask

[Daniel Vetter]

The handling of the error interrupts isn't wired up at all. And it
hasn't been ever since ilk happened, so don't bother.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e32c08af8e8a..b620337e6d67 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2132,8 +2132,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 
 	if (INTEL_INFO(dev)->gen >= 8) {
 		ring->irq_enable_mask =
-			(GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT) |
-			GT_RENDER_CS_MASTER_ERROR_INTERRUPT;
+			GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
 		ring->irq_get = gen8_ring_get_irq;
 		ring->irq_put = gen8_ring_put_irq;
 		ring->dispatch_execbuffer = gen8_ring_dispatch_execbuffer;
-- 
1.8.4.rc3

Re: [PATCH 1/2] drm/i915: Mask the vblank interrupt on bdw by default

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 03:31:52PM +0100, Daniel Vetter wrote:
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bf71e352fd74..1ce5722c2462 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2917,15 +2917,15 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>  static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> -	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
> -				   GEN8_PIPE_VBLANK |
> -				   GEN8_PIPE_CDCLK_CRC_DONE |
> -				   GEN8_PIPE_FIFO_UNDERRUN |
> -				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
> +	uint32_t de_pipe_masked = GEN8_PIPE_FLIP_DONE |
> +		GEN8_PIPE_CDCLK_CRC_DONE |
> +		GEN8_PIPE_FIFO_UNDERRUN |
> +		GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
> +	uint32_t de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK;
>  	int pipe;
> -	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
> -	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_enables;
> -	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_enables;
> +	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_masked;
> +	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_masked;
> +	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
>  
>  	for_each_pipe(pipe) {
>  		u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> -- 
> 1.8.4.rc3

-- 
Ville Syrjälä
Intel OTC

[PATCH 7/7] drm/i915: Wire up cpu fifo underrun reporting support for bdw

[Daniel Vetter]

HW engineers have listened and given us again a real interrupt with
masking and status regs. Yay!

For consistency with other platforms call the #define FIFO_UNDERRUN.
Eventually we also might need to have some enable/disable functions
for bdw display interrupts, but for now open-coding seems to be good
enough.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |  2 +-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 51966feee5d2..d4687a8d75ec 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -270,6 +270,21 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 	}
 }
 
+static void broadwell_set_fifo_underrun_reporting(struct drm_device *dev,
+						  enum pipe pipe, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if (enable)
+		dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_FIFO_UNDERRUN;
+	else
+		dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_FIFO_UNDERRUN;
+	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
+	POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
+}
+
 /**
  * ibx_display_interrupt_update - update SDEIMR
  * @dev_priv: driver private
@@ -382,6 +397,8 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 		ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
 	else if (IS_GEN7(dev))
 		ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
+	else if (IS_GEN8(dev))
+		broadwell_set_fifo_underrun_reporting(dev, pipe, enable);
 
 done:
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
@@ -1811,6 +1828,13 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 		if (pipe_iir & GEN8_PIPE_CDCLK_CRC_DONE)
 			hsw_pipe_crc_irq_handler(dev, pipe);
 
+		if (pipe_iir & GEN8_PIPE_FIFO_UNDERRUN) {
+			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
+								  false))
+				DRM_DEBUG_DRIVER("Pipe %c FIFO underrun\n",
+						 pipe_name(pipe));
+		}
+
 		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
 			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
 				  'A' + pipe,
@@ -2896,6 +2920,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	uint32_t de_pipe_enables = GEN8_PIPE_FLIP_DONE |
 				   GEN8_PIPE_VBLANK |
 				   GEN8_PIPE_CDCLK_CRC_DONE |
+				   GEN8_PIPE_FIFO_UNDERRUN |
 				   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
 	int pipe;
 	dev_priv->de_irq_mask[PIPE_A] = ~de_pipe_enables;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index be0d98ae97f7..28096be8d8dc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4053,7 +4053,7 @@
 #define GEN8_DE_PIPE_IMR(pipe) (0x44404 + (0x10 * (pipe)))
 #define GEN8_DE_PIPE_IIR(pipe) (0x44408 + (0x10 * (pipe)))
 #define GEN8_DE_PIPE_IER(pipe) (0x4440c + (0x10 * (pipe)))
-#define  GEN8_PIPE_UNDERRUN		(1 << 31)
+#define  GEN8_PIPE_FIFO_UNDERRUN	(1 << 31)
 #define  GEN8_PIPE_CDCLK_CRC_ERROR	(1 << 29)
 #define  GEN8_PIPE_CDCLK_CRC_DONE	(1 << 28)
 #define  GEN8_PIPE_CURSOR_FAULT		(1 << 10)
-- 
1.8.4.rc3

Re: [PATCH 1/7] drm/i915: Optimize pipe irq handling on bdw

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 11:05:40AM +0100, Daniel Vetter wrote:
> We have a per-pipe bit in the master irq control register, so use it.
> This allows us to drop the masks for aggregate interrupt bits and be a
> bit more explicit in the code. It also removes one indentation level.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++++++---------------------
>  drivers/gpu/drm/i915/i915_reg.h |  5 +----
>  2 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 54338cf72feb..c04fbbf0acf7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1749,6 +1749,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  	u32 master_ctl;
>  	irqreturn_t ret = IRQ_NONE;
>  	uint32_t tmp = 0;
> +	enum pipe pipe;
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> @@ -1777,31 +1778,28 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  		}
>  	}
>  
> -	if (master_ctl & GEN8_DE_IRQS) {
> -		int de_ret = 0;
> -		int pipe;
> -		for_each_pipe(pipe) {
> -			uint32_t pipe_iir;
> -
> -			pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> -			if (pipe_iir & GEN8_PIPE_VBLANK)
> -				drm_handle_vblank(dev, pipe);
> +	for_each_pipe(pipe) {
> +		uint32_t pipe_iir;
>  
> -			if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> -				intel_prepare_page_flip(dev, pipe);
> -				intel_finish_page_flip_plane(dev, pipe);
> -			}
> +		if (!(master_ctl & GEN8_DE_PIPE_IRQ(pipe)))
> +			continue;
>  
> -			if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
> -				DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);
> +		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> +		if (pipe_iir & GEN8_PIPE_VBLANK)
> +			drm_handle_vblank(dev, pipe);
>  
> -			if (pipe_iir) {
> -				de_ret++;
> -				ret = IRQ_HANDLED;
> -				I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
> -			}
> +		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> +			intel_prepare_page_flip(dev, pipe);
> +			intel_finish_page_flip_plane(dev, pipe);
>  		}
> -		if (!de_ret)
> +
> +		if (pipe_iir & GEN8_DE_PIPE_IRQ_ERRORS)
> +			DRM_ERROR("Errors on pipe %c\n", 'A' + pipe);

Could use pipe_name(). Otherwise looks good.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +		if (pipe_iir) {
> +			ret = IRQ_HANDLED;
> +			I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
> +		} else
>  			DRM_ERROR("The master control interrupt lied (DE PIPE)!\n");
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2b9e66c0c6cf..f150edaa64ca 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4031,15 +4031,12 @@
>  #define  GEN8_DE_PIPE_C_IRQ		(1<<18)
>  #define  GEN8_DE_PIPE_B_IRQ		(1<<17)
>  #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
> +#define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+pipe))
>  #define  GEN8_GT_VECS_IRQ		(1<<6)
>  #define  GEN8_GT_VCS2_IRQ		(1<<3)
>  #define  GEN8_GT_VCS1_IRQ		(1<<2)
>  #define  GEN8_GT_BCS_IRQ		(1<<1)
>  #define  GEN8_GT_RCS_IRQ		(1<<0)
> -/* Lazy definition */
> -#define  GEN8_GT_IRQS			0x000000ff
> -#define  GEN8_DE_IRQS			0x01ff0000
> -#define  GEN8_RSVD_IRQS			0xB700ff00
>  
>  #define GEN8_GT_ISR(which) (0x44300 + (0x10 * (which)))
>  #define GEN8_GT_IMR(which) (0x44304 + (0x10 * (which)))
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/7] drm/i915: Optimize pipe irq handling on bdw

[Ville Syrjälä]

The rest of the patches (2,4,5,7) look fine to me, so:

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/7] drm/i915: Optimize pipe irq handling on bdw

[Daniel Vetter]

On Thu, Nov 07, 2013 at 03:45:16PM +0200, Ville Syrjälä wrote:
> The rest of the patches (2,4,5,7) look fine to me, so:
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pulled them all into bdw-stage1, thanks for review. I've also pulled in a
few of Ben's fixups already (and squash a few of them). On the other I'll
hold off until mailman resurrects (and I'll probably postpone them to the
next bdw pull next week).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] Workaround for flicker with panning on the i830

[Thomas Richter]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

Hi Daniel, dear intel-experts,

please find a revised patch attached that addresses the flicker with 
panning on the i830 chipset. This patch has now been tested
on various screen layouts and seems to be quite reliable, i.e. I haven't 
seen the flicker since.

Unfortunately, I have not been able to find a good workaround for the 
same problem on a tiled framebuffer, the pattern there, i.e. when
the flicker appears, is quite irregular, and it is unclear how to 
address it. The situation is even worse for 8bit/pixel framebuffers where,
for some panning positions, the display remains completely blank. It 
flickers once or twice, then gets a hick-up, and then stays off.

The patch is currently only active on the i830, and only on pipelines 
using the VGA or DVO output. Strangely enough, LVDS does not
seem to be affected, so maybe it is some memory/prefetch related 
problem. I also checked the debug output, though I found no
suspicious activity while the screen flickers or is off. For a linear 
framebuffer, it seems to be enough to position the start of the
pipeline ahead of the desired position, wait one vertical blank, and 
then adjust it to the correct position. For tiling or 8bit modes,
this does not work.

Sorry if the formatting is off. This is just what emacs left me with. 
Please feel free to reformat as you prefer.

As a related question: Is there possibly a command line tool that would 
allow me to modify the intel chipset registers on the fly without going 
through a kernel recompile? It would make some experiments just so much 
simpler.

Greetings,
     Thomas


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 2042 bytes --]

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5eb11d..e98298f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2087,8 +2087,44 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 				     i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
 		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
 		I915_WRITE(DSPLINOFF(plane), linear_offset);
-	} else
-		I915_WRITE(DSPADDR(plane), i915_gem_obj_ggtt_offset(obj) + linear_offset);
+	} else if (INTEL_INFO(dev)->gen == 2 && IS_I830(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+	  unsigned long planeadr = i915_gem_obj_ggtt_offset(obj) + linear_offset;
+	  DRM_DEBUG_KMS("Plane address is 0x%lx",planeadr);
+	  // I830 panning flicker work around. Only for non-LVDS output, only for i830.
+	  if (obj->tiling_mode != I915_TILING_NONE) {
+	    if ((planeadr & 0x40)) {
+	      DRM_DEBUG_KMS("Detected potential flicker in tiling mode");
+	      DRM_DEBUG_KMS("No workaround available");
+	      DRM_DEBUG_KMS("Use a linear frame buffer");
+	    }
+	  } else {
+	    switch (fb->pixel_format) {
+	    case DRM_FORMAT_XRGB1555:
+	    case DRM_FORMAT_ARGB1555:
+	    case DRM_FORMAT_RGB565:
+	    case DRM_FORMAT_XRGB8888:
+	    case DRM_FORMAT_ARGB8888:
+	    case DRM_FORMAT_XBGR8888:
+	    case DRM_FORMAT_ABGR8888:
+	      {
+		unsigned long int oldadr = I915_READ(DSPADDR(plane));
+		if (((oldadr ^ planeadr) & 0x40) && (planeadr & 0xc0) == 0xc0) {
+		  DRM_DEBUG_KMS("Detected potential flicker in linear mode");
+		  I915_WRITE(DSPADDR(plane), planeadr & (~(0x80)));
+		  POSTING_READ(reg);
+		  intel_wait_for_vblank(dev,intel_crtc->pipe);
+		}
+	      }
+	      break;
+	    default:
+	      DRM_DEBUG_KMS("No flicker workaround available\n");
+	      break;
+	    }
+	  }
+	  I915_WRITE(DSPADDR(plane), planeadr);
+	} else {
+	  I915_WRITE(DSPADDR(plane), i915_gem_obj_ggtt_offset(obj) + linear_offset);
+	}
 	POSTING_READ(reg);
 
 	return 0;

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] Workaround for flicker with panning on the i830

[Daniel Vetter]

On Fri, Nov 08, 2013 at 04:25:01PM +0100, Thomas Richter wrote:
> Hi Daniel, dear intel-experts,
> 
> please find a revised patch attached that addresses the flicker with
> panning on the i830 chipset. This patch has now been tested
> on various screen layouts and seems to be quite reliable, i.e. I
> haven't seen the flicker since.
> 
> Unfortunately, I have not been able to find a good workaround for
> the same problem on a tiled framebuffer, the pattern there, i.e.
> when
> the flicker appears, is quite irregular, and it is unclear how to
> address it. The situation is even worse for 8bit/pixel framebuffers
> where,
> for some panning positions, the display remains completely blank. It
> flickers once or twice, then gets a hick-up, and then stays off.
> 
> The patch is currently only active on the i830, and only on
> pipelines using the VGA or DVO output. Strangely enough, LVDS does
> not
> seem to be affected, so maybe it is some memory/prefetch related
> problem. I also checked the debug output, though I found no
> suspicious activity while the screen flickers or is off. For a
> linear framebuffer, it seems to be enough to position the start of
> the
> pipeline ahead of the desired position, wait one vertical blank, and
> then adjust it to the correct position. For tiling or 8bit modes,
> this does not work.
> 
> Sorry if the formatting is off. This is just what emacs left me
> with. Please feel free to reformat as you prefer.

Kernel has a tool in scripts/checkpatch.pl which will tell you what's all
off ;-) Also sob line and similar essential things are missing, but the
script should notice this all.

Also I think it'd be good to extract this hack into a little helper
function, maybe called i830_plane_panning_hack or so. That way it's out of
the normal code flow and clearly marked as something exceptionel.

> As a related question: Is there possibly a command line tool that
> would allow me to modify the intel chipset registers on the fly
> without going through a kernel recompile? It would make some
> experiments just so much simpler.

intel-gpu-tools has intel_reg_read/write tools. That should help ;-)

Cheers, Daniel
> 
> Greetings,
>     Thomas
> 

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e5eb11d..e98298f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2087,8 +2087,44 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  				     i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
>  		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
>  		I915_WRITE(DSPLINOFF(plane), linear_offset);
> -	} else
> -		I915_WRITE(DSPADDR(plane), i915_gem_obj_ggtt_offset(obj) + linear_offset);
> +	} else if (INTEL_INFO(dev)->gen == 2 && IS_I830(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> +	  unsigned long planeadr = i915_gem_obj_ggtt_offset(obj) + linear_offset;
> +	  DRM_DEBUG_KMS("Plane address is 0x%lx",planeadr);
> +	  // I830 panning flicker work around. Only for non-LVDS output, only for i830.
> +	  if (obj->tiling_mode != I915_TILING_NONE) {
> +	    if ((planeadr & 0x40)) {
> +	      DRM_DEBUG_KMS("Detected potential flicker in tiling mode");
> +	      DRM_DEBUG_KMS("No workaround available");
> +	      DRM_DEBUG_KMS("Use a linear frame buffer");
> +	    }
> +	  } else {
> +	    switch (fb->pixel_format) {
> +	    case DRM_FORMAT_XRGB1555:
> +	    case DRM_FORMAT_ARGB1555:
> +	    case DRM_FORMAT_RGB565:
> +	    case DRM_FORMAT_XRGB8888:
> +	    case DRM_FORMAT_ARGB8888:
> +	    case DRM_FORMAT_XBGR8888:
> +	    case DRM_FORMAT_ABGR8888:
> +	      {
> +		unsigned long int oldadr = I915_READ(DSPADDR(plane));
> +		if (((oldadr ^ planeadr) & 0x40) && (planeadr & 0xc0) == 0xc0) {
> +		  DRM_DEBUG_KMS("Detected potential flicker in linear mode");
> +		  I915_WRITE(DSPADDR(plane), planeadr & (~(0x80)));
> +		  POSTING_READ(reg);
> +		  intel_wait_for_vblank(dev,intel_crtc->pipe);
> +		}
> +	      }
> +	      break;
> +	    default:
> +	      DRM_DEBUG_KMS("No flicker workaround available\n");
> +	      break;
> +	    }
> +	  }
> +	  I915_WRITE(DSPADDR(plane), planeadr);
> +	} else {
> +	  I915_WRITE(DSPADDR(plane), i915_gem_obj_ggtt_offset(obj) + linear_offset);
> +	}
>  	POSTING_READ(reg);
>  
>  	return 0;


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

Re: [PATCH] Workaround for flicker with panning on the i830

[Thomas Richter]

Am 08.11.2013 17:32, schrieb Daniel Vetter:
> Kernel has a tool in scripts/checkpatch.pl which will tell you what's all
> off ;-) Also sob line and similar essential things are missing, but the
> script should notice this all.
>
> Also I think it'd be good to extract this hack into a little helper
> function, maybe called i830_plane_panning_hack or so. That way it's out of
> the normal code flow and clearly marked as something exceptionel.

Thanks, I'll try that. Still working on it, though.
>
>> As a related question: Is there possibly a command line tool that
>> would allow me to modify the intel chipset registers on the fly
>> without going through a kernel recompile? It would make some
>> experiments just so much simpler.
> intel-gpu-tools has intel_reg_read/write tools. That should help ;-)


These tools are exceptional, and exactly what I was looking for. Thanks 
a lot!

Now, how much is known about the register DSPARB, found at offset 0x70030?

Because, if I just feed this register with "correct" values (for 
whatever "correct" means), I do get a stable image
on pipe A and pipe B. I haven't found out what "correct" is, precisely, 
but there is something I can do here.
By default, on my setup, the value here is 0x17e5f, but depending on the 
scroll position, 0x17e51, 0x17e61, 0x17e6f
or 0x17e63 create stable images. So there is at least some way how to 
get the image right, though I don't quite
understand the mechanics yet.

BTW, now that I looked at intel_regs_dump: When the screen is 
flickering, I do get "pipe A underruns". I tried working
with the watermark registers, but all I can get is a crashed GPU or a 
ripped display, so this is not quite the right approach.
DSPARB seems to be the best trick so far, even though I don't quite get 
how it works.

Any ideas?

Greetings,
     Thomas

Re: [PATCH] Workaround for flicker with panning on the i830

[Daniel Vetter]

On Mon, Nov 11, 2013 at 4:33 PM, Thomas Richter <thor@math.tu-berlin.de> wrote:
> Now, how much is known about the register DSPARB, found at offset 0x70030?
>
> Because, if I just feed this register with "correct" values (for whatever
> "correct" means), I do get a stable image
> on pipe A and pipe B. I haven't found out what "correct" is, precisely, but
> there is something I can do here.
> By default, on my setup, the value here is 0x17e5f, but depending on the
> scroll position, 0x17e51, 0x17e61, 0x17e6f
> or 0x17e63 create stable images. So there is at least some way how to get
> the image right, though I don't quite
> understand the mechanics yet.
>
> BTW, now that I looked at intel_regs_dump: When the screen is flickering, I
> do get "pipe A underruns". I tried working
> with the watermark registers, but all I can get is a crashed GPU or a ripped
> display, so this is not quite the right approach.
> DSPARB seems to be the best trick so far, even though I don't quite get how
> it works.

Oh, that's really interesting. gen2 has a unified display fifo on
machines that support 2 outputs. DSPARB tells the hw how to exactly
split this up between the two pipes. There are two bit ranges of
interest here:

bits0-8: "AEND. This field selects one of the splits between two of
the three portions of the display RAM allocated
between display plane A, plane B, and display plane C. This field can
only be programmed when one of
the two pipes is disabled. When it is written. It takes effect on the
next VBLANK for whichever pipe is
currently active. The control granularity is 16-bytes.
The total size of the Almador-M RAM is 288*16 bytes.
The total size of the Montara-GM RAM is 256*16 bytes."

bits 9-18: "BEND. This field selects one of the split between the two
of the three portions of the display RAM
allocated between display planes A, B, and C. This field can only be
programmed when one of the two
display pipes is disabled. It takes effect on the next VBLANK for
whichever pipe is currently active. It
must be programmed to a number larger than the value in AEND.
The control granularity is 16-bytes and the total size of the
Almador-M RAM is 288*16 bytes and the
Montara-GM RAM is 256*16 bytes."

Almador-M is i830M, Montara-GM is i855GM. Other gen2 chips have a
different meaning for this register.

What we'd need to do here is to update this register when switching
the number of active display pipes in the ->modeset_global_resources
hook. We also need to make sure we have updated watermark values set
up already, before rewriting the value of DSPARB (since the watermarks
depend upon the size of the fifo).

For I start I'd go with splitting the fifo according to the display
clock between plane A and B and giving nothing to plane C. We don't
have any code to use plane C so giving everything to just A and B is
better.

I hope this helps you in your endeavor. Otherwise feel free to ping me
on irc or mail.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] Workaround for flicker with panning on the i830

[Thomas Richter]

Am 11.11.2013 16:43, schrieb Daniel Vetter:
> Oh, that's really interesting. gen2 has a unified display fifo on
> machines that support 2 outputs. DSPARB tells the hw how to exactly
> split this up between the two pipes. There are two bit ranges of
> interest here:

/* snip */

Hmm, why I understand *that* it does make a difference, I do not 
understand the details.
By a unified display fifo do you mean that the display output has an 
internal buffer memory (the fifo) which
basically feeds the the DVOs or the LVDS with memory, which comes via 
DMA into the fifo. Is that right?

By "split", do you mean that a fixed amount of bytes (or rather, lines 
as in multiples of 16 bytes) are allocated for each
participating pipe?

Simply enlarging the fifo does not help (i.e. writing a larger value 
into the register). Just the positions where I get the flicker change, 
but the problem does not go away. So whatever needs to be done is to 
adjust this register according to the alignment of the base address of 
the corresponding DMA engine that feeds the pipe.


> What we'd need to do here is to update this register when switching
> the number of active display pipes in the ->modeset_global_resources
> hook. We also need to make sure we have updated watermark values set
> up already, before rewriting the value of DSPARB (since the watermarks
> depend upon the size of the fifo).
>
> For I start I'd go with splitting the fifo according to the display
> clock between plane A and B and giving nothing to plane C. We don't
> have any code to use plane C so giving everything to just A and B is
> better.

By that you mean "BEND = maximum" and "AEND" in between? That does not 
seem to be sufficient. It needs to be modified according to the buffer 
alignment, and sometimes smaller values work, sometimes larger ones.

Greetings,
     Thomas

Re: [PATCH] Workaround for flicker with panning on the i830

[Daniel Vetter]

On Tue, Nov 12, 2013 at 05:41:12PM +0100, Thomas Richter wrote:
> Am 11.11.2013 16:43, schrieb Daniel Vetter:
> >Oh, that's really interesting. gen2 has a unified display fifo on
> >machines that support 2 outputs. DSPARB tells the hw how to exactly
> >split this up between the two pipes. There are two bit ranges of
> >interest here:
> 
> /* snip */
> 
> Hmm, why I understand *that* it does make a difference, I do not
> understand the details.
> By a unified display fifo do you mean that the display output has an
> internal buffer memory (the fifo) which
> basically feeds the the DVOs or the LVDS with memory, which comes
> via DMA into the fifo. Is that right?

Yup. The fifo is there ot paper over memory fetch latencies. The memory
controller sends 64b blocks, but the display output needs to be continous.

> By "split", do you mean that a fixed amount of bytes (or rather,
> lines as in multiples of 16 bytes) are allocated for each
> participating pipe?

Yup.

> Simply enlarging the fifo does not help (i.e. writing a larger value
> into the register). Just the positions where I get the flicker
> change, but the problem does not go away. So whatever needs to be
> done is to adjust this register according to the alignment of the
> base address of the corresponding DMA engine that feeds the pipe.

Hm, that's strange.

> >What we'd need to do here is to update this register when switching
> >the number of active display pipes in the ->modeset_global_resources
> >hook. We also need to make sure we have updated watermark values set
> >up already, before rewriting the value of DSPARB (since the watermarks
> >depend upon the size of the fifo).
> >
> >For I start I'd go with splitting the fifo according to the display
> >clock between plane A and B and giving nothing to plane C. We don't
> >have any code to use plane C so giving everything to just A and B is
> >better.
> 
> By that you mean "BEND = maximum" and "AEND" in between? That does
> not seem to be sufficient. It needs to be modified according to the
> buffer alignment, and sometimes smaller values work, sometimes
> larger ones.

Yeah, I've meant BEND = max and AEND split so that the two resulting sizes
are proportional to the pixel clock (i.e. both pipes should take equal
amount of time roughly to go through the full fifo). Indeed really strang
that this doesn't seem to work.

Looking at docs I don't see any mention of a w/a :(

The only (totally crazy) idea I have is that the fifo falls over if it
fetches accross a tile/page boundary. But no idea how to figure out a
formula that'd work everywhere ...

To simplify things I'd start with just just one pipe display to VGA and
then going through different resolutions and different offset. Maybe
there's a pattern in the display fifo lenghts that work.

Happy hacking!

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

Re: [PATCH] Workaround for flicker with panning on the i830

[Thomas Richter]

On 12.11.2013 18:22, Daniel Vetter wrote:

Thanks for the explanation how the fifos work, that was helpful.


> Yeah, I've meant BEND = max and AEND split so that the two resulting sizes
> are proportional to the pixel clock (i.e. both pipes should take equal
> amount of time roughly to go through the full fifo). Indeed really strang
> that this doesn't seem to work.
>
> Looking at docs I don't see any mention of a w/a :(

Too bad. I tried to find some systematic in the settings where I do get 
flicker and where not, in specific which settings of AEND create the 
problem. However, it's more complicated than I thought. It not only 
depends on the current scroll position, but also on the history (!) of 
what you did before. It's probably the relative fill position of the 
FIFOs that plays the role here. If the fifo was relatively full before 
starting the scroll, everything remains fine. Otherwise, it runs dry too 
soon. If you keep the history consistent, the outcome is consistent, but 
otherwise results are hard to predict.

Maybe the whole approach of adjusting the start address of the DMA 
engine to realize scrolling is not quite right, and the engine prefers 
to have data aligned to 64 byte boundaries or something like this. Is 
there some other way to delay the output of the FIFO into the pipe? Some 
very antique systems (ehem, like the Atari 800 or Amiga chipsets ;-) had 
"horizontal scroll registers" that realized a pixel delay for an 
otherwise byte-oriented pipeline.

Is there something like this in the i830 such that the actual start 
address of the DMA engine would remain constant, but the pipeline would 
refer or disregard the first n elements? This would avoid the whole problem.

The trouble is not only cosmetical (the flicker): If that happens, any 
application that uses an X video overlay misbehaives (xine crashes the 
system really badly) so the pipe-A underrun should be avoided at all costs.

Concerning the intel gpu tools: Currently, they just brute-force "poke" 
into the display registers. Is there some way to synchronize this 
activity to vblank from user space (I got their sources, so modifying 
them would work, but I would need some kind of mechanism to wait for 
vblank to do that savely).

> The only (totally crazy) idea I have is that the fifo falls over if it
> fetches accross a tile/page boundary. But no idea how to figure out a
> formula that'd work everywhere ...

Exactly. Might be possible, given that I have a relatively easy fix for 
sequential, but not for tiled. If I may add another question: How 
exactly does the tiled access then work? As far as I understand, the 
video RAM is still organized linearly, but the DMA engine fetches data 
differently? Is this right? How exactly are the tiles laid out, and how 
does memory fetches then work?

> To simplify things I'd start with just just one pipe display to VGA and
> then going through different resolutions and different offset. Maybe
> there's a pattern in the display fifo lenghts that work.

See above. Nope. Or rather "to some extend", but it's more complicated 
than that.

I wonder how that works in windows, though. The intel driver does 
support panning to some degree, at least if the resolutions of the 
internal and external display differ.

> Happy hacking!

I'm certainly having fun here. (-:

Greetings,
	Thomas

Re: [PATCH] Workaround for flicker with panning on the i830

[Daniel Vetter]

On Wed, Nov 13, 2013 at 08:50:50PM +0100, Thomas Richter wrote:
> On 12.11.2013 18:22, Daniel Vetter wrote:
> 
> Thanks for the explanation how the fifos work, that was helpful.
> 
> 
> >Yeah, I've meant BEND = max and AEND split so that the two resulting sizes
> >are proportional to the pixel clock (i.e. both pipes should take equal
> >amount of time roughly to go through the full fifo). Indeed really strang
> >that this doesn't seem to work.
> >
> >Looking at docs I don't see any mention of a w/a :(
> 
> Too bad. I tried to find some systematic in the settings where I do
> get flicker and where not, in specific which settings of AEND create
> the problem. However, it's more complicated than I thought. It not
> only depends on the current scroll position, but also on the history
> (!) of what you did before. It's probably the relative fill position
> of the FIFOs that plays the role here. If the fifo was relatively
> full before starting the scroll, everything remains fine. Otherwise,
> it runs dry too soon. If you keep the history consistent, the
> outcome is consistent, but otherwise results are hard to predict.

Hm, scrolling should be vsynced, and I'd expect the fifo to properly
refill in the vblank. So this is pretty astonishing. Could it be that
rendering activity affects this, or is this always with an otherwise
completely idle desktop?

> Maybe the whole approach of adjusting the start address of the DMA
> engine to realize scrolling is not quite right, and the engine
> prefers to have data aligned to 64 byte boundaries or something like
> this. Is there some other way to delay the output of the FIFO into
> the pipe? Some very antique systems (ehem, like the Atari 800 or
> Amiga chipsets ;-) had "horizontal scroll registers" that realized a
> pixel delay for an otherwise byte-oriented pipeline.
> 
> Is there something like this in the i830 such that the actual start
> address of the DMA engine would remain constant, but the pipeline
> would refer or disregard the first n elements? This would avoid the
> whole problem.

Nope, we don't have a buffer offset in pixels like on later generations.

> The trouble is not only cosmetical (the flicker): If that happens,
> any application that uses an X video overlay misbehaives (xine
> crashes the system really badly) so the pipe-A underrun should be
> avoided at all costs.

Yeah, the overlay is extremely fickle and if it's dead it'll never work
again until you reboot. And once it crashed all Xv apps won't work any
more :(

> Concerning the intel gpu tools: Currently, they just brute-force
> "poke" into the display registers. Is there some way to synchronize
> this activity to vblank from user space (I got their sources, so
> modifying them would work, but I would need some kind of mechanism
> to wait for vblank to do that savely).

The registers are vblank synced already. So if you poke both the dsparb
and the plane base register from userspace to fake scrolling you should
have as good a vsync as the kernel will get (if you script both register
writes together ofc).
> 
> >The only (totally crazy) idea I have is that the fifo falls over if it
> >fetches accross a tile/page boundary. But no idea how to figure out a
> >formula that'd work everywhere ...
> 
> Exactly. Might be possible, given that I have a relatively easy fix
> for sequential, but not for tiled. If I may add another question:
> How exactly does the tiled access then work? As far as I understand,
> the video RAM is still organized linearly, but the DMA engine
> fetches data differently? Is this right? How exactly are the tiles
> laid out, and how does memory fetches then work?

Tile buffers aren't linear any more in memory. Tiles are 2kb in size and
are laid out in x-major direction. The tile itself is  128 bytes wide and
16 lines high. So presuming you start scanning out out at offset 0 the
dma-engine will read:

0 - 127, 2k - (2k+127), 4k - (4k+127), ... for the first display lane
128 - 255, (2k+128) - (2k+255), (4k+128) - (4k+255), ... for the 2nd display lane
...

> >To simplify things I'd start with just just one pipe display to VGA and
> >then going through different resolutions and different offset. Maybe
> >there's a pattern in the display fifo lenghts that work.
> 
> See above. Nope. Or rather "to some extend", but it's more
> complicated than that.
> 
> I wonder how that works in windows, though. The intel driver does
> support panning to some degree, at least if the resolutions of the
> internal and external display differ.

Could be that windows scrolls in software by copying the buffer around.
Maybe the tearfree option can help here, although I expect it to hurt
badly for gtt constrained i830M platforms. And tbh I don't know whether it
works with panning.

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

Re: [PATCH] Workaround for flicker with panning on the i830

[Thomas Richter]

On 13.11.2013 21:20, Daniel Vetter wrote:

Thanks Daniel for your explanations. As always, very helpful.

>
> Tile buffers aren't linear any more in memory. Tiles are 2kb in size and
> are laid out in x-major direction. The tile itself is  128 bytes wide and
> 16 lines high. So presuming you start scanning out out at offset 0 the
> dma-engine will read:
>
> 0 - 127, 2k - (2k+127), 4k - (4k+127), ... for the first display lane
> 128 - 255, (2k+128) - (2k+255), (4k+128) - (4k+255), ... for the 2nd display lane
> ...

Hmm, then there is something I don't quite understand. If I check the 
code for the plane buffer start address computation, I do indeed see 
something like tile addressing for GEN4 and up, but for GEN2, I simply 
see a linear address = x + y * stride. This does not look consistent:

/* snip : code from i9xx_update_plane() in i915_display.c */

         linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);

         if (INTEL_INFO(dev)->gen >= 4) {
                 intel_crtc->dspaddr_offset =
                         intel_gen4_compute_page_offset(&x, &y, 
obj->tiling_mode,
 
fb->bits_per_pixel / 8,
                                                        fb->pitches[0]);
                 linear_offset -= intel_crtc->dspaddr_offset;
         } else {
                 intel_crtc->dspaddr_offset = linear_offset;
         }

/* snip */

Are you *really sure* GEN2 does have tiling? Or could it be that this 
bit is used for something else and probably turns on some weird 
powersaving feature that creates some mischief with the FIFO? After all, 
a tile cannot always be 128 pixels long independent of the display 
organization (i.e. "pixel format") *and* have the above formula correct?

There is then at least something I must be missing.

>> I wonder how that works in windows, though. The intel driver does
>> support panning to some degree, at least if the resolutions of the
>> internal and external display differ.
>
> Could be that windows scrolls in software by copying the buffer around.
> Maybe the tearfree option can help here, although I expect it to hurt
> badly for gtt constrained i830M platforms. And tbh I don't know whether it
> works with panning.

Actually, scrolling is quite smooth, I doubt it's a software scrolling. 
But anyhow...

Greetings,
	Thomas

Re: [PATCH] Workaround for flicker with panning on the i830

[Daniel Vetter]

On Thu, Nov 14, 2013 at 8:14 AM, Thomas Richter <thor@math.tu-berlin.de> wrote:
> Are you *really sure* GEN2 does have tiling? Or could it be that this bit is
> used for something else and probably turns on some weird powersaving feature
> that creates some mischief with the FIFO? After all, a tile cannot always be
> 128 pixels long independent of the display organization (i.e. "pixel
> format") *and* have the above formula correct?
>
> There is then at least something I must be missing.

On gen2/3 the fence registers make a tile range look linear to both
the gpu and the cpu. On gen4+ the fence registers are only for access
with the cpu, and everything else needs to take tiling into account
explicitly (and there are bits in the registers to tell the gpu that
something is tiled). See the various functions with fence in their
name in i915_gem.c for how this is set up/tracked.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] Workaround for flicker with panning on the i830

[Thomas Richter]

On 14.11.2013 09:21, Daniel Vetter wrote:

> On gen2/3 the fence registers make a tile range look linear to both
> the gpu and the cpu. On gen4+ the fence registers are only for access
> with the cpu, and everything else needs to take tiling into account
> explicitly (and there are bits in the registers to tell the gpu that
> something is tiled). See the various functions with fence in their
> name in i915_gem.c for how this is set up/tracked.

Hmm. Probably I still don't quite understand. Memory is shared between 
CPU and GPU, so why does a memory write or read by the CPU depend on the 
GPU programming? The GPU is, after all, not a MMU that manipulates the 
address bus of the CPU (or does it?)

If the start address of the display is altered, are the tiles always 
relative to this start, i.e. at the same absolute display position, or 
do they move on screen, i.e. are at the same absolute memory position? 
Would it possibly make sense to check whether modifying the fence 
registers avoids the problem because then the tiles move, and probably 
stay aligned to 16-byte (or 32-byte) boundaries?

Greetings,
	Thomas

Re: [PATCH] Workaround for flicker with panning on the i830

[Daniel Vetter]

On Thu, Nov 14, 2013 at 07:15:52PM +0100, Thomas Richter wrote:
> On 14.11.2013 09:21, Daniel Vetter wrote:
> 
> >On gen2/3 the fence registers make a tile range look linear to both
> >the gpu and the cpu. On gen4+ the fence registers are only for access
> >with the cpu, and everything else needs to take tiling into account
> >explicitly (and there are bits in the registers to tell the gpu that
> >something is tiled). See the various functions with fence in their
> >name in i915_gem.c for how this is set up/tracked.
> 
> Hmm. Probably I still don't quite understand. Memory is shared
> between CPU and GPU, so why does a memory write or read by the CPU
> depend on the GPU programming? The GPU is, after all, not a MMU that
> manipulates the address bus of the CPU (or does it?)

The big pci bar in the gpu _is_ an iommu ;-)

> If the start address of the display is altered, are the tiles always
> relative to this start, i.e. at the same absolute display position,
> or do they move on screen, i.e. are at the same absolute memory
> position? Would it possibly make sense to check whether modifying
> the fence registers avoids the problem because then the tiles move,
> and probably stay aligned to 16-byte (or 32-byte) boundaries?

Tiled areas have rather strict alignement constraints. The stride must be
a power of two, the size also and the start needs to be aligned to the
size. So you can't move the tiles. Also moving the tiles would totally
wreak havoc with the screen contents ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Workaround for flicker with panning on the i830 - found a way for tiled displays

[Thomas Richter]

Hi Daniel, hi others,

did even more experiments. I guess I understand now better. Indeed, the 
trouble seems to be the watermark levels. I played more
with all that, and the real culprit seems to be the FW_BLC register 
controlling the watermarks.

On the i830 with the current settings, it is defined to be 0x1080304 
which sets the watermark a bit too low. If I set it to
0x1080306 instead, I get a stable display in all panning positions 
(hurray!).

I would like to fix this, but I guess I would need to understand the 
logic a little bit better. At the time being, you probably better
put the linear frame buffer workaround on hold, it looks I really got 
something here.

Greetings,
     Thomas

Re: Workaround for flicker with panning on the i830 - found a way for tiled displays

[Daniel Vetter]

On Fri, Nov 15, 2013 at 02:16:11PM +0100, Thomas Richter wrote:
> Hi Daniel, hi others,
> 
> did even more experiments. I guess I understand now better. Indeed,
> the trouble seems to be the watermark levels. I played more
> with all that, and the real culprit seems to be the FW_BLC register
> controlling the watermarks.
> 
> On the i830 with the current settings, it is defined to be 0x1080304
> which sets the watermark a bit too low. If I set it to
> 0x1080306 instead, I get a stable display in all panning positions
> (hurray!).
> 
> I would like to fix this, but I guess I would need to understand the
> logic a little bit better. At the time being, you probably better
> put the linear frame buffer workaround on hold, it looks I really
> got something here.

Gosh, should have read the code more closely. We have a totally botched wm
setup on i830M - the watermark code for the 2nd pipe is just not there!

I'll try to wip up a patch to fix this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Workaround for flicker with panning on the i830 - found a way for tiled displays

[Thomas Richter]

Am 15.11.2013 16:41, schrieb Daniel Vetter:
>
> Gosh, should have read the code more closely. We have a totally botched wm
> setup on i830M - the watermark code for the 2nd pipe is just not there!

(-: Guess that explains something. Just disregard my earlier patch, 
simply superfluous.
In the meantime, I recompiled the code and *decreased* the latency from 
5000 to 3000, then getting a stable image,
even the boot console is then stable (has never been before). Its still 
off by half a screen, but no longer flickering
left and right.

However, what I do not understand about the watermark computation is 
that a *lower* latency results in a *higher* number
of entries in the FIFO. Shouldn't this quite the reverse?
In specific, I do not understand the subtraction in intel_calculate_wm, 
line 1058.

Anyhow, I let you proceed with the patch and I'm ready and happy to test it.

Greetings,
     Thomas

Re: Workaround for flicker with panning on the i830 - found a way for tiled displays

[Thomas Richter]

Hi Daniel,
> Gosh, should have read the code more closely. We have a totally botched wm
> setup on i830M - the watermark code for the 2nd pipe is just not there!

Well, nice try, but no cigar. (-: That's actually much worse than 
before. The display is now unstable on *both* the internal
and external display, and inspecting the FW_BLC register, it is 
completely off. The current code leaves it at 0x1050101, but
it should be at least 0x1060106, that is, the watermark needs to be 
higher, not lower. I still don't understand why a higher
latency causes a lower watermark, but maybe things work different than 
in my mental model.

Greetings,
     Thomas