[PATCH 1/9] drm/i915: SAGV is not SKL-only, so rename a few things

[PATCH 1/9] drm/i915: SAGV is not SKL-only, so rename a few things

[Paulo Zanoni]

The plan is to introduce intel_has_sagv() and then use it to discover
which platforms actually support it.

I thought about keeping the functions with their current skl names,
but found two problems: (i) skl_has_sagv() would become a very
confusing name, and (ii) intel_atomic_commit_tail() doesn't seem to be
calling any functions whose name start with a platform name, so the
"intel_" naming scheme seems make more sense than the "firstplatorm_"
naming scheme here.

Cc: stable@vger.kernel.org
Reviewed-by: Lyude <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 10 +++++-----
 drivers/gpu/drm/i915/intel_display.c |  8 ++++----
 drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
 drivers/gpu/drm/i915/intel_pm.c      | 26 +++++++++++++-------------
 4 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0f0f11..23bc43d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1988,11 +1988,11 @@ struct drm_i915_private {
 	struct vlv_s0ix_state vlv_s0ix_state;
 
 	enum {
-		I915_SKL_SAGV_UNKNOWN = 0,
-		I915_SKL_SAGV_DISABLED,
-		I915_SKL_SAGV_ENABLED,
-		I915_SKL_SAGV_NOT_CONTROLLED
-	} skl_sagv_status;
+		I915_SAGV_UNKNOWN = 0,
+		I915_SAGV_DISABLED,
+		I915_SAGV_ENABLED,
+		I915_SAGV_NOT_CONTROLLED
+	} sagv_status;
 
 	struct {
 		/*
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5ad101..45d6183 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14367,8 +14367,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		 * SKL workaround: bspec recommends we disable the SAGV when we
 		 * have more then one pipe enabled
 		 */
-		if (IS_SKYLAKE(dev_priv) && !skl_can_enable_sagv(state))
-			skl_disable_sagv(dev_priv);
+		if (IS_SKYLAKE(dev_priv) && !intel_can_enable_sagv(state))
+			intel_disable_sagv(dev_priv);
 
 		intel_modeset_verify_disabled(dev);
 	}
@@ -14426,8 +14426,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
-	    skl_can_enable_sagv(state))
-		skl_enable_sagv(dev_priv);
+	    intel_can_enable_sagv(state))
+		intel_enable_sagv(dev_priv);
 
 	drm_atomic_helper_commit_hw_done(state);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9ce2611..4bfb01c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1746,9 +1746,9 @@ 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,
 			  struct skl_ddb_allocation *ddb /* out */);
-bool skl_can_enable_sagv(struct drm_atomic_state *state);
-int skl_enable_sagv(struct drm_i915_private *dev_priv);
-int skl_disable_sagv(struct drm_i915_private *dev_priv);
+bool intel_can_enable_sagv(struct drm_atomic_state *state);
+int intel_enable_sagv(struct drm_i915_private *dev_priv);
+int intel_disable_sagv(struct drm_i915_private *dev_priv);
 bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 			       const struct skl_ddb_allocation *new,
 			       enum pipe pipe);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a79212f..4db3b04 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2889,12 +2889,12 @@ skl_wm_plane_id(const struct intel_plane *plane)
  *  - We're not using an interlaced display configuration
  */
 int
-skl_enable_sagv(struct drm_i915_private *dev_priv)
+intel_enable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
-	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
+	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
+	    dev_priv->sagv_status == I915_SAGV_ENABLED)
 		return 0;
 
 	DRM_DEBUG_KMS("Enabling the SAGV\n");
@@ -2912,19 +2912,19 @@ skl_enable_sagv(struct drm_i915_private *dev_priv)
 	 */
 	if (ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
-		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
 	} else if (ret < 0) {
 		DRM_ERROR("Failed to enable the SAGV\n");
 		return ret;
 	}
 
-	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
+	dev_priv->sagv_status = I915_SAGV_ENABLED;
 	return 0;
 }
 
 static int
