Re: [PATCH 3/9] drm/i915/wm: s/skl_check_nv12_wm_level()/skl_check_wm_level_nv12()/

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 19 Mar 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Rename skl_check_nv12_wm_level() to skl_check_wm_level_nv12(). There
> will be a sort of DDB counterparts to skl_check_wm_level*(), and
> putting the "nv12" part to the end will allow consistent naming.

Overall I dislike "check" in function names. What does it check? What
does it mean? Should it have a return value? Or is it like an assert?

In skl_watermark.c, there are three types of check functions, all
behaving differently. check_mbus_joined() is really just
is_mbus_joined(). I don't know what skl_check_wm_level() or
skl_check_nv12_wm_level() should be called, because they conditionally
clear the watermarks. And "checking" doesn't sound like something that
should modify its arguments. Then you have skl_wm_check_vblank(), which
modifies its arguments and returns an error code, and I really don't
know what about it is "checking".

/rant

Anyway, for the patch at hand,

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


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 1664b84d0387..24978f312fec 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -1356,7 +1356,7 @@ skl_check_wm_level(struct skl_wm_level *wm, const struct skl_ddb_entry *ddb)
>  }
>  
>  static void
> -skl_check_nv12_wm_level(struct skl_wm_level *wm, struct skl_wm_level *uv_wm,
> +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)
>  {
>  	if (wm->min_ddb_alloc > skl_ddb_entry_size(ddb_y) ||
> @@ -1555,7 +1555,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_nv12_wm_level(&wm->wm[level],
> +				skl_check_wm_level_nv12(&wm->wm[level],
>  							&wm->uv_wm[level],
>  							ddb_y, ddb);
>  			else

-- 
Jani Nikula, Intel