[PATCH 00/11] More watermarks improvements

[PATCH 00/11] More watermarks improvements

[Paulo Zanoni]

Except for maybe patch 1, I don't believe this series will allow us to
close any real bugs, but at least it should make the code more
readable. Please notice that we removed more lines than we added :).

Thanks,
Paulo

Paulo Zanoni (11):
  drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
  drm/i915: remove padding from struct skl_wm_level
  drm/i915: fix handling of invisible planes in watermarks code
  drm/i915: remove useless memset() for watermarks parameters
  drm/i915: simplify wm->is_planar assignment
  drm/i915: refactor skl_write_plane_wm()
  drm/i915: move ddb_blocks to be a watermark parameter
  drm/i915: reorganize the error message for invalid watermarks
  drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
  drm/i915: add pipe_htotal to struct skl_wm_params
  drm/i915: pass dev_priv instead of cstate to
    skl_compute_transition_wm()

 drivers/gpu/drm/i915/i915_drv.h |   5 +-
 drivers/gpu/drm/i915/intel_pm.c | 198 ++++++++++++++++++----------------------
 2 files changed, 92 insertions(+), 111 deletions(-)

-- 
2.14.4

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

[PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Paulo Zanoni]

BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	res_lines = div_round_up_fixed16(selected_result,
 					 wp->plane_blocks_per_line);
 
-	/* Display WA #1125: skl,bxt,kbl,glk */
-	if (level == 0 && wp->rc_surface)
-		res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
+	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
+		/* Display WA #1125: skl,bxt,kbl */
+		if (level == 0 && wp->rc_surface)
+			res_blocks +=
+				fixed16_to_u32_round_up(wp->y_tile_minimum);
+
+		/* Display WA #1126: skl,bxt,kbl */
+		if (level >= 1 && level <= 7) {
+			if (wp->y_tiled) {
+				res_blocks +=
+				    fixed16_to_u32_round_up(wp->y_tile_minimum);
+				res_lines += wp->y_min_scanlines;
+			} else {
+				res_blocks++;
+			}
 
-	/* Display WA #1126: skl,bxt,kbl,glk */
-	if (level >= 1 && level <= 7) {
-		if (wp->y_tiled) {
-			res_blocks += fixed16_to_u32_round_up(
-							wp->y_tile_minimum);
-			res_lines += wp->y_min_scanlines;
-		} else {
-			res_blocks++;
+			/*
+			 * Make sure result blocks for higher latency levels are
+			 * atleast as high as level below the current level.
+			 * Assumption in DDB algorithm optimization for special
+			 * cases. Also covers Display WA #1125 for RC.
+			 */
+			if (result_prev->plane_res_b > res_blocks)
+				res_blocks = result_prev->plane_res_b;
 		}
-
-		/*
-		 * Make sure result blocks for higher latency levels are atleast
-		 * as high as level below the current level.
-		 * Assumption in DDB algorithm optimization for special cases.
-		 * Also covers Display WA #1125 for RC.
-		 */
-		if (result_prev->plane_res_b > res_blocks)
-			res_blocks = result_prev->plane_res_b;
 	}
 
 	if (INTEL_GEN(dev_priv) >= 11) {
-- 
2.14.4

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

[PATCH 02/11] drm/i915: remove padding from struct skl_wm_level

[Paulo Zanoni]

This reduces the size of struct skl_wm_level from 6 to 4, which
reduces the size of struct skl_plane_wm from 104 to 70, which reduces
the size of struct skl_pipe_wm from 524 to 356. A reduction of 168
padding bytes per pipe. This will increase even more the next time we
bump I915_MAX_PLANES.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3017ef037fed..3616b718b5d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1243,9 +1243,9 @@ struct skl_ddb_values {
 };
 
 struct skl_wm_level {
-	bool plane_en;
 	uint16_t plane_res_b;
 	uint8_t plane_res_l;
+	bool plane_en;
 };
 
 /* Stores plane specific WM parameters */
-- 
2.14.4

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

[PATCH 03/11] drm/i915: fix handling of invisible planes in watermarks code

[Paulo Zanoni]

Before the patch, if a plane was not visible,
skl_compute_plane_wm_params() would return early without writing
anything to the wm_params struct. This would leave garbage in the
struct since it is not previously zeroed, and then we would later
check for wm_params.is_planar, which could be true due to the usage of
uninitialized memory. This would lead us to calculate the zeroed
watermarks for the second inexistent plane and mark the plane as a
planar plane. Then later this check would affect our decisions in
skl_write_plane_wm().

I can't see how this would lead to a noticeable bug in our code, but
it leads us to calculate watermarks for every level + transition
watermarks, twice (due to the is_planar bug). So the fix saves us a
lot of instructions.

This problem was found when I decided to add a DRM_ERROR for the
currently unsupported planar formats on ICL: kms_cursor_legacy would
trigger the error message without using planar formats.

So the fix we adopt in this patch is to create a new watermark
parameter called plane_visible and use it to avoid computing the
watermarks for invisible planes later. We also remove the checks that
are now made redundant by it.

Testcase: igt/kms_cursor_legacy/nonblocking-modeset-vs-cursor-atomic
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++---------
 2 files changed, 7 insertions(+), 9 deletions(-)

The error message mentioned above isd the one added by patch 06 of the
series.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3616b718b5d2..4b1e8471609b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1250,6 +1250,7 @@ struct skl_wm_level {
 
 /* Stores plane specific WM parameters */
 struct skl_wm_params {
+	bool plane_visible;
 	bool x_tiled, y_tiled;
 	bool rc_surface;
 	bool is_planar;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 18157c6ee126..9043ffe40ce8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4532,7 +4532,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 		to_intel_atomic_state(cstate->base.state);
 	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
 
-	if (!intel_wm_plane_visible(cstate, intel_pstate))
+	wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
+	if (!wp->plane_visible)
 		return 0;
 
 	/* only NV12 format has two planes */
@@ -4645,8 +4646,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
 	uint32_t min_disp_buf_needed;
 
-	if (latency == 0 ||
-	    !intel_wm_plane_visible(cstate, intel_pstate)) {
+	if (latency == 0) {
 		result->plane_en = false;
 		return 0;
 	}
@@ -4805,9 +4805,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 	enum plane_id intel_plane_id = intel_plane->id;
 	int ret;
 
-	if (WARN_ON(!intel_pstate->base.fb))
-		return -EINVAL;
-
 	ddb_blocks = plane_id ?
 		     skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
 		     skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
@@ -4876,9 +4873,6 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 	const uint16_t trans_amount = 10; /* This is configurable amount */
 	uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
 
-	if (!cstate->base.active)
-		goto exit;
-
 	/* Transition WM are not recommended by HW team for GEN9 */
 	if (INTEL_GEN(dev_priv) <= 9)
 		goto exit;
@@ -4965,6 +4959,9 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 		if (ret)
 			return ret;
 
+		if (!wm_params.plane_visible)
+			continue;
+
 		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
 					    intel_pstate, &wm_params, wm, 0);
 		if (ret)
-- 
2.14.4

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

[PATCH 04/11] drm/i915: remove useless memset() for watermarks parameters

[Paulo Zanoni]

The skl_compute_plane_wm_params() already completely sets the contents
of its struct, or returns plane_visible=0 or returns an error code.
There's no need to memset() it at this point for the same reason we
don't zero-initialize it up when dealing with plane 0.

If we want to keep the memset "just to be safe", then we should also
zero initialize it when we use it for plane 0.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9043ffe40ce8..d1dd3ae408f9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4972,7 +4972,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 
 		/* uv plane watermarks must also be validated for NV12/Planar */
 		if (wm_params.is_planar) {
-			memset(&wm_params, 0, sizeof(struct skl_wm_params));
 			wm->is_planar = true;
 
 			ret = skl_compute_plane_wm_params(dev_priv, cstate,
-- 
2.14.4

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

[PATCH 05/11] drm/i915: simplify wm->is_planar assignment

[Paulo Zanoni]

We're currently doing it in two different ways, none of them based on
the wm_params struct. Both places are correct, so I chose to keep the
one in skl_compute_wm_levels() since it's the function that sets the
other values for the same struct. But I'm open to better suggestions
on the place to assign it.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d1dd3ae408f9..7fd344b81d66 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4832,8 +4832,7 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 			return ret;
 	}
 
-	if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
-		wm->is_planar = true;
+	wm->is_planar = wm_params->is_planar;
 
 	return 0;
 }
@@ -4972,8 +4971,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 
 		/* uv plane watermarks must also be validated for NV12/Planar */
 		if (wm_params.is_planar) {
-			wm->is_planar = true;
-
 			ret = skl_compute_plane_wm_params(dev_priv, cstate,
 							  intel_pstate,
 							  &wm_params, 1);
-- 
2.14.4

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

[PATCH 06/11] drm/i915: refactor skl_write_plane_wm()

[Paulo Zanoni]

Its control flow is not as easy to follow as it could be. We recently
even had a double register write that went unnoticed until commit
9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")
fixed it. The return statement in the middle along with the fact that
it's on a void function really doesn't help readability IMHO.

Refactor the function so that the first level of checks is per
platform and the second level is for planar planes. IMHO that makes
the code much more readable.

Requested-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7fd344b81d66..f388bfa99a97 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5033,21 +5033,28 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
 			   &wm->trans_wm);
 
-	/* FIXME: add proper NV12 support for ICL. */
-	if (INTEL_GEN(dev_priv) >= 11)
-		return skl_ddb_entry_write(dev_priv,
-					   PLANE_BUF_CFG(pipe, plane_id),
-					   &ddb->plane[pipe][plane_id]);
-	if (wm->is_planar) {
-		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
-				    &ddb->uv_plane[pipe][plane_id]);
+	if (INTEL_GEN(dev_priv) >= 11) {
+		/* FIXME: add proper NV12 support for ICL. */
+		if (wm->is_planar)
+			DRM_ERROR("No DDB support for planar formats yet\n");
+
 		skl_ddb_entry_write(dev_priv,
-				    PLANE_NV12_BUF_CFG(pipe, plane_id),
-				    &ddb->plane[pipe][plane_id]);
+				   PLANE_BUF_CFG(pipe, plane_id),
+				   &ddb->plane[pipe][plane_id]);
 	} else {
-		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
-				    &ddb->plane[pipe][plane_id]);
-		I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
+		if (wm->is_planar) {
+			skl_ddb_entry_write(dev_priv,
+					    PLANE_BUF_CFG(pipe, plane_id),
+					    &ddb->uv_plane[pipe][plane_id]);
+			skl_ddb_entry_write(dev_priv,
+					    PLANE_NV12_BUF_CFG(pipe, plane_id),
+					    &ddb->plane[pipe][plane_id]);
+		} else {
+			skl_ddb_entry_write(dev_priv,
+					    PLANE_BUF_CFG(pipe, plane_id),
+					    &ddb->plane[pipe][plane_id]);
+			I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
+		}
 	}
 }
 
-- 
2.14.4

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

[PATCH 07/11] drm/i915: move ddb_blocks to be a watermark parameter

[Paulo Zanoni]

The goal of struct skl_wm_params is to cache every watermark
parameter so the other functions can just use them without worrying
about the appropriate place to fetch each parameter requested by the
spec, and without having to recompute parameters that are used in
different steps of the calculation.

The ddb_blocks parameter is one that is used by both the the plane
watermarks and the transition watermarks. Move ddb_blocks to the
parameter struct so we can simplify the code.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++-------------------------
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4b1e8471609b..b32d680d9bf0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1255,6 +1255,7 @@ struct skl_wm_params {
 	bool rc_surface;
 	bool is_planar;
 	uint32_t width;
+	uint16_t ddb_blocks;
 	uint8_t cpp;
 	uint32_t plane_pixel_rate;
 	uint32_t y_min_scanlines;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f388bfa99a97..4053f4a68657 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4522,11 +4522,13 @@ static int
 skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 			    struct intel_crtc_state *cstate,
 			    const struct intel_plane_state *intel_pstate,
+			    const struct skl_ddb_allocation *ddb,
 			    struct skl_wm_params *wp, int plane_id)
 {
 	struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
 	const struct drm_plane_state *pstate = &intel_pstate->base;
 	const struct drm_framebuffer *fb = pstate->fb;
+	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
 	uint32_t interm_pbpl;
 	struct intel_atomic_state *state =
 		to_intel_atomic_state(cstate->base.state);
@@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 	wp->linetime_us = fixed16_to_u32_round_up(
 					intel_get_linetime_us(cstate));
 
+	wp->ddb_blocks = plane_id ?
+		     skl_ddb_entry_size(&ddb->uv_plane[pipe][plane->id]) :
+		     skl_ddb_entry_size(&ddb->plane[pipe][plane->id]);
+
 	return 0;
 }
 
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_crtc_state *cstate,
 				const struct intel_plane_state *intel_pstate,
-				uint16_t ddb_allocation,
 				int level,
 				const struct skl_wm_params *wp,
 				const struct skl_wm_level *result_prev,
@@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		     wp->dbuf_block_size < 1) &&
 		     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
 			selected_result = method2;
-		} else if (ddb_allocation >=
+		} else if (wp->ddb_blocks >=
 			 fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
 			if (INTEL_GEN(dev_priv) == 9 &&
 			    !IS_GEMINILAKE(dev_priv))
@@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	}
 
 	if ((level > 0 && res_lines > 31) ||
-	    res_blocks >= ddb_allocation ||
-	    min_disp_buf_needed >= ddb_allocation) {
+	    res_blocks >= wp->ddb_blocks ||
+	    min_disp_buf_needed >= wp->ddb_blocks) {
 		result->plane_en = false;
 
 		/*
@@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
 			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
 				      plane->base.id, plane->name,
-				      res_blocks, ddb_allocation, res_lines);
+				      res_blocks, wp->ddb_blocks, res_lines);
 			return -EINVAL;
 		}
 	}
@@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 static int
 skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
-		      struct skl_ddb_allocation *ddb,
 		      struct intel_crtc_state *cstate,
 		      const struct intel_plane_state *intel_pstate,
 		      const struct skl_wm_params *wm_params,
 		      struct skl_plane_wm *wm,
 		      int plane_id)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_plane *plane = intel_pstate->base.plane;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	uint16_t ddb_blocks;
-	enum pipe pipe = intel_crtc->pipe;
 	int level, max_level = ilk_wm_max_level(dev_priv);
-	enum plane_id intel_plane_id = intel_plane->id;
 	int ret;
 
-	ddb_blocks = plane_id ?
-		     skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
-		     skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
-
 	for (level = 0; level <= max_level; level++) {
 		struct skl_wm_level *result = plane_id ? &wm->uv_wm[level] :
 							  &wm->wm[level];
@@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 		ret = skl_compute_plane_wm(dev_priv,
 					   cstate,
 					   intel_pstate,
-					   ddb_blocks,
 					   level,
 					   wm_params,
 					   result_prev,
@@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
 static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 				      struct skl_wm_params *wp,
 				      struct skl_wm_level *wm_l0,
-				      uint16_t ddb_allocation,
 				      struct skl_wm_level *trans_wm /* out */)
 {
 	struct drm_device *dev = cstate->base.crtc->dev;
@@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 
 	res_blocks += 1;
 
-	if (res_blocks < ddb_allocation) {
+	if (res_blocks < wp->ddb_blocks) {
 		trans_wm->plane_res_b = res_blocks;
 		trans_wm->plane_en = true;
 		return;
@@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 						to_intel_plane_state(pstate);
 		enum plane_id plane_id = to_intel_plane(plane)->id;
 		struct skl_wm_params wm_params;
-		enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
-		uint16_t ddb_blocks;
 
 		wm = &pipe_wm->planes[plane_id];
-		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
 
 		ret = skl_compute_plane_wm_params(dev_priv, cstate,
-						  intel_pstate, &wm_params, 0);
+						  intel_pstate, ddb,
+						  &wm_params, 0);
 		if (ret)
 			return ret;
 
 		if (!wm_params.plane_visible)
 			continue;
 
-		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
+		ret = skl_compute_wm_levels(dev_priv, cstate,
 					    intel_pstate, &wm_params, wm, 0);
 		if (ret)
 			return ret;
 
 		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
-					  ddb_blocks, &wm->trans_wm);
+					  &wm->trans_wm);
 
 		/* uv plane watermarks must also be validated for NV12/Planar */
 		if (wm_params.is_planar) {
 			ret = skl_compute_plane_wm_params(dev_priv, cstate,
-							  intel_pstate,
+							  intel_pstate, ddb,
 							  &wm_params, 1);
 			if (ret)
 				return ret;
 
-			ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
+			ret = skl_compute_wm_levels(dev_priv, cstate,
 						    intel_pstate, &wm_params,
 						    wm, 1);
 			if (ret)
-- 
2.14.4

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

[PATCH 08/11] drm/i915: reorganize the error message for invalid watermarks

[Paulo Zanoni]

Print a more generic "failed to compute watermark levels" whenever any
of skl_compute_wm_levels() fail, and print only the specific error
message for the specific cases. This allows us to stop passing pstate
everywhere, making the watermarks computation code a little less
dependent on random intel state structs.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4053f4a68657..1290efc64869 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_crtc_state *cstate,
-				const struct intel_plane_state *intel_pstate,
 				int level,
 				const struct skl_wm_params *wp,
 				const struct skl_wm_level *result_prev,
 				struct skl_wm_level *result /* out */)
 {
-	const struct drm_plane_state *pstate = &intel_pstate->base;
 	uint32_t latency = dev_priv->wm.skl_latency[level];
 	uint_fixed_16_16_t method1, method2;
 	uint_fixed_16_16_t selected_result;
@@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		if (level) {
 			return 0;
 		} else {
-			struct drm_plane *plane = pstate->plane;
-
-			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
-			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
-				      plane->base.id, plane->name,
+			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n",
 				      res_blocks, wp->ddb_blocks, res_lines);
 			return -EINVAL;
 		}
