[PATCH 0/9] SKL/KBL watermark fixes, v2

[PATCH 0/9] SKL/KBL watermark fixes, v2

[Paulo Zanoni]

Hi

Here's the series with the reviews implemented. There's a new patch,
based on the additional issue spotted by Lyude.

Thanks for all the reviews,
Paulo

Paulo Zanoni (9):
  drm/i915: SAGV is not SKL-only, so rename a few things
  drm/i915: introduce intel_has_sagv()
  drm/i915/kbl: KBL also needs to run the SAGV code
  drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
  drm/i915/gen9: minimum scanlines for Y tile is not always 4
  drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations
  drm/i915/gen9: fix the watermark res_blocks value
  drm/i915/gen9: implement missing case for SKL watermarks calculation
  drm/i915/gen9: fail the modeset instead of WARNing on unsuported
    config

 drivers/gpu/drm/i915/i915_drv.h      |  10 +-
 drivers/gpu/drm/i915/intel_display.c |   9 +-
 drivers/gpu/drm/i915/intel_drv.h     |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++---------------
 4 files changed, 118 insertions(+), 93 deletions(-)

-- 
2.7.4

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

[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.

Reviewed-by: Lyude <cpaul@redhat.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 5b1c241..35c5ea9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1985,11 +1985,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 d045b2b..e2b3979 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14364,8 +14364,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);
 	}
@@ -14423,8 +14423,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 0d05712..5ac18a4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1738,9 +1738,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 6af438f..ed5738f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2896,12 +2896,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");
@@ -2919,19 +2919,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;
@@ -2945,19 +2945,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) {
@@ -2971,18 +2971,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

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

[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).

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 e2b3979..73018d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14364,7 +14364,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);
@@ -14422,8 +14422,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 ed5738f..5b2eb7a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2884,6 +2884,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
@@ -2900,8 +2907,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");
@@ -2949,8 +2958,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");
@@ -2991,6 +3002,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

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

