[PATCH v2 00/12] drm/i915: Redo VLV/CHV watermark code (v2)

[PATCH v2 00/12] drm/i915: Redo VLV/CHV watermark code (v2)

[ville.syrjala]

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

Here's an updated version of the series to redo the VLV/CHV watermark
code. The most important thing is that this makes the display stable
on BSW with the fancy new memory PM features enabled by the firmware.

Changes since v1:
* Drop the two plane maxfifo mode patch (Vijay)
* Move one the misplaced hunk to the right patch (Jesse)
* Use the right amount of PFI credits for the cdclk<czclk case (Vijay)
* Drop the extra drain latency multipler frobbing as it no longer
  seems to be necessary to get a stable picture with multiple 4k
  displays. Not really sure if this was due to a BIOS update or what.
* Use plane->state->fb instead of plane->fb in the wm/ddl calculations.
* Don't break VLV maxfifo enable/disable. Noticed this one myself while
  going through the patches

I'm still missing a review on
"drm/i915: Rewrite VLV/CHV watermark code" and "drm/i915: Disable DDR DVFS on CHV"
The rest have r-bs now. I kept Jesses r-b on "drm/i915: Pass plane to
vlv_compute_drain_latency()" even though v2 is a slightly different that
v1 due to the plane->state changes.

Vidya Srinivas (1):
  drm/i915: Program PFI credits for VLV

Ville Syrjälä (11):
  drm/i915: Reduce CHV DDL multiplier to 16/8
  drm/i915: Kill DRAIN_LATENCY_PRECISION_* defines
  drm/i915: Simplify VLV drain latency computation
  drm/i915: Hide VLV DDL precision handling
  drm/i915: Reorganize VLV DDL setup
  drm/i915: Pass plane to vlv_compute_drain_latency()
  drm/i915: Read out display FIFO size on VLV/CHV
  drm/i915: Make sure PND deadline mode is enabled on VLV/CHV
  drm/i915: Rewrite VLV/CHV watermark code
  drm/i915: Enable the maxfifo PM5 mode when appropriate on CHV
  drm/i915: Disable DDR DVFS on CHV

 drivers/gpu/drm/i915/i915_drv.h      |  20 ++
 drivers/gpu/drm/i915/i915_reg.h      |  39 ++-
 drivers/gpu/drm/i915/intel_display.c |  38 +++
 drivers/gpu/drm/i915/intel_pm.c      | 577 +++++++++++++++++++++--------------
 4 files changed, 432 insertions(+), 242 deletions(-)

-- 
2.0.5

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

[PATCH v2 01/12] drm/i915: Reduce CHV DDL multiplier to 16/8

[ville.syrjala]

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

Apparently we must yet halve the DDL drain latency from what we're
using currently. This little nugget is not in any spec, but came
down through the grapevine.

This makes the displays a bit more stable. Not quite fully stable but at
least they don't fall over immediately on driver load.

v2: Update high_precision in valleyview_update_sprite_wm() too (Jesse)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4ee1964..d8a0205 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4166,6 +4166,7 @@ enum skl_disp_power_wells {
 #define   DSPFW_PLANEA_WM1_HI_MASK	(1<<0)
 
 /* drain latency register values*/
+#define DRAIN_LATENCY_PRECISION_8	8
 #define DRAIN_LATENCY_PRECISION_16	16
 #define DRAIN_LATENCY_PRECISION_32	32
 #define DRAIN_LATENCY_PRECISION_64	64
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3c64810..efbcfef 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -728,8 +728,8 @@ static bool vlv_compute_drain_latency(struct drm_crtc *crtc,
 
 	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
 	if (IS_CHERRYVIEW(dev))
-		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_32 :
-					       DRAIN_LATENCY_PRECISION_16;
+		*prec_mult = (entries > 32) ? DRAIN_LATENCY_PRECISION_16 :
+					      DRAIN_LATENCY_PRECISION_8;
 	else
 		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_64 :
 					       DRAIN_LATENCY_PRECISION_32;
@@ -759,7 +759,7 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 	enum pipe pipe = intel_crtc->pipe;
 	int plane_prec, prec_mult, plane_dl;
 	const int high_precision = IS_CHERRYVIEW(dev) ?
-		DRAIN_LATENCY_PRECISION_32 : DRAIN_LATENCY_PRECISION_64;
+		DRAIN_LATENCY_PRECISION_16 : DRAIN_LATENCY_PRECISION_64;
 
 	plane_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_PLANE_PRECISION_HIGH |
 		   DRAIN_LATENCY_MASK | DDL_CURSOR_PRECISION_HIGH |
@@ -958,7 +958,7 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
 	int sprite_dl;
 	int prec_mult;
 	const int high_precision = IS_CHERRYVIEW(dev) ?
-		DRAIN_LATENCY_PRECISION_32 : DRAIN_LATENCY_PRECISION_64;
+		DRAIN_LATENCY_PRECISION_16 : DRAIN_LATENCY_PRECISION_64;
 
 	sprite_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_SPRITE_PRECISION_HIGH(sprite) |
 		    (DRAIN_LATENCY_MASK << DDL_SPRITE_SHIFT(sprite)));
-- 
2.0.5

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

Re: [PATCH v2 01/12] drm/i915: Reduce CHV DDL multiplier to 16/8

[Arun R Murthy]

On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Apparently we must yet halve the DDL drain latency from what we're
> using currently. This little nugget is not in any spec, but came
> down through the grapevine.
>
> This makes the displays a bit more stable. Not quite fully stable but at
> least they don't fall over immediately on driver load.
>
> v2: Update high_precision in valleyview_update_sprite_wm() too (Jesse)
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h | 1 +
>   drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4ee1964..d8a0205 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4166,6 +4166,7 @@ enum skl_disp_power_wells {
>   #define   DSPFW_PLANEA_WM1_HI_MASK	(1<<0)
>   
>   /* drain latency register values*/
> +#define DRAIN_LATENCY_PRECISION_8	8
>   #define DRAIN_LATENCY_PRECISION_16	16
>   #define DRAIN_LATENCY_PRECISION_32	32
>   #define DRAIN_LATENCY_PRECISION_64	64
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3c64810..efbcfef 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -728,8 +728,8 @@ static bool vlv_compute_drain_latency(struct drm_crtc *crtc,
>   
>   	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
>   	if (IS_CHERRYVIEW(dev))
> -		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_32 :
> -					       DRAIN_LATENCY_PRECISION_16;
> +		*prec_mult = (entries > 32) ? DRAIN_LATENCY_PRECISION_16 :
> +					      DRAIN_LATENCY_PRECISION_8;
As per the spec the lower precision is "16" and not "8".
With this calculated DDL we see some flickers and hence as a temporary
solution we further divide the DDL by 2.
>   	else
>   		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_64 :
>   					       DRAIN_LATENCY_PRECISION_32;
> @@ -759,7 +759,7 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
>   	enum pipe pipe = intel_crtc->pipe;
>   	int plane_prec, prec_mult, plane_dl;
>   	const int high_precision = IS_CHERRYVIEW(dev) ?
> -		DRAIN_LATENCY_PRECISION_32 : DRAIN_LATENCY_PRECISION_64;
> +		DRAIN_LATENCY_PRECISION_16 : DRAIN_LATENCY_PRECISION_64;
The higher precision as per the spec is "32".
With this calculated DDL we see some flickers and hence as a temporary
solution we further divide the DDL by 2.
>   
>   	plane_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_PLANE_PRECISION_HIGH |
>   		   DRAIN_LATENCY_MASK | DDL_CURSOR_PRECISION_HIGH |
> @@ -958,7 +958,7 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
>   	int sprite_dl;
>   	int prec_mult;
>   	const int high_precision = IS_CHERRYVIEW(dev) ?
> -		DRAIN_LATENCY_PRECISION_32 : DRAIN_LATENCY_PRECISION_64;
> +		DRAIN_LATENCY_PRECISION_16 : DRAIN_LATENCY_PRECISION_64;
>   
>   	sprite_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_SPRITE_PRECISION_HIGH(sprite) |
>   		    (DRAIN_LATENCY_MASK << DDL_SPRITE_SHIFT(sprite)));

Thanks and Regards,
Arun R Murthy
-------------------
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 02/12] drm/i915: Kill DRAIN_LATENCY_PRECISION_* defines

[ville.syrjala]

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

Kill the silly DRAIN_LATENCY_PRECISION_* defines and just use the raw
number instead.

v2: Move the sprite 32/16 -> 16/8 preision multiplier
    change to another patch (Jesse)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  4 ----
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++--------
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d8a0205..230320b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4166,10 +4166,6 @@ enum skl_disp_power_wells {
 #define   DSPFW_PLANEA_WM1_HI_MASK	(1<<0)
 
 /* drain latency register values*/
-#define DRAIN_LATENCY_PRECISION_8	8
-#define DRAIN_LATENCY_PRECISION_16	16
-#define DRAIN_LATENCY_PRECISION_32	32
-#define DRAIN_LATENCY_PRECISION_64	64
 #define VLV_DDL(pipe)			(VLV_DISPLAY_BASE + 0x70050 + 4 * (pipe))
 #define DDL_CURSOR_PRECISION_HIGH	(1<<31)
 #define DDL_CURSOR_PRECISION_LOW	(0<<31)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index efbcfef..0f0281a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -728,11 +728,9 @@ static bool vlv_compute_drain_latency(struct drm_crtc *crtc,
 
 	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
 	if (IS_CHERRYVIEW(dev))
-		*prec_mult = (entries > 32) ? DRAIN_LATENCY_PRECISION_16 :
-					      DRAIN_LATENCY_PRECISION_8;
+		*prec_mult = (entries > 32) ? 16 : 8;
 	else
-		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_64 :
-					       DRAIN_LATENCY_PRECISION_32;
+		*prec_mult = (entries > 128) ? 64 : 32;
 	*drain_latency = (64 * (*prec_mult) * 4) / entries;
 
 	if (*drain_latency > DRAIN_LATENCY_MASK)
@@ -758,8 +756,7 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 	int drain_latency;
 	enum pipe pipe = intel_crtc->pipe;
 	int plane_prec, prec_mult, plane_dl;
-	const int high_precision = IS_CHERRYVIEW(dev) ?
-		DRAIN_LATENCY_PRECISION_16 : DRAIN_LATENCY_PRECISION_64;
+	const int high_precision = IS_CHERRYVIEW(dev) ? 16 : 64;
 
 	plane_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_PLANE_PRECISION_HIGH |
 		   DRAIN_LATENCY_MASK | DDL_CURSOR_PRECISION_HIGH |
@@ -957,8 +954,7 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
 	int plane_prec;
 	int sprite_dl;
 	int prec_mult;
-	const int high_precision = IS_CHERRYVIEW(dev) ?
-		DRAIN_LATENCY_PRECISION_16 : DRAIN_LATENCY_PRECISION_64;
+	const int high_precision = IS_CHERRYVIEW(dev) ? 16 : 64;
 
 	sprite_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_SPRITE_PRECISION_HIGH(sprite) |
 		    (DRAIN_LATENCY_MASK << DDL_SPRITE_SHIFT(sprite)));
-- 
2.0.5

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

Re: [PATCH v2 02/12] drm/i915: Kill DRAIN_LATENCY_PRECISION_* defines

[Arun R Murthy]

On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Kill the silly DRAIN_LATENCY_PRECISION_* defines and just use the raw
> number instead.
>
> v2: Move the sprite 32/16 -> 16/8 preision multiplier
>      change to another patch (Jesse)

Can this be merged with the previous patch?
Also the comments on the patch "[PATCH v2 01/12] drm/i915:
Reduce CHV DDL multiplier to 16/8" holds good here also.

Thanks and Regards,
Arun R Murthy
-------------------

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

Re: [PATCH v2 02/12] drm/i915: Kill DRAIN_LATENCY_PRECISION_* defines

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 09:18:37AM +0530, Arun R Murthy wrote:
> 
> On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Kill the silly DRAIN_LATENCY_PRECISION_* defines and just use the raw
> > number instead.
> >
> > v2: Move the sprite 32/16 -> 16/8 preision multiplier
> >      change to another patch (Jesse)
> 
> Can this be merged with the previous patch?

Could be, but since it's purely a cosmetic change I think having it
separate keeps the other patch a bit cleaner.

> Also the comments on the patch "[PATCH v2 01/12] drm/i915:
> Reduce CHV DDL multiplier to 16/8" holds good here also.
> 
> Thanks and Regards,
> Arun R Murthy
> -------------------

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

[PATCH 03/12] drm/i915: Simplify VLV drain latency computation

[ville.syrjala]

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

The current drain lantency computation relies on hardcoded limits to
determine when the to use the low vs. high precision multiplier.
Rewrite the code to use a more straightforward approach.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0f0281a..d6c6c1b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -727,12 +727,15 @@ static bool vlv_compute_drain_latency(struct drm_crtc *crtc,
 		return false;
 
 	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
-	if (IS_CHERRYVIEW(dev))
-		*prec_mult = (entries > 32) ? 16 : 8;
-	else
-		*prec_mult = (entries > 128) ? 64 : 32;
+
+	*prec_mult = IS_CHERRYVIEW(dev) ? 16 : 64;
 	*drain_latency = (64 * (*prec_mult) * 4) / entries;
 
+	if (*drain_latency > DRAIN_LATENCY_MASK) {
+		*prec_mult /= 2;
+		*drain_latency = (64 * (*prec_mult) * 4) / entries;
+	}
+
 	if (*drain_latency > DRAIN_LATENCY_MASK)
 		*drain_latency = DRAIN_LATENCY_MASK;
 
-- 
2.0.5

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

[PATCH 04/12] drm/i915: Hide VLV DDL precision handling

[ville.syrjala]

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

Move the DDL precision handling into vlv_compute_drain_latency() so the
callers don't have to duplicate the same code to deal with it.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  8 ++---
 drivers/gpu/drm/i915/intel_pm.c | 74 +++++++++++++++--------------------------
 2 files changed, 28 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 230320b..b35aaf3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4167,15 +4167,11 @@ enum skl_disp_power_wells {
 
 /* drain latency register values*/
 #define VLV_DDL(pipe)			(VLV_DISPLAY_BASE + 0x70050 + 4 * (pipe))
-#define DDL_CURSOR_PRECISION_HIGH	(1<<31)
-#define DDL_CURSOR_PRECISION_LOW	(0<<31)
 #define DDL_CURSOR_SHIFT		24
-#define DDL_SPRITE_PRECISION_HIGH(sprite)	(1<<(15+8*(sprite)))
-#define DDL_SPRITE_PRECISION_LOW(sprite)	(0<<(15+8*(sprite)))
 #define DDL_SPRITE_SHIFT(sprite)	(8+8*(sprite))
-#define DDL_PLANE_PRECISION_HIGH	(1<<7)
-#define DDL_PLANE_PRECISION_LOW		(0<<7)
 #define DDL_PLANE_SHIFT			0
+#define DDL_PRECISION_HIGH		(1<<7)
+#define DDL_PRECISION_LOW		(0<<7)
 #define DRAIN_LATENCY_MASK		0x7f
 
 /* FIFO watermark sizes etc */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d6c6c1b..c4c2317 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -711,35 +711,35 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 			      display, cursor);
 }
 
-static bool vlv_compute_drain_latency(struct drm_crtc *crtc,
-				      int pixel_size,
-				      int *prec_mult,
-				      int *drain_latency)
+static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
+					 int pixel_size)
 {
 	struct drm_device *dev = crtc->dev;
-	int entries;
+	int entries, prec_mult, drain_latency;
 	int clock = to_intel_crtc(crtc)->config->base.adjusted_mode.crtc_clock;
+	const int high_precision = IS_CHERRYVIEW(dev) ? 16 : 64;
 
 	if (WARN(clock == 0, "Pixel clock is zero!\n"))
-		return false;
+		return 0;
 
 	if (WARN(pixel_size == 0, "Pixel size is zero!\n"))
-		return false;
+		return 0;
 
 	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
 
-	*prec_mult = IS_CHERRYVIEW(dev) ? 16 : 64;
-	*drain_latency = (64 * (*prec_mult) * 4) / entries;
+	prec_mult = high_precision;
+	drain_latency = 64 * prec_mult * 4 / entries;
 
-	if (*drain_latency > DRAIN_LATENCY_MASK) {
-		*prec_mult /= 2;
-		*drain_latency = (64 * (*prec_mult) * 4) / entries;
+	if (drain_latency > DRAIN_LATENCY_MASK) {
+		prec_mult /= 2;
+		drain_latency = 64 * prec_mult * 4 / entries;
 	}
 
-	if (*drain_latency > DRAIN_LATENCY_MASK)
-		*drain_latency = DRAIN_LATENCY_MASK;
+	if (drain_latency > DRAIN_LATENCY_MASK)
+		drain_latency = DRAIN_LATENCY_MASK;
 
-	return true;
+	return drain_latency | (prec_mult == high_precision ?
+				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
 }
 
 /*
@@ -756,14 +756,12 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pixel_size;
-	int drain_latency;
 	enum pipe pipe = intel_crtc->pipe;
-	int plane_prec, prec_mult, plane_dl;
-	const int high_precision = IS_CHERRYVIEW(dev) ? 16 : 64;
+	int plane_dl;
 
-	plane_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_PLANE_PRECISION_HIGH |
-		   DRAIN_LATENCY_MASK | DDL_CURSOR_PRECISION_HIGH |
-		   (DRAIN_LATENCY_MASK << DDL_CURSOR_SHIFT));
+	plane_dl = I915_READ(VLV_DDL(pipe)) &
+		~(((DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK) << DDL_CURSOR_SHIFT) |
+		  ((DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK) << DDL_PLANE_SHIFT));
 
 	if (!intel_crtc_active(crtc)) {
 		I915_WRITE(VLV_DDL(pipe), plane_dl);
@@ -772,12 +770,7 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 
 	/* Primary plane Drain Latency */
 	pixel_size = crtc->primary->fb->bits_per_pixel / 8;	/* BPP */
-	if (vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, &drain_latency)) {
-		plane_prec = (prec_mult == high_precision) ?
-					   DDL_PLANE_PRECISION_HIGH :
-					   DDL_PLANE_PRECISION_LOW;
-		plane_dl |= plane_prec | drain_latency;
-	}
+	plane_dl = vlv_compute_drain_latency(crtc, pixel_size) << DDL_PLANE_SHIFT;
 
 	/* Cursor Drain Latency
 	 * BPP is always 4 for cursor
@@ -785,13 +778,8 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 	pixel_size = 4;
 
 	/* Program cursor DL only if it is enabled */
-	if (intel_crtc->cursor_base &&
-	    vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, &drain_latency)) {
-		plane_prec = (prec_mult == high_precision) ?
-					   DDL_CURSOR_PRECISION_HIGH :
-					   DDL_CURSOR_PRECISION_LOW;
-		plane_dl |= plane_prec | (drain_latency << DDL_CURSOR_SHIFT);
-	}
+	if (intel_crtc->cursor_base)
+		plane_dl |= vlv_compute_drain_latency(crtc, pixel_size) << DDL_CURSOR_SHIFT;
 
 	I915_WRITE(VLV_DDL(pipe), plane_dl);
 }
@@ -953,23 +941,13 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe = to_intel_plane(plane)->pipe;
 	int sprite = to_intel_plane(plane)->plane;
-	int drain_latency;
-	int plane_prec;
 	int sprite_dl;
-	int prec_mult;
-	const int high_precision = IS_CHERRYVIEW(dev) ? 16 : 64;
 
-	sprite_dl = I915_READ(VLV_DDL(pipe)) & ~(DDL_SPRITE_PRECISION_HIGH(sprite) |
-		    (DRAIN_LATENCY_MASK << DDL_SPRITE_SHIFT(sprite)));
+	sprite_dl = I915_READ(VLV_DDL(pipe)) &
+		~((DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK) << DDL_SPRITE_SHIFT(sprite));
 
-	if (enabled && vlv_compute_drain_latency(crtc, pixel_size, &prec_mult,
-						 &drain_latency)) {
-		plane_prec = (prec_mult == high_precision) ?
-					   DDL_SPRITE_PRECISION_HIGH(sprite) :
-					   DDL_SPRITE_PRECISION_LOW(sprite);
-		sprite_dl |= plane_prec |
-			     (drain_latency << DDL_SPRITE_SHIFT(sprite));
-	}
+	if (enabled)
+		sprite_dl |= vlv_compute_drain_latency(crtc, pixel_size) << DDL_SPRITE_SHIFT(sprite);
 
 	I915_WRITE(VLV_DDL(pipe), sprite_dl);
 }
-- 
2.0.5

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

Re: [PATCH 04/12] drm/i915: Hide VLV DDL precision handling

[Arun R Murthy]

On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Move the DDL precision handling into vlv_compute_drain_latency() so the
> callers don't have to duplicate the same code to deal with it.
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,
Arun R Murthy
-------------------

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

[PATCH 05/12] drm/i915: Reorganize VLV DDL setup

[ville.syrjala]

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

Introduce struct vlv_wm_values to house VLV watermark/drain latency
values. We start by using it when computing the drain latency values.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  9 ++++++++
 drivers/gpu/drm/i915/intel_pm.c | 46 +++++++++++++++++++++++++++--------------
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 26ffe8b..5de69a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1514,6 +1514,14 @@ struct ilk_wm_values {
 	enum intel_ddb_partitioning partitioning;
 };
 
+struct vlv_wm_values {
+	struct {
+		uint8_t cursor;
+		uint8_t sprite[2];
+		uint8_t primary;
+	} ddl[3];
+};
+
 struct skl_ddb_entry {
 	uint16_t start, end;	/* in number of blocks, 'end' is exclusive */
 };
@@ -1870,6 +1878,7 @@ struct drm_i915_private {
 		union {
 			struct ilk_wm_values hw;
 			struct skl_wm_values skl_hw;
+			struct vlv_wm_values vlv;
 		};
 	} wm;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c4c2317..5515d10 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -711,6 +711,21 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 			      display, cursor);
 }
 
+static void vlv_write_wm_values(struct intel_crtc *crtc,
+				const struct vlv_wm_values *wm)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+
+	I915_WRITE(VLV_DDL(pipe),
+		   (wm->ddl[pipe].cursor << DDL_CURSOR_SHIFT) |
+		   (wm->ddl[pipe].sprite[1] << DDL_SPRITE_SHIFT(1)) |
+		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
+		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
+
+	dev_priv->wm.vlv = *wm;
+}
+
 static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
 					 int pixel_size)
 {
@@ -757,20 +772,19 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pixel_size;
 	enum pipe pipe = intel_crtc->pipe;
-	int plane_dl;
+	struct vlv_wm_values wm = dev_priv->wm.vlv;
 
-	plane_dl = I915_READ(VLV_DDL(pipe)) &
-		~(((DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK) << DDL_CURSOR_SHIFT) |
-		  ((DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK) << DDL_PLANE_SHIFT));
+	wm.ddl[pipe].primary = 0;
+	wm.ddl[pipe].cursor = 0;
 
 	if (!intel_crtc_active(crtc)) {
-		I915_WRITE(VLV_DDL(pipe), plane_dl);
+		vlv_write_wm_values(intel_crtc, &wm);
 		return;
 	}
 
 	/* Primary plane Drain Latency */
 	pixel_size = crtc->primary->fb->bits_per_pixel / 8;	/* BPP */
-	plane_dl = vlv_compute_drain_latency(crtc, pixel_size) << DDL_PLANE_SHIFT;
+	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, pixel_size);
 
 	/* Cursor Drain Latency
 	 * BPP is always 4 for cursor
@@ -779,9 +793,10 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 
 	/* Program cursor DL only if it is enabled */
 	if (intel_crtc->cursor_base)
-		plane_dl |= vlv_compute_drain_latency(crtc, pixel_size) << DDL_CURSOR_SHIFT;
+		wm.ddl[pipe].cursor =
+			vlv_compute_drain_latency(crtc, pixel_size);
 
-	I915_WRITE(VLV_DDL(pipe), plane_dl);
+	vlv_write_wm_values(intel_crtc, &wm);
 }
 
 #define single_plane_enabled(mask) is_power_of_2(mask)
@@ -939,17 +954,18 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe = to_intel_plane(plane)->pipe;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	int sprite = to_intel_plane(plane)->plane;
-	int sprite_dl;
-
-	sprite_dl = I915_READ(VLV_DDL(pipe)) &
-		~((DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK) << DDL_SPRITE_SHIFT(sprite));
+	struct vlv_wm_values wm = dev_priv->wm.vlv;
 
 	if (enabled)
-		sprite_dl |= vlv_compute_drain_latency(crtc, pixel_size) << DDL_SPRITE_SHIFT(sprite);
+		wm.ddl[pipe].sprite[sprite] =
+			vlv_compute_drain_latency(crtc, pixel_size);
+	else
+		wm.ddl[pipe].sprite[sprite] = 0;
 
-	I915_WRITE(VLV_DDL(pipe), sprite_dl);
+	vlv_write_wm_values(intel_crtc, &wm);
 }
 
 static void g4x_update_wm(struct drm_crtc *crtc)
-- 
2.0.5

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

[PATCH v2 06/12] drm/i915: Pass plane to vlv_compute_drain_latency()

[ville.syrjala]

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

Now that we have drm_planes for the cursor and primary we can move the
pixel_size handling into vlv_compute_drain_latency() and just pass the
appropriate plane to it.

v2: Check plane->state->fb instead of plane->fb

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 42 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5515d10..f8f6cee 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -727,16 +727,26 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 }
 
 static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
-					 int pixel_size)
+					 struct drm_plane *plane)
 {
 	struct drm_device *dev = crtc->dev;
-	int entries, prec_mult, drain_latency;
-	int clock = to_intel_crtc(crtc)->config->base.adjusted_mode.crtc_clock;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int entries, prec_mult, drain_latency, pixel_size;
+	int clock = intel_crtc->config->base.adjusted_mode.crtc_clock;
 	const int high_precision = IS_CHERRYVIEW(dev) ? 16 : 64;
 
+	/*
+	 * FIXME the plane might have an fb
+	 * but be invisible (eg. due to clipping)
+	 */
+	if (!intel_crtc->active || !plane->state->fb)
+		return 0;
+
 	if (WARN(clock == 0, "Pixel clock is zero!\n"))
 		return 0;
 
+	pixel_size = drm_format_plane_cpp(plane->state->fb->pixel_format, 0);
+
 	if (WARN(pixel_size == 0, "Pixel size is zero!\n"))
 		return 0;
 
@@ -770,31 +780,11 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pixel_size;
 	enum pipe pipe = intel_crtc->pipe;
 	struct vlv_wm_values wm = dev_priv->wm.vlv;
 
-	wm.ddl[pipe].primary = 0;
-	wm.ddl[pipe].cursor = 0;
-
-	if (!intel_crtc_active(crtc)) {
-		vlv_write_wm_values(intel_crtc, &wm);
-		return;
-	}
-
-	/* Primary plane Drain Latency */
-	pixel_size = crtc->primary->fb->bits_per_pixel / 8;	/* BPP */
-	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, pixel_size);
-
-	/* Cursor Drain Latency
-	 * BPP is always 4 for cursor
-	 */
-	pixel_size = 4;
-
-	/* Program cursor DL only if it is enabled */
-	if (intel_crtc->cursor_base)
-		wm.ddl[pipe].cursor =
-			vlv_compute_drain_latency(crtc, pixel_size);
+	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
+	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
 
 	vlv_write_wm_values(intel_crtc, &wm);
 }
@@ -961,7 +951,7 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
 
 	if (enabled)
 		wm.ddl[pipe].sprite[sprite] =
-			vlv_compute_drain_latency(crtc, pixel_size);
+			vlv_compute_drain_latency(crtc, plane);
 	else
 		wm.ddl[pipe].sprite[sprite] = 0;
 
-- 
2.0.5

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

[PATCH v2 07/12] drm/i915: Read out display FIFO size on VLV/CHV

[ville.syrjala]

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

VLV/CHV have similar DSPARB registers as older platforms, just more of
them due to more planes. Add a bit of code to read out the current FIFO
split from the registers. Will be useful later when we improve the WM
calculations.

v2: Add display_mmio_offset to DSPARB

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  5 +++-
 drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b35aaf3..3b48f4b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4026,7 +4026,7 @@ enum skl_disp_power_wells {
 #define   DPINVGTT_STATUS_MASK			0xff
 #define   DPINVGTT_STATUS_MASK_CHV		0xfff
 
-#define DSPARB			0x70030
+#define DSPARB			(dev_priv->info.display_mmio_offset + 0x70030)
 #define   DSPARB_CSTART_MASK	(0x7f << 7)
 #define   DSPARB_CSTART_SHIFT	7
 #define   DSPARB_BSTART_MASK	(0x7f)
@@ -4034,6 +4034,9 @@ enum skl_disp_power_wells {
 #define   DSPARB_BEND_SHIFT	9 /* on 855 */
 #define   DSPARB_AEND_SHIFT	0
 
+#define DSPARB2			(VLV_DISPLAY_BASE + 0x70060) /* vlv/chv */
+#define DSPARB3			(VLV_DISPLAY_BASE + 0x7006c) /* chv */
+
 /* pnv/gen4/g4x/vlv/chv */
 #define DSPFW1			(dev_priv->info.display_mmio_offset + 0x70034)
 #define   DSPFW_SR_SHIFT		23
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f8f6cee..27ce40c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -280,6 +280,61 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
  */
 static const int pessimal_latency_ns = 5000;
 
+#define VLV_FIFO_START(dsparb, dsparb2, lo_shift, hi_shift) \
+	((((dsparb) >> (lo_shift)) & 0xff) | ((((dsparb2) >> (hi_shift)) & 0x1) << 8))
+
+static int vlv_get_fifo_size(struct drm_device *dev,
+			      enum pipe pipe, int plane)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int sprite0_start, sprite1_start, size;
+
+	switch (pipe) {
+		uint32_t dsparb, dsparb2, dsparb3;
+	case PIPE_A:
+		dsparb = I915_READ(DSPARB);
+		dsparb2 = I915_READ(DSPARB2);
+		sprite0_start = VLV_FIFO_START(dsparb, dsparb2, 0, 0);
+		sprite1_start = VLV_FIFO_START(dsparb, dsparb2, 8, 4);
+		break;
+	case PIPE_B:
+		dsparb = I915_READ(DSPARB);
+		dsparb2 = I915_READ(DSPARB2);
+		sprite0_start = VLV_FIFO_START(dsparb, dsparb2, 16, 8);
+		sprite1_start = VLV_FIFO_START(dsparb, dsparb2, 24, 12);
+		break;
+	case PIPE_C:
+		dsparb2 = I915_READ(DSPARB2);
+		dsparb3 = I915_READ(DSPARB3);
+		sprite0_start = VLV_FIFO_START(dsparb3, dsparb2, 0, 16);
+		sprite1_start = VLV_FIFO_START(dsparb3, dsparb2, 8, 20);
+		break;
+	default:
+		return 0;
+	}
+
+	switch (plane) {
+	case 0:
+		size = sprite0_start;
+		break;
+	case 1:
+		size = sprite1_start - sprite0_start;
+		break;
+	case 2:
+		size = 512 - 1 - sprite1_start;
+		break;
+	default:
+		return 0;
+	}
+
+	DRM_DEBUG_KMS("Pipe %c %s %c FIFO size: %d\n",
+		      pipe_name(pipe), plane == 0 ? "primary" : "sprite",
+		      plane == 0 ? plane_name(pipe) : sprite_name(pipe, plane - 1),
+		      size);
+
+	return size;
+}
+
 static int i9xx_get_fifo_size(struct drm_device *dev, int plane)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
2.0.5

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

[PATCH 08/12] drm/i915: Make sure PND deadline mode is enabled on VLV/CHV

[ville.syrjala]

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

Poke at the CBR1_VLV register during init_clock_gating to make sure the
PND deadline scheme is used.

The hardware has two modes of operation wrt. watermarks:

1) PND deadline mode:
 - memory request deadline is calculated from actual FIFO level * DDL
 - WM1 watermark values are unused (AFAIK)
 - WM watermark level defines when to start fetching data from memory
   (assuming trickle feed is not used)

2) backup mode
 - deadline is based on FIFO status, DDL is unused
 - FIFO split into three regions with WM and WM1 watermarks, each
   part specifying a different FIFO status

We want to use the PND deadline mode, so let's make sure the chicken
bit is in the correct position on init.

Also take the opportunity to refactor the shared code between VLV and
CHV to a shared function.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  3 +++
 drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3b48f4b..8178610 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4177,6 +4177,9 @@ enum skl_disp_power_wells {
 #define DDL_PRECISION_LOW		(0<<7)
 #define DRAIN_LATENCY_MASK		0x7f
 
+#define CBR1_VLV			(VLV_DISPLAY_BASE + 0x70400)
+#define  CBR_PND_DEADLINE_DISABLE	(1<<31)
+
 /* FIFO watermark sizes etc */
 #define G4X_FIFO_LINE_SIZE	64
 #define I915_FIFO_LINE_SIZE	64
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27ce40c..bdb0f5d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6197,11 +6197,22 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
 	gen6_check_mch_setup(dev);
 }
 
