[PATCH 00/10] drm/i915: Another WM rewrite to enable DDR DVFS on CHV

[PATCH 00/10] drm/i915: Another WM rewrite to enable DDR DVFS on CHV

[ville.syrjala]

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

So here we go again. Yet another attempt at making CHV watermarks sane.
This time we totally back to a memory latency based approach so that DDR DVFS
and PM5 can be enabled safely.

I also opted to follow the same path for VLV to avoid too much differences
between the platforms, and to get decent memory self refresh residency numbers.

This is now starting to resemble the ILK way of doing things quite a bit, so
hopefully that will make the eventual two stage WM update easier to achieve
on all platforms.

Ville Syrjälä (10):
  drm/i915: POSTING_READ() in intel_set_memory_cxsr()
  drm/i915: Split atomic wm update to pre and post variants
  drm/i915: Read wm values from hardware at init on CHV
  drm/i915: CHV DDR DVFS support and another watermark rewrite
  drm/i915: Compute display FIFO split dynamically for CHV
  drm/i915: Use the memory latency based WM computation on VLV too
  drm/i915: Try to make sure cxsr is disabled around plane
    enable/disable
  drm/i915: Don't do PM5/DDR DVFS with multiple pipes
  drm/i915: Add debugfs knobs for VLVCHV memory latency values
  drm/i915: Zero unused WM1 watermarks on VLV/CHV

 drivers/gpu/drm/i915/i915_debugfs.c  |  24 +-
 drivers/gpu/drm/i915/i915_drv.h      |  30 +-
 drivers/gpu/drm/i915/i915_reg.h      |  25 +-
 drivers/gpu/drm/i915/intel_display.c |  53 ++-
 drivers/gpu/drm/i915/intel_drv.h     |  18 +-
 drivers/gpu/drm/i915/intel_pm.c      | 708 +++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_sprite.c  |   6 -
 7 files changed, 679 insertions(+), 185 deletions(-)

-- 
2.3.6

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

[PATCH 01/10] drm/i915: POSTING_READ() in intel_set_memory_cxsr()

[ville.syrjala]

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

We want cxsr exit to happen ASAP, so toss in some POSTING_READ()s to
make sure things are really kicked off.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 32ff034..9706275 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -334,22 +334,27 @@ 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);
+		POSTING_READ(FW_BLC_SELF_VLV);
 		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);
+		POSTING_READ(FW_BLC_SELF);
 	} else if (IS_PINEVIEW(dev)) {
 		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
 		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
 		I915_WRITE(DSPFW3, val);
+		POSTING_READ(DSPFW3);
 	} else if (IS_I945G(dev) || IS_I945GM(dev)) {
 		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
 			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
 		I915_WRITE(FW_BLC_SELF, val);
+		POSTING_READ(FW_BLC_SELF);
 	} else if (IS_I915GM(dev)) {
 		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
 			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
 		I915_WRITE(INSTPM, val);
+		POSTING_READ(INSTPM);
 	} else {
 		return;
 	}
-- 
2.3.6

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

Re: [PATCH 01/10] drm/i915: POSTING_READ() in intel_set_memory_cxsr()

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We want cxsr exit to happen ASAP, so toss in some POSTING_READ()s to
> make sure things are really kicked off.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 32ff034..9706275 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -334,22 +334,27 @@ 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);
> +		POSTING_READ(FW_BLC_SELF_VLV);
>   		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);
> +		POSTING_READ(FW_BLC_SELF);
>   	} else if (IS_PINEVIEW(dev)) {
>   		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
>   		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
>   		I915_WRITE(DSPFW3, val);
> +		POSTING_READ(DSPFW3);
>   	} else if (IS_I945G(dev) || IS_I945GM(dev)) {
>   		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
>   			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
>   		I915_WRITE(FW_BLC_SELF, val);
> +		POSTING_READ(FW_BLC_SELF);
>   	} else if (IS_I915GM(dev)) {
>   		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
>   			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
>   		I915_WRITE(INSTPM, val);
> +		POSTING_READ(INSTPM);
>   	} else {
>   		return;
>   	}
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH 02/10] drm/i915: Split atomic wm update to pre and post variants

[ville.syrjala]

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

