[PATCH v3 0/7] drm/i915/wm: some clean-ups and a bit of refactoring

[PATCH v3 0/7] drm/i915/wm: some clean-ups and a bit of refactoring

[Luca Coelho]

Hi,

This is v3 of my initial clean-ups and small refactoring in
preparation for further refactoring of the watermark code.

Please review.  Changes in v3 described in the relevant patches.

Cheers,
Luca.


Luca Coelho (7):
  drm/i915/wm: clarify watermark ops with comments
  drm/i915/wm: move intel_sagv_init() to avoid forward declaration
  drm/i915/wm: remove stale FIXME in skl_needs_memory_bw_wa()
  drm/i915/wm: convert x/y-tiling bools to an enum
  drm/i915/wm: convert tiling mode check in slk_compute_plane_wm() to a
    switch-case
  drm/i915/wm: move method selection and calculation to a separate
    function
  drm/i915/wm: reduce size of y_min_scanlines to u8

 .../gpu/drm/i915/display/intel_display_core.h |   6 +-
 drivers/gpu/drm/i915/display/skl_watermark.c  | 166 +++++++++++-------
 2 files changed, 106 insertions(+), 66 deletions(-)

-- 
2.51.0

[PATCH v3 1/7] drm/i915/wm: clarify watermark ops with comments

[Luca Coelho]

Some of the ops in struct intel_wm_funcs are used only for legacy
watermark management, while others are only for SKL+ or both.  Clarify
that in the struct definition.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_core.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index df4da52cbdb3..7144b61fb1ff 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -78,7 +78,7 @@ struct intel_display_funcs {
 
 /* functions used for watermark calcs for display. */
 struct intel_wm_funcs {
-	/* update_wm is for legacy wm management */
+	/* these are only for legacy wm management */
 	void (*update_wm)(struct intel_display *display);
 	int (*compute_watermarks)(struct intel_atomic_state *state,
 				  struct intel_crtc *crtc);
@@ -88,8 +88,12 @@ struct intel_wm_funcs {
 					 struct intel_crtc *crtc);
 	void (*optimize_watermarks)(struct intel_atomic_state *state,
 				    struct intel_crtc *crtc);
+
+	/* these are for SKL+ wm management */
 	int (*compute_global_watermarks)(struct intel_atomic_state *state);
 	void (*get_hw_state)(struct intel_display *display);
+
+	/* this is used by both legacy and SKL+ */
 	void (*sanitize)(struct intel_display *display);
 };
 
-- 
2.51.0

[PATCH v3 2/7] drm/i915/wm: move intel_sagv_init() to avoid forward declaration

[Luca Coelho]

There's no need to have a forward-declaration for skl_sagv_disable(),
so move the intel_sagv_init() function below the called function to
prevent it.

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 60 ++++++++++----------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 9df9ee137bf9..5e69fe034d6a 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -53,8 +53,6 @@ struct intel_dbuf_state {
 #define intel_atomic_get_new_dbuf_state(state) \
 	to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_intel_display(state)->dbuf.obj))
 
-static void skl_sagv_disable(struct intel_display *display);
-
 /* Stores plane specific WM parameters */
 struct skl_wm_params {
 	bool x_tiled, y_tiled;
@@ -130,35 +128,6 @@ intel_sagv_block_time(struct intel_display *display)
 	}
 }
 
-static void intel_sagv_init(struct intel_display *display)
-{
-	if (!HAS_SAGV(display))
-		display->sagv.status = I915_SAGV_NOT_CONTROLLED;
-
-	/*
-	 * Probe to see if we have working SAGV control.
-	 * For icl+ this was already determined by intel_bw_init_hw().
-	 */
-	if (DISPLAY_VER(display) < 11)
-		skl_sagv_disable(display);
-
-	drm_WARN_ON(display->drm, display->sagv.status == I915_SAGV_UNKNOWN);
-
-	display->sagv.block_time_us = intel_sagv_block_time(display);
-
-	drm_dbg_kms(display->drm, "SAGV supported: %s, original SAGV block time: %u us\n",
-		    str_yes_no(intel_has_sagv(display)), display->sagv.block_time_us);
-
-	/* avoid overflow when adding with wm0 latency/etc. */
-	if (drm_WARN(display->drm, display->sagv.block_time_us > U16_MAX,
-		     "Excessive SAGV block time %u, ignoring\n",
-		     display->sagv.block_time_us))
-		display->sagv.block_time_us = 0;
-
-	if (!intel_has_sagv(display))
-		display->sagv.block_time_us = 0;
-}
-
 /*
  * SAGV dynamically adjusts the system agent voltage and clock frequencies
  * depending on power and performance requirements. The display engine access
@@ -233,6 +202,35 @@ static void skl_sagv_disable(struct intel_display *display)
 	display->sagv.status = I915_SAGV_DISABLED;
 }
 
+static void intel_sagv_init(struct intel_display *display)
+{
+	if (!HAS_SAGV(display))
+		display->sagv.status = I915_SAGV_NOT_CONTROLLED;
+
+	/*
+	 * Probe to see if we have working SAGV control.
+	 * For icl+ this was already determined by intel_bw_init_hw().
+	 */
+	if (DISPLAY_VER(display) < 11)
+		skl_sagv_disable(display);
+
+	drm_WARN_ON(display->drm, display->sagv.status == I915_SAGV_UNKNOWN);
+
+	display->sagv.block_time_us = intel_sagv_block_time(display);
+
+	drm_dbg_kms(display->drm, "SAGV supported: %s, original SAGV block time: %u us\n",
+		    str_yes_no(intel_has_sagv(display)), display->sagv.block_time_us);
+
+	/* avoid overflow when adding with wm0 latency/etc. */
+	if (drm_WARN(display->drm, display->sagv.block_time_us > U16_MAX,
+		     "Excessive SAGV block time %u, ignoring\n",
+		     display->sagv.block_time_us))
+		display->sagv.block_time_us = 0;
+
+	if (!intel_has_sagv(display))
+		display->sagv.block_time_us = 0;
+}
+
 static void skl_sagv_pre_plane_update(struct intel_atomic_state *state)
 {
 	struct intel_display *display = to_intel_display(state);
-- 
2.51.0

[PATCH v3 3/7] drm/i915/wm: remove stale FIXME in skl_needs_memory_bw_wa()

[Luca Coelho]

This FIXME has been there forever and apparently the _proper code_ has
never been added, and, since it's a very old platform alreday, most
likely never will.  It hasn't been a problem to keep the workaround
for all cases, so let's drop the FIXME tag.

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 5e69fe034d6a..3a982458395e 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -83,8 +83,8 @@ u8 intel_enabled_dbuf_slices_mask(struct intel_display *display)
 }
 
 /*
- * 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.
+ * We 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_display *display)
 {
-- 
2.51.0

[PATCH v3 4/7] drm/i915/wm: convert x/y-tiling bools to an enum

[Luca Coelho]

There are currently two booleans to define three tiling modes, which
is bad practice because it allows representing an invalid mode.  In
order to simplify this, convert these two booleans into one
enumeration with three possible tiling modes.

Additionally, introduce the concept of Y "family" of tiling, which
groups Y, Yf and 4 tiling, since they're effectively treated in the
same way in the watermark calculations.  Describe the grouping in the
enumeration definition.

v2: - remove redundant "tiled" and get rid of "family" in the enums (Ville)
    - remove holes introduced in the skl_wm_params struct (Ville)
    - improve if-else block to avoid intel_fb_is_tiled_modifier() call (Ville)

v3:
    - fix rebase damage (Jani)

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 51 ++++++++++++++------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 3a982458395e..f3af372767d0 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -53,13 +53,20 @@ struct intel_dbuf_state {
 #define intel_atomic_get_new_dbuf_state(state) \
 	to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_intel_display(state)->dbuf.obj))
 
+/* Tiling mode groups relevant to WM calculations */
+enum wm_tiling_mode {
+	WM_TILING_LINEAR,
+	WM_TILING_X,
+	WM_TILING_Y_Yf_4,
+};
+
 /* Stores plane specific WM parameters */
 struct skl_wm_params {
-	bool x_tiled, y_tiled;
-	bool rc_surface;
-	bool is_planar;
+	enum wm_tiling_mode tiling;
 	u32 width;
 	u8 cpp;
+	bool rc_surface;
+	bool is_planar;
 	u32 plane_pixel_rate;
 	u32 y_min_scanlines;
 	u32 plane_bytes_per_line;
@@ -618,7 +625,8 @@ static unsigned int skl_wm_latency(struct intel_display *display, int level,
 	     display->platform.cometlake) && skl_watermark_ipc_enabled(display))
 		latency += 4;
 
-	if (skl_needs_memory_bw_wa(display) && wp && wp->x_tiled)
+	if (skl_needs_memory_bw_wa(display) &&
+	    wp && wp->tiling == WM_TILING_X)
 		latency += 15;
 
 	return latency;
@@ -1659,9 +1667,13 @@ skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
 		return -EINVAL;
 	}
 
-	wp->x_tiled = modifier == I915_FORMAT_MOD_X_TILED;
-	wp->y_tiled = modifier != I915_FORMAT_MOD_X_TILED &&
-		intel_fb_is_tiled_modifier(modifier);
+	if (modifier == DRM_FORMAT_MOD_LINEAR)
+		wp->tiling = WM_TILING_LINEAR;
+	else if (modifier == I915_FORMAT_MOD_X_TILED)
+		wp->tiling = WM_TILING_X;
+	else
+		wp->tiling = WM_TILING_Y_Yf_4;
+
 	wp->rc_surface = intel_fb_is_ccs_modifier(modifier);
 	wp->is_planar = intel_format_info_is_yuv_semiplanar(format, modifier);
 
@@ -1701,7 +1713,7 @@ skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
 		wp->y_min_scanlines *= 2;
 
 	wp->plane_bytes_per_line = wp->width * wp->cpp;
-	if (wp->y_tiled) {
+	if (wp->tiling == WM_TILING_Y_Yf_4) {
 		interm_pbpl = DIV_ROUND_UP(wp->plane_bytes_per_line *
 					   wp->y_min_scanlines,
 					   wp->dbuf_block_size);
@@ -1717,7 +1729,7 @@ skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
 		interm_pbpl = DIV_ROUND_UP(wp->plane_bytes_per_line,
 					   wp->dbuf_block_size);
 
-		if (!wp->x_tiled || DISPLAY_VER(display) >= 10)
+		if (wp->tiling != WM_TILING_X || DISPLAY_VER(display) >= 10)
 			interm_pbpl++;
 
 		wp->plane_blocks_per_line = u32_to_fixed16(interm_pbpl);
@@ -1805,9 +1817,20 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 				 latency,
 				 wp->plane_blocks_per_line);
 
-	if (wp->y_tiled) {
+	switch (wp->tiling) {
+	case WM_TILING_Y_Yf_4:
 		selected_result = max_fixed16(method2, wp->y_tile_minimum);
-	} else {
+		break;
+
+	default:
+		MISSING_CASE(wp->tiling);
+		fallthrough;
+	case WM_TILING_LINEAR:
+	case WM_TILING_X:
+		/*
+		 * Special case for unrealistically small horizontal
+		 * total with plane downscaling.
+		 */
 		if ((wp->cpp * crtc_state->hw.pipe_mode.crtc_htotal /
 		     wp->dbuf_block_size < 1) &&
 		     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
@@ -1855,7 +1878,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 
 		/* Display WA #1126: skl,bxt,kbl */
 		if (level >= 1 && level <= 7) {
-			if (wp->y_tiled) {
+			if (wp->tiling == WM_TILING_Y_Yf_4) {
 				blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
 				lines += wp->y_min_scanlines;
 			} else {
@@ -1877,7 +1900,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 	lines = max_t(u32, lines, result_prev->lines);
 
 	if (DISPLAY_VER(display) >= 11) {
-		if (wp->y_tiled) {
+		if (wp->tiling == WM_TILING_Y_Yf_4) {
 			int extra_lines;
 
 			if (lines % wp->y_min_scanlines == 0)
@@ -2003,7 +2026,7 @@ static void skl_compute_transition_wm(struct intel_display *display,
 	 */
 	wm0_blocks = wm0->blocks - 1;
 
-	if (wp->y_tiled) {
+	if (wp->tiling == WM_TILING_Y_Yf_4) {
 		trans_y_tile_min =
 			(u16)mul_round_up_u32_fixed16(2, wp->y_tile_minimum);
 		blocks = max(wm0_blocks, trans_y_tile_min) + trans_offset;
-- 
2.51.0

[PATCH v3 5/7] drm/i915/wm: convert tiling mode check in slk_compute_plane_wm() to a switch-case

[Luca Coelho]

Make the code a bit clearer by using a switch-case to check the tiling
mode in skl_compute_plane_wm(), because all the possible states and
the calculations they use are explicitly handled.

v2: - Remove useless comment (Gustavo)
    - Move the default case above linear as a fallthrough (Gustavo)

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index f3af372767d0..35a73ddc282b 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -1833,7 +1833,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 		 */
 		if ((wp->cpp * crtc_state->hw.pipe_mode.crtc_htotal /
 		     wp->dbuf_block_size < 1) &&
-		     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
+		    (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
 			selected_result = method2;
 		} else if (latency >= wp->linetime_us) {
 			if (DISPLAY_VER(display) == 9)
@@ -1841,8 +1841,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 			else
 				selected_result = method2;
 		} else {
+			/* everything else with linear/X-tiled uses method 1 */
 			selected_result = method1;
 		}
+		break;
+
 	}
 
 	blocks = fixed16_to_u32_round_up(selected_result);
-- 
2.51.0

[PATCH v3 6/7] drm/i915/wm: move method selection and calculation to a separate function

[Luca Coelho]

Isolate the code that handles method selection and calculation, so
skl_compute_plane_wm() doesn't get too long.

v2: - Rebased after moving the default case change to an earlier patch

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 44 +++++++++++++-------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 35a73ddc282b..fc771f476bbe 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -1790,25 +1790,14 @@ static bool xe3_auto_min_alloc_capable(struct intel_plane *plane, int level)
 	return DISPLAY_VER(display) >= 30 && level == 0 && plane->id != PLANE_CURSOR;
 }
 
-static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
-				 struct intel_plane *plane,
-				 int level,
-				 unsigned int latency,
-				 const struct skl_wm_params *wp,
-				 const struct skl_wm_level *result_prev,
-				 struct skl_wm_level *result /* out */)
+static uint_fixed_16_16_t
+skl_wm_run_method(struct intel_display *display,
+		  const struct intel_crtc_state *crtc_state,
+		  const struct skl_wm_params *wp,
+		  unsigned int latency)
 {
-	struct intel_display *display = to_intel_display(crtc_state);
 	uint_fixed_16_16_t method1, method2;
 	uint_fixed_16_16_t selected_result;
-	u32 blocks, lines, min_ddb_alloc = 0;
-
-	if (latency == 0 ||
-	    (use_minimal_wm0_only(crtc_state, plane) && level > 0)) {
-		/* reject it */
-		result->min_ddb_alloc = U16_MAX;
-		return;
-	}
 
 	method1 = skl_wm_method1(display, wp->plane_pixel_rate,
 				 wp->cpp, latency, wp->dbuf_block_size);
@@ -1845,9 +1834,32 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 			selected_result = method1;
 		}
 		break;
+	}
+
+	return selected_result;
+}
 
+static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
+				 struct intel_plane *plane,
+				 int level,
+				 unsigned int latency,
+				 const struct skl_wm_params *wp,
+				 const struct skl_wm_level *result_prev,
+				 struct skl_wm_level *result /* out */)
+{
+	struct intel_display *display = to_intel_display(crtc_state);
+	uint_fixed_16_16_t selected_result;
+	u32 blocks, lines, min_ddb_alloc = 0;
+
+	if (latency == 0 ||
+	    (use_minimal_wm0_only(crtc_state, plane) && level > 0)) {
+		/* reject it */
+		result->min_ddb_alloc = U16_MAX;
+		return;
 	}
 
+	selected_result = skl_wm_run_method(display, crtc_state, wp, latency);
+
 	blocks = fixed16_to_u32_round_up(selected_result);
 	if (DISPLAY_VER(display) < 30)
 		blocks++;
-- 
2.51.0

[PATCH v3 7/7] drm/i915/wm: reduce size of y_min_scanlines to u8

[Luca Coelho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1162 bytes --]

We don't need more than an u8 to represent y_min_scanlines in the
watermark code, so we can convert it from u32 to u8 and use it to fill
one hole in the structure, saving 4 bytes.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index fc771f476bbe..c125e46e2530 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -64,16 +64,16 @@ enum wm_tiling_mode {
 struct skl_wm_params {
 	enum wm_tiling_mode tiling;
 	u32 width;
-	u8 cpp;
-	bool rc_surface;
-	bool is_planar;
 	u32 plane_pixel_rate;
-	u32 y_min_scanlines;
 	u32 plane_bytes_per_line;
 	uint_fixed_16_16_t plane_blocks_per_line;
 	uint_fixed_16_16_t y_tile_minimum;
 	u32 linetime_us;
 	u32 dbuf_block_size;
+	u8 cpp;
+	u8 y_min_scanlines;
+	bool rc_surface;
+	bool is_planar;
 };
 
 u8 intel_enabled_dbuf_slices_mask(struct intel_display *display)
-- 
2.51.0

Re: [PATCH v3 1/7] drm/i915/wm: clarify watermark ops with comments

[Ville Syrjälä]

On Thu, Oct 09, 2025 at 10:54:32AM +0300, Luca Coelho wrote:
> Some of the ops in struct intel_wm_funcs are used only for legacy
> watermark management, while others are only for SKL+ or both.  Clarify
> that in the struct definition.

We have (roughly) three vintages of wm stuff right now.

pre-g4x without proper atomic watermark code currently
(though I do have it in a branch somewhere...):
- .compute_watermarks() 
   doesn't really do what it says on the tin here, but I just
   needed a place to hide the ugliness from higher level code
- .update_wm()

g4x/vlv/chv/ilk+ (hw with single buffered wm registers)
 .compute_watermarks()
 .initial_watermarks()
 .optimize_watermarks()
 .atomic_update_watermarks()
 .get_hw_state()
 .sanitize()

skl+ (hw with double buffered wm registers)
 .compute_global_watermarks()
 .get_hw_state()
 .sanitize()

Most of the differences between the three are more
accidental than intentional, and should be unified.

I think if we managed to clean this up properly then
we would be left with this:
 .compute()
 .sanitize()
 .get_hw_state()
 .initial_watermarks() (pre-skl only)
 .optimize_watermarks() (pre-skl only)
 .atomic_update_watermarks() (pre-skl only)

Getting there would be mostly a matter of figuring out the right
order to do things in intel_atomic_check().

> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_core.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index df4da52cbdb3..7144b61fb1ff 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -78,7 +78,7 @@ struct intel_display_funcs {
>  
>  /* functions used for watermark calcs for display. */
>  struct intel_wm_funcs {
> -	/* update_wm is for legacy wm management */
> +	/* these are only for legacy wm management */
>  	void (*update_wm)(struct intel_display *display);
>  	int (*compute_watermarks)(struct intel_atomic_state *state,
>  				  struct intel_crtc *crtc);
> @@ -88,8 +88,12 @@ struct intel_wm_funcs {
>  					 struct intel_crtc *crtc);
>  	void (*optimize_watermarks)(struct intel_atomic_state *state,
>  				    struct intel_crtc *crtc);
> +
> +	/* these are for SKL+ wm management */
>  	int (*compute_global_watermarks)(struct intel_atomic_state *state);
>  	void (*get_hw_state)(struct intel_display *display);
> +
> +	/* this is used by both legacy and SKL+ */
>  	void (*sanitize)(struct intel_display *display);
>  };
>  
> -- 
> 2.51.0

-- 
Ville Syrjälä
Intel