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

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

[Luca Coelho]

Hi,

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

Please review.  Changes in v2 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 v2 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 v2 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 v2 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 v2 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)

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

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 3a982458395e..dc00b5cd3ff7 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_TILED)
 		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 == WM_TILING_LINEAR)
+		wp->tiling = WM_TILING_LINEAR;
+	else if (modifier == I915_FORMAT_MOD_X_TILED)
+		wp->tiling = WM_TILING_X_TILED;
+	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_FAMILY) {
 		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_TILED || DISPLAY_VER(display) >= 10)
 			interm_pbpl++;
 
 		wp->plane_blocks_per_line = u32_to_fixed16(interm_pbpl);
@@ -1805,7 +1817,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 				 latency,
 				 wp->plane_blocks_per_line);
 
-	if (wp->y_tiled) {
+	if (wp->tiling == WM_TILING_Y_FAMILY) {
 		selected_result = max_fixed16(method2, wp->y_tile_minimum);
 	} else {
 		if ((wp->cpp * crtc_state->hw.pipe_mode.crtc_htotal /
@@ -1855,7 +1867,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_FAMILY) {
 				blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
 				lines += wp->y_min_scanlines;
 			} else {
@@ -1877,7 +1889,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_FAMILY) {
 			int extra_lines;
 
 			if (lines % wp->y_min_scanlines == 0)
@@ -2003,7 +2015,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_FAMILY) {
 		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

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

[Jani Nikula]

On Tue, 07 Oct 2025, Luca Coelho <luciano.coelho@intel.com> wrote:
> 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)
>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 38 +++++++++++++-------
>  1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 3a982458395e..dc00b5cd3ff7 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_TILED)
>  		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 == WM_TILING_LINEAR)

"Namespace" mistmatch between modifier and WM_TILING_LINEAR?

> +		wp->tiling = WM_TILING_LINEAR;
> +	else if (modifier == I915_FORMAT_MOD_X_TILED)
> +		wp->tiling = WM_TILING_X_TILED;
> +	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_FAMILY) {
>  		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_TILED || DISPLAY_VER(display) >= 10)
>  			interm_pbpl++;
>  
>  		wp->plane_blocks_per_line = u32_to_fixed16(interm_pbpl);
> @@ -1805,7 +1817,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  				 latency,
>  				 wp->plane_blocks_per_line);
>  
> -	if (wp->y_tiled) {
> +	if (wp->tiling == WM_TILING_Y_FAMILY) {
>  		selected_result = max_fixed16(method2, wp->y_tile_minimum);
>  	} else {
>  		if ((wp->cpp * crtc_state->hw.pipe_mode.crtc_htotal /
> @@ -1855,7 +1867,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_FAMILY) {
>  				blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
>  				lines += wp->y_min_scanlines;
>  			} else {
> @@ -1877,7 +1889,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_FAMILY) {
>  			int extra_lines;
>  
>  			if (lines % wp->y_min_scanlines == 0)
> @@ -2003,7 +2015,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_FAMILY) {
>  		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;

-- 
Jani Nikula, Intel

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

[Luca Coelho]

On Tue, 2025-10-07 at 11:07 +0300, Jani Nikula wrote:
> On Tue, 07 Oct 2025, Luca Coelho <luciano.coelho@intel.com> wrote:
> > 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)
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 38 +++++++++++++-------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 3a982458395e..dc00b5cd3ff7 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_TILED)
> >  		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 == WM_TILING_LINEAR)
> 
> "Namespace" mistmatch between modifier and WM_TILING_LINEAR?

Oops, sorry, b0rked rebase.  I'll fix and submit v3.

--
Cheers,
Luca.

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

[kernel test robot]

Hi Luca,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.17 next-20251009]
[cannot apply to drm-i915/for-linux-next-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luca-Coelho/drm-i915-wm-clarify-watermark-ops-with-comments/20251010-123245
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20251007075729.468669-5-luciano.coelho%40intel.com
patch subject: [PATCH v2 4/7] drm/i915/wm: convert x/y-tiling bools to an enum
config: arm-randconfig-002-20251010 (https://download.01.org/0day-ci/archive/20251010/202510101543.qQD4XNoc-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251010/202510101543.qQD4XNoc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510101543.qQD4XNoc-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/display/skl_watermark.c: In function 'skl_wm_latency':
>> drivers/gpu/drm/i915/display/skl_watermark.c:629:33: error: 'WM_TILING_X_TILED' undeclared (first use in this function)
     629 |             wp && wp->tiling == WM_TILING_X_TILED)
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/skl_watermark.c:629:33: note: each undeclared identifier is reported only once for each function it appears in
   drivers/gpu/drm/i915/display/skl_watermark.c: In function 'skl_compute_wm_params':
   drivers/gpu/drm/i915/display/skl_watermark.c:1673:30: error: 'WM_TILING_X_TILED' undeclared (first use in this function)
    1673 |                 wp->tiling = WM_TILING_X_TILED;
         |                              ^~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/display/skl_watermark.c:1716:27: error: 'WM_TILING_Y_FAMILY' undeclared (first use in this function); did you mean 'WM_TILING_Y_Yf_4'?
    1716 |         if (wp->tiling == WM_TILING_Y_FAMILY) {
         |                           ^~~~~~~~~~~~~~~~~~
         |                           WM_TILING_Y_Yf_4
   drivers/gpu/drm/i915/display/skl_watermark.c: In function 'skl_compute_plane_wm':
   drivers/gpu/drm/i915/display/skl_watermark.c:1820:27: error: 'WM_TILING_Y_FAMILY' undeclared (first use in this function); did you mean 'WM_TILING_Y_Yf_4'?
    1820 |         if (wp->tiling == WM_TILING_Y_FAMILY) {
         |                           ^~~~~~~~~~~~~~~~~~
         |                           WM_TILING_Y_Yf_4
   drivers/gpu/drm/i915/display/skl_watermark.c: In function 'skl_compute_transition_wm':
   drivers/gpu/drm/i915/display/skl_watermark.c:2018:27: error: 'WM_TILING_Y_FAMILY' undeclared (first use in this function); did you mean 'WM_TILING_Y_Yf_4'?
    2018 |         if (wp->tiling == WM_TILING_Y_FAMILY) {
         |                           ^~~~~~~~~~~~~~~~~~
         |                           WM_TILING_Y_Yf_4


vim +/WM_TILING_X_TILED +629 drivers/gpu/drm/i915/display/skl_watermark.c

   597	
   598	static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
   599					 int width, const struct drm_format_info *format,
   600					 u64 modifier, unsigned int rotation,
   601					 u32 plane_pixel_rate, struct skl_wm_params *wp,
   602					 int color_plane, unsigned int pan_x);
   603	
   604	static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
   605					 struct intel_plane *plane,
   606					 int level,
   607					 unsigned int latency,
   608					 const struct skl_wm_params *wp,
   609					 const struct skl_wm_level *result_prev,
   610					 struct skl_wm_level *result /* out */);
   611	
   612	static unsigned int skl_wm_latency(struct intel_display *display, int level,
   613					   const struct skl_wm_params *wp)
   614	{
   615		unsigned int latency = display->wm.skl_latency[level];
   616	
   617		if (latency == 0)
   618			return 0;
   619	
   620		/*
   621		 * WaIncreaseLatencyIPCEnabled: kbl,cfl
   622		 * Display WA #1141: kbl,cfl
   623		 */
   624		if ((display->platform.kabylake || display->platform.coffeelake ||
   625		     display->platform.cometlake) && skl_watermark_ipc_enabled(display))
   626			latency += 4;
   627	
   628		if (skl_needs_memory_bw_wa(display) &&
 > 629		    wp && wp->tiling == WM_TILING_X_TILED)
   630			latency += 15;
   631	
   632		return latency;
   633	}
   634	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index dc00b5cd3ff7..a9d1bc432f75 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -1817,12 +1817,23 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 				 latency,
 				 wp->plane_blocks_per_line);
 
-	if (wp->tiling == WM_TILING_Y_FAMILY) {
+	switch (wp->tiling) {
+	case WM_TILING_Y_FAMILY:
 		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_TILED:
+		/*
+		 * 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)) {
+		    (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
 			selected_result = method2;
 		} else if (latency >= wp->linetime_us) {
 			if (DISPLAY_VER(display) == 9)
@@ -1830,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 v2 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 a9d1bc432f75..4f9fd8c6ccb4 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 v2 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: 1161 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 Syrä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 4f9fd8c6ccb4..4149b891615e 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