Re: [PATCH drm-dp 02/10] drm/hisilicon/hibmc: fix dp probabilistical detect errors after HPD irq

[Yongbang Shi]

On Fri, May 30, 2025 at 05:54:24PM +0800, Yongbang Shi wrote:

From: Baihan Li <libai...@huawei.com>

The debouncing when HPD pulled out still remains sometimes, 200ms still can
not ensure helper_detect() is correct. So add a flag to hold the sink
status, and changed detect_ctx() functions by using flag to check status.

Fixes: 3c7623fb5bb6 ("drm/hisilicon/hibmc: Enable this hot plug detect of irq 
feature")
Signed-off-by: Baihan Li <libai...@huawei.com>
---
  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  1 +
  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 38 +++++++++++++------
  2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h 
b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 665f5b166dfb..68867475508c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -50,6 +50,7 @@ struct hibmc_dp {
        struct drm_dp_aux aux;
        struct hibmc_dp_cbar_cfg cfg;
        u32 irq_status;
+       int hpd_status;
  };

int hibmc_dp_hw_init(struct hibmc_dp *dp);

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c 
b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index d06832e62e96..191fb434baa7 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -13,7 +13,8 @@
  #include "hibmc_drm_drv.h"
  #include "dp/dp_hw.h"

-#define DP_MASKED_SINK_HPD_PLUG_INT BIT(2)

+#define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT      BIT(2)
+#define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT    BIT(3)

static int hibmc_dp_connector_get_modes(struct drm_connector *connector)

  {
@@ -34,9 +35,12 @@ static int hibmc_dp_connector_get_modes(struct drm_connector 
*connector)
  static int hibmc_dp_detect(struct drm_connector *connector,
                           struct drm_modeset_acquire_ctx *ctx, bool force)
  {
-       mdelay(200);
+       struct hibmc_dp *dp = to_hibmc_dp(connector);

- return drm_connector_helper_detect_from_ddc(connector, ctx, force);

+       if (dp->hpd_status)
+               return connector_status_connected;
+       else
+               return connector_status_disconnected;
  }

static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {

@@ -115,22 +119,34 @@ irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
  {
        struct drm_device *dev = (struct drm_device *)arg;
        struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+       struct hibmc_dp *dp = &priv->dp;
        int idx;

if (!drm_dev_enter(dev, &idx))

                return -ENODEV;

- if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {

-               drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
-               hibmc_dp_hpd_cfg(&priv->dp);
+       if (dp->hpd_status) { /* only check unplug int when the last status is 
HPD in */

I think this way you'll ignore HPD short pulses. Could you possibly
clarify whether it is the case or not?

We actually doesn't enable short HPD here, this feature just used in our 
electrical tests.

+               if ((dp->irq_status & HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT)) {
+                       drm_dbg_dp(dev, "HPD OUT isr occur.");
+                       hibmc_dp_reset_link(dp);
+                       dp->hpd_status = 0;
+                       if (dev->registered)
+                               
drm_connector_helper_hpd_irq_event(&dp->connector);
+               } else {
+                       drm_warn(dev, "HPD OUT occurs, irq status err: %u", 
dp->irq_status);

These should be ratelimited.

Sorry, I didn't get it. Do you mean I need print the link rate here?

+               }
        } else {
-               drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
-               hibmc_dp_reset_link(&priv->dp);
+               if (dp->irq_status & HIBMC_DP_MASKED_SINK_HPD_PLUG_INT) {
+                       drm_dbg_dp(&priv->dev, "HPD IN isr occur.");
+                       hibmc_dp_hpd_cfg(dp);
+                       dp->hpd_status = 1;
+                       if (dev->registered)
+                               
drm_connector_helper_hpd_irq_event(&dp->connector);
+               } else {
+                       drm_warn(dev, "HPD IN occurs, irq status err: %u", 
dp->irq_status);
+               }
        }

- if (dev->registered)

-               drm_connector_helper_hpd_irq_event(&priv->dp.connector);

There is no need to, just call this function always at the end of the
ISR handler as it is done currently.

Ok.

-
        drm_dev_exit(idx);

return IRQ_HANDLED;

--
2.33.0