On Mon, Sep 08, 2025 at 10:35:35AM +0300, Luca Coelho wrote:
> Isolate the code that handles method selection and calculation, so
> skl_compute_plane_wm() doesn't get too long.
>
> Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
> ---
> drivers/gpu/drm/i915/display/skl_watermark.c | 51 ++++++++++++--------
> 1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 21f8d52ec1d2..33853a18ee9c 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -1806,25 +1806,14 @@ static bool xe3_auto_min_alloc_capable(struct
> intel_plane *plane, int level)
> return DISPLAY_VER(display) >= 30 && level == 0 && plane->id !=
> PLANE_CURSOR;
> }
>
> -static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> - struct intel_plane *plane,
> - int level,
> - unsigned int latency,
> - const struct skl_wm_params *wp,
> - const struct skl_wm_level *result_prev,
> - struct skl_wm_level *result /* out */)
> +static uint_fixed_16_16_t
> +skl_wm_run_method(struct intel_display *display,
I was confused what a "run method" is, but I guess "run" is supposed
to be a verb here.
However this thing does a lot more than "run a method", so I don't
really like this.
I susepct it would make more sense to carve up skl_compute_plane_wm()
into several smaller (possibly platform dependent) pieces. Eg.
the result selection part seems like one thing that could extracted
into a small function. The min_ddb_alloc calculation would be another
clear piece that can be extracted.
> + const struct intel_crtc_state *crtc_state,
> + const struct skl_wm_params *wp,
> + unsigned int latency)
> {
> - struct intel_display *display = to_intel_display(crtc_state);
> uint_fixed_16_16_t method1, method2;
> uint_fixed_16_16_t selected_result;
> - u32 blocks, lines, min_ddb_alloc = 0;
> -
> - if (latency == 0 ||
> - (use_minimal_wm0_only(crtc_state, plane) && level > 0)) {
> - /* reject it */
> - result->min_ddb_alloc = U16_MAX;
> - return;
> - }
>
> method1 = skl_wm_method1(display, wp->plane_pixel_rate,
> wp->cpp, latency, wp->dbuf_block_size);
> @@ -1837,7 +1826,9 @@ static void skl_compute_plane_wm(const struct
> intel_crtc_state *crtc_state,
> case WM_TILING_Y_FAMILY:
> selected_result = max_fixed16(method2, wp->y_tile_minimum);
> break;
> -
> + default:
> + MISSING_CASE(wp->tiling);
> + fallthrough;
> case WM_TILING_LINEAR:
> case WM_TILING_X_TILED:
> /*
> @@ -1862,12 +1853,32 @@ static void skl_compute_plane_wm(const struct
> intel_crtc_state *crtc_state,
> selected_result = method1;
> }
> break;
> + }
>
> - default:
> - drm_err(display->drm, "Invalid tiling mode\n", wp->tiling);
> - break;
> + return selected_result;
> +}
> +
> +static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> + struct intel_plane *plane,
> + int level,
> + unsigned int latency,
> + const struct skl_wm_params *wp,
> + const struct skl_wm_level *result_prev,
> + struct skl_wm_level *result /* out */)
> +{
> + struct intel_display *display = to_intel_display(crtc_state);
> + uint_fixed_16_16_t selected_result;
> + u32 blocks, lines, min_ddb_alloc = 0;
> +
> + if (latency == 0 ||
> + (use_minimal_wm0_only(crtc_state, plane) && level > 0)) {
> + /* reject it */
> + result->min_ddb_alloc = U16_MAX;
> + return;
> }
>
> + selected_result = skl_wm_run_method(display, crtc_state, wp, latency);
> +
> blocks = fixed16_to_u32_round_up(selected_result);
> if (DISPLAY_VER(display) < 30)
> blocks++;
> --
> 2.50.1
--
Ville Syrjälä
Intel
