On Fri, 2025-09-19 at 21:00 +0300, Ville Syrjälä wrote: > 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 <[email protected]> > > --- > > 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.
Okay, I hear you. I was not happy at all with "run method" either, but couldn't find anything better. I tried to isolate the method selection and execution, but "skl_wm_select_and_run_method()" would be too long. In some of my later draft patches on top of this, I'm moving platform dependent parts into their own functions. The step in this patch is "carve out for further processing". Method 1 is rarely needed nowadays, so skipping the calculation could improve performance in critical paths. If you think this doesn't have any performance impact or other issue, it would be nice to merge it as is, so I can continue carving, trimming and sorting this whole thing out. What do you think? -- Cheers, Luca.
