Re: [PATCH v2 02/10] drm/i915/alpm: alpm_init() for DP2.1

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

On Tue, 04 Nov 2025, Animesh Manna <animesh.manna@intel.com> wrote:
> Initialize alpm for DP2.1 and separate out alpm mutex-init
> from alpm-init.
>
> v1: Initial version.
> v2: Separate out mutex-init. [Jani]

Under no circumstances should you have two places and code paths to
initialize a mutex.

I think you need two or three functions, one for initializing the mutex
(and potentially other similar stuff), one for dynamic initialization
for eDP and one for DP. Don't sprinkle the display version checks all
over the place.

BR,
Jani.

>
> Cc: Jouni Högander <jouni.hogander@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c |  1 -
>  drivers/gpu/drm/i915/display/intel_dp.c   | 11 ++++++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 6372f533f65b..639941e332f3 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -49,7 +49,6 @@ void intel_alpm_init(struct intel_dp *intel_dp)
>  		return;
>  
>  	intel_dp->alpm_dpcd = dpcd;
> -	mutex_init(&intel_dp->alpm.lock);
>  }
>  
>  static int get_silence_period_symbols(const struct intel_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0ec82fcbcf48..f995a98df2c3 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6074,8 +6074,12 @@ intel_dp_detect(struct drm_connector *_connector,
>  	if (ret == 1)
>  		connector->base.epoch_counter++;
>  
> -	if (!intel_dp_is_edp(intel_dp))
> +	if (!intel_dp_is_edp(intel_dp)) {
> +		if (DISPLAY_VER(display) >= 35)
> +			intel_alpm_init(intel_dp);
> +
>  		intel_psr_init_dpcd(intel_dp);
> +	}
>  
>  	intel_dp_detect_dsc_caps(intel_dp, connector);
>  
> @@ -6717,6 +6721,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	intel_hpd_enable_detection(encoder);
>  
>  	intel_alpm_init(intel_dp);
> +	if (intel_dp->alpm_dpcd)
> +		mutex_init(&intel_dp->alpm.lock);
>  
>  	/* Cache DPCD and EDID for edp. */
>  	has_dpcd = intel_edp_init_dpcd(intel_dp, connector);
> @@ -6932,6 +6938,9 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  
>  	intel_psr_init(intel_dp);
>  
> +	if (DISPLAY_VER(display) >= 35)
> +		mutex_init(&intel_dp->alpm.lock);
> +
>  	return true;
>  
>  fail:

-- 
Jani Nikula, Intel