@@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 static int
 skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 		      struct intel_crtc_state *cstate,
-		      const struct intel_plane_state *intel_pstate,
 		      const struct skl_wm_params *wm_params,
 		      struct skl_plane_wm *wm,
 		      int plane_id)
@@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 
 		ret = skl_compute_plane_wm(dev_priv,
 					   cstate,
-					   intel_pstate,
 					   level,
 					   wm_params,
 					   result_prev,
@@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 		if (!wm_params.plane_visible)
 			continue;
 
-		ret = skl_compute_wm_levels(dev_priv, cstate,
-					    intel_pstate, &wm_params, wm, 0);
-		if (ret)
+		ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
+					    0);
+		if (ret) {
+			DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
+				      plane->base.id, plane->name);
 			return ret;
+		}
 
 		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
 					  &wm->trans_wm);
@@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 				return ret;
 
 			ret = skl_compute_wm_levels(dev_priv, cstate,
-						    intel_pstate, &wm_params,
-						    wm, 1);
-			if (ret)
+						    &wm_params, wm, 1);
+			if (ret) {
+				DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
+					      plane->base.id, plane->name);
 				return ret;
+			}
 		}
 	}
 
-- 
2.14.4

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

[PATCH 09/11] drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state

[Paulo Zanoni]

The function only really needs dev_priv to make its decision. If we
ever need more, we can change it again. But then, in this case we
should make needs_memory_bw_wa be a variable inside struct
skl_wm_params so we won't need to keep passing intel states deep
inside pure watermark value calculation functions.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1290efc64869..d101c542f10d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3599,10 +3599,8 @@ static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
  * FIXME: We still don't have the proper code detect if we need to apply the WA,
  * so assume we'll always need it in order to avoid underruns.
  */
-static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
+static bool skl_needs_memory_bw_wa(const struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-
 	if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv))
 		return true;
 
@@ -3765,7 +3763,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 
 		latency = dev_priv->wm.skl_latency[level];
 
