Re: [PATCH v7 04/20] drm/i915/alpm: alpm_init() for DP2.1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915/alpm: wrong transcoder ALPM_CTL access in intel_alpm_disable()
--

commit 16555494a05a6a62d73ca6ad3e0999793ea5b8b4
Author: Animesh Manna <animesh.manna@intel.com>

drm/i915/alpm: alpm_init() for DP2.1

This commit initializes ALPM for DP 2.1 on display version 35 and newer,
separating the ALPM mutex initialization from DPCD capability initialization
and checking source support instead of sink capabilities in
intel_alpm_disable().

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index cb1d68fef1986..b6307862b7560 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
[ ... ]
> @@ -750,7 +763,11 @@ void intel_alpm_disable(struct intel_dp *intel_dp)
>  	struct intel_display *display = to_intel_display(intel_dp);
>  	enum transcoder cpu_transcoder = intel_dp->alpm.transcoder;
>  
> -	if (DISPLAY_VER(display) < 20 || !intel_dp->alpm_dpcd)
> +	/*
> +	 * Don't check sink ALPM caps here, they are reset on disconnect
> +	 * before the CRTC is disabled.
> +	 */
> +	if (DISPLAY_VER(display) < 20 || !intel_alpm_source_supported(intel_dp))
>  		return;

[Severity: High]
Can replacing the intel_dp->alpm_dpcd check with
intel_alpm_source_supported(intel_dp) here cause intel_alpm_disable() to
modify ALPM_CTL on the wrong transcoder when ALPM was never configured on
intel_dp?

Because struct intel_digital_port is zero-allocated,
intel_dp->alpm.transcoder starts at 0 (TRANSCODER_A, rather than
INVALID_TRANSCODER), and intel_alpm_init() does not initialize it. Across
the driver, intel_dp->alpm.transcoder is only set in
intel_alpm_configure():

drivers/gpu/drm/i915/display/intel_alpm.c:intel_alpm_configure() {
	lnl_alpm_configure(intel_dp, crtc_state);
	intel_dp->alpm.transcoder = crtc_state->cpu_transcoder;
}

which is only called from intel_psr_enable_source() when
crtc_state->has_psr is true or from intel_alpm_lobf_enable() when
new_crtc_state->has_lobf is true.

However, intel_ddi_disable_dp() unconditionally calls
intel_alpm_disable(intel_dp) whenever any DP or eDP encoder is disabled:

drivers/gpu/drm/i915/display/intel_ddi.c:intel_ddi_disable_dp() {
	...
	intel_panel_unprepare(old_conn_state);
	intel_psr_disable(intel_dp, old_crtc_state);
	intel_alpm_disable(intel_dp);
	...
}

And intel_dp_short_pulse() also calls intel_alpm_disable(intel_dp) if
reading DP_RECEIVER_ALPM_STATUS fails or reports an error:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_short_pulse() {
	...
	if (intel_alpm_get_error(intel_dp)) {
		intel_alpm_disable(intel_dp);
		intel_dp->alpm.sink_alpm_error = true;
	}
	...
}

Previously, !intel_dp->alpm_dpcd caused intel_alpm_disable() to return
early for external DP connectors and eDP panels without ALPM support. With
this change, if an external DP connector on DISPLAY_VER(display) >= 35 (or
an eDP connector on DISPLAY_VER(display) >= 20) is enabled on
TRANSCODER_B/C/D without PSR, Panel Replay, or LOBF, intel_alpm_configure()
is never called and intel_dp->alpm.transcoder remains TRANSCODER_A (or a
stale transcoder from a previous modeset).

When that connector is later disabled or hits the short HPD pulse error
path, intel_alpm_disable() will lock that connector's intel_dp->alpm.lock
and clear ALPM_CTL_ALPM_ENABLE | ALPM_CTL_LOBF_ENABLE in
ALPM_CTL(display, TRANSCODER_A). If TRANSCODER_A is active on another
connector (such as eDP on Pipe A), this races with Pipe A and disables its
active ALPM/LOBF state; if TRANSCODER_A's power well is off, it triggers an
unclaimed MMIO register access.

Should intel_dp->alpm.transcoder be initialized to INVALID_TRANSCODER in
intel_alpm_init(), reset to INVALID_TRANSCODER in intel_alpm_disable(), and
checked for INVALID_TRANSCODER before accessing ALPM_CTL()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026054001.799894-1-animesh.manna@intel.com?part=4