Re: [PATCH 5/5] drm/nouveau: don't let an IRQ_HPD hide a plug or unplug event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/nouveau: lost plug/unplug bits in nouveau_connector_hpd() when pending
- [Medium] drm/nouveau: nouveau_display_hpd_work() skips link check on IRQ with plug
--

commit 8aa36f0d839e814a361fc038f601698183c285f9
Author: David R. Hedges <linuxdev844@thelittleman.net>

drm/nouveau: don't let an IRQ_HPD hide a plug or unplug event

This commit updates nouveau_display_hpd_work() so that the
nouveau_dp_link_check() shortcut is only taken when NVIF_CONN_EVENT_V0_IRQ is
set without NVIF_CONN_EVENT_V0_PLUG or NVIF_CONN_EVENT_V0_UNPLUG. This
prevents plug or unplug events queued alongside an IRQ_HPD from skipping
connector detection.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index d71dcfc6ee66b..4398cf0fc541c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -462,7 +462,12 @@ nouveau_display_hpd_work(struct work_struct *work)
>  			    !!(bits & NVIF_CONN_EVENT_V0_UNPLUG),
>  			    !!(bits & NVIF_CONN_EVENT_V0_IRQ));
>  
> -		if (bits & NVIF_CONN_EVENT_V0_IRQ) {
> +		/* A plug or unplug event pending alongside the IRQ, such as a
> +		 * sink count change reported by nouveau_dp_irq(), still needs
> +		 * connector detection.
> +		 */
> +		if ((bits & NVIF_CONN_EVENT_V0_IRQ) &&
> +		    !(bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG))) {

[Severity: Medium]
Can a separate plug or unplug event still be dropped if it arrives while an
earlier NVIF_CONN_EVENT_V0_IRQ is already pending for the same connector?

In drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_hpd(),
updating nv_connector->hpd_pending is guarded by the drm->hpd_pending check:

	spin_lock_irqsave(&drm->hpd_lock, flags);
	if (!(drm->hpd_pending & mask)) {
		nv_connector->hpd_pending |= bits;
		drm->hpd_pending |= mask;
		schedule_work(&drm->hpd_work);
	}
	spin_unlock_irqrestore(&drm->hpd_lock, flags);

If nouveau_connector_irq() -> nouveau_dp_irq() first queues
NVIF_CONN_EVENT_V0_IRQ (before a sink count change is visible), and a plug or
unplug event then arrives via nouveau_connector_hotplug() or a follow-up
nouveau_dp_irq() before nouveau_display_hpd_work() finishes
pm_runtime_get_sync() and clears drm->hpd_pending, drm->hpd_pending & mask is
already set. The new NVIF_CONN_EVENT_V0_PLUG or NVIF_CONN_EVENT_V0_UNPLUG bit
is never ORed into nv_connector->hpd_pending.

When nouveau_display_hpd_work() then reads nv_connector->hpd_pending, bits
still only contains NVIF_CONN_EVENT_V0_IRQ, so nouveau_dp_link_check() returns
true when no link is trained and connector detection is still skipped.

Should nv_connector->hpd_pending |= bits be moved outside the
if (!(drm->hpd_pending & mask)) check in nouveau_connector_hpd()?

>  			if (nouveau_dp_link_check(nv_connector))
>  				continue;
>  		}

[Severity: Medium]
Does guarding the outer if statement instead of the continue statement skip
DP link retraining when NVIF_CONN_EVENT_V0_IRQ is set alongside
NVIF_CONN_EVENT_V0_PLUG or NVIF_CONN_EVENT_V0_UNPLUG?

In drivers/gpu/drm/nouveau/nouveau_dp.c, nouveau_dp_link_check() ->
nouveau_dp_link_check_locked() is the only HPD path that checks channel
equalization on an active link (outp->dp.lt.nr != 0) and retrains it via
nouveau_dp_train_link(outp, true):

	if (drm_dp_dpcd_read_phy_link_status(&outp->conn->aux, DP_PHY_DPRX, link_status) < 0)
		return false;

	if (drm_dp_channel_eq_ok(link_status, outp->dp.lt.nr))
		return true;

	return nouveau_dp_train_link(outp, true);

If a plug event runs first and nouveau_display_hpd_work() clears
drm->hpd_pending = 0 before acquiring dev->mode_config.mutex, a subsequent
IRQ_HPD in nouveau_dp_irq() -> nouveau_connector_hpd() sees
!(drm->hpd_pending & mask), ORs NVIF_CONN_EVENT_V0_IRQ into
nv_connector->hpd_pending alongside NVIF_CONN_EVENT_V0_PLUG, and reschedules
drm->hpd_work. The same combination also occurs directly when nouveau_dp_irq()
observes a sink count change on an active SST branch device while the link
needs retraining.

The first worker run sees both NVIF_CONN_EVENT_V0_IRQ and
NVIF_CONN_EVENT_V0_PLUG in bits, clears nv_connector->hpd_pending = 0, and
skips nouveau_dp_link_check(). Because drm_helper_probe_detect() ->
nouveau_connector_detect() -> nouveau_dp_detect() does not check link status
or retrain the link, and any rescheduled worker run sees bits == 0, the link
check is lost.

Would it be better to always run nouveau_dp_link_check(nv_connector) when
NVIF_CONN_EVENT_V0_IRQ is set, and only guard the continue statement with
!(bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG))?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-nouveau-dp-fixes-v1-0-892f4bd357cd@thelittleman.net?part=5