-skl_do_sagv_disable(struct drm_i915_private *dev_priv)
+intel_do_sagv_disable(struct drm_i915_private *dev_priv)
 {
 	int ret;
 	uint32_t temp = GEN9_SAGV_DISABLE;
@@ -2938,19 +2938,19 @@ skl_do_sagv_disable(struct drm_i915_private *dev_priv)
 }
 
 int
-skl_disable_sagv(struct drm_i915_private *dev_priv)
+intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret, result;
 
-	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
-	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
+	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
+	    dev_priv->sagv_status == I915_SAGV_DISABLED)
 		return 0;
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
 	mutex_lock(&dev_priv->rps.hw_lock);
 
 	/* bspec says to keep retrying for at least 1 ms */
-	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
+	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	if (ret == -ETIMEDOUT) {
@@ -2964,18 +2964,18 @@ skl_disable_sagv(struct drm_i915_private *dev_priv)
 	 */
 	if (result == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
-		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
 	} else if (result < 0) {
 		DRM_ERROR("Failed to disable the SAGV\n");
 		return result;
 	}
 
-	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
+	dev_priv->sagv_status = I915_SAGV_DISABLED;
 	return 0;
 }
 
-bool skl_can_enable_sagv(struct drm_atomic_state *state)
+bool intel_can_enable_sagv(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-- 
2.7.4

[PATCH 2/9] drm/i915: introduce intel_has_sagv()

[Paulo Zanoni]

And use it to move knowledge about the SAGV-supporting platforms from
the callers to the SAGV code.

We'll add more platforms to intel_has_sagv(), so IMHO it makes more
sense to move all this to a single function instead of patching all
the callers every time we add SAGV support to a new platform.

v2: Move I915_SAGV_NOT_CONTROLLED to the new function (Lyude).

Cc: stable@vger.kernel.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 ++---
 drivers/gpu/drm/i915/intel_pm.c      | 22 ++++++++++++++++++----
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 45d6183..8e464e0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14367,7 +14367,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		 * SKL workaround: bspec recommends we disable the SAGV when we
 		 * have more then one pipe enabled
 		 */
-		if (IS_SKYLAKE(dev_priv) && !intel_can_enable_sagv(state))
+		if (!intel_can_enable_sagv(state))
 			intel_disable_sagv(dev_priv);
 
 		intel_modeset_verify_disabled(dev);
@@ -14425,8 +14425,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
-	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
-	    intel_can_enable_sagv(state))
+	if (intel_state->modeset && intel_can_enable_sagv(state))
 		intel_enable_sagv(dev_priv);
 
 	drm_atomic_helper_commit_hw_done(state);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4db3b04..13809a3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2877,6 +2877,13 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	}
 }
 
+static bool
+intel_has_sagv(struct drm_i915_private *dev_priv)
+{
+	return IS_SKYLAKE(dev_priv) &&
+	       dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
+}
+
 /*
  * SAGV dynamically adjusts the system agent voltage and clock frequencies
  * depending on power and performance requirements. The display engine access
@@ -2893,8 +2900,10 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
-	    dev_priv->sagv_status == I915_SAGV_ENABLED)
+	if (!intel_has_sagv(dev_priv))
+		return 0;
+
+	if (dev_priv->sagv_status == I915_SAGV_ENABLED)
 		return 0;
 
 	DRM_DEBUG_KMS("Enabling the SAGV\n");
@@ -2942,8 +2951,10 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret, result;
 
-	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
-	    dev_priv->sagv_status == I915_SAGV_DISABLED)
+	if (!intel_has_sagv(dev_priv))
+		return 0;
+
+	if (dev_priv->sagv_status == I915_SAGV_DISABLED)
 		return 0;
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
@@ -2984,6 +2995,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	enum pipe pipe;
 	int level, plane;
 
+	if (!intel_has_sagv(dev_priv))
+		return false;
+
 	/*
 	 * SKL workaround: bspec recommends we disable the SAGV when we have
 	 * more then one pipe enabled
-- 
2.7.4

[PATCH 3/9] drm/i915/kbl: KBL also needs to run the SAGV code

[Paulo Zanoni]

According to BSpec, it's the "core CPUs" that need the code, which
means SKL and KBL, but not BXT.

I don't have a KBL to test this patch on it.

v2: Only SKL should have I915_SAGV_NOT_CONTROLLED.

Cc: stable@vger.kernel.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 13809a3..f09d912 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2880,8 +2880,14 @@ skl_wm_plane_id(const struct intel_plane *plane)
 static bool
 intel_has_sagv(struct drm_i915_private *dev_priv)
 {
-	return IS_SKYLAKE(dev_priv) &&
-	       dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
+	if (IS_KABYLAKE(dev_priv))
+		return true;
+
+	if (IS_SKYLAKE(dev_priv) &&
+	    dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED)
+		return true;
+
+	return false;
 }
 
 /*
@@ -2919,7 +2925,7 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
 	 * Some skl systems, pre-release machines in particular,
 	 * don't actually have an SAGV.
 	 */