Try to update the watermarks on the right side of the plane update. This
is just a temporary hack until we get the proper two part update into
place. However in the meantime this might have some chance of at least
working.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 29c584c..1a1c686 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4713,6 +4713,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
+	if (crtc->atomic.update_wm_post)
+		intel_update_watermarks(&crtc->base);
+
 	if (atomic->update_fbc) {
 		mutex_lock(&dev->struct_mutex);
 		intel_fbc_update(dev);
@@ -11641,8 +11644,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (intel_wm_need_update(plane, plane_state))
-		intel_crtc->atomic.update_wm = true;
+	if (turn_on)
+		intel_crtc->atomic.update_wm_pre = true;
+	else if (turn_off)
+		intel_crtc->atomic.update_wm_post = true;
+	else if (intel_wm_need_update(plane, plane_state))
+		intel_crtc->atomic.update_wm_pre = true;
 
 	if (visible)
 		intel_crtc->atomic.fb_bits |=
@@ -11800,7 +11807,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		intel_crtc_check_initial_planes(crtc, crtc_state);
 
 	if (mode_changed)
-		intel_crtc->atomic.update_wm = !crtc_state->active;
+		intel_crtc->atomic.update_wm_post = !crtc_state->active;
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
@@ -13729,7 +13736,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (!needs_modeset(crtc->state))
 		intel_pre_plane_update(intel_crtc);
 
-	if (intel_crtc->atomic.update_wm)
+	if (intel_crtc->atomic.update_wm_pre)
 		intel_update_watermarks(crtc);
 
 	intel_runtime_pm_get(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index de2cc26..fefaf01 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -499,7 +499,7 @@ struct intel_crtc_atomic_commit {
 	bool wait_for_flips;
 	bool disable_fbc;
 	bool pre_disable_primary;
-	bool update_wm;
+	bool update_wm_pre, update_wm_post;
 	unsigned disabled_planes;
 
 	/* Sleepable operations to perform after commit */
-- 
2.3.6

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

Re: [PATCH 02/10] drm/i915: Split atomic wm update to pre and post variants

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Try to update the watermarks on the right side of the plane update. This
> is just a temporary hack until we get the proper two part update into
> place. However in the meantime this might have some chance of at least
> working.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>   drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>   2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 29c584c..1a1c686 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4713,6 +4713,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>
>   	intel_frontbuffer_flip(dev, atomic->fb_bits);
>
> +	if (crtc->atomic.update_wm_post)
> +		intel_update_watermarks(&crtc->base);
> +
>   	if (atomic->update_fbc) {
>   		mutex_lock(&dev->struct_mutex);
>   		intel_fbc_update(dev);
> @@ -11641,8 +11644,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>   			 plane->base.id, was_visible, visible,
>   			 turn_off, turn_on, mode_changed);
>
> -	if (intel_wm_need_update(plane, plane_state))
> -		intel_crtc->atomic.update_wm = true;
> +	if (turn_on)
> +		intel_crtc->atomic.update_wm_pre = true;
> +	else if (turn_off)
> +		intel_crtc->atomic.update_wm_post = true;
> +	else if (intel_wm_need_update(plane, plane_state))
> +		intel_crtc->atomic.update_wm_pre = true;
>
>   	if (visible)
>   		intel_crtc->atomic.fb_bits |=
> @@ -11800,7 +11807,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>   		intel_crtc_check_initial_planes(crtc, crtc_state);
>
>   	if (mode_changed)
> -		intel_crtc->atomic.update_wm = !crtc_state->active;
> +		intel_crtc->atomic.update_wm_post = !crtc_state->active;
>
>   	if (mode_changed && crtc_state->enable &&
>   	    dev_priv->display.crtc_compute_clock &&
> @@ -13729,7 +13736,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>   	if (!needs_modeset(crtc->state))
>   		intel_pre_plane_update(intel_crtc);
>
> -	if (intel_crtc->atomic.update_wm)
> +	if (intel_crtc->atomic.update_wm_pre)
>   		intel_update_watermarks(crtc);
>
>   	intel_runtime_pm_get(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index de2cc26..fefaf01 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -499,7 +499,7 @@ struct intel_crtc_atomic_commit {
>   	bool wait_for_flips;
>   	bool disable_fbc;
>   	bool pre_disable_primary;
> -	bool update_wm;
> +	bool update_wm_pre, update_wm_post;
>   	unsigned disabled_planes;
>
>   	/* Sleepable operations to perform after commit */
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/10] drm/i915: Read wm values from hardware at init on CHV

[ville.syrjala]

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

Read out the current watermark settings from the hardware at driver init
time. This will allow us to compare the newly calculated values against
the currrent ones and potentially avoid needless WM updates.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   2 +
 drivers/gpu/drm/i915/intel_display.c |   4 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 141 +++++++++++++++++++++++++++++++++++
 4 files changed, 148 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c3b9fcf..514adcf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1515,6 +1515,8 @@ struct vlv_wm_values {
 		uint8_t sprite[2];
 		uint8_t primary;
 	} ddl[3];
+	uint8_t level;
+	bool cxsr;
 };
 
 struct skl_ddb_entry {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1a1c686..b15d57f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15474,7 +15474,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		pll->on = false;
 	}
 
-	if (IS_GEN9(dev))
+	if (IS_CHERRYVIEW(dev))
+		vlv_wm_get_hw_state(dev);
+	else if (IS_GEN9(dev))
 		skl_wm_get_hw_state(dev);
 	else if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fefaf01..3673a71 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -582,6 +582,7 @@ struct intel_plane_wm_parameters {
 	bool scaled;
 	u64 tiling;
 	unsigned int rotation;
+	uint16_t fifo_size;
 };
 
 struct intel_plane {
@@ -1380,6 +1381,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
 		    unsigned long submitted);
 void intel_queue_rps_boost_for_request(struct drm_device *dev,
 				       struct drm_i915_gem_request *req);
+void vlv_wm_get_hw_state(struct drm_device *dev);
 void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9706275..e67548d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1006,6 +1006,14 @@ static int vlv_compute_wm(struct intel_crtc *crtc,
 	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
 }
 
+enum vlv_wm_level {
+	VLV_WM_LEVEL_PM2,
+	VLV_WM_LEVEL_PM5,
+	VLV_WM_LEVEL_DDR_DVFS,
+	CHV_WM_NUM_LEVELS,
+	VLV_WM_NUM_LEVELS = 1,
+};
+
 static bool vlv_compute_sr_wm(struct drm_device *dev,
 			      struct vlv_wm_values *wm)
 {
@@ -3689,6 +3697,139 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	}
 }
 
+#define _FW_WM(value, plane) \
+	(((value) & DSPFW_ ## plane ## _MASK) >> DSPFW_ ## plane ## _SHIFT)
+#define _FW_WM_VLV(value, plane) \
+	(((value) & DSPFW_ ## plane ## _MASK_VLV) >> DSPFW_ ## plane ## _SHIFT)
+
+static void vlv_read_wm_values(struct drm_i915_private *dev_priv,
+			       struct vlv_wm_values *wm)
+{
+	enum pipe pipe;
+	uint32_t tmp;
+
+	for_each_pipe(dev_priv, pipe) {
+		tmp = I915_READ(VLV_DDL(pipe));
+
+		wm->ddl[pipe].primary =
+			(tmp >> DDL_PLANE_SHIFT) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
+		wm->ddl[pipe].cursor =
+			(tmp >> DDL_CURSOR_SHIFT) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
+		wm->ddl[pipe].sprite[0] =
+			(tmp >> DDL_SPRITE_SHIFT(0)) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
+		wm->ddl[pipe].sprite[1] =
+			(tmp >> DDL_SPRITE_SHIFT(1)) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
+	}
+
+	tmp = I915_READ(DSPFW1);
+	wm->sr.plane = _FW_WM(tmp, SR);
+	wm->pipe[PIPE_B].cursor = _FW_WM(tmp, CURSORB);
+	wm->pipe[PIPE_B].primary = _FW_WM_VLV(tmp, PLANEB);
+	wm->pipe[PIPE_A].primary = _FW_WM_VLV(tmp, PLANEA);
+
+	tmp = I915_READ(DSPFW2);
+	wm->pipe[PIPE_A].sprite[1] = _FW_WM_VLV(tmp, SPRITEB);
+	wm->pipe[PIPE_A].cursor = _FW_WM(tmp, CURSORA);
+	wm->pipe[PIPE_A].sprite[0] = _FW_WM_VLV(tmp, SPRITEA);
+
+	tmp = I915_READ(DSPFW3);
+	wm->sr.cursor = _FW_WM(tmp, CURSOR_SR);
+
+	if (IS_CHERRYVIEW(dev_priv)) {
+		tmp = I915_READ(DSPFW7_CHV);
+		wm->pipe[PIPE_B].sprite[1] = _FW_WM_VLV(tmp, SPRITED);
+		wm->pipe[PIPE_B].sprite[0] = _FW_WM_VLV(tmp, SPRITEC);
+
+		tmp = I915_READ(DSPFW8_CHV);
+		wm->pipe[PIPE_C].sprite[1] = _FW_WM_VLV(tmp, SPRITEF);
+		wm->pipe[PIPE_C].sprite[0] = _FW_WM_VLV(tmp, SPRITEE);
+
+		tmp = I915_READ(DSPFW9_CHV);
+		wm->pipe[PIPE_C].primary = _FW_WM_VLV(tmp, PLANEC);
+		wm->pipe[PIPE_C].cursor = _FW_WM(tmp, CURSORC);
+
+		tmp = I915_READ(DSPHOWM);
+		wm->sr.plane |= _FW_WM(tmp, SR_HI) << 9;
+		wm->pipe[PIPE_C].sprite[1] |= _FW_WM(tmp, SPRITEF_HI) << 8;
+		wm->pipe[PIPE_C].sprite[0] |= _FW_WM(tmp, SPRITEE_HI) << 8;
+		wm->pipe[PIPE_C].primary |= _FW_WM(tmp, PLANEC_HI) << 8;
+		wm->pipe[PIPE_B].sprite[1] |= _FW_WM(tmp, SPRITED_HI) << 8;
+		wm->pipe[PIPE_B].sprite[0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
+		wm->pipe[PIPE_B].primary |= _FW_WM(tmp, PLANEB_HI) << 8;
+		wm->pipe[PIPE_A].sprite[1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
+		wm->pipe[PIPE_A].sprite[0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
+		wm->pipe[PIPE_A].primary |= _FW_WM(tmp, PLANEA_HI) << 8;
+	} else {
+		tmp = I915_READ(DSPFW7);
+		wm->pipe[PIPE_B].sprite[1] = _FW_WM_VLV(tmp, SPRITED);
+		wm->pipe[PIPE_B].sprite[0] = _FW_WM_VLV(tmp, SPRITEC);
+
+		tmp = I915_READ(DSPHOWM);
+		wm->sr.plane |= _FW_WM(tmp, SR_HI) << 9;
+		wm->pipe[PIPE_B].sprite[1] |= _FW_WM(tmp, SPRITED_HI) << 8;
+		wm->pipe[PIPE_B].sprite[0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
+		wm->pipe[PIPE_B].primary |= _FW_WM(tmp, PLANEB_HI) << 8;
+		wm->pipe[PIPE_A].sprite[1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
+		wm->pipe[PIPE_A].sprite[0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
+		wm->pipe[PIPE_A].primary |= _FW_WM(tmp, PLANEA_HI) << 8;
+	}
+}
+
+#undef _FW_WM
+#undef _FW_WM_VLV
+
+void vlv_wm_get_hw_state(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct vlv_wm_values *wm = &dev_priv->wm.vlv;
+	struct intel_plane *plane;
+	enum pipe pipe;
+	u32 val;
+
+	vlv_read_wm_values(dev_priv, wm);
+
+	for_each_intel_plane(dev, plane) {
+		switch (plane->base.type) {
+			int sprite;
+		case DRM_PLANE_TYPE_CURSOR:
+			plane->wm.fifo_size = 63;
+			break;
+		case DRM_PLANE_TYPE_PRIMARY:
+			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			sprite = plane->plane;
+			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
+			break;
+		}
+	}
+
+	wm->cxsr = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
+	wm->level = VLV_WM_LEVEL_PM2;
+
+	if (IS_CHERRYVIEW(dev_priv)) {
+		mutex_lock(&dev_priv->rps.hw_lock);
+
+		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
+		if (val & DSP_MAXFIFO_PM5_ENABLE)
+			wm->level = VLV_WM_LEVEL_PM5;
+
+		val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
+		if ((val & FORCE_DDR_HIGH_FREQ) == 0)
+			wm->level = VLV_WM_LEVEL_DDR_DVFS;
+
+		mutex_unlock(&dev_priv->rps.hw_lock);
+	}
+
+	for_each_pipe(dev_priv, pipe)
+		DRM_DEBUG_KMS("Initial watermarks: pipe %c, plane=%d, cursor=%d, sprite0=%d, sprite1=%d\n",
+			      pipe_name(pipe), wm->pipe[pipe].primary, wm->pipe[pipe].cursor,
+			      wm->pipe[pipe].sprite[0], wm->pipe[pipe].sprite[1]);
+
+	DRM_DEBUG_KMS("Initial watermarks: SR plane=%d, SR cursor=%d level=%d cxsr=%d\n",
+		      wm->sr.plane, wm->sr.cursor, wm->level, wm->cxsr);
+}
+
 void ilk_wm_get_hw_state(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
2.3.6

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

Re: [PATCH 03/10] drm/i915: Read wm values from hardware at init on CHV

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Read out the current watermark settings from the hardware at driver init
> time. This will allow us to compare the newly calculated values against
> the currrent ones and potentially avoid needless WM updates.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |   2 +
>   drivers/gpu/drm/i915/intel_display.c |   4 +-
>   drivers/gpu/drm/i915/intel_drv.h     |   2 +
>   drivers/gpu/drm/i915/intel_pm.c      | 141 +++++++++++++++++++++++++++++++++++
>   4 files changed, 148 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c3b9fcf..514adcf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1515,6 +1515,8 @@ struct vlv_wm_values {
>   		uint8_t sprite[2];
>   		uint8_t primary;
>   	} ddl[3];
> +	uint8_t level;
> +	bool cxsr;
>   };
>
>   struct skl_ddb_entry {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1a1c686..b15d57f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15474,7 +15474,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>   		pll->on = false;
>   	}
>
> -	if (IS_GEN9(dev))
> +	if (IS_CHERRYVIEW(dev))
> +		vlv_wm_get_hw_state(dev);
> +	else if (IS_GEN9(dev))
>   		skl_wm_get_hw_state(dev);
>   	else if (HAS_PCH_SPLIT(dev))
>   		ilk_wm_get_hw_state(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fefaf01..3673a71 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -582,6 +582,7 @@ struct intel_plane_wm_parameters {
>   	bool scaled;
>   	u64 tiling;
>   	unsigned int rotation;
> +	uint16_t fifo_size;
>   };
>
>   struct intel_plane {
> @@ -1380,6 +1381,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>   		    unsigned long submitted);
>   void intel_queue_rps_boost_for_request(struct drm_device *dev,
>   				       struct drm_i915_gem_request *req);
> +void vlv_wm_get_hw_state(struct drm_device *dev);
>   void ilk_wm_get_hw_state(struct drm_device *dev);
>   void skl_wm_get_hw_state(struct drm_device *dev);
>   void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9706275..e67548d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1006,6 +1006,14 @@ static int vlv_compute_wm(struct intel_crtc *crtc,
>   	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
>   }
>
> +enum vlv_wm_level {
> +	VLV_WM_LEVEL_PM2,
> +	VLV_WM_LEVEL_PM5,
> +	VLV_WM_LEVEL_DDR_DVFS,
> +	CHV_WM_NUM_LEVELS,
> +	VLV_WM_NUM_LEVELS = 1,
> +};
> +
>   static bool vlv_compute_sr_wm(struct drm_device *dev,
>   			      struct vlv_wm_values *wm)
>   {
> @@ -3689,6 +3697,139 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>   	}
>   }
>
> +#define _FW_WM(value, plane) \
> +	(((value) & DSPFW_ ## plane ## _MASK) >> DSPFW_ ## plane ## _SHIFT)
> +#define _FW_WM_VLV(value, plane) \
> +	(((value) & DSPFW_ ## plane ## _MASK_VLV) >> DSPFW_ ## plane ## _SHIFT)
> +
> +static void vlv_read_wm_values(struct drm_i915_private *dev_priv,
> +			       struct vlv_wm_values *wm)
> +{
> +	enum pipe pipe;
> +	uint32_t tmp;
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		tmp = I915_READ(VLV_DDL(pipe));
> +
> +		wm->ddl[pipe].primary =
> +			(tmp >> DDL_PLANE_SHIFT) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
> +		wm->ddl[pipe].cursor =
> +			(tmp >> DDL_CURSOR_SHIFT) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
> +		wm->ddl[pipe].sprite[0] =
> +			(tmp >> DDL_SPRITE_SHIFT(0)) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
> +		wm->ddl[pipe].sprite[1] =
> +			(tmp >> DDL_SPRITE_SHIFT(1)) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
> +	}
> +
> +	tmp = I915_READ(DSPFW1);
> +	wm->sr.plane = _FW_WM(tmp, SR);
> +	wm->pipe[PIPE_B].cursor = _FW_WM(tmp, CURSORB);
> +	wm->pipe[PIPE_B].primary = _FW_WM_VLV(tmp, PLANEB);
> +	wm->pipe[PIPE_A].primary = _FW_WM_VLV(tmp, PLANEA);
> +
> +	tmp = I915_READ(DSPFW2);
> +	wm->pipe[PIPE_A].sprite[1] = _FW_WM_VLV(tmp, SPRITEB);
> +	wm->pipe[PIPE_A].cursor = _FW_WM(tmp, CURSORA);
> +	wm->pipe[PIPE_A].sprite[0] = _FW_WM_VLV(tmp, SPRITEA);
> +
> +	tmp = I915_READ(DSPFW3);
> +	wm->sr.cursor = _FW_WM(tmp, CURSOR_SR);
> +
> +	if (IS_CHERRYVIEW(dev_priv)) {
> +		tmp = I915_READ(DSPFW7_CHV);
> +		wm->pipe[PIPE_B].sprite[1] = _FW_WM_VLV(tmp, SPRITED);
> +		wm->pipe[PIPE_B].sprite[0] = _FW_WM_VLV(tmp, SPRITEC);
> +
> +		tmp = I915_READ(DSPFW8_CHV);
> +		wm->pipe[PIPE_C].sprite[1] = _FW_WM_VLV(tmp, SPRITEF);
> +		wm->pipe[PIPE_C].sprite[0] = _FW_WM_VLV(tmp, SPRITEE);
> +
> +		tmp = I915_READ(DSPFW9_CHV);
> +		wm->pipe[PIPE_C].primary = _FW_WM_VLV(tmp, PLANEC);
> +		wm->pipe[PIPE_C].cursor = _FW_WM(tmp, CURSORC);
> +
> +		tmp = I915_READ(DSPHOWM);
> +		wm->sr.plane |= _FW_WM(tmp, SR_HI) << 9;
> +		wm->pipe[PIPE_C].sprite[1] |= _FW_WM(tmp, SPRITEF_HI) << 8;
> +		wm->pipe[PIPE_C].sprite[0] |= _FW_WM(tmp, SPRITEE_HI) << 8;
> +		wm->pipe[PIPE_C].primary |= _FW_WM(tmp, PLANEC_HI) << 8;
> +		wm->pipe[PIPE_B].sprite[1] |= _FW_WM(tmp, SPRITED_HI) << 8;
> +		wm->pipe[PIPE_B].sprite[0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
> +		wm->pipe[PIPE_B].primary |= _FW_WM(tmp, PLANEB_HI) << 8;
> +		wm->pipe[PIPE_A].sprite[1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
> +		wm->pipe[PIPE_A].sprite[0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
> +		wm->pipe[PIPE_A].primary |= _FW_WM(tmp, PLANEA_HI) << 8;
> +	} else {
> +		tmp = I915_READ(DSPFW7);
> +		wm->pipe[PIPE_B].sprite[1] = _FW_WM_VLV(tmp, SPRITED);
> +		wm->pipe[PIPE_B].sprite[0] = _FW_WM_VLV(tmp, SPRITEC);
> +
> +		tmp = I915_READ(DSPHOWM);
> +		wm->sr.plane |= _FW_WM(tmp, SR_HI) << 9;
> +		wm->pipe[PIPE_B].sprite[1] |= _FW_WM(tmp, SPRITED_HI) << 8;
> +		wm->pipe[PIPE_B].sprite[0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
> +		wm->pipe[PIPE_B].primary |= _FW_WM(tmp, PLANEB_HI) << 8;
> +		wm->pipe[PIPE_A].sprite[1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
> +		wm->pipe[PIPE_A].sprite[0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
> +		wm->pipe[PIPE_A].primary |= _FW_WM(tmp, PLANEA_HI) << 8;
> +	}
> +}
> +
> +#undef _FW_WM
> +#undef _FW_WM_VLV
> +
> +void vlv_wm_get_hw_state(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct vlv_wm_values *wm = &dev_priv->wm.vlv;
> +	struct intel_plane *plane;
> +	enum pipe pipe;
> +	u32 val;
> +
> +	vlv_read_wm_values(dev_priv, wm);
> +
> +	for_each_intel_plane(dev, plane) {
> +		switch (plane->base.type) {
> +			int sprite;
> +		case DRM_PLANE_TYPE_CURSOR:
> +			plane->wm.fifo_size = 63;
> +			break;
> +		case DRM_PLANE_TYPE_PRIMARY:
> +			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
> +			break;
> +		case DRM_PLANE_TYPE_OVERLAY:
> +			sprite = plane->plane;
> +			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
> +			break;
> +		}
> +	}
> +
> +	wm->cxsr = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
> +	wm->level = VLV_WM_LEVEL_PM2;
> +
> +	if (IS_CHERRYVIEW(dev_priv)) {
> +		mutex_lock(&dev_priv->rps.hw_lock);
> +
> +		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> +		if (val & DSP_MAXFIFO_PM5_ENABLE)
> +			wm->level = VLV_WM_LEVEL_PM5;
> +
> +		val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> +		if ((val & FORCE_DDR_HIGH_FREQ) == 0)
> +			wm->level = VLV_WM_LEVEL_DDR_DVFS;
> +
> +		mutex_unlock(&dev_priv->rps.hw_lock);
> +	}
> +
> +	for_each_pipe(dev_priv, pipe)
> +		DRM_DEBUG_KMS("Initial watermarks: pipe %c, plane=%d, cursor=%d, sprite0=%d, sprite1=%d\n",
> +			      pipe_name(pipe), wm->pipe[pipe].primary, wm->pipe[pipe].cursor,
> +			      wm->pipe[pipe].sprite[0], wm->pipe[pipe].sprite[1]);
> +
> +	DRM_DEBUG_KMS("Initial watermarks: SR plane=%d, SR cursor=%d level=%d cxsr=%d\n",
> +		      wm->sr.plane, wm->sr.cursor, wm->level, wm->cxsr);
> +}
> +
>   void ilk_wm_get_hw_state(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH 04/10] drm/i915: CHV DDR DVFS support and another watermark rewrite

[ville.syrjala]

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

Turns out the VLV/CHV system agent doesn't understand memory
latencies, so trying to rely on the PND deadline mechanism is not
going to fly especially when DDR DVFS is enabled. Currently we try to
avoid the problems by lying to the system agent about the deadlines
and setting the FIFO watermarks to 8 cachelines. This however leads to
bad memory self refresh residency.

So in order to satosfy everyone we'll just give up on the deadline
scheme and program the watermarks old school based on the worst case
memory latency.

I've modelled this a bit on the ILK+ approach where we compute multiple
sets of watermarks for each pipe (PM2,PM5,DDR DVFS) and when merge thet
appropriate one later with the watermarks from other pipes. There isn't
too much to merge actually since each pipe has a totally independent
FIFO (well apart from the mess with the partially shared DSPARB
registers), but still decopuling the pipes from each other seems like a
good idea.

Eventually we'll want to perform the watermark update in two phases
around the plane update to avoid underruns due to the single buffered
watermark registers. But that's still in limbo for ILK+ too, so I've not
gone that far yet for VLV/CHV either.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  28 +--
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_drv.h     |  11 ++
 drivers/gpu/drm/i915/intel_pm.c      | 318 ++++++++++++++++++++++++++++++++++-
 4 files changed, 345 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 514adcf..37cc653 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -276,6 +276,12 @@ struct i915_hotplug {
 			    &dev->mode_config.plane_list,	\
 			    base.head)
 
+#define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)	\
+	list_for_each_entry(intel_plane,				\
+			    &(dev)->mode_config.plane_list,		\
+			    base.head)					\
+		if ((intel_plane)->pipe == (intel_crtc)->pipe)
+
 #define for_each_intel_crtc(dev, intel_crtc) \
 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
 
@@ -1498,18 +1504,20 @@ struct ilk_wm_values {
 	enum intel_ddb_partitioning partitioning;
 };
 
-struct vlv_wm_values {
-	struct {
-		uint16_t primary;
-		uint16_t sprite[2];
-		uint8_t cursor;
-	} pipe[3];
+struct vlv_pipe_wm {
+	uint16_t primary;
+	uint16_t sprite[2];
+	uint8_t cursor;
+};
 
-	struct {
-		uint16_t plane;
-		uint8_t cursor;
-	} sr;
+struct vlv_sr_wm {
+	uint16_t plane;
+	uint8_t cursor;
+};
 
+struct vlv_wm_values {
+	struct vlv_pipe_wm pipe[3];
+	struct vlv_sr_wm sr;
 	struct {
 		uint8_t cursor;
 		uint8_t sprite[2];
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b15d57f..1424320 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4690,8 +4690,11 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	 * event which is after the vblank start event, so we need to have a
 	 * wait-for-vblank between disabling the plane and the pipe.
 	 */
-	if (HAS_GMCH_DISPLAY(dev))
+	if (HAS_GMCH_DISPLAY(dev)) {
 		intel_set_memory_cxsr(dev_priv, false);
+		dev_priv->wm.vlv.cxsr = false;
+		intel_wait_for_vblank(dev, pipe);
+	}
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is
@@ -6005,7 +6008,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc_load_lut(crtc);
 
-	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
 
 	assert_vblank_disabled(crtc);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3673a71..f26a680 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -462,6 +462,15 @@ struct intel_crtc_state {
 	enum pipe hsw_workaround_pipe;
 };
 
+struct vlv_wm_state {
+	struct vlv_pipe_wm wm[3];
+	struct vlv_sr_wm sr[3];
+	uint8_t num_active_planes;
+	uint8_t num_levels;
+	uint8_t level;
+	bool cxsr;
+};
+
 struct intel_pipe_wm {
 	struct intel_wm_level wm[5];
 	uint32_t linetime;
@@ -564,6 +573,8 @@ struct intel_crtc {
 
 	/* scalers available on this crtc */
 	int num_scalers;
+
+	struct vlv_wm_state wm_state;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e67548d..d046e5f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -335,8 +335,6 @@ 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);
 		POSTING_READ(FW_BLC_SELF_VLV);
-		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);
 		POSTING_READ(FW_BLC_SELF);
@@ -929,8 +927,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 	}
 
 	POSTING_READ(DSPFW1);
-
-	dev_priv->wm.vlv = *wm;
 }
 
 #undef FW_WM_VLV
@@ -1014,6 +1010,72 @@ enum vlv_wm_level {
 	VLV_WM_NUM_LEVELS = 1,
 };
 
+/* latency must be in 0.1us units. */
+static unsigned int vlv_wm_method2(unsigned int pixel_rate,
+				   unsigned int pipe_htotal,
+				   unsigned int horiz_pixels,
+				   unsigned int bytes_per_pixel,
+				   unsigned int latency)
+{
+	unsigned int ret;
+
+	ret = (latency * pixel_rate) / (pipe_htotal * 10000);
+	ret = (ret + 1) * horiz_pixels * bytes_per_pixel;
+	ret = DIV_ROUND_UP(ret, 64);
+
+	return ret;
+}
+
+static void vlv_setup_wm_latency(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* all latencies in usec */
+	dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM2] = 3;
+
+	if (IS_CHERRYVIEW(dev_priv)) {
+		dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM5] = 12;
+		dev_priv->wm.pri_latency[VLV_WM_LEVEL_DDR_DVFS] = 33;
+	}
+}
+
+static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
+				     struct intel_crtc *crtc,
+				     const struct intel_plane_state *state,
+				     int level)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	int clock, htotal, pixel_size, width, wm;
+
+	if (dev_priv->wm.pri_latency[level] == 0)
+		return USHRT_MAX;
+
+	if (!state->visible)
+		return 0;
+
+	pixel_size = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
+	clock = crtc->config->base.adjusted_mode.crtc_clock;
+	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
+	width = crtc->config->pipe_src_w;
+	if (WARN_ON(htotal == 0))
+		htotal = 1;
+
+	if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
+		/*
+		 * FIXME the formula gives values that are
+		 * too big for the cursor FIFO, and hence we
+		 * would never be able to use cursors. For
+		 * now just hardcode the watermark.
+		 */
+		wm = 63;
+	} else {
+		wm = vlv_wm_method2(clock, htotal, width, pixel_size,
+				    dev_priv->wm.pri_latency[level] * 10);
+	}
+
+	return min_t(int, wm, USHRT_MAX);
+}
+
 static bool vlv_compute_sr_wm(struct drm_device *dev,
 			      struct vlv_wm_values *wm)
 {
@@ -1105,6 +1167,249 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
 
 	if (cxsr_enabled)
 		intel_set_memory_cxsr(dev_priv, true);
+
+	dev_priv->wm.vlv = wm;
+}
+
+static void vlv_invert_wms(struct intel_crtc *crtc)
+{
+	struct vlv_wm_state *wm_state = &crtc->wm_state;
+	int level;
+
+	for (level = 0; level < wm_state->num_levels; level++) {
+		struct drm_device *dev = crtc->base.dev;
+		const int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
+		struct intel_plane *plane;
+
+		wm_state->sr[level].plane = sr_fifo_size - wm_state->sr[level].plane;
+		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
+
+		for_each_intel_plane_on_crtc(dev, crtc, plane) {
+			switch (plane->base.type) {
+				int sprite;
+			case DRM_PLANE_TYPE_CURSOR:
+				wm_state->wm[level].cursor = plane->wm.fifo_size -
+					wm_state->wm[level].cursor;
+				break;
+			case DRM_PLANE_TYPE_PRIMARY:
+				wm_state->wm[level].primary = plane->wm.fifo_size -
+					wm_state->wm[level].primary;
+				break;
+			case DRM_PLANE_TYPE_OVERLAY:
+				sprite = plane->plane;
+				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
+					wm_state->wm[level].sprite[sprite];
+				break;
+			}
+		}
+	}
+}
+
+static void _vlv_compute_wm(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct vlv_wm_state *wm_state = &crtc->wm_state;
+	struct intel_plane *plane;
+	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
+	int level;
+
+	memset(wm_state, 0, sizeof(*wm_state));
+
+	wm_state->cxsr = crtc->pipe != PIPE_C;
+	if (IS_CHERRYVIEW(dev))
+		wm_state->num_levels = CHV_WM_NUM_LEVELS;
+	else
+		wm_state->num_levels = VLV_WM_NUM_LEVELS;
+
+	wm_state->num_active_planes = 0;
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		struct intel_plane_state *state =
+			to_intel_plane_state(plane->base.state);
+
+		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			continue;
+
+		if (state->visible)
+			wm_state->num_active_planes++;
+	}
+
+	if (wm_state->num_active_planes != 1)
+		wm_state->cxsr = false;
+
+	if (wm_state->cxsr) {
+		for (level = 0; level < wm_state->num_levels; level++) {
+			wm_state->sr[level].plane = sr_fifo_size;
+			wm_state->sr[level].cursor = 63;
+		}
+	}
+
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		struct intel_plane_state *state =
+			to_intel_plane_state(plane->base.state);
+
+		if (!state->visible)
+			continue;
+
+		/* normal watermarks */
+		for (level = 0; level < wm_state->num_levels; level++) {
+			int wm = vlv_compute_wm_level(plane, crtc, state, level);
+			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
+
+			/* hack */
+			if (WARN_ON(level == 0 && wm > max_wm))
+				wm = max_wm;
+
+			if (wm > plane->wm.fifo_size)
+				break;
+
+			switch (plane->base.type) {
+				int sprite;
+			case DRM_PLANE_TYPE_CURSOR:
+				wm_state->wm[level].cursor = wm;
+				break;
+			case DRM_PLANE_TYPE_PRIMARY:
+				wm_state->wm[level].primary = wm;
+				break;
+			case DRM_PLANE_TYPE_OVERLAY:
+				sprite = plane->plane;
+				wm_state->wm[level].sprite[sprite] = wm;
+				break;
+			}
+		}
+
+		wm_state->num_levels = level;
+
+		if (!wm_state->cxsr)
+			continue;
+
+		/* maxfifo watermarks */
+		switch (plane->base.type) {
+			int sprite, level;
+		case DRM_PLANE_TYPE_CURSOR:
+			for (level = 0; level < wm_state->num_levels; level++)
+				wm_state->sr[level].cursor =
+					wm_state->sr[level].cursor;
+			break;
+		case DRM_PLANE_TYPE_PRIMARY:
+			for (level = 0; level < wm_state->num_levels; level++)
+				wm_state->sr[level].plane =
+					min(wm_state->sr[level].plane,
+					    wm_state->wm[level].primary);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			sprite = plane->plane;
+			for (level = 0; level < wm_state->num_levels; level++)
+				wm_state->sr[level].plane =
+					min(wm_state->sr[level].plane,
+					    wm_state->wm[level].sprite[sprite]);
+			break;
+		}
+	}
+
+	/* clear any (partially) filled invalid levels */
+	for (level = wm_state->num_levels; level < CHV_WM_NUM_LEVELS; level++) {
+		memset(&wm_state->wm[level], 0, sizeof(wm_state->wm[level]));
+		memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
+	}
+
+	vlv_invert_wms(crtc);
+}
+
+static void vlv_merge_wm(struct drm_device *dev,
+			 struct vlv_wm_values *wm)
+{
+	struct intel_crtc *crtc;
+	int num_active_crtcs = 0;
+
+	if (IS_CHERRYVIEW(dev))
+		wm->level = VLV_WM_LEVEL_DDR_DVFS;
+	else
+		wm->level = VLV_WM_LEVEL_PM2;
+	wm->cxsr = true;
+
+	for_each_intel_crtc(dev, crtc) {
+		const struct vlv_wm_state *wm_state = &crtc->wm_state;
+
+		if (!crtc->active)
+			continue;
+
+		if (!wm_state->cxsr)
+			wm->cxsr = false;
+
+		num_active_crtcs++;
+		wm->level = min_t(int, wm->level, wm_state->num_levels - 1);
+	}
+
+	if (num_active_crtcs != 1)
+		wm->cxsr = false;
+
+	for_each_intel_crtc(dev, crtc) {
+		struct vlv_wm_state *wm_state = &crtc->wm_state;
+		enum pipe pipe = crtc->pipe;
+
+		if (!crtc->active)
+			continue;
+
+		wm->pipe[pipe] = wm_state->wm[wm->level];
+		if (wm->cxsr)
+			wm->sr = wm_state->sr[wm->level];
+
+		wm->ddl[pipe].primary = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].sprite[0] = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].sprite[1] = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].cursor = DDL_PRECISION_HIGH | 2;
+	}
+}
+
+static void vlv_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;
+	struct vlv_wm_values wm = {};
+
+	_vlv_compute_wm(intel_crtc);
+	vlv_merge_wm(dev, &wm);
+
+	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0)
+		return;
+
+	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
+	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
+		chv_set_memory_dvfs(dev_priv, false);
+
+	if (wm.level < VLV_WM_LEVEL_PM5 &&
+	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
+		chv_set_memory_pm5(dev_priv, false);
+
+	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
+		intel_set_memory_cxsr(dev_priv, false);
+		intel_wait_for_vblank(dev, pipe);
+	}
+
+	vlv_write_wm_values(intel_crtc, &wm);
+
+	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
+		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
+		      pipe_name(pipe), wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
+		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
+		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
+
+	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
+		intel_wait_for_vblank(dev, pipe);
+		intel_set_memory_cxsr(dev_priv, true);
+	}
+
+	if (wm.level >= VLV_WM_LEVEL_PM5 &&
+	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
+		chv_set_memory_pm5(dev_priv, true);
+
+	if (wm.level >= VLV_WM_LEVEL_DDR_DVFS &&
+	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_DDR_DVFS)
+		chv_set_memory_dvfs(dev_priv, true);
+
+	dev_priv->wm.vlv = wm;
 }
 
 static void valleyview_update_sprite_wm(struct drm_plane *plane,
@@ -6823,8 +7128,9 @@ 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 = valleyview_update_wm;
-		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
+		vlv_setup_wm_latency(dev);
+
+		dev_priv->display.update_wm = vlv_update_wm;
 		dev_priv->display.init_clock_gating =
 			cherryview_init_clock_gating;
 	} else if (IS_VALLEYVIEW(dev)) {
-- 
2.3.6

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

Re: [PATCH 04/10] drm/i915: CHV DDR DVFS support and another watermark rewrite

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turns out the VLV/CHV system agent doesn't understand memory
> latencies, so trying to rely on the PND deadline mechanism is not
> going to fly especially when DDR DVFS is enabled. Currently we try to
> avoid the problems by lying to the system agent about the deadlines
> and setting the FIFO watermarks to 8 cachelines. This however leads to
> bad memory self refresh residency.
>
> So in order to satosfy everyone we'll just give up on the deadline
> scheme and program the watermarks old school based on the worst case
> memory latency.
>
> I've modelled this a bit on the ILK+ approach where we compute multiple
> sets of watermarks for each pipe (PM2,PM5,DDR DVFS) and when merge thet
> appropriate one later with the watermarks from other pipes. There isn't
> too much to merge actually since each pipe has a totally independent
> FIFO (well apart from the mess with the partially shared DSPARB
> registers), but still decopuling the pipes from each other seems like a
> good idea.
>
> Eventually we'll want to perform the watermark update in two phases
> around the plane update to avoid underruns due to the single buffered
> watermark registers. But that's still in limbo for ILK+ too, so I've not
> gone that far yet for VLV/CHV either.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  28 +--
>   drivers/gpu/drm/i915/intel_display.c |   6 +-
>   drivers/gpu/drm/i915/intel_drv.h     |  11 ++
>   drivers/gpu/drm/i915/intel_pm.c      | 318 ++++++++++++++++++++++++++++++++++-
>   4 files changed, 345 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 514adcf..37cc653 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -276,6 +276,12 @@ struct i915_hotplug {
>   			    &dev->mode_config.plane_list,	\
>   			    base.head)
>
> +#define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)	\
> +	list_for_each_entry(intel_plane,				\
> +			    &(dev)->mode_config.plane_list,		\
> +			    base.head)					\
> +		if ((intel_plane)->pipe == (intel_crtc)->pipe)
> +
>   #define for_each_intel_crtc(dev, intel_crtc) \
>   	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
>
> @@ -1498,18 +1504,20 @@ struct ilk_wm_values {
>   	enum intel_ddb_partitioning partitioning;
>   };
>
> -struct vlv_wm_values {
> -	struct {
> -		uint16_t primary;
> -		uint16_t sprite[2];
> -		uint8_t cursor;
> -	} pipe[3];
> +struct vlv_pipe_wm {
> +	uint16_t primary;
> +	uint16_t sprite[2];
> +	uint8_t cursor;
> +};
>
> -	struct {
> -		uint16_t plane;
> -		uint8_t cursor;
> -	} sr;
> +struct vlv_sr_wm {
> +	uint16_t plane;
> +	uint8_t cursor;
> +};
>
> +struct vlv_wm_values {
> +	struct vlv_pipe_wm pipe[3];
> +	struct vlv_sr_wm sr;
>   	struct {
>   		uint8_t cursor;
>   		uint8_t sprite[2];
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b15d57f..1424320 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4690,8 +4690,11 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>   	 * event which is after the vblank start event, so we need to have a
>   	 * wait-for-vblank between disabling the plane and the pipe.
>   	 */
> -	if (HAS_GMCH_DISPLAY(dev))
> +	if (HAS_GMCH_DISPLAY(dev)) {
>   		intel_set_memory_cxsr(dev_priv, false);
> +		dev_priv->wm.vlv.cxsr = false;
> +		intel_wait_for_vblank(dev, pipe);
> +	}
>
>   	/*
>   	 * FIXME IPS should be fine as long as one plane is
> @@ -6005,7 +6008,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
>   	intel_crtc_load_lut(crtc);
>
> -	intel_update_watermarks(crtc);
>   	intel_enable_pipe(intel_crtc);
>
>   	assert_vblank_disabled(crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3673a71..f26a680 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -462,6 +462,15 @@ struct intel_crtc_state {
>   	enum pipe hsw_workaround_pipe;
>   };
>
> +struct vlv_wm_state {
> +	struct vlv_pipe_wm wm[3];
> +	struct vlv_sr_wm sr[3];
> +	uint8_t num_active_planes;
> +	uint8_t num_levels;
> +	uint8_t level;
> +	bool cxsr;
> +};
> +
>   struct intel_pipe_wm {
>   	struct intel_wm_level wm[5];
>   	uint32_t linetime;
> @@ -564,6 +573,8 @@ struct intel_crtc {
>
>   	/* scalers available on this crtc */
>   	int num_scalers;
> +
> +	struct vlv_wm_state wm_state;
>   };
>
>   struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e67548d..d046e5f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -335,8 +335,6 @@ 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);
>   		POSTING_READ(FW_BLC_SELF_VLV);
> -		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);
>   		POSTING_READ(FW_BLC_SELF);
> @@ -929,8 +927,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>   	}
>
>   	POSTING_READ(DSPFW1);
> -
> -	dev_priv->wm.vlv = *wm;
>   }
>
>   #undef FW_WM_VLV
> @@ -1014,6 +1010,72 @@ enum vlv_wm_level {
>   	VLV_WM_NUM_LEVELS = 1,
>   };
>
> +/* latency must be in 0.1us units. */
> +static unsigned int vlv_wm_method2(unsigned int pixel_rate,
> +				   unsigned int pipe_htotal,
> +				   unsigned int horiz_pixels,
> +				   unsigned int bytes_per_pixel,
> +				   unsigned int latency)
> +{
> +	unsigned int ret;
> +
> +	ret = (latency * pixel_rate) / (pipe_htotal * 10000);
> +	ret = (ret + 1) * horiz_pixels * bytes_per_pixel;
> +	ret = DIV_ROUND_UP(ret, 64);
> +
> +	return ret;
> +}
> +
> +static void vlv_setup_wm_latency(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* all latencies in usec */
> +	dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM2] = 3;
> +
> +	if (IS_CHERRYVIEW(dev_priv)) {
> +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM5] = 12;
> +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_DDR_DVFS] = 33;

nit #defines for these magic values please

> +	}
> +}
> +
> +static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> +				     struct intel_crtc *crtc,
> +				     const struct intel_plane_state *state,
> +				     int level)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	int clock, htotal, pixel_size, width, wm;
> +
> +	if (dev_priv->wm.pri_latency[level] == 0)
> +		return USHRT_MAX;
> +
> +	if (!state->visible)
> +		return 0;
> +
> +	pixel_size = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> +	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
> +	width = crtc->config->pipe_src_w;
> +	if (WARN_ON(htotal == 0))
> +		htotal = 1;
> +
> +	if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> +		/*
> +		 * FIXME the formula gives values that are
> +		 * too big for the cursor FIFO, and hence we
> +		 * would never be able to use cursors. For
> +		 * now just hardcode the watermark.
> +		 */
> +		wm = 63;

Hard coding to maximum value of 63. Should probably be programmed to 
worst case instead of maximum.

> +	} else {
> +		wm = vlv_wm_method2(clock, htotal, width, pixel_size,
> +				    dev_priv->wm.pri_latency[level] * 10);
> +	}
> +
> +	return min_t(int, wm, USHRT_MAX);
> +}
> +
>   static bool vlv_compute_sr_wm(struct drm_device *dev,
>   			      struct vlv_wm_values *wm)
>   {
> @@ -1105,6 +1167,249 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>
>   	if (cxsr_enabled)
>   		intel_set_memory_cxsr(dev_priv, true);
> +
> +	dev_priv->wm.vlv = wm;
> +}
> +
> +static void vlv_invert_wms(struct intel_crtc *crtc)
> +{
> +	struct vlv_wm_state *wm_state = &crtc->wm_state;
> +	int level;
> +
> +	for (level = 0; level < wm_state->num_levels; level++) {
> +		struct drm_device *dev = crtc->base.dev;
> +		const int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> +		struct intel_plane *plane;
> +
> +		wm_state->sr[level].plane = sr_fifo_size - wm_state->sr[level].plane;
> +		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
> +
> +		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +			switch (plane->base.type) {
> +				int sprite;
> +			case DRM_PLANE_TYPE_CURSOR:
> +				wm_state->wm[level].cursor = plane->wm.fifo_size -
> +					wm_state->wm[level].cursor;
> +				break;
> +			case DRM_PLANE_TYPE_PRIMARY:
> +				wm_state->wm[level].primary = plane->wm.fifo_size -
> +					wm_state->wm[level].primary;
> +				break;
> +			case DRM_PLANE_TYPE_OVERLAY:
> +				sprite = plane->plane;
> +				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
> +					wm_state->wm[level].sprite[sprite];
> +				break;
> +			}
> +		}
> +	}
> +}
> +
> +static void _vlv_compute_wm(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct vlv_wm_state *wm_state = &crtc->wm_state;
> +	struct intel_plane *plane;
> +	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> +	int level;
> +
> +	memset(wm_state, 0, sizeof(*wm_state));
> +
> +	wm_state->cxsr = crtc->pipe != PIPE_C;
> +	if (IS_CHERRYVIEW(dev))
> +		wm_state->num_levels = CHV_WM_NUM_LEVELS;
> +	else
> +		wm_state->num_levels = VLV_WM_NUM_LEVELS;
> +
> +	wm_state->num_active_planes = 0;
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		struct intel_plane_state *state =
> +			to_intel_plane_state(plane->base.state);
> +
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			continue;
> +
> +		if (state->visible)
> +			wm_state->num_active_planes++;
> +	}
> +
> +	if (wm_state->num_active_planes != 1)
> +		wm_state->cxsr = false;
> +
> +	if (wm_state->cxsr) {
> +		for (level = 0; level < wm_state->num_levels; level++) {
> +			wm_state->sr[level].plane = sr_fifo_size;
> +			wm_state->sr[level].cursor = 63;
> +		}
> +	}
> +
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		struct intel_plane_state *state =
> +			to_intel_plane_state(plane->base.state);
> +
> +		if (!state->visible)
> +			continue;
> +
> +		/* normal watermarks */
> +		for (level = 0; level < wm_state->num_levels; level++) {
> +			int wm = vlv_compute_wm_level(plane, crtc, state, level);
> +			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> +
> +			/* hack */
> +			if (WARN_ON(level == 0 && wm > max_wm))
> +				wm = max_wm;
> +
> +			if (wm > plane->wm.fifo_size)
> +				break;
> +
> +			switch (plane->base.type) {
> +				int sprite;
> +			case DRM_PLANE_TYPE_CURSOR:
> +				wm_state->wm[level].cursor = wm;
> +				break;
> +			case DRM_PLANE_TYPE_PRIMARY:
> +				wm_state->wm[level].primary = wm;
> +				break;
> +			case DRM_PLANE_TYPE_OVERLAY:
> +				sprite = plane->plane;
> +				wm_state->wm[level].sprite[sprite] = wm;
> +				break;
> +			}
> +		}
> +
> +		wm_state->num_levels = level;
> +
> +		if (!wm_state->cxsr)
> +			continue;
> +
> +		/* maxfifo watermarks */
> +		switch (plane->base.type) {
> +			int sprite, level;
> +		case DRM_PLANE_TYPE_CURSOR:
> +			for (level = 0; level < wm_state->num_levels; level++)
> +				wm_state->sr[level].cursor =
> +					wm_state->sr[level].cursor;
> +			break;
> +		case DRM_PLANE_TYPE_PRIMARY:
> +			for (level = 0; level < wm_state->num_levels; level++)
> +				wm_state->sr[level].plane =
> +					min(wm_state->sr[level].plane,
> +					    wm_state->wm[level].primary);
> +			break;
> +		case DRM_PLANE_TYPE_OVERLAY:
> +			sprite = plane->plane;
> +			for (level = 0; level < wm_state->num_levels; level++)
> +				wm_state->sr[level].plane =
> +					min(wm_state->sr[level].plane,
> +					    wm_state->wm[level].sprite[sprite]);
> +			break;
> +		}
> +	}
> +
> +	/* clear any (partially) filled invalid levels */
> +	for (level = wm_state->num_levels; level < CHV_WM_NUM_LEVELS; level++) {
> +		memset(&wm_state->wm[level], 0, sizeof(wm_state->wm[level]));
> +		memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
> +	}
> +
> +	vlv_invert_wms(crtc);
> +}
> +
> +static void vlv_merge_wm(struct drm_device *dev,
> +			 struct vlv_wm_values *wm)
> +{
> +	struct intel_crtc *crtc;
> +	int num_active_crtcs = 0;
> +
> +	if (IS_CHERRYVIEW(dev))
> +		wm->level = VLV_WM_LEVEL_DDR_DVFS;
> +	else
> +		wm->level = VLV_WM_LEVEL_PM2;
> +	wm->cxsr = true;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		const struct vlv_wm_state *wm_state = &crtc->wm_state;
> +
> +		if (!crtc->active)
> +			continue;
> +
> +		if (!wm_state->cxsr)
> +			wm->cxsr = false;
> +
> +		num_active_crtcs++;
> +		wm->level = min_t(int, wm->level, wm_state->num_levels - 1);
> +	}
> +
> +	if (num_active_crtcs != 1)
> +		wm->cxsr = false;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		struct vlv_wm_state *wm_state = &crtc->wm_state;
> +		enum pipe pipe = crtc->pipe;
> +
> +		if (!crtc->active)
> +			continue;
> +
> +		wm->pipe[pipe] = wm_state->wm[wm->level];
> +		if (wm->cxsr)
> +			wm->sr = wm_state->sr[wm->level];
> +
> +		wm->ddl[pipe].primary = DDL_PRECISION_HIGH | 2;
> +		wm->ddl[pipe].sprite[0] = DDL_PRECISION_HIGH | 2;
> +		wm->ddl[pipe].sprite[1] = DDL_PRECISION_HIGH | 2;
> +		wm->ddl[pipe].cursor = DDL_PRECISION_HIGH | 2;

Did we really decide that 0x2 was the final correct value for the DL 
registers? I figured we would be running 0x7 for CHV.

> +	}
> +}
> +
> +static void vlv_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;
> +	struct vlv_wm_values wm = {};
> +
> +	_vlv_compute_wm(intel_crtc);
> +	vlv_merge_wm(dev, &wm);
> +
> +	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0)
> +		return;
> +
> +	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
> +	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
> +		chv_set_memory_dvfs(dev_priv, false);
> +
> +	if (wm.level < VLV_WM_LEVEL_PM5 &&
> +	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
> +		chv_set_memory_pm5(dev_priv, false);
> +
> +	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
> +		intel_set_memory_cxsr(dev_priv, false);
> +		intel_wait_for_vblank(dev, pipe);
> +	}
> +
> +	vlv_write_wm_values(intel_crtc, &wm);
> +
> +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> +		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
> +		      pipe_name(pipe), wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> +		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
> +		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
> +

