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

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Mon, 2025-09-08 at 10:35 +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.
> 
> 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(-)
> 

Hi Luca,

I am not sure if these comments by itself help in any understanding of the wm handling code better -
rather than browsing through the code! IMO, probably you need to have bit bigger explanation of this
"struct intel_wm_funcs" and the split of legacy/SKL+ usage - before these comments! 

BR
Vinod

> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 8c226406c5cd..938971591470 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);
>  };
>