-	if (ret == -ENXIO) {
+	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
@@ -2973,7 +2979,7 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 	 * Some skl systems, pre-release machines in particular,
 	 * don't actually have an SAGV.
 	 */
-	if (result == -ENXIO) {
+	if (IS_SKYLAKE(dev_priv) && result == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
-- 
2.7.4

[PATCH 4/9] drm/i915/gen9: fix the WaWmMemoryReadLatency implementation

[Paulo Zanoni]

Bspec says:
  "The mailbox response data may not account for memory read latency.
   If the mailbox response data for level 0 is 0us, add 2 microseconds
   to the result for each valid level."

This means we should only do the +2 in case wm[0] == 0, not always.

So split the sanitizing implementation from the WA implementation and
fix the WA implementation.

v2: Add Fixes tag (Maarten).

Fixes: 367294be7c25 ("drm/i915/gen9: Add 2us read latency to WM level")
Cc: stable@vger.kernel.org
Cc: Vandana Kannan <vandana.kannan@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 42 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f09d912..ee561c2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2127,32 +2127,34 @@ static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[8])
 				GEN9_MEM_LATENCY_LEVEL_MASK;
 
 		/*
+		 * If a level n (n > 1) has a 0us latency, all levels m (m >= n)
+		 * need to be disabled. We make sure to sanitize the values out
+		 * of the punit to satisfy this requirement.
+		 */
+		for (level = 1; level <= max_level; level++) {
+			if (wm[level] == 0) {
+				for (i = level + 1; i <= max_level; i++)
+					wm[i] = 0;
+				break;
+			}
+		}
+
+		/*
 		 * WaWmMemoryReadLatency:skl
 		 *
 		 * punit doesn't take into account the read latency so we need
-		 * to add 2us to the various latency levels we retrieve from
-		 * the punit.
-		 *   - W0 is a bit special in that it's the only level that
-		 *   can't be disabled if we want to have display working, so
-		 *   we always add 2us there.
-		 *   - For levels >=1, punit returns 0us latency when they are
-		 *   disabled, so we respect that and don't add 2us then
-		 *
-		 * Additionally, if a level n (n > 1) has a 0us latency, all
-		 * levels m (m >= n) need to be disabled. We make sure to
-		 * sanitize the values out of the punit to satisfy this
-		 * requirement.
+		 * to add 2us to the various latency levels we retrieve from the
+		 * punit when level 0 response data us 0us.
 		 */
-		wm[0] += 2;
-		for (level = 1; level <= max_level; level++)
-			if (wm[level] != 0)
+		if (wm[0] == 0) {
+			wm[0] += 2;
+			for (level = 1; level <= max_level; level++) {
+				if (wm[level] == 0)
+					break;
 				wm[level] += 2;
-			else {
-				for (i = level + 1; i <= max_level; i++)
-					wm[i] = 0;
-
-				break;
 			}
+		}
+
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		uint64_t sskpd = I915_READ64(MCH_SSKPD);
 
-- 
2.7.4

[PATCH 5/9] drm/i915/gen9: minimum scanlines for Y tile is not always 4

[Paulo Zanoni]

During watermarks calculations, this value is used in 3 different
places. Only one of them was not using a hardcoded 4. Move the code up
so everybody can benefit from the actual value.

This should only help on situations with Y tiling + 90/270 rotation +
1 or 2 bpp or NV12.

Cc: stable@vger.kernel.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 56 +++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee561c2..a7f5f7f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3495,7 +3495,8 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
 
 static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 			       uint32_t horiz_pixels, uint8_t cpp,
-			       uint64_t tiling, uint32_t latency)
+			       uint64_t tiling, uint32_t latency,
+			       uint32_t y_min_scanlines)
 {
 	uint32_t ret;
 	uint32_t plane_bytes_per_line, plane_blocks_per_line;
@@ -3508,9 +3509,9 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 
 	if (tiling == I915_FORMAT_MOD_Y_TILED ||
 	    tiling == I915_FORMAT_MOD_Yf_TILED) {
-		plane_bytes_per_line *= 4;
+		plane_bytes_per_line *= y_min_scanlines;
 		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
-		plane_blocks_per_line /= 4;
+		plane_blocks_per_line /= y_min_scanlines;
 	} else if (tiling == DRM_FORMAT_MOD_NONE) {
 		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
 	} else {
@@ -3567,6 +3568,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint8_t cpp;
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
+	uint32_t y_min_scanlines;
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
@@ -3582,38 +3584,44 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 	plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate, intel_pstate);
 
+	if (intel_rotation_90_or_270(pstate->rotation)) {
+		int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
+			drm_format_plane_cpp(fb->pixel_format, 1) :
+			drm_format_plane_cpp(fb->pixel_format, 0);
+
+		switch (cpp) {
+		case 1:
+			y_min_scanlines = 16;
+			break;
+		case 2:
+			y_min_scanlines = 8;
+			break;
+		default:
+			WARN(1, "Unsupported pixel depth for rotation");
+		case 4:
+			y_min_scanlines = 4;
+			break;
+		}
+	} else {
+		y_min_scanlines = 4;
+	}
+
 	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
 	method2 = skl_wm_method2(plane_pixel_rate,
 				 cstate->base.adjusted_mode.crtc_htotal,
 				 width,
 				 cpp,
 				 fb->modifier[0],
-				 latency);
+				 latency,
+				 y_min_scanlines);
 
 	plane_bytes_per_line = width * cpp;
 	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 
 	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t min_scanlines = 4;
-		uint32_t y_tile_minimum;
-		if (intel_rotation_90_or_270(pstate->rotation)) {
-			int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
-				drm_format_plane_cpp(fb->pixel_format, 1) :
-				drm_format_plane_cpp(fb->pixel_format, 0);
-
-			switch (cpp) {
-			case 1:
-				min_scanlines = 16;
-				break;
-			case 2:
-				min_scanlines = 8;
-				break;
-			case 8:
-				WARN(1, "Unsupported pixel depth for rotation");
-			}
-		}
-		y_tile_minimum = plane_blocks_per_line * min_scanlines;
+		uint32_t y_tile_minimum = plane_blocks_per_line *
+					  y_min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3628,7 +3636,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	if (level >= 1 && level <= 7) {
 		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
-			res_lines += 4;
+			res_lines += y_min_scanlines;
 		else
 			res_blocks++;
 	}
-- 
2.7.4

Re: [Intel-gfx] [PATCH 5/9] drm/i915/gen9: minimum scanlines for Y tile is not always 4

[Tvrtko Ursulin]

Hi,

On 22/09/2016 22:00, Paulo Zanoni wrote:
> During watermarks calculations, this value is used in 3 different
> places. Only one of them was not using a hardcoded 4. Move the code up
> so everybody can benefit from the actual value.
>
> This should only help on situations with Y tiling + 90/270 rotation +
> 1 or 2 bpp or NV12.

I don't think bothering stable with this was required since 90/270 on 
NV12 was never fully enabled AFAIR. Unless Ville accidentaly enabled it 
in his recent rewrite? But it was not put in due instability issues so I 
hope not.

Regards,

Tvrtko

> Cc: stable@vger.kernel.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 56 +++++++++++++++++++++++------------------
>   1 file changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee561c2..a7f5f7f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3495,7 +3495,8 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
>   
>   static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>   			       uint32_t horiz_pixels, uint8_t cpp,
> -			       uint64_t tiling, uint32_t latency)
> +			       uint64_t tiling, uint32_t latency,
> +			       uint32_t y_min_scanlines)
>   {
>   	uint32_t ret;
>   	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> @@ -3508,9 +3509,9 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>   
>   	if (tiling == I915_FORMAT_MOD_Y_TILED ||
>   	    tiling == I915_FORMAT_MOD_Yf_TILED) {
> -		plane_bytes_per_line *= 4;
> +		plane_bytes_per_line *= y_min_scanlines;
>   		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> -		plane_blocks_per_line /= 4;
> +		plane_blocks_per_line /= y_min_scanlines;
>   	} else if (tiling == DRM_FORMAT_MOD_NONE) {
>   		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
>   	} else {
> @@ -3567,6 +3568,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>   	uint8_t cpp;
>   	uint32_t width = 0, height = 0;
>   	uint32_t plane_pixel_rate;
> +	uint32_t y_min_scanlines;
>   
>   	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
>   		*enabled = false;
> @@ -3582,38 +3584,44 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>   	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>   	plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate, intel_pstate);
>   
> +	if (intel_rotation_90_or_270(pstate->rotation)) {
> +		int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
> +			drm_format_plane_cpp(fb->pixel_format, 1) :
> +			drm_format_plane_cpp(fb->pixel_format, 0);
> +
> +		switch (cpp) {
> +		case 1:
> +			y_min_scanlines = 16;
> +			break;
> +		case 2:
> +			y_min_scanlines = 8;
> +			break;
> +		default:
> +			WARN(1, "Unsupported pixel depth for rotation");
> +		case 4:
> +			y_min_scanlines = 4;
> +			break;
> +		}
> +	} else {
> +		y_min_scanlines = 4;
> +	}
> +
>   	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
>   	method2 = skl_wm_method2(plane_pixel_rate,
>   				 cstate->base.adjusted_mode.crtc_htotal,
>   				 width,
>   				 cpp,
>   				 fb->modifier[0],
> -				 latency);
> +				 latency,
> +				 y_min_scanlines);
>   
>   	plane_bytes_per_line = width * cpp;
>   	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>   
>   	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>   	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> -		uint32_t min_scanlines = 4;
> -		uint32_t y_tile_minimum;
> -		if (intel_rotation_90_or_270(pstate->rotation)) {
> -			int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
> -				drm_format_plane_cpp(fb->pixel_format, 1) :
> -				drm_format_plane_cpp(fb->pixel_format, 0);
> -
> -			switch (cpp) {
> -			case 1:
> -				min_scanlines = 16;
> -				break;
> -			case 2:
> -				min_scanlines = 8;
> -				break;
> -			case 8:
> -				WARN(1, "Unsupported pixel depth for rotation");
> -			}
> -		}
> -		y_tile_minimum = plane_blocks_per_line * min_scanlines;
> +		uint32_t y_tile_minimum = plane_blocks_per_line *
> +					  y_min_scanlines;
>   		selected_result = max(method2, y_tile_minimum);
>   	} else {
>   		if ((ddb_allocation / plane_blocks_per_line) >= 1)
> @@ -3628,7 +3636,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>   	if (level >= 1 && level <= 7) {
>   		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>   		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
> -			res_lines += 4;
> +			res_lines += y_min_scanlines;
>   		else
>   			res_blocks++;
>   	}

[PATCH 6/9] drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations

[Paulo Zanoni]

The confusing thing is that plane_blocks_per_line is listed as part of
the method 2 calculation but is also used for other things. We
calculated it in two different places and different ways: one inside
skl_wm_method2() and the other inside skl_compute_plane_wm(). The
skl_wm_method2() implementation is the one that matches the
specification.

With this patch we fix the skl_compute_plane_wm() calculation and just
pass it as a parameter to skl_wm_method2(). We also take care to not
modify the value of plane_bytes_per_line since we're going to rely on
it having a correct value in later patches.

This should affect the watermarks for Linear and Y-tiled.

>From my analysis, it looks like the two plane_blocks_per_line
variables got out of sync on 0fda65680e92, but we can't really say
that commit was a regression, it looks like just an incomplete fix.
There's always the possibility that 0fda65680e92 matched our
specification at that time, and then later the specification changed.

v2: Try to add a "Fixes" tag (Maarten).

Fixes: 0fda65680e92 ("drm/i915/skl: Update watermarks for Y tiling")
Cc: stable@vger.kernel.org
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Lyude <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a7f5f7f..a6ae7b7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3494,30 +3494,14 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
 }
 
 static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
-			       uint32_t horiz_pixels, uint8_t cpp,
-			       uint64_t tiling, uint32_t latency,
-			       uint32_t y_min_scanlines)
+			       uint32_t latency, uint32_t plane_blocks_per_line)
 {
 	uint32_t ret;
-	uint32_t plane_bytes_per_line, plane_blocks_per_line;
 	uint32_t wm_intermediate_val;
 
 	if (latency == 0)
 		return UINT_MAX;
 
-	plane_bytes_per_line = horiz_pixels * cpp;
-
-	if (tiling == I915_FORMAT_MOD_Y_TILED ||
-	    tiling == I915_FORMAT_MOD_Yf_TILED) {
-		plane_bytes_per_line *= y_min_scanlines;
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
-		plane_blocks_per_line /= y_min_scanlines;
-	} else if (tiling == DRM_FORMAT_MOD_NONE) {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
-	} else {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
-	}
-
 	wm_intermediate_val = latency * pixel_rate;
 	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
 				plane_blocks_per_line;
@@ -3606,17 +3590,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		y_min_scanlines = 4;
 	}
 
+	plane_bytes_per_line = width * cpp;
+	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
+	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
+		plane_blocks_per_line =
+		      DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
+		plane_blocks_per_line /= y_min_scanlines;
+	} else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) {
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
+					+ 1;
+	} else {
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+	}
+
 	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
 	method2 = skl_wm_method2(plane_pixel_rate,
 				 cstate->base.adjusted_mode.crtc_htotal,
-				 width,
-				 cpp,
-				 fb->modifier[0],
 				 latency,
-				 y_min_scanlines);
-
-	plane_bytes_per_line = width * cpp;
-	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+				 plane_blocks_per_line);
 
 	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