Love the detailed DRM_DEBUG_KMS - of course its showing the only real 
issue I have found in the series that the cursor watermark is being 
programmed inverted. 63 when the cursor is disabled and 0 when its 
enabled. This leads to an SRR% increase of ~10% when the cursor is 
enabled.

// Cursor disabled
[ 2662.355218] [drm:vlv_update_wm] Setting FIFO watermarks - A: 
plane=340, cursor=63, sprite0=0, sprite1=0, SR: plane=1364, cursor=0 
level=2 cxsr=1

// Cursor Enabled
[ 2667.531049] [drm:vlv_update_wm] Setting FIFO watermarks - A: 
plane=340, cursor=0, sprite0=0, sprite1=0, SR: plane=1364, cursor=0 
level=2 cxsr=1

I haven't found the line of code that actually inverts this programming. 
I will continue to investigate during testing of this series.


> +	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
> +		intel_wait_for_vblank(dev, pipe);
> +		intel_set_memory_cxsr(dev_priv, true);
> +	}
> +
> +	if (wm.level >= VLV_WM_LEVEL_PM5 &&
> +	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
> +		chv_set_memory_pm5(dev_priv, true);
> +
> +	if (wm.level >= VLV_WM_LEVEL_DDR_DVFS &&
> +	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_DDR_DVFS)
> +		chv_set_memory_dvfs(dev_priv, true);
> +
> +	dev_priv->wm.vlv = wm;
>   }
>
>   static void valleyview_update_sprite_wm(struct drm_plane *plane,
> @@ -6823,8 +7128,9 @@ 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 = valleyview_update_wm;
> -		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> +		vlv_setup_wm_latency(dev);
> +
> +		dev_priv->display.update_wm = vlv_update_wm;
>   		dev_priv->display.init_clock_gating =
>   			cherryview_init_clock_gating;
>   	} else if (IS_VALLEYVIEW(dev)) {
>

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

Re: [PATCH 04/10] drm/i915: CHV DDR DVFS support and another watermark rewrite

[Ville Syrjälä]

On Fri, Jun 26, 2015 at 10:56:33AM -0700, Clint Taylor wrote:
> On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Turns out the VLV/CHV system agent doesn't understand memory
> > latencies, so trying to rely on the PND deadline mechanism is not
> > going to fly especially when DDR DVFS is enabled. Currently we try to
> > avoid the problems by lying to the system agent about the deadlines
> > and setting the FIFO watermarks to 8 cachelines. This however leads to
> > bad memory self refresh residency.
> >
> > So in order to satosfy everyone we'll just give up on the deadline
> > scheme and program the watermarks old school based on the worst case
> > memory latency.
> >
> > I've modelled this a bit on the ILK+ approach where we compute multiple
> > sets of watermarks for each pipe (PM2,PM5,DDR DVFS) and when merge thet
> > appropriate one later with the watermarks from other pipes. There isn't
> > too much to merge actually since each pipe has a totally independent
> > FIFO (well apart from the mess with the partially shared DSPARB
> > registers), but still decopuling the pipes from each other seems like a
> > good idea.
> >
> > Eventually we'll want to perform the watermark update in two phases
> > around the plane update to avoid underruns due to the single buffered
> > watermark registers. But that's still in limbo for ILK+ too, so I've not
> > gone that far yet for VLV/CHV either.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |  28 +--
> >   drivers/gpu/drm/i915/intel_display.c |   6 +-
> >   drivers/gpu/drm/i915/intel_drv.h     |  11 ++
> >   drivers/gpu/drm/i915/intel_pm.c      | 318 ++++++++++++++++++++++++++++++++++-
> >   4 files changed, 345 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 514adcf..37cc653 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -276,6 +276,12 @@ struct i915_hotplug {
> >   			    &dev->mode_config.plane_list,	\
> >   			    base.head)
> >
> > +#define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)	\
> > +	list_for_each_entry(intel_plane,				\
> > +			    &(dev)->mode_config.plane_list,		\
> > +			    base.head)					\
> > +		if ((intel_plane)->pipe == (intel_crtc)->pipe)
> > +
> >   #define for_each_intel_crtc(dev, intel_crtc) \
> >   	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
> >
> > @@ -1498,18 +1504,20 @@ struct ilk_wm_values {
> >   	enum intel_ddb_partitioning partitioning;
> >   };
> >
> > -struct vlv_wm_values {
> > -	struct {
> > -		uint16_t primary;
> > -		uint16_t sprite[2];
> > -		uint8_t cursor;
> > -	} pipe[3];
> > +struct vlv_pipe_wm {
> > +	uint16_t primary;
> > +	uint16_t sprite[2];
> > +	uint8_t cursor;
> > +};
> >
> > -	struct {
> > -		uint16_t plane;
> > -		uint8_t cursor;
> > -	} sr;
> > +struct vlv_sr_wm {
> > +	uint16_t plane;
> > +	uint8_t cursor;
> > +};
> >
> > +struct vlv_wm_values {
> > +	struct vlv_pipe_wm pipe[3];
> > +	struct vlv_sr_wm sr;
> >   	struct {
> >   		uint8_t cursor;
> >   		uint8_t sprite[2];
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b15d57f..1424320 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4690,8 +4690,11 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >   	 * event which is after the vblank start event, so we need to have a
> >   	 * wait-for-vblank between disabling the plane and the pipe.
> >   	 */
> > -	if (HAS_GMCH_DISPLAY(dev))
> > +	if (HAS_GMCH_DISPLAY(dev)) {
> >   		intel_set_memory_cxsr(dev_priv, false);
> > +		dev_priv->wm.vlv.cxsr = false;
> > +		intel_wait_for_vblank(dev, pipe);
> > +	}
> >
> >   	/*
> >   	 * FIXME IPS should be fine as long as one plane is
> > @@ -6005,7 +6008,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >
> >   	intel_crtc_load_lut(crtc);
> >
> > -	intel_update_watermarks(crtc);
> >   	intel_enable_pipe(intel_crtc);
> >
> >   	assert_vblank_disabled(crtc);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 3673a71..f26a680 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -462,6 +462,15 @@ struct intel_crtc_state {
> >   	enum pipe hsw_workaround_pipe;
> >   };
> >
> > +struct vlv_wm_state {
> > +	struct vlv_pipe_wm wm[3];
> > +	struct vlv_sr_wm sr[3];
> > +	uint8_t num_active_planes;
> > +	uint8_t num_levels;
> > +	uint8_t level;
> > +	bool cxsr;
> > +};
> > +
> >   struct intel_pipe_wm {
> >   	struct intel_wm_level wm[5];
> >   	uint32_t linetime;
> > @@ -564,6 +573,8 @@ struct intel_crtc {
> >
> >   	/* scalers available on this crtc */
> >   	int num_scalers;
> > +
> > +	struct vlv_wm_state wm_state;
> >   };
> >
> >   struct intel_plane_wm_parameters {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index e67548d..d046e5f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -335,8 +335,6 @@ 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);
> >   		POSTING_READ(FW_BLC_SELF_VLV);
> > -		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);
> >   		POSTING_READ(FW_BLC_SELF);
> > @@ -929,8 +927,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >   	}
> >
> >   	POSTING_READ(DSPFW1);
> > -
> > -	dev_priv->wm.vlv = *wm;
> >   }
> >
> >   #undef FW_WM_VLV
> > @@ -1014,6 +1010,72 @@ enum vlv_wm_level {
> >   	VLV_WM_NUM_LEVELS = 1,
> >   };
> >
> > +/* latency must be in 0.1us units. */
> > +static unsigned int vlv_wm_method2(unsigned int pixel_rate,
> > +				   unsigned int pipe_htotal,
> > +				   unsigned int horiz_pixels,
> > +				   unsigned int bytes_per_pixel,
> > +				   unsigned int latency)
> > +{
> > +	unsigned int ret;
> > +
> > +	ret = (latency * pixel_rate) / (pipe_htotal * 10000);
> > +	ret = (ret + 1) * horiz_pixels * bytes_per_pixel;
> > +	ret = DIV_ROUND_UP(ret, 64);
> > +
> > +	return ret;
> > +}
> > +
> > +static void vlv_setup_wm_latency(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	/* all latencies in usec */
> > +	dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM2] = 3;
> > +
> > +	if (IS_CHERRYVIEW(dev_priv)) {
> > +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM5] = 12;
> > +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_DDR_DVFS] = 33;
> 
> nit #defines for these magic values please

