Re: [PATCH v8 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 plug and when powered off
--

commit 975cd993397b923350a403b7275ed9d5223bb6a9
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 by USB-C AltMode handlers.

> 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]
Could running msm_dp_hpd_plug_handle() and msm_dp_irq_hpd_handle()
sequentially here trigger link maintenance while the display and link clocks
are still powered off, or attempt AUX transactions while the PHY is exited?

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_bridge_hpd_notify() now executes msm_dp_hpd_plug_handle(dp) followed
unconditionally by msm_dp_irq_hpd_handle(dp).

If dp->plugged is false when this runs (for example, when an AltMode driver
reports both HPD state and HPD IRQ on initial connect, or when a monitor is
plugged into a USB-C dock after msm_dp_hpd_unplug_handle() set dp->plugged
to false on sink_count == 0), msm_dp_hpd_plug_handle() initializes the PHY
and calls msm_dp_display_process_hpd_high():

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_process_hpd_high() {
	...
	msm_dp_link_process_request(dp->link);
	...
}

That first call to msm_dp_link_process_request() updates
link->prev_sink_count to match link->msm_dp_link.sink_count. However,
drm_kms_helper_connector_hotplug_event() and msm_dp_display_atomic_enable()
have not enabled the stream yet, so dp->msm_dp_display.power_on is false,
ctrl->link_clks_on is false, and the mainlink is untrained.

Immediately afterward, msm_dp_irq_hpd_handle() calls
msm_dp_link_process_request() a second time:

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);
	}
	...
}

Because link->prev_sink_count already equals sink_count, if
DP_DOWNSTREAM_PORT_STATUS_CHANGED is no longer set in
DP_LANE_ALIGN_STATUS_UPDATED, 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;
		}
	}
	...
}

Since the link has not been trained yet (or if an out-of-band
DRM_CONNECTOR_DP_IRQ_HPD arrives while dp->plugged is true before
msm_dp_display_atomic_enable() turns the display on), drm_dp_channel_eq_ok()
is false and DP_LINK_STATUS_UPDATED is flagged. That causes
msm_dp_display_handle_irq_hpd() -> msm_dp_ctrl_handle_sink_request() ->
msm_dp_ctrl_link_maintenance() to program DP_STATE_CTRL_PUSH_IDLE, mainlink
setup, and DP_STATE_CTRL_SEND_VIDEO into dp_link registers while
dp->msm_dp_display.power_on and ctrl->link_clks_on are false.

In addition, if the display was disabled via
msm_dp_display_atomic_post_disable() -> msm_dp_display_disable() while a
sink remained connected (sink_count != 0):

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_disable() {
	...
	if (dp->link->sink_count == 0)
		/* re-init the PHY so that we can listen to Dongle disconnect */
		msm_dp_ctrl_reinit_phy(dp->ctrl);
	else
		msm_dp_display_host_phy_exit(dp);

	msm_dp_display->power_on = false;
	...
}

dp->phy_initialized is false while dp->plugged stays true. If an out-of-band
DRM_CONNECTOR_DP_IRQ_HPD arrives in this state, msm_dp_hpd_plug_handle()
returns early because dp->plugged is true, and msm_dp_irq_hpd_handle() then
attempts AUX reads while the PHY is powered down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-hpd-irq-events-v8-0-6ee949e7a384@oss.qualcomm.com?part=7

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic