Re: [PATCH 5/9] drm/i915/wm: Nuke wm->uv_wm[]

Jani Nikula
2026-03-19T14:06:35Z

On Thu, 19 Mar 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We currently keep around the full watermarks for the UV plane
> on pre-icl, even though the hardware doesn't need most of this
> information. The only thing we need to keep is the min_ddb_alloc
> for the UV plane. Move that into the main wm->wm[].min_ddb_alloc_uv
> alongside the other min_ddb_alloc (used for Y/RGB).
>
> This makes our state tracking match the hardware more closely,
> and avoids having to justify everwhere why uv_wm[] is being
> ignored.

Somehow found this change difficult to follow, but didn't spot anything
obviously wrong either.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> .../drm/i915/display/intel_display_types.h | 2 +-
> drivers/gpu/drm/i915/display/skl_watermark.c | 43 ++++++++-----------
> 2 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 93b8b2f91484..e2496db1642a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -835,6 +835,7 @@ struct intel_pipe_wm {
>
> struct skl_wm_level {
> u16 min_ddb_alloc;
> + u16 min_ddb_alloc_uv; /* for pre-icl */
> u16 blocks;
> u8 lines;
> bool enable;
> @@ -845,7 +846,6 @@ struct skl_wm_level {
>
> struct skl_plane_wm {
> struct skl_wm_level wm[8];
> - struct skl_wm_level uv_wm[8];
> struct skl_wm_level trans_wm;
> struct {
> struct skl_wm_level wm0;
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 7c4c42dde991..8b1b371fbfab 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -1356,14 +1356,13 @@ skl_check_wm_level(struct skl_wm_level *wm, const struct skl_ddb_entry *ddb)
> }
>
> static void
> -skl_check_wm_level_nv12(struct skl_wm_level *wm, struct skl_wm_level *uv_wm,
> - const struct skl_ddb_entry *ddb_y, const struct skl_ddb_entry *ddb)
> +skl_check_wm_level_nv12(struct skl_wm_level *wm,
> + const struct skl_ddb_entry *ddb_y,
> + const struct skl_ddb_entry *ddb)
> {
> if (wm->min_ddb_alloc > skl_ddb_entry_size(ddb_y) ||
> - uv_wm->min_ddb_alloc > skl_ddb_entry_size(ddb)) {
> + wm->min_ddb_alloc_uv > skl_ddb_entry_size(ddb))
> memset(wm, 0, sizeof(*wm));
> - memset(uv_wm, 0, sizeof(*uv_wm));
> - }
> }
>
> static bool skl_need_wm_copy_wa(struct intel_display *display, int level,
> @@ -1427,11 +1426,10 @@ static void
> skl_allocate_plane_ddb_nv12(struct skl_plane_ddb_iter *iter,
> const struct skl_wm_level *wm,
> struct skl_ddb_entry *ddb_y, u64 data_rate_y,
> - const struct skl_wm_level *uv_wm,
> struct skl_ddb_entry *ddb, u64 data_rate)
> {
> _skl_allocate_plane_ddb(iter, wm->min_ddb_alloc, ddb_y, data_rate_y);
> - _skl_allocate_plane_ddb(iter, uv_wm->min_ddb_alloc, ddb, data_rate);
> + _skl_allocate_plane_ddb(iter, wm->min_ddb_alloc_uv, ddb, data_rate);
> }
>
> static int
> @@ -1499,7 +1497,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> }
>
> blocks += wm->wm[level].min_ddb_alloc;
> - blocks += wm->uv_wm[level].min_ddb_alloc;
> + blocks += wm->wm[level].min_ddb_alloc_uv;
> }
>
> if (blocks <= iter.size) {
> @@ -1543,7 +1541,6 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> crtc_state->nv12_planes & BIT(plane_id))
> skl_allocate_plane_ddb_nv12(&iter, &wm->wm[level],
> ddb_y, crtc_state->rel_data_rate_y[plane_id],
> - &wm->uv_wm[level],
> ddb, crtc_state->rel_data_rate[plane_id]);
> else
> skl_allocate_plane_ddb(&iter, &wm->wm[level],
> @@ -1573,9 +1570,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
>
> if (DISPLAY_VER(display) < 11 &&
> crtc_state->nv12_planes & BIT(plane_id))
> - skl_check_wm_level_nv12(&wm->wm[level],
> - &wm->uv_wm[level],
> - ddb_y, ddb);
> + skl_check_wm_level_nv12(&wm->wm[level], ddb_y, ddb);
> else
> skl_check_wm_level(&wm->wm[level], ddb);
>
> @@ -2084,9 +2079,11 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state,
> struct intel_plane *plane)
> {
> + struct intel_display *display = to_intel_display(crtc_state);
> struct skl_plane_wm *wm = &crtc_state->wm.skl.raw.planes[plane->id];
> + struct skl_wm_level uv_wm[ARRAY_SIZE(wm->wm)] = {};
> struct skl_wm_params wm_params;
> - int ret;
> + int ret, level;
>
> /* uv plane watermarks must also be validated for NV12/Planar */
> ret = skl_compute_plane_wm_params(crtc_state, plane_state,
> @@ -2094,7 +2091,14 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> if (ret)
> return ret;
>
> - skl_compute_wm_levels(crtc_state, plane, &wm_params, wm->uv_wm);
> + skl_compute_wm_levels(crtc_state, plane, &wm_params, uv_wm);
> +
> + /*
> + * Only keep the min_ddb_alloc for UV as
> + * the hardware needs nothing else.
> + */
> + for (level = 0; level < display->wm.num_levels; level++)
> + wm->wm[level].min_ddb_alloc_uv = uv_wm[level].min_ddb_alloc;
>
> return 0;
> }
> @@ -2317,7 +2321,6 @@ static int skl_wm_check_vblank(struct intel_crtc_state *crtc_state)
> * thing as bad via min_ddb_alloc=U16_MAX?
> */
> wm->wm[level].enable = false;
> - wm->uv_wm[level].enable = false;
> }
> }
>
> @@ -2388,11 +2391,6 @@ static bool skl_plane_wm_equals(struct intel_display *display,
> int level;
>
> for (level = 0; level < display->wm.num_levels; level++) {
> - /*
> - * We don't check uv_wm as the hardware doesn't actually
> - * use it. It only gets used for calculating the required
> - * ddb allocation.
> - */
> if (!skl_wm_level_equals(&wm1->wm[level], &wm2->wm[level]))
> return false;
> }
> @@ -2753,11 +2751,6 @@ static bool skl_plane_selected_wm_equals(struct intel_plane *plane,
> int level;
>
> for (level = 0; level < display->wm.num_levels; level++) {
> - /*
> - * We don't check uv_wm as the hardware doesn't actually
> - * use it. It only gets used for calculating the required
> - * ddb allocation.
> - */
> if (!skl_wm_level_equals(skl_plane_wm_level(old_pipe_wm, plane->id, level),
> skl_plane_wm_level(new_pipe_wm, plane->id, level)))
> return false;

--
Jani Nikula, Intel