+static void vlv_init_display_clock_gating(struct drm_i915_private *dev_priv)
+{
+	I915_WRITE(DSPCLK_GATE_D, VRHUNIT_CLOCK_GATE_DISABLE);
+
+	/*
+	 * Disable trickle feed and enable pnd deadline calculation
+	 */
+	I915_WRITE(MI_ARB_VLV, MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
+	I915_WRITE(CBR1_VLV, 0);
+}
+
 static void valleyview_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(DSPCLK_GATE_D, VRHUNIT_CLOCK_GATE_DISABLE);
+	vlv_init_display_clock_gating(dev_priv);
 
 	/* WaDisableEarlyCull:vlv */
 	I915_WRITE(_3D_CHICKEN3,
@@ -6249,8 +6260,6 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(GEN7_UCGCTL4,
 		   I915_READ(GEN7_UCGCTL4) | GEN7_L3BANK2X_CLOCK_GATE_DISABLE);
 
-	I915_WRITE(MI_ARB_VLV, MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
-
 	/*
 	 * BSpec says this must be set, even though
 	 * WaDisable4x2SubspanOptimization isn't listed for VLV.
@@ -6287,9 +6296,7 @@ static void cherryview_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(DSPCLK_GATE_D, VRHUNIT_CLOCK_GATE_DISABLE);
-
-	I915_WRITE(MI_ARB_VLV, MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
+	vlv_init_display_clock_gating(dev_priv);
 
 	/* WaVSRefCountFullforceMissDisable:chv */
 	/* WaDSRefCountFullforceMissDisable:chv */
-- 
2.0.5

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

Re: [PATCH 08/12] drm/i915: Make sure PND deadline mode is enabled on VLV/CHV

[Daniel Vetter]

On Thu, Mar 05, 2015 at 09:19:48PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Poke at the CBR1_VLV register during init_clock_gating to make sure the
> PND deadline scheme is used.
> 
> The hardware has two modes of operation wrt. watermarks:
> 
> 1) PND deadline mode:
>  - memory request deadline is calculated from actual FIFO level * DDL
>  - WM1 watermark values are unused (AFAIK)
>  - WM watermark level defines when to start fetching data from memory
>    (assuming trickle feed is not used)
> 
> 2) backup mode
>  - deadline is based on FIFO status, DDL is unused
>  - FIFO split into three regions with WM and WM1 watermarks, each
>    part specifying a different FIFO status
> 
> We want to use the PND deadline mode, so let's make sure the chicken
> bit is in the correct position on init.
> 
> Also take the opportunity to refactor the shared code between VLV and
> CHV to a shared function.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged up to this one (had to resolve some minor conflicts with the
ongoing stateification work from Matt).

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_reg.h |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++------
>  2 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3b48f4b..8178610 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4177,6 +4177,9 @@ enum skl_disp_power_wells {
>  #define DDL_PRECISION_LOW		(0<<7)
>  #define DRAIN_LATENCY_MASK		0x7f
>  
> +#define CBR1_VLV			(VLV_DISPLAY_BASE + 0x70400)
> +#define  CBR_PND_DEADLINE_DISABLE	(1<<31)
> +
>  /* FIFO watermark sizes etc */
>  #define G4X_FIFO_LINE_SIZE	64
>  #define I915_FIFO_LINE_SIZE	64
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 27ce40c..bdb0f5d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6197,11 +6197,22 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
>  	gen6_check_mch_setup(dev);
>  }
>  
> +static void vlv_init_display_clock_gating(struct drm_i915_private *dev_priv)
> +{
> +	I915_WRITE(DSPCLK_GATE_D, VRHUNIT_CLOCK_GATE_DISABLE);
> +
> +	/*
> +	 * Disable trickle feed and enable pnd deadline calculation
> +	 */
> +	I915_WRITE(MI_ARB_VLV, MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
> +	I915_WRITE(CBR1_VLV, 0);
> +}
> +
>  static void valleyview_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	I915_WRITE(DSPCLK_GATE_D, VRHUNIT_CLOCK_GATE_DISABLE);
> +	vlv_init_display_clock_gating(dev_priv);
>  
>  	/* WaDisableEarlyCull:vlv */
>  	I915_WRITE(_3D_CHICKEN3,
> @@ -6249,8 +6260,6 @@ static void valleyview_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(GEN7_UCGCTL4,
>  		   I915_READ(GEN7_UCGCTL4) | GEN7_L3BANK2X_CLOCK_GATE_DISABLE);
>  
> -	I915_WRITE(MI_ARB_VLV, MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
> -
>  	/*
>  	 * BSpec says this must be set, even though
>  	 * WaDisable4x2SubspanOptimization isn't listed for VLV.
> @@ -6287,9 +6296,7 @@ static void cherryview_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	I915_WRITE(DSPCLK_GATE_D, VRHUNIT_CLOCK_GATE_DISABLE);
> -
> -	I915_WRITE(MI_ARB_VLV, MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
> +	vlv_init_display_clock_gating(dev_priv);
>  
>  	/* WaVSRefCountFullforceMissDisable:chv */
>  	/* WaDSRefCountFullforceMissDisable:chv */
> -- 
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[ville.syrjala]

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

Assuming the PND deadline mechanism works reasonably we should do
memory requests as early as possible so that PND has schedule the
requests more intelligently. Currently we're still calculating
the watermarks as if VLV/CHV are identical to g4x, which isn't
the case.

The current code also seems to calculate insufficient watermarks
and hence we're seeing some underruns, especially on high resolution
displays.

To fix it just rip out the current code and replace is with something
that tries to utilize PND as efficiently as possible.

We now calculate the WM watermark to trigger when the FIFO still has
256us worth of data. 256us is the maximum deadline value supoorted by
PND, so issuing memory requests earlier would mean we probably couldn't
utilize the full FIFO as PND would attempt to return the data at
least in at least 256us. We also clamp the watermark to at least 8
cachelines as that's the magic watermark that enabling trickle feed
would also impose. I'm assuming it matches some burst size.

In theory we could just enable trickle feed and ignore the WM values,
except trickle feed doesn't work with max fifo mode anyway, so we'd
still need to calculate the SR watermarks. It seems cleaner to just
disable trickle feed and calculate all watermarks the same way. Also
trickle feed wouldn't account for the 256us max deadline value, thoguh
that may be a moot point in non-max fifo mode sicne the FIFOs are fairly
small.

On VLV max fifo mode can be used with either primary or sprite planes.
So the code now also checks all the planes (apart from the cursor)
when calculating the SR plane watermark.

We don't have to worry about the WM1 watermarks since we're using the
PND deadline scheme which means the hardware ignores WM1 values.

v2: Use plane->state->fb instead of plane->fb

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  11 ++
 drivers/gpu/drm/i915/i915_reg.h |   4 +-
 drivers/gpu/drm/i915/intel_pm.c | 330 +++++++++++++++++++++-------------------
 3 files changed, 188 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5de69a0..b191b12 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1516,6 +1516,17 @@ struct ilk_wm_values {
 
 struct vlv_wm_values {
 	struct {
+		uint16_t primary;
+		uint16_t sprite[2];
+		uint8_t cursor;
+	} pipe[3];
+
+	struct {
+		uint16_t plane;
+		uint8_t cursor;
+	} sr;
+
+	struct {
 		uint8_t cursor;
 		uint8_t sprite[2];
 		uint8_t primary;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8178610..9f98384 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4127,7 +4127,7 @@ enum skl_disp_power_wells {
 /* vlv/chv high order bits */
 #define DSPHOWM			(VLV_DISPLAY_BASE + 0x70064)
 #define   DSPFW_SR_HI_SHIFT		24
-#define   DSPFW_SR_HI_MASK		(1<<24)
+#define   DSPFW_SR_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
 #define   DSPFW_SPRITEF_HI_SHIFT	23
 #define   DSPFW_SPRITEF_HI_MASK		(1<<23)
 #define   DSPFW_SPRITEE_HI_SHIFT	22
@@ -4148,7 +4148,7 @@ enum skl_disp_power_wells {
 #define   DSPFW_PLANEA_HI_MASK		(1<<0)
 #define DSPHOWM1		(VLV_DISPLAY_BASE + 0x70068)
 #define   DSPFW_SR_WM1_HI_SHIFT		24
-#define   DSPFW_SR_WM1_HI_MASK		(1<<24)
+#define   DSPFW_SR_WM1_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
 #define   DSPFW_SPRITEF_WM1_HI_SHIFT	23
 #define   DSPFW_SPRITEF_WM1_HI_MASK	(1<<23)
 #define   DSPFW_SPRITEE_WM1_HI_SHIFT	22
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bdb0f5d..497847c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -778,6 +778,55 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
 		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
 
+	I915_WRITE(DSPFW1,
+		   ((wm->sr.plane << DSPFW_SR_SHIFT) & DSPFW_SR_MASK) |
+		   ((wm->pipe[PIPE_B].cursor << DSPFW_CURSORB_SHIFT) & DSPFW_CURSORB_MASK) |
+		   ((wm->pipe[PIPE_B].primary << DSPFW_PLANEB_SHIFT) & DSPFW_PLANEB_MASK_VLV) |
+		   ((wm->pipe[PIPE_A].primary << DSPFW_PLANEA_SHIFT) & DSPFW_PLANEA_MASK_VLV));
+	I915_WRITE(DSPFW2,
+		   ((wm->pipe[PIPE_A].sprite[1] << DSPFW_SPRITEB_SHIFT) & DSPFW_SPRITEB_MASK_VLV) |
+		   ((wm->pipe[PIPE_A].cursor << DSPFW_CURSORA_SHIFT) & DSPFW_CURSORA_MASK) |
+		   ((wm->pipe[PIPE_A].sprite[0] << DSPFW_SPRITEA_SHIFT) & DSPFW_SPRITEA_MASK_VLV));
+	I915_WRITE(DSPFW3,
+		   ((wm->sr.cursor << DSPFW_CURSOR_SR_SHIFT) & DSPFW_CURSOR_SR_MASK));
+
+	if (IS_CHERRYVIEW(dev_priv)) {
+		I915_WRITE(DSPFW7_CHV,
+			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
+			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
+		I915_WRITE(DSPFW8_CHV,
+			   ((wm->pipe[PIPE_C].sprite[1] << DSPFW_SPRITEF_SHIFT) & DSPFW_SPRITEF_MASK) |
+			   ((wm->pipe[PIPE_C].sprite[0] << DSPFW_SPRITEE_SHIFT) & DSPFW_SPRITEE_MASK));
+		I915_WRITE(DSPFW9_CHV,
+			   ((wm->pipe[PIPE_C].primary << DSPFW_PLANEC_SHIFT) & DSPFW_PLANEC_MASK) |
+			   ((wm->pipe[PIPE_C].cursor << DSPFW_CURSORC_SHIFT) & DSPFW_CURSORC_MASK));
+		I915_WRITE(DSPHOWM,
+			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
+			   (((wm->pipe[PIPE_C].sprite[1] >> 8) << DSPFW_SPRITEF_HI_SHIFT) & DSPFW_SPRITEF_HI_MASK) |
+			   (((wm->pipe[PIPE_C].sprite[0] >> 8) << DSPFW_SPRITEE_HI_SHIFT) & DSPFW_SPRITEE_HI_MASK) |
+			   (((wm->pipe[PIPE_C].primary >> 8) << DSPFW_PLANEC_HI_SHIFT) & DSPFW_PLANEC_HI_MASK) |
+			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
+			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
+			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
+			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
+			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
+			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
+	} else {
+		I915_WRITE(DSPFW7,
+			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
+			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
+		I915_WRITE(DSPHOWM,
+			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
+			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
+			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
+			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
+			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
+			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
+			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
+	}
+
+	POSTING_READ(DSPFW1);
+
 	dev_priv->wm.vlv = *wm;
 }
 
@@ -822,169 +871,113 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
 				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
 }
 
-/*
- * Update drain latency registers of memory arbiter
- *
- * Valleyview SoC has a new memory arbiter and needs drain latency registers
- * to be programmed. Each plane has a drain latency multiplier and a drain
- * latency value.
- */
-
-static void vlv_update_drain_latency(struct drm_crtc *crtc)
+static int vlv_compute_wm(struct intel_crtc *crtc,
+			  struct intel_plane *plane,
+			  int fifo_size)
+{
+	int clock, entries, pixel_size;
+
+	/*
+	 * FIXME the plane might have an fb
+	 * but be invisible (eg. due to clipping)
+	 */
+	if (!crtc->active || !plane->base.state->fb)
+		return 0;
+
+	pixel_size = drm_format_plane_cpp(plane->base.state->fb->pixel_format, 0);
+	clock = crtc->config->base.adjusted_mode.crtc_clock;
+
+	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
+
+	/*
+	 * Set up the watermark such that we don't start issuing memory
+	 * requests until we are within PND's max deadline value (256us).
+	 * Idea being to be idle as long as possible while still taking
+	 * advatange of PND's deadline scheduling. The limit of 8
+	 * cachelines (used when the FIFO will anyway drain in less time
+	 * than 256us) should match what we would be done if trickle
+	 * feed were enabled.
+	 */
+	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
+}
+
+static bool vlv_compute_sr_wm(struct drm_device *dev,
+			      struct vlv_wm_values *wm)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc *crtc;
+	enum pipe pipe = INVALID_PIPE;
+	int num_planes = 0;
+	int fifo_size = 0;
+	struct intel_plane *plane;
+
+	wm->sr.cursor = wm->sr.plane = 0;
+
+	crtc = single_enabled_crtc(dev);
+	/* maxfifo not supported on pipe C */
+	if (crtc && to_intel_crtc(crtc)->pipe != PIPE_C) {
+		pipe = to_intel_crtc(crtc)->pipe;
+		num_planes = !!wm->pipe[pipe].primary +
+			!!wm->pipe[pipe].sprite[0] +
+			!!wm->pipe[pipe].sprite[1];
+		fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
+	}
+
+	if (fifo_size == 0 || num_planes > 1)
+		return false;
+
+	wm->sr.cursor = vlv_compute_wm(to_intel_crtc(crtc),
+				       to_intel_plane(crtc->cursor), 0x3f);
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
+		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			continue;
+
+		if (plane->pipe != pipe)
+			continue;
+
+		wm->sr.plane = vlv_compute_wm(to_intel_crtc(crtc),
+					      plane, fifo_size);
+		if (wm->sr.plane != 0)
+			break;
+	}
+
+	return true;
+}
+
+static void valleyview_update_wm(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
+	bool cxsr_enabled;
 	struct vlv_wm_values wm = dev_priv->wm.vlv;
 
 	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
+	wm.pipe[pipe].primary = vlv_compute_wm(intel_crtc,
+					       to_intel_plane(crtc->primary),
+					       vlv_get_fifo_size(dev, pipe, 0));
+
 	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
+	wm.pipe[pipe].cursor = vlv_compute_wm(intel_crtc,
+					      to_intel_plane(crtc->cursor),
+					      0x3f);
+
+	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
+
+	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
+		return;
+
+	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
+		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
+		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
+		      wm.sr.plane, wm.sr.cursor);
+
+	if (!cxsr_enabled)
+		intel_set_memory_cxsr(dev_priv, false);
 
 	vlv_write_wm_values(intel_crtc, &wm);
-}
-
-#define single_plane_enabled(mask) is_power_of_2(mask)
-
-static void valleyview_update_wm(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	static const int sr_latency_ns = 12000;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
-	int plane_sr, cursor_sr;
-	int ignore_plane_sr, ignore_cursor_sr;
-	unsigned int enabled = 0;
-	bool cxsr_enabled;
-
-	vlv_update_drain_latency(crtc);
-
-	if (g4x_compute_wm0(dev, PIPE_A,
-			    &valleyview_wm_info, pessimal_latency_ns,
-			    &valleyview_cursor_wm_info, pessimal_latency_ns,
-			    &planea_wm, &cursora_wm))
-		enabled |= 1 << PIPE_A;
-
-	if (g4x_compute_wm0(dev, PIPE_B,
-			    &valleyview_wm_info, pessimal_latency_ns,
-			    &valleyview_cursor_wm_info, pessimal_latency_ns,
-			    &planeb_wm, &cursorb_wm))
-		enabled |= 1 << PIPE_B;
-
-	if (single_plane_enabled(enabled) &&
-	    g4x_compute_srwm(dev, ffs(enabled) - 1,
-			     sr_latency_ns,
-			     &valleyview_wm_info,
-			     &valleyview_cursor_wm_info,
-			     &plane_sr, &ignore_cursor_sr) &&
-	    g4x_compute_srwm(dev, ffs(enabled) - 1,
-			     2*sr_latency_ns,
-			     &valleyview_wm_info,
-			     &valleyview_cursor_wm_info,
-			     &ignore_plane_sr, &cursor_sr)) {
-		cxsr_enabled = true;
-	} else {
-		cxsr_enabled = false;
-		intel_set_memory_cxsr(dev_priv, false);
-		plane_sr = cursor_sr = 0;
-	}
-
-	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
-		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
-		      planea_wm, cursora_wm,
-		      planeb_wm, cursorb_wm,
-		      plane_sr, cursor_sr);
-
-	I915_WRITE(DSPFW1,
-		   (plane_sr << DSPFW_SR_SHIFT) |
-		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
-		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
-		   (planea_wm << DSPFW_PLANEA_SHIFT));
-	I915_WRITE(DSPFW2,
-		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
-		   (cursora_wm << DSPFW_CURSORA_SHIFT));
-	I915_WRITE(DSPFW3,
-		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
-		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
-
-	if (cxsr_enabled)
-		intel_set_memory_cxsr(dev_priv, true);
-}
-
-static void cherryview_update_wm(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	static const int sr_latency_ns = 12000;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int planea_wm, planeb_wm, planec_wm;
-	int cursora_wm, cursorb_wm, cursorc_wm;
-	int plane_sr, cursor_sr;
-	int ignore_plane_sr, ignore_cursor_sr;
-	unsigned int enabled = 0;
-	bool cxsr_enabled;
-
-	vlv_update_drain_latency(crtc);
-
-	if (g4x_compute_wm0(dev, PIPE_A,
-			    &valleyview_wm_info, pessimal_latency_ns,
-			    &valleyview_cursor_wm_info, pessimal_latency_ns,
-			    &planea_wm, &cursora_wm))
-		enabled |= 1 << PIPE_A;
-
-	if (g4x_compute_wm0(dev, PIPE_B,
-			    &valleyview_wm_info, pessimal_latency_ns,
-			    &valleyview_cursor_wm_info, pessimal_latency_ns,
-			    &planeb_wm, &cursorb_wm))
-		enabled |= 1 << PIPE_B;
-
-	if (g4x_compute_wm0(dev, PIPE_C,
-			    &valleyview_wm_info, pessimal_latency_ns,
-			    &valleyview_cursor_wm_info, pessimal_latency_ns,
-			    &planec_wm, &cursorc_wm))
-		enabled |= 1 << PIPE_C;
-
-	if (single_plane_enabled(enabled) &&
-	    g4x_compute_srwm(dev, ffs(enabled) - 1,
-			     sr_latency_ns,
-			     &valleyview_wm_info,
-			     &valleyview_cursor_wm_info,
-			     &plane_sr, &ignore_cursor_sr) &&
-	    g4x_compute_srwm(dev, ffs(enabled) - 1,
-			     2*sr_latency_ns,
-			     &valleyview_wm_info,
-			     &valleyview_cursor_wm_info,
-			     &ignore_plane_sr, &cursor_sr)) {
-		cxsr_enabled = true;
-	} else {
-		cxsr_enabled = false;
-		intel_set_memory_cxsr(dev_priv, false);
-		plane_sr = cursor_sr = 0;
-	}
-
-	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
-		      "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
-		      "SR: plane=%d, cursor=%d\n",
-		      planea_wm, cursora_wm,
-		      planeb_wm, cursorb_wm,
-		      planec_wm, cursorc_wm,
-		      plane_sr, cursor_sr);
-
-	I915_WRITE(DSPFW1,
-		   (plane_sr << DSPFW_SR_SHIFT) |
-		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
-		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
-		   (planea_wm << DSPFW_PLANEA_SHIFT));
-	I915_WRITE(DSPFW2,
-		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
-		   (cursora_wm << DSPFW_CURSORA_SHIFT));
-	I915_WRITE(DSPFW3,
-		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
-		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
-	I915_WRITE(DSPFW9_CHV,
-		   (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
-					      DSPFW_CURSORC_MASK)) |
-		   (planec_wm << DSPFW_PLANEC_SHIFT) |
-		   (cursorc_wm << DSPFW_CURSORC_SHIFT));
 
 	if (cxsr_enabled)
 		intel_set_memory_cxsr(dev_priv, true);
@@ -1002,17 +995,44 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	int sprite = to_intel_plane(plane)->plane;
+	bool cxsr_enabled;
 	struct vlv_wm_values wm = dev_priv->wm.vlv;
 
-	if (enabled)
+	if (enabled) {
 		wm.ddl[pipe].sprite[sprite] =
 			vlv_compute_drain_latency(crtc, plane);
-	else
+
+		wm.pipe[pipe].sprite[sprite] =
+			vlv_compute_wm(intel_crtc,
+				       to_intel_plane(plane),
+				       vlv_get_fifo_size(dev, pipe, sprite+1));
+	} else {
 		wm.ddl[pipe].sprite[sprite] = 0;
+		wm.pipe[pipe].sprite[sprite] = 0;
+	}
+
+	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
+
+	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
+		return;
+
+	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: sprite %c=%d, "
+		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
+		      sprite_name(pipe, sprite),
+		      wm.pipe[pipe].sprite[sprite],
+		      wm.sr.plane, wm.sr.cursor);
+
+	if (!cxsr_enabled)
+		intel_set_memory_cxsr(dev_priv, false);
 
 	vlv_write_wm_values(intel_crtc, &wm);
+
+	if (cxsr_enabled)
+		intel_set_memory_cxsr(dev_priv, true);
 }
 
+#define single_plane_enabled(mask) is_power_of_2(mask)
+
 static void g4x_update_wm(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -6485,7 +6505,7 @@ void intel_init_pm(struct drm_device *dev)
 		else if (INTEL_INFO(dev)->gen == 8)
 			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
 	} else if (IS_CHERRYVIEW(dev)) {
-		dev_priv->display.update_wm = cherryview_update_wm;
+		dev_priv->display.update_wm = valleyview_update_wm;
 		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
 		dev_priv->display.init_clock_gating =
 			cherryview_init_clock_gating;
-- 
2.0.5

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

Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Jesse Barnes]

On 03/05/2015 11:19 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Assuming the PND deadline mechanism works reasonably we should do
> memory requests as early as possible so that PND has schedule the
> requests more intelligently. Currently we're still calculating
> the watermarks as if VLV/CHV are identical to g4x, which isn't
> the case.
> 
> The current code also seems to calculate insufficient watermarks
> and hence we're seeing some underruns, especially on high resolution
> displays.
> 
> To fix it just rip out the current code and replace is with something
> that tries to utilize PND as efficiently as possible.
> 
> We now calculate the WM watermark to trigger when the FIFO still has
> 256us worth of data. 256us is the maximum deadline value supoorted by
> PND, so issuing memory requests earlier would mean we probably couldn't
> utilize the full FIFO as PND would attempt to return the data at
> least in at least 256us. We also clamp the watermark to at least 8
> cachelines as that's the magic watermark that enabling trickle feed
> would also impose. I'm assuming it matches some burst size.
> 
> In theory we could just enable trickle feed and ignore the WM values,
> except trickle feed doesn't work with max fifo mode anyway, so we'd
> still need to calculate the SR watermarks. It seems cleaner to just
> disable trickle feed and calculate all watermarks the same way. Also
> trickle feed wouldn't account for the 256us max deadline value, thoguh
> that may be a moot point in non-max fifo mode sicne the FIFOs are fairly
> small.
> 
> On VLV max fifo mode can be used with either primary or sprite planes.
> So the code now also checks all the planes (apart from the cursor)
> when calculating the SR plane watermark.
> 
> We don't have to worry about the WM1 watermarks since we're using the
> PND deadline scheme which means the hardware ignores WM1 values.
> 
> v2: Use plane->state->fb instead of plane->fb
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  11 ++
>  drivers/gpu/drm/i915/i915_reg.h |   4 +-
>  drivers/gpu/drm/i915/intel_pm.c | 330 +++++++++++++++++++++-------------------
>  3 files changed, 188 insertions(+), 157 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5de69a0..b191b12 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1516,6 +1516,17 @@ struct ilk_wm_values {
>  
>  struct vlv_wm_values {
>  	struct {
> +		uint16_t primary;
> +		uint16_t sprite[2];
> +		uint8_t cursor;
> +	} pipe[3];
> +
> +	struct {
> +		uint16_t plane;
> +		uint8_t cursor;
> +	} sr;
> +
> +	struct {
>  		uint8_t cursor;
>  		uint8_t sprite[2];
>  		uint8_t primary;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8178610..9f98384 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4127,7 +4127,7 @@ enum skl_disp_power_wells {
>  /* vlv/chv high order bits */
>  #define DSPHOWM			(VLV_DISPLAY_BASE + 0x70064)
>  #define   DSPFW_SR_HI_SHIFT		24
> -#define   DSPFW_SR_HI_MASK		(1<<24)
> +#define   DSPFW_SR_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
>  #define   DSPFW_SPRITEF_HI_SHIFT	23
>  #define   DSPFW_SPRITEF_HI_MASK		(1<<23)
>  #define   DSPFW_SPRITEE_HI_SHIFT	22
> @@ -4148,7 +4148,7 @@ enum skl_disp_power_wells {
>  #define   DSPFW_PLANEA_HI_MASK		(1<<0)
>  #define DSPHOWM1		(VLV_DISPLAY_BASE + 0x70068)
>  #define   DSPFW_SR_WM1_HI_SHIFT		24
> -#define   DSPFW_SR_WM1_HI_MASK		(1<<24)
> +#define   DSPFW_SR_WM1_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
>  #define   DSPFW_SPRITEF_WM1_HI_SHIFT	23
>  #define   DSPFW_SPRITEF_WM1_HI_MASK	(1<<23)
>  #define   DSPFW_SPRITEE_WM1_HI_SHIFT	22
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bdb0f5d..497847c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -778,6 +778,55 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>  		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
>  		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
>  
> +	I915_WRITE(DSPFW1,
> +		   ((wm->sr.plane << DSPFW_SR_SHIFT) & DSPFW_SR_MASK) |
> +		   ((wm->pipe[PIPE_B].cursor << DSPFW_CURSORB_SHIFT) & DSPFW_CURSORB_MASK) |
> +		   ((wm->pipe[PIPE_B].primary << DSPFW_PLANEB_SHIFT) & DSPFW_PLANEB_MASK_VLV) |
> +		   ((wm->pipe[PIPE_A].primary << DSPFW_PLANEA_SHIFT) & DSPFW_PLANEA_MASK_VLV));
> +	I915_WRITE(DSPFW2,
> +		   ((wm->pipe[PIPE_A].sprite[1] << DSPFW_SPRITEB_SHIFT) & DSPFW_SPRITEB_MASK_VLV) |
> +		   ((wm->pipe[PIPE_A].cursor << DSPFW_CURSORA_SHIFT) & DSPFW_CURSORA_MASK) |
> +		   ((wm->pipe[PIPE_A].sprite[0] << DSPFW_SPRITEA_SHIFT) & DSPFW_SPRITEA_MASK_VLV));
> +	I915_WRITE(DSPFW3,
> +		   ((wm->sr.cursor << DSPFW_CURSOR_SR_SHIFT) & DSPFW_CURSOR_SR_MASK));
> +
> +	if (IS_CHERRYVIEW(dev_priv)) {
> +		I915_WRITE(DSPFW7_CHV,
> +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> +		I915_WRITE(DSPFW8_CHV,
> +			   ((wm->pipe[PIPE_C].sprite[1] << DSPFW_SPRITEF_SHIFT) & DSPFW_SPRITEF_MASK) |
> +			   ((wm->pipe[PIPE_C].sprite[0] << DSPFW_SPRITEE_SHIFT) & DSPFW_SPRITEE_MASK));
> +		I915_WRITE(DSPFW9_CHV,
> +			   ((wm->pipe[PIPE_C].primary << DSPFW_PLANEC_SHIFT) & DSPFW_PLANEC_MASK) |
> +			   ((wm->pipe[PIPE_C].cursor << DSPFW_CURSORC_SHIFT) & DSPFW_CURSORC_MASK));
> +		I915_WRITE(DSPHOWM,
> +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> +			   (((wm->pipe[PIPE_C].sprite[1] >> 8) << DSPFW_SPRITEF_HI_SHIFT) & DSPFW_SPRITEF_HI_MASK) |
> +			   (((wm->pipe[PIPE_C].sprite[0] >> 8) << DSPFW_SPRITEE_HI_SHIFT) & DSPFW_SPRITEE_HI_MASK) |
> +			   (((wm->pipe[PIPE_C].primary >> 8) << DSPFW_PLANEC_HI_SHIFT) & DSPFW_PLANEC_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> +	} else {
> +		I915_WRITE(DSPFW7,
> +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> +		I915_WRITE(DSPHOWM,
> +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> +	}
> +
> +	POSTING_READ(DSPFW1);
> +
>  	dev_priv->wm.vlv = *wm;
>  }
>  
> @@ -822,169 +871,113 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
>  				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
>  }
>  
> -/*
> - * Update drain latency registers of memory arbiter
> - *
> - * Valleyview SoC has a new memory arbiter and needs drain latency registers
> - * to be programmed. Each plane has a drain latency multiplier and a drain
> - * latency value.
> - */
> -
> -static void vlv_update_drain_latency(struct drm_crtc *crtc)
> +static int vlv_compute_wm(struct intel_crtc *crtc,
> +			  struct intel_plane *plane,
> +			  int fifo_size)
> +{
> +	int clock, entries, pixel_size;
> +
> +	/*
> +	 * FIXME the plane might have an fb
> +	 * but be invisible (eg. due to clipping)
> +	 */
> +	if (!crtc->active || !plane->base.state->fb)
> +		return 0;
> +
> +	pixel_size = drm_format_plane_cpp(plane->base.state->fb->pixel_format, 0);
> +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> +
> +	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> +
> +	/*
> +	 * Set up the watermark such that we don't start issuing memory
> +	 * requests until we are within PND's max deadline value (256us).
> +	 * Idea being to be idle as long as possible while still taking
> +	 * advatange of PND's deadline scheduling. The limit of 8
> +	 * cachelines (used when the FIFO will anyway drain in less time
> +	 * than 256us) should match what we would be done if trickle
> +	 * feed were enabled.
> +	 */
> +	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
> +}
> +
> +static bool vlv_compute_sr_wm(struct drm_device *dev,
> +			      struct vlv_wm_values *wm)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_crtc *crtc;
> +	enum pipe pipe = INVALID_PIPE;
> +	int num_planes = 0;
> +	int fifo_size = 0;
> +	struct intel_plane *plane;
> +
> +	wm->sr.cursor = wm->sr.plane = 0;
> +
> +	crtc = single_enabled_crtc(dev);
> +	/* maxfifo not supported on pipe C */
> +	if (crtc && to_intel_crtc(crtc)->pipe != PIPE_C) {
> +		pipe = to_intel_crtc(crtc)->pipe;
> +		num_planes = !!wm->pipe[pipe].primary +
> +			!!wm->pipe[pipe].sprite[0] +
> +			!!wm->pipe[pipe].sprite[1];
> +		fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
> +	}
> +
> +	if (fifo_size == 0 || num_planes > 1)
> +		return false;
> +
> +	wm->sr.cursor = vlv_compute_wm(to_intel_crtc(crtc),
> +				       to_intel_plane(crtc->cursor), 0x3f);
> +
> +	list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			continue;
> +
> +		if (plane->pipe != pipe)
> +			continue;
> +
> +		wm->sr.plane = vlv_compute_wm(to_intel_crtc(crtc),
> +					      plane, fifo_size);
> +		if (wm->sr.plane != 0)
> +			break;
> +	}
> +
> +	return true;
> +}
> +
> +static void valleyview_update_wm(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> +	bool cxsr_enabled;
>  	struct vlv_wm_values wm = dev_priv->wm.vlv;
>  
>  	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
> +	wm.pipe[pipe].primary = vlv_compute_wm(intel_crtc,
> +					       to_intel_plane(crtc->primary),
> +					       vlv_get_fifo_size(dev, pipe, 0));
> +
>  	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
> +	wm.pipe[pipe].cursor = vlv_compute_wm(intel_crtc,
> +					      to_intel_plane(crtc->cursor),
> +					      0x3f);
> +
> +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> +
> +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> +		return;
> +
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> +		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> +		      wm.sr.plane, wm.sr.cursor);
> +
> +	if (!cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, false);
>  
>  	vlv_write_wm_values(intel_crtc, &wm);
> -}
> -
> -#define single_plane_enabled(mask) is_power_of_2(mask)
> -
> -static void valleyview_update_wm(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	static const int sr_latency_ns = 12000;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> -	int plane_sr, cursor_sr;
> -	int ignore_plane_sr, ignore_cursor_sr;
> -	unsigned int enabled = 0;
> -	bool cxsr_enabled;
> -
> -	vlv_update_drain_latency(crtc);
> -
> -	if (g4x_compute_wm0(dev, PIPE_A,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planea_wm, &cursora_wm))
> -		enabled |= 1 << PIPE_A;
> -
> -	if (g4x_compute_wm0(dev, PIPE_B,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planeb_wm, &cursorb_wm))
> -		enabled |= 1 << PIPE_B;
> -
> -	if (single_plane_enabled(enabled) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &plane_sr, &ignore_cursor_sr) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     2*sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &ignore_plane_sr, &cursor_sr)) {
> -		cxsr_enabled = true;
> -	} else {
> -		cxsr_enabled = false;
> -		intel_set_memory_cxsr(dev_priv, false);
> -		plane_sr = cursor_sr = 0;
> -	}
> -
> -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> -		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> -		      planea_wm, cursora_wm,
> -		      planeb_wm, cursorb_wm,
> -		      plane_sr, cursor_sr);
> -
> -	I915_WRITE(DSPFW1,
> -		   (plane_sr << DSPFW_SR_SHIFT) |
> -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> -	I915_WRITE(DSPFW2,
> -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> -	I915_WRITE(DSPFW3,
> -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> -
> -	if (cxsr_enabled)
> -		intel_set_memory_cxsr(dev_priv, true);
> -}
> -
> -static void cherryview_update_wm(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	static const int sr_latency_ns = 12000;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int planea_wm, planeb_wm, planec_wm;
> -	int cursora_wm, cursorb_wm, cursorc_wm;
> -	int plane_sr, cursor_sr;
> -	int ignore_plane_sr, ignore_cursor_sr;
> -	unsigned int enabled = 0;
> -	bool cxsr_enabled;
> -
> -	vlv_update_drain_latency(crtc);
> -
> -	if (g4x_compute_wm0(dev, PIPE_A,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planea_wm, &cursora_wm))
> -		enabled |= 1 << PIPE_A;
> -
> -	if (g4x_compute_wm0(dev, PIPE_B,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planeb_wm, &cursorb_wm))
> -		enabled |= 1 << PIPE_B;
> -
> -	if (g4x_compute_wm0(dev, PIPE_C,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planec_wm, &cursorc_wm))
> -		enabled |= 1 << PIPE_C;
> -
> -	if (single_plane_enabled(enabled) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &plane_sr, &ignore_cursor_sr) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     2*sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &ignore_plane_sr, &cursor_sr)) {
> -		cxsr_enabled = true;
> -	} else {
> -		cxsr_enabled = false;
> -		intel_set_memory_cxsr(dev_priv, false);
> -		plane_sr = cursor_sr = 0;
> -	}
> -
> -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> -		      "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> -		      "SR: plane=%d, cursor=%d\n",
> -		      planea_wm, cursora_wm,
> -		      planeb_wm, cursorb_wm,
> -		      planec_wm, cursorc_wm,
> -		      plane_sr, cursor_sr);
> -
> -	I915_WRITE(DSPFW1,
> -		   (plane_sr << DSPFW_SR_SHIFT) |
> -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> -	I915_WRITE(DSPFW2,
> -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> -	I915_WRITE(DSPFW3,
> -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> -	I915_WRITE(DSPFW9_CHV,
> -		   (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> -					      DSPFW_CURSORC_MASK)) |
> -		   (planec_wm << DSPFW_PLANEC_SHIFT) |
> -		   (cursorc_wm << DSPFW_CURSORC_SHIFT));
>  
>  	if (cxsr_enabled)
>  		intel_set_memory_cxsr(dev_priv, true);
> @@ -1002,17 +995,44 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
>  	int sprite = to_intel_plane(plane)->plane;
> +	bool cxsr_enabled;
>  	struct vlv_wm_values wm = dev_priv->wm.vlv;
>  
> -	if (enabled)
> +	if (enabled) {
>  		wm.ddl[pipe].sprite[sprite] =
>  			vlv_compute_drain_latency(crtc, plane);
> -	else
> +
> +		wm.pipe[pipe].sprite[sprite] =
> +			vlv_compute_wm(intel_crtc,
> +				       to_intel_plane(plane),
> +				       vlv_get_fifo_size(dev, pipe, sprite+1));
> +	} else {
>  		wm.ddl[pipe].sprite[sprite] = 0;
> +		wm.pipe[pipe].sprite[sprite] = 0;
> +	}
> +
> +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> +
> +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> +		return;
> +
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: sprite %c=%d, "
> +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> +		      sprite_name(pipe, sprite),
> +		      wm.pipe[pipe].sprite[sprite],
> +		      wm.sr.plane, wm.sr.cursor);
> +
> +	if (!cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, false);
>  
>  	vlv_write_wm_values(intel_crtc, &wm);
> +
> +	if (cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, true);
>  }
>  
> +#define single_plane_enabled(mask) is_power_of_2(mask)
> +
>  static void g4x_update_wm(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -6485,7 +6505,7 @@ void intel_init_pm(struct drm_device *dev)
>  		else if (INTEL_INFO(dev)->gen == 8)
>  			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
>  	} else if (IS_CHERRYVIEW(dev)) {
> -		dev_priv->display.update_wm = cherryview_update_wm;
> +		dev_priv->display.update_wm = valleyview_update_wm;
>  		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
>  		dev_priv->display.init_clock_gating =
>  			cherryview_init_clock_gating;
> 

I wonder if we should be warning if the wm values we end up with exceed
the mask size (the fact that you write them with a shift and mask made
me think of it), but that could be a follow on, or even put into the
calc code instead.  Anyway that's something we can do later after all
the atomic changes hit.

Does this fix any of our underrun bugs?  Should it have any references:
lines?

Either way:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Daniel Vetter]

On Fri, Mar 06, 2015 at 09:31:20AM -0800, Jesse Barnes wrote:
> I wonder if we should be warning if the wm values we end up with exceed
> the mask size (the fact that you write them with a shift and mask made
> me think of it), but that could be a follow on, or even put into the
> calc code instead.  Anyway that's something we can do later after all
> the atomic changes hit.

Hm, I've thought the overall plan for wm computation code is that we
calculate the wm lines we need and then the magic atomic consolidation
step meshes it all together appropriately. But hey I actually about
something approaching 0 clue on this topic, it's really all magic here ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Ville Syrjälä]

On Fri, Mar 06, 2015 at 09:31:20AM -0800, Jesse Barnes wrote:
> On 03/05/2015 11:19 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Assuming the PND deadline mechanism works reasonably we should do
> > memory requests as early as possible so that PND has schedule the
> > requests more intelligently. Currently we're still calculating
> > the watermarks as if VLV/CHV are identical to g4x, which isn't
> > the case.
> > 
> > The current code also seems to calculate insufficient watermarks
> > and hence we're seeing some underruns, especially on high resolution
> > displays.
> > 
> > To fix it just rip out the current code and replace is with something
> > that tries to utilize PND as efficiently as possible.
> > 
> > We now calculate the WM watermark to trigger when the FIFO still has
> > 256us worth of data. 256us is the maximum deadline value supoorted by
> > PND, so issuing memory requests earlier would mean we probably couldn't
> > utilize the full FIFO as PND would attempt to return the data at
> > least in at least 256us. We also clamp the watermark to at least 8
> > cachelines as that's the magic watermark that enabling trickle feed
> > would also impose. I'm assuming it matches some burst size.
> > 
> > In theory we could just enable trickle feed and ignore the WM values,
> > except trickle feed doesn't work with max fifo mode anyway, so we'd
> > still need to calculate the SR watermarks. It seems cleaner to just
> > disable trickle feed and calculate all watermarks the same way. Also
> > trickle feed wouldn't account for the 256us max deadline value, thoguh
> > that may be a moot point in non-max fifo mode sicne the FIFOs are fairly
> > small.
> > 
> > On VLV max fifo mode can be used with either primary or sprite planes.
> > So the code now also checks all the planes (apart from the cursor)
> > when calculating the SR plane watermark.
> > 
> > We don't have to worry about the WM1 watermarks since we're using the
> > PND deadline scheme which means the hardware ignores WM1 values.
> > 
> > v2: Use plane->state->fb instead of plane->fb
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  11 ++
> >  drivers/gpu/drm/i915/i915_reg.h |   4 +-
> >  drivers/gpu/drm/i915/intel_pm.c | 330 +++++++++++++++++++++-------------------
> >  3 files changed, 188 insertions(+), 157 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 5de69a0..b191b12 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1516,6 +1516,17 @@ struct ilk_wm_values {
> >  
> >  struct vlv_wm_values {
> >  	struct {
> > +		uint16_t primary;
> > +		uint16_t sprite[2];
> > +		uint8_t cursor;
> > +	} pipe[3];
> > +
> > +	struct {
> > +		uint16_t plane;
> > +		uint8_t cursor;
> > +	} sr;
> > +
> > +	struct {
> >  		uint8_t cursor;
> >  		uint8_t sprite[2];
> >  		uint8_t primary;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8178610..9f98384 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4127,7 +4127,7 @@ enum skl_disp_power_wells {
> >  /* vlv/chv high order bits */
> >  #define DSPHOWM			(VLV_DISPLAY_BASE + 0x70064)
> >  #define   DSPFW_SR_HI_SHIFT		24
> > -#define   DSPFW_SR_HI_MASK		(1<<24)
> > +#define   DSPFW_SR_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
> >  #define   DSPFW_SPRITEF_HI_SHIFT	23
> >  #define   DSPFW_SPRITEF_HI_MASK		(1<<23)
> >  #define   DSPFW_SPRITEE_HI_SHIFT	22
> > @@ -4148,7 +4148,7 @@ enum skl_disp_power_wells {
> >  #define   DSPFW_PLANEA_HI_MASK		(1<<0)
> >  #define DSPHOWM1		(VLV_DISPLAY_BASE + 0x70068)
> >  #define   DSPFW_SR_WM1_HI_SHIFT		24
> > -#define   DSPFW_SR_WM1_HI_MASK		(1<<24)
> > +#define   DSPFW_SR_WM1_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
> >  #define   DSPFW_SPRITEF_WM1_HI_SHIFT	23
> >  #define   DSPFW_SPRITEF_WM1_HI_MASK	(1<<23)
> >  #define   DSPFW_SPRITEE_WM1_HI_SHIFT	22
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bdb0f5d..497847c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -778,6 +778,55 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >  		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
> >  		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
> >  
> > +	I915_WRITE(DSPFW1,
> > +		   ((wm->sr.plane << DSPFW_SR_SHIFT) & DSPFW_SR_MASK) |
> > +		   ((wm->pipe[PIPE_B].cursor << DSPFW_CURSORB_SHIFT) & DSPFW_CURSORB_MASK) |
> > +		   ((wm->pipe[PIPE_B].primary << DSPFW_PLANEB_SHIFT) & DSPFW_PLANEB_MASK_VLV) |
> > +		   ((wm->pipe[PIPE_A].primary << DSPFW_PLANEA_SHIFT) & DSPFW_PLANEA_MASK_VLV));
> > +	I915_WRITE(DSPFW2,
> > +		   ((wm->pipe[PIPE_A].sprite[1] << DSPFW_SPRITEB_SHIFT) & DSPFW_SPRITEB_MASK_VLV) |
> > +		   ((wm->pipe[PIPE_A].cursor << DSPFW_CURSORA_SHIFT) & DSPFW_CURSORA_MASK) |
> > +		   ((wm->pipe[PIPE_A].sprite[0] << DSPFW_SPRITEA_SHIFT) & DSPFW_SPRITEA_MASK_VLV));
> > +	I915_WRITE(DSPFW3,
> > +		   ((wm->sr.cursor << DSPFW_CURSOR_SR_SHIFT) & DSPFW_CURSOR_SR_MASK));
> > +
> > +	if (IS_CHERRYVIEW(dev_priv)) {
> > +		I915_WRITE(DSPFW7_CHV,
> > +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> > +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> > +		I915_WRITE(DSPFW8_CHV,
> > +			   ((wm->pipe[PIPE_C].sprite[1] << DSPFW_SPRITEF_SHIFT) & DSPFW_SPRITEF_MASK) |
> > +			   ((wm->pipe[PIPE_C].sprite[0] << DSPFW_SPRITEE_SHIFT) & DSPFW_SPRITEE_MASK));
> > +		I915_WRITE(DSPFW9_CHV,
> > +			   ((wm->pipe[PIPE_C].primary << DSPFW_PLANEC_SHIFT) & DSPFW_PLANEC_MASK) |
> > +			   ((wm->pipe[PIPE_C].cursor << DSPFW_CURSORC_SHIFT) & DSPFW_CURSORC_MASK));
> > +		I915_WRITE(DSPHOWM,
> > +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].sprite[1] >> 8) << DSPFW_SPRITEF_HI_SHIFT) & DSPFW_SPRITEF_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].sprite[0] >> 8) << DSPFW_SPRITEE_HI_SHIFT) & DSPFW_SPRITEE_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].primary >> 8) << DSPFW_PLANEC_HI_SHIFT) & DSPFW_PLANEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> > +	} else {
> > +		I915_WRITE(DSPFW7,
> > +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> > +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> > +		I915_WRITE(DSPHOWM,
> > +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> > +	}
> > +
> > +	POSTING_READ(DSPFW1);
> > +
> >  	dev_priv->wm.vlv = *wm;
> >  }
> >  
> > @@ -822,169 +871,113 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
> >  				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
> >  }
> >  
> > -/*
> > - * Update drain latency registers of memory arbiter
> > - *
> > - * Valleyview SoC has a new memory arbiter and needs drain latency registers
> > - * to be programmed. Each plane has a drain latency multiplier and a drain
> > - * latency value.
> > - */
> > -
> > -static void vlv_update_drain_latency(struct drm_crtc *crtc)
> > +static int vlv_compute_wm(struct intel_crtc *crtc,
> > +			  struct intel_plane *plane,
> > +			  int fifo_size)
> > +{
> > +	int clock, entries, pixel_size;
> > +
> > +	/*
> > +	 * FIXME the plane might have an fb
> > +	 * but be invisible (eg. due to clipping)
> > +	 */
> > +	if (!crtc->active || !plane->base.state->fb)
> > +		return 0;
> > +
> > +	pixel_size = drm_format_plane_cpp(plane->base.state->fb->pixel_format, 0);
> > +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> > +
> > +	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> > +
> > +	/*
> > +	 * Set up the watermark such that we don't start issuing memory
> > +	 * requests until we are within PND's max deadline value (256us).
> > +	 * Idea being to be idle as long as possible while still taking
> > +	 * advatange of PND's deadline scheduling. The limit of 8
> > +	 * cachelines (used when the FIFO will anyway drain in less time
> > +	 * than 256us) should match what we would be done if trickle
> > +	 * feed were enabled.
> > +	 */
> > +	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
> > +}
> > +
> > +static bool vlv_compute_sr_wm(struct drm_device *dev,
> > +			      struct vlv_wm_values *wm)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_crtc *crtc;
> > +	enum pipe pipe = INVALID_PIPE;
> > +	int num_planes = 0;
> > +	int fifo_size = 0;
> > +	struct intel_plane *plane;
> > +
> > +	wm->sr.cursor = wm->sr.plane = 0;
> > +
> > +	crtc = single_enabled_crtc(dev);
> > +	/* maxfifo not supported on pipe C */
> > +	if (crtc && to_intel_crtc(crtc)->pipe != PIPE_C) {
> > +		pipe = to_intel_crtc(crtc)->pipe;
> > +		num_planes = !!wm->pipe[pipe].primary +
> > +			!!wm->pipe[pipe].sprite[0] +
> > +			!!wm->pipe[pipe].sprite[1];
> > +		fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
> > +	}
> > +
> > +	if (fifo_size == 0 || num_planes > 1)
> > +		return false;
> > +
> > +	wm->sr.cursor = vlv_compute_wm(to_intel_crtc(crtc),
> > +				       to_intel_plane(crtc->cursor), 0x3f);
> > +
> > +	list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> > +			continue;
> > +
> > +		if (plane->pipe != pipe)
> > +			continue;
> > +
> > +		wm->sr.plane = vlv_compute_wm(to_intel_crtc(crtc),
> > +					      plane, fifo_size);
> > +		if (wm->sr.plane != 0)
> > +			break;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void valleyview_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> > +	bool cxsr_enabled;
> >  	struct vlv_wm_values wm = dev_priv->wm.vlv;
> >  
> >  	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
> > +	wm.pipe[pipe].primary = vlv_compute_wm(intel_crtc,
> > +					       to_intel_plane(crtc->primary),
> > +					       vlv_get_fifo_size(dev, pipe, 0));
> > +
> >  	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
> > +	wm.pipe[pipe].cursor = vlv_compute_wm(intel_crtc,
> > +					      to_intel_plane(crtc->cursor),
> > +					      0x3f);
> > +
> > +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> > +
> > +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> > +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> > +		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> > +		      wm.sr.plane, wm.sr.cursor);
> > +
> > +	if (!cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, false);
> >  
> >  	vlv_write_wm_values(intel_crtc, &wm);
> > -}
> > -
> > -#define single_plane_enabled(mask) is_power_of_2(mask)
> > -
> > -static void valleyview_update_wm(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	static const int sr_latency_ns = 12000;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> > -	int plane_sr, cursor_sr;
> > -	int ignore_plane_sr, ignore_cursor_sr;
> > -	unsigned int enabled = 0;
> > -	bool cxsr_enabled;
> > -
> > -	vlv_update_drain_latency(crtc);
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_A,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planea_wm, &cursora_wm))
> > -		enabled |= 1 << PIPE_A;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_B,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planeb_wm, &cursorb_wm))
> > -		enabled |= 1 << PIPE_B;
> > -
> > -	if (single_plane_enabled(enabled) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &plane_sr, &ignore_cursor_sr) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     2*sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &ignore_plane_sr, &cursor_sr)) {
> > -		cxsr_enabled = true;
> > -	} else {
> > -		cxsr_enabled = false;
> > -		intel_set_memory_cxsr(dev_priv, false);
> > -		plane_sr = cursor_sr = 0;
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > -		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> > -		      planea_wm, cursora_wm,
> > -		      planeb_wm, cursorb_wm,
> > -		      plane_sr, cursor_sr);
> > -
> > -	I915_WRITE(DSPFW1,
> > -		   (plane_sr << DSPFW_SR_SHIFT) |
> > -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> > -	I915_WRITE(DSPFW2,
> > -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > -	I915_WRITE(DSPFW3,
> > -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > -
> > -	if (cxsr_enabled)
> > -		intel_set_memory_cxsr(dev_priv, true);
> > -}
> > -
> > -static void cherryview_update_wm(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	static const int sr_latency_ns = 12000;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int planea_wm, planeb_wm, planec_wm;
> > -	int cursora_wm, cursorb_wm, cursorc_wm;
> > -	int plane_sr, cursor_sr;
> > -	int ignore_plane_sr, ignore_cursor_sr;
> > -	unsigned int enabled = 0;
> > -	bool cxsr_enabled;
> > -
> > -	vlv_update_drain_latency(crtc);
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_A,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planea_wm, &cursora_wm))
> > -		enabled |= 1 << PIPE_A;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_B,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planeb_wm, &cursorb_wm))
> > -		enabled |= 1 << PIPE_B;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_C,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planec_wm, &cursorc_wm))
> > -		enabled |= 1 << PIPE_C;
> > -
> > -	if (single_plane_enabled(enabled) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &plane_sr, &ignore_cursor_sr) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     2*sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &ignore_plane_sr, &cursor_sr)) {
> > -		cxsr_enabled = true;
> > -	} else {
> > -		cxsr_enabled = false;
> > -		intel_set_memory_cxsr(dev_priv, false);
> > -		plane_sr = cursor_sr = 0;
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > -		      "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> > -		      "SR: plane=%d, cursor=%d\n",
> > -		      planea_wm, cursora_wm,
> > -		      planeb_wm, cursorb_wm,
> > -		      planec_wm, cursorc_wm,
> > -		      plane_sr, cursor_sr);
> > -
> > -	I915_WRITE(DSPFW1,
> > -		   (plane_sr << DSPFW_SR_SHIFT) |
> > -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> > -	I915_WRITE(DSPFW2,
> > -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > -	I915_WRITE(DSPFW3,
> > -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > -	I915_WRITE(DSPFW9_CHV,
> > -		   (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> > -					      DSPFW_CURSORC_MASK)) |
> > -		   (planec_wm << DSPFW_PLANEC_SHIFT) |
> > -		   (cursorc_wm << DSPFW_CURSORC_SHIFT));
> >  
> >  	if (cxsr_enabled)
> >  		intel_set_memory_cxsr(dev_priv, true);
> > @@ -1002,17 +995,44 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> >  	int sprite = to_intel_plane(plane)->plane;
> > +	bool cxsr_enabled;
> >  	struct vlv_wm_values wm = dev_priv->wm.vlv;
> >  
> > -	if (enabled)
> > +	if (enabled) {
> >  		wm.ddl[pipe].sprite[sprite] =
> >  			vlv_compute_drain_latency(crtc, plane);
> > -	else
> > +
> > +		wm.pipe[pipe].sprite[sprite] =
> > +			vlv_compute_wm(intel_crtc,
> > +				       to_intel_plane(plane),
> > +				       vlv_get_fifo_size(dev, pipe, sprite+1));
> > +	} else {
> >  		wm.ddl[pipe].sprite[sprite] = 0;
> > +		wm.pipe[pipe].sprite[sprite] = 0;
> > +	}
> > +
> > +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> > +
> > +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: sprite %c=%d, "
> > +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> > +		      sprite_name(pipe, sprite),
> > +		      wm.pipe[pipe].sprite[sprite],
> > +		      wm.sr.plane, wm.sr.cursor);
> > +
> > +	if (!cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, false);
> >  
> >  	vlv_write_wm_values(intel_crtc, &wm);
> > +
> > +	if (cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, true);
> >  }
> >  
> > +#define single_plane_enabled(mask) is_power_of_2(mask)
> > +
> >  static void g4x_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -6485,7 +6505,7 @@ void intel_init_pm(struct drm_device *dev)
> >  		else if (INTEL_INFO(dev)->gen == 8)
> >  			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
> >  	} else if (IS_CHERRYVIEW(dev)) {
> > -		dev_priv->display.update_wm = cherryview_update_wm;
> > +		dev_priv->display.update_wm = valleyview_update_wm;
> >  		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> >  		dev_priv->display.init_clock_gating =
> >  			cherryview_init_clock_gating;
> > 
> 
> I wonder if we should be warning if the wm values we end up with exceed
> the mask size (the fact that you write them with a shift and mask made
> me think of it), but that could be a follow on, or even put into the
> calc code instead.  Anyway that's something we can do later after all
> the atomic changes hit.

IIRC we always have enough bits up to any legal FIFO size, so the clamping
done by vlv_compute_wm() should be enough. I should double check that though
since that isn't the case on a bunch of the other platforms.

I think in general I'd really like magic register bitfield macros that
scream whenever we overflow something by accident. But that's a much
bigger topic. For one we'd have to parametrize all the macros rather than
using raw shifts.

> 
> Does this fix any of our underrun bugs?  Should it have any references:
> lines?

Those should probably be at the DDR DVFS disable patch since before that
pretty much anything can happen. I was too lazy to trawl bugzilla though.
Pretty much hoping QA can just go retest all display bugs once we get
this landed.

> 
> Either way:
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks.

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

Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Jesse Barnes]

