On Mon, 2025-09-08 at 09:43 -0300, Gustavo Sousa wrote:
> 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.coe...@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.
This was mainly to make some sense out of the different tiling modes
that affect watermark calculations. If you check newer bspecs, there's
no mention of Y-tiling anymore, only 4-tiling, which superseeded y-
tiling. It was quite confusing to figure this all out, not clear from
the code at all. Additionally, in the watermark code, there has been
mention of linear, which sort of included x-tiling as well.
With the enumeration, we not only remove the invalid combinations (e.g.
x-tiled and y-tiled both being set to true), but also make it clear
that these are all different values for the "tiling" attribute for
watermark calculations.
> On the other hand, we do reduce the number of members in struct
> skl_wm_params.
>
> So, I have mixed feelings about this change.
It's not only the reduction of members, but also clarity in the code,
IMHO and avoidance of impossible combinations. With one of the next
patches I convert some of this stuff into a switch-case, so it becomes
clearer to see how each one is handled.
--
Cheers,
Luca.
