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. > + 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. > + 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. > + 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. > > ret = hibmc_dp_hw_init(&priv->dp); > if (ret) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
