Re: [PATCH v7 7/9] drm/msm/dp: handle the IRQ_HPD events reported by USB-C

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: msm_dp_bridge_hpd_notify() runs irq_hpd on unpowered link
--

commit 378c862ef1c761eb90130da10b12f8747c15b1a2
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dp: handle the IRQ_HPD events reported by USB-C

This commit updates msm_dp_bridge_hpd_notify() to handle out-of-band
DRM_CONNECTOR_DP_IRQ_HPD events reported via USB-C AltMode notifications
and restructures the connected branch so that msm_dp_hpd_plug_handle() runs
prior to msm_dp_irq_hpd_handle().

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ce919a26fba28..771c8b0744aee 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1566,11 +1566,13 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
>  		if (hpd_link_status == ISR_HPD_REPLUG_COUNT) {
>  			msm_dp_hpd_unplug_handle(dp);
>  			msm_dp_hpd_plug_handle(dp);
> -		} else if (hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT) {
> -			msm_dp_irq_hpd_handle(dp);
>  		} else {
>  			msm_dp_hpd_plug_handle(dp);
>  		}
> +
> +		if (hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT ||
> +		    event == DRM_CONNECTOR_DP_IRQ_HPD)
> +			msm_dp_irq_hpd_handle(dp);

[Severity: High]
Can calling msm_dp_irq_hpd_handle(dp) unconditionally after
msm_dp_hpd_plug_handle(dp) trigger link maintenance while the mainlink and
link clocks are still off?

Previously, msm_dp_hpd_plug_handle(dp) and msm_dp_irq_hpd_handle(dp) were in
mutually exclusive branches in msm_dp_bridge_hpd_notify(). With this change,
when status is connector_status_connected and either event is
DRM_CONNECTOR_DP_IRQ_HPD or hpd_link_status is ISR_IRQ_HPD_PULSE_COUNT,
msm_dp_hpd_plug_handle(dp) runs first and is immediately followed by
msm_dp_irq_hpd_handle(dp).

When dp->plugged is false on entry (such as when a USB-C AltMode driver
reports both HPD_STATE and IRQ_HPD together on initial plug, or when a
monitor is plugged into a connected USB-C branch device whose prior
sink_count == 0 caused msm_dp_hpd_unplug_handle() to clear dp->plugged), or
after msm_dp_hpd_unplug_handle(dp) in the ISR_HPD_REPLUG_COUNT path,
msm_dp_hpd_plug_handle(dp) runs:

msm_dp_hpd_plug_handle()
  -> msm_dp_display_process_hpd_high()
       -> msm_dp_link_process_request()
            -> msm_dp_link_process_ds_port_status_change()

At this point, msm_dp_hpd_plug_handle(dp) only enables AUX transfers and
initializes the PHY; dp->msm_dp_display.power_on and ctrl->link_clks_on
remain false until msm_dp_display_atomic_enable() is called. However,
msm_dp_link_process_ds_port_status_change() has already updated
link->prev_sink_count to match link->msm_dp_link.sink_count.

When msm_dp_irq_hpd_handle(dp) then runs immediately afterward (or when an
IRQ_HPD arrives while dp->plugged is true before atomic_enable() has enabled
the mainlink):

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_irq_hpd_handle() {
	rc = msm_dp_link_process_request(dp->link);
	if (!rc) {
		sink_request = dp->link->sink_request;
		drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request);
		if (sink_request & DS_PORT_STATUS_CHANGED)
			rc = msm_dp_display_process_hpd_high(dp);
		else
			rc = msm_dp_display_handle_irq_hpd(dp);
	}
}

If DP_DOWNSTREAM_PORT_STATUS_CHANGED is still set in
DP_LANE_ALIGN_STATUS_UPDATED, msm_dp_display_process_hpd_high(dp) is called
a second time, redundantly re-initializing LTTPRs and re-reading DPCD and
EDID.

Otherwise, because link->prev_sink_count already equals sink_count,
msm_dp_link_process_ds_port_status_change() returns -EINVAL and
msm_dp_link_process_request() falls through to
msm_dp_link_process_link_status_update():

drivers/gpu/drm/msm/dp/dp_link.c:msm_dp_link_process_request() {
	...
	} else {
		ret = msm_dp_link_process_link_status_update(link);
		if (!ret) {
			msm_dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
}

Because the link has not been trained yet (dp->msm_dp_display.power_on is
false and DP_INTERLANE_ALIGN_DONE is 0), drm_dp_channel_eq_ok() is false, so
msm_dp_link_process_link_status_update() returns 0 and flags
DP_LINK_STATUS_UPDATED.

msm_dp_irq_hpd_handle(dp) then takes the else branch into
msm_dp_display_handle_irq_hpd(dp) -> msm_dp_ctrl_handle_sink_request() ->
msm_dp_ctrl_link_maintenance():

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_link_maintenance() {
	int ret = 0;
	int training_step = DP_TRAINING_NONE;

	msm_dp_ctrl_push_idle(&ctrl->msm_dp_ctrl);

	ctrl->link->phy_params.p_level = 0;
	ctrl->link->phy_params.v_level = 0;

	ret = msm_dp_ctrl_setup_main_link(ctrl, panel, &training_step);
	if (ret)
		goto end;

	msm_dp_ctrl_clear_training_pattern(ctrl, panel, DP_PHY_DPRX);

	msm_dp_write_link(ctrl, REG_DP_STATE_CTRL, DP_STATE_CTRL_SEND_VIDEO);

	ret = msm_dp_ctrl_wait4video_ready(ctrl);
}

This drives msm_dp_ctrl_push_idle() and writes to link_base registers while
!dp->msm_dp_display.power_on and !ctrl->link_clks_on, which causes unclocked
register accesses, PUSH_IDLE timeouts, or a TrustZone force-stop and SoC
reset (as noted in msm_dp_display_atomic_disable() where
msm_dp_ctrl_push_idle() is guarded by dp->power_on).

Should msm_dp_irq_hpd_handle(dp) be skipped when msm_dp_hpd_plug_handle(dp)
just handled a fresh plug, and should link maintenance in the IRQ_HPD path
check that the mainlink is powered on before attempting to retrain the link?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-hpd-irq-events-v7-0-57a3aea3f09e@oss.qualcomm.com?part=7