[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.

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 5b2eb7a..1968672 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2887,8 +2887,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;
 }
 
 /*
@@ -2926,7 +2932,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;
@@ -2980,7 +2986,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

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

[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 1968672..fe43044 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.

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 fe43044..4d46315 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3504,7 +3504,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;
@@ -3517,9 +3518,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 {
@@ -3576,6 +3577,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;
@@ -3591,38 +3593,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)
@@ -3637,7 +3645,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

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

[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>
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 4d46315..8eae4e2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3503,30 +3503,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;
@@ -3615,17 +3599,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

[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>
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 8eae4e2..2e6099b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3561,7 +3561,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;
@@ -3618,10 +3618,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)
@@ -3635,10 +3635,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

[PATCH 8/9] drm/i915/gen9: implement missing case for SKL watermarks calculation

[Paulo Zanoni]

This should affect linear and X tiled planes on really small htotal
cases. It doesn't seem to be a very feasible case, but let's implement
it since it's on the specification and it's better to have it and
never need than not have it and realize we needed it.

Reviewed-by: Lyude <cpaul@redhat.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2e6099b..9edc8ce 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3624,7 +3624,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
 		selected_result = max(method2, y_tile_minimum);
 	} else {
-		if ((ddb_allocation / plane_blocks_per_line) >= 1)
+		if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
+		    (plane_bytes_per_line / 512 < 1))
+			selected_result = method2;
+		else if ((ddb_allocation / plane_blocks_per_line) >= 1)
 			selected_result = min(method1, method2);
 		else
 			selected_result = method1;
-- 
2.7.4

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

[PATCH 9/9] drm/i915/gen9: fail the modeset instead of WARNing on unsuported config

[Paulo Zanoni]

Now that this code is part of the compute stage we can return -EINVAL
to prevent the modeset instead of giving a WARN and trying anyway.

Reported-by: Lyude <cpaul@redhat.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9edc8ce..6394db7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3589,11 +3589,11 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		case 2:
 			y_min_scanlines = 8;
 			break;
-		default:
-			WARN(1, "Unsupported pixel depth for rotation");
 		case 4:
 			y_min_scanlines = 4;
 			break;
+		default:
+			return -EINVAL;
 		}
 	} else {
 		y_min_scanlines = 4;
-- 
2.7.4

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

✗ Fi.CI.BAT: failure for SKL/KBL watermark fixes (rev2)

[Patchwork]

== Series Details ==

Series: SKL/KBL watermark fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/12082/
State : failure

== Summary ==

Series 12082v2 SKL/KBL watermark fixes
https://patchwork.freedesktop.org/api/1.0/series/12082/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-hsw-4770k)

fi-bsw-n3050     total:254  pass:208  dwarn:0   dfail:0   fail:0   skip:46 
fi-hsw-4770k     total:212  pass:191  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:254  pass:228  dwarn:0   dfail:0   fail:0   skip:26 
fi-ilk-650       total:254  pass:184  dwarn:0   dfail:0   fail:2   skip:68 
fi-ivb-3520m     total:254  pass:223  dwarn:0   dfail:0   fail:0   skip:31 
fi-ivb-3770      total:254  pass:211  dwarn:0   dfail:0   fail:0   skip:43 
fi-skl-6260u     total:254  pass:240  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:254  pass:227  dwarn:0   dfail:0   fail:1   skip:26 
fi-skl-6700k     total:254  pass:225  dwarn:1   dfail:0   fail:0   skip:28 
fi-snb-2520m     total:254  pass:210  dwarn:0   dfail:0   fail:0   skip:44 
fi-snb-2600      total:254  pass:209  dwarn:0   dfail:0   fail:0   skip:45 

Results at /archive/results/CI_IGT_test/Patchwork_2525/

208290026552464713d3897ab5d649f4445d5513 drm-intel-nightly: 2016y-09m-13d-14h-45m-32s UTC integration manifest
e231e9c drm/i915/gen9: fail the modeset instead of WARNing on unsuported config
46e54b0 drm/i915/gen9: implement missing case for SKL watermarks calculation
223c06a drm/i915/gen9: fix the watermark res_blocks value
9a41c0d drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations
ba8b5f4 drm/i915/gen9: minimum scanlines for Y tile is not always 4
96a04ba drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
6945097 drm/i915/kbl: KBL also needs to run the SAGV code
2a30f99 drm/i915: introduce intel_has_sagv()
a62591e drm/i915: SAGV is not SKL-only, so rename a few things

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

Re: [PATCH 0/9] SKL/KBL watermark fixes, v2

[Jani Nikula]

On Wed, 14 Sep 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> Hi
>
> Here's the series with the reviews implemented. There's a new patch,
> based on the additional issue spotted by Lyude.

There's a bunch of cc: stable patches mixed with non cc: stable patches
in the series. Do the cc: stable fixes work and backport cleanly without
the the other non cc: stable patches? If not, can you arrange the series
to not depend on the other patches?

BR,
Jani.


>
> Thanks for all the reviews,
> Paulo
>
> Paulo Zanoni (9):
>   drm/i915: SAGV is not SKL-only, so rename a few things
>   drm/i915: introduce intel_has_sagv()
>   drm/i915/kbl: KBL also needs to run the SAGV code
>   drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
>   drm/i915/gen9: minimum scanlines for Y tile is not always 4
>   drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations
>   drm/i915/gen9: fix the watermark res_blocks value
>   drm/i915/gen9: implement missing case for SKL watermarks calculation
>   drm/i915/gen9: fail the modeset instead of WARNing on unsuported
>     config
>
>  drivers/gpu/drm/i915/i915_drv.h      |  10 +-
>  drivers/gpu/drm/i915/intel_display.c |   9 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++---------------
>  4 files changed, 118 insertions(+), 93 deletions(-)

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

Re: [PATCH 0/9] SKL/KBL watermark fixes, v2

[Zanoni, Paulo R]

Em Qua, 2016-09-14 às 12:34 +0300, Jani Nikula escreveu:
> On Wed, 14 Sep 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> > 
> > Hi
> > 
> > Here's the series with the reviews implemented. There's a new
> > patch,
> > based on the additional issue spotted by Lyude.
> 
> There's a bunch of cc: stable patches mixed with non cc: stable
> patches
> in the series. Do the cc: stable fixes work and backport cleanly
> without
> the the other non cc: stable patches? If not, can you arrange the
> series
> to not depend on the other patches?

Yeah, my bad. I was just pasting the output of "dim fixes" without
considering this aspect. I think the best thing is probably to backport
everything to stable and hope it works, considering the current
complaints we're seeing about screen flickering on SKL. Agree?

I suppose I don't need to resend the series just for these new tags.
I'll add them when it's time to merge.

> 
> BR,
> Jani.
> 
> 
> > 
> > 
> > Thanks for all the reviews,
> > Paulo
> > 
> > Paulo Zanoni (9):
> >   drm/i915: SAGV is not SKL-only, so rename a few things
> >   drm/i915: introduce intel_has_sagv()
> >   drm/i915/kbl: KBL also needs to run the SAGV code
> >   drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
> >   drm/i915/gen9: minimum scanlines for Y tile is not always 4
> >   drm/i915/gen9: fix plane_blocks_per_line on watermarks
> > calculations
> >   drm/i915/gen9: fix the watermark res_blocks value
> >   drm/i915/gen9: implement missing case for SKL watermarks
> > calculation
> >   drm/i915/gen9: fail the modeset instead of WARNing on unsuported
> >     config
> > 
> >  drivers/gpu/drm/i915/i915_drv.h      |  10 +-
> >  drivers/gpu/drm/i915/intel_display.c |   9 +-
> >  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
> >  drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++---
> > ------------
> >  4 files changed, 118 insertions(+), 93 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] SKL/KBL watermark fixes, v2

[Jani Nikula]

On Wed, 14 Sep 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote:
> Em Qua, 2016-09-14 às 12:34 +0300, Jani Nikula escreveu:
>> On Wed, 14 Sep 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
>> > 
>> > Hi
>> > 
>> > Here's the series with the reviews implemented. There's a new
>> > patch,
>> > based on the additional issue spotted by Lyude.
>> 
>> There's a bunch of cc: stable patches mixed with non cc: stable
>> patches
>> in the series. Do the cc: stable fixes work and backport cleanly
>> without
>> the the other non cc: stable patches? If not, can you arrange the
>> series
>> to not depend on the other patches?
>
> Yeah, my bad. I was just pasting the output of "dim fixes" without
> considering this aspect. I think the best thing is probably to backport
> everything to stable and hope it works, considering the current
> complaints we're seeing about screen flickering on SKL. Agree?

The short answer, agreed.

This probably needs actual backporting, i.e. doesn't cherry-pick
cleanly, and it could probably use some testing too. What happens if the
stable folks manage to cherry-pick some but not all the patches cleanly?

BR,
Jani.



>
> I suppose I don't need to resend the series just for these new tags.
> I'll add them when it's time to merge.
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> > 
>> > 
>> > Thanks for all the reviews,
>> > Paulo
>> > 
>> > Paulo Zanoni (9):
>> >   drm/i915: SAGV is not SKL-only, so rename a few things
>> >   drm/i915: introduce intel_has_sagv()
>> >   drm/i915/kbl: KBL also needs to run the SAGV code
>> >   drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
>> >   drm/i915/gen9: minimum scanlines for Y tile is not always 4
>> >   drm/i915/gen9: fix plane_blocks_per_line on watermarks
>> > calculations
>> >   drm/i915/gen9: fix the watermark res_blocks value
>> >   drm/i915/gen9: implement missing case for SKL watermarks
>> > calculation
>> >   drm/i915/gen9: fail the modeset instead of WARNing on unsuported
>> >     config
>> > 
>> >  drivers/gpu/drm/i915/i915_drv.h      |  10 +-
>> >  drivers/gpu/drm/i915/intel_display.c |   9 +-
>> >  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>> >  drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++---
>> > ------------
>> >  4 files changed, 118 insertions(+), 93 deletions(-)
>> 

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

Re: [PATCH 9/9] drm/i915/gen9: fail the modeset instead of WARNing on unsuported config

[Ville Syrjälä]

On Tue, Sep 13, 2016 at 09:38:22PM -0300, Paulo Zanoni wrote:
> Now that this code is part of the compute stage we can return -EINVAL
> to prevent the modeset instead of giving a WARN and trying anyway.
> 
> Reported-by: Lyude <cpaul@redhat.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9edc8ce..6394db7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3589,11 +3589,11 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  		case 2:
>  			y_min_scanlines = 8;
>  			break;
> -		default:
> -			WARN(1, "Unsupported pixel depth for rotation");
>  		case 4:
>  			y_min_scanlines = 4;
>  			break;
> +		default:
> +			return -EINVAL;

Considering this should never happen, MISSING_CASE() would be the
appropriate choice here.

>  		}
>  	} else {
>  		y_min_scanlines = 4;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Maarten Lankhorst]

Op 14-09-16 om 02:38 schreef 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.
>
> 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 fe43044..4d46315 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3504,7 +3504,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;
> @@ -3517,9 +3518,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 {
> @@ -3576,6 +3577,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;
> @@ -3591,38 +3593,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");
I wanted to comment that MISSING_CASE was appropriate here, but seems ville beat me to it on patch 9. :-)

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

[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

[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

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