-- 
2.7.4

Re: [Intel-gfx] [PATCH 6/9] drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations

[Tvrtko Ursulin]

On 22/09/2016 22:00, Paulo Zanoni wrote:
> The confusing thing is that plane_blocks_per_line is listed as part of
> the method 2 calculation but is also used for other things. We
> calculated it in two different places and different ways: one inside
> skl_wm_method2() and the other inside skl_compute_plane_wm(). The
> skl_wm_method2() implementation is the one that matches the
> specification.
>
> With this patch we fix the skl_compute_plane_wm() calculation and just
> pass it as a parameter to skl_wm_method2(). We also take care to not
> modify the value of plane_bytes_per_line since we're going to rely on
> it having a correct value in later patches.
>
> This should affect the watermarks for Linear and Y-tiled.
>
>  From my analysis, it looks like the two plane_blocks_per_line
> variables got out of sync on 0fda65680e92, but we can't really say
> that commit was a regression, it looks like just an incomplete fix.
> There's always the possibility that 0fda65680e92 matched our
> specification at that time, and then later the specification changed.

Yes, it wouldn't be a regression since before that commit changing 
tiling with page flips did not work at all.

Whether or not calculation was fully correct at the time I am not sure. 
The formulas changed at least twice after the initial implementation so  
both are possible.

