On Thu, 19 Mar 2026, Ville Syrjala <[email protected]> wrote: > From: Ville Syrjälä <[email protected]> > > 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 <[email protected]> > > Signed-off-by: Ville Syrjälä <[email protected]> > --- > 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
