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

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Luca Coelho (2025-09-08 04:35:33-03:00)
>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.
>
>Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>---
> drivers/gpu/drm/i915/display/skl_watermark.c | 35 ++++++++++++++------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
>index 0ce3420a919e..dd4bed02c3c0 100644
>--- a/drivers/gpu/drm/i915/display/skl_watermark.c
>+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
>@@ -53,9 +53,16 @@ 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_TILED,        /* mostly like linear */
>+        WM_TILING_Y_FAMILY,        /* includes Y, Yf and 4 tiling */
>+};
>+
> /* Stores plane specific WM parameters */
> struct skl_wm_params {
>-        bool x_tiled, y_tiled;
>+        enum wm_tiling_mode tiling;
>         bool rc_surface;
>         bool is_planar;
>         u32 width;
>@@ -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;
>@@ -1674,9 +1682,14 @@ 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 == I915_FORMAT_MOD_X_TILED)
>+                wp->tiling = WM_TILING_X_TILED;
>+        else if (modifier != I915_FORMAT_MOD_X_TILED &&
>+                 intel_fb_is_tiled_modifier(modifier))
>+                wp->tiling = WM_TILING_Y_FAMILY;
>+        else
>+                wp->tiling = WM_TILING_LINEAR;
>+

Hm...  I feel like x_tiled and y_tiled, just like other members of
struct skl_wm_params, are more about represeting *properties* of the
framebuffer/plane that are relevant to the algorithm rather than the
tiling mode itself.  Invalid combinations of values would reflect a
problem outside of the watermark calculation.  So, I'm not sure we
really need an enumeration here.  If, in the future, we end up needing
to know a tiling-related property that could be common to different
tiling modes, the enumeration would not work for us.

On the other hand, we do reduce the number of members in struct
skl_wm_params.

So, I have mixed feelings about this change.

>         wp->rc_surface = intel_fb_is_ccs_modifier(modifier);
>         wp->is_planar = intel_format_info_is_yuv_semiplanar(format, modifier);
> 
>@@ -1716,7 +1729,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);
>@@ -1732,7 +1745,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);
>@@ -1820,7 +1833,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 /
>@@ -1870,7 +1883,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 {
>@@ -1889,7 +1902,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>         }
> 
>         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)
>@@ -2015,7 +2028,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.50.1
>