What's the point of doing that? These values are not repeated anywhere
else.

> 
> > +	}
> > +}
> > +
> > +static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> > +				     struct intel_crtc *crtc,
> > +				     const struct intel_plane_state *state,
> > +				     int level)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	int clock, htotal, pixel_size, width, wm;
> > +
> > +	if (dev_priv->wm.pri_latency[level] == 0)
> > +		return USHRT_MAX;
> > +
> > +	if (!state->visible)
> > +		return 0;
> > +
> > +	pixel_size = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> > +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> > +	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
> > +	width = crtc->config->pipe_src_w;
> > +	if (WARN_ON(htotal == 0))
> > +		htotal = 1;
> > +
> > +	if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> > +		/*
> > +		 * FIXME the formula gives values that are
> > +		 * too big for the cursor FIFO, and hence we
> > +		 * would never be able to use cursors. For
> > +		 * now just hardcode the watermark.
> > +		 */
> > +		wm = 63;
> 
> Hard coding to maximum value of 63. Should probably be programmed to 
> worst case instead of maximum.

I have no idea what's the worst case. I was too lazy to try to
empirically deduce some kind of number/formula that works all the time.
Since it's a very small plane usually hardcoding it like this shouldn't
hurt too much (I hope). We can't go above 63, so if that fails we're
screwed anyway.

> > +	} else {
> > +		wm = vlv_wm_method2(clock, htotal, width, pixel_size,
> > +				    dev_priv->wm.pri_latency[level] * 10);
> > +	}
> > +
> > +	return min_t(int, wm, USHRT_MAX);
> > +}
> > +
> >   static bool vlv_compute_sr_wm(struct drm_device *dev,
> >   			      struct vlv_wm_values *wm)
> >   {
> > @@ -1105,6 +1167,249 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> >
> >   	if (cxsr_enabled)
> >   		intel_set_memory_cxsr(dev_priv, true);
> > +
> > +	dev_priv->wm.vlv = wm;
> > +}
> > +
> > +static void vlv_invert_wms(struct intel_crtc *crtc)
> > +{
> > +	struct vlv_wm_state *wm_state = &crtc->wm_state;
> > +	int level;
> > +
> > +	for (level = 0; level < wm_state->num_levels; level++) {
> > +		struct drm_device *dev = crtc->base.dev;
> > +		const int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> > +		struct intel_plane *plane;
> > +
> > +		wm_state->sr[level].plane = sr_fifo_size - wm_state->sr[level].plane;
> > +		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
> > +
> > +		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > +			switch (plane->base.type) {
> > +				int sprite;
> > +			case DRM_PLANE_TYPE_CURSOR:
> > +				wm_state->wm[level].cursor = plane->wm.fifo_size -
> > +					wm_state->wm[level].cursor;
> > +				break;
> > +			case DRM_PLANE_TYPE_PRIMARY:
> > +				wm_state->wm[level].primary = plane->wm.fifo_size -
> > +					wm_state->wm[level].primary;
> > +				break;
> > +			case DRM_PLANE_TYPE_OVERLAY:
> > +				sprite = plane->plane;
> > +				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
> > +					wm_state->wm[level].sprite[sprite];
> > +				break;
> > +			}
> > +		}
> > +	}
> > +}
> > +
> > +static void _vlv_compute_wm(struct intel_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct vlv_wm_state *wm_state = &crtc->wm_state;
> > +	struct intel_plane *plane;
> > +	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> > +	int level;
> > +
> > +	memset(wm_state, 0, sizeof(*wm_state));
> > +
> > +	wm_state->cxsr = crtc->pipe != PIPE_C;
> > +	if (IS_CHERRYVIEW(dev))
> > +		wm_state->num_levels = CHV_WM_NUM_LEVELS;
> > +	else
> > +		wm_state->num_levels = VLV_WM_NUM_LEVELS;
> > +
> > +	wm_state->num_active_planes = 0;
> > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > +		struct intel_plane_state *state =
> > +			to_intel_plane_state(plane->base.state);
> > +
> > +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> > +			continue;
> > +
> > +		if (state->visible)
> > +			wm_state->num_active_planes++;
> > +	}
> > +
> > +	if (wm_state->num_active_planes != 1)
> > +		wm_state->cxsr = false;
> > +
> > +	if (wm_state->cxsr) {
> > +		for (level = 0; level < wm_state->num_levels; level++) {
> > +			wm_state->sr[level].plane = sr_fifo_size;
> > +			wm_state->sr[level].cursor = 63;
> > +		}
> > +	}
> > +
> > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > +		struct intel_plane_state *state =
> > +			to_intel_plane_state(plane->base.state);
> > +
> > +		if (!state->visible)
> > +			continue;
> > +
> > +		/* normal watermarks */
> > +		for (level = 0; level < wm_state->num_levels; level++) {
> > +			int wm = vlv_compute_wm_level(plane, crtc, state, level);
> > +			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> > +
> > +			/* hack */
> > +			if (WARN_ON(level == 0 && wm > max_wm))
> > +				wm = max_wm;
> > +
> > +			if (wm > plane->wm.fifo_size)
> > +				break;
> > +
> > +			switch (plane->base.type) {
> > +				int sprite;
> > +			case DRM_PLANE_TYPE_CURSOR:
> > +				wm_state->wm[level].cursor = wm;
> > +				break;
> > +			case DRM_PLANE_TYPE_PRIMARY:
> > +				wm_state->wm[level].primary = wm;
> > +				break;
> > +			case DRM_PLANE_TYPE_OVERLAY:
> > +				sprite = plane->plane;
> > +				wm_state->wm[level].sprite[sprite] = wm;
> > +				break;
> > +			}
> > +		}
> > +
> > +		wm_state->num_levels = level;
> > +
> > +		if (!wm_state->cxsr)
> > +			continue;
> > +
> > +		/* maxfifo watermarks */
> > +		switch (plane->base.type) {
> > +			int sprite, level;
> > +		case DRM_PLANE_TYPE_CURSOR:
> > +			for (level = 0; level < wm_state->num_levels; level++)
> > +				wm_state->sr[level].cursor =
> > +					wm_state->sr[level].cursor;
> > +			break;
> > +		case DRM_PLANE_TYPE_PRIMARY:
> > +			for (level = 0; level < wm_state->num_levels; level++)
> > +				wm_state->sr[level].plane =
> > +					min(wm_state->sr[level].plane,
> > +					    wm_state->wm[level].primary);
> > +			break;
> > +		case DRM_PLANE_TYPE_OVERLAY:
> > +			sprite = plane->plane;
> > +			for (level = 0; level < wm_state->num_levels; level++)
> > +				wm_state->sr[level].plane =
> > +					min(wm_state->sr[level].plane,
> > +					    wm_state->wm[level].sprite[sprite]);
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* clear any (partially) filled invalid levels */
> > +	for (level = wm_state->num_levels; level < CHV_WM_NUM_LEVELS; level++) {
> > +		memset(&wm_state->wm[level], 0, sizeof(wm_state->wm[level]));
> > +		memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
> > +	}
> > +
> > +	vlv_invert_wms(crtc);
> > +}
> > +
> > +static void vlv_merge_wm(struct drm_device *dev,
> > +			 struct vlv_wm_values *wm)
> > +{
> > +	struct intel_crtc *crtc;
> > +	int num_active_crtcs = 0;
> > +
> > +	if (IS_CHERRYVIEW(dev))
> > +		wm->level = VLV_WM_LEVEL_DDR_DVFS;
> > +	else
> > +		wm->level = VLV_WM_LEVEL_PM2;
> > +	wm->cxsr = true;
> > +
> > +	for_each_intel_crtc(dev, crtc) {
> > +		const struct vlv_wm_state *wm_state = &crtc->wm_state;
> > +
> > +		if (!crtc->active)
> > +			continue;
> > +
> > +		if (!wm_state->cxsr)
> > +			wm->cxsr = false;
> > +
> > +		num_active_crtcs++;
> > +		wm->level = min_t(int, wm->level, wm_state->num_levels - 1);
> > +	}
> > +
> > +	if (num_active_crtcs != 1)
> > +		wm->cxsr = false;
> > +
> > +	for_each_intel_crtc(dev, crtc) {
> > +		struct vlv_wm_state *wm_state = &crtc->wm_state;
> > +		enum pipe pipe = crtc->pipe;
> > +
> > +		if (!crtc->active)
> > +			continue;
> > +
> > +		wm->pipe[pipe] = wm_state->wm[wm->level];
> > +		if (wm->cxsr)
> > +			wm->sr = wm_state->sr[wm->level];
> > +
> > +		wm->ddl[pipe].primary = DDL_PRECISION_HIGH | 2;
> > +		wm->ddl[pipe].sprite[0] = DDL_PRECISION_HIGH | 2;
> > +		wm->ddl[pipe].sprite[1] = DDL_PRECISION_HIGH | 2;
> > +		wm->ddl[pipe].cursor = DDL_PRECISION_HIGH | 2;
> 
> Did we really decide that 0x2 was the final correct value for the DL 
> registers? I figured we would be running 0x7 for CHV.

I'm wary of increasing it too much. That'll give the system agent a
better chance of making a mess of things.

> 
> > +	}
> > +}
> > +
> > +static void vlv_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;
> > +	struct vlv_wm_values wm = {};
> > +
> > +	_vlv_compute_wm(intel_crtc);
> > +	vlv_merge_wm(dev, &wm);
> > +
> > +	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
> > +	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
> > +		chv_set_memory_dvfs(dev_priv, false);
> > +
> > +	if (wm.level < VLV_WM_LEVEL_PM5 &&
> > +	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
> > +		chv_set_memory_pm5(dev_priv, false);
> > +
> > +	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
> > +		intel_set_memory_cxsr(dev_priv, false);
> > +		intel_wait_for_vblank(dev, pipe);
> > +	}
> > +
> > +	vlv_write_wm_values(intel_crtc, &wm);
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> > +		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
> > +		      pipe_name(pipe), wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> > +		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
> > +		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
> > +
> 
> Love the detailed DRM_DEBUG_KMS - of course its showing the only real 
> issue I have found in the series that the cursor watermark is being 
> programmed inverted. 63 when the cursor is disabled and 0 when its 
> enabled. This leads to an SRR% increase of ~10% when the cursor is 
> enabled.
> 
> // Cursor disabled
> [ 2662.355218] [drm:vlv_update_wm] Setting FIFO watermarks - A: 
> plane=340, cursor=63, sprite0=0, sprite1=0, SR: plane=1364, cursor=0 
> level=2 cxsr=1
> 
> // Cursor Enabled
> [ 2667.531049] [drm:vlv_update_wm] Setting FIFO watermarks - A: 
> plane=340, cursor=0, sprite0=0, sprite1=0, SR: plane=1364, cursor=0 
> level=2 cxsr=1
> 
> I haven't found the line of code that actually inverts this programming. 
> I will continue to investigate during testing of this series.

vlv_invert_wms()

That's done on purpose since calculating the watermarks the "right way
up" first makes makes it easier to think about them (ie. to compare with
the FIFO size, and to merge the plane SR watermark). It also make things
looks more ILK-like since ILK+ take the watermarks in the non-inverted
form.

> 
> 
> > +	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
> > +		intel_wait_for_vblank(dev, pipe);
> > +		intel_set_memory_cxsr(dev_priv, true);
> > +	}
> > +
> > +	if (wm.level >= VLV_WM_LEVEL_PM5 &&
> > +	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
> > +		chv_set_memory_pm5(dev_priv, true);
> > +
> > +	if (wm.level >= VLV_WM_LEVEL_DDR_DVFS &&
> > +	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_DDR_DVFS)
> > +		chv_set_memory_dvfs(dev_priv, true);
> > +
> > +	dev_priv->wm.vlv = wm;
> >   }
> >
> >   static void valleyview_update_sprite_wm(struct drm_plane *plane,
> > @@ -6823,8 +7128,9 @@ 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 = valleyview_update_wm;
> > -		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> > +		vlv_setup_wm_latency(dev);
> > +
> > +		dev_priv->display.update_wm = vlv_update_wm;
> >   		dev_priv->display.init_clock_gating =
> >   			cherryview_init_clock_gating;
> >   	} else if (IS_VALLEYVIEW(dev)) {
> >

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

Re: [PATCH 04/10] drm/i915: CHV DDR DVFS support and another watermark rewrite

[Clint Taylor]

On 06/26/2015 12:48 PM, Ville Syrjälä wrote:
> On Fri, Jun 26, 2015 at 10:56:33AM -0700, Clint Taylor wrote:
>> On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Turns out the VLV/CHV system agent doesn't understand memory
>>> latencies, so trying to rely on the PND deadline mechanism is not
>>> going to fly especially when DDR DVFS is enabled. Currently we try to
>>> avoid the problems by lying to the system agent about the deadlines
>>> and setting the FIFO watermarks to 8 cachelines. This however leads to
>>> bad memory self refresh residency.
>>>
>>> So in order to satosfy everyone we'll just give up on the deadline
>>> scheme and program the watermarks old school based on the worst case
>>> memory latency.
>>>
>>> I've modelled this a bit on the ILK+ approach where we compute multiple
>>> sets of watermarks for each pipe (PM2,PM5,DDR DVFS) and when merge thet
>>> appropriate one later with the watermarks from other pipes. There isn't
>>> too much to merge actually since each pipe has a totally independent
>>> FIFO (well apart from the mess with the partially shared DSPARB
>>> registers), but still decopuling the pipes from each other seems like a
>>> good idea.
>>>
>>> Eventually we'll want to perform the watermark update in two phases
>>> around the plane update to avoid underruns due to the single buffered
>>> watermark registers. But that's still in limbo for ILK+ too, so I've not
>>> gone that far yet for VLV/CHV either.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.h      |  28 +--
>>>    drivers/gpu/drm/i915/intel_display.c |   6 +-
>>>    drivers/gpu/drm/i915/intel_drv.h     |  11 ++
>>>    drivers/gpu/drm/i915/intel_pm.c      | 318 ++++++++++++++++++++++++++++++++++-
>>>    4 files changed, 345 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 514adcf..37cc653 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -276,6 +276,12 @@ struct i915_hotplug {
>>>    			    &dev->mode_config.plane_list,	\
>>>    			    base.head)
>>>
>>> +#define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)	\
>>> +	list_for_each_entry(intel_plane,				\
>>> +			    &(dev)->mode_config.plane_list,		\
>>> +			    base.head)					\
>>> +		if ((intel_plane)->pipe == (intel_crtc)->pipe)
>>> +
>>>    #define for_each_intel_crtc(dev, intel_crtc) \
>>>    	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
>>>
>>> @@ -1498,18 +1504,20 @@ struct ilk_wm_values {
>>>    	enum intel_ddb_partitioning partitioning;
>>>    };
>>>
>>> -struct vlv_wm_values {
>>> -	struct {
>>> -		uint16_t primary;
>>> -		uint16_t sprite[2];
>>> -		uint8_t cursor;
>>> -	} pipe[3];
>>> +struct vlv_pipe_wm {
>>> +	uint16_t primary;
>>> +	uint16_t sprite[2];
>>> +	uint8_t cursor;
>>> +};
>>>
>>> -	struct {
>>> -		uint16_t plane;
>>> -		uint8_t cursor;
>>> -	} sr;
>>> +struct vlv_sr_wm {
>>> +	uint16_t plane;
>>> +	uint8_t cursor;
>>> +};
>>>
>>> +struct vlv_wm_values {
>>> +	struct vlv_pipe_wm pipe[3];
>>> +	struct vlv_sr_wm sr;
>>>    	struct {
>>>    		uint8_t cursor;
>>>    		uint8_t sprite[2];
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index b15d57f..1424320 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -4690,8 +4690,11 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>>>    	 * event which is after the vblank start event, so we need to have a
>>>    	 * wait-for-vblank between disabling the plane and the pipe.
>>>    	 */
>>> -	if (HAS_GMCH_DISPLAY(dev))
>>> +	if (HAS_GMCH_DISPLAY(dev)) {
>>>    		intel_set_memory_cxsr(dev_priv, false);
>>> +		dev_priv->wm.vlv.cxsr = false;
>>> +		intel_wait_for_vblank(dev, pipe);
>>> +	}
>>>
>>>    	/*
>>>    	 * FIXME IPS should be fine as long as one plane is
>>> @@ -6005,7 +6008,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>>>
>>>    	intel_crtc_load_lut(crtc);
>>>
>>> -	intel_update_watermarks(crtc);
>>>    	intel_enable_pipe(intel_crtc);
>>>
>>>    	assert_vblank_disabled(crtc);
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 3673a71..f26a680 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -462,6 +462,15 @@ struct intel_crtc_state {
>>>    	enum pipe hsw_workaround_pipe;
>>>    };
>>>
>>> +struct vlv_wm_state {
>>> +	struct vlv_pipe_wm wm[3];
>>> +	struct vlv_sr_wm sr[3];
>>> +	uint8_t num_active_planes;
>>> +	uint8_t num_levels;
>>> +	uint8_t level;
>>> +	bool cxsr;
>>> +};
>>> +
>>>    struct intel_pipe_wm {
>>>    	struct intel_wm_level wm[5];
>>>    	uint32_t linetime;
>>> @@ -564,6 +573,8 @@ struct intel_crtc {
>>>
>>>    	/* scalers available on this crtc */
>>>    	int num_scalers;
>>> +
>>> +	struct vlv_wm_state wm_state;
>>>    };
>>>
>>>    struct intel_plane_wm_parameters {
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index e67548d..d046e5f 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -335,8 +335,6 @@ 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);
>>>    		POSTING_READ(FW_BLC_SELF_VLV);
>>> -		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);
>>>    		POSTING_READ(FW_BLC_SELF);
>>> @@ -929,8 +927,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>>>    	}
>>>
>>>    	POSTING_READ(DSPFW1);
>>> -
>>> -	dev_priv->wm.vlv = *wm;
>>>    }
>>>
>>>    #undef FW_WM_VLV
>>> @@ -1014,6 +1010,72 @@ enum vlv_wm_level {
>>>    	VLV_WM_NUM_LEVELS = 1,
>>>    };
>>>
>>> +/* latency must be in 0.1us units. */
>>> +static unsigned int vlv_wm_method2(unsigned int pixel_rate,
>>> +				   unsigned int pipe_htotal,
>>> +				   unsigned int horiz_pixels,
>>> +				   unsigned int bytes_per_pixel,
>>> +				   unsigned int latency)
>>> +{
>>> +	unsigned int ret;
>>> +
>>> +	ret = (latency * pixel_rate) / (pipe_htotal * 10000);
>>> +	ret = (ret + 1) * horiz_pixels * bytes_per_pixel;
>>> +	ret = DIV_ROUND_UP(ret, 64);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void vlv_setup_wm_latency(struct drm_device *dev)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +
>>> +	/* all latencies in usec */
>>> +	dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM2] = 3;
>>> +
>>> +	if (IS_CHERRYVIEW(dev_priv)) {
>>> +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM5] = 12;
>>> +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_DDR_DVFS] = 33;
>>
>> nit #defines for these magic values please
>
> What's the point of doing that? These values are not repeated anywhere
> else.
>
>>
>>> +	}
>>> +}
>>> +
>>> +static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>>> +				     struct intel_crtc *crtc,
>>> +				     const struct intel_plane_state *state,
>>> +				     int level)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> +	int clock, htotal, pixel_size, width, wm;
>>> +
>>> +	if (dev_priv->wm.pri_latency[level] == 0)
>>> +		return USHRT_MAX;
>>> +
>>> +	if (!state->visible)
>>> +		return 0;
>>> +
>>> +	pixel_size = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
>>> +	clock = crtc->config->base.adjusted_mode.crtc_clock;
>>> +	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
>>> +	width = crtc->config->pipe_src_w;
>>> +	if (WARN_ON(htotal == 0))
>>> +		htotal = 1;
>>> +
>>> +	if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
>>> +		/*
>>> +		 * FIXME the formula gives values that are
>>> +		 * too big for the cursor FIFO, and hence we
>>> +		 * would never be able to use cursors. For
>>> +		 * now just hardcode the watermark.
>>> +		 */
>>> +		wm = 63;
>>
>> Hard coding to maximum value of 63. Should probably be programmed to
>> worst case instead of maximum.
>
> I have no idea what's the worst case. I was too lazy to try to
> empirically deduce some kind of number/formula that works all the time.
> Since it's a very small plane usually hardcoding it like this shouldn't
> hurt too much (I hope). We can't go above 63, so if that fails we're
> screwed anyway.
>

