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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.


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,

-- 
Jani Nikula, Intel