Re: [PATCH v4 2/2] drm: bridge: cdns-mhdp8546: Add no-hpd property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Polling monitor presence using `drm_dp_dpcd_read_link_status()` reads from link training status registers (DP_LANE0_1_STATUS), which can disrupt the DP sink's state machine.
- [High] Calling `cdns_mhdp_update_link_status()` from the `.detect()` callback continuously re-programs live video registers during DRM polling.
- [High] Missing `drm_dp_aux_unregister` on the firmware load timeout error path in `cdns_mhdp_attach`.
- [Medium] In no-hpd mode, link status evaluation is bypassed, preventing recovery from main link failures.
- [Medium] A hardcoded 100ms timeout in `cdns_mhdp_attach` to wait for asynchronous firmware loading is too short and will cause initialization failures.
--

commit 2c6e286375fe2e77fec97f0ea17066c50894dd26
Author: Rahul T R <r-ravikumar@ti.com>

drm: bridge: cdns-mhdp8546: Add no-hpd property

This patch adds a 'no-hpd' property to support boards where the HPD line cannot
be used for hotplug detection. It implements a polling mechanism using AUX reads
to detect monitor presence when hardware HPD signals are unavailable.

> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 36c07b71fe04b..5e1bad8fc73ea 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
[ ... ]
> @@ -1523,6 +1532,26 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
>  
>  	spin_unlock(&mhdp->start_lock);
>  
> +	if (mhdp->no_hpd) {
> +		/*
> +		 * In no-hpd mode there is no interrupt to signal firmware
> +		 * readiness. The firmware loads asynchronously after probe(),
> +		 * so we must wait here until the uCPU is running before
> +		 * attempting the first AUX channel poll for monitor presence.
> +		 */
> +		ret = wait_event_timeout(mhdp->fw_load_wq,
> +					 mhdp->hw_state == MHDP_HW_READY,
> +					 msecs_to_jiffies(100));

[Severity: Medium]
Will this 100ms timeout cause initialization failures?

Async firmware loading can routinely take longer than 100ms on a busy system or
when reading from slow storage.

> +		if (ret == 0) {
> +			dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n",
> +				__func__);
> +			return -ETIMEDOUT;

[Severity: High]
Does this error path leak the registered AUX channel?

Earlier in cdns_mhdp_attach(), the DRM DP AUX channel is registered using
drm_dp_aux_register(). Returning directly here skips the aux_unregister error
path label at the end of the function, leaving the AUX channel registered but
in an invalid state.

> +		}
> +
> +		cdns_mhdp_update_link_status(mhdp);
> +		return 0;
> +	}
> +
[ ... ]
> @@ -2100,7 +2132,16 @@ static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp)
>  
>  	mutex_lock(&mhdp->link_mutex);
>  
> -	mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
> +	if (mhdp->no_hpd) {
> +		ret = drm_dp_dpcd_read_link_status(&mhdp->aux, status);

[Severity: High]
Is it safe to read link training status registers to poll for monitor presence?

drm_dp_dpcd_read_link_status() reads the DP_LANE0_1_STATUS (0x202) register.
According to the DRM subsystem guidelines, reading link training status
registers (0x202-0x207) outside of the expected training sequence can trigger
unintended state machine transitions in DP/eDP sinks. Could this cause hardware
state corruption or screen flickering?

> +		hpd_pulse = false;

[Severity: Medium]
Does hardcoding hpd_pulse to false bypass link retraining?

Later in cdns_mhdp_update_link_status(), link validation logic checks
if (hpd_pulse && old_plugged == mhdp->plugged). By hardcoding this to false,
that logic is skipped. If the main high-speed link loses synchronization but
the monitor remains connected, wouldn't the driver fail to retrain the link?

[Severity: High]
Does this also cause live video stream registers to be reprogrammed on every
polling event?

Since hpd_pulse is false, the function skips the early return path for a healthy
link (goto out;). It then falls through to the end of the function where it
unconditionally executes cdns_mhdp_sst_enable(mhdp, current_mode) if
mhdp->bridge_enabled is true.

When called via periodic DRM polling via the .detect() callback, won't this
cause severe screen flickering or sync loss by continuously overwriting active
hardware display timing registers?

> +		if (ret < 0)
> +			mhdp->plugged = false;
> +		else
> +			mhdp->plugged = true;
> +	} else {
> +		mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630102610.1849902-1-y-d@ti.com?part=2