Re: [PATCH v3 1/7] drm/i915/wm: clarify watermark ops with comments

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 09, 2025 at 10:54:32AM +0300, Luca Coelho wrote:
> Some of the ops in struct intel_wm_funcs are used only for legacy
> watermark management, while others are only for SKL+ or both.  Clarify
> that in the struct definition.

We have (roughly) three vintages of wm stuff right now.

pre-g4x without proper atomic watermark code currently
(though I do have it in a branch somewhere...):
- .compute_watermarks() 
   doesn't really do what it says on the tin here, but I just
   needed a place to hide the ugliness from higher level code
- .update_wm()

g4x/vlv/chv/ilk+ (hw with single buffered wm registers)
 .compute_watermarks()
 .initial_watermarks()
 .optimize_watermarks()
 .atomic_update_watermarks()
 .get_hw_state()
 .sanitize()

skl+ (hw with double buffered wm registers)
 .compute_global_watermarks()
 .get_hw_state()
 .sanitize()

Most of the differences between the three are more
accidental than intentional, and should be unified.

I think if we managed to clean this up properly then
we would be left with this:
 .compute()
 .sanitize()
 .get_hw_state()
 .initial_watermarks() (pre-skl only)
 .optimize_watermarks() (pre-skl only)
 .atomic_update_watermarks() (pre-skl only)

Getting there would be mostly a matter of figuring out the right
order to do things in intel_atomic_check().

> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_core.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index df4da52cbdb3..7144b61fb1ff 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -78,7 +78,7 @@ struct intel_display_funcs {
>  
>  /* functions used for watermark calcs for display. */
>  struct intel_wm_funcs {
> -	/* update_wm is for legacy wm management */
> +	/* these are only for legacy wm management */
>  	void (*update_wm)(struct intel_display *display);
>  	int (*compute_watermarks)(struct intel_atomic_state *state,
>  				  struct intel_crtc *crtc);
> @@ -88,8 +88,12 @@ struct intel_wm_funcs {
>  					 struct intel_crtc *crtc);
>  	void (*optimize_watermarks)(struct intel_atomic_state *state,
>  				    struct intel_crtc *crtc);
> +
> +	/* these are for SKL+ wm management */
>  	int (*compute_global_watermarks)(struct intel_atomic_state *state);
>  	void (*get_hw_state)(struct intel_display *display);
> +
> +	/* this is used by both legacy and SKL+ */
>  	void (*sanitize)(struct intel_display *display);
>  };
>  
> -- 
> 2.51.0

-- 
Ville Syrjälä
Intel