Regards,

Tvrtko

> v2: Try to add a "Fixes" tag (Maarten).
>
> Fixes: 0fda65680e92 ("drm/i915/skl: Update watermarks for Y tiling")
> Cc: stable@vger.kernel.org
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++------------------------
>   1 file changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a7f5f7f..a6ae7b7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3494,30 +3494,14 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
>   }
>   
>   static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
> -			       uint32_t horiz_pixels, uint8_t cpp,
> -			       uint64_t tiling, uint32_t latency,
> -			       uint32_t y_min_scanlines)
> +			       uint32_t latency, uint32_t plane_blocks_per_line)
>   {
>   	uint32_t ret;
> -	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>   	uint32_t wm_intermediate_val;
>   
>   	if (latency == 0)
>   		return UINT_MAX;
>   
> -	plane_bytes_per_line = horiz_pixels * cpp;
> -
> -	if (tiling == I915_FORMAT_MOD_Y_TILED ||
> -	    tiling == I915_FORMAT_MOD_Yf_TILED) {
> -		plane_bytes_per_line *= y_min_scanlines;
> -		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> -		plane_blocks_per_line /= y_min_scanlines;
> -	} else if (tiling == DRM_FORMAT_MOD_NONE) {
> -		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
> -	} else {
> -		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> -	}
> -
>   	wm_intermediate_val = latency * pixel_rate;
>   	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
>   				plane_blocks_per_line;
> @@ -3606,17 +3590,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>   		y_min_scanlines = 4;
>   	}
>   
> +	plane_bytes_per_line = width * cpp;
> +	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> +	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> +		plane_blocks_per_line =
> +		      DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
> +		plane_blocks_per_line /= y_min_scanlines;
> +	} else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) {
> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
> +					+ 1;
> +	} else {
> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +	}
> +
>   	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
>   	method2 = skl_wm_method2(plane_pixel_rate,
>   				 cstate->base.adjusted_mode.crtc_htotal,
> -				 width,
> -				 cpp,
> -				 fb->modifier[0],
>   				 latency,
> -				 y_min_scanlines);
> -
> -	plane_bytes_per_line = width * cpp;
> -	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +				 plane_blocks_per_line);
>   
>   	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>   	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {

[PATCH 7/9] drm/i915/gen9: fix the watermark res_blocks value

[Paulo Zanoni]

We forgot the "res_blocks += y_tile_minimum" that's described on step
V of our documentation.

Again, this should only affect the Y tiling cases.

It looks like the relevant code was introduced in 0fda65680e92, but
there's always the possibility that it matched our specification when
it was introduced, and then the specification changed while the code
stayed the same. So we can't really say this was a regression, but
let's try to add a "Fixes" tag anyway to help backporting.

v2: Try to add a "Fixes" tag (Maarten).

Fixes: 0fda65680e92 ("drm/i915/skl: Update watermarks for Y tiling")
Cc: stable@vger.kernel.org
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Lyude <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a6ae7b7..0957f5f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3552,7 +3552,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint8_t cpp;
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
-	uint32_t y_min_scanlines;
+	uint32_t y_tile_minimum, y_min_scanlines;
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
@@ -3609,10 +3609,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				 latency,
 				 plane_blocks_per_line);
 
+	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
+
 	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t y_tile_minimum = plane_blocks_per_line *
-					  y_min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3626,10 +3626,12 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	if (level >= 1 && level <= 7) {
 		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
-		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
+		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
+			res_blocks += y_tile_minimum;
 			res_lines += y_min_scanlines;
-		else
+		} else {
 			res_blocks++;
+		}
 	}
 
 	if (res_blocks >= ddb_allocation || res_lines > 31) {
-- 
2.7.4