63 gets inverted to 0 and we are screwed if that doesn't work.

>>> +	} else {
>>> +		wm = vlv_wm_method2(clock, htotal, width, pixel_size,
>>> +				    dev_priv->wm.pri_latency[level] * 10);
>>> +	}
>>> +
>>> +	return min_t(int, wm, USHRT_MAX);
>>> +}
>>> +
>>>    static bool vlv_compute_sr_wm(struct drm_device *dev,
>>>    			      struct vlv_wm_values *wm)
>>>    {
>>> @@ -1105,6 +1167,249 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>>>
>>>    	if (cxsr_enabled)
>>>    		intel_set_memory_cxsr(dev_priv, true);
>>> +
>>> +	dev_priv->wm.vlv = wm;
>>> +}
>>> +
>>> +static void vlv_invert_wms(struct intel_crtc *crtc)
>>> +{
>>> +	struct vlv_wm_state *wm_state = &crtc->wm_state;
>>> +	int level;
>>> +
>>> +	for (level = 0; level < wm_state->num_levels; level++) {
>>> +		struct drm_device *dev = crtc->base.dev;
>>> +		const int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
>>> +		struct intel_plane *plane;
>>> +
>>> +		wm_state->sr[level].plane = sr_fifo_size - wm_state->sr[level].plane;
>>> +		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
>>> +
>>> +		for_each_intel_plane_on_crtc(dev, crtc, plane) {
>>> +			switch (plane->base.type) {
>>> +				int sprite;
>>> +			case DRM_PLANE_TYPE_CURSOR:
>>> +				wm_state->wm[level].cursor = plane->wm.fifo_size -
>>> +					wm_state->wm[level].cursor;
>>> +				break;
>>> +			case DRM_PLANE_TYPE_PRIMARY:
>>> +				wm_state->wm[level].primary = plane->wm.fifo_size -
>>> +					wm_state->wm[level].primary;
>>> +				break;
>>> +			case DRM_PLANE_TYPE_OVERLAY:
>>> +				sprite = plane->plane;
>>> +				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
>>> +					wm_state->wm[level].sprite[sprite];
>>> +				break;
>>> +			}
>>> +		}
>>> +	}
>>> +}
>>> +
>>> +static void _vlv_compute_wm(struct intel_crtc *crtc)
>>> +{
>>> +	struct drm_device *dev = crtc->base.dev;
>>> +	struct vlv_wm_state *wm_state = &crtc->wm_state;
>>> +	struct intel_plane *plane;
>>> +	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
>>> +	int level;
>>> +
>>> +	memset(wm_state, 0, sizeof(*wm_state));
>>> +
>>> +	wm_state->cxsr = crtc->pipe != PIPE_C;
>>> +	if (IS_CHERRYVIEW(dev))
>>> +		wm_state->num_levels = CHV_WM_NUM_LEVELS;
>>> +	else
>>> +		wm_state->num_levels = VLV_WM_NUM_LEVELS;
>>> +
>>> +	wm_state->num_active_planes = 0;
>>> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
>>> +		struct intel_plane_state *state =
>>> +			to_intel_plane_state(plane->base.state);
>>> +
>>> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
>>> +			continue;
>>> +
>>> +		if (state->visible)
>>> +			wm_state->num_active_planes++;
>>> +	}
>>> +
>>> +	if (wm_state->num_active_planes != 1)
>>> +		wm_state->cxsr = false;
>>> +
>>> +	if (wm_state->cxsr) {
>>> +		for (level = 0; level < wm_state->num_levels; level++) {
>>> +			wm_state->sr[level].plane = sr_fifo_size;
>>> +			wm_state->sr[level].cursor = 63;
>>> +		}
>>> +	}
>>> +
>>> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
>>> +		struct intel_plane_state *state =
>>> +			to_intel_plane_state(plane->base.state);
>>> +
>>> +		if (!state->visible)
>>> +			continue;
>>> +
>>> +		/* normal watermarks */
>>> +		for (level = 0; level < wm_state->num_levels; level++) {
>>> +			int wm = vlv_compute_wm_level(plane, crtc, state, level);
>>> +			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>>> +
>>> +			/* hack */
>>> +			if (WARN_ON(level == 0 && wm > max_wm))
>>> +				wm = max_wm;
>>> +
>>> +			if (wm > plane->wm.fifo_size)
>>> +				break;
>>> +
>>> +			switch (plane->base.type) {
>>> +				int sprite;
>>> +			case DRM_PLANE_TYPE_CURSOR:
>>> +				wm_state->wm[level].cursor = wm;
>>> +				break;
>>> +			case DRM_PLANE_TYPE_PRIMARY:
>>> +				wm_state->wm[level].primary = wm;
>>> +				break;
>>> +			case DRM_PLANE_TYPE_OVERLAY:
>>> +				sprite = plane->plane;
>>> +				wm_state->wm[level].sprite[sprite] = wm;
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		wm_state->num_levels = level;
>>> +
>>> +		if (!wm_state->cxsr)
>>> +			continue;
>>> +
>>> +		/* maxfifo watermarks */
>>> +		switch (plane->base.type) {
>>> +			int sprite, level;
>>> +		case DRM_PLANE_TYPE_CURSOR:
>>> +			for (level = 0; level < wm_state->num_levels; level++)
>>> +				wm_state->sr[level].cursor =
>>> +					wm_state->sr[level].cursor;
>>> +			break;
>>> +		case DRM_PLANE_TYPE_PRIMARY:
>>> +			for (level = 0; level < wm_state->num_levels; level++)
>>> +				wm_state->sr[level].plane =
>>> +					min(wm_state->sr[level].plane,
>>> +					    wm_state->wm[level].primary);
>>> +			break;
>>> +		case DRM_PLANE_TYPE_OVERLAY:
>>> +			sprite = plane->plane;
>>> +			for (level = 0; level < wm_state->num_levels; level++)
>>> +				wm_state->sr[level].plane =
>>> +					min(wm_state->sr[level].plane,
>>> +					    wm_state->wm[level].sprite[sprite]);
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	/* clear any (partially) filled invalid levels */
>>> +	for (level = wm_state->num_levels; level < CHV_WM_NUM_LEVELS; level++) {
>>> +		memset(&wm_state->wm[level], 0, sizeof(wm_state->wm[level]));
>>> +		memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
>>> +	}
>>> +
>>> +	vlv_invert_wms(crtc);
>>> +}
>>> +
>>> +static void vlv_merge_wm(struct drm_device *dev,
>>> +			 struct vlv_wm_values *wm)
>>> +{
>>> +	struct intel_crtc *crtc;
>>> +	int num_active_crtcs = 0;
>>> +
>>> +	if (IS_CHERRYVIEW(dev))
>>> +		wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>> +	else
>>> +		wm->level = VLV_WM_LEVEL_PM2;
>>> +	wm->cxsr = true;
>>> +
>>> +	for_each_intel_crtc(dev, crtc) {
>>> +		const struct vlv_wm_state *wm_state = &crtc->wm_state;
>>> +
>>> +		if (!crtc->active)
>>> +			continue;
>>> +
>>> +		if (!wm_state->cxsr)
>>> +			wm->cxsr = false;
>>> +
>>> +		num_active_crtcs++;
>>> +		wm->level = min_t(int, wm->level, wm_state->num_levels - 1);
>>> +	}
>>> +
>>> +	if (num_active_crtcs != 1)
>>> +		wm->cxsr = false;
>>> +
>>> +	for_each_intel_crtc(dev, crtc) {
>>> +		struct vlv_wm_state *wm_state = &crtc->wm_state;
>>> +		enum pipe pipe = crtc->pipe;
>>> +
>>> +		if (!crtc->active)
>>> +			continue;
>>> +
>>> +		wm->pipe[pipe] = wm_state->wm[wm->level];
>>> +		if (wm->cxsr)
>>> +			wm->sr = wm_state->sr[wm->level];
>>> +
>>> +		wm->ddl[pipe].primary = DDL_PRECISION_HIGH | 2;
>>> +		wm->ddl[pipe].sprite[0] = DDL_PRECISION_HIGH | 2;
>>> +		wm->ddl[pipe].sprite[1] = DDL_PRECISION_HIGH | 2;
>>> +		wm->ddl[pipe].cursor = DDL_PRECISION_HIGH | 2;
>>
>> Did we really decide that 0x2 was the final correct value for the DL
>> registers? I figured we would be running 0x7 for CHV.
>
> I'm wary of increasing it too much. That'll give the system agent a
> better chance of making a mess of things.
>

It does appear to work at a level of 2 and gives us room for improvement 
if there is a performance problem in the furture

>>
>>> +	}
>>> +}
>>> +
>>> +static void vlv_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;
>>> +	struct vlv_wm_values wm = {};
>>> +
>>> +	_vlv_compute_wm(intel_crtc);
>>> +	vlv_merge_wm(dev, &wm);
>>> +
>>> +	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0)
>>> +		return;
>>> +
>>> +	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
>>> +	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
>>> +		chv_set_memory_dvfs(dev_priv, false);
>>> +
>>> +	if (wm.level < VLV_WM_LEVEL_PM5 &&
>>> +	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
>>> +		chv_set_memory_pm5(dev_priv, false);
>>> +
>>> +	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
>>> +		intel_set_memory_cxsr(dev_priv, false);
>>> +		intel_wait_for_vblank(dev, pipe);
>>> +	}
>>> +
>>> +	vlv_write_wm_values(intel_crtc, &wm);
>>> +
>>> +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
>>> +		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
>>> +		      pipe_name(pipe), wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
>>> +		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
>>> +		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
>>> +
>>
>> Love the detailed DRM_DEBUG_KMS - of course its showing the only real
>> issue I have found in the series that the cursor watermark is being
>> programmed inverted. 63 when the cursor is disabled and 0 when its
>> enabled. This leads to an SRR% increase of ~10% when the cursor is
>> enabled.
>>
>> // Cursor disabled
>> [ 2662.355218] [drm:vlv_update_wm] Setting FIFO watermarks - A:
>> plane=340, cursor=63, sprite0=0, sprite1=0, SR: plane=1364, cursor=0
>> level=2 cxsr=1
>>
>> // Cursor Enabled
>> [ 2667.531049] [drm:vlv_update_wm] Setting FIFO watermarks - A:
>> plane=340, cursor=0, sprite0=0, sprite1=0, SR: plane=1364, cursor=0
>> level=2 cxsr=1
>>
>> I haven't found the line of code that actually inverts this programming.
>> I will continue to investigate during testing of this series.
>
> vlv_invert_wms()
>
> That's done on purpose since calculating the watermarks the "right way
> up" first makes makes it easier to think about them (ie. to compare with
> the FIFO size, and to merge the plane SR watermark). It also make things
> looks more ILK-like since ILK+ take the watermarks in the non-inverted
> form.
>

I wasn't aware the registers were inverted compared to ILK+. This is the 
only real issue I had with the series and after testing here by writing 
63 into the register I agree that this isn't really a problem.

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

>>
>>
>>> +	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
>>> +		intel_wait_for_vblank(dev, pipe);
>>> +		intel_set_memory_cxsr(dev_priv, true);
>>> +	}
>>> +
>>> +	if (wm.level >= VLV_WM_LEVEL_PM5 &&
>>> +	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
>>> +		chv_set_memory_pm5(dev_priv, true);
>>> +
>>> +	if (wm.level >= VLV_WM_LEVEL_DDR_DVFS &&
>>> +	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_DDR_DVFS)
>>> +		chv_set_memory_dvfs(dev_priv, true);
>>> +
>>> +	dev_priv->wm.vlv = wm;
>>>    }
>>>
>>>    static void valleyview_update_sprite_wm(struct drm_plane *plane,
>>> @@ -6823,8 +7128,9 @@ 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 = valleyview_update_wm;
>>> -		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
>>> +		vlv_setup_wm_latency(dev);
>>> +
>>> +		dev_priv->display.update_wm = vlv_update_wm;
>>>    		dev_priv->display.init_clock_gating =
>>>    			cherryview_init_clock_gating;
>>>    	} else if (IS_VALLEYVIEW(dev)) {
>>>
>

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

Re: [PATCH 04/10] drm/i915: CHV DDR DVFS support and another watermark rewrite

[Jani Nikula]

On Fri, 26 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jun 26, 2015 at 10:56:33AM -0700, Clint Taylor wrote:
>> On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
>> > +	if (IS_CHERRYVIEW(dev_priv)) {
>> > +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM5] = 12;
>> > +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_DDR_DVFS] = 33;
>> 
>> nit #defines for these magic values please
>
> What's the point of doing that? These values are not repeated anywhere
> else.

Documentation.

BR,
Jani.


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

Re: [PATCH 04/10] drm/i915: CHV DDR DVFS support and another watermark rewrite

[Daniel Vetter]

On Mon, Jun 29, 2015 at 11:03:04AM +0300, Jani Nikula wrote:
> On Fri, 26 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jun 26, 2015 at 10:56:33AM -0700, Clint Taylor wrote:
> >> On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> >> > +	if (IS_CHERRYVIEW(dev_priv)) {
> >> > +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_PM5] = 12;
> >> > +		dev_priv->wm.pri_latency[VLV_WM_LEVEL_DDR_DVFS] = 33;
> >> 
> >> nit #defines for these magic values please
> >
> > What's the point of doing that? These values are not repeated anywhere
> > else.
> 
> Documentation.

I've seend the original watermark code which did this for the massive mess
that where gen2/3/4 wm code. It was unreadable, unreviewable and because
of that had bugs. I concur with Ville here.

What we might want to do is a macro to do the "logical wm setting" -> hw
value encoding, since there's some surprising differences there between
platforms. But imo that's better done as some large-scale overall project.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/10] drm/i915: Compute display FIFO split dynamically for CHV

[ville.syrjala]

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

Consider which planes are active and compute the FIFO split based on the
relative data rates. Since we only consider the pipe src width rather
than the plane width when computing watermarks it seems best to do the
same when computing the FIFO split as well. This means the only thing we
actually have to consider for the FIFO splut is the bpp, and we can
ignore the rest.

I've just stuffed the logic into the watermark code for now. Eventually
it'll need to move into the atomic update for the crtc.

There's also one extra complication I've not yet considered; Some of the
DSPARB registers contain bits related to multiple pipes. The registers
are double buffered but apparently they update on the vblank of any
active pipe. So doing the FIFO reconfiguration properly when multiple
pipes are active is not going to be fun. But let's ignore that mess for
now.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b9f6b8c..fa6780f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4411,9 +4411,32 @@ enum skl_disp_power_wells {
 #define   DSPARB_BSTART_SHIFT	0
 #define   DSPARB_BEND_SHIFT	9 /* on 855 */
 #define   DSPARB_AEND_SHIFT	0
-
+#define   DSPARB_SPRITEA_SHIFT_VLV	0
+#define   DSPARB_SPRITEA_MASK_VLV	(0xff << 0)
+#define   DSPARB_SPRITEB_SHIFT_VLV	8
+#define   DSPARB_SPRITEB_MASK_VLV	(0xff << 8)
+#define   DSPARB_SPRITEC_SHIFT_VLV	16
+#define   DSPARB_SPRITEC_MASK_VLV	(0xff << 16)
+#define   DSPARB_SPRITED_SHIFT_VLV	24
+#define   DSPARB_SPRITED_MASK_VLV	(0xff << 24)
 #define DSPARB2			(VLV_DISPLAY_BASE + 0x70060) /* vlv/chv */
+#define   DSPARB_SPRITEA_HI_SHIFT_VLV	0
+#define   DSPARB_SPRITEA_HI_MASK_VLV	(0x1 << 0)
+#define   DSPARB_SPRITEB_HI_SHIFT_VLV	4
+#define   DSPARB_SPRITEB_HI_MASK_VLV	(0x1 << 4)
+#define   DSPARB_SPRITEC_HI_SHIFT_VLV	8
+#define   DSPARB_SPRITEC_HI_MASK_VLV	(0x1 << 8)
+#define   DSPARB_SPRITED_HI_SHIFT_VLV	12
+#define   DSPARB_SPRITED_HI_MASK_VLV	(0x1 << 12)
+#define   DSPARB_SPRITEE_HI_SHIFT_VLV	16
+#define   DSPARB_SPRITEE_HI_MASK_VLV	(0x1 << 16)
+#define   DSPARB_SPRITEF_HI_SHIFT_VLV	20
+#define   DSPARB_SPRITEF_HI_MASK_VLV	(0x1 << 20)
 #define DSPARB3			(VLV_DISPLAY_BASE + 0x7006c) /* chv */
+#define   DSPARB_SPRITEE_SHIFT_VLV	0
+#define   DSPARB_SPRITEE_MASK_VLV	(0xff << 0)
+#define   DSPARB_SPRITEF_SHIFT_VLV	8
+#define   DSPARB_SPRITEF_MASK_VLV	(0xff << 8)
 
 /* pnv/gen4/g4x/vlv/chv */
 #define DSPFW1			(dev_priv->info.display_mmio_offset + 0x70034)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d046e5f..ffdca62 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1171,6 +1171,73 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
 	dev_priv->wm.vlv = wm;
 }
 
+static void vlv_compute_fifo(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct vlv_wm_state *wm_state = &crtc->wm_state;
+	struct intel_plane *plane;
+	unsigned int total_rate = 0;
+	const int fifo_size = 512 - 1;
+	int fifo_extra, fifo_left = fifo_size;
+
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		struct intel_plane_state *state =
+			to_intel_plane_state(plane->base.state);
+
+		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			continue;
+
+		if (state->visible) {
+			wm_state->num_active_planes++;
+			total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
+		}
+	}
+
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		struct intel_plane_state *state =
+			to_intel_plane_state(plane->base.state);
+		unsigned int rate;
+
+		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
+			plane->wm.fifo_size = 63;
+			continue;
+		}
+
+		if (!state->visible) {
+			plane->wm.fifo_size = 0;
+			continue;
+		}
+
+		rate = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
+		plane->wm.fifo_size = fifo_size * rate / total_rate;
+		fifo_left -= plane->wm.fifo_size;
+	}
+
+	fifo_extra = DIV_ROUND_UP(fifo_left, wm_state->num_active_planes ?: 1);
+
+	/* spread the remainder evenly */
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		int plane_extra;
+
+		if (fifo_left == 0)
+			break;
+
+		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			continue;
+
+		/* give it all to the first plane if none are active */
+		if (plane->wm.fifo_size == 0 &&
+		    wm_state->num_active_planes)
+			continue;
+
+		plane_extra = min(fifo_extra, fifo_left);
+		plane->wm.fifo_size += plane_extra;
+		fifo_left -= plane_extra;
+	}
+
+	WARN_ON(fifo_left != 0);
+}
+
 static void vlv_invert_wms(struct intel_crtc *crtc)
 {
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
@@ -1222,16 +1289,8 @@ static void _vlv_compute_wm(struct intel_crtc *crtc)
 		wm_state->num_levels = VLV_WM_NUM_LEVELS;
 
 	wm_state->num_active_planes = 0;
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
-		struct intel_plane_state *state =
-			to_intel_plane_state(plane->base.state);
-
-		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
-			continue;
 
-		if (state->visible)
-			wm_state->num_active_planes++;
-	}
+	vlv_compute_fifo(crtc);
 
 	if (wm_state->num_active_planes != 1)
 		wm_state->cxsr = false;
@@ -1315,6 +1374,96 @@ static void _vlv_compute_wm(struct intel_crtc *crtc)
 	vlv_invert_wms(crtc);
 }
 
+#define VLV_FIFO(plane, value) \
+	(((value) << DSPARB_ ## plane ## _SHIFT_VLV) & DSPARB_ ## plane ## _MASK_VLV)
+
+static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_plane *plane;
+	int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
+
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
+			WARN_ON(plane->wm.fifo_size != 63);
+			continue;
+		}
+
+		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
+			sprite0_start = plane->wm.fifo_size;
+		else if (plane->plane == 0)
+			sprite1_start = sprite0_start + plane->wm.fifo_size;
+		else
+			fifo_size = sprite1_start + plane->wm.fifo_size;
+	}
+
+	WARN_ON(fifo_size != 512 - 1);
+
+	DRM_DEBUG_KMS("Pipe %c FIFO split %d / %d / %d\n",
+		      pipe_name(crtc->pipe), sprite0_start,
+		      sprite1_start, fifo_size);
+
+	switch (crtc->pipe) {
+		uint32_t dsparb, dsparb2, dsparb3;
+	case PIPE_A:
+		dsparb = I915_READ(DSPARB);
+		dsparb2 = I915_READ(DSPARB2);
+
+		dsparb &= ~(VLV_FIFO(SPRITEA, 0xff) |
+			    VLV_FIFO(SPRITEB, 0xff));
+		dsparb |= (VLV_FIFO(SPRITEA, sprite0_start) |
+			   VLV_FIFO(SPRITEB, sprite1_start));
+
+		dsparb2 &= ~(VLV_FIFO(SPRITEA_HI, 0x1) |
+			     VLV_FIFO(SPRITEB_HI, 0x1));
+		dsparb2 |= (VLV_FIFO(SPRITEA_HI, sprite0_start >> 8) |
+			   VLV_FIFO(SPRITEB_HI, sprite1_start >> 8));
+
+		I915_WRITE(DSPARB, dsparb);
+		I915_WRITE(DSPARB2, dsparb2);
+		break;
+	case PIPE_B:
+		dsparb = I915_READ(DSPARB);
+		dsparb2 = I915_READ(DSPARB2);
+
+		dsparb &= ~(VLV_FIFO(SPRITEC, 0xff) |
+			    VLV_FIFO(SPRITED, 0xff));
+		dsparb |= (VLV_FIFO(SPRITEC, sprite0_start) |
+			   VLV_FIFO(SPRITED, sprite1_start));
+
+		dsparb2 &= ~(VLV_FIFO(SPRITEC_HI, 0xff) |
+			     VLV_FIFO(SPRITED_HI, 0xff));
+		dsparb2 |= (VLV_FIFO(SPRITEC_HI, sprite0_start >> 8) |
+			   VLV_FIFO(SPRITED_HI, sprite1_start >> 8));
+
+		I915_WRITE(DSPARB, dsparb);
+		I915_WRITE(DSPARB2, dsparb2);
+		break;
+	case PIPE_C:
+		dsparb3 = I915_READ(DSPARB3);
+		dsparb2 = I915_READ(DSPARB2);
+
+		dsparb3 &= ~(VLV_FIFO(SPRITEE, 0xff) |
+			     VLV_FIFO(SPRITEF, 0xff));
+		dsparb3 |= (VLV_FIFO(SPRITEE, sprite0_start) |
+			    VLV_FIFO(SPRITEF, sprite1_start));
+
+		dsparb2 &= ~(VLV_FIFO(SPRITEE_HI, 0xff) |
+			     VLV_FIFO(SPRITEF_HI, 0xff));
+		dsparb2 |= (VLV_FIFO(SPRITEE_HI, sprite0_start >> 8) |
+			   VLV_FIFO(SPRITEF_HI, sprite1_start >> 8));
+
+		I915_WRITE(DSPARB3, dsparb3);
+		I915_WRITE(DSPARB2, dsparb2);
+		break;
+	default:
+		break;
+	}
+}
+
+#undef VLV_FIFO
+
 static void vlv_merge_wm(struct drm_device *dev,
 			 struct vlv_wm_values *wm)
 {
@@ -1372,8 +1521,11 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	_vlv_compute_wm(intel_crtc);
 	vlv_merge_wm(dev, &wm);
 
-	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0)
+	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
+		/* FIXME should be part of crtc atomic commit */
+		vlv_pipe_set_fifo_size(intel_crtc);
 		return;
+	}
 
 	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
 	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
@@ -1388,6 +1540,9 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 		intel_wait_for_vblank(dev, pipe);
 	}
 
+	/* FIXME should be part of crtc atomic commit */
+	vlv_pipe_set_fifo_size(intel_crtc);
+
 	vlv_write_wm_values(intel_crtc, &wm);
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
-- 
2.3.6

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

Re: [PATCH 05/10] drm/i915: Compute display FIFO split dynamically for CHV

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Consider which planes are active and compute the FIFO split based on the
> relative data rates. Since we only consider the pipe src width rather
> than the plane width when computing watermarks it seems best to do the
> same when computing the FIFO split as well. This means the only thing we
> actually have to consider for the FIFO splut is the bpp, and we can
> ignore the rest.
>
> I've just stuffed the logic into the watermark code for now. Eventually
> it'll need to move into the atomic update for the crtc.
>
> There's also one extra complication I've not yet considered; Some of the
> DSPARB registers contain bits related to multiple pipes. The registers
> are double buffered but apparently they update on the vblank of any
> active pipe. So doing the FIFO reconfiguration properly when multiple
> pipes are active is not going to be fun. But let's ignore that mess for
> now.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h |  25 +++++-
>   drivers/gpu/drm/i915/intel_pm.c | 175 +++++++++++++++++++++++++++++++++++++---
>   2 files changed, 189 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b9f6b8c..fa6780f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4411,9 +4411,32 @@ enum skl_disp_power_wells {
>   #define   DSPARB_BSTART_SHIFT	0
>   #define   DSPARB_BEND_SHIFT	9 /* on 855 */
>   #define   DSPARB_AEND_SHIFT	0
> -
> +#define   DSPARB_SPRITEA_SHIFT_VLV	0
> +#define   DSPARB_SPRITEA_MASK_VLV	(0xff << 0)
> +#define   DSPARB_SPRITEB_SHIFT_VLV	8
> +#define   DSPARB_SPRITEB_MASK_VLV	(0xff << 8)
> +#define   DSPARB_SPRITEC_SHIFT_VLV	16
> +#define   DSPARB_SPRITEC_MASK_VLV	(0xff << 16)
> +#define   DSPARB_SPRITED_SHIFT_VLV	24
> +#define   DSPARB_SPRITED_MASK_VLV	(0xff << 24)
>   #define DSPARB2			(VLV_DISPLAY_BASE + 0x70060) /* vlv/chv */
> +#define   DSPARB_SPRITEA_HI_SHIFT_VLV	0
> +#define   DSPARB_SPRITEA_HI_MASK_VLV	(0x1 << 0)
> +#define   DSPARB_SPRITEB_HI_SHIFT_VLV	4
> +#define   DSPARB_SPRITEB_HI_MASK_VLV	(0x1 << 4)
> +#define   DSPARB_SPRITEC_HI_SHIFT_VLV	8
> +#define   DSPARB_SPRITEC_HI_MASK_VLV	(0x1 << 8)
> +#define   DSPARB_SPRITED_HI_SHIFT_VLV	12
> +#define   DSPARB_SPRITED_HI_MASK_VLV	(0x1 << 12)
> +#define   DSPARB_SPRITEE_HI_SHIFT_VLV	16
> +#define   DSPARB_SPRITEE_HI_MASK_VLV	(0x1 << 16)
> +#define   DSPARB_SPRITEF_HI_SHIFT_VLV	20
> +#define   DSPARB_SPRITEF_HI_MASK_VLV	(0x1 << 20)
>   #define DSPARB3			(VLV_DISPLAY_BASE + 0x7006c) /* chv */
> +#define   DSPARB_SPRITEE_SHIFT_VLV	0
> +#define   DSPARB_SPRITEE_MASK_VLV	(0xff << 0)
> +#define   DSPARB_SPRITEF_SHIFT_VLV	8
> +#define   DSPARB_SPRITEF_MASK_VLV	(0xff << 8)
>
>   /* pnv/gen4/g4x/vlv/chv */
>   #define DSPFW1			(dev_priv->info.display_mmio_offset + 0x70034)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d046e5f..ffdca62 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1171,6 +1171,73 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
>   	dev_priv->wm.vlv = wm;
>   }
>
> +static void vlv_compute_fifo(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct vlv_wm_state *wm_state = &crtc->wm_state;
> +	struct intel_plane *plane;
> +	unsigned int total_rate = 0;
> +	const int fifo_size = 512 - 1;
> +	int fifo_extra, fifo_left = fifo_size;
> +
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		struct intel_plane_state *state =
> +			to_intel_plane_state(plane->base.state);
> +
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			continue;
> +
> +		if (state->visible) {
> +			wm_state->num_active_planes++;
> +			total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> +		}
> +	}
> +
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		struct intel_plane_state *state =
> +			to_intel_plane_state(plane->base.state);
> +		unsigned int rate;
> +
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> +			plane->wm.fifo_size = 63;
> +			continue;
> +		}
> +
> +		if (!state->visible) {
> +			plane->wm.fifo_size = 0;
> +			continue;
> +		}
> +
> +		rate = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> +		plane->wm.fifo_size = fifo_size * rate / total_rate;
> +		fifo_left -= plane->wm.fifo_size;
> +	}
> +
> +	fifo_extra = DIV_ROUND_UP(fifo_left, wm_state->num_active_planes ?: 1);
> +
> +	/* spread the remainder evenly */
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		int plane_extra;
> +
> +		if (fifo_left == 0)
> +			break;
> +
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			continue;
> +
> +		/* give it all to the first plane if none are active */
> +		if (plane->wm.fifo_size == 0 &&
> +		    wm_state->num_active_planes)
> +			continue;
> +
> +		plane_extra = min(fifo_extra, fifo_left);
> +		plane->wm.fifo_size += plane_extra;
> +		fifo_left -= plane_extra;
> +	}
> +
> +	WARN_ON(fifo_left != 0);
> +}
> +
>   static void vlv_invert_wms(struct intel_crtc *crtc)
>   {
>   	struct vlv_wm_state *wm_state = &crtc->wm_state;
> @@ -1222,16 +1289,8 @@ static void _vlv_compute_wm(struct intel_crtc *crtc)
>   		wm_state->num_levels = VLV_WM_NUM_LEVELS;
>
>   	wm_state->num_active_planes = 0;
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -		struct intel_plane_state *state =
> -			to_intel_plane_state(plane->base.state);
> -
> -		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> -			continue;
>
> -		if (state->visible)
> -			wm_state->num_active_planes++;
> -	}
> +	vlv_compute_fifo(crtc);
>
>   	if (wm_state->num_active_planes != 1)
>   		wm_state->cxsr = false;
> @@ -1315,6 +1374,96 @@ static void _vlv_compute_wm(struct intel_crtc *crtc)
>   	vlv_invert_wms(crtc);
>   }
>
> +#define VLV_FIFO(plane, value) \
> +	(((value) << DSPARB_ ## plane ## _SHIFT_VLV) & DSPARB_ ## plane ## _MASK_VLV)
> +
> +static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_plane *plane;
> +	int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
> +
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> +			WARN_ON(plane->wm.fifo_size != 63);
> +			continue;
> +		}
> +
> +		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +			sprite0_start = plane->wm.fifo_size;
> +		else if (plane->plane == 0)
> +			sprite1_start = sprite0_start + plane->wm.fifo_size;
> +		else
> +			fifo_size = sprite1_start + plane->wm.fifo_size;
> +	}
> +
> +	WARN_ON(fifo_size != 512 - 1);
> +
> +	DRM_DEBUG_KMS("Pipe %c FIFO split %d / %d / %d\n",
> +		      pipe_name(crtc->pipe), sprite0_start,
> +		      sprite1_start, fifo_size);
> +
> +	switch (crtc->pipe) {
> +		uint32_t dsparb, dsparb2, dsparb3;
> +	case PIPE_A:
> +		dsparb = I915_READ(DSPARB);
> +		dsparb2 = I915_READ(DSPARB2);
> +
> +		dsparb &= ~(VLV_FIFO(SPRITEA, 0xff) |
> +			    VLV_FIFO(SPRITEB, 0xff));
> +		dsparb |= (VLV_FIFO(SPRITEA, sprite0_start) |
> +			   VLV_FIFO(SPRITEB, sprite1_start));
> +
> +		dsparb2 &= ~(VLV_FIFO(SPRITEA_HI, 0x1) |
> +			     VLV_FIFO(SPRITEB_HI, 0x1));
> +		dsparb2 |= (VLV_FIFO(SPRITEA_HI, sprite0_start >> 8) |
> +			   VLV_FIFO(SPRITEB_HI, sprite1_start >> 8));
> +
> +		I915_WRITE(DSPARB, dsparb);
> +		I915_WRITE(DSPARB2, dsparb2);
> +		break;
> +	case PIPE_B:
> +		dsparb = I915_READ(DSPARB);
> +		dsparb2 = I915_READ(DSPARB2);
> +
> +		dsparb &= ~(VLV_FIFO(SPRITEC, 0xff) |
> +			    VLV_FIFO(SPRITED, 0xff));
> +		dsparb |= (VLV_FIFO(SPRITEC, sprite0_start) |
> +			   VLV_FIFO(SPRITED, sprite1_start));
> +
> +		dsparb2 &= ~(VLV_FIFO(SPRITEC_HI, 0xff) |
> +			     VLV_FIFO(SPRITED_HI, 0xff));
> +		dsparb2 |= (VLV_FIFO(SPRITEC_HI, sprite0_start >> 8) |
> +			   VLV_FIFO(SPRITED_HI, sprite1_start >> 8));
> +
> +		I915_WRITE(DSPARB, dsparb);
> +		I915_WRITE(DSPARB2, dsparb2);
> +		break;
> +	case PIPE_C:
> +		dsparb3 = I915_READ(DSPARB3);
> +		dsparb2 = I915_READ(DSPARB2);
> +
> +		dsparb3 &= ~(VLV_FIFO(SPRITEE, 0xff) |
> +			     VLV_FIFO(SPRITEF, 0xff));
> +		dsparb3 |= (VLV_FIFO(SPRITEE, sprite0_start) |
> +			    VLV_FIFO(SPRITEF, sprite1_start));
> +
> +		dsparb2 &= ~(VLV_FIFO(SPRITEE_HI, 0xff) |
> +			     VLV_FIFO(SPRITEF_HI, 0xff));
> +		dsparb2 |= (VLV_FIFO(SPRITEE_HI, sprite0_start >> 8) |
> +			   VLV_FIFO(SPRITEF_HI, sprite1_start >> 8));
> +
> +		I915_WRITE(DSPARB3, dsparb3);
> +		I915_WRITE(DSPARB2, dsparb2);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +#undef VLV_FIFO
> +
>   static void vlv_merge_wm(struct drm_device *dev,
>   			 struct vlv_wm_values *wm)
>   {
> @@ -1372,8 +1521,11 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>   	_vlv_compute_wm(intel_crtc);
>   	vlv_merge_wm(dev, &wm);
>
> -	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0)
> +	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
> +		/* FIXME should be part of crtc atomic commit */
> +		vlv_pipe_set_fifo_size(intel_crtc);
>   		return;
> +	}
>
>   	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
>   	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
> @@ -1388,6 +1540,9 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>   		intel_wait_for_vblank(dev, pipe);
>   	}
>
> +	/* FIXME should be part of crtc atomic commit */
> +	vlv_pipe_set_fifo_size(intel_crtc);
> +
>   	vlv_write_wm_values(intel_crtc, &wm);
>
>   	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH 06/10] drm/i915: Use the memory latency based WM computation on VLV too

[ville.syrjala]

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

In order to get decnet memory self refresh residency on VLV, flip it
over to the new CHV way of doing things. VLV doesn't do PM5 or DDR DVFS
so it's a bit simpler.

I'm not sure the currently memory latency used for CHV is really
appropriate for VLV. Some further testing will probably be needed to
figure that out.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 223 +----------------------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   6 -
 3 files changed, 6 insertions(+), 225 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1424320..d67b5f1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15476,7 +15476,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		pll->on = false;
 	}
 
-	if (IS_CHERRYVIEW(dev))
+	if (IS_VALLEYVIEW(dev))
 		vlv_wm_get_hw_state(dev);
 	else if (IS_GEN9(dev))
 		skl_wm_get_hw_state(dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ffdca62..c7c90ce 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -931,77 +931,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 
 #undef FW_WM_VLV
 
-static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
-					 struct drm_plane *plane)
-{
-	struct drm_device *dev = crtc->dev;
-	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;
-
-	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
-
-	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)
-		drain_latency = DRAIN_LATENCY_MASK;
-
-	return drain_latency | (prec_mult == high_precision ?
-				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
-}
-
-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);
-}
-
 enum vlv_wm_level {
 	VLV_WM_LEVEL_PM2,
 	VLV_WM_LEVEL_PM5,
@@ -1076,101 +1005,6 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 	return min_t(int, wm, USHRT_MAX);
 }
 
-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);
-
-	/*
-	 * 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);
-
-	vlv_write_wm_values(intel_crtc, &wm);
-
-	if (cxsr_enabled)
-		intel_set_memory_cxsr(dev_priv, true);
-
-	dev_priv->wm.vlv = wm;
-}
-
 static void vlv_compute_fifo(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -1272,7 +1106,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 	}
 }
 
-static void _vlv_compute_wm(struct intel_crtc *crtc)
+static void vlv_compute_wm(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
@@ -1518,7 +1352,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	enum pipe pipe = intel_crtc->pipe;
 	struct vlv_wm_values wm = {};
 
-	_vlv_compute_wm(intel_crtc);
+	vlv_compute_wm(intel_crtc);
 	vlv_merge_wm(dev, &wm);
 
 	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
@@ -1567,54 +1401,6 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	dev_priv->wm.vlv = wm;
 }
 
-static void valleyview_update_sprite_wm(struct drm_plane *plane,
-					struct drm_crtc *crtc,
-					uint32_t sprite_width,
-					uint32_t sprite_height,
-					int pixel_size,
-					bool enabled, bool scaled)
-{
-	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;
-	int sprite = to_intel_plane(plane)->plane;
-	bool cxsr_enabled;
-	struct vlv_wm_values wm = dev_priv->wm.vlv;
-
-	if (enabled) {
-		wm.ddl[pipe].sprite[sprite] =
-			vlv_compute_drain_latency(crtc, plane);
-
-		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)
@@ -7289,8 +7075,9 @@ void intel_init_pm(struct drm_device *dev)
 		dev_priv->display.init_clock_gating =
 			cherryview_init_clock_gating;
 	} else if (IS_VALLEYVIEW(dev)) {
-		dev_priv->display.update_wm = valleyview_update_wm;
-		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
+		vlv_setup_wm_latency(dev);
+
+		dev_priv->display.update_wm = vlv_update_wm;
 		dev_priv->display.init_clock_gating =
 			valleyview_init_clock_gating;
 	} else if (IS_PINEVIEW(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 3806f83..d23269b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -402,10 +402,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	if (obj->tiling_mode != I915_TILING_NONE)
 		sprctl |= SP_TILED;
 
-	intel_update_sprite_watermarks(dplane, crtc, src_w, src_h,
-				       pixel_size, true,
-				       src_w != crtc_w || src_h != crtc_h);
-
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -470,8 +466,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
-
-	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
 }
 
 static void
-- 
2.3.6

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

Re: [PATCH 06/10] drm/i915: Use the memory latency based WM computation on VLV too

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In order to get decnet memory self refresh residency on VLV, flip it
> over to the new CHV way of doing things. VLV doesn't do PM5 or DDR DVFS
> so it's a bit simpler.
>
> I'm not sure the currently memory latency used for CHV is really
> appropriate for VLV. Some further testing will probably be needed to
> figure that out.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c |   2 +-
>   drivers/gpu/drm/i915/intel_pm.c      | 223 +----------------------------------
>   drivers/gpu/drm/i915/intel_sprite.c  |   6 -
>   3 files changed, 6 insertions(+), 225 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1424320..d67b5f1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15476,7 +15476,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>   		pll->on = false;
>   	}
>
> -	if (IS_CHERRYVIEW(dev))
> +	if (IS_VALLEYVIEW(dev))
>   		vlv_wm_get_hw_state(dev);
>   	else if (IS_GEN9(dev))
>   		skl_wm_get_hw_state(dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ffdca62..c7c90ce 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -931,77 +931,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>
>   #undef FW_WM_VLV
>
> -static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
> -					 struct drm_plane *plane)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	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;
> -
> -	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> -
> -	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)
> -		drain_latency = DRAIN_LATENCY_MASK;
> -
> -	return drain_latency | (prec_mult == high_precision ?
> -				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
> -}
> -
> -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);
> -}
> -
>   enum vlv_wm_level {
>   	VLV_WM_LEVEL_PM2,
>   	VLV_WM_LEVEL_PM5,
> @@ -1076,101 +1005,6 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>   	return min_t(int, wm, USHRT_MAX);
>   }
>
> -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);
> -
> -	/*
> -	 * 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);
> -
> -	vlv_write_wm_values(intel_crtc, &wm);
> -
> -	if (cxsr_enabled)
> -		intel_set_memory_cxsr(dev_priv, true);
> -
> -	dev_priv->wm.vlv = wm;
> -}
> -
>   static void vlv_compute_fifo(struct intel_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->base.dev;
> @@ -1272,7 +1106,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>   	}
>   }
>
> -static void _vlv_compute_wm(struct intel_crtc *crtc)
> +static void vlv_compute_wm(struct intel_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->base.dev;
>   	struct vlv_wm_state *wm_state = &crtc->wm_state;
> @@ -1518,7 +1352,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>   	enum pipe pipe = intel_crtc->pipe;
>   	struct vlv_wm_values wm = {};
>
> -	_vlv_compute_wm(intel_crtc);
> +	vlv_compute_wm(intel_crtc);
>   	vlv_merge_wm(dev, &wm);
>
>   	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
> @@ -1567,54 +1401,6 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>   	dev_priv->wm.vlv = wm;
>   }
>
> -static void valleyview_update_sprite_wm(struct drm_plane *plane,
> -					struct drm_crtc *crtc,
> -					uint32_t sprite_width,
> -					uint32_t sprite_height,
> -					int pixel_size,
> -					bool enabled, bool scaled)
> -{
> -	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;
> -	int sprite = to_intel_plane(plane)->plane;
> -	bool cxsr_enabled;
> -	struct vlv_wm_values wm = dev_priv->wm.vlv;
> -
> -	if (enabled) {
> -		wm.ddl[pipe].sprite[sprite] =
> -			vlv_compute_drain_latency(crtc, plane);
> -
> -		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)
> @@ -7289,8 +7075,9 @@ void intel_init_pm(struct drm_device *dev)
>   		dev_priv->display.init_clock_gating =
>   			cherryview_init_clock_gating;
>   	} else if (IS_VALLEYVIEW(dev)) {
> -		dev_priv->display.update_wm = valleyview_update_wm;
> -		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> +		vlv_setup_wm_latency(dev);
> +
> +		dev_priv->display.update_wm = vlv_update_wm;
>   		dev_priv->display.init_clock_gating =
>   			valleyview_init_clock_gating;
>   	} else if (IS_PINEVIEW(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 3806f83..d23269b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -402,10 +402,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>   	if (obj->tiling_mode != I915_TILING_NONE)
>   		sprctl |= SP_TILED;
>
> -	intel_update_sprite_watermarks(dplane, crtc, src_w, src_h,
> -				       pixel_size, true,
> -				       src_w != crtc_w || src_h != crtc_h);
> -
>   	/* Sizes are 0 based */
>   	src_w--;
>   	src_h--;
> @@ -470,8 +466,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>
>   	I915_WRITE(SPSURF(pipe, plane), 0);
>   	POSTING_READ(SPSURF(pipe, plane));
> -
> -	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
>   }
>
>   static void
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable

[ville.syrjala]

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

CxSR (or maxfifo on VLV/CHV) blocks somne changes to the plane control
register (enable bit at least, not quite sure about the rest). So in
order to have the plane enable/disable when we want we need to first
kick the hardware out of cxsr.

Unfortunateloy this requires some extra vblank waits. For the CxSR
enable after the plane update we should eventually use an async
vblank worker, but since we don't have that just do sync vblank
waits. For the disable case we have no choice but to do it
synchronously.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 drivers/gpu/drm/i915/intel_pm.c      | 11 ++++-------
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d67b5f1..19aedf9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4716,6 +4716,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
+	if (atomic->disable_cxsr)
+		crtc->wm.cxsr_allowed = true;
+
 	if (crtc->atomic.update_wm_post)
 		intel_update_watermarks(&crtc->base);
 
@@ -4765,6 +4768,11 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
+
+	if (atomic->disable_cxsr) {
+		crtc->wm.cxsr_allowed = false;
+		intel_set_memory_cxsr(dev_priv, false);
+	}
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -11646,12 +11654,26 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on)
+	if (turn_on) {
 		intel_crtc->atomic.update_wm_pre = true;
-	else if (turn_off)
+		/* must disable cxsr around plane enable/disable */
+		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+			intel_crtc->atomic.disable_cxsr = true;
+			/* to potentially re-enable cxsr */
+			intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.update_wm_post = true;
+		}
+	} else if (turn_off) {
 		intel_crtc->atomic.update_wm_post = true;
-	else if (intel_wm_need_update(plane, plane_state))
+		/* must disable cxsr around plane enable/disable */
+		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+			if (is_crtc_enabled)
+				intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.disable_cxsr = true;
+		}
+	} else if (intel_wm_need_update(plane, plane_state)) {
 		intel_crtc->atomic.update_wm_pre = true;
+	}
 
 	if (visible)
 		intel_crtc->atomic.fb_bits |=
@@ -11808,8 +11830,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
 		intel_crtc_check_initial_planes(crtc, crtc_state);
 
-	if (mode_changed)
-		intel_crtc->atomic.update_wm_post = !crtc_state->active;
+	if (mode_changed && !crtc_state->active)
+		intel_crtc->atomic.update_wm_post = true;
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
@@ -13129,6 +13151,8 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 		if (!needs_modeset(crtc->state))
 			continue;
 
+		intel_pre_plane_update(intel_crtc);
+
 		any_ms = true;
 		intel_pre_plane_update(intel_crtc);
 
@@ -14089,6 +14113,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_cntl = ~0;
 	intel_crtc->cursor_size = ~0;
 
+	intel_crtc->wm.cxsr_allowed = true;
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f26a680..4e8d13e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -507,6 +507,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
+	bool disable_cxsr;
 	bool pre_disable_primary;
 	bool update_wm_pre, update_wm_post;
 	unsigned disabled_planes;
@@ -565,6 +566,8 @@ struct intel_crtc {
 		struct intel_pipe_wm active;
 		/* SKL wm values currently in use */
 		struct skl_pipe_wm skl_active;
+		/* allow CxSR on this pipe */
+		bool cxsr_allowed;
 	} wm;
 
 	int scanline_offset;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c7c90ce..b65817d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -335,6 +335,7 @@ 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);
 		POSTING_READ(FW_BLC_SELF_VLV);
+		dev_priv->wm.vlv.cxsr = enable;
 	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
 		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
 		POSTING_READ(FW_BLC_SELF);
@@ -1116,7 +1117,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 
 	memset(wm_state, 0, sizeof(*wm_state));
 
-	wm_state->cxsr = crtc->pipe != PIPE_C;
+	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
 	if (IS_CHERRYVIEW(dev))
 		wm_state->num_levels = CHV_WM_NUM_LEVELS;
 	else
@@ -1369,10 +1370,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
 		chv_set_memory_pm5(dev_priv, false);
 
-	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
+	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
 		intel_set_memory_cxsr(dev_priv, false);
-		intel_wait_for_vblank(dev, pipe);
-	}
 
 	/* FIXME should be part of crtc atomic commit */
 	vlv_pipe_set_fifo_size(intel_crtc);
@@ -1385,10 +1384,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
 		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
 
-	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
-		intel_wait_for_vblank(dev, pipe);
+	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
 		intel_set_memory_cxsr(dev_priv, true);
-	}
 
 	if (wm.level >= VLV_WM_LEVEL_PM5 &&
 	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
-- 
2.3.6

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

Re: [PATCH 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> CxSR (or maxfifo on VLV/CHV) blocks somne changes to the plane control
> register (enable bit at least, not quite sure about the rest). So in
> order to have the plane enable/disable when we want we need to first
> kick the hardware out of cxsr.
>
> Unfortunateloy this requires some extra vblank waits. For the CxSR
> enable after the plane update we should eventually use an async
> vblank worker, but since we don't have that just do sync vblank
> waits. For the disable case we have no choice but to do it
> synchronously.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++-----
>   drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>   drivers/gpu/drm/i915/intel_pm.c      | 11 ++++-------
>   3 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d67b5f1..19aedf9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4716,6 +4716,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>
>   	intel_frontbuffer_flip(dev, atomic->fb_bits);
>
> +	if (atomic->disable_cxsr)
> +		crtc->wm.cxsr_allowed = true;
> +
>   	if (crtc->atomic.update_wm_post)
>   		intel_update_watermarks(&crtc->base);
>
> @@ -4765,6 +4768,11 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>
>   	if (atomic->pre_disable_primary)
>   		intel_pre_disable_primary(&crtc->base);
> +
> +	if (atomic->disable_cxsr) {
> +		crtc->wm.cxsr_allowed = false;
> +		intel_set_memory_cxsr(dev_priv, false);
> +	}
>   }
>
>   static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> @@ -11646,12 +11654,26 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>   			 plane->base.id, was_visible, visible,
>   			 turn_off, turn_on, mode_changed);
>
> -	if (turn_on)
> +	if (turn_on) {
>   		intel_crtc->atomic.update_wm_pre = true;
> -	else if (turn_off)
> +		/* must disable cxsr around plane enable/disable */
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			intel_crtc->atomic.disable_cxsr = true;
> +			/* to potentially re-enable cxsr */
> +			intel_crtc->atomic.wait_vblank = true;
> +			intel_crtc->atomic.update_wm_post = true;
> +		}
> +	} else if (turn_off) {
>   		intel_crtc->atomic.update_wm_post = true;
> -	else if (intel_wm_need_update(plane, plane_state))
> +		/* must disable cxsr around plane enable/disable */
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			if (is_crtc_enabled)
> +				intel_crtc->atomic.wait_vblank = true;
> +			intel_crtc->atomic.disable_cxsr = true;
> +		}
> +	} else if (intel_wm_need_update(plane, plane_state)) {
>   		intel_crtc->atomic.update_wm_pre = true;
> +	}
>
>   	if (visible)
>   		intel_crtc->atomic.fb_bits |=
> @@ -11808,8 +11830,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>   	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
>   		intel_crtc_check_initial_planes(crtc, crtc_state);
>
> -	if (mode_changed)
> -		intel_crtc->atomic.update_wm_post = !crtc_state->active;
> +	if (mode_changed && !crtc_state->active)
> +		intel_crtc->atomic.update_wm_post = true;
>
>   	if (mode_changed && crtc_state->enable &&
>   	    dev_priv->display.crtc_compute_clock &&
> @@ -13129,6 +13151,8 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>   		if (!needs_modeset(crtc->state))
>   			continue;
>
> +		intel_pre_plane_update(intel_crtc);
> +
>   		any_ms = true;
>   		intel_pre_plane_update(intel_crtc);
>
> @@ -14089,6 +14113,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>   	intel_crtc->cursor_cntl = ~0;
>   	intel_crtc->cursor_size = ~0;
>
> +	intel_crtc->wm.cxsr_allowed = true;
> +
>   	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>   	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>   	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f26a680..4e8d13e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -507,6 +507,7 @@ struct intel_crtc_atomic_commit {
>   	/* Sleepable operations to perform before commit */
>   	bool wait_for_flips;
>   	bool disable_fbc;
> +	bool disable_cxsr;
>   	bool pre_disable_primary;
>   	bool update_wm_pre, update_wm_post;
>   	unsigned disabled_planes;
> @@ -565,6 +566,8 @@ struct intel_crtc {
>   		struct intel_pipe_wm active;
>   		/* SKL wm values currently in use */
>   		struct skl_pipe_wm skl_active;
> +		/* allow CxSR on this pipe */
> +		bool cxsr_allowed;
>   	} wm;
>
>   	int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c7c90ce..b65817d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -335,6 +335,7 @@ 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);
>   		POSTING_READ(FW_BLC_SELF_VLV);
> +		dev_priv->wm.vlv.cxsr = enable;
>   	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
>   		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
>   		POSTING_READ(FW_BLC_SELF);
> @@ -1116,7 +1117,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>
>   	memset(wm_state, 0, sizeof(*wm_state));
>
> -	wm_state->cxsr = crtc->pipe != PIPE_C;
> +	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
>   	if (IS_CHERRYVIEW(dev))
>   		wm_state->num_levels = CHV_WM_NUM_LEVELS;
>   	else
> @@ -1369,10 +1370,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>   	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
>   		chv_set_memory_pm5(dev_priv, false);
>
> -	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
> +	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
>   		intel_set_memory_cxsr(dev_priv, false);
> -		intel_wait_for_vblank(dev, pipe);
> -	}
>
>   	/* FIXME should be part of crtc atomic commit */
>   	vlv_pipe_set_fifo_size(intel_crtc);
> @@ -1385,10 +1384,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>   		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
>   		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
>
> -	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
> -		intel_wait_for_vblank(dev, pipe);
> +	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
>   		intel_set_memory_cxsr(dev_priv, true);
> -	}
>
>   	if (wm.level >= VLV_WM_LEVEL_PM5 &&
>   	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH v2 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable

[ville.syrjala]

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

CxSR (or maxfifo on VLV/CHV) blocks somne changes to the plane control
register (enable bit at least, not quite sure about the rest). So in
order to have the plane enable/disable when we want we need to first
kick the hardware out of cxsr.

Unfortunateloy this requires some extra vblank waits. For the CxSR
enable after the plane update we should eventually use an async
vblank worker, but since we don't have that just do sync vblank
waits. For the disable case we have no choice but to do it
synchronously.

v2: Don't add a spurious intel_pre_plane_update() to crtc disable

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
Paulo noticed some frontbuffer_bits WARNs from this patch, and sure enough
I accidentally added another intel_pre_plane_update() to the crtc disable loop.
I failed to notice because I had commented out the frontbuffer_bits WARNs earlier
from my tree since they were too noisy.

 drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 drivers/gpu/drm/i915/intel_pm.c      | 11 ++++-------
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d67b5f1..defc4ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4716,6 +4716,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
+	if (atomic->disable_cxsr)
+		crtc->wm.cxsr_allowed = true;
+
 	if (crtc->atomic.update_wm_post)
 		intel_update_watermarks(&crtc->base);
 
@@ -4765,6 +4768,11 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
+
+	if (atomic->disable_cxsr) {
+		crtc->wm.cxsr_allowed = false;
+		intel_set_memory_cxsr(dev_priv, false);
+	}
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -11646,12 +11654,26 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on)
+	if (turn_on) {
 		intel_crtc->atomic.update_wm_pre = true;
-	else if (turn_off)
+		/* must disable cxsr around plane enable/disable */
+		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+			intel_crtc->atomic.disable_cxsr = true;
+			/* to potentially re-enable cxsr */
+			intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.update_wm_post = true;
+		}
+	} else if (turn_off) {
 		intel_crtc->atomic.update_wm_post = true;
-	else if (intel_wm_need_update(plane, plane_state))
+		/* must disable cxsr around plane enable/disable */
+		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+			if (is_crtc_enabled)
+				intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.disable_cxsr = true;
+		}
+	} else if (intel_wm_need_update(plane, plane_state)) {
 		intel_crtc->atomic.update_wm_pre = true;
+	}
 
 	if (visible)
 		intel_crtc->atomic.fb_bits |=
@@ -11808,8 +11830,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
 		intel_crtc_check_initial_planes(crtc, crtc_state);
 
-	if (mode_changed)
-		intel_crtc->atomic.update_wm_post = !crtc_state->active;
+	if (mode_changed && !crtc_state->active)
+		intel_crtc->atomic.update_wm_post = true;
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
@@ -14089,6 +14111,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_cntl = ~0;
 	intel_crtc->cursor_size = ~0;
 
+	intel_crtc->wm.cxsr_allowed = true;
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f26a680..4e8d13e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -507,6 +507,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
+	bool disable_cxsr;
 	bool pre_disable_primary;
 	bool update_wm_pre, update_wm_post;
 	unsigned disabled_planes;
@@ -565,6 +566,8 @@ struct intel_crtc {
 		struct intel_pipe_wm active;
 		/* SKL wm values currently in use */
 		struct skl_pipe_wm skl_active;
+		/* allow CxSR on this pipe */
+		bool cxsr_allowed;
 	} wm;
 
 	int scanline_offset;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c7c90ce..b65817d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -335,6 +335,7 @@ 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);
 		POSTING_READ(FW_BLC_SELF_VLV);
+		dev_priv->wm.vlv.cxsr = enable;
 	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
 		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
 		POSTING_READ(FW_BLC_SELF);
@@ -1116,7 +1117,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 
 	memset(wm_state, 0, sizeof(*wm_state));
 
-	wm_state->cxsr = crtc->pipe != PIPE_C;
+	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
 	if (IS_CHERRYVIEW(dev))
 		wm_state->num_levels = CHV_WM_NUM_LEVELS;
 	else
@@ -1369,10 +1370,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
 		chv_set_memory_pm5(dev_priv, false);
 
-	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
+	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
 		intel_set_memory_cxsr(dev_priv, false);
-		intel_wait_for_vblank(dev, pipe);
-	}
 
 	/* FIXME should be part of crtc atomic commit */
 	vlv_pipe_set_fifo_size(intel_crtc);
@@ -1385,10 +1384,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
 		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
 
-	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
-		intel_wait_for_vblank(dev, pipe);
+	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
 		intel_set_memory_cxsr(dev_priv, true);
-	}
 
 	if (wm.level >= VLV_WM_LEVEL_PM5 &&
 	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
-- 
2.3.6

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

Re: [PATCH v2 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable

[Paulo Zanoni]

2015-07-01 16:13 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> CxSR (or maxfifo on VLV/CHV) blocks somne changes to the plane control
> register (enable bit at least, not quite sure about the rest). So in
> order to have the plane enable/disable when we want we need to first
> kick the hardware out of cxsr.
>
> Unfortunateloy this requires some extra vblank waits. For the CxSR
> enable after the plane update we should eventually use an async
> vblank worker, but since we don't have that just do sync vblank
> waits. For the disable case we have no choice but to do it
> synchronously.
>
> v2: Don't add a spurious intel_pre_plane_update() to crtc disable
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> Paulo noticed some frontbuffer_bits WARNs from this patch, and sure enough
> I accidentally added another intel_pre_plane_update() to the crtc disable loop.
> I failed to notice because I had commented out the frontbuffer_bits WARNs earlier
> from my tree since they were too noisy.

Ok, so I didn't test this exact v2 patch since I'm on -nightly and v1
is already applied there. But I removed the extra
intel_pre_plane_update() added by v1 of the patch, booted, and I can
confirm the WARNs are gone. The test I was using to reproduce the
WARNs was:

sudo ./kms_frontbuffer_tracking --run-subtest fbc-1p-rte

Kinda-tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for the quick response and fix!

>
>  drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c      | 11 ++++-------
>  3 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d67b5f1..defc4ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4716,6 +4716,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>
>         intel_frontbuffer_flip(dev, atomic->fb_bits);
>
> +       if (atomic->disable_cxsr)
> +               crtc->wm.cxsr_allowed = true;
> +
>         if (crtc->atomic.update_wm_post)
>                 intel_update_watermarks(&crtc->base);
>
> @@ -4765,6 +4768,11 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>
>         if (atomic->pre_disable_primary)
>                 intel_pre_disable_primary(&crtc->base);
> +
> +       if (atomic->disable_cxsr) {
> +               crtc->wm.cxsr_allowed = false;
> +               intel_set_memory_cxsr(dev_priv, false);
> +       }
>  }
>
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> @@ -11646,12 +11654,26 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>                          plane->base.id, was_visible, visible,
>                          turn_off, turn_on, mode_changed);
>
> -       if (turn_on)
> +       if (turn_on) {
>                 intel_crtc->atomic.update_wm_pre = true;
> -       else if (turn_off)
> +               /* must disable cxsr around plane enable/disable */
> +               if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +                       intel_crtc->atomic.disable_cxsr = true;
> +                       /* to potentially re-enable cxsr */
> +                       intel_crtc->atomic.wait_vblank = true;
> +                       intel_crtc->atomic.update_wm_post = true;
> +               }
> +       } else if (turn_off) {
>                 intel_crtc->atomic.update_wm_post = true;
> -       else if (intel_wm_need_update(plane, plane_state))
> +               /* must disable cxsr around plane enable/disable */
> +               if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +                       if (is_crtc_enabled)
> +                               intel_crtc->atomic.wait_vblank = true;
> +                       intel_crtc->atomic.disable_cxsr = true;
> +               }
> +       } else if (intel_wm_need_update(plane, plane_state)) {
>                 intel_crtc->atomic.update_wm_pre = true;
> +       }
>
>         if (visible)
>                 intel_crtc->atomic.fb_bits |=
> @@ -11808,8 +11830,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>         if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
>                 intel_crtc_check_initial_planes(crtc, crtc_state);
>
> -       if (mode_changed)
> -               intel_crtc->atomic.update_wm_post = !crtc_state->active;
> +       if (mode_changed && !crtc_state->active)
> +               intel_crtc->atomic.update_wm_post = true;
>
>         if (mode_changed && crtc_state->enable &&
>             dev_priv->display.crtc_compute_clock &&
> @@ -14089,6 +14111,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>         intel_crtc->cursor_cntl = ~0;
>         intel_crtc->cursor_size = ~0;
>
> +       intel_crtc->wm.cxsr_allowed = true;
> +
>         BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>                dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>         dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f26a680..4e8d13e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -507,6 +507,7 @@ struct intel_crtc_atomic_commit {
>         /* Sleepable operations to perform before commit */
>         bool wait_for_flips;
>         bool disable_fbc;
> +       bool disable_cxsr;
>         bool pre_disable_primary;
>         bool update_wm_pre, update_wm_post;
>         unsigned disabled_planes;
> @@ -565,6 +566,8 @@ struct intel_crtc {
>                 struct intel_pipe_wm active;
>                 /* SKL wm values currently in use */
>                 struct skl_pipe_wm skl_active;
> +               /* allow CxSR on this pipe */
> +               bool cxsr_allowed;
>         } wm;
>
>         int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c7c90ce..b65817d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -335,6 +335,7 @@ 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);
>                 POSTING_READ(FW_BLC_SELF_VLV);
> +               dev_priv->wm.vlv.cxsr = enable;
>         } else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
>                 I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
>                 POSTING_READ(FW_BLC_SELF);
> @@ -1116,7 +1117,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>
>         memset(wm_state, 0, sizeof(*wm_state));
>
> -       wm_state->cxsr = crtc->pipe != PIPE_C;
> +       wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
>         if (IS_CHERRYVIEW(dev))
>                 wm_state->num_levels = CHV_WM_NUM_LEVELS;
>         else
> @@ -1369,10 +1370,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>             dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
>                 chv_set_memory_pm5(dev_priv, false);
>
> -       if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
> +       if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
>                 intel_set_memory_cxsr(dev_priv, false);
> -               intel_wait_for_vblank(dev, pipe);
> -       }
>
>         /* FIXME should be part of crtc atomic commit */
>         vlv_pipe_set_fifo_size(intel_crtc);
> @@ -1385,10 +1384,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>                       wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
>                       wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
>
> -       if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
> -               intel_wait_for_vblank(dev, pipe);
> +       if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
>                 intel_set_memory_cxsr(dev_priv, true);
> -       }
>
>         if (wm.level >= VLV_WM_LEVEL_PM5 &&
>             dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
> --
> 2.3.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH v2 07/10] drm/i915: Try to make sure cxsr is disabled around plane enable/disable

[Matt Roper]

On Wed, Jul 01, 2015 at 10:13:38PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CxSR (or maxfifo on VLV/CHV) blocks somne changes to the plane control
> register (enable bit at least, not quite sure about the rest). So in
> order to have the plane enable/disable when we want we need to first
> kick the hardware out of cxsr.
> 
> Unfortunateloy this requires some extra vblank waits. For the CxSR
> enable after the plane update we should eventually use an async
> vblank worker, but since we don't have that just do sync vblank
> waits. For the disable case we have no choice but to do it
> synchronously.
> 
> v2: Don't add a spurious intel_pre_plane_update() to crtc disable
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> Paulo noticed some frontbuffer_bits WARNs from this patch, and sure enough
> I accidentally added another intel_pre_plane_update() to the crtc disable loop.
> I failed to notice because I had commented out the frontbuffer_bits WARNs earlier
> from my tree since they were too noisy.

I can confirm that your v2 fixes the warning spam caused by v1.

Regarding the v2 fix:
Tested-by: Matt Roper <matthew.d.roper@intel.com>

> 
>  drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c      | 11 ++++-------
>  3 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d67b5f1..defc4ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4716,6 +4716,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
> +	if (atomic->disable_cxsr)
> +		crtc->wm.cxsr_allowed = true;
> +
>  	if (crtc->atomic.update_wm_post)
>  		intel_update_watermarks(&crtc->base);
>  
> @@ -4765,6 +4768,11 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  
>  	if (atomic->pre_disable_primary)
>  		intel_pre_disable_primary(&crtc->base);
> +
> +	if (atomic->disable_cxsr) {
> +		crtc->wm.cxsr_allowed = false;
> +		intel_set_memory_cxsr(dev_priv, false);
> +	}
>  }
>  
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> @@ -11646,12 +11654,26 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			 plane->base.id, was_visible, visible,
>  			 turn_off, turn_on, mode_changed);
>  
> -	if (turn_on)
> +	if (turn_on) {
>  		intel_crtc->atomic.update_wm_pre = true;
> -	else if (turn_off)
> +		/* must disable cxsr around plane enable/disable */
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			intel_crtc->atomic.disable_cxsr = true;
> +			/* to potentially re-enable cxsr */
> +			intel_crtc->atomic.wait_vblank = true;
> +			intel_crtc->atomic.update_wm_post = true;
> +		}
> +	} else if (turn_off) {
>  		intel_crtc->atomic.update_wm_post = true;
> -	else if (intel_wm_need_update(plane, plane_state))
> +		/* must disable cxsr around plane enable/disable */
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			if (is_crtc_enabled)
> +				intel_crtc->atomic.wait_vblank = true;
> +			intel_crtc->atomic.disable_cxsr = true;
> +		}
> +	} else if (intel_wm_need_update(plane, plane_state)) {
>  		intel_crtc->atomic.update_wm_pre = true;
> +	}
>  
>  	if (visible)
>  		intel_crtc->atomic.fb_bits |=
> @@ -11808,8 +11830,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
>  		intel_crtc_check_initial_planes(crtc, crtc_state);
>  
> -	if (mode_changed)
> -		intel_crtc->atomic.update_wm_post = !crtc_state->active;
> +	if (mode_changed && !crtc_state->active)
> +		intel_crtc->atomic.update_wm_post = true;
>  
>  	if (mode_changed && crtc_state->enable &&
>  	    dev_priv->display.crtc_compute_clock &&
> @@ -14089,6 +14111,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->cursor_cntl = ~0;
>  	intel_crtc->cursor_size = ~0;
>  
> +	intel_crtc->wm.cxsr_allowed = true;
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f26a680..4e8d13e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -507,6 +507,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool wait_for_flips;
>  	bool disable_fbc;
> +	bool disable_cxsr;
>  	bool pre_disable_primary;
>  	bool update_wm_pre, update_wm_post;
>  	unsigned disabled_planes;
> @@ -565,6 +566,8 @@ struct intel_crtc {
>  		struct intel_pipe_wm active;
>  		/* SKL wm values currently in use */
>  		struct skl_pipe_wm skl_active;
> +		/* allow CxSR on this pipe */
> +		bool cxsr_allowed;
>  	} wm;
>  
>  	int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c7c90ce..b65817d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -335,6 +335,7 @@ 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);
>  		POSTING_READ(FW_BLC_SELF_VLV);
> +		dev_priv->wm.vlv.cxsr = enable;
>  	} else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
>  		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
>  		POSTING_READ(FW_BLC_SELF);
> @@ -1116,7 +1117,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  
>  	memset(wm_state, 0, sizeof(*wm_state));
>  
> -	wm_state->cxsr = crtc->pipe != PIPE_C;
> +	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
>  	if (IS_CHERRYVIEW(dev))
>  		wm_state->num_levels = CHV_WM_NUM_LEVELS;
>  	else
> @@ -1369,10 +1370,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
>  		chv_set_memory_pm5(dev_priv, false);
>  
> -	if (!wm.cxsr && dev_priv->wm.vlv.cxsr) {
> +	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
>  		intel_set_memory_cxsr(dev_priv, false);
> -		intel_wait_for_vblank(dev, pipe);
> -	}
>  
>  	/* FIXME should be part of crtc atomic commit */
>  	vlv_pipe_set_fifo_size(intel_crtc);
> @@ -1385,10 +1384,8 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
>  		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
>  
> -	if (wm.cxsr && !dev_priv->wm.vlv.cxsr) {
> -		intel_wait_for_vblank(dev, pipe);
> +	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
>  		intel_set_memory_cxsr(dev_priv, true);
> -	}
>  
>  	if (wm.level >= VLV_WM_LEVEL_PM5 &&
>  	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
> -- 
> 2.3.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm/i915: Don't do PM5/DDR DVFS with multiple pipes

[ville.syrjala]

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

Enabling PM5/DDR DVFS with multiple active pipes isn't a validated
configuration. It does seem to work most of the time at least, but
there is clearly an additional risk of underruns, so let's not play
with fire.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b65817d..c8e7ef3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1327,6 +1327,9 @@ static void vlv_merge_wm(struct drm_device *dev,
 	if (num_active_crtcs != 1)
 		wm->cxsr = false;
 
+	if (num_active_crtcs > 1)
+		wm->level = VLV_WM_LEVEL_PM2;
+
 	for_each_intel_crtc(dev, crtc) {
 		struct vlv_wm_state *wm_state = &crtc->wm_state;
 		enum pipe pipe = crtc->pipe;
-- 
2.3.6

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

Re: [PATCH 08/10] drm/i915: Don't do PM5/DDR DVFS with multiple pipes

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Enabling PM5/DDR DVFS with multiple active pipes isn't a validated
> configuration. It does seem to work most of the time at least, but
> there is clearly an additional risk of underruns, so let's not play
> with fire.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b65817d..c8e7ef3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1327,6 +1327,9 @@ static void vlv_merge_wm(struct drm_device *dev,
>   	if (num_active_crtcs != 1)
>   		wm->cxsr = false;
>
> +	if (num_active_crtcs > 1)
> +		wm->level = VLV_WM_LEVEL_PM2;
> +
>   	for_each_intel_crtc(dev, crtc) {
>   		struct vlv_wm_state *wm_state = &crtc->wm_state;
>   		enum pipe pipe = crtc->pipe;
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH 09/10] drm/i915: Add debugfs knobs for VLVCHV memory latency values

[ville.syrjala]

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

Allow tweaking the VLV/CHV memory latencies thorugh sysfs, like we do
for ILK+.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c49fe2a..656bb0d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4180,8 +4180,15 @@ static const struct file_operations i915_displayport_test_type_fops = {
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
-	int num_levels = ilk_wm_max_level(dev) + 1;
 	int level;
+	int num_levels;
+
+	if (IS_CHERRYVIEW(dev))
+		num_levels = 3;
+	else if (IS_VALLEYVIEW(dev))
+		num_levels = 1;
+	else
+		num_levels = ilk_wm_max_level(dev) + 1;
 
 	drm_modeset_lock_all(dev);
 
@@ -4190,9 +4197,9 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 
 		/*
 		 * - WM1+ latency values in 0.5us units
-		 * - latencies are in us on gen9
+		 * - latencies are in us on gen9/vlv/chv
 		 */
-		if (INTEL_INFO(dev)->gen >= 9)
+		if (INTEL_INFO(dev)->gen >= 9 || IS_VALLEYVIEW(dev))
 			latency *= 10;
 		else if (level > 0)
 			latency *= 5;
@@ -4256,7 +4263,7 @@ static int pri_wm_latency_open(struct inode *inode, struct file *file)
 {
 	struct drm_device *dev = inode->i_private;
 
-	if (HAS_GMCH_DISPLAY(dev))
+	if (INTEL_INFO(dev)->gen < 5)
 		return -ENODEV;
 
 	return single_open(file, pri_wm_latency_show, dev);
@@ -4288,11 +4295,18 @@ static ssize_t wm_latency_write(struct file *file, const char __user *ubuf,
 	struct seq_file *m = file->private_data;
 	struct drm_device *dev = m->private;
 	uint16_t new[8] = { 0 };
-	int num_levels = ilk_wm_max_level(dev) + 1;
+	int num_levels;
 	int level;
 	int ret;
 	char tmp[32];
 
+	if (IS_CHERRYVIEW(dev))
+		num_levels = 3;
+	else if (IS_VALLEYVIEW(dev))
+		num_levels = 1;
+	else
+		num_levels = ilk_wm_max_level(dev) + 1;
+
 	if (len >= sizeof(tmp))
 		return -EINVAL;
 
-- 
2.3.6

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

Re: [PATCH 09/10] drm/i915: Add debugfs knobs for VLVCHV memory latency values

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Allow tweaking the VLV/CHV memory latencies thorugh sysfs, like we do
> for ILK+.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c49fe2a..656bb0d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4180,8 +4180,15 @@ static const struct file_operations i915_displayport_test_type_fops = {
>   static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
>   {
>   	struct drm_device *dev = m->private;
> -	int num_levels = ilk_wm_max_level(dev) + 1;
>   	int level;
> +	int num_levels;
> +
> +	if (IS_CHERRYVIEW(dev))
> +		num_levels = 3;
> +	else if (IS_VALLEYVIEW(dev))
> +		num_levels = 1;
> +	else
> +		num_levels = ilk_wm_max_level(dev) + 1;
>
>   	drm_modeset_lock_all(dev);
>
> @@ -4190,9 +4197,9 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
>
>   		/*
>   		 * - WM1+ latency values in 0.5us units
> -		 * - latencies are in us on gen9
> +		 * - latencies are in us on gen9/vlv/chv
>   		 */
> -		if (INTEL_INFO(dev)->gen >= 9)
> +		if (INTEL_INFO(dev)->gen >= 9 || IS_VALLEYVIEW(dev))
>   			latency *= 10;
>   		else if (level > 0)
>   			latency *= 5;
> @@ -4256,7 +4263,7 @@ static int pri_wm_latency_open(struct inode *inode, struct file *file)
>   {
>   	struct drm_device *dev = inode->i_private;
>
> -	if (HAS_GMCH_DISPLAY(dev))
> +	if (INTEL_INFO(dev)->gen < 5)
>   		return -ENODEV;
>
>   	return single_open(file, pri_wm_latency_show, dev);
> @@ -4288,11 +4295,18 @@ static ssize_t wm_latency_write(struct file *file, const char __user *ubuf,
>   	struct seq_file *m = file->private_data;
>   	struct drm_device *dev = m->private;
>   	uint16_t new[8] = { 0 };
> -	int num_levels = ilk_wm_max_level(dev) + 1;
> +	int num_levels;
>   	int level;
>   	int ret;
>   	char tmp[32];
>
> +	if (IS_CHERRYVIEW(dev))
> +		num_levels = 3;
> +	else if (IS_VALLEYVIEW(dev))
> +		num_levels = 1;
> +	else
> +		num_levels = ilk_wm_max_level(dev) + 1;
> +
>   	if (len >= sizeof(tmp))
>   		return -EINVAL;
>
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

[PATCH 10/10] drm/i915: Zero unused WM1 watermarks on VLV/CHV

[ville.syrjala]

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

The hardware supposedly ignores the WM1 watermarks while the PND
deadline mode is enabled, but clear out the register just in case.
This is what the other OS does, and it does make register dumps look
more consistent when we don't have partial WM1 values lingering in
the registers (some WM1 watermarks already get zeroed when the actually
used DSPFW registers get written).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c8e7ef3..dc8a9c9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -927,6 +927,12 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 			   FW_WM(wm->pipe[PIPE_A].primary >> 8, PLANEA_HI));
 	}
 
+	/* zero (unused) WM1 watermarks */
+	I915_WRITE(DSPFW4, 0);
+	I915_WRITE(DSPFW5, 0);
+	I915_WRITE(DSPFW6, 0);
+	I915_WRITE(DSPHOWM1, 0);
+
 	POSTING_READ(DSPFW1);
 }
 
-- 
2.3.6

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

Re: [PATCH 10/10] drm/i915: Zero unused WM1 watermarks on VLV/CHV

[Clint Taylor]

On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The hardware supposedly ignores the WM1 watermarks while the PND
> deadline mode is enabled, but clear out the register just in case.
> This is what the other OS does, and it does make register dumps look
> more consistent when we don't have partial WM1 values lingering in
> the registers (some WM1 watermarks already get zeroed when the actually
> used DSPFW registers get written).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c8e7ef3..dc8a9c9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -927,6 +927,12 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>   			   FW_WM(wm->pipe[PIPE_A].primary >> 8, PLANEA_HI));
>   	}
>
> +	/* zero (unused) WM1 watermarks */
> +	I915_WRITE(DSPFW4, 0);
> +	I915_WRITE(DSPFW5, 0);
> +	I915_WRITE(DSPFW6, 0);
> +	I915_WRITE(DSPHOWM1, 0);
> +
>   	POSTING_READ(DSPFW1);
>   }
>
>

Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com>

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

Re: [PATCH 10/10] drm/i915: Zero unused WM1 watermarks on VLV/CHV

[Daniel Vetter]

On Fri, Jun 26, 2015 at 01:24:17PM -0700, Clint Taylor wrote:
> On 06/24/2015 12:00 PM, ville.syrjala@linux.intel.com wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >The hardware supposedly ignores the WM1 watermarks while the PND
> >deadline mode is enabled, but clear out the register just in case.
> >This is what the other OS does, and it does make register dumps look
> >more consistent when we don't have partial WM1 values lingering in
> >the registers (some WM1 watermarks already get zeroed when the actually
> >used DSPFW registers get written).
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >index c8e7ef3..dc8a9c9 100644
> >--- a/drivers/gpu/drm/i915/intel_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_pm.c
> >@@ -927,6 +927,12 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >  			   FW_WM(wm->pipe[PIPE_A].primary >> 8, PLANEA_HI));
> >  	}
> >
> >+	/* zero (unused) WM1 watermarks */
> >+	I915_WRITE(DSPFW4, 0);
> >+	I915_WRITE(DSPFW5, 0);
> >+	I915_WRITE(DSPFW6, 0);
> >+	I915_WRITE(DSPHOWM1, 0);
> >+
> >  	POSTING_READ(DSPFW1);
> >  }
> >
> >
> 
> Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com>

Pulled in entire series, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx