[PATCH] drm/i915: Replicate BIOS eDP bpp clamping hack for hsw

[PATCH] drm/i915: Replicate BIOS eDP bpp clamping hack for hsw

[Daniel Vetter]

Haswell's DDI encoders have their own ->get_config callback and in

commit c6cd2ee2d59111a07cd9199564c9bdcb2d11e5cf
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Mon Oct 21 10:52:07 2013 +0300

    drm/i915/dp: workaround BIOS eDP bpp clamping issue

we've forgotten to replicate this hack. So let's do it that.

Note for backporters: The above commit and all it's depencies need to
be backported first.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71049
Cc: stable@vger.kernel.org
Tested-by: Gökçen Eraslan <gokcen.eraslan@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1591576a6101..330077bcd0bd 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1406,6 +1406,26 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 	default:
 		break;
 	}
+
+	if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp_bpp &&
+	    pipe_config->pipe_bpp > dev_priv->vbt.edp_bpp) {
+		/*
+		 * This is a big fat ugly hack.
+		 *
+		 * Some machines in UEFI boot mode provide us a VBT that has 18
+		 * bpp and 1.62 GHz link bandwidth for eDP, which for reasons
+		 * unknown we fail to light up. Yet the same BIOS boots up with
+		 * 24 bpp and 2.7 GHz link. Use the same bpp as the BIOS uses as
+		 * max, not what it tells us to use.
+		 *
+		 * Note: This will still be broken if the eDP panel is not lit
+		 * up by the BIOS, and thus we can't get the mode at module
+		 * load.
+		 */
+		DRM_DEBUG_KMS("pipe has %d bpp for eDP panel, overriding BIOS-provided max %d bpp\n",
+			      pipe_config->pipe_bpp, dev_priv->vbt.edp_bpp);
+		dev_priv->vbt.edp_bpp = pipe_config->pipe_bpp;
+	}
 }
 
 static void intel_ddi_destroy(struct drm_encoder *encoder)
-- 
1.8.4.3

Re: [Intel-gfx] [PATCH] drm/i915: Replicate BIOS eDP bpp clamping hack for hsw

[Ville Syrjälä]

On Mon, Nov 18, 2013 at 07:38:16AM +0100, Daniel Vetter wrote:
> Haswell's DDI encoders have their own ->get_config callback and in
> 
> commit c6cd2ee2d59111a07cd9199564c9bdcb2d11e5cf
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Mon Oct 21 10:52:07 2013 +0300
> 
>     drm/i915/dp: workaround BIOS eDP bpp clamping issue
> 
> we've forgotten to replicate this hack. So let's do it that.
> 
> Note for backporters: The above commit and all it's depencies need to
> be backported first.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71049
> Cc: stable@vger.kernel.org
> Tested-by: Gökçen Eraslan <gokcen.eraslan@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

Although I might suggest moving the hack into a small function of its
own and calling it from both ddi and dp code.

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1591576a6101..330077bcd0bd 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1406,6 +1406,26 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  	default:
>  		break;
>  	}
> +
> +	if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp_bpp &&
> +	    pipe_config->pipe_bpp > dev_priv->vbt.edp_bpp) {
> +		/*
> +		 * This is a big fat ugly hack.
> +		 *
> +		 * Some machines in UEFI boot mode provide us a VBT that has 18
> +		 * bpp and 1.62 GHz link bandwidth for eDP, which for reasons
> +		 * unknown we fail to light up. Yet the same BIOS boots up with
> +		 * 24 bpp and 2.7 GHz link. Use the same bpp as the BIOS uses as
> +		 * max, not what it tells us to use.
> +		 *
> +		 * Note: This will still be broken if the eDP panel is not lit
> +		 * up by the BIOS, and thus we can't get the mode at module
> +		 * load.
> +		 */
> +		DRM_DEBUG_KMS("pipe has %d bpp for eDP panel, overriding BIOS-provided max %d bpp\n",
> +			      pipe_config->pipe_bpp, dev_priv->vbt.edp_bpp);
> +		dev_priv->vbt.edp_bpp = pipe_config->pipe_bpp;
> +	}
>  }
>  
>  static void intel_ddi_destroy(struct drm_encoder *encoder)
> -- 
> 1.8.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Replicate BIOS eDP bpp clamping hack for hsw

[Daniel Vetter]

On Tue, Nov 19, 2013 at 05:51:54PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 18, 2013 at 07:38:16AM +0100, Daniel Vetter wrote:
> > Haswell's DDI encoders have their own ->get_config callback and in
> > 
> > commit c6cd2ee2d59111a07cd9199564c9bdcb2d11e5cf
> > Author: Jani Nikula <jani.nikula@intel.com>
> > Date:   Mon Oct 21 10:52:07 2013 +0300
> > 
> >     drm/i915/dp: workaround BIOS eDP bpp clamping issue
> > 
> > we've forgotten to replicate this hack. So let's do it that.
> > 
> > Note for backporters: The above commit and all it's depencies need to
> > be backported first.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71049
> > Cc: stable@vger.kernel.org
> > Tested-by: Gökçen Eraslan <gokcen.eraslan@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review, merged to -fixes.
> 
> Although I might suggest moving the hack into a small function of its
> own and calling it from both ddi and dp code.

I've considered this, but then 90% of the code is the comment explaining
what's going on, so I've figured it's better to duplicate this. If we grow
more edp vbt hacks in ->get_config we can reconsider. But I hope not,
since atm we have no chance to light up the panel if the bios didn't do so
already :(
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1591576a6101..330077bcd0bd 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1406,6 +1406,26 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >  	default:
> >  		break;
> >  	}
> > +
> > +	if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp_bpp &&
> > +	    pipe_config->pipe_bpp > dev_priv->vbt.edp_bpp) {
> > +		/*
> > +		 * This is a big fat ugly hack.
> > +		 *
> > +		 * Some machines in UEFI boot mode provide us a VBT that has 18
> > +		 * bpp and 1.62 GHz link bandwidth for eDP, which for reasons
> > +		 * unknown we fail to light up. Yet the same BIOS boots up with
> > +		 * 24 bpp and 2.7 GHz link. Use the same bpp as the BIOS uses as
> > +		 * max, not what it tells us to use.
> > +		 *
> > +		 * Note: This will still be broken if the eDP panel is not lit
> > +		 * up by the BIOS, and thus we can't get the mode at module
> > +		 * load.
> > +		 */
> > +		DRM_DEBUG_KMS("pipe has %d bpp for eDP panel, overriding BIOS-provided max %d bpp\n",
> > +			      pipe_config->pipe_bpp, dev_priv->vbt.edp_bpp);
> > +		dev_priv->vbt.edp_bpp = pipe_config->pipe_bpp;
> > +	}
> >  }
> >  
> >  static void intel_ddi_destroy(struct drm_encoder *encoder)
> > -- 
> > 1.8.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

[PATCH] Added a lower limit for the watermark setting

[Thomas Richter]

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

Hi Daniel, dear intel experts,

please find a patch attached that finally addresses the display flicker 
on i830 chipsets. This
patch adds a lower watermark setting in intel_watermark_params{}, but 
keeps it zero for
all but the i830 chipsets. The necessary new defines are in i915_reg.h.

Greetings,
     Thomas


[-- Attachment #2: 0003-Added-a-lower-limit-for-the-watermark-setting.patch --]
[-- Type: text/x-patch, Size: 9582 bytes --]

>From 916d8354b2134d587b946fa88c66c3d098d38df4 Mon Sep 17 00:00:00 2001
From: Thomas Richter <thor@math.tu-berlin.de>
Date: Tue, 19 Nov 2013 17:09:45 +0100
Subject: [PATCH 3/3] Added a lower limit for the watermark setting.

This patch adds a minimum value for the watermark setting, in addition
to the already existing upper limit. The lower limit is necessary on
i830 chipsets as otherwise display flickering may result, especially
on panning. While the flicker itself is caused by the FIFO running
dry. Interestingly, an overrunning FIFO may also cause a hickup in
the chipset which results in a pipeline stall with the very same
effects.

Lower limits for all other chipsets are zero to remain consistent
with the currently active code.

Signed-off-by: Thomas Richter <thor@math.tu-berlin.de>
---
 drivers/gpu/drm/i915/i915_reg.h  |   16 ++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 drivers/gpu/drm/i915/intel_pm.c  |   24 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 849e595..c144957 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3361,24 +3361,32 @@
 #define I915_FIFO_SIZE		95
 #define I855GM_FIFO_SIZE	127 /* In cachelines */
 #define I830_FIFO_SIZE		95
+#define I830_MIN_WM             8 /* instabilities if below this point */
 
+#define VALLEYVIEW_MIN_WM	0
 #define VALLEYVIEW_MAX_WM	0xff
+#define G4X_MIN_WM		0
 #define G4X_MAX_WM		0x3f
+#define I915_MIN_WM		0
 #define I915_MAX_WM		0x3f
 
 #define PINEVIEW_DISPLAY_FIFO	512 /* in 64byte unit */
 #define PINEVIEW_FIFO_LINE_SIZE	64
+#define PINEVIEW_MIN_WM                0
 #define PINEVIEW_MAX_WM		0x1ff
 #define PINEVIEW_DFT_WM		0x3f
 #define PINEVIEW_DFT_HPLLOFF_WM	0
 #define PINEVIEW_GUARD_WM		10
 #define PINEVIEW_CURSOR_FIFO		64
+#define PINEVIEW_CURSOR_MIN_WM	0
 #define PINEVIEW_CURSOR_MAX_WM	0x3f
 #define PINEVIEW_CURSOR_DFT_WM	0
 #define PINEVIEW_CURSOR_GUARD_WM	5
 
 #define VALLEYVIEW_CURSOR_MAX_WM 64
+#define VALLEYVIEW_CURSOR_MIN_WM 0
 #define I965_CURSOR_FIFO	64
+#define I965_CURSOR_MIN_WM	0
 #define I965_CURSOR_MAX_WM	32
 #define I965_CURSOR_DFT_WM	8
 
@@ -3424,16 +3432,20 @@
 
 /* define the fifo size on Ironlake */
 #define ILK_DISPLAY_FIFO	128
+#define ILK_DISPLAY_MINWM	0
 #define ILK_DISPLAY_MAXWM	64
 #define ILK_DISPLAY_DFTWM	8
 #define ILK_CURSOR_FIFO		32
+#define ILK_CURSOR_MINWM        0
 #define ILK_CURSOR_MAXWM	16
 #define ILK_CURSOR_DFTWM	8
 
 #define ILK_DISPLAY_SR_FIFO	512
+#define ILK_DISPLAY_MIN_SRWM	0
 #define ILK_DISPLAY_MAX_SRWM	0x1ff
 #define ILK_DISPLAY_DFT_SRWM	0x3f
 #define ILK_CURSOR_SR_FIFO	64
+#define ILK_CURSOR_MIN_SRWM	0
 #define ILK_CURSOR_MAX_SRWM	0x3f
 #define ILK_CURSOR_DFT_SRWM	8
 
@@ -3441,16 +3453,20 @@
 
 /* define the WM info on Sandybridge */
 #define SNB_DISPLAY_FIFO	128
+#define SNB_DISPLAY_MINWM	0
 #define SNB_DISPLAY_MAXWM	0x7f	/* bit 16:22 */
 #define SNB_DISPLAY_DFTWM	8
 #define SNB_CURSOR_FIFO		32
+#define SNB_CURSOR_MINWM	0
 #define SNB_CURSOR_MAXWM	0x1f	/* bit 4:0 */
 #define SNB_CURSOR_DFTWM	8
 
 #define SNB_DISPLAY_SR_FIFO	512
+#define SNB_DISPLAY_MIN_SRWM	0
 #define SNB_DISPLAY_MAX_SRWM	0x1ff	/* bit 16:8 */
 #define SNB_DISPLAY_DFT_SRWM	0x3f
 #define SNB_CURSOR_SR_FIFO	64
+#define SNB_CURSOR_MIN_SRWM	0
 #define SNB_CURSOR_MAX_SRWM	0x3f	/* bit 5:0 */
 #define SNB_CURSOR_DFT_SRWM	8
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8e133de..9400b1a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -418,6 +418,7 @@ struct intel_plane {
 
 struct intel_watermark_params {
 	unsigned long fifo_size;
+	unsigned long min_wm;
 	unsigned long max_wm;
 	unsigned long default_wm;
 	unsigned long guard_size;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9c8b374..47f6fc8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -868,6 +868,7 @@ static int i830_get_fifo_size(struct drm_device *dev, int plane)
 /* Pineview has different values for various configs */
 static const struct intel_watermark_params pineview_display_wm = {
 	PINEVIEW_DISPLAY_FIFO,
+	PINEVIEW_MIN_WM,
 	PINEVIEW_MAX_WM,
 	PINEVIEW_DFT_WM,
 	PINEVIEW_GUARD_WM,
@@ -875,6 +876,7 @@ static const struct intel_watermark_params pineview_display_wm = {
 };
 static const struct intel_watermark_params pineview_display_hplloff_wm = {
 	PINEVIEW_DISPLAY_FIFO,
+	PINEVIEW_MIN_WM,
 	PINEVIEW_MAX_WM,
 	PINEVIEW_DFT_HPLLOFF_WM,
 	PINEVIEW_GUARD_WM,
@@ -882,6 +884,7 @@ static const struct intel_watermark_params pineview_display_hplloff_wm = {
 };
 static const struct intel_watermark_params pineview_cursor_wm = {
 	PINEVIEW_CURSOR_FIFO,
+	PINEVIEW_CURSOR_MIN_WM,
 	PINEVIEW_CURSOR_MAX_WM,
 	PINEVIEW_CURSOR_DFT_WM,
 	PINEVIEW_CURSOR_GUARD_WM,
@@ -889,6 +892,7 @@ static const struct intel_watermark_params pineview_cursor_wm = {
 };
 static const struct intel_watermark_params pineview_cursor_hplloff_wm = {
 	PINEVIEW_CURSOR_FIFO,
+	PINEVIEW_CURSOR_MIN_WM,
 	PINEVIEW_CURSOR_MAX_WM,
 	PINEVIEW_CURSOR_DFT_WM,
 	PINEVIEW_CURSOR_GUARD_WM,
@@ -896,6 +900,7 @@ static const struct intel_watermark_params pineview_cursor_hplloff_wm = {
 };
 static const struct intel_watermark_params g4x_wm_info = {
 	G4X_FIFO_SIZE,
+	G4X_MIN_WM,
 	G4X_MAX_WM,
 	G4X_MAX_WM,
 	2,
@@ -903,6 +908,7 @@ static const struct intel_watermark_params g4x_wm_info = {
 };
 static const struct intel_watermark_params g4x_cursor_wm_info = {
 	I965_CURSOR_FIFO,
+	I965_CURSOR_MIN_WM,
 	I965_CURSOR_MAX_WM,
 	I965_CURSOR_DFT_WM,
 	2,
@@ -910,6 +916,7 @@ static const struct intel_watermark_params g4x_cursor_wm_info = {
 };
 static const struct intel_watermark_params valleyview_wm_info = {
 	VALLEYVIEW_FIFO_SIZE,
+	VALLEYVIEW_MIN_WM,
 	VALLEYVIEW_MAX_WM,
 	VALLEYVIEW_MAX_WM,
 	2,
@@ -917,6 +924,7 @@ static const struct intel_watermark_params valleyview_wm_info = {
 };
 static const struct intel_watermark_params valleyview_cursor_wm_info = {
 	I965_CURSOR_FIFO,
+	VALLEYVIEW_CURSOR_MIN_WM,
 	VALLEYVIEW_CURSOR_MAX_WM,
 	I965_CURSOR_DFT_WM,
 	2,
@@ -924,6 +932,7 @@ static const struct intel_watermark_params valleyview_cursor_wm_info = {
 };
 static const struct intel_watermark_params i965_cursor_wm_info = {
 	I965_CURSOR_FIFO,
+	I965_CURSOR_MIN_WM,
 	I965_CURSOR_MAX_WM,
 	I965_CURSOR_DFT_WM,
 	2,
@@ -931,6 +940,7 @@ static const struct intel_watermark_params i965_cursor_wm_info = {
 };
 static const struct intel_watermark_params i945_wm_info = {
 	I945_FIFO_SIZE,
+	I915_MIN_WM,
 	I915_MAX_WM,
 	1,
 	2,
@@ -938,6 +948,7 @@ static const struct intel_watermark_params i945_wm_info = {
 };
 static const struct intel_watermark_params i915_wm_info = {
 	I915_FIFO_SIZE,
+	I915_MIN_WM,
 	I915_MAX_WM,
 	1,
 	2,
@@ -945,6 +956,7 @@ static const struct intel_watermark_params i915_wm_info = {
 };
 static const struct intel_watermark_params i855_wm_info = {
 	I855GM_FIFO_SIZE,
+	I830_MIN_WM,
 	I915_MAX_WM,
 	1,
 	2,
@@ -952,6 +964,7 @@ static const struct intel_watermark_params i855_wm_info = {
 };
 static const struct intel_watermark_params i830_wm_info = {
 	I830_FIFO_SIZE,
+	I830_MIN_WM,
 	I915_MAX_WM,
 	1,
 	2,
@@ -960,6 +973,7 @@ static const struct intel_watermark_params i830_wm_info = {
 
 static const struct intel_watermark_params ironlake_display_wm_info = {
 	ILK_DISPLAY_FIFO,
+	ILK_DISPLAY_MINWM,
 	ILK_DISPLAY_MAXWM,
 	ILK_DISPLAY_DFTWM,
 	2,
@@ -967,6 +981,7 @@ static const struct intel_watermark_params ironlake_display_wm_info = {
 };
 static const struct intel_watermark_params ironlake_cursor_wm_info = {
 	ILK_CURSOR_FIFO,
+	ILK_CURSOR_MINWM,
 	ILK_CURSOR_MAXWM,
 	ILK_CURSOR_DFTWM,
 	2,
@@ -974,6 +989,7 @@ static const struct intel_watermark_params ironlake_cursor_wm_info = {
 };
 static const struct intel_watermark_params ironlake_display_srwm_info = {
 	ILK_DISPLAY_SR_FIFO,
+	ILK_DISPLAY_MIN_SRWM,
 	ILK_DISPLAY_MAX_SRWM,
 	ILK_DISPLAY_DFT_SRWM,
 	2,
@@ -981,6 +997,7 @@ static const struct intel_watermark_params ironlake_display_srwm_info = {
 };
 static const struct intel_watermark_params ironlake_cursor_srwm_info = {
 	ILK_CURSOR_SR_FIFO,
+	ILK_CURSOR_MIN_SRWM,
 	ILK_CURSOR_MAX_SRWM,
 	ILK_CURSOR_DFT_SRWM,
 	2,
@@ -989,6 +1006,7 @@ static const struct intel_watermark_params ironlake_cursor_srwm_info = {
 
 static const struct intel_watermark_params sandybridge_display_wm_info = {
 	SNB_DISPLAY_FIFO,
+	SNB_DISPLAY_MINWM,
 	SNB_DISPLAY_MAXWM,
 	SNB_DISPLAY_DFTWM,
 	2,
@@ -996,6 +1014,7 @@ static const struct intel_watermark_params sandybridge_display_wm_info = {
 };
 static const struct intel_watermark_params sandybridge_cursor_wm_info = {
 	SNB_CURSOR_FIFO,
+	SNB_CURSOR_MINWM,
 	SNB_CURSOR_MAXWM,
 	SNB_CURSOR_DFTWM,
 	2,
@@ -1003,6 +1022,7 @@ static const struct intel_watermark_params sandybridge_cursor_wm_info = {
 };
 static const struct intel_watermark_params sandybridge_display_srwm_info = {
 	SNB_DISPLAY_SR_FIFO,
+	SNB_DISPLAY_MIN_SRWM,
 	SNB_DISPLAY_MAX_SRWM,
 	SNB_DISPLAY_DFT_SRWM,
 	2,
@@ -1010,6 +1030,7 @@ static const struct intel_watermark_params sandybridge_display_srwm_info = {
 };
 static const struct intel_watermark_params sandybridge_cursor_srwm_info = {
 	SNB_CURSOR_SR_FIFO,
+	SNB_CURSOR_MIN_SRWM,
 	SNB_CURSOR_MAX_SRWM,
 	SNB_CURSOR_DFT_SRWM,
 	2,
@@ -1064,6 +1085,9 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
 		wm_size = wm->max_wm;
 	if (wm_size <= 0)
 		wm_size = wm->default_wm;
+	if (wm_size < (long)wm->min_wm)
+		wm_size = wm->min_wm;
+
 	return wm_size;
 }
 
-- 
1.7.10.4


[-- 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] Added a lower limit for the watermark setting

[Daniel Vetter]

On Tue, Nov 19, 2013 at 05:15:09PM +0100, Thomas Richter wrote:
> Hi Daniel, dear intel experts,
> 
> please find a patch attached that finally addresses the display
> flicker on i830 chipsets. This
> patch adds a lower watermark setting in intel_watermark_params{},
> but keeps it zero for
> all but the i830 chipsets. The necessary new defines are in i915_reg.h.

I think we ned to split out the gen2/3 single/dual pipe watermark code a
bit better from everything else. A bugfix for i830M shouldn't really
affect snb ;-)

I've pushed out my current (and rather broken) wip branch with my idea on
the take to

http://cgit.freedesktop.org/~danvet/drm/log/?h=for-thomas

Cheers, Daniel
> 
> Greetings,
>     Thomas
> 

> From 916d8354b2134d587b946fa88c66c3d098d38df4 Mon Sep 17 00:00:00 2001
> From: Thomas Richter <thor@math.tu-berlin.de>
> Date: Tue, 19 Nov 2013 17:09:45 +0100
> Subject: [PATCH 3/3] Added a lower limit for the watermark setting.
> 
> This patch adds a minimum value for the watermark setting, in addition
> to the already existing upper limit. The lower limit is necessary on
> i830 chipsets as otherwise display flickering may result, especially
> on panning. While the flicker itself is caused by the FIFO running
> dry. Interestingly, an overrunning FIFO may also cause a hickup in
> the chipset which results in a pipeline stall with the very same
> effects.
> 
> Lower limits for all other chipsets are zero to remain consistent
> with the currently active code.
> 
> Signed-off-by: Thomas Richter <thor@math.tu-berlin.de>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |   16 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>  drivers/gpu/drm/i915/intel_pm.c  |   24 ++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 849e595..c144957 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3361,24 +3361,32 @@
>  #define I915_FIFO_SIZE		95
>  #define I855GM_FIFO_SIZE	127 /* In cachelines */
>  #define I830_FIFO_SIZE		95
> +#define I830_MIN_WM             8 /* instabilities if below this point */
>  
> +#define VALLEYVIEW_MIN_WM	0
>  #define VALLEYVIEW_MAX_WM	0xff
> +#define G4X_MIN_WM		0
>  #define G4X_MAX_WM		0x3f
> +#define I915_MIN_WM		0
>  #define I915_MAX_WM		0x3f
>  
>  #define PINEVIEW_DISPLAY_FIFO	512 /* in 64byte unit */
>  #define PINEVIEW_FIFO_LINE_SIZE	64
> +#define PINEVIEW_MIN_WM                0
>  #define PINEVIEW_MAX_WM		0x1ff
>  #define PINEVIEW_DFT_WM		0x3f
>  #define PINEVIEW_DFT_HPLLOFF_WM	0
>  #define PINEVIEW_GUARD_WM		10
>  #define PINEVIEW_CURSOR_FIFO		64
> +#define PINEVIEW_CURSOR_MIN_WM	0
>  #define PINEVIEW_CURSOR_MAX_WM	0x3f
>  #define PINEVIEW_CURSOR_DFT_WM	0
>  #define PINEVIEW_CURSOR_GUARD_WM	5
>  
>  #define VALLEYVIEW_CURSOR_MAX_WM 64
> +#define VALLEYVIEW_CURSOR_MIN_WM 0
>  #define I965_CURSOR_FIFO	64
> +#define I965_CURSOR_MIN_WM	0
>  #define I965_CURSOR_MAX_WM	32
>  #define I965_CURSOR_DFT_WM	8
>  
> @@ -3424,16 +3432,20 @@
>  
>  /* define the fifo size on Ironlake */
>  #define ILK_DISPLAY_FIFO	128
> +#define ILK_DISPLAY_MINWM	0
>  #define ILK_DISPLAY_MAXWM	64
>  #define ILK_DISPLAY_DFTWM	8
>  #define ILK_CURSOR_FIFO		32
> +#define ILK_CURSOR_MINWM        0
>  #define ILK_CURSOR_MAXWM	16
>  #define ILK_CURSOR_DFTWM	8
>  
>  #define ILK_DISPLAY_SR_FIFO	512
> +#define ILK_DISPLAY_MIN_SRWM	0
>  #define ILK_DISPLAY_MAX_SRWM	0x1ff
>  #define ILK_DISPLAY_DFT_SRWM	0x3f
>  #define ILK_CURSOR_SR_FIFO	64
> +#define ILK_CURSOR_MIN_SRWM	0
>  #define ILK_CURSOR_MAX_SRWM	0x3f
>  #define ILK_CURSOR_DFT_SRWM	8
>  
> @@ -3441,16 +3453,20 @@
>  
>  /* define the WM info on Sandybridge */
>  #define SNB_DISPLAY_FIFO	128
> +#define SNB_DISPLAY_MINWM	0
>  #define SNB_DISPLAY_MAXWM	0x7f	/* bit 16:22 */
>  #define SNB_DISPLAY_DFTWM	8
>  #define SNB_CURSOR_FIFO		32
> +#define SNB_CURSOR_MINWM	0
>  #define SNB_CURSOR_MAXWM	0x1f	/* bit 4:0 */
>  #define SNB_CURSOR_DFTWM	8
>  
>  #define SNB_DISPLAY_SR_FIFO	512
> +#define SNB_DISPLAY_MIN_SRWM	0
>  #define SNB_DISPLAY_MAX_SRWM	0x1ff	/* bit 16:8 */
>  #define SNB_DISPLAY_DFT_SRWM	0x3f
>  #define SNB_CURSOR_SR_FIFO	64
> +#define SNB_CURSOR_MIN_SRWM	0
>  #define SNB_CURSOR_MAX_SRWM	0x3f	/* bit 5:0 */
>  #define SNB_CURSOR_DFT_SRWM	8
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8e133de..9400b1a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -418,6 +418,7 @@ struct intel_plane {
>  
>  struct intel_watermark_params {
>  	unsigned long fifo_size;
> +	unsigned long min_wm;
>  	unsigned long max_wm;
>  	unsigned long default_wm;
>  	unsigned long guard_size;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9c8b374..47f6fc8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -868,6 +868,7 @@ static int i830_get_fifo_size(struct drm_device *dev, int plane)
>  /* Pineview has different values for various configs */
>  static const struct intel_watermark_params pineview_display_wm = {
>  	PINEVIEW_DISPLAY_FIFO,
> +	PINEVIEW_MIN_WM,
>  	PINEVIEW_MAX_WM,
>  	PINEVIEW_DFT_WM,
>  	PINEVIEW_GUARD_WM,
> @@ -875,6 +876,7 @@ static const struct intel_watermark_params pineview_display_wm = {
>  };
>  static const struct intel_watermark_params pineview_display_hplloff_wm = {
>  	PINEVIEW_DISPLAY_FIFO,
> +	PINEVIEW_MIN_WM,
>  	PINEVIEW_MAX_WM,
>  	PINEVIEW_DFT_HPLLOFF_WM,
>  	PINEVIEW_GUARD_WM,
> @@ -882,6 +884,7 @@ static const struct intel_watermark_params pineview_display_hplloff_wm = {
>  };
>  static const struct intel_watermark_params pineview_cursor_wm = {
>  	PINEVIEW_CURSOR_FIFO,
> +	PINEVIEW_CURSOR_MIN_WM,
>  	PINEVIEW_CURSOR_MAX_WM,
>  	PINEVIEW_CURSOR_DFT_WM,
>  	PINEVIEW_CURSOR_GUARD_WM,
> @@ -889,6 +892,7 @@ static const struct intel_watermark_params pineview_cursor_wm = {
>  };
>  static const struct intel_watermark_params pineview_cursor_hplloff_wm = {
>  	PINEVIEW_CURSOR_FIFO,
> +	PINEVIEW_CURSOR_MIN_WM,
>  	PINEVIEW_CURSOR_MAX_WM,
>  	PINEVIEW_CURSOR_DFT_WM,
>  	PINEVIEW_CURSOR_GUARD_WM,
> @@ -896,6 +900,7 @@ static const struct intel_watermark_params pineview_cursor_hplloff_wm = {
>  };
>  static const struct intel_watermark_params g4x_wm_info = {
>  	G4X_FIFO_SIZE,
> +	G4X_MIN_WM,
>  	G4X_MAX_WM,
>  	G4X_MAX_WM,
>  	2,
> @@ -903,6 +908,7 @@ static const struct intel_watermark_params g4x_wm_info = {
>  };
>  static const struct intel_watermark_params g4x_cursor_wm_info = {
>  	I965_CURSOR_FIFO,
> +	I965_CURSOR_MIN_WM,
>  	I965_CURSOR_MAX_WM,
>  	I965_CURSOR_DFT_WM,
>  	2,
> @@ -910,6 +916,7 @@ static const struct intel_watermark_params g4x_cursor_wm_info = {
>  };
>  static const struct intel_watermark_params valleyview_wm_info = {
>  	VALLEYVIEW_FIFO_SIZE,
> +	VALLEYVIEW_MIN_WM,
>  	VALLEYVIEW_MAX_WM,
>  	VALLEYVIEW_MAX_WM,
>  	2,
> @@ -917,6 +924,7 @@ static const struct intel_watermark_params valleyview_wm_info = {
>  };
>  static const struct intel_watermark_params valleyview_cursor_wm_info = {
>  	I965_CURSOR_FIFO,
> +	VALLEYVIEW_CURSOR_MIN_WM,
>  	VALLEYVIEW_CURSOR_MAX_WM,
>  	I965_CURSOR_DFT_WM,
>  	2,
> @@ -924,6 +932,7 @@ static const struct intel_watermark_params valleyview_cursor_wm_info = {
>  };
>  static const struct intel_watermark_params i965_cursor_wm_info = {
>  	I965_CURSOR_FIFO,
> +	I965_CURSOR_MIN_WM,
>  	I965_CURSOR_MAX_WM,
>  	I965_CURSOR_DFT_WM,
>  	2,
> @@ -931,6 +940,7 @@ static const struct intel_watermark_params i965_cursor_wm_info = {
>  };
>  static const struct intel_watermark_params i945_wm_info = {
>  	I945_FIFO_SIZE,
> +	I915_MIN_WM,
>  	I915_MAX_WM,
>  	1,
>  	2,
> @@ -938,6 +948,7 @@ static const struct intel_watermark_params i945_wm_info = {
>  };
>  static const struct intel_watermark_params i915_wm_info = {
>  	I915_FIFO_SIZE,
> +	I915_MIN_WM,
>  	I915_MAX_WM,
>  	1,
>  	2,
> @@ -945,6 +956,7 @@ static const struct intel_watermark_params i915_wm_info = {
>  };
>  static const struct intel_watermark_params i855_wm_info = {
>  	I855GM_FIFO_SIZE,
> +	I830_MIN_WM,
>  	I915_MAX_WM,
>  	1,
>  	2,
> @@ -952,6 +964,7 @@ static const struct intel_watermark_params i855_wm_info = {
>  };
>  static const struct intel_watermark_params i830_wm_info = {
>  	I830_FIFO_SIZE,
> +	I830_MIN_WM,
>  	I915_MAX_WM,
>  	1,
>  	2,
> @@ -960,6 +973,7 @@ static const struct intel_watermark_params i830_wm_info = {
>  
>  static const struct intel_watermark_params ironlake_display_wm_info = {
>  	ILK_DISPLAY_FIFO,
> +	ILK_DISPLAY_MINWM,
>  	ILK_DISPLAY_MAXWM,
>  	ILK_DISPLAY_DFTWM,
>  	2,
> @@ -967,6 +981,7 @@ static const struct intel_watermark_params ironlake_display_wm_info = {
>  };
>  static const struct intel_watermark_params ironlake_cursor_wm_info = {
>  	ILK_CURSOR_FIFO,
> +	ILK_CURSOR_MINWM,
>  	ILK_CURSOR_MAXWM,
>  	ILK_CURSOR_DFTWM,
>  	2,
> @@ -974,6 +989,7 @@ static const struct intel_watermark_params ironlake_cursor_wm_info = {
>  };
>  static const struct intel_watermark_params ironlake_display_srwm_info = {
>  	ILK_DISPLAY_SR_FIFO,
> +	ILK_DISPLAY_MIN_SRWM,
>  	ILK_DISPLAY_MAX_SRWM,
>  	ILK_DISPLAY_DFT_SRWM,
>  	2,
> @@ -981,6 +997,7 @@ static const struct intel_watermark_params ironlake_display_srwm_info = {
>  };
>  static const struct intel_watermark_params ironlake_cursor_srwm_info = {
>  	ILK_CURSOR_SR_FIFO,
> +	ILK_CURSOR_MIN_SRWM,
>  	ILK_CURSOR_MAX_SRWM,
>  	ILK_CURSOR_DFT_SRWM,
>  	2,
> @@ -989,6 +1006,7 @@ static const struct intel_watermark_params ironlake_cursor_srwm_info = {
>  
>  static const struct intel_watermark_params sandybridge_display_wm_info = {
>  	SNB_DISPLAY_FIFO,
> +	SNB_DISPLAY_MINWM,
>  	SNB_DISPLAY_MAXWM,
>  	SNB_DISPLAY_DFTWM,
>  	2,
> @@ -996,6 +1014,7 @@ static const struct intel_watermark_params sandybridge_display_wm_info = {
>  };
>  static const struct intel_watermark_params sandybridge_cursor_wm_info = {
>  	SNB_CURSOR_FIFO,
> +	SNB_CURSOR_MINWM,
>  	SNB_CURSOR_MAXWM,
>  	SNB_CURSOR_DFTWM,
>  	2,
> @@ -1003,6 +1022,7 @@ static const struct intel_watermark_params sandybridge_cursor_wm_info = {
>  };
>  static const struct intel_watermark_params sandybridge_display_srwm_info = {
>  	SNB_DISPLAY_SR_FIFO,
> +	SNB_DISPLAY_MIN_SRWM,
>  	SNB_DISPLAY_MAX_SRWM,
>  	SNB_DISPLAY_DFT_SRWM,
>  	2,
> @@ -1010,6 +1030,7 @@ static const struct intel_watermark_params sandybridge_display_srwm_info = {
>  };
>  static const struct intel_watermark_params sandybridge_cursor_srwm_info = {
>  	SNB_CURSOR_SR_FIFO,
> +	SNB_CURSOR_MIN_SRWM,
>  	SNB_CURSOR_MAX_SRWM,
>  	SNB_CURSOR_DFT_SRWM,
>  	2,
> @@ -1064,6 +1085,9 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
>  		wm_size = wm->max_wm;
>  	if (wm_size <= 0)
>  		wm_size = wm->default_wm;
> +	if (wm_size < (long)wm->min_wm)
> +		wm_size = wm->min_wm;
> +
>  	return wm_size;
>  }
>  
> -- 
> 1.7.10.4
> 


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

Re: [PATCH] Added a lower limit for the watermark setting

[Thomas Richter]

Am 19.11.2013 17:41, schrieb Daniel Vetter:
> On Tue, Nov 19, 2013 at 05:15:09PM +0100, Thomas Richter wrote:
>> Hi Daniel, dear intel experts,
>>
>> please find a patch attached that finally addresses the display
>> flicker on i830 chipsets. This
>> patch adds a lower watermark setting in intel_watermark_params{},
>> but keeps it zero for
>> all but the i830 chipsets. The necessary new defines are in i915_reg.h.
> I think we ned to split out the gen2/3 single/dual pipe watermark code a
> bit better from everything else. A bugfix for i830M shouldn't really
> affect snb ;-)

Actually, the fun part is that it does not because all the lower limits 
are zero for all other architectures.

>
> I've pushed out my current (and rather broken) wip branch with my idea on
> the take to
>
> http://cgit.freedesktop.org/~danvet/drm/log/?h=for-thomas

Could you please help me here how to apply it? I'm not very experienced 
with git, and it does not seem to fit to the sources of
intel_pm.c I have. Do I first need to instruct git to download another 
branch? I'm currently at drm-intel-nightly.

Thanks,
     Thomas

Re: [PATCH] Added a lower limit for the watermark setting

[Daniel Vetter]

On Wed, Nov 20, 2013 at 10:18 AM, Thomas Richter <thor@math.tu-berlin.de> wrote:
>> I think we ned to split out the gen2/3 single/dual pipe watermark code a
>>
>> bit better from everything else. A bugfix for i830M shouldn't really
>> affect snb ;-)
>
>
> Actually, the fun part is that it does not because all the lower limits are
> zero for all other architectures.

What I've meant to say is that I want to split up the watermark code
more anyway, so that there's no need to fill in the 0 all over the
place where we don't care one bit. I.e. all the gen4+ platforms.

>> I've pushed out my current (and rather broken) wip branch with my idea on
>> the take to
>>
>> http://cgit.freedesktop.org/~danvet/drm/log/?h=for-thomas
>
>
> Could you please help me here how to apply it? I'm not very experienced with
> git, and it does not seem to fit to the sources of
> intel_pm.c I have. Do I first need to instruct git to download another
> branch? I'm currently at drm-intel-nightly.

It's more just to read through the patches for ideas. Atm the branch
doesn't even compile - I'll try to clean it up and beat it into shape
in the next few days. The core idea is to have a minimal wm of 4
(lowest burst setting) and then set the actual burst setting to the
watermark rounded down to the next multiple of 4, up to the
recommended value in Bspec (which is 16 fifo cachelines). But I've
fumbled the job a bit and broke a few things ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] Added a lower limit for the watermark setting

[Thomas Richter]

Am 20.11.2013 10:27, schrieb Daniel Vetter:
> What I've meant to say is that I want to split up the watermark code
> more anyway, so that there's no need to fill in the 0 all over the
> place where we don't care one bit. I.e. all the gen4+ platforms.

Ok - well, I guess I cannot judge whether that's necessary or required. 
Given that the i830 is the only chipset
that comes with a unified FIFO, it might be actually necessary.
>
>>> I've pushed out my current (and rather broken) wip branch with my idea on
>>> the take to
>>>
>>> http://cgit.freedesktop.org/~danvet/drm/log/?h=for-thomas
>>
>> Could you please help me here how to apply it? I'm not very experienced with
>> git, and it does not seem to fit to the sources of
>> intel_pm.c I have. Do I first need to instruct git to download another
>> branch? I'm currently at drm-intel-nightly.
> It's more just to read through the patches for ideas. Atm the branch
> doesn't even compile - I'll try to clean it up and beat it into shape
> in the next few days. The core idea is to have a minimal wm of 4
> (lowest burst setting) and then set the actual burst setting to the
> watermark rounded down to the next multiple of 4, up to the
> recommended value in Bspec (which is 16 fifo cachelines). But I've
> fumbled the job a bit and broke a few things ;-)

Ah, that explains something, thank you. Probably two things: The way how the
current code is organized, the wm handling of the i830 is handled in two 
functions,
not one. In i830_update_wm and i9xx_update_wm. The former seems to be 
used during bootstrap
when only one pipe is active, the latter as soon as I activate the 
second pipe. This is probably needlessly
complex, and it would be better to have this just in one place, not two 
(found that when manually
adding a lower limit as first attempt).

The second is concerned the lower limit: Four is not enough, six gives a 
"reasonably stable image" most the time,
but if scrolling wildly, still some hickups. Eight is better, but still 
not perfect. If you try really, really hard, one can
still provoke a crash if a video overlay is active. Unclear why this is 
- if it happens, the screen seems to go blank for
just one frame, then recovers. Given that the plane pointers are only 
shadow registers that are automatically updated
by the chip on a vblank, I have no good idea why that happens.

I also see that your code seems to use a modified formula for the 
"latency". Given that I cannot compile the code, it's
at this time hard to say whether that's better or worse. The current one 
is "too pessimistic". I tried to find the minimum
permissible latency and find that for a pixel clock of 135Mhz, the 
watermark should be at most 30, and for a pixel clock
of 108 Mhz, the watermark should be at most 32. From this one can 
compute the latencies in ns. If I did everything
right, I get values of approximately 1185 ns and 1066 ns, thus 
approximately in the same ball park. A value of 1500
should likely be fine, but requires testing on additional hardware (I've 
currently no access to the Fujistu, the other laptop
with a i830 chipset and the strange DVO).

BTW, even with the maximal watermarks (minimum latency), I do get 
"hickups" if I try really really hard. So they are
likely provoked by something else, not the wrong watermark.

Thanks!

Thomas