On 03/06/2015 10:14 AM, Ville Syrjälä wrote:
> On Fri, Mar 06, 2015 at 09:31:20AM -0800, Jesse Barnes wrote:
>> On 03/05/2015 11:19 AM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> I wonder if we should be warning if the wm values we end up with exceed
>> the mask size (the fact that you write them with a shift and mask made
>> me think of it), but that could be a follow on, or even put into the
>> calc code instead.  Anyway that's something we can do later after all
>> the atomic changes hit.
> 
> IIRC we always have enough bits up to any legal FIFO size, so the clamping
> done by vlv_compute_wm() should be enough. I should double check that though
> since that isn't the case on a bunch of the other platforms.
> 
> I think in general I'd really like magic register bitfield macros that
> scream whenever we overflow something by accident. But that's a much
> bigger topic. For one we'd have to parametrize all the macros rather than
> using raw shifts.

Yeah and that would have the added benefit of more readability.
Something for another day if/when we see underruns due to failed wm
programming in the future. :)

> 
>>
>> Does this fix any of our underrun bugs?  Should it have any references:
>> lines?
> 
> Those should probably be at the DDR DVFS disable patch since before that
> pretty much anything can happen. I was too lazy to trawl bugzilla though.
> Pretty much hoping QA can just go retest all display bugs once we get
> this landed.

Ok, sounds good.

Thanks,
Jesse

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

Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Daniel Vetter]

On Thu, Mar 05, 2015 at 09:19:49PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Assuming the PND deadline mechanism works reasonably we should do
> memory requests as early as possible so that PND has schedule the
> requests more intelligently. Currently we're still calculating
> the watermarks as if VLV/CHV are identical to g4x, which isn't
> the case.
> 
> The current code also seems to calculate insufficient watermarks
> and hence we're seeing some underruns, especially on high resolution
> displays.
> 
> To fix it just rip out the current code and replace is with something
> that tries to utilize PND as efficiently as possible.
> 
> We now calculate the WM watermark to trigger when the FIFO still has
> 256us worth of data. 256us is the maximum deadline value supoorted by
> PND, so issuing memory requests earlier would mean we probably couldn't
> utilize the full FIFO as PND would attempt to return the data at
> least in at least 256us. We also clamp the watermark to at least 8
> cachelines as that's the magic watermark that enabling trickle feed
> would also impose. I'm assuming it matches some burst size.
> 
> In theory we could just enable trickle feed and ignore the WM values,
> except trickle feed doesn't work with max fifo mode anyway, so we'd
> still need to calculate the SR watermarks. It seems cleaner to just
> disable trickle feed and calculate all watermarks the same way. Also
> trickle feed wouldn't account for the 256us max deadline value, thoguh
> that may be a moot point in non-max fifo mode sicne the FIFOs are fairly
> small.
> 
> On VLV max fifo mode can be used with either primary or sprite planes.
> So the code now also checks all the planes (apart from the cursor)
> when calculating the SR plane watermark.
> 
> We don't have to worry about the WM1 watermarks since we're using the
> PND deadline scheme which means the hardware ignores WM1 values.
> 
> v2: Use plane->state->fb instead of plane->fb
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  11 ++
>  drivers/gpu/drm/i915/i915_reg.h |   4 +-
>  drivers/gpu/drm/i915/intel_pm.c | 330 +++++++++++++++++++++-------------------
>  3 files changed, 188 insertions(+), 157 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5de69a0..b191b12 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1516,6 +1516,17 @@ struct ilk_wm_values {
>  
>  struct vlv_wm_values {
>  	struct {
> +		uint16_t primary;
> +		uint16_t sprite[2];
> +		uint8_t cursor;
> +	} pipe[3];
> +
> +	struct {
> +		uint16_t plane;
> +		uint8_t cursor;
> +	} sr;
> +
> +	struct {
>  		uint8_t cursor;
>  		uint8_t sprite[2];
>  		uint8_t primary;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8178610..9f98384 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4127,7 +4127,7 @@ enum skl_disp_power_wells {
>  /* vlv/chv high order bits */
>  #define DSPHOWM			(VLV_DISPLAY_BASE + 0x70064)
>  #define   DSPFW_SR_HI_SHIFT		24
> -#define   DSPFW_SR_HI_MASK		(1<<24)
> +#define   DSPFW_SR_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
>  #define   DSPFW_SPRITEF_HI_SHIFT	23
>  #define   DSPFW_SPRITEF_HI_MASK		(1<<23)
>  #define   DSPFW_SPRITEE_HI_SHIFT	22
> @@ -4148,7 +4148,7 @@ enum skl_disp_power_wells {
>  #define   DSPFW_PLANEA_HI_MASK		(1<<0)
>  #define DSPHOWM1		(VLV_DISPLAY_BASE + 0x70068)
>  #define   DSPFW_SR_WM1_HI_SHIFT		24
> -#define   DSPFW_SR_WM1_HI_MASK		(1<<24)
> +#define   DSPFW_SR_WM1_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
>  #define   DSPFW_SPRITEF_WM1_HI_SHIFT	23
>  #define   DSPFW_SPRITEF_WM1_HI_MASK	(1<<23)
>  #define   DSPFW_SPRITEE_WM1_HI_SHIFT	22
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bdb0f5d..497847c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -778,6 +778,55 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>  		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
>  		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
>  
> +	I915_WRITE(DSPFW1,
> +		   ((wm->sr.plane << DSPFW_SR_SHIFT) & DSPFW_SR_MASK) |
> +		   ((wm->pipe[PIPE_B].cursor << DSPFW_CURSORB_SHIFT) & DSPFW_CURSORB_MASK) |
> +		   ((wm->pipe[PIPE_B].primary << DSPFW_PLANEB_SHIFT) & DSPFW_PLANEB_MASK_VLV) |
> +		   ((wm->pipe[PIPE_A].primary << DSPFW_PLANEA_SHIFT) & DSPFW_PLANEA_MASK_VLV));
> +	I915_WRITE(DSPFW2,
> +		   ((wm->pipe[PIPE_A].sprite[1] << DSPFW_SPRITEB_SHIFT) & DSPFW_SPRITEB_MASK_VLV) |
> +		   ((wm->pipe[PIPE_A].cursor << DSPFW_CURSORA_SHIFT) & DSPFW_CURSORA_MASK) |
> +		   ((wm->pipe[PIPE_A].sprite[0] << DSPFW_SPRITEA_SHIFT) & DSPFW_SPRITEA_MASK_VLV));
> +	I915_WRITE(DSPFW3,
> +		   ((wm->sr.cursor << DSPFW_CURSOR_SR_SHIFT) & DSPFW_CURSOR_SR_MASK));
> +
> +	if (IS_CHERRYVIEW(dev_priv)) {
> +		I915_WRITE(DSPFW7_CHV,
> +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> +		I915_WRITE(DSPFW8_CHV,
> +			   ((wm->pipe[PIPE_C].sprite[1] << DSPFW_SPRITEF_SHIFT) & DSPFW_SPRITEF_MASK) |
> +			   ((wm->pipe[PIPE_C].sprite[0] << DSPFW_SPRITEE_SHIFT) & DSPFW_SPRITEE_MASK));
> +		I915_WRITE(DSPFW9_CHV,
> +			   ((wm->pipe[PIPE_C].primary << DSPFW_PLANEC_SHIFT) & DSPFW_PLANEC_MASK) |
> +			   ((wm->pipe[PIPE_C].cursor << DSPFW_CURSORC_SHIFT) & DSPFW_CURSORC_MASK));
> +		I915_WRITE(DSPHOWM,
> +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> +			   (((wm->pipe[PIPE_C].sprite[1] >> 8) << DSPFW_SPRITEF_HI_SHIFT) & DSPFW_SPRITEF_HI_MASK) |
> +			   (((wm->pipe[PIPE_C].sprite[0] >> 8) << DSPFW_SPRITEE_HI_SHIFT) & DSPFW_SPRITEE_HI_MASK) |
> +			   (((wm->pipe[PIPE_C].primary >> 8) << DSPFW_PLANEC_HI_SHIFT) & DSPFW_PLANEC_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> +	} else {
> +		I915_WRITE(DSPFW7,
> +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> +		I915_WRITE(DSPHOWM,
> +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));

Checkpatch complained about this code and I concur that it's not pretty.
Care for a prettification follow-up? Some #defines to do the shift+mask
would help a lot imo. Or maybe not computing everything in-place.
-Daniel

> +	}
> +
> +	POSTING_READ(DSPFW1);
> +
>  	dev_priv->wm.vlv = *wm;
>  }
>  
> @@ -822,169 +871,113 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
>  				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
>  }
>  
> -/*
> - * Update drain latency registers of memory arbiter
> - *
> - * Valleyview SoC has a new memory arbiter and needs drain latency registers
> - * to be programmed. Each plane has a drain latency multiplier and a drain
> - * latency value.
> - */
> -
> -static void vlv_update_drain_latency(struct drm_crtc *crtc)
> +static int vlv_compute_wm(struct intel_crtc *crtc,
> +			  struct intel_plane *plane,
> +			  int fifo_size)
> +{
> +	int clock, entries, pixel_size;
> +
> +	/*
> +	 * FIXME the plane might have an fb
> +	 * but be invisible (eg. due to clipping)
> +	 */
> +	if (!crtc->active || !plane->base.state->fb)
> +		return 0;
> +
> +	pixel_size = drm_format_plane_cpp(plane->base.state->fb->pixel_format, 0);
> +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> +
> +	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> +
> +	/*
> +	 * Set up the watermark such that we don't start issuing memory
> +	 * requests until we are within PND's max deadline value (256us).
> +	 * Idea being to be idle as long as possible while still taking
> +	 * advatange of PND's deadline scheduling. The limit of 8
> +	 * cachelines (used when the FIFO will anyway drain in less time
> +	 * than 256us) should match what we would be done if trickle
> +	 * feed were enabled.
> +	 */
> +	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
> +}
> +
> +static bool vlv_compute_sr_wm(struct drm_device *dev,
> +			      struct vlv_wm_values *wm)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_crtc *crtc;
> +	enum pipe pipe = INVALID_PIPE;
> +	int num_planes = 0;
> +	int fifo_size = 0;
> +	struct intel_plane *plane;
> +
> +	wm->sr.cursor = wm->sr.plane = 0;
> +
> +	crtc = single_enabled_crtc(dev);
> +	/* maxfifo not supported on pipe C */
> +	if (crtc && to_intel_crtc(crtc)->pipe != PIPE_C) {
> +		pipe = to_intel_crtc(crtc)->pipe;
> +		num_planes = !!wm->pipe[pipe].primary +
> +			!!wm->pipe[pipe].sprite[0] +
> +			!!wm->pipe[pipe].sprite[1];
> +		fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
> +	}
> +
> +	if (fifo_size == 0 || num_planes > 1)
> +		return false;
> +
> +	wm->sr.cursor = vlv_compute_wm(to_intel_crtc(crtc),
> +				       to_intel_plane(crtc->cursor), 0x3f);
> +
> +	list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			continue;
> +
> +		if (plane->pipe != pipe)
> +			continue;
> +
> +		wm->sr.plane = vlv_compute_wm(to_intel_crtc(crtc),
> +					      plane, fifo_size);
> +		if (wm->sr.plane != 0)
> +			break;
> +	}
> +
> +	return true;
> +}
> +
> +static void valleyview_update_wm(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> +	bool cxsr_enabled;
>  	struct vlv_wm_values wm = dev_priv->wm.vlv;
>  
>  	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
> +	wm.pipe[pipe].primary = vlv_compute_wm(intel_crtc,
> +					       to_intel_plane(crtc->primary),
> +					       vlv_get_fifo_size(dev, pipe, 0));
> +
>  	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
> +	wm.pipe[pipe].cursor = vlv_compute_wm(intel_crtc,
> +					      to_intel_plane(crtc->cursor),
> +					      0x3f);
> +
> +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> +
> +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> +		return;
> +
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> +		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> +		      wm.sr.plane, wm.sr.cursor);
> +
> +	if (!cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, false);
>  
>  	vlv_write_wm_values(intel_crtc, &wm);
> -}
> -
> -#define single_plane_enabled(mask) is_power_of_2(mask)
> -
> -static void valleyview_update_wm(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	static const int sr_latency_ns = 12000;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> -	int plane_sr, cursor_sr;
> -	int ignore_plane_sr, ignore_cursor_sr;
> -	unsigned int enabled = 0;
> -	bool cxsr_enabled;
> -
> -	vlv_update_drain_latency(crtc);
> -
> -	if (g4x_compute_wm0(dev, PIPE_A,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planea_wm, &cursora_wm))
> -		enabled |= 1 << PIPE_A;
> -
> -	if (g4x_compute_wm0(dev, PIPE_B,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planeb_wm, &cursorb_wm))
> -		enabled |= 1 << PIPE_B;
> -
> -	if (single_plane_enabled(enabled) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &plane_sr, &ignore_cursor_sr) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     2*sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &ignore_plane_sr, &cursor_sr)) {
> -		cxsr_enabled = true;
> -	} else {
> -		cxsr_enabled = false;
> -		intel_set_memory_cxsr(dev_priv, false);
> -		plane_sr = cursor_sr = 0;
> -	}
> -
> -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> -		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> -		      planea_wm, cursora_wm,
> -		      planeb_wm, cursorb_wm,
> -		      plane_sr, cursor_sr);
> -
> -	I915_WRITE(DSPFW1,
> -		   (plane_sr << DSPFW_SR_SHIFT) |
> -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> -	I915_WRITE(DSPFW2,
> -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> -	I915_WRITE(DSPFW3,
> -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> -
> -	if (cxsr_enabled)
> -		intel_set_memory_cxsr(dev_priv, true);
> -}
> -
> -static void cherryview_update_wm(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	static const int sr_latency_ns = 12000;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int planea_wm, planeb_wm, planec_wm;
> -	int cursora_wm, cursorb_wm, cursorc_wm;
> -	int plane_sr, cursor_sr;
> -	int ignore_plane_sr, ignore_cursor_sr;
> -	unsigned int enabled = 0;
> -	bool cxsr_enabled;
> -
> -	vlv_update_drain_latency(crtc);
> -
> -	if (g4x_compute_wm0(dev, PIPE_A,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planea_wm, &cursora_wm))
> -		enabled |= 1 << PIPE_A;
> -
> -	if (g4x_compute_wm0(dev, PIPE_B,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planeb_wm, &cursorb_wm))
> -		enabled |= 1 << PIPE_B;
> -
> -	if (g4x_compute_wm0(dev, PIPE_C,
> -			    &valleyview_wm_info, pessimal_latency_ns,
> -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> -			    &planec_wm, &cursorc_wm))
> -		enabled |= 1 << PIPE_C;
> -
> -	if (single_plane_enabled(enabled) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &plane_sr, &ignore_cursor_sr) &&
> -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> -			     2*sr_latency_ns,
> -			     &valleyview_wm_info,
> -			     &valleyview_cursor_wm_info,
> -			     &ignore_plane_sr, &cursor_sr)) {
> -		cxsr_enabled = true;
> -	} else {
> -		cxsr_enabled = false;
> -		intel_set_memory_cxsr(dev_priv, false);
> -		plane_sr = cursor_sr = 0;
> -	}
> -
> -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> -		      "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> -		      "SR: plane=%d, cursor=%d\n",
> -		      planea_wm, cursora_wm,
> -		      planeb_wm, cursorb_wm,
> -		      planec_wm, cursorc_wm,
> -		      plane_sr, cursor_sr);
> -
> -	I915_WRITE(DSPFW1,
> -		   (plane_sr << DSPFW_SR_SHIFT) |
> -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> -	I915_WRITE(DSPFW2,
> -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> -	I915_WRITE(DSPFW3,
> -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> -	I915_WRITE(DSPFW9_CHV,
> -		   (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> -					      DSPFW_CURSORC_MASK)) |
> -		   (planec_wm << DSPFW_PLANEC_SHIFT) |
> -		   (cursorc_wm << DSPFW_CURSORC_SHIFT));
>  
>  	if (cxsr_enabled)
>  		intel_set_memory_cxsr(dev_priv, true);
> @@ -1002,17 +995,44 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
>  	int sprite = to_intel_plane(plane)->plane;
> +	bool cxsr_enabled;
>  	struct vlv_wm_values wm = dev_priv->wm.vlv;
>  
> -	if (enabled)
> +	if (enabled) {
>  		wm.ddl[pipe].sprite[sprite] =
>  			vlv_compute_drain_latency(crtc, plane);
> -	else
> +
> +		wm.pipe[pipe].sprite[sprite] =
> +			vlv_compute_wm(intel_crtc,
> +				       to_intel_plane(plane),
> +				       vlv_get_fifo_size(dev, pipe, sprite+1));
> +	} else {
>  		wm.ddl[pipe].sprite[sprite] = 0;
> +		wm.pipe[pipe].sprite[sprite] = 0;
> +	}
> +
> +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> +
> +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> +		return;
> +
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: sprite %c=%d, "
> +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> +		      sprite_name(pipe, sprite),
> +		      wm.pipe[pipe].sprite[sprite],
> +		      wm.sr.plane, wm.sr.cursor);
> +
> +	if (!cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, false);
>  
>  	vlv_write_wm_values(intel_crtc, &wm);
> +
> +	if (cxsr_enabled)
> +		intel_set_memory_cxsr(dev_priv, true);
>  }
>  
> +#define single_plane_enabled(mask) is_power_of_2(mask)
> +
>  static void g4x_update_wm(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -6485,7 +6505,7 @@ void intel_init_pm(struct drm_device *dev)
>  		else if (INTEL_INFO(dev)->gen == 8)
>  			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
>  	} else if (IS_CHERRYVIEW(dev)) {
> -		dev_priv->display.update_wm = cherryview_update_wm;
> +		dev_priv->display.update_wm = valleyview_update_wm;
>  		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
>  		dev_priv->display.init_clock_gating =
>  			cherryview_init_clock_gating;
> -- 
> 2.0.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Ville Syrjälä]

On Tue, Mar 10, 2015 at 11:26:29AM +0100, Daniel Vetter wrote:
> On Thu, Mar 05, 2015 at 09:19:49PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Assuming the PND deadline mechanism works reasonably we should do
> > memory requests as early as possible so that PND has schedule the
> > requests more intelligently. Currently we're still calculating
> > the watermarks as if VLV/CHV are identical to g4x, which isn't
> > the case.
> > 
> > The current code also seems to calculate insufficient watermarks
> > and hence we're seeing some underruns, especially on high resolution
> > displays.
> > 
> > To fix it just rip out the current code and replace is with something
> > that tries to utilize PND as efficiently as possible.
> > 
> > We now calculate the WM watermark to trigger when the FIFO still has
> > 256us worth of data. 256us is the maximum deadline value supoorted by
> > PND, so issuing memory requests earlier would mean we probably couldn't
> > utilize the full FIFO as PND would attempt to return the data at
> > least in at least 256us. We also clamp the watermark to at least 8
> > cachelines as that's the magic watermark that enabling trickle feed
> > would also impose. I'm assuming it matches some burst size.
> > 
> > In theory we could just enable trickle feed and ignore the WM values,
> > except trickle feed doesn't work with max fifo mode anyway, so we'd
> > still need to calculate the SR watermarks. It seems cleaner to just
> > disable trickle feed and calculate all watermarks the same way. Also
> > trickle feed wouldn't account for the 256us max deadline value, thoguh
> > that may be a moot point in non-max fifo mode sicne the FIFOs are fairly
> > small.
> > 
> > On VLV max fifo mode can be used with either primary or sprite planes.
> > So the code now also checks all the planes (apart from the cursor)
> > when calculating the SR plane watermark.
> > 
> > We don't have to worry about the WM1 watermarks since we're using the
> > PND deadline scheme which means the hardware ignores WM1 values.
> > 
> > v2: Use plane->state->fb instead of plane->fb
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  11 ++
> >  drivers/gpu/drm/i915/i915_reg.h |   4 +-
> >  drivers/gpu/drm/i915/intel_pm.c | 330 +++++++++++++++++++++-------------------
> >  3 files changed, 188 insertions(+), 157 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 5de69a0..b191b12 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1516,6 +1516,17 @@ struct ilk_wm_values {
> >  
> >  struct vlv_wm_values {
> >  	struct {
> > +		uint16_t primary;
> > +		uint16_t sprite[2];
> > +		uint8_t cursor;
> > +	} pipe[3];
> > +
> > +	struct {
> > +		uint16_t plane;
> > +		uint8_t cursor;
> > +	} sr;
> > +
> > +	struct {
> >  		uint8_t cursor;
> >  		uint8_t sprite[2];
> >  		uint8_t primary;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8178610..9f98384 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4127,7 +4127,7 @@ enum skl_disp_power_wells {
> >  /* vlv/chv high order bits */
> >  #define DSPHOWM			(VLV_DISPLAY_BASE + 0x70064)
> >  #define   DSPFW_SR_HI_SHIFT		24
> > -#define   DSPFW_SR_HI_MASK		(1<<24)
> > +#define   DSPFW_SR_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
> >  #define   DSPFW_SPRITEF_HI_SHIFT	23
> >  #define   DSPFW_SPRITEF_HI_MASK		(1<<23)
> >  #define   DSPFW_SPRITEE_HI_SHIFT	22
> > @@ -4148,7 +4148,7 @@ enum skl_disp_power_wells {
> >  #define   DSPFW_PLANEA_HI_MASK		(1<<0)
> >  #define DSPHOWM1		(VLV_DISPLAY_BASE + 0x70068)
> >  #define   DSPFW_SR_WM1_HI_SHIFT		24
> > -#define   DSPFW_SR_WM1_HI_MASK		(1<<24)
> > +#define   DSPFW_SR_WM1_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
> >  #define   DSPFW_SPRITEF_WM1_HI_SHIFT	23
> >  #define   DSPFW_SPRITEF_WM1_HI_MASK	(1<<23)
> >  #define   DSPFW_SPRITEE_WM1_HI_SHIFT	22
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bdb0f5d..497847c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -778,6 +778,55 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >  		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
> >  		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
> >  
> > +	I915_WRITE(DSPFW1,
> > +		   ((wm->sr.plane << DSPFW_SR_SHIFT) & DSPFW_SR_MASK) |
> > +		   ((wm->pipe[PIPE_B].cursor << DSPFW_CURSORB_SHIFT) & DSPFW_CURSORB_MASK) |
> > +		   ((wm->pipe[PIPE_B].primary << DSPFW_PLANEB_SHIFT) & DSPFW_PLANEB_MASK_VLV) |
> > +		   ((wm->pipe[PIPE_A].primary << DSPFW_PLANEA_SHIFT) & DSPFW_PLANEA_MASK_VLV));
> > +	I915_WRITE(DSPFW2,
> > +		   ((wm->pipe[PIPE_A].sprite[1] << DSPFW_SPRITEB_SHIFT) & DSPFW_SPRITEB_MASK_VLV) |
> > +		   ((wm->pipe[PIPE_A].cursor << DSPFW_CURSORA_SHIFT) & DSPFW_CURSORA_MASK) |
> > +		   ((wm->pipe[PIPE_A].sprite[0] << DSPFW_SPRITEA_SHIFT) & DSPFW_SPRITEA_MASK_VLV));
> > +	I915_WRITE(DSPFW3,
> > +		   ((wm->sr.cursor << DSPFW_CURSOR_SR_SHIFT) & DSPFW_CURSOR_SR_MASK));
> > +
> > +	if (IS_CHERRYVIEW(dev_priv)) {
> > +		I915_WRITE(DSPFW7_CHV,
> > +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> > +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> > +		I915_WRITE(DSPFW8_CHV,
> > +			   ((wm->pipe[PIPE_C].sprite[1] << DSPFW_SPRITEF_SHIFT) & DSPFW_SPRITEF_MASK) |
> > +			   ((wm->pipe[PIPE_C].sprite[0] << DSPFW_SPRITEE_SHIFT) & DSPFW_SPRITEE_MASK));
> > +		I915_WRITE(DSPFW9_CHV,
> > +			   ((wm->pipe[PIPE_C].primary << DSPFW_PLANEC_SHIFT) & DSPFW_PLANEC_MASK) |
> > +			   ((wm->pipe[PIPE_C].cursor << DSPFW_CURSORC_SHIFT) & DSPFW_CURSORC_MASK));
> > +		I915_WRITE(DSPHOWM,
> > +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].sprite[1] >> 8) << DSPFW_SPRITEF_HI_SHIFT) & DSPFW_SPRITEF_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].sprite[0] >> 8) << DSPFW_SPRITEE_HI_SHIFT) & DSPFW_SPRITEE_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].primary >> 8) << DSPFW_PLANEC_HI_SHIFT) & DSPFW_PLANEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> > +	} else {
> > +		I915_WRITE(DSPFW7,
> > +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> > +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> > +		I915_WRITE(DSPHOWM,
> > +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> 
> Checkpatch complained about this code and I concur that it's not pretty.
> Care for a prettification follow-up? Some #defines to do the shift+mask
> would help a lot imo. Or maybe not computing everything in-place.

Yeah, that part is about as ugly as you can get. I did think about
wrapping the magic in a macro, but decided that I didn't want to waste
my time with too much polish at that point. Now that things are a bit
more calm perhaps, I can take another look at that. Wrapping it up would
also reduce the chances of a typo somewhere.

> -Daniel
> 
> > +	}
> > +
> > +	POSTING_READ(DSPFW1);
> > +
> >  	dev_priv->wm.vlv = *wm;
> >  }
> >  
> > @@ -822,169 +871,113 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
> >  				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
> >  }
> >  
> > -/*
> > - * Update drain latency registers of memory arbiter
> > - *
> > - * Valleyview SoC has a new memory arbiter and needs drain latency registers
> > - * to be programmed. Each plane has a drain latency multiplier and a drain
> > - * latency value.
> > - */
> > -
> > -static void vlv_update_drain_latency(struct drm_crtc *crtc)
> > +static int vlv_compute_wm(struct intel_crtc *crtc,
> > +			  struct intel_plane *plane,
> > +			  int fifo_size)
> > +{
> > +	int clock, entries, pixel_size;
> > +
> > +	/*
> > +	 * FIXME the plane might have an fb
> > +	 * but be invisible (eg. due to clipping)
> > +	 */
> > +	if (!crtc->active || !plane->base.state->fb)
> > +		return 0;
> > +
> > +	pixel_size = drm_format_plane_cpp(plane->base.state->fb->pixel_format, 0);
> > +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> > +
> > +	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> > +
> > +	/*
> > +	 * Set up the watermark such that we don't start issuing memory
> > +	 * requests until we are within PND's max deadline value (256us).
> > +	 * Idea being to be idle as long as possible while still taking
> > +	 * advatange of PND's deadline scheduling. The limit of 8
> > +	 * cachelines (used when the FIFO will anyway drain in less time
> > +	 * than 256us) should match what we would be done if trickle
> > +	 * feed were enabled.
> > +	 */
> > +	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
> > +}
> > +
> > +static bool vlv_compute_sr_wm(struct drm_device *dev,
> > +			      struct vlv_wm_values *wm)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_crtc *crtc;
> > +	enum pipe pipe = INVALID_PIPE;
> > +	int num_planes = 0;
> > +	int fifo_size = 0;
> > +	struct intel_plane *plane;
> > +
> > +	wm->sr.cursor = wm->sr.plane = 0;
> > +
> > +	crtc = single_enabled_crtc(dev);
> > +	/* maxfifo not supported on pipe C */
> > +	if (crtc && to_intel_crtc(crtc)->pipe != PIPE_C) {
> > +		pipe = to_intel_crtc(crtc)->pipe;
> > +		num_planes = !!wm->pipe[pipe].primary +
> > +			!!wm->pipe[pipe].sprite[0] +
> > +			!!wm->pipe[pipe].sprite[1];
> > +		fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
> > +	}
> > +
> > +	if (fifo_size == 0 || num_planes > 1)
> > +		return false;
> > +
> > +	wm->sr.cursor = vlv_compute_wm(to_intel_crtc(crtc),
> > +				       to_intel_plane(crtc->cursor), 0x3f);
> > +
> > +	list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> > +			continue;
> > +
> > +		if (plane->pipe != pipe)
> > +			continue;
> > +
> > +		wm->sr.plane = vlv_compute_wm(to_intel_crtc(crtc),
> > +					      plane, fifo_size);
> > +		if (wm->sr.plane != 0)
> > +			break;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void valleyview_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> > +	bool cxsr_enabled;
> >  	struct vlv_wm_values wm = dev_priv->wm.vlv;
> >  
> >  	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
> > +	wm.pipe[pipe].primary = vlv_compute_wm(intel_crtc,
> > +					       to_intel_plane(crtc->primary),
> > +					       vlv_get_fifo_size(dev, pipe, 0));
> > +
> >  	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
> > +	wm.pipe[pipe].cursor = vlv_compute_wm(intel_crtc,
> > +					      to_intel_plane(crtc->cursor),
> > +					      0x3f);
> > +
> > +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> > +
> > +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> > +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> > +		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> > +		      wm.sr.plane, wm.sr.cursor);
> > +
> > +	if (!cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, false);
> >  
> >  	vlv_write_wm_values(intel_crtc, &wm);
> > -}
> > -
> > -#define single_plane_enabled(mask) is_power_of_2(mask)
> > -
> > -static void valleyview_update_wm(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	static const int sr_latency_ns = 12000;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> > -	int plane_sr, cursor_sr;
> > -	int ignore_plane_sr, ignore_cursor_sr;
> > -	unsigned int enabled = 0;
> > -	bool cxsr_enabled;
> > -
> > -	vlv_update_drain_latency(crtc);
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_A,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planea_wm, &cursora_wm))
> > -		enabled |= 1 << PIPE_A;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_B,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planeb_wm, &cursorb_wm))
> > -		enabled |= 1 << PIPE_B;
> > -
> > -	if (single_plane_enabled(enabled) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &plane_sr, &ignore_cursor_sr) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     2*sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &ignore_plane_sr, &cursor_sr)) {
> > -		cxsr_enabled = true;
> > -	} else {
> > -		cxsr_enabled = false;
> > -		intel_set_memory_cxsr(dev_priv, false);
> > -		plane_sr = cursor_sr = 0;
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > -		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> > -		      planea_wm, cursora_wm,
> > -		      planeb_wm, cursorb_wm,
> > -		      plane_sr, cursor_sr);
> > -
> > -	I915_WRITE(DSPFW1,
> > -		   (plane_sr << DSPFW_SR_SHIFT) |
> > -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> > -	I915_WRITE(DSPFW2,
> > -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > -	I915_WRITE(DSPFW3,
> > -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > -
> > -	if (cxsr_enabled)
> > -		intel_set_memory_cxsr(dev_priv, true);
> > -}
> > -
> > -static void cherryview_update_wm(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	static const int sr_latency_ns = 12000;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int planea_wm, planeb_wm, planec_wm;
> > -	int cursora_wm, cursorb_wm, cursorc_wm;
> > -	int plane_sr, cursor_sr;
> > -	int ignore_plane_sr, ignore_cursor_sr;
> > -	unsigned int enabled = 0;
> > -	bool cxsr_enabled;
> > -
> > -	vlv_update_drain_latency(crtc);
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_A,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planea_wm, &cursora_wm))
> > -		enabled |= 1 << PIPE_A;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_B,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planeb_wm, &cursorb_wm))
> > -		enabled |= 1 << PIPE_B;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_C,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planec_wm, &cursorc_wm))
> > -		enabled |= 1 << PIPE_C;
> > -
> > -	if (single_plane_enabled(enabled) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &plane_sr, &ignore_cursor_sr) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     2*sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &ignore_plane_sr, &cursor_sr)) {
> > -		cxsr_enabled = true;
> > -	} else {
> > -		cxsr_enabled = false;
> > -		intel_set_memory_cxsr(dev_priv, false);
> > -		plane_sr = cursor_sr = 0;
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > -		      "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> > -		      "SR: plane=%d, cursor=%d\n",
> > -		      planea_wm, cursora_wm,
> > -		      planeb_wm, cursorb_wm,
> > -		      planec_wm, cursorc_wm,
> > -		      plane_sr, cursor_sr);
> > -
> > -	I915_WRITE(DSPFW1,
> > -		   (plane_sr << DSPFW_SR_SHIFT) |
> > -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> > -	I915_WRITE(DSPFW2,
> > -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > -	I915_WRITE(DSPFW3,
> > -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > -	I915_WRITE(DSPFW9_CHV,
> > -		   (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> > -					      DSPFW_CURSORC_MASK)) |
> > -		   (planec_wm << DSPFW_PLANEC_SHIFT) |
> > -		   (cursorc_wm << DSPFW_CURSORC_SHIFT));
> >  
> >  	if (cxsr_enabled)
> >  		intel_set_memory_cxsr(dev_priv, true);
> > @@ -1002,17 +995,44 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> >  	int sprite = to_intel_plane(plane)->plane;
> > +	bool cxsr_enabled;
> >  	struct vlv_wm_values wm = dev_priv->wm.vlv;
> >  
> > -	if (enabled)
> > +	if (enabled) {
> >  		wm.ddl[pipe].sprite[sprite] =
> >  			vlv_compute_drain_latency(crtc, plane);
> > -	else
> > +
> > +		wm.pipe[pipe].sprite[sprite] =
> > +			vlv_compute_wm(intel_crtc,
> > +				       to_intel_plane(plane),
> > +				       vlv_get_fifo_size(dev, pipe, sprite+1));
> > +	} else {
> >  		wm.ddl[pipe].sprite[sprite] = 0;
> > +		wm.pipe[pipe].sprite[sprite] = 0;
> > +	}
> > +
> > +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> > +
> > +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: sprite %c=%d, "
> > +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> > +		      sprite_name(pipe, sprite),
> > +		      wm.pipe[pipe].sprite[sprite],
> > +		      wm.sr.plane, wm.sr.cursor);
> > +
> > +	if (!cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, false);
> >  
> >  	vlv_write_wm_values(intel_crtc, &wm);
> > +
> > +	if (cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, true);
> >  }
> >  
> > +#define single_plane_enabled(mask) is_power_of_2(mask)
> > +
> >  static void g4x_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -6485,7 +6505,7 @@ void intel_init_pm(struct drm_device *dev)
> >  		else if (INTEL_INFO(dev)->gen == 8)
> >  			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
> >  	} else if (IS_CHERRYVIEW(dev)) {
> > -		dev_priv->display.update_wm = cherryview_update_wm;
> > +		dev_priv->display.update_wm = valleyview_update_wm;
> >  		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> >  		dev_priv->display.init_clock_gating =
> >  			cherryview_init_clock_gating;
> > -- 
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[PATCH v4 10/12] drm/i915: Program PFI credits for VLV

[ville.syrjala]

From: Vidya Srinivas <vidya.srinivas@intel.com>

PFI credit programming is required when CD clock (related to data flow from
display pipeline to end display) is greater than CZ clock (related to data
flow from memory to display plane). This programming should be done when all
planes are OFF to avoid intermittent hangs while accessing memory even from
different Gfx units (not just display).

If cdclk/czclk >=1, PFI credits could be set as any number. To get better
performance, larger PFI credit can be assigned to PND. Otherwise if
cdclk/czclk<1, the default PFI credit of 8 should be set.

v2:
    - Change log to lower log level instead of DRM_ERROR
    - Change function name to valleyview_program_pfi_credits
    - Move program PFI credits to modeset_init instead of intel_set_mode
    - Change magic numbers to logical constants

[vsyrjala v3:
 - only program in response to cdclk update
 - program the credits also when cdclk<czclk
 - add CHV bits
 v4:
 - Change CHV cdclk<czclk credits to 12 (Vijay)]

Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9f98384..145f0d4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2061,6 +2061,14 @@ enum skl_disp_power_wells {
 #define   CDCLK_FREQ_SHIFT	4
 #define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
 #define   CZCLK_FREQ_MASK	0xf
+
+#define GCI_CONTROL		(VLV_DISPLAY_BASE + 0x650C)
+#define   PFI_CREDIT_63		(9 << 28)		/* chv only */
+#define   PFI_CREDIT_31		(8 << 28)		/* chv only */
+#define   PFI_CREDIT(x)		(((x) - 8) << 28)	/* 8-15 */
+#define   PFI_CREDIT_RESEND	(1 << 27)
+#define   VGA_FAST_MODE_DISABLE	(1 << 14)
+
 #define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3fe9598..29ee72d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4987,6 +4987,42 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev,
 			*prepare_pipes |= (1 << intel_crtc->pipe);
 }
 
+static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
+{
+	unsigned int credits, default_credits;
+
+	if (IS_CHERRYVIEW(dev_priv))
+		default_credits = PFI_CREDIT(12);
+	else
+		default_credits = PFI_CREDIT(8);
+
+	if (DIV_ROUND_CLOSEST(dev_priv->vlv_cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
+		/* CHV suggested value is 31 or 63 */
+		if (IS_CHERRYVIEW(dev_priv))
+			credits = PFI_CREDIT_31;
+		else
+			credits = PFI_CREDIT(15);
+	} else {
+		credits = default_credits;
+	}
+
+	/*
+	 * WA - write default credits before re-programming
+	 * FIXME: should we also set the resend bit here?
+	 */
+	I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE |
+		   default_credits);
+
+	I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE |
+		   credits | PFI_CREDIT_RESEND);
+
+	/*
+	 * FIXME is this guaranteed to clear
+	 * immediately or should we poll for it?
+	 */
+	WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
+}
+
 static void valleyview_modeset_global_resources(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5010,6 +5046,8 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
 		else
 			valleyview_set_cdclk(dev, req_cdclk);
 
+		vlv_program_pfi_credits(dev_priv);
+
 		intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
 	}
 }
-- 
2.0.5

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

Re: [PATCH v4 10/12] drm/i915: Program PFI credits for VLV

[Purushothaman, Vijay A]

On 3/6/2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> From: Vidya Srinivas <vidya.srinivas@intel.com>
>
> PFI credit programming is required when CD clock (related to data flow from
> display pipeline to end display) is greater than CZ clock (related to data
> flow from memory to display plane). This programming should be done when all
> planes are OFF to avoid intermittent hangs while accessing memory even from
> different Gfx units (not just display).
>
> If cdclk/czclk >=1, PFI credits could be set as any number. To get better
> performance, larger PFI credit can be assigned to PND. Otherwise if
> cdclk/czclk<1, the default PFI credit of 8 should be set.
>
> v2:
>      - Change log to lower log level instead of DRM_ERROR
>      - Change function name to valleyview_program_pfi_credits
>      - Move program PFI credits to modeset_init instead of intel_set_mode
>      - Change magic numbers to logical constants
>
> [vsyrjala v3:
>   - only program in response to cdclk update
>   - program the credits also when cdclk<czclk
>   - add CHV bits
>   v4:
>   - Change CHV cdclk<czclk credits to 12 (Vijay)]
>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>

Thanks,
Vijay

>   drivers/gpu/drm/i915/i915_reg.h      |  8 ++++++++
>   drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9f98384..145f0d4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2061,6 +2061,14 @@ enum skl_disp_power_wells {
>   #define   CDCLK_FREQ_SHIFT	4
>   #define   CDCLK_FREQ_MASK	(0x1f << CDCLK_FREQ_SHIFT)
>   #define   CZCLK_FREQ_MASK	0xf
> +
> +#define GCI_CONTROL		(VLV_DISPLAY_BASE + 0x650C)
> +#define   PFI_CREDIT_63		(9 << 28)		/* chv only */
> +#define   PFI_CREDIT_31		(8 << 28)		/* chv only */
> +#define   PFI_CREDIT(x)		(((x) - 8) << 28)	/* 8-15 */
> +#define   PFI_CREDIT_RESEND	(1 << 27)
> +#define   VGA_FAST_MODE_DISABLE	(1 << 14)
> +
>   #define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
>   
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3fe9598..29ee72d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4987,6 +4987,42 @@ static void valleyview_modeset_global_pipes(struct drm_device *dev,
>   			*prepare_pipes |= (1 << intel_crtc->pipe);
>   }
>   
> +static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> +{
> +	unsigned int credits, default_credits;
> +
> +	if (IS_CHERRYVIEW(dev_priv))
> +		default_credits = PFI_CREDIT(12);
> +	else
> +		default_credits = PFI_CREDIT(8);
> +
> +	if (DIV_ROUND_CLOSEST(dev_priv->vlv_cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
> +		/* CHV suggested value is 31 or 63 */
> +		if (IS_CHERRYVIEW(dev_priv))
> +			credits = PFI_CREDIT_31;
> +		else
> +			credits = PFI_CREDIT(15);
> +	} else {
> +		credits = default_credits;
> +	}
> +
> +	/*
> +	 * WA - write default credits before re-programming
> +	 * FIXME: should we also set the resend bit here?
> +	 */
> +	I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE |
> +		   default_credits);
> +
> +	I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE |
> +		   credits | PFI_CREDIT_RESEND);
> +
> +	/*
> +	 * FIXME is this guaranteed to clear
> +	 * immediately or should we poll for it?
> +	 */
> +	WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
> +}
> +
>   static void valleyview_modeset_global_resources(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5010,6 +5046,8 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
>   		else
>   			valleyview_set_cdclk(dev, req_cdclk);
>   
> +		vlv_program_pfi_credits(dev_priv);
> +
>   		intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
>   	}
>   }

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

Re: [PATCH v4 10/12] drm/i915: Program PFI credits for VLV

[Daniel Vetter]

On Tue, Mar 10, 2015 at 03:35:19PM +0530, Purushothaman, Vijay A wrote:
> On 3/6/2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> >From: Vidya Srinivas <vidya.srinivas@intel.com>
> >
> >PFI credit programming is required when CD clock (related to data flow from
> >display pipeline to end display) is greater than CZ clock (related to data
> >flow from memory to display plane). This programming should be done when all
> >planes are OFF to avoid intermittent hangs while accessing memory even from
> >different Gfx units (not just display).
> >
> >If cdclk/czclk >=1, PFI credits could be set as any number. To get better
> >performance, larger PFI credit can be assigned to PND. Otherwise if
> >cdclk/czclk<1, the default PFI credit of 8 should be set.
> >
> >v2:
> >     - Change log to lower log level instead of DRM_ERROR
> >     - Change function name to valleyview_program_pfi_credits
> >     - Move program PFI credits to modeset_init instead of intel_set_mode
> >     - Change magic numbers to logical constants
> >
> >[vsyrjala v3:
> >  - only program in response to cdclk update
> >  - program the credits also when cdclk<czclk
> >  - add CHV bits
> >  v4:
> >  - Change CHV cdclk<czclk credits to 12 (Vijay)]
> >
> >Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> >Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> >Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> 
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>

Pulled in the remaining 4 patches from this series, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 11/12] drm/i915: Enable the maxfifo PM5 mode when appropriate on CHV

[ville.syrjala]

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

CHV has a new knob in Punit to select between some memory power savings
modes PM2 and PM5. We can allow the deeper PM5 when maxfifo mode is
enabled, so let's do so in the hopes for moar power savings.

v2: Put the thing into a separate function to avoid churn later
v3: Don't break VLV

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  3 +++
 drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 145f0d4..5a20f58 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -552,6 +552,9 @@
 #define   DSPFREQSTAT_MASK			(0x3 << DSPFREQSTAT_SHIFT)
 #define   DSPFREQGUAR_SHIFT			14
 #define   DSPFREQGUAR_MASK			(0x3 << DSPFREQGUAR_SHIFT)
+#define   DSP_MAXFIFO_PM5_STATUS		(1 << 22) /* chv */
+#define   DSP_AUTO_CDCLK_GATE_DISABLE		(1 << 7) /* chv */
+#define   DSP_MAXFIFO_PM5_ENABLE		(1 << 6) /* chv */
 #define   _DP_SSC(val, pipe)			((val) << (2 * (pipe)))
 #define   DP_SSC_MASK(pipe)			_DP_SSC(0x3, (pipe))
 #define   DP_SSC_PWR_ON(pipe)			_DP_SSC(0x0, (pipe))
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 497847c..1dd82ec 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -235,6 +235,22 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
 	return NULL;
 }
 
+static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
+{
+	u32 val;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+
+	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
+	if (enable)
+		val |= DSP_MAXFIFO_PM5_ENABLE;
+	else
+		val &= ~DSP_MAXFIFO_PM5_ENABLE;
+	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
+
 void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -242,6 +258,8 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 
 	if (IS_VALLEYVIEW(dev)) {
 		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
+		if (IS_CHERRYVIEW(dev))
+			chv_set_memory_pm5(dev_priv, enable);
 	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
 		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
 	} else if (IS_PINEVIEW(dev)) {
-- 
2.0.5

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

Re: [PATCH v3 11/12] drm/i915: Enable the maxfifo PM5 mode when appropriate on CHV

[Arun R Murthy]

On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> CHV has a new knob in Punit to select between some memory power savings
> modes PM2 and PM5. We can allow the deeper PM5 when maxfifo mode is
> enabled, so let's do so in the hopes for moar power savings.
>
> v2: Put the thing into a separate function to avoid churn later
> v3: Don't break VLV
>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,
Arun R Murthy
-------------------

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

[PATCH 12/12] drm/i915: Disable DDR DVFS on CHV

[ville.syrjala]

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

DDR DVFS introduces massive memory latencies which can't be handled by
the PND deadline stuff. Instead the watermarks will need to be
programmed to compensate for the latency and the deadlines will need to
be programmed to tight fixed values. That means DDR DVFS can only be
enabled if the display FIFOs are large enough, and that pretty much
means we have to manually repartition them to suit the needs of the
moment.

That's a lot of change, so in the meantime let's just disable DDR DVFS
to get the display(s) to be stable.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  5 +++++
 drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5a20f58..744d162 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -630,6 +630,11 @@ enum skl_disp_power_wells {
 #define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
 #define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
 
+#define PUNIT_REG_DDR_SETUP2			0x139
+#define   FORCE_DDR_FREQ_REQ_ACK		(1 << 8)
+#define   FORCE_DDR_LOW_FREQ			(1 << 1)
+#define   FORCE_DDR_HIGH_FREQ			(1 << 0)
+
 #define PUNIT_GPU_STATUS_REG			0xdb
 #define PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
 #define PUNIT_GPU_STATUS_MAX_FREQ_MASK		0xff
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1dd82ec..fc03e24 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -235,6 +235,28 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
 	return NULL;
 }
 
+static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
+{
+	u32 val;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+
+	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
+	if (enable)
+		val &= ~FORCE_DDR_HIGH_FREQ;
+	else
+		val |= FORCE_DDR_HIGH_FREQ;
+	val &= ~FORCE_DDR_LOW_FREQ;
+	val |= FORCE_DDR_FREQ_REQ_ACK;
+	vlv_punit_write(dev_priv, PUNIT_REG_DDR_SETUP2, val);
+
+	if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2) &
+		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
+		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
+
 static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 {
 	u32 val;
@@ -282,6 +304,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 		      enable ? "enabled" : "disabled");
 }
 
+
 /*
  * Latency for FIFO fetches is dependent on several factors:
  *   - memory configuration (speed, channels)
@@ -992,6 +1015,17 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
 		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
 		      wm.sr.plane, wm.sr.cursor);
 
+	/*
+	 * FIXME DDR DVFS introduces massive memory latencies which
+	 * are not known to system agent so any deadline specified
+	 * by the display may not be respected. To support DDR DVFS
+	 * the watermark code needs to be rewritten to essentially
+	 * bypass deadline mechanism and rely solely on the
+	 * watermarks. For now disable DDR DVFS.
+	 */
+	if (IS_CHERRYVIEW(dev_priv))
+		chv_set_memory_dvfs(dev_priv, false);
+
 	if (!cxsr_enabled)
 		intel_set_memory_cxsr(dev_priv, false);
 
-- 
2.0.5

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

Re: [PATCH 12/12] drm/i915: Disable DDR DVFS on CHV

[Jesse Barnes]

On 03/05/2015 11:19 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DDR DVFS introduces massive memory latencies which can't be handled by
> the PND deadline stuff. Instead the watermarks will need to be
> programmed to compensate for the latency and the deadlines will need to
> be programmed to tight fixed values. That means DDR DVFS can only be
> enabled if the display FIFOs are large enough, and that pretty much
> means we have to manually repartition them to suit the needs of the
> moment.
> 
> That's a lot of change, so in the meantime let's just disable DDR DVFS
> to get the display(s) to be stable.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  5 +++++
>  drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5a20f58..744d162 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -630,6 +630,11 @@ enum skl_disp_power_wells {
>  #define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
>  #define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
>  
> +#define PUNIT_REG_DDR_SETUP2			0x139
> +#define   FORCE_DDR_FREQ_REQ_ACK		(1 << 8)
> +#define   FORCE_DDR_LOW_FREQ			(1 << 1)
> +#define   FORCE_DDR_HIGH_FREQ			(1 << 0)
> +
>  #define PUNIT_GPU_STATUS_REG			0xdb
>  #define PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
>  #define PUNIT_GPU_STATUS_MAX_FREQ_MASK		0xff
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1dd82ec..fc03e24 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -235,6 +235,28 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
>  	return NULL;
>  }
>  
> +static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> +{
> +	u32 val;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +
> +	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> +	if (enable)
> +		val &= ~FORCE_DDR_HIGH_FREQ;
> +	else
> +		val |= FORCE_DDR_HIGH_FREQ;
> +	val &= ~FORCE_DDR_LOW_FREQ;
> +	val |= FORCE_DDR_FREQ_REQ_ACK;
> +	vlv_punit_write(dev_priv, PUNIT_REG_DDR_SETUP2, val);
> +
> +	if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2) &
> +		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> +		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> +
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
>  static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>  {
>  	u32 val;
> @@ -282,6 +304,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
>  		      enable ? "enabled" : "disabled");
>  }
>  
> +
>  /*
>   * Latency for FIFO fetches is dependent on several factors:
>   *   - memory configuration (speed, channels)
> @@ -992,6 +1015,17 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>  		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
>  		      wm.sr.plane, wm.sr.cursor);
>  
> +	/*
> +	 * FIXME DDR DVFS introduces massive memory latencies which
> +	 * are not known to system agent so any deadline specified
> +	 * by the display may not be respected. To support DDR DVFS
> +	 * the watermark code needs to be rewritten to essentially
> +	 * bypass deadline mechanism and rely solely on the
> +	 * watermarks. For now disable DDR DVFS.
> +	 */
> +	if (IS_CHERRYVIEW(dev_priv))
> +		chv_set_memory_dvfs(dev_priv, false);
> +
>  	if (!cxsr_enabled)
>  		intel_set_memory_cxsr(dev_priv, false);
>  
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Disable DDR DVFS on CHV

[Arun R Murthy]

On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> DDR DVFS introduces massive memory latencies which can't be handled by
> the PND deadline stuff. Instead the watermarks will need to be
> programmed to compensate for the latency and the deadlines will need to
> be programmed to tight fixed values. That means DDR DVFS can only be
> enabled if the display FIFOs are large enough, and that pretty much
> means we have to manually repartition them to suit the needs of the
> moment.
>
> That's a lot of change, so in the meantime let's just disable DDR DVFS
> to get the display(s) to be stable.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

Since this patch disabled DDR DVFS in the driver, can this be done
once. Here I assume that DDR DVFS gets disabled on each update
watermark call.
Moreover enabling DDR DVFS should be followed with updating
watermarks and display arbiter.

Thanks and Regards,
Arun R Murthy
-------------------
>   drivers/gpu/drm/i915/i915_reg.h |  5 +++++
>   drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5a20f58..744d162 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -630,6 +630,11 @@ enum skl_disp_power_wells {
>   #define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
>   #define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
>   
> +#define PUNIT_REG_DDR_SETUP2			0x139
> +#define   FORCE_DDR_FREQ_REQ_ACK		(1 << 8)
> +#define   FORCE_DDR_LOW_FREQ			(1 << 1)
> +#define   FORCE_DDR_HIGH_FREQ			(1 << 0)
> +
>   #define PUNIT_GPU_STATUS_REG			0xdb
>   #define PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
>   #define PUNIT_GPU_STATUS_MAX_FREQ_MASK		0xff
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1dd82ec..fc03e24 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -235,6 +235,28 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
>   	return NULL;
>   }
>   
> +static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> +{
> +	u32 val;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +
> +	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> +	if (enable)
> +		val &= ~FORCE_DDR_HIGH_FREQ;
> +	else
> +		val |= FORCE_DDR_HIGH_FREQ;
> +	val &= ~FORCE_DDR_LOW_FREQ;
> +	val |= FORCE_DDR_FREQ_REQ_ACK;
> +	vlv_punit_write(dev_priv, PUNIT_REG_DDR_SETUP2, val);
> +
> +	if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2) &
> +		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> +		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> +
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
>   static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>   {
>   	u32 val;
> @@ -282,6 +304,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
>   		      enable ? "enabled" : "disabled");
>   }
>   
> +
>   /*
>    * Latency for FIFO fetches is dependent on several factors:
>    *   - memory configuration (speed, channels)
> @@ -992,6 +1015,17 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>   		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
>   		      wm.sr.plane, wm.sr.cursor);
>   
> +	/*
> +	 * FIXME DDR DVFS introduces massive memory latencies which
> +	 * are not known to system agent so any deadline specified
> +	 * by the display may not be respected. To support DDR DVFS
> +	 * the watermark code needs to be rewritten to essentially
> +	 * bypass deadline mechanism and rely solely on the
> +	 * watermarks. For now disable DDR DVFS.
> +	 */
> +	if (IS_CHERRYVIEW(dev_priv))
> +		chv_set_memory_dvfs(dev_priv, false);
> +
>   	if (!cxsr_enabled)
>   		intel_set_memory_cxsr(dev_priv, false);
>   

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

Re: [PATCH 12/12] drm/i915: Disable DDR DVFS on CHV

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 10:14:05AM +0530, Arun R Murthy wrote:
> 
> On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > DDR DVFS introduces massive memory latencies which can't be handled by
> > the PND deadline stuff. Instead the watermarks will need to be
> > programmed to compensate for the latency and the deadlines will need to
> > be programmed to tight fixed values. That means DDR DVFS can only be
> > enabled if the display FIFOs are large enough, and that pretty much
> > means we have to manually repartition them to suit the needs of the
> > moment.
> >
> > That's a lot of change, so in the meantime let's just disable DDR DVFS
> > to get the display(s) to be stable.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> 
> Since this patch disabled DDR DVFS in the driver, can this be done
> once. Here I assume that DDR DVFS gets disabled on each update
> watermark call.

Yeah it did occur to me that we could get away with disabling just once,
but once we get the DDR DVFS enabled we're going to be changing it more,
potentially at every watermark update (although having to do it every
time is rather unlikely).

In any case, you may have noticed that I skip the entire WM programming
if the calculated watermarks didn't change from the old values, so we
won't be frobbing the Punit unless something in the WM values has actually
changed. So I figured that's good enough for now.

My plan is to optimize away redundant Punit communication (for both PM5
and DDR DVFS) eventually, but that is part of the task to get DDR DVFS
enabled in the first place.

> Moreover enabling DDR DVFS should be followed with updating
> watermarks and display arbiter.
> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> >   drivers/gpu/drm/i915/i915_reg.h |  5 +++++
> >   drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 5a20f58..744d162 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -630,6 +630,11 @@ enum skl_disp_power_wells {
> >   #define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
> >   #define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
> >   
> > +#define PUNIT_REG_DDR_SETUP2			0x139
> > +#define   FORCE_DDR_FREQ_REQ_ACK		(1 << 8)
> > +#define   FORCE_DDR_LOW_FREQ			(1 << 1)
> > +#define   FORCE_DDR_HIGH_FREQ			(1 << 0)
> > +
> >   #define PUNIT_GPU_STATUS_REG			0xdb
> >   #define PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
> >   #define PUNIT_GPU_STATUS_MAX_FREQ_MASK		0xff
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1dd82ec..fc03e24 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -235,6 +235,28 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
> >   	return NULL;
> >   }
> >   
> > +static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> > +{
> > +	u32 val;
> > +
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +
> > +	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> > +	if (enable)
> > +		val &= ~FORCE_DDR_HIGH_FREQ;
> > +	else
> > +		val |= FORCE_DDR_HIGH_FREQ;
> > +	val &= ~FORCE_DDR_LOW_FREQ;
> > +	val |= FORCE_DDR_FREQ_REQ_ACK;
> > +	vlv_punit_write(dev_priv, PUNIT_REG_DDR_SETUP2, val);
> > +
> > +	if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2) &
> > +		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> > +		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> > +
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +}
> > +
> >   static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >   {
> >   	u32 val;
> > @@ -282,6 +304,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
> >   		      enable ? "enabled" : "disabled");
> >   }
> >   
> > +
> >   /*
> >    * Latency for FIFO fetches is dependent on several factors:
> >    *   - memory configuration (speed, channels)
> > @@ -992,6 +1015,17 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> >   		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> >   		      wm.sr.plane, wm.sr.cursor);
> >   
> > +	/*
> > +	 * FIXME DDR DVFS introduces massive memory latencies which
> > +	 * are not known to system agent so any deadline specified
> > +	 * by the display may not be respected. To support DDR DVFS
> > +	 * the watermark code needs to be rewritten to essentially
> > +	 * bypass deadline mechanism and rely solely on the
> > +	 * watermarks. For now disable DDR DVFS.
> > +	 */
> > +	if (IS_CHERRYVIEW(dev_priv))
> > +		chv_set_memory_dvfs(dev_priv, false);
> > +
> >   	if (!cxsr_enabled)
> >   		intel_set_memory_cxsr(dev_priv, false);
> >   

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

Re: [PATCH 12/12] drm/i915: Disable DDR DVFS on CHV

[Daniel Vetter]

On Mon, Mar 09, 2015 at 05:00:09PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 10:14:05AM +0530, Arun R Murthy wrote:
> > 
> > On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > DDR DVFS introduces massive memory latencies which can't be handled by
> > > the PND deadline stuff. Instead the watermarks will need to be
> > > programmed to compensate for the latency and the deadlines will need to
> > > be programmed to tight fixed values. That means DDR DVFS can only be
> > > enabled if the display FIFOs are large enough, and that pretty much
> > > means we have to manually repartition them to suit the needs of the
> > > moment.
> > >
> > > That's a lot of change, so in the meantime let's just disable DDR DVFS
> > > to get the display(s) to be stable.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > 
> > Since this patch disabled DDR DVFS in the driver, can this be done
> > once. Here I assume that DDR DVFS gets disabled on each update
> > watermark call.
> 
> Yeah it did occur to me that we could get away with disabling just once,
> but once we get the DDR DVFS enabled we're going to be changing it more,
> potentially at every watermark update (although having to do it every
> time is rather unlikely).
> 
> In any case, you may have noticed that I skip the entire WM programming
> if the calculated watermarks didn't change from the old values, so we
> won't be frobbing the Punit unless something in the WM values has actually
> changed. So I figured that's good enough for now.
> 
> My plan is to optimize away redundant Punit communication (for both PM5
> and DDR DVFS) eventually, but that is part of the task to get DDR DVFS
> enabled in the first place.

Yeah I think this plan makes sense long-term. Generally I'd like to move
the wm code towards always recomputing everything and optimizing out no-op
updates afterwards. Atm we have have a split between the code that decides
when to recompute wm and the code that recomputes them, and that tends to
be a bit fragile. Especially since we add a lot more complexity to the wm
code with new features like Y-tiling or atomic updates.
-Daneil
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx