Thank you for your contribution! Sashiko AI review found 4 potential issue(s)
to consider:
- [High] drm/hisilicon: Hardcoded initial HPD_OUT state breaks boot display
detection
- [High] drm/hisilicon: Removal of HPD debounce causes DPCD read failures on
hotplug
- [Medium] drm/hisilicon: Early exit in encoder_enable breaks KMS forced
modesets
- [Low] drm/hisilicon: Data race on dp->hpd_status between hardirq and process
contexts
--
commit 19dc442443127128a0c2f9bcb0ceca722bb81f14
Author: Lin He <[email protected]>
drm/hisilicon/hibmc: Modify the method of obtaining the hpd_status
This commit replaces the polling mechanism in the lower half of the HPD
interrupt with a direct read of the interrupt status in the upper half.
It also skips detection and training if the status is not HPD_IN, and
initializes the state to HPD_OUT.
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index d5bd3c45649b2..a4cd4cd8cc75e 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
[ ... ]
@@ -322,20 +326,26 @@ void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct
hibmc_dp_cbar_cfg *cfg)
writel(HIBMC_DP_SYNC_EN_MASK, dp_dev->base + HIBMC_DP_TIMING_SYNC_CTRL);
}
-bool hibmc_dp_check_hpd_status(struct hibmc_dp *dp, int exp_status)
+int hibmc_dp_get_hpd_status(struct hibmc_dp *dp)
{
+ int hpd_status = HIBMC_HPD_UNKNOWN;
u32 status;
- int ret;
- ret = readl_poll_timeout(dp->dp_dev->base + HIBMC_DP_HPD_STATUS, status,
- FIELD_GET(HIBMC_DP_HPD_CUR_STATE, status) ==
exp_status,
- 1000, 100000); /* DP spec says 100ms */
- if (ret) {
- drm_dbg_dp(dp->drm_dev, "wait hpd status timeout");
- return false;
+ status = FIELD_GET(HIBMC_DP_HPD_CUR_STATE,
+ readl(dp->dp_dev->base + HIBMC_DP_HPD_STATUS));
+ switch (status) {
+ case 0: /* idle */
+ case 3: /* unplug */
+ case 4: /* unplug intermediate */
+ hpd_status = HIBMC_HPD_OUT;
+ break;
+ case 1: /* plug */
+ case 2: /* plug intermediate */
+ hpd_status = HIBMC_HPD_IN;
[Severity: High]
Does mapping the 'plug intermediate' state (value 2) directly to HIBMC_HPD_IN
without the previous 100ms debounce loop cause DPCD read failures?
When the link is physically bouncing during a hotplug event, returning
HIBMC_HPD_IN immediately schedules hibmc_dp_detect(). The immediate AUX
transaction in hibmc_dp_get_dpcd() can then fail because the connection has
not stabilized, causing the hotplug event to be dropped.
As answered in [1]: debouncing is implemented on-chip. The driver
reads the debounced HPD state-machine output (HIBMC_DP_HPD_CUR_STATE
in HIBMC_DP_HPD_STATUS), never the raw HPD pin, so mechanical contact
bounce is filtered by hardware before software can observe it.
Regarding 'plug intermediate' (state 2): despite the name, this is
not an unstable plug. The state machine enters state 2 only from
the stable plug state, to debounce a possible unplug; the sink
remains connected throughout. So mapping it to HIBMC_HPD_IN is
consistent with the plug interrupt that fired.
Note also that the removed loop was not a debounce: our hardware
already implements the debouncing, so the software 100 ms loop
served no purpose.
+ break;
+ default:
+ break;
}
- dp->dp_dev->hpd_status = exp_status;
-
- return true;
}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 2e9403b8bf3c8..1f8ea0b092537 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
[ ... ]
@@ -63,11 +61,10 @@ static int hibmc_dp_detect(struct drm_connector *connector,
struct hibmc_dp_dev *dp_dev = dp->dp_dev;
int ret = connector_status_disconnected;
- if (dp->irq_status) {
- if (dp_dev->hpd_status != HIBMC_HPD_IN) {
- ret = connector_status_disconnected;
- goto exit;
- }
+ if (dp->hpd_status != HIBMC_HPD_IN) {
[Severity: Low]
Is there a theoretical data race here when reading dp->hpd_status?
It appears to be modified in hardirq context by hibmc_dp_interrupt() and read
here in process context without WRITE_ONCE()/READ_ONCE() annotations or
locking.
Okay, I'll change it to use READ_ONCE and WRITE_ONCE; they are indeed more
secure.
+ drm_dbg_dp(dp->drm_dev, "dp detect skipped, hpd (%d)\n",
+ dp->hpd_status);
+ goto exit;
}
if (!hibmc_dp_get_dpcd(dp_dev)) {
[ ... ]
@@ -166,6 +163,12 @@ static void hibmc_dp_encoder_enable(struct drm_encoder
*drm_encoder,
struct hibmc_dp *dp = container_of(drm_encoder, struct hibmc_dp,
encoder);
struct drm_display_mode *mode = &drm_encoder->crtc->state->mode;
+ if (dp->hpd_status != HIBMC_HPD_IN) {
[Severity: Medium]
Does this early exit break KMS forced modesets?
If a user overrides the connector to 'connected' (e.g., via sysfs or kernel
command line) and the DRM core attempts an atomic commit, this check will
refuse to program the hardware. This would desynchronize the KMS software
state from the actual hardware state.
Okay, the original purpose of this interceptor was to reduce the number of
exception logs when HPD_OUT; it serves no other purpose. I will remove it in v4.
+ drm_dbg_dp(dp->drm_dev, "dp encoder enable skipped, hpd (%d)\n",
+ dp->hpd_status);
+ return;
+ }
+
if (hibmc_dp_prepare(dp, mode))
return;
[ ... ]
@@ -223,6 +228,7 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
dp->mmio = priv->mmio;
dp->drm_dev = dev;
+ dp->hpd_status = HIBMC_HPD_OUT;
[Severity: High]
Will hardcoding the initial state to HIBMC_HPD_OUT break display detection
at boot?
During boot, drm_client_setup() runs detect() before or immediately after
interrupts are enabled in late_register(). Because detect() no longer queries
the live hardware register and relies only on this cached value, it will read
HPD_OUT and silently force a disconnected state even if a monitor is already
connected.
As explained in [1], a monitor connected before boot does trigger an
HPD interrupt: the event is latched by hardware while DP interrupts
are still masked. hibmc_dp_hw_init() ends with a 100 ms delay, which
gives the hardware time to complete the debouncing and latch the
interrupt; this already detects the plug-in in the vast majority of
cases.
Moreover, even if a transient or sporadic hardware issue prevents
the plug-in from being recognized within the 100 ms window, or even
by the time drm_client_setup() runs, hibmc_dp_hpd_isr(), the
threaded bottom half of the HPD interrupt, re-triggers the hotplug
flow via drm_connector_helper_hpd_irq_event(), so the DP plug-in
event is never lost.
[1] https://lore.kernel.org/all/[email protected]/
Thanks,
Yongbang.
