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

[Luca Coelho <luca@coelho.fi>]

On Tue, 2025-09-16 at 08:52 +0000, Govindapillai, Vinod wrote:
> 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! 

Thanks, Vinod!

This tiny comment doesn't add much, for sure.  But it did help me
quickly figure out which functions are not relevant to newer hardware.

It's a tiny comment that, IMHO, doesn't harm, and if it were there to
start with, my initial attempts at understanding this code would have
been slightly easier.

If you insist, I can drop this patch, but otherwise I think it's useful
enough to be merged as is.  With the other changes I'm planning to make
in this code, hopefully this whole thing will be easier to understand.

--
Cheers,
Luca.



> 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);
> >  };
> >