-		if (skl_needs_memory_bw_wa(intel_state) &&
+		if (skl_needs_memory_bw_wa(dev_priv) &&
 		    plane->base.state->fb->modifier ==
 		    I915_FORMAT_MOD_X_TILED)
 			latency += 15;
@@ -4530,9 +4528,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 	const struct drm_framebuffer *fb = pstate->fb;
 	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
 	uint32_t interm_pbpl;
-	struct intel_atomic_state *state =
-		to_intel_atomic_state(cstate->base.state);
-	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
 
 	wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
 	if (!wp->plane_visible)
@@ -4644,9 +4640,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint_fixed_16_16_t method1, method2;
 	uint_fixed_16_16_t selected_result;
 	uint32_t res_blocks, res_lines;
-	struct intel_atomic_state *state =
-		to_intel_atomic_state(cstate->base.state);
-	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
 	uint32_t min_disp_buf_needed;
 
 	if (latency == 0) {
-- 
2.14.4

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

[PATCH 10/11] drm/i915: add pipe_htotal to struct skl_wm_params

[Paulo Zanoni]

With this one here we can finally drop the intel state structures from
the functions that compute watermark values: they all rely on struct
skl_wm_params now. This should help the watermarks code be a little
more clear on its intent and also match the spec a little bit more
with the carefully chosen names for its parameters.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++------------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b32d680d9bf0..4712eaea9744 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1254,6 +1254,7 @@ struct skl_wm_params {
 	bool x_tiled, y_tiled;
 	bool rc_surface;
 	bool is_planar;
+	uint32_t pipe_htotal;
 	uint32_t width;
 	uint16_t ddb_blocks;
 	uint8_t cpp;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d101c542f10d..b01f3d807ff6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4549,6 +4549,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 			 fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
 	wp->is_planar = fb->format->format == DRM_FORMAT_NV12;
 
+	wp->pipe_htotal = cstate->base.adjusted_mode.crtc_htotal;
+
 	if (plane->id == PLANE_CURSOR) {
 		wp->width = intel_pstate->base.crtc_w;
 	} else {
@@ -4630,7 +4632,6 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
 }
 
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
-				struct intel_crtc_state *cstate,
 				int level,
 				const struct skl_wm_params *wp,
 				const struct skl_wm_level *result_prev,
@@ -4659,17 +4660,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
 				 wp->cpp, latency, wp->dbuf_block_size);
-	method2 = skl_wm_method2(wp->plane_pixel_rate,
-				 cstate->base.adjusted_mode.crtc_htotal,
+	method2 = skl_wm_method2(wp->plane_pixel_rate, wp->pipe_htotal,
 				 latency,
 				 wp->plane_blocks_per_line);
 
 	if (wp->y_tiled) {
 		selected_result = max_fixed16(method2, wp->y_tile_minimum);
 	} else {
-		if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
-		     wp->dbuf_block_size < 1) &&
-		     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
+		if ((wp->cpp * wp->pipe_htotal / wp->dbuf_block_size < 1) &&
+		    (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
 			selected_result = method2;
 		} else if (wp->ddb_blocks >=
 			 fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
@@ -4782,7 +4781,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 static int
 skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
-		      struct intel_crtc_state *cstate,
 		      const struct skl_wm_params *wm_params,
 		      struct skl_plane_wm *wm,
 		      int plane_id)
@@ -4802,7 +4800,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 			result_prev = plane_id ? &wm->uv_wm[0] : &wm->wm[0];
 
 		ret = skl_compute_plane_wm(dev_priv,
-					   cstate,
 					   level,
 					   wm_params,
 					   result_prev,
@@ -4937,8 +4934,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 		if (!wm_params.plane_visible)
 			continue;
 
-		ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
-					    0);
+		ret = skl_compute_wm_levels(dev_priv, &wm_params, wm, 0);
 		if (ret) {
 			DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
 				      plane->base.id, plane->name);
@@ -4956,8 +4952,8 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 			if (ret)
 				return ret;
 
-			ret = skl_compute_wm_levels(dev_priv, cstate,
-						    &wm_params, wm, 1);
+			ret = skl_compute_wm_levels(dev_priv, &wm_params, wm,
+						    1);
 			if (ret) {
 				DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
 					      plane->base.id, plane->name);
-- 
2.14.4

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

[PATCH 11/11] drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()

[Paulo Zanoni]

Stop passing modeset state structures to functions that should work
only with the skl_wm_params. The only use for cstate there was to
reach dev_priv, so pass it directly.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b01f3d807ff6..dac2613aaaa3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4836,13 +4836,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
 	return linetime_wm;
 }
 
-static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
+static void skl_compute_transition_wm(const struct drm_i915_private *dev_priv,
 				      struct skl_wm_params *wp,
 				      struct skl_wm_level *wm_l0,
 				      struct skl_wm_level *trans_wm /* out */)
 {
-	struct drm_device *dev = cstate->base.crtc->dev;
-	const struct drm_i915_private *dev_priv = to_i915(dev);
 	uint16_t trans_min, trans_y_tile_min;
 	const uint16_t trans_amount = 10; /* This is configurable amount */
 	uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
@@ -4941,7 +4939,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 			return ret;
 		}
 
-		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
+		skl_compute_transition_wm(dev_priv, &wm_params, &wm->wm[0],
 					  &wm->trans_wm);
 
 		/* uv plane watermarks must also be validated for NV12/Planar */
-- 
2.14.4

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

✗ Fi.CI.CHECKPATCH: warning for More watermarks improvements

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL   : https://patchwork.freedesktop.org/series/51086/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
6165f4257098 drm/i915: remove padding from struct skl_wm_level
-:25: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#25: FILE: drivers/gpu/drm/i915/i915_drv.h:1248:
+	bool plane_en;

total: 0 errors, 0 warnings, 1 checks, 10 lines checked
7d4eced125cf drm/i915: fix handling of invisible planes in watermarks code
-:41: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#41: FILE: drivers/gpu/drm/i915/i915_drv.h:1253:
+	bool plane_visible;

total: 0 errors, 0 warnings, 1 checks, 52 lines checked
20ce35c3029a drm/i915: remove useless memset() for watermarks parameters
10134ecdb23d drm/i915: simplify wm->is_planar assignment
b6f917785676 drm/i915: refactor skl_write_plane_wm()
-:8: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")'
#8: 
9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")

-:16: WARNING:BAD_SIGN_OFF: Non-standard signature: Requested-by:
#16: 
Requested-by: Matt Roper <matthew.d.roper@intel.com>

total: 1 errors, 1 warnings, 0 checks, 41 lines checked
52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
c801352ac21a drm/i915: reorganize the error message for invalid watermarks
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()

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

✗ Fi.CI.SPARSE: warning for More watermarks improvements

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL   : https://patchwork.freedesktop.org/series/51086/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
Okay!

Commit: drm/i915: remove padding from struct skl_wm_level
Okay!

Commit: drm/i915: fix handling of invisible planes in watermarks code
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3725:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3726:16: warning: expression using sizeof(void)

Commit: drm/i915: remove useless memset() for watermarks parameters
Okay!

Commit: drm/i915: simplify wm->is_planar assignment
Okay!

Commit: drm/i915: refactor skl_write_plane_wm()
Okay!

Commit: drm/i915: move ddb_blocks to be a watermark parameter
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3726:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3727:16: warning: expression using sizeof(void)

Commit: drm/i915: reorganize the error message for invalid watermarks
Okay!

Commit: drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
Okay!

Commit: drm/i915: add pipe_htotal to struct skl_wm_params
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3727:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3728:16: warning: expression using sizeof(void)

Commit: drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()
Okay!

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

Re: ✗ Fi.CI.CHECKPATCH: warning for More watermarks improvements

[Paulo Zanoni]

Em Ter, 2018-10-16 às 22:21 +0000, Patchwork escreveu:
> == Series Details ==
> 
> Series: More watermarks improvements
> URL   : https://patchwork.freedesktop.org/series/51086/
> State : warning
> 
> == Summary ==
> 
> $ dim checkpatch origin/drm-tip
> 6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to
> GLK/CNL+
> 6165f4257098 drm/i915: remove padding from struct skl_wm_level
> -:25: CHECK:BOOL_MEMBER: Avoid using bool structure members because
> of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/
> 384
> #25: FILE: drivers/gpu/drm/i915/i915_drv.h:1248:
> +	bool plane_en;
> 
> total: 0 errors, 0 warnings, 1 checks, 10 lines checked

That won't save us anything in this specific case, and it's not our
usual coding standard. If we conclude i915.ko should go this way, I'd
be happy to comply. Maintainers?

> 7d4eced125cf drm/i915: fix handling of invisible planes in watermarks
> code
> -:41: CHECK:BOOL_MEMBER: Avoid using bool structure members because
> of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/
> 384
> #41: FILE: drivers/gpu/drm/i915/i915_drv.h:1253:
> +	bool plane_visible;

The rest of the struct also uses bools like this. If we go this route,
we should probably change the whole struct.

> 
> total: 0 errors, 0 warnings, 1 checks, 52 lines checked
> 20ce35c3029a drm/i915: remove useless memset() for watermarks
> parameters
> 10134ecdb23d drm/i915: simplify wm->is_planar assignment
> b6f917785676 drm/i915: refactor skl_write_plane_wm()
> -:8: ERROR:GIT_COMMIT_ID: Please use git commit description style
> 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit
> 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every
> time")'
> #8: 
> 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")

Well, I used that description style, but it's in the middle of a
paragraph so the word "commit" is in the line above. If that's not what
you're asking, then I don't know.


> 
> -:16: WARNING:BAD_SIGN_OFF: Non-standard signature: Requested-by:
> #16: 
> Requested-by: Matt Roper <matthew.d.roper@intel.com>

Is this really undesirable?


> 
> total: 1 errors, 1 warnings, 0 checks, 41 lines checked
> 52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
> c801352ac21a drm/i915: reorganize the error message for invalid
> watermarks
> e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv
> instead of state
> 03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
> 4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to
> skl_compute_transition_wm()
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for More watermarks improvements

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL   : https://patchwork.freedesktop.org/series/51086/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4990 -> Patchwork_10481 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10481 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10481, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51086/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10481:

  === IGT changes ===

    ==== Possible regressions ====

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       PASS -> DMESG-FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_10481 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107859, fdo#107774, fdo#107556)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000, fdo#106097)

    igt@kms_flip@basic-plain-flip:
      fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387) +2

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-icl-u2:          INCOMPLETE (fdo#108315) -> PASS

    igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
      fi-glk-j4005:       FAIL (fdo#106765) -> PASS

    igt@kms_flip@basic-flip-vs-dpms:
      fi-hsw-4770r:       DMESG-WARN (fdo#105602) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-icl-u2:          FAIL (fdo#103167) -> PASS
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
  fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
  fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315


== Participating hosts (46 -> 38) ==

  Additional (1): fi-kbl-soraka 
  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-pnv-d510 fi-kbl-7560u 


== Build changes ==

    * Linux: CI_DRM_4990 -> Patchwork_10481

  CI_DRM_4990: 0bd34d92e9a27784cb988a300619f497ca0e99ec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4681: 959d6f95cb1344e0c0dace5b236e17755826fac1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10481: 4a4d7716aeaa870a7d0aaa1e541706cb919096cd @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
c801352ac21a drm/i915: reorganize the error message for invalid watermarks
52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
b6f917785676 drm/i915: refactor skl_write_plane_wm()
10134ecdb23d drm/i915: simplify wm->is_planar assignment
20ce35c3029a drm/i915: remove useless memset() for watermarks parameters
7d4eced125cf drm/i915: fix handling of invisible planes in watermarks code
6165f4257098 drm/i915: remove padding from struct skl_wm_level
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10481/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for More watermarks improvements

[Paulo Zanoni]

Em Ter, 2018-10-16 às 22:39 +0000, Patchwork escreveu:
> == Series Details ==
> 
> Series: More watermarks improvements
> URL   : https://patchwork.freedesktop.org/series/51086/
> State : failure
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4990 -> Patchwork_10481 =
> 
> == Summary - FAILURE ==
> 
>   Serious unknown changes coming with Patchwork_10481 absolutely need
> to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the
> changes
>   introduced in Patchwork_10481, please notify your bug team to allow
> them
>   to document this new failure mode, which will reduce false
> positives in CI.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/5108
> 6/revisions/1/mbox/
> 
> == Possible new issues ==
> 
>   Here are the unknown changes that may have been introduced in
> Patchwork_10481:
> 
>   === IGT changes ===
> 
>     ==== Possible regressions ====
> 
>     igt@pm_rpm@module-reload:
>       fi-glk-j4005:       PASS -> DMESG-FAIL


EDID shows invalid data after module reload, triggering error during
mode comparison in the subtest. Hard to think how this series could
cause that.

> 
>     
> == Known issues ==
> 
>   Here are the changes found in Patchwork_10481 that come from known
> issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@gem_exec_suspend@basic-s3:
>       fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107859,
> fdo#107774, fdo#107556)
> 
>     igt@kms_flip@basic-flip-vs-dpms:
>       fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000, fdo#106097)
> 
>     igt@kms_flip@basic-plain-flip:
>       fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387) +2
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
>       fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)
> 
>     
>     ==== Possible fixes ====
> 
>     igt@drv_module_reload@basic-reload:
>       fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> PASS
> 
>     igt@drv_selftest@live_hangcheck:
>       fi-icl-u2:          INCOMPLETE (fdo#108315) -> PASS
> 
>     igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
>       fi-glk-j4005:       FAIL (fdo#106765) -> PASS
> 
>     igt@kms_flip@basic-flip-vs-dpms:
>       fi-hsw-4770r:       DMESG-WARN (fdo#105602) -> PASS
> 
>     igt@kms_flip@basic-flip-vs-wf_vblank:
>       fi-glk-j4005:       FAIL (fdo#100368) -> PASS
> 
>     igt@kms_flip@basic-plain-flip:
>       fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS
> 
>     igt@kms_frontbuffer_tracking@basic:
>       fi-icl-u2:          FAIL (fdo#103167) -> PASS
>       fi-byt-clapper:     FAIL (fdo#103167) -> PASS
> 
>     
>   fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
>   fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
>   fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
>   fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
>   fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
>   fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
>   fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
>   fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
>   fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
>   fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
>   fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
>   fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
>   fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
>   fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
> 
> 
> == Participating hosts (46 -> 38) ==
> 
>   Additional (1): fi-kbl-soraka 
>   Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-skl-guc
> fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-pnv-d510 fi-kbl-7560u 
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_4990 -> Patchwork_10481
> 
>   CI_DRM_4990: 0bd34d92e9a27784cb988a300619f497ca0e99ec @
> git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4681: 959d6f95cb1344e0c0dace5b236e17755826fac1 @
> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_10481: 4a4d7716aeaa870a7d0aaa1e541706cb919096cd @
> git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to
> skl_compute_transition_wm()
> 03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
> e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv
> instead of state
> c801352ac21a drm/i915: reorganize the error message for invalid
> watermarks
> 52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
> b6f917785676 drm/i915: refactor skl_write_plane_wm()
> 10134ecdb23d drm/i915: simplify wm->is_planar assignment
> 20ce35c3029a drm/i915: remove useless memset() for watermarks
> parameters
> 7d4eced125cf drm/i915: fix handling of invisible planes in watermarks
> code
> 6165f4257098 drm/i915: remove padding from struct skl_wm_level
> 6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to
> GLK/CNL+
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw
> ork_10481/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/11] drm/i915: remove padding from struct skl_wm_level

[Lucas De Marchi]

On Tue, Oct 16, 2018 at 03:01:24PM -0700, Paulo Zanoni wrote:
> This reduces the size of struct skl_wm_level from 6 to 4, which
> reduces the size of struct skl_plane_wm from 104 to 70, which reduces
> the size of struct skl_pipe_wm from 524 to 356. A reduction of 168
> padding bytes per pipe. This will increase even more the next time we
> bump I915_MAX_PLANES.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

For completeness:

$ pahole -s -C skl_wm_level drivers/gpu/drm/i915/i915.o
struct skl_wm_level {
	bool                       plane_en;             /*     0     1 */

	/* XXX 1 byte hole, try to pack */

	uint16_t                   plane_res_b;          /*     2     2 */
	uint8_t                    plane_res_l;          /*     4     1 */

	/* size: 6, cachelines: 1, members: 3 */
	/* sum members: 4, holes: 1, sum holes: 1 */
	/* padding: 1 */
	/* last cacheline: 6 bytes */
};

So yes, this removes the padding and hole that are there due to alignment.

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3017ef037fed..3616b718b5d2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1243,9 +1243,9 @@ struct skl_ddb_values {
>  };
>  
>  struct skl_wm_level {
> -	bool plane_en;
>  	uint16_t plane_res_b;
>  	uint8_t plane_res_l;
> +	bool plane_en;
>  };
>  
>  /* Stores plane specific WM parameters */
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
> BSpec does not show these WAs as applicable to GLK, and for CNL it
> only shows them applicable for a super early pre-production stepping
> we shouldn't be caring about anymore. Remove these so we can avoid
> them on ICL too.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 67a4d0735291..18157c6ee126 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	res_lines = div_round_up_fixed16(selected_result,
>  					 wp->plane_blocks_per_line);
>  
> -	/* Display WA #1125: skl,bxt,kbl,glk */
> -	if (level == 0 && wp->rc_surface)
> -		res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
> +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT

would be a little easier to parse, me thinks.

> +		/* Display WA #1125: skl,bxt,kbl */
> +		if (level == 0 && wp->rc_surface)
> +			res_blocks +=
> +				fixed16_to_u32_round_up(wp->y_tile_minimum);
> +
> +		/* Display WA #1126: skl,bxt,kbl */
> +		if (level >= 1 && level <= 7) {
> +			if (wp->y_tiled) {
> +				res_blocks +=
> +				    fixed16_to_u32_round_up(wp->y_tile_minimum);
> +				res_lines += wp->y_min_scanlines;
> +			} else {
> +				res_blocks++;
> +			}
>  
> -	/* Display WA #1126: skl,bxt,kbl,glk */
> -	if (level >= 1 && level <= 7) {
> -		if (wp->y_tiled) {
> -			res_blocks += fixed16_to_u32_round_up(
> -							wp->y_tile_minimum);
> -			res_lines += wp->y_min_scanlines;
> -		} else {
> -			res_blocks++;
> +			/*
> +			 * Make sure result blocks for higher latency levels are
> +			 * atleast as high as level below the current level.
> +			 * Assumption in DDB algorithm optimization for special
> +			 * cases. Also covers Display WA #1125 for RC.
> +			 */
> +			if (result_prev->plane_res_b > res_blocks)
> +				res_blocks = result_prev->plane_res_b;
>  		}
> -
> -		/*
> -		 * Make sure result blocks for higher latency levels are atleast
> -		 * as high as level below the current level.
> -		 * Assumption in DDB algorithm optimization for special cases.
> -		 * Also covers Display WA #1125 for RC.
> -		 */
> -		if (result_prev->plane_res_b > res_blocks)
> -			res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed. One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to see if
that's true.

Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.

We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

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


>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 03/11] drm/i915: fix handling of invisible planes in watermarks code

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:25PM -0700, Paulo Zanoni wrote:
> Before the patch, if a plane was not visible,
> skl_compute_plane_wm_params() would return early without writing
> anything to the wm_params struct. This would leave garbage in the
> struct since it is not previously zeroed, and then we would later
> check for wm_params.is_planar, which could be true due to the usage of
> uninitialized memory. This would lead us to calculate the zeroed
> watermarks for the second inexistent plane and mark the plane as a
> planar plane. Then later this check would affect our decisions in
> skl_write_plane_wm().
> 
> I can't see how this would lead to a noticeable bug in our code, but
> it leads us to calculate watermarks for every level + transition
> watermarks, twice (due to the is_planar bug). So the fix saves us a
> lot of instructions.
> 
> This problem was found when I decided to add a DRM_ERROR for the
> currently unsupported planar formats on ICL: kms_cursor_legacy would
> trigger the error message without using planar formats.
> 
> So the fix we adopt in this patch is to create a new watermark
> parameter called plane_visible and use it to avoid computing the
> watermarks for invisible planes later. We also remove the checks that
> are now made redundant by it.
> 
> Testcase: igt/kms_cursor_legacy/nonblocking-modeset-vs-cursor-atomic
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++---------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> The error message mentioned above isd the one added by patch 06 of the
> series.
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3616b718b5d2..4b1e8471609b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1250,6 +1250,7 @@ struct skl_wm_level {
>  
>  /* Stores plane specific WM parameters */
>  struct skl_wm_params {
> +	bool plane_visible;

>  	bool x_tiled, y_tiled;
>  	bool rc_surface;
>  	bool is_planar;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 18157c6ee126..9043ffe40ce8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4532,7 +4532,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  		to_intel_atomic_state(cstate->base.state);
>  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
>  
> -	if (!intel_wm_plane_visible(cstate, intel_pstate))
> +	wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
> +	if (!wp->plane_visible)
>  		return 0;
>  
>  	/* only NV12 format has two planes */
> @@ -4645,8 +4646,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
>  	uint32_t min_disp_buf_needed;
>  
> -	if (latency == 0 ||
> -	    !intel_wm_plane_visible(cstate, intel_pstate)) {
> +	if (latency == 0) {
>  		result->plane_en = false;
>  		return 0;
>  	}
> @@ -4805,9 +4805,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
>  	enum plane_id intel_plane_id = intel_plane->id;
>  	int ret;
>  
> -	if (WARN_ON(!intel_pstate->base.fb))
> -		return -EINVAL;
> -
>  	ddb_blocks = plane_id ?
>  		     skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
>  		     skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
> @@ -4876,9 +4873,6 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>  	const uint16_t trans_amount = 10; /* This is configurable amount */
>  	uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
>  
> -	if (!cstate->base.active)
> -		goto exit;
> -
>  	/* Transition WM are not recommended by HW team for GEN9 */
>  	if (INTEL_GEN(dev_priv) <= 9)
>  		goto exit;
> @@ -4965,6 +4959,9 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  		if (ret)
>  			return ret;
>  
> +		if (!wm_params.plane_visible)
> +			continue;
> +

So this will now skip both skl_compute_wm_levels() and
skl_compute_transition_wm() for this plane, and we've
zeroed the entire pipe_wm->planes thing earlier.
Seems good.

Since we're only checking this in this one place I don't
think we really need to add wm_params.plane_visible. Could
just do the intel_wm_plane_visible() check right here no?

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

>  		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
>  					    intel_pstate, &wm_params, wm, 0);
>  		if (ret)
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 04/11] drm/i915: remove useless memset() for watermarks parameters

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:26PM -0700, Paulo Zanoni wrote:
> The skl_compute_plane_wm_params() already completely sets the contents
> of its struct, or returns plane_visible=0 or returns an error code.
> There's no need to memset() it at this point for the same reason we
> don't zero-initialize it up when dealing with plane 0.

If old garbage in the struct is OK for the luma plane then it
should be OK for chroma as well.

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

> 
> If we want to keep the memset "just to be safe", then we should also
> zero initialize it when we use it for plane 0.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9043ffe40ce8..d1dd3ae408f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4972,7 +4972,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  
>  		/* uv plane watermarks must also be validated for NV12/Planar */
>  		if (wm_params.is_planar) {
> -			memset(&wm_params, 0, sizeof(struct skl_wm_params));
>  			wm->is_planar = true;
>  
>  			ret = skl_compute_plane_wm_params(dev_priv, cstate,
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 05/11] drm/i915: simplify wm->is_planar assignment

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:27PM -0700, Paulo Zanoni wrote:
> We're currently doing it in two different ways, none of them based on
> the wm_params struct. Both places are correct, so I chose to keep the
> one in skl_compute_wm_levels() since it's the function that sets the
> other values for the same struct. But I'm open to better suggestions
> on the place to assign it.

I'm not a huge fan of the entire wm_params thing. Lots of
duplicated infromation for dubious gain. But given that we have
it I think this patch is fine.

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

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d1dd3ae408f9..7fd344b81d66 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4832,8 +4832,7 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
>  			return ret;
>  	}
>  
> -	if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
> -		wm->is_planar = true;
> +	wm->is_planar = wm_params->is_planar;
>  
>  	return 0;
>  }
> @@ -4972,8 +4971,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  
>  		/* uv plane watermarks must also be validated for NV12/Planar */
>  		if (wm_params.is_planar) {
> -			wm->is_planar = true;
> -
>  			ret = skl_compute_plane_wm_params(dev_priv, cstate,
>  							  intel_pstate,
>  							  &wm_params, 1);
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 06/11] drm/i915: refactor skl_write_plane_wm()

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:28PM -0700, Paulo Zanoni wrote:
> Its control flow is not as easy to follow as it could be. We recently
> even had a double register write that went unnoticed until commit
> 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")
> fixed it. The return statement in the middle along with the fact that
> it's on a void function really doesn't help readability IMHO.
> 
> Refactor the function so that the first level of checks is per
> platform and the second level is for planar planes. IMHO that makes
> the code much more readable.
> 
> Requested-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7fd344b81d66..f388bfa99a97 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5033,21 +5033,28 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>  	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
>  			   &wm->trans_wm);
>  
> -	/* FIXME: add proper NV12 support for ICL. */
> -	if (INTEL_GEN(dev_priv) >= 11)
> -		return skl_ddb_entry_write(dev_priv,
> -					   PLANE_BUF_CFG(pipe, plane_id),
> -					   &ddb->plane[pipe][plane_id]);
> -	if (wm->is_planar) {
> -		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> -				    &ddb->uv_plane[pipe][plane_id]);
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		/* FIXME: add proper NV12 support for ICL. */
> +		if (wm->is_planar)
> +			DRM_ERROR("No DDB support for planar formats yet\n");
> +
>  		skl_ddb_entry_write(dev_priv,
> -				    PLANE_NV12_BUF_CFG(pipe, plane_id),
> -				    &ddb->plane[pipe][plane_id]);
> +				   PLANE_BUF_CFG(pipe, plane_id),
> +				   &ddb->plane[pipe][plane_id]);
>  	} else {
> -		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> -				    &ddb->plane[pipe][plane_id]);
> -		I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> +		if (wm->is_planar) {
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_BUF_CFG(pipe, plane_id),
> +					    &ddb->uv_plane[pipe][plane_id]);
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_NV12_BUF_CFG(pipe, plane_id),
> +					    &ddb->plane[pipe][plane_id]);
> +		} else {
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_BUF_CFG(pipe, plane_id),
> +					    &ddb->plane[pipe][plane_id]);
> +			I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> +		}
>  	}
>  }
>  
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 07/11] drm/i915: move ddb_blocks to be a watermark parameter

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:29PM -0700, Paulo Zanoni wrote:
> The goal of struct skl_wm_params is to cache every watermark
> parameter so the other functions can just use them without worrying
> about the appropriate place to fetch each parameter requested by the
> spec, and without having to recompute parameters that are used in
> different steps of the calculation.
> 
> The ddb_blocks parameter is one that is used by both the the plane
> watermarks and the transition watermarks. Move ddb_blocks to the
> parameter struct so we can simplify the code.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++-------------------------
>  2 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4b1e8471609b..b32d680d9bf0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1255,6 +1255,7 @@ struct skl_wm_params {
>  	bool rc_surface;
>  	bool is_planar;
>  	uint32_t width;
> +	uint16_t ddb_blocks;
>  	uint8_t cpp;
>  	uint32_t plane_pixel_rate;
>  	uint32_t y_min_scanlines;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f388bfa99a97..4053f4a68657 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4522,11 +4522,13 @@ static int
>  skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  			    struct intel_crtc_state *cstate,
>  			    const struct intel_plane_state *intel_pstate,
> +			    const struct skl_ddb_allocation *ddb,
>  			    struct skl_wm_params *wp, int plane_id)
>  {
>  	struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
>  	const struct drm_plane_state *pstate = &intel_pstate->base;
>  	const struct drm_framebuffer *fb = pstate->fb;
> +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
>  	uint32_t interm_pbpl;
>  	struct intel_atomic_state *state =
>  		to_intel_atomic_state(cstate->base.state);
> @@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  	wp->linetime_us = fixed16_to_u32_round_up(
>  					intel_get_linetime_us(cstate));
>  
> +	wp->ddb_blocks = plane_id ?

Ugh. That 'plane_id' really should be renamed to something else.
'color_plane' would be the newfangled name I started to use elsewhere.

Patch looks good to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		     skl_ddb_entry_size(&ddb->uv_plane[pipe][plane->id]) :
> +		     skl_ddb_entry_size(&ddb->plane[pipe][plane->id]);
> +
>  	return 0;
>  }
>  
>  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  				struct intel_crtc_state *cstate,
>  				const struct intel_plane_state *intel_pstate,
> -				uint16_t ddb_allocation,
>  				int level,
>  				const struct skl_wm_params *wp,
>  				const struct skl_wm_level *result_prev,
> @@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  		     wp->dbuf_block_size < 1) &&
>  		     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
>  			selected_result = method2;
> -		} else if (ddb_allocation >=
> +		} else if (wp->ddb_blocks >=
>  			 fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
>  			if (INTEL_GEN(dev_priv) == 9 &&
>  			    !IS_GEMINILAKE(dev_priv))
> @@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	}
>  
>  	if ((level > 0 && res_lines > 31) ||
> -	    res_blocks >= ddb_allocation ||
> -	    min_disp_buf_needed >= ddb_allocation) {
> +	    res_blocks >= wp->ddb_blocks ||
> +	    min_disp_buf_needed >= wp->ddb_blocks) {
>  		result->plane_en = false;
>  
>  		/*
> @@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
>  			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
>  				      plane->base.id, plane->name,
> -				      res_blocks, ddb_allocation, res_lines);
> +				      res_blocks, wp->ddb_blocks, res_lines);
>  			return -EINVAL;
>  		}
>  	}
> @@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  
>  static int
>  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> -		      struct skl_ddb_allocation *ddb,
>  		      struct intel_crtc_state *cstate,
>  		      const struct intel_plane_state *intel_pstate,
>  		      const struct skl_wm_params *wm_params,
>  		      struct skl_plane_wm *wm,
>  		      int plane_id)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> -	struct drm_plane *plane = intel_pstate->base.plane;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	uint16_t ddb_blocks;
> -	enum pipe pipe = intel_crtc->pipe;
>  	int level, max_level = ilk_wm_max_level(dev_priv);
> -	enum plane_id intel_plane_id = intel_plane->id;
>  	int ret;
>  
> -	ddb_blocks = plane_id ?
> -		     skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
> -		     skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
> -
>  	for (level = 0; level <= max_level; level++) {
>  		struct skl_wm_level *result = plane_id ? &wm->uv_wm[level] :
>  							  &wm->wm[level];
> @@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
>  		ret = skl_compute_plane_wm(dev_priv,
>  					   cstate,
>  					   intel_pstate,
> -					   ddb_blocks,
>  					   level,
>  					   wm_params,
>  					   result_prev,
> @@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
>  static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>  				      struct skl_wm_params *wp,
>  				      struct skl_wm_level *wm_l0,
> -				      uint16_t ddb_allocation,
>  				      struct skl_wm_level *trans_wm /* out */)
>  {
>  	struct drm_device *dev = cstate->base.crtc->dev;
> @@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>  
>  	res_blocks += 1;
>  
> -	if (res_blocks < ddb_allocation) {
> +	if (res_blocks < wp->ddb_blocks) {
>  		trans_wm->plane_res_b = res_blocks;
>  		trans_wm->plane_en = true;
>  		return;
> @@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  						to_intel_plane_state(pstate);
>  		enum plane_id plane_id = to_intel_plane(plane)->id;
>  		struct skl_wm_params wm_params;
> -		enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> -		uint16_t ddb_blocks;
>  
>  		wm = &pipe_wm->planes[plane_id];
> -		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
>  
>  		ret = skl_compute_plane_wm_params(dev_priv, cstate,
> -						  intel_pstate, &wm_params, 0);
> +						  intel_pstate, ddb,
> +						  &wm_params, 0);
>  		if (ret)
>  			return ret;
>  
>  		if (!wm_params.plane_visible)
>  			continue;
>  
> -		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> +		ret = skl_compute_wm_levels(dev_priv, cstate,
>  					    intel_pstate, &wm_params, wm, 0);
>  		if (ret)
>  			return ret;
>  
>  		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> -					  ddb_blocks, &wm->trans_wm);
> +					  &wm->trans_wm);
>  
>  		/* uv plane watermarks must also be validated for NV12/Planar */
>  		if (wm_params.is_planar) {
>  			ret = skl_compute_plane_wm_params(dev_priv, cstate,
> -							  intel_pstate,
> +							  intel_pstate, ddb,
>  							  &wm_params, 1);
>  			if (ret)
>  				return ret;
>  
> -			ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> +			ret = skl_compute_wm_levels(dev_priv, cstate,
>  						    intel_pstate, &wm_params,
>  						    wm, 1);
>  			if (ret)
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 08/11] drm/i915: reorganize the error message for invalid watermarks

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote:
> Print a more generic "failed to compute watermark levels" whenever any
> of skl_compute_wm_levels() fail, and print only the specific error
> message for the specific cases. This allows us to stop passing pstate
> everywhere, making the watermarks computation code a little less
> dependent on random intel state structs.

Nothing random about those structs. They are *the* state.
Using them directly rather than duplicating informationa in the
wm_params struct would probably make the code look a bit less alien.

But given that the information is duplicated I guess we don't have to
pass in the plane state. So the patch seems fine in that sense.

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

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4053f4a68657..1290efc64869 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  
>  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  				struct intel_crtc_state *cstate,
> -				const struct intel_plane_state *intel_pstate,
>  				int level,
>  				const struct skl_wm_params *wp,
>  				const struct skl_wm_level *result_prev,
>  				struct skl_wm_level *result /* out */)
>  {
> -	const struct drm_plane_state *pstate = &intel_pstate->base;
>  	uint32_t latency = dev_priv->wm.skl_latency[level];
>  	uint_fixed_16_16_t method1, method2;
>  	uint_fixed_16_16_t selected_result;
> @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  		if (level) {
>  			return 0;
>  		} else {
> -			struct drm_plane *plane = pstate->plane;
> -
> -			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> -			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> -				      plane->base.id, plane->name,
> +			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n",
>  				      res_blocks, wp->ddb_blocks, res_lines);
>  			return -EINVAL;
>  		}
> @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  static int
>  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
>  		      struct intel_crtc_state *cstate,
> -		      const struct intel_plane_state *intel_pstate,
>  		      const struct skl_wm_params *wm_params,
>  		      struct skl_plane_wm *wm,
>  		      int plane_id)
> @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
>  
>  		ret = skl_compute_plane_wm(dev_priv,
>  					   cstate,
> -					   intel_pstate,
>  					   level,
>  					   wm_params,
>  					   result_prev,
> @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  		if (!wm_params.plane_visible)
>  			continue;
>  
> -		ret = skl_compute_wm_levels(dev_priv, cstate,
> -					    intel_pstate, &wm_params, wm, 0);
> -		if (ret)
> +		ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
> +					    0);
> +		if (ret) {
> +			DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
> +				      plane->base.id, plane->name);
>  			return ret;
> +		}
>  
>  		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
>  					  &wm->trans_wm);
> @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  				return ret;
>  
>  			ret = skl_compute_wm_levels(dev_priv, cstate,
> -						    intel_pstate, &wm_params,
> -						    wm, 1);
> -			if (ret)
> +						    &wm_params, wm, 1);
> +			if (ret) {
> +				DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
> +					      plane->base.id, plane->name);
>  				return ret;
> +			}
>  		}
>  	}
>  
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 09/11] drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:31PM -0700, Paulo Zanoni wrote:
> The function only really needs dev_priv to make its decision. If we
> ever need more, we can change it again. But then, in this case we
> should make needs_memory_bw_wa be a variable inside struct
> skl_wm_params so we won't need to keep passing intel states deep
> inside pure watermark value calculation functions.

Again, I tend to disagree with the notion that wm_params is
somehow better than the atomic state(s) proper.

But the patch itself looks fine so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1290efc64869..d101c542f10d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3599,10 +3599,8 @@ static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
>   * FIXME: We still don't have the proper code detect if we need to apply the WA,
>   * so assume we'll always need it in order to avoid underruns.
>   */
> -static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
> +static bool skl_needs_memory_bw_wa(const struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -
>  	if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv))
>  		return true;
>  
> @@ -3765,7 +3763,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
>  
>  		latency = dev_priv->wm.skl_latency[level];
>  
> -		if (skl_needs_memory_bw_wa(intel_state) &&
> +		if (skl_needs_memory_bw_wa(dev_priv) &&
>  		    plane->base.state->fb->modifier ==
>  		    I915_FORMAT_MOD_X_TILED)
>  			latency += 15;
> @@ -4530,9 +4528,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  	const struct drm_framebuffer *fb = pstate->fb;
>  	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
>  	uint32_t interm_pbpl;
> -	struct intel_atomic_state *state =
> -		to_intel_atomic_state(cstate->base.state);
> -	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> +	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
>  
>  	wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
>  	if (!wp->plane_visible)
> @@ -4644,9 +4640,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	uint_fixed_16_16_t method1, method2;
>  	uint_fixed_16_16_t selected_result;
>  	uint32_t res_blocks, res_lines;
> -	struct intel_atomic_state *state =
> -		to_intel_atomic_state(cstate->base.state);
> -	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> +	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
>  	uint32_t min_disp_buf_needed;
>  
>  	if (latency == 0) {
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 10/11] drm/i915: add pipe_htotal to struct skl_wm_params

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:32PM -0700, Paulo Zanoni wrote:
> With this one here we can finally drop the intel state structures from
> the functions that compute watermark values: they all rely on struct
> skl_wm_params now. This should help the watermarks code be a little
> more clear on its intent and also match the spec a little bit more
> with the carefully chosen names for its parameters.

I guess we'll just roll with the duplication.

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

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++------------
>  2 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b32d680d9bf0..4712eaea9744 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1254,6 +1254,7 @@ struct skl_wm_params {
>  	bool x_tiled, y_tiled;
>  	bool rc_surface;
>  	bool is_planar;
> +	uint32_t pipe_htotal;
>  	uint32_t width;
>  	uint16_t ddb_blocks;
>  	uint8_t cpp;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d101c542f10d..b01f3d807ff6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4549,6 +4549,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  			 fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
>  	wp->is_planar = fb->format->format == DRM_FORMAT_NV12;
>  
> +	wp->pipe_htotal = cstate->base.adjusted_mode.crtc_htotal;
> +
>  	if (plane->id == PLANE_CURSOR) {
>  		wp->width = intel_pstate->base.crtc_w;
>  	} else {
> @@ -4630,7 +4632,6 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
>  }
>  
>  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> -				struct intel_crtc_state *cstate,
>  				int level,
>  				const struct skl_wm_params *wp,
>  				const struct skl_wm_level *result_prev,
> @@ -4659,17 +4660,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  
>  	method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
>  				 wp->cpp, latency, wp->dbuf_block_size);
> -	method2 = skl_wm_method2(wp->plane_pixel_rate,
> -				 cstate->base.adjusted_mode.crtc_htotal,
> +	method2 = skl_wm_method2(wp->plane_pixel_rate, wp->pipe_htotal,
>  				 latency,
>  				 wp->plane_blocks_per_line);
>  
>  	if (wp->y_tiled) {
>  		selected_result = max_fixed16(method2, wp->y_tile_minimum);
>  	} else {
> -		if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
> -		     wp->dbuf_block_size < 1) &&
> -		     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
> +		if ((wp->cpp * wp->pipe_htotal / wp->dbuf_block_size < 1) &&
> +		    (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
>  			selected_result = method2;
>  		} else if (wp->ddb_blocks >=
>  			 fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
> @@ -4782,7 +4781,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  
>  static int
>  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> -		      struct intel_crtc_state *cstate,
>  		      const struct skl_wm_params *wm_params,
>  		      struct skl_plane_wm *wm,
>  		      int plane_id)
> @@ -4802,7 +4800,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
>  			result_prev = plane_id ? &wm->uv_wm[0] : &wm->wm[0];
>  
>  		ret = skl_compute_plane_wm(dev_priv,
> -					   cstate,
>  					   level,
>  					   wm_params,
>  					   result_prev,
> @@ -4937,8 +4934,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  		if (!wm_params.plane_visible)
>  			continue;
>  
> -		ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
> -					    0);
> +		ret = skl_compute_wm_levels(dev_priv, &wm_params, wm, 0);
>  		if (ret) {
>  			DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
>  				      plane->base.id, plane->name);
> @@ -4956,8 +4952,8 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  			if (ret)
>  				return ret;
>  
> -			ret = skl_compute_wm_levels(dev_priv, cstate,
> -						    &wm_params, wm, 1);
> +			ret = skl_compute_wm_levels(dev_priv, &wm_params, wm,
> +						    1);
>  			if (ret) {
>  				DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
>  					      plane->base.id, plane->name);
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 11/11] drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 03:01:33PM -0700, Paulo Zanoni wrote:
> Stop passing modeset state structures to functions that should work
> only with the skl_wm_params. The only use for cstate there was to
> reach dev_priv, so pass it directly.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b01f3d807ff6..dac2613aaaa3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4836,13 +4836,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
>  	return linetime_wm;
>  }
>  
> -static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
> +static void skl_compute_transition_wm(const struct drm_i915_private *dev_priv,
>  				      struct skl_wm_params *wp,
>  				      struct skl_wm_level *wm_l0,
>  				      struct skl_wm_level *trans_wm /* out */)
>  {
> -	struct drm_device *dev = cstate->base.crtc->dev;
> -	const struct drm_i915_private *dev_priv = to_i915(dev);
>  	uint16_t trans_min, trans_y_tile_min;
>  	const uint16_t trans_amount = 10; /* This is configurable amount */
>  	uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
> @@ -4941,7 +4939,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>  			return ret;
>  		}
>  
> -		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> +		skl_compute_transition_wm(dev_priv, &wm_params, &wm->wm[0],
>  					  &wm->trans_wm);
>  
>  		/* uv plane watermarks must also be validated for NV12/Planar */
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: ✗ Fi.CI.BAT: failure for More watermarks improvements

[Saarinen, Jani]

HI, 

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Paulo Zanoni
> Sent: keskiviikko 17. lokakuuta 2018 1.53
> To: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for More watermarks improvements
> 
> Em Ter, 2018-10-16 às 22:39 +0000, Patchwork escreveu:
> > == Series Details ==
> >
> > Series: More watermarks improvements
> > URL   : https://patchwork.freedesktop.org/series/51086/
> > State : failure
> >
> > == Summary ==
> >
> > = CI Bug Log - changes from CI_DRM_4990 -> Patchwork_10481 =
> >
> > == Summary - FAILURE ==
> >
> >   Serious unknown changes coming with Patchwork_10481 absolutely need
> > to be
> >   verified manually.
> >
> >   If you think the reported changes have nothing to do with the
> > changes
> >   introduced in Patchwork_10481, please notify your bug team to allow
> > them
> >   to document this new failure mode, which will reduce false positives
> > in CI.
> >
> >   External URL: https://patchwork.freedesktop.org/api/1.0/series/5108
> > 6/revisions/1/mbox/
> >
> > == Possible new issues ==
> >
> >   Here are the unknown changes that may have been introduced in
> > Patchwork_10481:
> >
> >   === IGT changes ===
> >
> >     ==== Possible regressions ====
> >
> >     igt@pm_rpm@module-reload:
> >       fi-glk-j4005:       PASS -> DMESG-FAIL
> 
> 
> EDID shows invalid data after module reload, triggering error during mode
> comparison in the subtest. Hard to think how this series could cause that.
Re-run? 

> 
> >
> >
> > == Known issues ==
> >
> >   Here are the changes found in Patchwork_10481 that come from known
> > issues:
> >
> >   === IGT changes ===
> >
> >     ==== Issues hit ====
> >
> >     igt@gem_exec_suspend@basic-s3:
> >       fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107859,
> > fdo#107774, fdo#107556)
> >
> >     igt@kms_flip@basic-flip-vs-dpms:
> >       fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000, fdo#106097)
> >
> >     igt@kms_flip@basic-plain-flip:
> >       fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387) +2
> >
> >     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
> >       fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)
> >
> >
> >     ==== Possible fixes ====
> >
> >     igt@drv_module_reload@basic-reload:
> >       fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> PASS
> >
> >     igt@drv_selftest@live_hangcheck:
> >       fi-icl-u2:          INCOMPLETE (fdo#108315) -> PASS
> >
> >     igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
> >       fi-glk-j4005:       FAIL (fdo#106765) -> PASS
> >
> >     igt@kms_flip@basic-flip-vs-dpms:
> >       fi-hsw-4770r:       DMESG-WARN (fdo#105602) -> PASS
> >
> >     igt@kms_flip@basic-flip-vs-wf_vblank:
> >       fi-glk-j4005:       FAIL (fdo#100368) -> PASS
> >
> >     igt@kms_flip@basic-plain-flip:
> >       fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS
> >
> >     igt@kms_frontbuffer_tracking@basic:
> >       fi-icl-u2:          FAIL (fdo#103167) -> PASS
> >       fi-byt-clapper:     FAIL (fdo#103167) -> PASS
> >
> >
> >   fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
> >   fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
> >   fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
> >   fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
> >   fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
> >   fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
> >   fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
> >   fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
> >   fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
> >   fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
> >   fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
> >   fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
> >   fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
> >   fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
> >   fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
> >
> >
> > == Participating hosts (46 -> 38) ==
> >
> >   Additional (1): fi-kbl-soraka
> >   Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-skl-guc
> > fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-pnv-d510 fi-kbl-7560u
> >
> >
> > == Build changes ==
> >
> >     * Linux: CI_DRM_4990 -> Patchwork_10481
> >
> >   CI_DRM_4990: 0bd34d92e9a27784cb988a300619f497ca0e99ec @
> > git://anongit.freedesktop.org/gfx-ci/linux
> >   IGT_4681: 959d6f95cb1344e0c0dace5b236e17755826fac1 @
> > git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
> >   Patchwork_10481: 4a4d7716aeaa870a7d0aaa1e541706cb919096cd @
> > git://anongit.freedesktop.org/gfx-ci/linux
> >
> >
> > == Linux commits ==
> >
> > 4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to
> > skl_compute_transition_wm()
> > 03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
> > e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv
> > instead of state c801352ac21a drm/i915: reorganize the error message
> > for invalid watermarks 52262264df7e drm/i915: move ddb_blocks to be a
> > watermark parameter
> > b6f917785676 drm/i915: refactor skl_write_plane_wm() 10134ecdb23d
> > drm/i915: simplify wm->is_planar assignment 20ce35c3029a drm/i915:
> > remove useless memset() for watermarks parameters 7d4eced125cf
> > drm/i915: fix handling of invisible planes in watermarks code
> > 6165f4257098 drm/i915: remove padding from struct skl_wm_level
> > 6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to
> > GLK/CNL+
> >
> > == Logs ==
> >
> > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw
> > ork_10481/issues.html
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/11] drm/i915: reorganize the error message for invalid watermarks

[Ville Syrjälä]

On Thu, Oct 18, 2018 at 04:55:52PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote:
> > Print a more generic "failed to compute watermark levels" whenever any
> > of skl_compute_wm_levels() fail, and print only the specific error
> > message for the specific cases. This allows us to stop passing pstate
> > everywhere, making the watermarks computation code a little less
> > dependent on random intel state structs.
> 
> Nothing random about those structs. They are *the* state.
> Using them directly rather than duplicating informationa in the
> wm_params struct would probably make the code look a bit less alien.
> 
> But given that the information is duplicated I guess we don't have to
> pass in the plane state. So the patch seems fine in that sense.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And this will definitely conflict with Maarten's NV12 work. Either we
need to duplicate even more stuff or just pass the proper states around.

> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 4053f4a68657..1290efc64869 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> >  
> >  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  				struct intel_crtc_state *cstate,
> > -				const struct intel_plane_state *intel_pstate,
> >  				int level,
> >  				const struct skl_wm_params *wp,
> >  				const struct skl_wm_level *result_prev,
> >  				struct skl_wm_level *result /* out */)
> >  {
> > -	const struct drm_plane_state *pstate = &intel_pstate->base;
> >  	uint32_t latency = dev_priv->wm.skl_latency[level];
> >  	uint_fixed_16_16_t method1, method2;
> >  	uint_fixed_16_16_t selected_result;
> > @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  		if (level) {
> >  			return 0;
> >  		} else {
> > -			struct drm_plane *plane = pstate->plane;
> > -
> > -			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> > -			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> > -				      plane->base.id, plane->name,
> > +			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n",
> >  				      res_blocks, wp->ddb_blocks, res_lines);
> >  			return -EINVAL;
> >  		}
> > @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  static int
> >  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> >  		      struct intel_crtc_state *cstate,
> > -		      const struct intel_plane_state *intel_pstate,
> >  		      const struct skl_wm_params *wm_params,
> >  		      struct skl_plane_wm *wm,
> >  		      int plane_id)
> > @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> >  
> >  		ret = skl_compute_plane_wm(dev_priv,
> >  					   cstate,
> > -					   intel_pstate,
> >  					   level,
> >  					   wm_params,
> >  					   result_prev,
> > @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> >  		if (!wm_params.plane_visible)
> >  			continue;
> >  
> > -		ret = skl_compute_wm_levels(dev_priv, cstate,
> > -					    intel_pstate, &wm_params, wm, 0);
> > -		if (ret)
> > +		ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
> > +					    0);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
> > +				      plane->base.id, plane->name);
> >  			return ret;
> > +		}
> >  
> >  		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> >  					  &wm->trans_wm);
> > @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> >  				return ret;
> >  
> >  			ret = skl_compute_wm_levels(dev_priv, cstate,
> > -						    intel_pstate, &wm_params,
> > -						    wm, 1);
> > -			if (ret)
> > +						    &wm_params, wm, 1);
> > +			if (ret) {
> > +				DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
> > +					      plane->base.id, plane->name);
> >  				return ret;
> > +			}
> >  		}
> >  	}
> >  
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

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

Re: [PATCH 08/11] drm/i915: reorganize the error message for invalid watermarks

[Paulo Zanoni]

Em Qui, 2018-10-18 às 16:55 +0300, Ville Syrjälä escreveu:
> On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote:
> > Print a more generic "failed to compute watermark levels" whenever
> > any
> > of skl_compute_wm_levels() fail, and print only the specific error
> > message for the specific cases. This allows us to stop passing
> > pstate
> > everywhere, making the watermarks computation code a little less
> > dependent on random intel state structs.
> 
> Nothing random about those structs. They are *the* state.

What I'm about to say is all probably a reflex of my own incompetence
to understand the flows of our code, but here it goes.

1. There's duplication in those structs. At any given point of our
source code, should you use state structs passed as parameters, or
should you use object->state (e.g., intel_crtc->state)? Sometimes one
thing is the new state and the other thing is the old state, sometimes
it is the opposite, and checking which one is which is never trivial. I
always have to go back to intel_display.c and try to find that point in
the modeset where we assign crtc_config to crtc->config and then try to
figure out if my code runs before or after that point. And I'm never
100% confident I'm using the correct struct.

2. There's a lot of duplication in members of those structs. There are
like 5 different things that could mean "pixel rate" (for real mode,
for adjusted mode, considering/not-considering scaling, rotation, etc),
there are many things that could mean "source width", etc. So when you
need a specific value, it's not always clear where to get it from in
our driver.

3. The names inside the watermarks calculation page (or anywhere else
in our spec) don't easily translate to any of those struct members
mentioned in item 2. E.g., plane pixel rate.

I mean, look at how many times the spec (not only for watermarks, but
for other stuff too, like FBC and PSR) had wording like "source size"
and we fetched the value from one place, but then we learned that we
needed to fetch the value from this other place that was the same in
most cases except for these X and Y corner cases. You reviewed some of
those patches.

So, to conclude the argument, the nice thing about struct skl_wm_params
is that it allows us to have a single point where we translate "i915
terminology" to "watermarks calculation algorithm terminology", so we
can fix that single place if/when needed. And then everywhere else in
watermarks code we just fetch the value from this struct without having
to worry "at this point in the sequence, where should this value come
from?", allowing us to focus on the algorithm specifically. On top of
that, there's the fact that we only compute things once instead of up
to 8 times per plane (7 levels + transition watermarks).

Of course, the downside is that we address the problem of "too many
places to fetch information from" by introducing a new place where
information is fetched from, so I definitely can't argue that the
current solution is good either. I would really like to know what are
your proposals here: if we decide to remove the params struct, would
you suggest we simply fetch everything directly from the passed structs
and recompute all the values that are computed multiple times? I'm
totally willing to implement something better if you have it in your
head.


> Using them directly rather than duplicating informationa in the
> wm_params struct would probably make the code look a bit less alien.
> 
> But given that the information is duplicated I guess we don't have to
> pass in the plane state. So the patch seems fine in that sense.

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

Thanks for the reviews!

> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 4053f4a68657..1290efc64869 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct
> > drm_i915_private *dev_priv,
> >  
> >  static int skl_compute_plane_wm(const struct drm_i915_private
> > *dev_priv,
> >  				struct intel_crtc_state *cstate,
> > -				const struct intel_plane_state
> > *intel_pstate,
> >  				int level,
> >  				const struct skl_wm_params *wp,
> >  				const struct skl_wm_level
> > *result_prev,
> >  				struct skl_wm_level *result /* out
> > */)
> >  {
> > -	const struct drm_plane_state *pstate = &intel_pstate-
> > >base;
> >  	uint32_t latency = dev_priv->wm.skl_latency[level];
> >  	uint_fixed_16_16_t method1, method2;
> >  	uint_fixed_16_16_t selected_result;
> > @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  		if (level) {
> >  			return 0;
> >  		} else {
> > -			struct drm_plane *plane = pstate->plane;
> > -
> > -			DRM_DEBUG_KMS("Requested display
> > configuration exceeds system watermark limitations\n");
> > -			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks
> > required = %u/%u, lines required = %u/31\n",
> > -				      plane->base.id, plane->name,
> > +			DRM_DEBUG_KMS("Requested display
> > configuration exceeds system watermark limitations: blocks required
> > = %u/%u, lines required = %u/31\n",
> >  				      res_blocks, wp->ddb_blocks,
> > res_lines);
> >  			return -EINVAL;
> >  		}
> > @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  static int
> >  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> >  		      struct intel_crtc_state *cstate,
> > -		      const struct intel_plane_state
> > *intel_pstate,
> >  		      const struct skl_wm_params *wm_params,
> >  		      struct skl_plane_wm *wm,
> >  		      int plane_id)
> > @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct
> > drm_i915_private *dev_priv,
> >  
> >  		ret = skl_compute_plane_wm(dev_priv,
> >  					   cstate,
> > -					   intel_pstate,
> >  					   level,
> >  					   wm_params,
> >  					   result_prev,
> > @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct
> > intel_crtc_state *cstate,
> >  		if (!wm_params.plane_visible)
> >  			continue;
> >  
> > -		ret = skl_compute_wm_levels(dev_priv, cstate,
> > -					    intel_pstate,
> > &wm_params, wm, 0);
> > -		if (ret)
> > +		ret = skl_compute_wm_levels(dev_priv, cstate,
> > &wm_params, wm,
> > +					    0);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("[PLANE:%d:%s] failed to
> > compute watermark levels\n",
> > +				      plane->base.id, plane-
> > >name);
> >  			return ret;
> > +		}
> >  
> >  		skl_compute_transition_wm(cstate, &wm_params, &wm-
> > >wm[0],
> >  					  &wm->trans_wm);
> > @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct
> > intel_crtc_state *cstate,
> >  				return ret;
> >  
> >  			ret = skl_compute_wm_levels(dev_priv,
> > cstate,
> > -						    intel_pstate,
> > &wm_params,
> > -						    wm, 1);
> > -			if (ret)
> > +						    &wm_params,
> > wm, 1);
> > +			if (ret) {
> > +				DRM_DEBUG_KMS("[PLANE:%d:%s]
> > failed to compute planar watermark levels\n",
> > +					      plane->base.id,
> > plane->name);
> >  				return ret;
> > +			}
> >  		}
> >  	}
> >  
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/11] drm/i915: move ddb_blocks to be a watermark parameter

[Paulo Zanoni]

