Re: [PATCH v3 1/2] drm/i915/alpm: Make intel_alpm_enable_sink available for PSR

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Fri, 2025-05-09 at 13:43 +0300, Jani Nikula wrote:
> On Thu, 08 May 2025, Jouni Högander <jouni.hogander@intel.com> wrote:
> > We want to enable sink ALPM from PSR code. Make
> > intel_alpm_enable_sink
> > available for PSR.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_alpm.c | 11 +++++++++--
> >  drivers/gpu/drm/i915/display/intel_alpm.h |  2 ++
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > index 1bf08b80c23f..9442483058d2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > @@ -426,8 +426,15 @@ void intel_alpm_pre_plane_update(struct
> > intel_atomic_state *state,
> >  	}
> >  }
> >  
> > -static void intel_alpm_enable_sink(struct intel_dp *intel_dp,
> > -				   const struct intel_crtc_state
> > *crtc_state)
> > +/**
> > + * intel_alpm_enable_sink - Enable ALPM on sink
> > + * @intel_dp: Intel DP struct
> > + * @crtc_state: Intel CRTC struct
> > + *
> > + * This function is enabling DPCD on sink based on information
> > from crtc_state.
> > + */
> 
> Perhaps surprisingly I'm not a big fan of kernel-doc for all the
> simple
> little functions like this.
> 
> The function name already says what it does, the parameters are
> self-explanatory.
> 
> The kernel-doc is not even pulled into the Sphinx build... and why
> would
> it be, nobody reading the kernel documentation would be interested in
> this small detail.
> 
> I might add a small regular comment about writing DPCD as needed...
> but
> might not.
> 
> The documentation comments we absolutely need more are the high level
> descriptions at the top of files, which is a glaring omission in
> intel_alpm.c (and many other places). What is ALPM, what does it do,
> how, why, etc.
> 
> See commit b031ef5ea8b1 ("drm/i915/mst: add beginnings of DP MST
> documentation") or intel_dp_mst.c for the kind of comments I'd love
> to
> have more.
> 
> I'm not insisting you do that now, but perhaps consider dropping the
> kernel-doc.

Ok, I will drop it. For some reason I had an idea that these
"interface" functions should have docs. I've been just blindly
following that idea. 

BR,

Jouni Högander

> 
> 
> BR,
> Jani.
> 
> 
> > +void intel_alpm_enable_sink(struct intel_dp *intel_dp,
> > +			    const struct intel_crtc_state
> > *crtc_state)
> >  {
> >  	u8 val;
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> > b/drivers/gpu/drm/i915/display/intel_alpm.h
> > index d7126d65b60f..c9fe21e3e72c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> > @@ -23,6 +23,8 @@ void intel_alpm_lobf_compute_config(struct
> > intel_dp *intel_dp,
> >  				    struct drm_connector_state
> > *conn_state);
> >  void intel_alpm_configure(struct intel_dp *intel_dp,
> >  			  const struct intel_crtc_state
> > *crtc_state);
> > +void intel_alpm_enable_sink(struct intel_dp *intel_dp,
> > +			    const struct intel_crtc_state
> > *crtc_state);
> >  void intel_alpm_pre_plane_update(struct intel_atomic_state *state,
> >  				 struct intel_crtc *crtc);
> >  void intel_alpm_post_plane_update(struct intel_atomic_state
> > *state,
>