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.coe...@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);
> > };
> >