Em Qui, 2018-10-18 às 16:41 +0300, Ville Syrjälä escreveu:
> On Tue, Oct 16, 2018 at 03:01:29PM -0700, Paulo Zanoni wrote:
> > The goal of struct skl_wm_params is to cache every watermark
> > parameter so the other functions can just use them without worrying
> > about the appropriate place to fetch each parameter requested by
> > the
> > spec, and without having to recompute parameters that are used in
> > different steps of the calculation.
> > 
> > The ddb_blocks parameter is one that is used by both the the plane
> > watermarks and the transition watermarks. Move ddb_blocks to the
> > parameter struct so we can simplify the code.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++-------------
> > ------------
> >  2 files changed, 18 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 4b1e8471609b..b32d680d9bf0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1255,6 +1255,7 @@ struct skl_wm_params {
> >  	bool rc_surface;
> >  	bool is_planar;
> >  	uint32_t width;
> > +	uint16_t ddb_blocks;
> >  	uint8_t cpp;
> >  	uint32_t plane_pixel_rate;
> >  	uint32_t y_min_scanlines;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index f388bfa99a97..4053f4a68657 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4522,11 +4522,13 @@ static int
> >  skl_compute_plane_wm_params(const struct drm_i915_private
> > *dev_priv,
> >  			    struct intel_crtc_state *cstate,
> >  			    const struct intel_plane_state
> > *intel_pstate,
> > +			    const struct skl_ddb_allocation *ddb,
> >  			    struct skl_wm_params *wp, int
> > plane_id)
> >  {
> >  	struct intel_plane *plane = to_intel_plane(intel_pstate-
> > >base.plane);
> >  	const struct drm_plane_state *pstate = &intel_pstate-
> > >base;
> >  	const struct drm_framebuffer *fb = pstate->fb;
> > +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> >  	uint32_t interm_pbpl;
> >  	struct intel_atomic_state *state =
> >  		to_intel_atomic_state(cstate->base.state);
> > @@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct
> > drm_i915_private *dev_priv,
> >  	wp->linetime_us = fixed16_to_u32_round_up(
> >  					intel_get_linetime_us(csta
> > te));
> >  
> > +	wp->ddb_blocks = plane_id ?
> 
> Ugh. That 'plane_id' really should be renamed to something else.
> 'color_plane' would be the newfangled name I started to use
> elsewhere.

I completely agree: there's plane_id and plane->id and they mean
different things.

If you're already using color_plane I can totally go with it so we have
consistency. But, well, for RGB planes, plane 0 does contain color
information... But can I suggest us to use "subplane" terminology when
talking about the planes of a plane? What about subplane_index?
Something that does not use the word "plane" is probably better.

> 
> Patch looks good to me.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +		     skl_ddb_entry_size(&ddb-
> > >uv_plane[pipe][plane->id]) :
> > +		     skl_ddb_entry_size(&ddb->plane[pipe][plane-
> > >id]);
> > +
> >  	return 0;
> >  }
> >  
> >  static int skl_compute_plane_wm(const struct drm_i915_private
> > *dev_priv,
> >  				struct intel_crtc_state *cstate,
> >  				const struct intel_plane_state
> > *intel_pstate,
> > -				uint16_t ddb_allocation,
> >  				int level,
> >  				const struct skl_wm_params *wp,
> >  				const struct skl_wm_level
> > *result_prev,
> > @@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  		     wp->dbuf_block_size < 1) &&
> >  		     (wp->plane_bytes_per_line / wp-
> > >dbuf_block_size < 1)) {
> >  			selected_result = method2;
> > -		} else if (ddb_allocation >=
> > +		} else if (wp->ddb_blocks >=
> >  			 fixed16_to_u32_round_up(wp-
> > >plane_blocks_per_line)) {
> >  			if (INTEL_GEN(dev_priv) == 9 &&
> >  			    !IS_GEMINILAKE(dev_priv))
> > @@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  	}
> >  
> >  	if ((level > 0 && res_lines > 31) ||
> > -	    res_blocks >= ddb_allocation ||
> > -	    min_disp_buf_needed >= ddb_allocation) {
> > +	    res_blocks >= wp->ddb_blocks ||
> > +	    min_disp_buf_needed >= wp->ddb_blocks) {
> >  		result->plane_en = false;
> >  
> >  		/*
> > @@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  			DRM_DEBUG_KMS("Requested display
> > configuration exceeds system watermark limitations\n");
> >  			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks
> > required = %u/%u, lines required = %u/31\n",
> >  				      plane->base.id, plane->name,
> > -				      res_blocks, ddb_allocation,
> > res_lines);
> > +				      res_blocks, wp->ddb_blocks,
> > res_lines);
> >  			return -EINVAL;
> >  		}
> >  	}
> > @@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const
> > struct drm_i915_private *dev_priv,
> >  
> >  static int
> >  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > -		      struct skl_ddb_allocation *ddb,
> >  		      struct intel_crtc_state *cstate,
> >  		      const struct intel_plane_state
> > *intel_pstate,
> >  		      const struct skl_wm_params *wm_params,
> >  		      struct skl_plane_wm *wm,
> >  		      int plane_id)
> >  {
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > >base.crtc);
> > -	struct drm_plane *plane = intel_pstate->base.plane;
> > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> > -	uint16_t ddb_blocks;
> > -	enum pipe pipe = intel_crtc->pipe;
> >  	int level, max_level = ilk_wm_max_level(dev_priv);
> > -	enum plane_id intel_plane_id = intel_plane->id;
> >  	int ret;
> >  
> > -	ddb_blocks = plane_id ?
> > -		     skl_ddb_entry_size(&ddb-
> > >uv_plane[pipe][intel_plane_id]) :
> > -		     skl_ddb_entry_size(&ddb-
> > >plane[pipe][intel_plane_id]);
> > -
> >  	for (level = 0; level <= max_level; level++) {
> >  		struct skl_wm_level *result = plane_id ? &wm-
> > >uv_wm[level] :
> >  							  &wm-
> > >wm[level];
> > @@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct
> > drm_i915_private *dev_priv,
> >  		ret = skl_compute_plane_wm(dev_priv,
> >  					   cstate,
> >  					   intel_pstate,
> > -					   ddb_blocks,
> >  					   level,
> >  					   wm_params,
> >  					   result_prev,
> > @@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct
> > intel_crtc_state *cstate)
> >  static void skl_compute_transition_wm(struct intel_crtc_state
> > *cstate,
> >  				      struct skl_wm_params *wp,
> >  				      struct skl_wm_level *wm_l0,
> > -				      uint16_t ddb_allocation,
> >  				      struct skl_wm_level
> > *trans_wm /* out */)
> >  {
> >  	struct drm_device *dev = cstate->base.crtc->dev;
> > @@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct
> > intel_crtc_state *cstate,
> >  
> >  	res_blocks += 1;
> >  
> > -	if (res_blocks < ddb_allocation) {
> > +	if (res_blocks < wp->ddb_blocks) {
> >  		trans_wm->plane_res_b = res_blocks;
> >  		trans_wm->plane_en = true;
> >  		return;
> > @@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct
> > intel_crtc_state *cstate,
> >  						to_intel_plane_sta
> > te(pstate);
> >  		enum plane_id plane_id = to_intel_plane(plane)-
> > >id;
> >  		struct skl_wm_params wm_params;
> > -		enum pipe pipe = to_intel_crtc(cstate->base.crtc)-
> > >pipe;
> > -		uint16_t ddb_blocks;
> >  
> >  		wm = &pipe_wm->planes[plane_id];
> > -		ddb_blocks = skl_ddb_entry_size(&ddb-
> > >plane[pipe][plane_id]);
> >  
> >  		ret = skl_compute_plane_wm_params(dev_priv,
> > cstate,
> > -						  intel_pstate,
> > &wm_params, 0);
> > +						  intel_pstate,
> > ddb,
> > +						  &wm_params, 0);
> >  		if (ret)
> >  			return ret;
> >  
> >  		if (!wm_params.plane_visible)
> >  			continue;
> >  
> > -		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> > +		ret = skl_compute_wm_levels(dev_priv, cstate,
> >  					    intel_pstate,
> > &wm_params, wm, 0);
> >  		if (ret)
> >  			return ret;
> >  
> >  		skl_compute_transition_wm(cstate, &wm_params, &wm-
> > >wm[0],
> > -					  ddb_blocks, &wm-
> > >trans_wm);
> > +					  &wm->trans_wm);
> >  
> >  		/* uv plane watermarks must also be validated for
> > NV12/Planar */
> >  		if (wm_params.is_planar) {
> >  			ret =
> > skl_compute_plane_wm_params(dev_priv, cstate,
> > -							  intel_ps
> > tate,
> > +							  intel_ps
> > tate, ddb,
> >  							  &wm_para
> > ms, 1);
> >  			if (ret)
> >  				return ret;
> >  
> > -			ret = skl_compute_wm_levels(dev_priv, ddb,
> > cstate,
> > +			ret = skl_compute_wm_levels(dev_priv,
> > cstate,
> >  						    intel_pstate,
> > &wm_params,
> >  						    wm, 1);
> >  			if (ret)
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Paulo Zanoni]

Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
> On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
> > BSpec does not show these WAs as applicable to GLK, and for CNL it
> > only shows them applicable for a super early pre-production
> > stepping
> > we shouldn't be caring about anymore. Remove these so we can avoid
> > them on ICL too.
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------
> > ------------
> >  1 file changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 67a4d0735291..18157c6ee126 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
> > struct drm_i915_private *dev_priv,
> >  	res_lines = div_round_up_fixed16(selected_result,
> >  					 wp-
> > >plane_blocks_per_line);
> >  
> > -	/* Display WA #1125: skl,bxt,kbl,glk */
> > -	if (level == 0 && wp->rc_surface)
> > -		res_blocks += fixed16_to_u32_round_up(wp-
> > >y_tile_minimum);
> > +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> 
> IS_GEN9_BC || IS_BXT
> 
> would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

/me looks at Rodrigo

> 
> > +		/* Display WA #1125: skl,bxt,kbl */
> > +		if (level == 0 && wp->rc_surface)
> > +			res_blocks +=
> > +				fixed16_to_u32_round_up(wp-
> > >y_tile_minimum);
> > +
> > +		/* Display WA #1126: skl,bxt,kbl */
> > +		if (level >= 1 && level <= 7) {
> > +			if (wp->y_tiled) {
> > +				res_blocks +=
> > +				    fixed16_to_u32_round_up(wp-
> > >y_tile_minimum);
> > +				res_lines += wp->y_min_scanlines;
> > +			} else {
> > +				res_blocks++;
> > +			}
> >  
> > -	/* Display WA #1126: skl,bxt,kbl,glk */
> > -	if (level >= 1 && level <= 7) {
> > -		if (wp->y_tiled) {
> > -			res_blocks += fixed16_to_u32_round_up(
> > -							wp-
> > >y_tile_minimum);
> > -			res_lines += wp->y_min_scanlines;
> > -		} else {
> > -			res_blocks++;
> > +			/*
> > +			 * Make sure result blocks for higher
> > latency levels are
> > +			 * atleast as high as level below the
> > current level.
> > +			 * Assumption in DDB algorithm
> > optimization for special
> > +			 * cases. Also covers Display WA #1125 for
> > RC.
> > +			 */
> > +			if (result_prev->plane_res_b > res_blocks)
> > +				res_blocks = result_prev-
> > >plane_res_b;
> >  		}
> > -
> > -		/*
> > -		 * Make sure result blocks for higher latency
> > levels are atleast
> > -		 * as high as level below the current level.
> > -		 * Assumption in DDB algorithm optimization for
> > special cases.
> > -		 * Also covers Display WA #1125 for RC.
> > -		 */
> > -		if (result_prev->plane_res_b > res_blocks)
> > -			res_blocks = result_prev->plane_res_b;
> 
> This last thing is part of the glk+ watermark formula as well.
>  But
> I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?


>  One might assume that the the
> non-decrasing latency values guarantee that the resulting watermarks
> are also non-decreasing. But I've not actually done the math to see
> if
> that's true.
> 
> Hmm. It might not actually be true on account of the 'memory latency
> microseconds >= line time microseconds' check when selecting which
> method to use to calculate the watermark. Not quite sure which way
> that would make things go.
> 
> We also seem to be missing the res_lines handling here. But given
> that we only did this for compressed fbs before I don't think this
> patch is making things much worse by limiting this to pre-glk only.
> The glk+ handling and res_lines fix could be done as a followup.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 
> >  	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 11) {
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Rodrigo Vivi]

On Mon, Oct 22, 2018 at 04:32:00PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
> > On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
> > > BSpec does not show these WAs as applicable to GLK, and for CNL it
> > > only shows them applicable for a super early pre-production
> > > stepping
> > > we shouldn't be caring about anymore. Remove these so we can avoid
> > > them on ICL too.
> > >
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------
> > > ------------
> > >  1 file changed, 23 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 67a4d0735291..18157c6ee126 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
> > > struct drm_i915_private *dev_priv,
> > >  	res_lines = div_round_up_fixed16(selected_result,
> > >  					 wp-
> > > >plane_blocks_per_line);
> > >
> > > -	/* Display WA #1125: skl,bxt,kbl,glk */
> > > -	if (level == 0 && wp->rc_surface)
> > > -		res_blocks += fixed16_to_u32_round_up(wp-
> > > >y_tile_minimum);
> > > +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> >
> > IS_GEN9_BC || IS_BXT
> >
> > would be a little easier to parse, me thinks.
>
> I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

work in progress...

btw...

DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

I'm play around here with:

#define DISPLAY(dev_priv, g)    (!!((dev_priv)->info.display_mask & BIT(g-1)))
#define DISPLAY_RANGE(dev_priv, s, e) \
                (!!((dev_priv)->info.display_mask & INTEL_GEN_MASK((s), (e))))

thoughts? comments? ideas?

>
> /me looks at Rodrigo
>
> >
> > > +		/* Display WA #1125: skl,bxt,kbl */
> > > +		if (level == 0 && wp->rc_surface)
> > > +			res_blocks +=
> > > +				fixed16_to_u32_round_up(wp-
> > > >y_tile_minimum);
> > > +
> > > +		/* Display WA #1126: skl,bxt,kbl */
> > > +		if (level >= 1 && level <= 7) {
> > > +			if (wp->y_tiled) {
> > > +				res_blocks +=
> > > +				    fixed16_to_u32_round_up(wp-
> > > >y_tile_minimum);
> > > +				res_lines += wp->y_min_scanlines;
> > > +			} else {
> > > +				res_blocks++;
> > > +			}
> > >
> > > -	/* Display WA #1126: skl,bxt,kbl,glk */
> > > -	if (level >= 1 && level <= 7) {
> > > -		if (wp->y_tiled) {
> > > -			res_blocks += fixed16_to_u32_round_up(
> > > -							wp-
> > > >y_tile_minimum);
> > > -			res_lines += wp->y_min_scanlines;
> > > -		} else {
> > > -			res_blocks++;
> > > +			/*
> > > +			 * Make sure result blocks for higher
> > > latency levels are
> > > +			 * atleast as high as level below the
> > > current level.
> > > +			 * Assumption in DDB algorithm
> > > optimization for special
> > > +			 * cases. Also covers Display WA #1125 for
> > > RC.
> > > +			 */
> > > +			if (result_prev->plane_res_b > res_blocks)
> > > +				res_blocks = result_prev-
> > > >plane_res_b;
> > >  		}
> > > -
> > > -		/*
> > > -		 * Make sure result blocks for higher latency
> > > levels are atleast
> > > -		 * as high as level below the current level.
> > > -		 * Assumption in DDB algorithm optimization for
> > > special cases.
> > > -		 * Also covers Display WA #1125 for RC.
> > > -		 */
> > > -		if (result_prev->plane_res_b > res_blocks)
> > > -			res_blocks = result_prev->plane_res_b;
> >
> > This last thing is part of the glk+ watermark formula as well.
> >  But
> > I'm not 100% convinced that it's needed.
>
> I simply can't find where this is documented. WAs 1125 and 1126, which
> contain text that match this code exactly, are not applicable to GLK.
> Which BSpec page and paragraph/section mentions this?
>
>
> >  One might assume that the the
> > non-decrasing latency values guarantee that the resulting watermarks
> > are also non-decreasing. But I've not actually done the math to see
> > if
> > that's true.
> >
> > Hmm. It might not actually be true on account of the 'memory latency
> > microseconds >= line time microseconds' check when selecting which
> > method to use to calculate the watermark. Not quite sure which way
> > that would make things go.
> >
> > We also seem to be missing the res_lines handling here. But given
> > that we only did this for compressed fbs before I don't think this
> > patch is making things much worse by limiting this to pre-glk only.
> > The glk+ handling and res_lines fix could be done as a followup.
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >
> > >  	}
> > >
> > >  	if (INTEL_GEN(dev_priv) >= 11) {
> > > --
> > > 2.14.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for More watermarks improvements

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL   : https://patchwork.freedesktop.org/series/51086/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5018 -> Patchwork_10532 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51086/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10532 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    
    ==== Possible fixes ====

    igt@gem_cpu_reloc@basic:
      fi-kbl-7560u:       INCOMPLETE -> PASS

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (51 -> 45) ==

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u 


== Build changes ==

    * Linux: CI_DRM_5018 -> Patchwork_10532

  CI_DRM_5018: ae98bc614f3a2f29f3c48ed799d6fd863d200d3e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4685: 78619fde4008424c472906041edb1d204e014f7c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10532: e0dd3129b095e596afd67cd2e7ae6df5e46ed707 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e0dd3129b095 drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()
f908dda817d4 drm/i915: add pipe_htotal to struct skl_wm_params
28175f55b9c2 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
559e4b7abf44 drm/i915: reorganize the error message for invalid watermarks
ea0147852049 drm/i915: move ddb_blocks to be a watermark parameter
c22a33c74070 drm/i915: refactor skl_write_plane_wm()
1e5ceb113b99 drm/i915: simplify wm->is_planar assignment
ee3ca377b090 drm/i915: remove useless memset() for watermarks parameters
64810aa4560c drm/i915: fix handling of invisible planes in watermarks code
9a6a1e9b2d2c drm/i915: remove padding from struct skl_wm_level
f1e40c1fcdd8 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10532/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Paulo Zanoni]

Em Seg, 2018-10-22 às 16:55 -0700, Rodrigo Vivi escreveu:
> On Mon, Oct 22, 2018 at 04:32:00PM -0700, Paulo Zanoni wrote:
> > Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
> > > On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
> > > > BSpec does not show these WAs as applicable to GLK, and for CNL
> > > > it
> > > > only shows them applicable for a super early pre-production
> > > > stepping
> > > > we shouldn't be caring about anymore. Remove these so we can
> > > > avoid
> > > > them on ICL too.
> > > > 
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
> > > > ----
> > > > ------------
> > > >  1 file changed, 23 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 67a4d0735291..18157c6ee126 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
> > > > struct drm_i915_private *dev_priv,
> > > >  	res_lines = div_round_up_fixed16(selected_result,
> > > >  					 wp-
> > > > > plane_blocks_per_line);
> > > > 
> > > > -	/* Display WA #1125: skl,bxt,kbl,glk */
> > > > -	if (level == 0 && wp->rc_surface)
> > > > -		res_blocks += fixed16_to_u32_round_up(wp-
> > > > > y_tile_minimum);
> > > > 
> > > > +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> > > 
> > > IS_GEN9_BC || IS_BXT
> > > 
> > > would be a little easier to parse, me thinks.
> > 
> > I can do that, but what I really want is "DISPLAY_GEN(dev_priv) ==
> > 9".
> 
> work in progress...
> 
> btw...
> 
> DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.

I would expect a macro called DISPLAY() to return *the* Display or to
simply display somewhere the thing I pass as an argument. Now
DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
generates a Display).


> 
> I'm play around here with:
> 
> #define DISPLAY(dev_priv, g)    (!!((dev_priv)->info.display_mask &
> BIT(g-1)))
> #define DISPLAY_RANGE(dev_priv, s, e) \
>                 (!!((dev_priv)->info.display_mask &
> INTEL_GEN_MASK((s), (e))))
> 
> thoughts? comments? ideas?

#define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)

> 
> > 
> > /me looks at Rodrigo
> > 
> > > 
> > > > +		/* Display WA #1125: skl,bxt,kbl */
> > > > +		if (level == 0 && wp->rc_surface)
> > > > +			res_blocks +=
> > > > +				fixed16_to_u32_round_up(wp-
> > > > > y_tile_minimum);
> > > > 
> > > > +
> > > > +		/* Display WA #1126: skl,bxt,kbl */
> > > > +		if (level >= 1 && level <= 7) {
> > > > +			if (wp->y_tiled) {
> > > > +				res_blocks +=
> > > > +				    fixed16_to_u32_round_up(wp
> > > > -
> > > > > y_tile_minimum);
> > > > 
> > > > +				res_lines += wp-
> > > > >y_min_scanlines;
> > > > +			} else {
> > > > +				res_blocks++;
> > > > +			}
> > > > 
> > > > -	/* Display WA #1126: skl,bxt,kbl,glk */
> > > > -	if (level >= 1 && level <= 7) {
> > > > -		if (wp->y_tiled) {
> > > > -			res_blocks += fixed16_to_u32_round_up(
> > > > -							wp-
> > > > > y_tile_minimum);
> > > > 
> > > > -			res_lines += wp->y_min_scanlines;
> > > > -		} else {
> > > > -			res_blocks++;
> > > > +			/*
> > > > +			 * Make sure result blocks for higher
> > > > latency levels are
> > > > +			 * atleast as high as level below the
> > > > current level.
> > > > +			 * Assumption in DDB algorithm
> > > > optimization for special
> > > > +			 * cases. Also covers Display WA #1125
> > > > for
> > > > RC.
> > > > +			 */
> > > > +			if (result_prev->plane_res_b >
> > > > res_blocks)
> > > > +				res_blocks = result_prev-
> > > > > plane_res_b;
> > > > 
> > > >  		}
> > > > -
> > > > -		/*
> > > > -		 * Make sure result blocks for higher latency
> > > > levels are atleast
> > > > -		 * as high as level below the current level.
> > > > -		 * Assumption in DDB algorithm optimization
> > > > for
> > > > special cases.
> > > > -		 * Also covers Display WA #1125 for RC.
> > > > -		 */
> > > > -		if (result_prev->plane_res_b > res_blocks)
> > > > -			res_blocks = result_prev->plane_res_b;
> > > 
> > > This last thing is part of the glk+ watermark formula as well.
> > >  But
> > > I'm not 100% convinced that it's needed.
> > 
> > I simply can't find where this is documented. WAs 1125 and 1126,
> > which
> > contain text that match this code exactly, are not applicable to
> > GLK.
> > Which BSpec page and paragraph/section mentions this?
> > 
> > 
> > >  One might assume that the the
> > > non-decrasing latency values guarantee that the resulting
> > > watermarks
> > > are also non-decreasing. But I've not actually done the math to
> > > see
> > > if
> > > that's true.
> > > 
> > > Hmm. It might not actually be true on account of the 'memory
> > > latency
> > > microseconds >= line time microseconds' check when selecting
> > > which
> > > method to use to calculate the watermark. Not quite sure which
> > > way
> > > that would make things go.
> > > 
> > > We also seem to be missing the res_lines handling here. But given
> > > that we only did this for compressed fbs before I don't think
> > > this
> > > patch is making things much worse by limiting this to pre-glk
> > > only.
> > > The glk+ handling and res_lines fix could be done as a followup.
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > 
> > > >  	}
> > > > 
> > > >  	if (INTEL_GEN(dev_priv) >= 11) {
> > > > --
> > > > 2.14.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Rodrigo Vivi]

On Mon, Oct 22, 2018 at 05:12:18PM -0700, Paulo Zanoni wrote:
> Em Seg, 2018-10-22 às 16:55 -0700, Rodrigo Vivi escreveu:
> > On Mon, Oct 22, 2018 at 04:32:00PM -0700, Paulo Zanoni wrote:
> > > Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
> > > > On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
> > > > > BSpec does not show these WAs as applicable to GLK, and for CNL
> > > > > it
> > > > > only shows them applicable for a super early pre-production
> > > > > stepping
> > > > > we shouldn't be caring about anymore. Remove these so we can
> > > > > avoid
> > > > > them on ICL too.
> > > > > 
> > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
> > > > > ----
> > > > > ------------
> > > > >  1 file changed, 23 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index 67a4d0735291..18157c6ee126 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
> > > > > struct drm_i915_private *dev_priv,
> > > > >  	res_lines = div_round_up_fixed16(selected_result,
> > > > >  					 wp-
> > > > > > plane_blocks_per_line);
> > > > > 
> > > > > -	/* Display WA #1125: skl,bxt,kbl,glk */
> > > > > -	if (level == 0 && wp->rc_surface)
> > > > > -		res_blocks += fixed16_to_u32_round_up(wp-
> > > > > > y_tile_minimum);
> > > > > 
> > > > > +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> > > > 
> > > > IS_GEN9_BC || IS_BXT
> > > > 
> > > > would be a little easier to parse, me thinks.
> > > 
> > > I can do that, but what I really want is "DISPLAY_GEN(dev_priv) ==
> > > 9".
> > 
> > work in progress...
> > 
> > btw...
> > 
> > DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?
> 
> It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.

there's a macro defined on gen we end up never using
IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9

Should we just kill that or try to use more that instead of direct comparison?

The advantage seems to be the use of bitmasks...

> 
> I would expect a macro called DISPLAY() to return *the* Display or to
> simply display somewhere the thing I pass as an argument. Now
> DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
> generates a Display).

what about IS_DISPLAY(dev_priv, 9) ?
and IS_DISPLAY_RANGE(dev_priv, 5, 9)

> 
> 
> > 
> > I'm play around here with:
> > 
> > #define DISPLAY(dev_priv, g)    (!!((dev_priv)->info.display_mask &
> > BIT(g-1)))
> > #define DISPLAY_RANGE(dev_priv, s, e) \
> >                 (!!((dev_priv)->info.display_mask &
> > INTEL_GEN_MASK((s), (e))))
> > 
> > thoughts? comments? ideas?
> 
> #define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)
> 
> > 
> > > 
> > > /me looks at Rodrigo
> > > 
> > > > 
> > > > > +		/* Display WA #1125: skl,bxt,kbl */
> > > > > +		if (level == 0 && wp->rc_surface)
> > > > > +			res_blocks +=
> > > > > +				fixed16_to_u32_round_up(wp-
> > > > > > y_tile_minimum);
> > > > > 
> > > > > +
> > > > > +		/* Display WA #1126: skl,bxt,kbl */
> > > > > +		if (level >= 1 && level <= 7) {
> > > > > +			if (wp->y_tiled) {
> > > > > +				res_blocks +=
> > > > > +				    fixed16_to_u32_round_up(wp
> > > > > -
> > > > > > y_tile_minimum);
> > > > > 
> > > > > +				res_lines += wp-
> > > > > >y_min_scanlines;
> > > > > +			} else {
> > > > > +				res_blocks++;
> > > > > +			}
> > > > > 
> > > > > -	/* Display WA #1126: skl,bxt,kbl,glk */
> > > > > -	if (level >= 1 && level <= 7) {
> > > > > -		if (wp->y_tiled) {
> > > > > -			res_blocks += fixed16_to_u32_round_up(
> > > > > -							wp-
> > > > > > y_tile_minimum);
> > > > > 
> > > > > -			res_lines += wp->y_min_scanlines;
> > > > > -		} else {
> > > > > -			res_blocks++;
> > > > > +			/*
> > > > > +			 * Make sure result blocks for higher
> > > > > latency levels are
> > > > > +			 * atleast as high as level below the
> > > > > current level.
> > > > > +			 * Assumption in DDB algorithm
> > > > > optimization for special
> > > > > +			 * cases. Also covers Display WA #1125
> > > > > for
> > > > > RC.
> > > > > +			 */
> > > > > +			if (result_prev->plane_res_b >
> > > > > res_blocks)
> > > > > +				res_blocks = result_prev-
> > > > > > plane_res_b;
> > > > > 
> > > > >  		}
> > > > > -
> > > > > -		/*
> > > > > -		 * Make sure result blocks for higher latency
> > > > > levels are atleast
> > > > > -		 * as high as level below the current level.
> > > > > -		 * Assumption in DDB algorithm optimization
> > > > > for
> > > > > special cases.
> > > > > -		 * Also covers Display WA #1125 for RC.
> > > > > -		 */
> > > > > -		if (result_prev->plane_res_b > res_blocks)
> > > > > -			res_blocks = result_prev->plane_res_b;
> > > > 
> > > > This last thing is part of the glk+ watermark formula as well.
> > > >  But
> > > > I'm not 100% convinced that it's needed.
> > > 
> > > I simply can't find where this is documented. WAs 1125 and 1126,
> > > which
> > > contain text that match this code exactly, are not applicable to
> > > GLK.
> > > Which BSpec page and paragraph/section mentions this?
> > > 
> > > 
> > > >  One might assume that the the
> > > > non-decrasing latency values guarantee that the resulting
> > > > watermarks
> > > > are also non-decreasing. But I've not actually done the math to
> > > > see
> > > > if
> > > > that's true.
> > > > 
> > > > Hmm. It might not actually be true on account of the 'memory
> > > > latency
> > > > microseconds >= line time microseconds' check when selecting
> > > > which
> > > > method to use to calculate the watermark. Not quite sure which
> > > > way
> > > > that would make things go.
> > > > 
> > > > We also seem to be missing the res_lines handling here. But given
> > > > that we only did this for compressed fbs before I don't think
> > > > this
> > > > patch is making things much worse by limiting this to pre-glk
> > > > only.
> > > > The glk+ handling and res_lines fix could be done as a followup.
> > > > 
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > 
> > > > >  	}
> > > > > 
> > > > >  	if (INTEL_GEN(dev_priv) >= 11) {
> > > > > --
> > > > > 2.14.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for More watermarks improvements

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL   : https://patchwork.freedesktop.org/series/51086/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5018_full -> Patchwork_10532_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10532_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10532_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10532_full:

  === IGT changes ===

    ==== Warnings ====

    igt@perf_pmu@rc6:
      shard-kbl:          SKIP -> PASS

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_10532_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_workarounds@suspend-resume:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
      shard-hsw:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-apl:          PASS -> FAIL (fdo#103191, fdo#103232)

    igt@kms_cursor_crc@cursor-size-change:
      shard-apl:          PASS -> FAIL (fdo#103232)

    igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled:
      shard-skl:          PASS -> FAIL (fdo#103184)

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@plain-flip-ts-check:
      shard-skl:          NOTRUN -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@kms_plane_alpha_blend@pipe-a-alpha-transparant-fb:
      shard-kbl:          NOTRUN -> FAIL (fdo#108145)

    igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166)
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    igt@kms_sysfs_edid_timing:
      shard-kbl:          NOTRUN -> FAIL (fdo#100047)

    igt@perf@blocking:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    igt@pm_backlight@fade_with_suspend:
      shard-skl:          NOTRUN -> FAIL (fdo#107847)

    igt@pm_rps@reset:
      shard-kbl:          PASS -> FAIL (fdo#102250)

    igt@syncobj_wait@wait-all-for-submit-snapshot:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@syncobj_wait@wait-for-submit-complex:
      shard-skl:          NOTRUN -> INCOMPLETE (fdo#108490)

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#103665, fdo#106023) -> PASS

    igt@kms_busy@extended-pageflip-hang-newfb-render-c:
      shard-glk:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_cursor_crc@cursor-64x64-onscreen:
      shard-glk:          FAIL (fdo#103232) -> PASS +1

    igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
      shard-glk:          FAIL (fdo#104873) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-skl:          FAIL (fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-mmap-wc:
      shard-glk:          FAIL (fdo#103167) -> PASS +4

    igt@kms_frontbuffer_tracking@fbc-shrfb-scaledprimary:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS +2

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
      shard-apl:          FAIL (fdo#108145) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
      shard-glk:          FAIL (fdo#103166) -> PASS

    igt@pm_rps@reset:
      shard-glk:          FAIL (fdo#102250) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102250 https://bugs.freedesktop.org/show_bug.cgi?id=102250
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#107847 https://bugs.freedesktop.org/show_bug.cgi?id=107847
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108490 https://bugs.freedesktop.org/show_bug.cgi?id=108490
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5018 -> Patchwork_10532

  CI_DRM_5018: ae98bc614f3a2f29f3c48ed799d6fd863d200d3e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4685: 78619fde4008424c472906041edb1d204e014f7c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10532: e0dd3129b095e596afd67cd2e7ae6df5e46ed707 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10532/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Jani Nikula]

On Mon, 22 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Mon, Oct 22, 2018 at 05:12:18PM -0700, Paulo Zanoni wrote:
>> Em Seg, 2018-10-22 às 16:55 -0700, Rodrigo Vivi escreveu:
>> > On Mon, Oct 22, 2018 at 04:32:00PM -0700, Paulo Zanoni wrote:
>> > > Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
>> > > > On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
>> > > > > BSpec does not show these WAs as applicable to GLK, and for CNL
>> > > > > it
>> > > > > only shows them applicable for a super early pre-production
>> > > > > stepping
>> > > > > we shouldn't be caring about anymore. Remove these so we can
>> > > > > avoid
>> > > > > them on ICL too.
>> > > > > 
>> > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
>> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > > > ---
>> > > > >  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
>> > > > > ----
>> > > > > ------------
>> > > > >  1 file changed, 23 insertions(+), 20 deletions(-)
>> > > > > 
>> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> > > > > b/drivers/gpu/drm/i915/intel_pm.c
>> > > > > index 67a4d0735291..18157c6ee126 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > > > > @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
>> > > > > struct drm_i915_private *dev_priv,
>> > > > >  	res_lines = div_round_up_fixed16(selected_result,
>> > > > >  					 wp-
>> > > > > > plane_blocks_per_line);
>> > > > > 
>> > > > > -	/* Display WA #1125: skl,bxt,kbl,glk */
>> > > > > -	if (level == 0 && wp->rc_surface)
>> > > > > -		res_blocks += fixed16_to_u32_round_up(wp-
>> > > > > > y_tile_minimum);
>> > > > > 
>> > > > > +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
>> > > > 
>> > > > IS_GEN9_BC || IS_BXT
>> > > > 
>> > > > would be a little easier to parse, me thinks.
>> > > 
>> > > I can do that, but what I really want is "DISPLAY_GEN(dev_priv) ==
>> > > 9".
>> > 
>> > work in progress...
>> > 
>> > btw...
>> > 
>> > DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?
>> 
>> It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.
>
> there's a macro defined on gen we end up never using
> IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9
>
> Should we just kill that or try to use more that instead of direct comparison?
>
> The advantage seems to be the use of bitmasks...
>
>> 
>> I would expect a macro called DISPLAY() to return *the* Display or to
>> simply display somewhere the thing I pass as an argument. Now
>> DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
>> generates a Display).
>
> what about IS_DISPLAY(dev_priv, 9) ?
> and IS_DISPLAY_RANGE(dev_priv, 5, 9)

Perhaps IS_DISPLAY_GEN(dev_priv, start, end).

*However* the naming and composition of the macro is *much* less
important than what the code ends up looking. Does the display gen
adequately cover the differences between platforms ultimately?

For example, a clear counter indication is that you'll be hard pressed
to express the current HAS_GMCH_DISPLAY() in terms of display gen in a
way that doesn't have to check for VLV/CHV. I don't think you can avoid
IS_GEMINILAKE() either, you'll end up using display gen 10 in some
places but Geminilake in others, ultimately making the end result worse
than the starting point.

BR,
Jani.


>
>> 
>> 
>> > 
>> > I'm play around here with:
>> > 
>> > #define DISPLAY(dev_priv, g)    (!!((dev_priv)->info.display_mask &
>> > BIT(g-1)))
>> > #define DISPLAY_RANGE(dev_priv, s, e) \
>> >                 (!!((dev_priv)->info.display_mask &
>> > INTEL_GEN_MASK((s), (e))))
>> > 
>> > thoughts? comments? ideas?
>> 
>> #define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)
>> 
>> > 
>> > > 
>> > > /me looks at Rodrigo
>> > > 
>> > > > 
>> > > > > +		/* Display WA #1125: skl,bxt,kbl */
>> > > > > +		if (level == 0 && wp->rc_surface)
>> > > > > +			res_blocks +=
>> > > > > +				fixed16_to_u32_round_up(wp-
>> > > > > > y_tile_minimum);
>> > > > > 
>> > > > > +
>> > > > > +		/* Display WA #1126: skl,bxt,kbl */
>> > > > > +		if (level >= 1 && level <= 7) {
>> > > > > +			if (wp->y_tiled) {
>> > > > > +				res_blocks +=
>> > > > > +				    fixed16_to_u32_round_up(wp
>> > > > > -
>> > > > > > y_tile_minimum);
>> > > > > 
>> > > > > +				res_lines += wp-
>> > > > > >y_min_scanlines;
>> > > > > +			} else {
>> > > > > +				res_blocks++;
>> > > > > +			}
>> > > > > 
>> > > > > -	/* Display WA #1126: skl,bxt,kbl,glk */
>> > > > > -	if (level >= 1 && level <= 7) {
>> > > > > -		if (wp->y_tiled) {
>> > > > > -			res_blocks += fixed16_to_u32_round_up(
>> > > > > -							wp-
>> > > > > > y_tile_minimum);
>> > > > > 
>> > > > > -			res_lines += wp->y_min_scanlines;
>> > > > > -		} else {
>> > > > > -			res_blocks++;
>> > > > > +			/*
>> > > > > +			 * Make sure result blocks for higher
>> > > > > latency levels are
>> > > > > +			 * atleast as high as level below the
>> > > > > current level.
>> > > > > +			 * Assumption in DDB algorithm
>> > > > > optimization for special
>> > > > > +			 * cases. Also covers Display WA #1125
>> > > > > for
>> > > > > RC.
>> > > > > +			 */
>> > > > > +			if (result_prev->plane_res_b >
>> > > > > res_blocks)
>> > > > > +				res_blocks = result_prev-
>> > > > > > plane_res_b;
>> > > > > 
>> > > > >  		}
>> > > > > -
>> > > > > -		/*
>> > > > > -		 * Make sure result blocks for higher latency
>> > > > > levels are atleast
>> > > > > -		 * as high as level below the current level.
>> > > > > -		 * Assumption in DDB algorithm optimization
>> > > > > for
>> > > > > special cases.
>> > > > > -		 * Also covers Display WA #1125 for RC.
>> > > > > -		 */
>> > > > > -		if (result_prev->plane_res_b > res_blocks)
>> > > > > -			res_blocks = result_prev->plane_res_b;
>> > > > 
>> > > > This last thing is part of the glk+ watermark formula as well.
>> > > >  But
>> > > > I'm not 100% convinced that it's needed.
>> > > 
>> > > I simply can't find where this is documented. WAs 1125 and 1126,
>> > > which
>> > > contain text that match this code exactly, are not applicable to
>> > > GLK.
>> > > Which BSpec page and paragraph/section mentions this?
>> > > 
>> > > 
>> > > >  One might assume that the the
>> > > > non-decrasing latency values guarantee that the resulting
>> > > > watermarks
>> > > > are also non-decreasing. But I've not actually done the math to
>> > > > see
>> > > > if
>> > > > that's true.
>> > > > 
>> > > > Hmm. It might not actually be true on account of the 'memory
>> > > > latency
>> > > > microseconds >= line time microseconds' check when selecting
>> > > > which
>> > > > method to use to calculate the watermark. Not quite sure which
>> > > > way
>> > > > that would make things go.
>> > > > 
>> > > > We also seem to be missing the res_lines handling here. But given
>> > > > that we only did this for compressed fbs before I don't think
>> > > > this
>> > > > patch is making things much worse by limiting this to pre-glk
>> > > > only.
>> > > > The glk+ handling and res_lines fix could be done as a followup.
>> > > > 
>> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > 
>> > > > 
>> > > > >  	}
>> > > > > 
>> > > > >  	if (INTEL_GEN(dev_priv) >= 11) {
>> > > > > --
>> > > > > 2.14.4
>> > > > > 
>> > > > > _______________________________________________
>> > > > > Intel-gfx mailing list
>> > > > > Intel-gfx@lists.freedesktop.org
>> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> > > > 
>> > > > 
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 08/11] drm/i915: reorganize the error message for invalid watermarks

[Ville Syrjälä]

On Mon, Oct 22, 2018 at 03:22:27PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-10-18 às 16:55 +0300, Ville Syrjälä escreveu:
> > On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote:
> > > Print a more generic "failed to compute watermark levels" whenever
> > > any
> > > of skl_compute_wm_levels() fail, and print only the specific error
> > > message for the specific cases. This allows us to stop passing
> > > pstate
> > > everywhere, making the watermarks computation code a little less
> > > dependent on random intel state structs.
> > 
> > Nothing random about those structs. They are *the* state.
> 
> What I'm about to say is all probably a reflex of my own incompetence
> to understand the flows of our code, but here it goes.
> 
> 1. There's duplication in those structs. At any given point of our
> source code, should you use state structs passed as parameters, or
> should you use object->state (e.g., intel_crtc->state)? Sometimes one
> thing is the new state and the other thing is the old state, sometimes
> it is the opposite, and checking which one is which is never trivial.

That was solved with the mlankhorst iterators. So no more foo->state,
except during readout since we don't currently allocate new states
for that.

> I
> always have to go back to intel_display.c and try to find that point in
> the modeset where we assign crtc_config to crtc->config and then try to
> figure out if my code runs before or after that point. And I'm never
> 100% confident I'm using the correct struct.

And Maarten has been busy elimninating crtc->config as well. Not too
many uses left I hope.

> 
> 2. There's a lot of duplication in members of those structs. There are
> like 5 different things that could mean "pixel rate" (for real mode,
> for adjusted mode, considering/not-considering scaling, rotation, etc),
> there are many things that could mean "source width", etc. So when you
> need a specific value, it's not always clear where to get it from in
> our driver.
> 
> 3. The names inside the watermarks calculation page (or anywhere else
> in our spec) don't easily translate to any of those struct members
> mentioned in item 2. E.g., plane pixel rate.
> 
> I mean, look at how many times the spec (not only for watermarks, but
> for other stuff too, like FBC and PSR) had wording like "source size"
> and we fetched the value from one place, but then we learned that we
> needed to fetch the value from this other place that was the same in
> most cases except for these X and Y corner cases. You reviewed some of
> those patches.

I don't recall many corner cases. Well, I guess you can count the
cursor as one. But that's about it I think.

Rotation was maybe the one that caused the most confusion but I think
that was because the spec was very vague. One actually has to think
a bit to figure out if it's referring to the fb orientation or the
scanout orientation.

> 
> So, to conclude the argument, the nice thing about struct skl_wm_params
> is that it allows us to have a single point where we translate "i915
> terminology" to "watermarks calculation algorithm terminology", so we
> can fix that single place if/when needed.

That can usually be solved with functions as well.

> And then everywhere else in
> watermarks code we just fetch the value from this struct without having
> to worry "at this point in the sequence, where should this value come
> from?", allowing us to focus on the algorithm specifically. On top of
> that, there's the fact that we only compute things once instead of up
> to 8 times per plane (7 levels + transition watermarks).
> 
> Of course, the downside is that we address the problem of "too many
> places to fetch information from" by introducing a new place where
> information is fetched from, so I definitely can't argue that the
> current solution is good either. I would really like to know what are
> your proposals here: if we decide to remove the params struct, would
> you suggest we simply fetch everything directly from the passed structs
> and recompute all the values that are computed multiple times? I'm
> totally willing to implement something better if you have it in your
> head.

The "let's not recompute the same thing many times" argument may
have some merit. Though most things are perhaps cheap enough to not
matter much. And we could always stick more stuff into the plane/crtc
states.

But I don't have any concrete proposals at this time. I've tried to
stay away from the skl wm code as much as possible so as to not get
an urge to rewrite it :P

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

Re: [PATCH 07/11] drm/i915: move ddb_blocks to be a watermark parameter

[Ville Syrjälä]

On Mon, Oct 22, 2018 at 03:29:22PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-10-18 às 16:41 +0300, Ville Syrjälä escreveu:
> > On Tue, Oct 16, 2018 at 03:01:29PM -0700, Paulo Zanoni wrote:
> > > The goal of struct skl_wm_params is to cache every watermark
> > > parameter so the other functions can just use them without worrying
> > > about the appropriate place to fetch each parameter requested by
> > > the
> > > spec, and without having to recompute parameters that are used in
> > > different steps of the calculation.
> > > 
> > > The ddb_blocks parameter is one that is used by both the the plane
> > > watermarks and the transition watermarks. Move ddb_blocks to the
> > > parameter struct so we can simplify the code.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++-------------
> > > ------------
> > >  2 files changed, 18 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4b1e8471609b..b32d680d9bf0 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1255,6 +1255,7 @@ struct skl_wm_params {
> > >  	bool rc_surface;
> > >  	bool is_planar;
> > >  	uint32_t width;
> > > +	uint16_t ddb_blocks;
> > >  	uint8_t cpp;
> > >  	uint32_t plane_pixel_rate;
> > >  	uint32_t y_min_scanlines;
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index f388bfa99a97..4053f4a68657 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4522,11 +4522,13 @@ static int
> > >  skl_compute_plane_wm_params(const struct drm_i915_private
> > > *dev_priv,
> > >  			    struct intel_crtc_state *cstate,
> > >  			    const struct intel_plane_state
> > > *intel_pstate,
> > > +			    const struct skl_ddb_allocation *ddb,
> > >  			    struct skl_wm_params *wp, int
> > > plane_id)
> > >  {
> > >  	struct intel_plane *plane = to_intel_plane(intel_pstate-
> > > >base.plane);
> > >  	const struct drm_plane_state *pstate = &intel_pstate-
> > > >base;
> > >  	const struct drm_framebuffer *fb = pstate->fb;
> > > +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> > >  	uint32_t interm_pbpl;
> > >  	struct intel_atomic_state *state =
> > >  		to_intel_atomic_state(cstate->base.state);
> > > @@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct
> > > drm_i915_private *dev_priv,
> > >  	wp->linetime_us = fixed16_to_u32_round_up(
> > >  					intel_get_linetime_us(csta
> > > te));
> > >  
> > > +	wp->ddb_blocks = plane_id ?
> > 
> > Ugh. That 'plane_id' really should be renamed to something else.
> > 'color_plane' would be the newfangled name I started to use
> > elsewhere.
> 
> I completely agree: there's plane_id and plane->id and they mean
> different things.
> 
> If you're already using color_plane I can totally go with it so we have
> consistency. But, well, for RGB planes, plane 0 does contain color
> information... But can I suggest us to use "subplane" terminology when
> talking about the planes of a plane? What about subplane_index?
> Something that does not use the word "plane" is probably better.

What is "planes of a plane"?

There are drm_planes, hardware display planes (1:1 with drm_plane
for us), and there are the planes of a planar framebuffer (luma
plane, chroma plane, etc.). The latter is what I christened
"color plane".

> 
> > 
> > Patch looks good to me.
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > > +		     skl_ddb_entry_size(&ddb-
> > > >uv_plane[pipe][plane->id]) :
> > > +		     skl_ddb_entry_size(&ddb->plane[pipe][plane-
> > > >id]);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > >  static int skl_compute_plane_wm(const struct drm_i915_private
> > > *dev_priv,
> > >  				struct intel_crtc_state *cstate,
> > >  				const struct intel_plane_state
> > > *intel_pstate,
> > > -				uint16_t ddb_allocation,
> > >  				int level,
> > >  				const struct skl_wm_params *wp,
> > >  				const struct skl_wm_level
> > > *result_prev,
> > > @@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct
> > > drm_i915_private *dev_priv,
> > >  		     wp->dbuf_block_size < 1) &&
> > >  		     (wp->plane_bytes_per_line / wp-
> > > >dbuf_block_size < 1)) {
> > >  			selected_result = method2;
> > > -		} else if (ddb_allocation >=
> > > +		} else if (wp->ddb_blocks >=
> > >  			 fixed16_to_u32_round_up(wp-
> > > >plane_blocks_per_line)) {
> > >  			if (INTEL_GEN(dev_priv) == 9 &&
> > >  			    !IS_GEMINILAKE(dev_priv))
> > > @@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct
> > > drm_i915_private *dev_priv,
> > >  	}
> > >  
> > >  	if ((level > 0 && res_lines > 31) ||
> > > -	    res_blocks >= ddb_allocation ||
> > > -	    min_disp_buf_needed >= ddb_allocation) {
> > > +	    res_blocks >= wp->ddb_blocks ||
> > > +	    min_disp_buf_needed >= wp->ddb_blocks) {
> > >  		result->plane_en = false;
> > >  
> > >  		/*
> > > @@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct
> > > drm_i915_private *dev_priv,
> > >  			DRM_DEBUG_KMS("Requested display
> > > configuration exceeds system watermark limitations\n");
> > >  			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks
> > > required = %u/%u, lines required = %u/31\n",
> > >  				      plane->base.id, plane->name,
> > > -				      res_blocks, ddb_allocation,
> > > res_lines);
> > > +				      res_blocks, wp->ddb_blocks,
> > > res_lines);
> > >  			return -EINVAL;
> > >  		}
> > >  	}
> > > @@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const
> > > struct drm_i915_private *dev_priv,
> > >  
> > >  static int
> > >  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > > -		      struct skl_ddb_allocation *ddb,
> > >  		      struct intel_crtc_state *cstate,
> > >  		      const struct intel_plane_state
> > > *intel_pstate,
> > >  		      const struct skl_wm_params *wm_params,
> > >  		      struct skl_plane_wm *wm,
> > >  		      int plane_id)
> > >  {
> > > -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > > >base.crtc);
> > > -	struct drm_plane *plane = intel_pstate->base.plane;
> > > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > -	uint16_t ddb_blocks;
> > > -	enum pipe pipe = intel_crtc->pipe;
> > >  	int level, max_level = ilk_wm_max_level(dev_priv);
> > > -	enum plane_id intel_plane_id = intel_plane->id;
> > >  	int ret;
> > >  
> > > -	ddb_blocks = plane_id ?
> > > -		     skl_ddb_entry_size(&ddb-
> > > >uv_plane[pipe][intel_plane_id]) :
> > > -		     skl_ddb_entry_size(&ddb-
> > > >plane[pipe][intel_plane_id]);
> > > -
> > >  	for (level = 0; level <= max_level; level++) {
> > >  		struct skl_wm_level *result = plane_id ? &wm-
> > > >uv_wm[level] :
> > >  							  &wm-
> > > >wm[level];
> > > @@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct
> > > drm_i915_private *dev_priv,
> > >  		ret = skl_compute_plane_wm(dev_priv,
> > >  					   cstate,
> > >  					   intel_pstate,
> > > -					   ddb_blocks,
> > >  					   level,
> > >  					   wm_params,
> > >  					   result_prev,
> > > @@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct
> > > intel_crtc_state *cstate)
> > >  static void skl_compute_transition_wm(struct intel_crtc_state
> > > *cstate,
> > >  				      struct skl_wm_params *wp,
> > >  				      struct skl_wm_level *wm_l0,
> > > -				      uint16_t ddb_allocation,
> > >  				      struct skl_wm_level
> > > *trans_wm /* out */)
> > >  {
> > >  	struct drm_device *dev = cstate->base.crtc->dev;
> > > @@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct
> > > intel_crtc_state *cstate,
> > >  
> > >  	res_blocks += 1;
> > >  
> > > -	if (res_blocks < ddb_allocation) {
> > > +	if (res_blocks < wp->ddb_blocks) {
> > >  		trans_wm->plane_res_b = res_blocks;
> > >  		trans_wm->plane_en = true;
> > >  		return;
> > > @@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct
> > > intel_crtc_state *cstate,
> > >  						to_intel_plane_sta
> > > te(pstate);
> > >  		enum plane_id plane_id = to_intel_plane(plane)-
> > > >id;
> > >  		struct skl_wm_params wm_params;
> > > -		enum pipe pipe = to_intel_crtc(cstate->base.crtc)-
> > > >pipe;
> > > -		uint16_t ddb_blocks;
> > >  
> > >  		wm = &pipe_wm->planes[plane_id];
> > > -		ddb_blocks = skl_ddb_entry_size(&ddb-
> > > >plane[pipe][plane_id]);
> > >  
> > >  		ret = skl_compute_plane_wm_params(dev_priv,
> > > cstate,
> > > -						  intel_pstate,
> > > &wm_params, 0);
> > > +						  intel_pstate,
> > > ddb,
> > > +						  &wm_params, 0);
> > >  		if (ret)
> > >  			return ret;
> > >  
> > >  		if (!wm_params.plane_visible)
> > >  			continue;
> > >  
> > > -		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> > > +		ret = skl_compute_wm_levels(dev_priv, cstate,
> > >  					    intel_pstate,
> > > &wm_params, wm, 0);
> > >  		if (ret)
> > >  			return ret;
> > >  
> > >  		skl_compute_transition_wm(cstate, &wm_params, &wm-
> > > >wm[0],
> > > -					  ddb_blocks, &wm-
> > > >trans_wm);
> > > +					  &wm->trans_wm);
> > >  
> > >  		/* uv plane watermarks must also be validated for
> > > NV12/Planar */
> > >  		if (wm_params.is_planar) {
> > >  			ret =
> > > skl_compute_plane_wm_params(dev_priv, cstate,
> > > -							  intel_ps
> > > tate,
> > > +							  intel_ps
> > > tate, ddb,
> > >  							  &wm_para
> > > ms, 1);
> > >  			if (ret)
> > >  				return ret;
> > >  
> > > -			ret = skl_compute_wm_levels(dev_priv, ddb,
> > > cstate,
> > > +			ret = skl_compute_wm_levels(dev_priv,
> > > cstate,
> > >  						    intel_pstate,
> > > &wm_params,
> > >  						    wm, 1);
> > >  			if (ret)
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 

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

Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Rodrigo Vivi]

On Tue, Oct 23, 2018 at 10:30:18AM +0300, Jani Nikula wrote:
> On Mon, 22 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Mon, Oct 22, 2018 at 05:12:18PM -0700, Paulo Zanoni wrote:
> >> Em Seg, 2018-10-22 às 16:55 -0700, Rodrigo Vivi escreveu:
> >> > On Mon, Oct 22, 2018 at 04:32:00PM -0700, Paulo Zanoni wrote:
> >> > > Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
> >> > > > On Tue, Oct 16, 2018 at 03:01:23PM -0700, Paulo Zanoni wrote:
> >> > > > > BSpec does not show these WAs as applicable to GLK, and for CNL
> >> > > > > it
> >> > > > > only shows them applicable for a super early pre-production
> >> > > > > stepping
> >> > > > > we shouldn't be caring about anymore. Remove these so we can
> >> > > > > avoid
> >> > > > > them on ICL too.
> >> > > > > 
> >> > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> >> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > > > > ---
> >> > > > >  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
> >> > > > > ----
> >> > > > > ------------
> >> > > > >  1 file changed, 23 insertions(+), 20 deletions(-)
> >> > > > > 
> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> >> > > > > b/drivers/gpu/drm/i915/intel_pm.c
> >> > > > > index 67a4d0735291..18157c6ee126 100644
> >> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> >> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> > > > > @@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
> >> > > > > struct drm_i915_private *dev_priv,
> >> > > > >  	res_lines = div_round_up_fixed16(selected_result,
> >> > > > >  					 wp-
> >> > > > > > plane_blocks_per_line);
> >> > > > > 
> >> > > > > -	/* Display WA #1125: skl,bxt,kbl,glk */
> >> > > > > -	if (level == 0 && wp->rc_surface)
> >> > > > > -		res_blocks += fixed16_to_u32_round_up(wp-
> >> > > > > > y_tile_minimum);
> >> > > > > 
> >> > > > > +	if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> >> > > > 
> >> > > > IS_GEN9_BC || IS_BXT
> >> > > > 
> >> > > > would be a little easier to parse, me thinks.
> >> > > 
> >> > > I can do that, but what I really want is "DISPLAY_GEN(dev_priv) ==
> >> > > 9".
> >> > 
> >> > work in progress...
> >> > 
> >> > btw...
> >> > 
> >> > DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?
> >> 
> >> It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.
> >
> > there's a macro defined on gen we end up never using
> > IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9
> >
> > Should we just kill that or try to use more that instead of direct comparison?
> >
> > The advantage seems to be the use of bitmasks...
> >
> >> 
> >> I would expect a macro called DISPLAY() to return *the* Display or to
> >> simply display somewhere the thing I pass as an argument. Now
> >> DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
> >> generates a Display).
> >
> > what about IS_DISPLAY(dev_priv, 9) ?
> > and IS_DISPLAY_RANGE(dev_priv, 5, 9)
> 
> Perhaps IS_DISPLAY_GEN(dev_priv, start, end).
> 
> *However* the naming and composition of the macro is *much* less
> important than what the code ends up looking.

I fully agree. For this reason I started with the already existent GEN checks.

> Does the display gen
> adequately cover the differences between platforms ultimately?

This thought got me starting many different attempts and then
using git reset --hard HEAD
so many times this week.

No, this is not enough to cover all the needs. Geminilake right
now is the only exception case. We could have another case on internal
right now where we could use same approach to make it better.

But still the ugliest part can be the intersaction and gray areas with
GEM part.

> 
> For example, a clear counter indication is that you'll be hard pressed
> to express the current HAS_GMCH_DISPLAY() in terms of display gen in a
> way that doesn't have to check for VLV/CHV.

Yeap, I don't think this display_gen solve this case. Although maybe
the range can help a bit to reduce the gmch_display checks.

> I don't think you can avoid
> IS_GEMINILAKE() either, you'll end up using display gen 10 in some
> places but Geminilake in others, ultimately making the end result worse
> than the starting point.

Well... I decided to give a try starting for the glk case to see
how it gets.

https://github.com/vivijim/drm-intel/tree/display_gen

Could you please take a look and let me know what do you think?

This version is taking display_gen to extreme, but we could
have a reduced one with glk checks inside bxt and dsi functions.

well, either way one fact is that we already have a code that is mixed with
IS_GEN and platform codename checks. Even if we still have some cases
where platform codename checks is unavoidable maybe it is better if we
prefer gen or display gen over platform codename when possible.

At least it reduces the mixed cases and gets easier to add
new platforms.

Thanks,
Rodrigo.

> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> 
> >> > 
> >> > I'm play around here with:
> >> > 
> >> > #define DISPLAY(dev_priv, g)    (!!((dev_priv)->info.display_mask &
> >> > BIT(g-1)))
> >> > #define DISPLAY_RANGE(dev_priv, s, e) \
> >> >                 (!!((dev_priv)->info.display_mask &
> >> > INTEL_GEN_MASK((s), (e))))
> >> > 
> >> > thoughts? comments? ideas?
> >> 
> >> #define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)
> >> 
> >> > 
> >> > > 
> >> > > /me looks at Rodrigo
> >> > > 
> >> > > > 
> >> > > > > +		/* Display WA #1125: skl,bxt,kbl */
> >> > > > > +		if (level == 0 && wp->rc_surface)
> >> > > > > +			res_blocks +=
> >> > > > > +				fixed16_to_u32_round_up(wp-
> >> > > > > > y_tile_minimum);
> >> > > > > 
> >> > > > > +
> >> > > > > +		/* Display WA #1126: skl,bxt,kbl */
> >> > > > > +		if (level >= 1 && level <= 7) {
> >> > > > > +			if (wp->y_tiled) {
> >> > > > > +				res_blocks +=
> >> > > > > +				    fixed16_to_u32_round_up(wp
> >> > > > > -
> >> > > > > > y_tile_minimum);
> >> > > > > 
> >> > > > > +				res_lines += wp-
> >> > > > > >y_min_scanlines;
> >> > > > > +			} else {
> >> > > > > +				res_blocks++;
> >> > > > > +			}
> >> > > > > 
> >> > > > > -	/* Display WA #1126: skl,bxt,kbl,glk */
> >> > > > > -	if (level >= 1 && level <= 7) {
> >> > > > > -		if (wp->y_tiled) {
> >> > > > > -			res_blocks += fixed16_to_u32_round_up(
> >> > > > > -							wp-
> >> > > > > > y_tile_minimum);
> >> > > > > 
> >> > > > > -			res_lines += wp->y_min_scanlines;
> >> > > > > -		} else {
> >> > > > > -			res_blocks++;
> >> > > > > +			/*
> >> > > > > +			 * Make sure result blocks for higher
> >> > > > > latency levels are
> >> > > > > +			 * atleast as high as level below the
> >> > > > > current level.
> >> > > > > +			 * Assumption in DDB algorithm
> >> > > > > optimization for special
> >> > > > > +			 * cases. Also covers Display WA #1125
> >> > > > > for
> >> > > > > RC.
> >> > > > > +			 */
> >> > > > > +			if (result_prev->plane_res_b >
> >> > > > > res_blocks)
> >> > > > > +				res_blocks = result_prev-
> >> > > > > > plane_res_b;
> >> > > > > 
> >> > > > >  		}
> >> > > > > -
> >> > > > > -		/*
> >> > > > > -		 * Make sure result blocks for higher latency
> >> > > > > levels are atleast
> >> > > > > -		 * as high as level below the current level.
> >> > > > > -		 * Assumption in DDB algorithm optimization
> >> > > > > for
> >> > > > > special cases.
> >> > > > > -		 * Also covers Display WA #1125 for RC.
> >> > > > > -		 */
> >> > > > > -		if (result_prev->plane_res_b > res_blocks)
> >> > > > > -			res_blocks = result_prev->plane_res_b;
> >> > > > 
> >> > > > This last thing is part of the glk+ watermark formula as well.
> >> > > >  But
> >> > > > I'm not 100% convinced that it's needed.
> >> > > 
> >> > > I simply can't find where this is documented. WAs 1125 and 1126,
> >> > > which
> >> > > contain text that match this code exactly, are not applicable to
> >> > > GLK.
> >> > > Which BSpec page and paragraph/section mentions this?
> >> > > 
> >> > > 
> >> > > >  One might assume that the the
> >> > > > non-decrasing latency values guarantee that the resulting
> >> > > > watermarks
> >> > > > are also non-decreasing. But I've not actually done the math to
> >> > > > see
> >> > > > if
> >> > > > that's true.
> >> > > > 
> >> > > > Hmm. It might not actually be true on account of the 'memory
> >> > > > latency
> >> > > > microseconds >= line time microseconds' check when selecting
> >> > > > which
> >> > > > method to use to calculate the watermark. Not quite sure which
> >> > > > way
> >> > > > that would make things go.
> >> > > > 
> >> > > > We also seem to be missing the res_lines handling here. But given
> >> > > > that we only did this for compressed fbs before I don't think
> >> > > > this
> >> > > > patch is making things much worse by limiting this to pre-glk
> >> > > > only.
> >> > > > The glk+ handling and res_lines fix could be done as a followup.
> >> > > > 
> >> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > > > 
> >> > > > 
> >> > > > >  	}
> >> > > > > 
> >> > > > >  	if (INTEL_GEN(dev_priv) >= 11) {
> >> > > > > --
> >> > > > > 2.14.4
> >> > > > > 
> >> > > > > _______________________________________________
> >> > > > > Intel-gfx mailing list
> >> > > > > Intel-gfx@lists.freedesktop.org
> >> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> > > > 
> >> > > > 
> >> > 
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Paulo Zanoni]

BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.

v2: Change how we check for gen9 display platforms (Ville).

Cc: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9da8ff263d36..b5623a70c19c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4754,28 +4754,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	res_lines = div_round_up_fixed16(selected_result,
 					 wp->plane_blocks_per_line);
 
-	/* Display WA #1125: skl,bxt,kbl,glk */
-	if (level == 0 && wp->rc_surface)
-		res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
+	if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv)) {
+		/* Display WA #1125: skl,bxt,kbl */
+		if (level == 0 && wp->rc_surface)
+			res_blocks +=
+				fixed16_to_u32_round_up(wp->y_tile_minimum);
+
+		/* Display WA #1126: skl,bxt,kbl */
+		if (level >= 1 && level <= 7) {
+			if (wp->y_tiled) {
+				res_blocks +=
+				    fixed16_to_u32_round_up(wp->y_tile_minimum);
+				res_lines += wp->y_min_scanlines;
+			} else {
+				res_blocks++;
+			}
 
-	/* Display WA #1126: skl,bxt,kbl,glk */
-	if (level >= 1 && level <= 7) {
-		if (wp->y_tiled) {
-			res_blocks += fixed16_to_u32_round_up(
-							wp->y_tile_minimum);
-			res_lines += wp->y_min_scanlines;
-		} else {
-			res_blocks++;
+			/*
+			 * Make sure result blocks for higher latency levels are
+			 * atleast as high as level below the current level.
+			 * Assumption in DDB algorithm optimization for special
+			 * cases. Also covers Display WA #1125 for RC.
+			 */
+			if (result_prev->plane_res_b > res_blocks)
+				res_blocks = result_prev->plane_res_b;
 		}
-
-		/*
-		 * Make sure result blocks for higher latency levels are atleast
-		 * as high as level below the current level.
-		 * Assumption in DDB algorithm optimization for special cases.
-		 * Also covers Display WA #1125 for RC.
-		 */
-		if (result_prev->plane_res_b > res_blocks)
-			res_blocks = result_prev->plane_res_b;
 	}
 
 	if (INTEL_GEN(dev_priv) >= 11) {
-- 
2.14.4

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

✗ Fi.CI.BAT: failure for More watermarks improvements (rev2)

[Patchwork]

== Series Details ==

Series: More watermarks improvements (rev2)
URL   : https://patchwork.freedesktop.org/series/51086/
State : failure

== Summary ==

Applying: drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
Applying: drm/i915: remove padding from struct skl_wm_level
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915: fix handling of invisible planes in watermarks code
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.h
M	drivers/gpu/drm/i915/intel_pm.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_pm.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_pm.c
Auto-merging drivers/gpu/drm/i915/i915_drv.h
error: Failed to merge in the changes.
Patch failed at 0003 drm/i915: fix handling of invisible planes in watermarks code
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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