Re: [PATCH v7 16/20] drm/i915/alpm: Replace is_edp() with alpm_is_possible()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915/alpm: uninitialized mutex lock in intel_alpm_lobf_disable()
--

commit 3691f816d97c5d3f84209f1900c4bc02406c352b
Author: Animesh Manna <animesh.manna@intel.com>

drm/i915/alpm: Replace is_edp() with alpm_is_possible()

This commit introduces intel_alpm_is_possible() to check ALPM capability
from DPCD and LTTPR info instead of checking intel_dp_is_edp() in
intel_alpm_lobf_compute_config(). It also removes the intel_dp_is_edp()
checks from intel_alpm_lobf_enable() and intel_alpm_lobf_disable().

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 4e761022485d7..205f1ae10d431 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c

[ ... ]

> @@ -829,9 +848,6 @@ void intel_alpm_lobf_disable(const struct intel_crtc_state *new_crtc_state)
>  
>  		intel_dp = enc_to_intel_dp(encoder);
>  
> -		if (!intel_dp_is_edp(intel_dp))
> -			continue;
> -
>  		mutex_lock(&intel_dp->alpm.lock);
>  		intel_de_write(display, ALPM_CTL(display, cpu_transcoder), 0);
>  		drm_dbg_kms(display->drm, "Link off between frames (LOBF) disabled\n");

[Severity: High]
Can removing the intel_dp_is_edp() check in intel_alpm_lobf_disable() cause
mutex_lock(&intel_dp->alpm.lock) to be called on an uninitialized mutex?

In drivers/gpu/drm/i915/display/intel_alpm.c, intel_alpm_init() only
initializes alpm.lock when intel_alpm_source_supported(intel_dp) is true:

void intel_alpm_init(struct intel_dp *intel_dp)
{
	if (!intel_alpm_source_supported(intel_dp))
		return;

	mutex_init(&intel_dp->alpm.lock);
}

On platforms with DISPLAY_VER between 20 and 34, intel_alpm_source_supported()
is only true for eDP encoders, so non-eDP DP encoders leave alpm.lock
uninitialized.

In drivers/gpu/drm/i915/display/intel_display.c, intel_pre_plane_update()
checks whether LOBF is being disabled via intel_crtc_lobf_disabling(), which
inspects old_crtc_state->has_lobf, but then passes new_crtc_state to
intel_alpm_lobf_disable():

	if (intel_crtc_lobf_disabling(old_crtc_state, new_crtc_state))
		intel_alpm_lobf_disable(new_crtc_state);

If a CRTC with LOBF active on an eDP encoder (old_crtc_state->has_lobf is
true) is switched in an atomic modeset to a non-eDP DP encoder
(new_crtc_state->has_lobf is false), intel_alpm_lobf_disable() iterates over
new_crtc_state->uapi.encoder_mask and calls mutex_lock(&intel_dp->alpm.lock)
on the non-eDP encoder whose mutex was never initialized.

Additionally, because intel_alpm_lobf_disable() inspects new_crtc_state
rather than old_crtc_state, disabling a CRTC by detaching its connector
(leaving new_crtc_state->uapi.encoder_mask empty) skips the loop and leaves
ALPM_CTL enabled, or on DISPLAY_VER >= 35 locks the new encoder's alpm.lock
instead of the old encoder's.

Should intel_pre_plane_update() pass old_crtc_state to
intel_alpm_lobf_disable(const struct intel_crtc_state *old_crtc_state) so it
operates on the encoder and transcoder where LOBF was previously enabled?

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