Re: [PATCHv5] drm/i915/dp: On DPCD init wake the DPRx for eDP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing bitmask when checking the DP_SET_POWER state in intel_edp_wake_sink, leading to skipped wake sequences for valid sleep states.
--

commit 5baa07d75bcb19eae05989d83bea3bd4d212a13a
Author: Arun R Murthy <arun.r.murthy@intel.com>

drm/i915/dp: On DPCD init wake the DPRx for eDP

This commit attempts to wake the eDP sink before reading DPCD capabilities
to avoid AUX_CH failures when the sink is in an unknown or sleep state.

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4391
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16654

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0cd5e6b5034cf..07cc39a6f2e37 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4779,6 +4779,33 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)

[ ... ]

> +static void intel_edp_wake_sink(struct intel_dp *intel_dp)
> +{
> +	u8 value = 0;
> +	int ret;
> +
> +	/*
> +	 * Read the current sink power state. drm_dp_dpcd_read_byte() already
> +	 * retries the AUX transaction internally, so a single read suffices.
> +	 * First commercial eDP panels are Ver1.0 or 1.1, on which DPCD
> +	 * DP_SET_POWER is supported.
> +	 */
> +	ret = drm_dp_dpcd_read_byte(&intel_dp->aux, DP_SET_POWER, &value);
> +
> +	/*
> +	 * If the AUX read failed the sink may be asleep and not responding,
> +	 * or it read back D3; in either case wake it up to D3_AUX_ON.
> +	 */
> +	if (ret < 0 || value == DP_SET_POWER_D3) {
                       ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this strict equality check skip the wake sequence for valid sleep states?

If the sink returns a power state like DP_SET_POWER_D3_AUX_ON (0x5), or if
it returns 0x2 with non-zero reserved bits due to hardware quirks, this check
evaluates to false.

Without isolating the power state bits using a bitmask (or simply checking
if the value is not DP_SET_POWER_D0), the driver would skip writing
DP_SET_POWER_D0. Can this leave the main link powered down and cause
subsequent DPCD reads or link training to fail?

> +		/* After setting to D0 need a min of 1ms to wake (Spec DP2.1 sec 2.3.1.2) */
> +		drm_dp_dpcd_write_byte(&intel_dp->aux, DP_SET_POWER,
> +				       DP_SET_POWER_D0);
> +		fsleep(1000);
> +		drm_dp_dpcd_write_byte(&intel_dp->aux, DP_SET_POWER,
> +				       DP_SET_POWER_D0);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908161248.1901822-1-arun.r.murthy@intel.com?part=1