[PATCH drm-dp 00/10] Fix hibmc driver bugs

[PATCH drm-dp 00/10] Fix hibmc driver bugs

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

There are some bugfix for hibmc-drm driver.

Baihan Li (10):
  drm/hisilicon/hibmc: fix the i2c device resource leak when vdac init
    failed
  drm/hisilicon/hibmc: fix dp probabilistical detect errors after HPD
    irq
  drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local
  drm/hisilicon/hibmc: fix the hibmc loaded failed bug
  drm/hisilicon/hibmc: fix rare monitors cannot display problem
  drm/hisilicon/hibmc: add dp mode valid check
  drm/hisilicon/hibmc: add dp encoder modeset
  drm/hisilicon/hibmc: fix DP no showing after HPD with VGA connected
  drm/hisilicon/hibmc: fix HPD no showing with VGA para of GRUB
  drm/hisilicon/hibmc: fix no showing problem with loading hibmc
    manually

 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |   4 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  22 ++--
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |   8 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  |  33 +++--
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    |  12 --
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 115 ++++++++++++++++--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  14 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c   |   5 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  |  37 +++++-
 10 files changed, 198 insertions(+), 53 deletions(-)

-- 
2.33.0

[PATCH drm-dp 01/10] drm/hisilicon/hibmc: fix the i2c device resource leak when vdac init failed

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Currently the driver missed to clean the i2c adapter when vdac init failed.
It may cause resource leak.

Fixes: 94ee73ee3020 ("drm/hisilicon/hibmc: add dp hw moduel in hibmc driver")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  | 1 +
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c  | 5 +++++
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 9 +++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index 274feabe7df0..ca8502e2760c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -69,6 +69,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv);
 int hibmc_vdac_init(struct hibmc_drm_private *priv);
 
 int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
+void hibmc_ddc_del(struct hibmc_vdac *vdac);
 
 int hibmc_dp_init(struct hibmc_drm_private *priv);
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
index 99b3b77b5445..44860011855e 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
@@ -95,3 +95,8 @@ int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *vdac)
 
 	return i2c_bit_add_bus(&vdac->adapter);
 }
+
+void hibmc_ddc_del(struct hibmc_vdac *vdac)
+{
+	i2c_del_adapter(&vdac->adapter);
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index e8a527ede854..36401b46034c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -110,7 +110,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
 	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
 	if (ret) {
 		drm_err(dev, "failed to init encoder: %d\n", ret);
-		return ret;
+		goto err;
 	}
 
 	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
@@ -121,7 +121,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
 					  &vdac->adapter);
 	if (ret) {
 		drm_err(dev, "failed to init connector: %d\n", ret);
-		return ret;
+		goto err;
 	}
 
 	drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
@@ -131,4 +131,9 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return 0;
+
+err:
+	hibmc_ddc_del(vdac);
+
+	return ret;
 }
-- 
2.33.0

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

[Yongbang Shi]

From: Baihan Li <libaihan@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 <libaihan@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 */
+		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);
+		}
 	} 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);
-
 	drm_dev_exit(idx);
 
 	return IRQ_HANDLED;
-- 
2.33.0

[PATCH drm-dp 03/10] drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

The local variable of irq name is passed to devm_request_threaded_irq(),
which will make request_irq failed. Using the global irq name instead
of it to fix.

Fixes: b11bc1ae4658 ("drm/hisilicon/hibmc: Add MSI irq getting and requesting for HPD")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 768b97f9e74a..4cdcc34070ee 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -32,7 +32,7 @@
 
 DEFINE_DRM_GEM_FOPS(hibmc_fops);
 
-static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "vblank", "hpd" };
+static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "hibmc-vblank", "hibmc-hpd" };
 
 static irqreturn_t hibmc_interrupt(int irq, void *arg)
 {
@@ -277,7 +277,6 @@ static void hibmc_unload(struct drm_device *dev)
 static int hibmc_msi_init(struct drm_device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	char name[32] = {0};
 	int valid_irq_num;
 	int irq;
 	int ret;
@@ -292,9 +291,6 @@ static int hibmc_msi_init(struct drm_device *dev)
 	valid_irq_num = ret;
 
 	for (int i = 0; i < valid_irq_num; i++) {
-		snprintf(name, ARRAY_SIZE(name) - 1, "%s-%s-%s",
-			 dev->driver->name, pci_name(pdev), g_irqs_names_map[i]);
-
 		irq = pci_irq_vector(pdev, i);
 
 		if (i)
@@ -302,10 +298,10 @@ static int hibmc_msi_init(struct drm_device *dev)
 			ret = devm_request_threaded_irq(&pdev->dev, irq,
 							hibmc_dp_interrupt,
 							hibmc_dp_hpd_isr,
-							IRQF_SHARED, name, dev);
+							IRQF_SHARED, g_irqs_names_map[i], dev);
 		else
 			ret = devm_request_irq(&pdev->dev, irq, hibmc_interrupt,
-					       IRQF_SHARED, name, dev);
+					       IRQF_SHARED, g_irqs_names_map[i], dev);
 		if (ret) {
 			drm_err(dev, "install irq failed: %d\n", ret);
 			return ret;
-- 
2.33.0

[PATCH drm-dp 04/10] drm/hisilicon/hibmc: fix the hibmc loaded failed bug

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

When hibmc loaded failed, the driver use hibmc_unload to free the
resource, but the mutexes in mode.config are not init, which will
access an NULL pointer.

Fixes: b3df5e65cc03 ("drm/hibmc: Drop drm_vblank_cleanup")
Reported-by: oushixiong1025@163.com
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 4cdcc34070ee..ac552c339671 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -319,13 +319,13 @@ static int hibmc_load(struct drm_device *dev)
 
 	ret = hibmc_hw_init(priv);
 	if (ret)
-		goto err;
+		return ret;
 
 	ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
 				    pci_resource_len(pdev, 0));
 	if (ret) {
 		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
-		goto err;
+		return ret;
 	}
 
 	ret = hibmc_kms_init(priv);
-- 
2.33.0

[PATCH drm-dp 05/10] drm/hisilicon/hibmc: fix rare monitors cannot display problem

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

DP Link training successful at 8.1Gbps with some monitors' max link rate
are 2.7Gbps. So change the default 8.1Gbps link rate to the rate that reads
from devices' capabilities.

Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  4 ++-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 +---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 +++++++++++++------
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 12 -------
 4 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index 4add05c7f161..18a961466ff0 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -25,6 +25,9 @@ struct hibmc_link_status {
 struct hibmc_link_cap {
 	u8 link_rate;
 	u8 lanes;
+	int rx_dpcd_revision;
+	bool is_tps3;
+	bool is_tps4;
 };
 
 struct hibmc_dp_link {
@@ -62,7 +65,6 @@ struct hibmc_dp_dev {
 
 void hibmc_dp_aux_init(struct hibmc_dp *dp);
 int hibmc_dp_link_training(struct hibmc_dp_dev *dp);
-int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp);
 int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp);
 int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX]);
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index 8f0daec7d174..ee0b543afd7f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -154,7 +154,6 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 {
 	struct drm_device *drm_dev = dp->drm_dev;
 	struct hibmc_dp_dev *dp_dev;
-	int ret;
 
 	dp_dev = devm_kzalloc(drm_dev->dev, sizeof(struct hibmc_dp_dev), GFP_KERNEL);
 	if (!dp_dev)
@@ -166,13 +165,10 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 
 	dp_dev->dev = drm_dev;
 	dp_dev->base = dp->mmio + HIBMC_DP_OFFSET;
+	dp_dev->serdes_base = dp_dev->base + HIBMC_DP_HOST_OFFSET;
 
 	hibmc_dp_aux_init(dp);
 
-	ret = hibmc_dp_serdes_init(dp_dev);
-	if (ret)
-		return ret;
-
 	dp_dev->link.cap.lanes = 0x2;
 	dp_dev->link.cap.link_rate = DP_LINK_BW_8_1;
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
index 74f7832ea53e..6c69fa2ae9cf 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -39,6 +39,14 @@ static int hibmc_dp_link_training_configure(struct hibmc_dp_dev *dp)
 	/* enhanced frame */
 	hibmc_dp_reg_write_field(dp, HIBMC_DP_VIDEO_CTRL, HIBMC_DP_CFG_STREAM_FRAME_MODE, 0x1);
 
+	ret = hibmc_dp_get_serdes_rate_cfg(dp);
+	if (ret < 0)
+		return ret;
+
+	ret = hibmc_dp_serdes_rate_switch(ret, dp);
+	if (ret)
+		return ret;
+
 	/* set rate and lane count */
 	buf[0] = dp->link.cap.link_rate;
 	buf[1] = DP_LANE_COUNT_ENHANCED_FRAME_EN | dp->link.cap.lanes;
@@ -325,6 +333,20 @@ static int hibmc_dp_link_downgrade_training_eq(struct hibmc_dp_dev *dp)
 	return hibmc_dp_link_reduce_rate(dp);
 }
 
+static void hibmc_dp_update_caps(struct hibmc_dp_dev *dp)
+{
+	dp->link.cap.rx_dpcd_revision = dp->dpcd[DP_DPCD_REV];
+
+	dp->link.cap.is_tps3 = (dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_13) &&
+			       (dp->dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED);
+	dp->link.cap.is_tps4 = (dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14) &&
+			       (dp->dpcd[DP_MAX_DOWNSPREAD] & DP_TPS4_SUPPORTED);
+	dp->link.cap.link_rate = dp->dpcd[DP_MAX_LINK_RATE];
+	dp->link.cap.lanes = dp->dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK;
+	if (dp->link.cap.lanes > HIBMC_DP_LANE_NUM_MAX)
+		dp->link.cap.lanes = HIBMC_DP_LANE_NUM_MAX;
+}
+
 int hibmc_dp_link_training(struct hibmc_dp_dev *dp)
 {
 	struct hibmc_dp_link *link = &dp->link;
@@ -334,16 +356,7 @@ int hibmc_dp_link_training(struct hibmc_dp_dev *dp)
 	if (ret)
 		drm_err(dp->dev, "dp aux read dpcd failed, ret: %d\n", ret);
 
-	dp->link.cap.link_rate = dp->dpcd[DP_MAX_LINK_RATE];
-	dp->link.cap.lanes = 0x2;
-
-	ret = hibmc_dp_get_serdes_rate_cfg(dp);
-	if (ret < 0)
-		return ret;
-
-	ret = hibmc_dp_serdes_rate_switch(ret, dp);
-	if (ret)
-		return ret;
+	hibmc_dp_update_caps(dp);
 
 	while (true) {
 		ret = hibmc_dp_link_training_cr_pre(dp);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
index 676059d4c1e6..8191233aa965 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
@@ -57,15 +57,3 @@ int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
 
 	return 0;
 }
-
-int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp)
-{
-	dp->serdes_base = dp->base + HIBMC_DP_HOST_OFFSET;
-
-	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
-	       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET);
-	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
-	       dp->serdes_base + HIBMC_DP_PMA_LANE1_OFFSET);
-
-	return hibmc_dp_serdes_rate_switch(DP_SERDES_BW_8_1, dp);
-}
-- 
2.33.0

[PATCH drm-dp 06/10] drm/hisilicon/hibmc: add dp mode valid check

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

If DP is connected, add mode check and BW check in mode_valid_ctx() to
ensure DP's cfg is usable.

Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 10 ++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  7 +++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 59 +++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index ee0b543afd7f..4f93d60b932b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -259,6 +259,16 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp)
 	dp->dp_dev->link.status.channel_equalized = false;
 }
 
+u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.link_rate;
+}
+
+u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.lanes;
+}
+
 static const struct hibmc_dp_color_raw g_rgb_raw[] = {
 	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
 	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 68867475508c..ebc7256ad006 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -12,6 +12,10 @@
 #include <drm/drm_print.h>
 #include <drm/display/drm_dp_helper.h>
 
+/* 27 * 10000000 * 80% = 216000000 */
+#define DP_MODE_VALI_CAL	216000000
+#define BPP_24				24
+
 struct hibmc_dp_dev;
 
 enum hibmc_dp_cbar_pattern {
@@ -51,6 +55,7 @@ struct hibmc_dp {
 	struct hibmc_dp_cbar_cfg cfg;
 	u32 irq_status;
 	int hpd_status;
+	bool is_connected;
 };
 
 int hibmc_dp_hw_init(struct hibmc_dp *dp);
@@ -61,5 +66,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
 void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
 void hibmc_dp_enable_int(struct hibmc_dp *dp);
 void hibmc_dp_disable_int(struct hibmc_dp *dp);
+u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
+u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 191fb434baa7..e4b13f21ccb3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -16,8 +16,31 @@
 #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
 #define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
 
+struct hibmc_dp_disp_clk {
+	u16 hdisplay;
+	u16 vdisplay;
+	u32 clock;
+};
+
+static const struct hibmc_dp_disp_clk hibmc_dp_clk_table[] = {
+	{640, 480, 25175}, /* 25175 khz */
+	{800, 600, 40000}, /* 40000 khz */
+	{1024, 768, 65000}, /* 65000 khz */
+	{1152, 864, 80000}, /* 80000 khz */
+	{1280, 768, 79500}, /* 79500 khz */
+	{1280, 720, 74250}, /* 74250 khz */
+	{1280, 960, 108000}, /* 108000 khz */
+	{1280, 1024, 108000}, /* 108000 khz */
+	{1440, 900, 106500}, /* 106500 khz */
+	{1600, 900, 108000}, /* 108000 khz */
+	{1600, 1200, 162000}, /* 162000 khz */
+	{1920, 1080, 148500}, /* 148500 khz */
+	{1920, 1200, 193250}, /* 193250 khz */
+};
+
 static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
 {
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
 	const struct drm_edid *drm_edid;
 	int count;
 
@@ -27,6 +50,8 @@ static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
 
 	count = drm_edid_connector_add_modes(connector);
 
+	dp->is_connected = !!count;
+
 	drm_edid_free(drm_edid);
 
 	return count;
@@ -43,9 +68,43 @@ static int hibmc_dp_detect(struct drm_connector *connector,
 		return connector_status_disconnected;
 }
 
+static int hibmc_dp_mode_valid(struct drm_connector *connector,
+			       const struct drm_display_mode *mode,
+			       struct drm_modeset_acquire_ctx *ctx,
+			       enum drm_mode_status *status)
+{
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
+	u64 cur_val, max_val;
+
+	if (!dp->is_connected) {
+		*status = MODE_OK;
+		return 0;
+	}
+
+	/* check DP link BW */
+	cur_val = (u64)mode->htotal * mode->vtotal * drm_mode_vrefresh(mode) * BPP_24;
+	max_val = (u64)hibmc_dp_get_link_rate(dp) * DP_MODE_VALI_CAL * hibmc_dp_get_lanes(dp);
+	if (cur_val > max_val)
+		*status = MODE_CLOCK_HIGH;
+	else
+		*status = MODE_OK;
+
+	/* check the clock */
+	for (size_t i = 0; i < ARRAY_SIZE(hibmc_dp_clk_table); i++) {
+		if (hibmc_dp_clk_table[i].hdisplay == mode->hdisplay &&
+		    hibmc_dp_clk_table[i].vdisplay == mode->vdisplay) {
+			if (hibmc_dp_clk_table[i].clock != mode->clock)
+				*status = MODE_CLOCK_RANGE;
+		}
+	}
+
+	return 0;
+}
+
 static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
 	.get_modes = hibmc_dp_connector_get_modes,
 	.detect_ctx = hibmc_dp_detect,
+	.mode_valid_ctx = hibmc_dp_mode_valid,
 };
 
 static int hibmc_dp_late_register(struct drm_connector *connector)
-- 
2.33.0

[PATCH drm-dp 07/10] drm/hisilicon/hibmc: add dp encoder modeset

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Add GPU display control enable in dp_mode_set(), which is already
in vdac's mode_set, however, if vdac is not connected, GPU
cannot work.

Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index e4b13f21ccb3..d9ae7567ebb7 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -11,6 +11,7 @@
 #include <drm/drm_edid.h>
 
 #include "hibmc_drm_drv.h"
+#include "hibmc_drm_regs.h"
 #include "dp/dp_hw.h"
 
 #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
@@ -169,9 +170,26 @@ static void hibmc_dp_encoder_disable(struct drm_encoder *drm_encoder,
 	hibmc_dp_display_en(dp, false);
 }
 
+static void hibmc_dp_encoder_mode_set(struct drm_encoder *encoder,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state)
+{
+	struct drm_device *dev = encoder->dev;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+	u32 reg;
+
+	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
+	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
+	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
+	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
+	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
+	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
+}
+
 static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
 	.atomic_enable = hibmc_dp_encoder_enable,
 	.atomic_disable = hibmc_dp_encoder_disable,
+	.atomic_mode_set = hibmc_dp_encoder_mode_set,
 };
 
 irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
-- 
2.33.0

[PATCH drm-dp 08/10] drm/hisilicon/hibmc: fix DP no showing after HPD with VGA connected

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

If the system started with VGA connected, the desktop like GDM cannot get
DP's CRTC when DP device is plugged in, because there is only one crtc
sharing use of VGA and DP. So change VGA to disconnected when DP is
connected.

Fixes: 4c962bc929f1 ("drm/hisilicon/hibmc: Add vga connector detect functions")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 36401b46034c..73dd3d5fc26c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -49,6 +49,18 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
 	return count;
 }
 
+static int hibmc_vdac_detect(struct drm_connector *connector, struct drm_modeset_acquire_ctx *ctx,
+			     bool force)
+{
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(connector->dev);
+	struct hibmc_dp *dp = &priv->dp;
+
+	if (dp->hpd_status)
+		return connector_status_disconnected;
+
+	return connector_status_connected;
+}
+
 static void hibmc_connector_destroy(struct drm_connector *connector)
 {
 	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
@@ -60,7 +72,7 @@ static void hibmc_connector_destroy(struct drm_connector *connector)
 static const struct drm_connector_helper_funcs
 	hibmc_connector_helper_funcs = {
 	.get_modes = hibmc_connector_get_modes,
-	.detect_ctx = drm_connector_helper_detect_from_ddc,
+	.detect_ctx = hibmc_vdac_detect,
 };
 
 static const struct drm_connector_funcs hibmc_connector_funcs = {
-- 
2.33.0

[PATCH drm-dp 09/10] drm/hisilicon/hibmc: fix HPD no showing with VGA para of GRUB

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

In early OS versions， there is a bug in hibmc-drm driver previously,
so some OS add a VGA parameter in GRUB(video=VGA-1:640x480-32@60me) to
fix the bug, that will config a force VGA mode to drm driver. However, the
HPD problem exists that mentioned in previous patch, so change VGA's status
in force() to compatible with some older OS versions.

Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 73dd3d5fc26c..d609ccda2f2a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -61,6 +61,19 @@ static int hibmc_vdac_detect(struct drm_connector *connector, struct drm_modeset
 	return connector_status_connected;
 }
 
+static void hibmc_vdac_force(struct drm_connector *connector)
+{
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(connector->dev);
+	struct hibmc_dp *dp = &priv->dp;
+
+	if (dp->hpd_status) {
+		connector->status = connector_status_disconnected;
+		return;
+	}
+
+	connector->status = connector_status_connected;
+}
+
 static void hibmc_connector_destroy(struct drm_connector *connector)
 {
 	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
@@ -81,6 +94,7 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.force = hibmc_vdac_force,
 };
 
 static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
-- 
2.33.0

[PATCH drm-dp 10/10] drm/hisilicon/hibmc: fix no showing problem with loading hibmc manually

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

When using command rmmod and insmod, there is no showing in second time
insmoding. Because DP controller won't send HPD signals, if connection
doesn't change or controller isn't reset. So add reset before unreset
in hibmc_dp_hw_init().

Fixes: 94ee73ee3020 ("drm/hisilicon/hibmc: add dp hw moduel in hibmc driver")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index 4f93d60b932b..e1b9589ce639 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -172,13 +172,15 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 	dp_dev->link.cap.lanes = 0x2;
 	dp_dev->link.cap.link_rate = DP_LINK_BW_8_1;
 
-	/* hdcp data */
-	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
 	/* int init */
 	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
 	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
 	/* rst */
+	writel(0, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
+	usleep_range(30, 50);
 	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
+	/* hdcp data */
+	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
 	/* clock enable */
 	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
 
-- 
2.33.0

Re: [PATCH drm-dp 04/10] drm/hisilicon/hibmc: fix the hibmc loaded failed bug

[Yongbang Shi]

>
>> From: Baihan Li<libaihan@huawei.com>
>>
>> When hibmc loaded failed, the driver use hibmc_unload to free the
>> resource, but the mutexes in mode.config are not init, which will
>> access an NULL pointer.
>>
>> Fixes: b3df5e65cc03 ("drm/hibmc: Drop drm_vblank_cleanup")
>> Reported-by:oushixiong1025@163.com
>> Signed-off-by: Baihan Li<libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 4cdcc34070ee..ac552c339671 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -319,13 +319,13 @@ static int hibmc_load(struct drm_device *dev)
>>   
>>   	ret = hibmc_hw_init(priv);
>>   	if (ret)
>> -		goto err;
>> +		return ret;
>>   
>>   	ret = drmm_vram_helper_init(dev, pci_resource_start(pdev, 0),
>>   				    pci_resource_len(pdev, 0));
>>   	if (ret) {
>>   		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
>> -		goto err;
>> +		return ret;
>
> The mutexes in mode.config are initialized when calling hibmc_kms_init(),
> if calling hibmc_kms_init() failed it also need to return.
>
> You may want to look at the following patch:
> LKML: oushixiong1025@163 ...: [PATCH] drm/hisilicon: Fix a NULL 
> pointer access when hibmc_load failed 
> <https://lkml.org/lkml/2025/5/20/331>
>
> Reported-by: Shixiong Ou <oushixiong@kylinos.cn>
>
> Thanks and Regards,
> Shixiong Ou.
>

Hi Shixiong,
Thanks for your advice!
Actually, even if the calling of drmm_mode_config_init() failed, these mutexes are still initialized,
and it's the same for hibmc_kms_init() failed, so we don't need to change its return.

Thanks,
Baihan Li

>>   	}
>>   
>>   	ret = hibmc_kms_init(priv);

Re: [PATCH drm-dp 01/10] drm/hisilicon/hibmc: fix the i2c device resource leak when vdac init failed

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:23PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Currently the driver missed to clean the i2c adapter when vdac init failed.
> It may cause resource leak.
> 
> Fixes: 94ee73ee3020 ("drm/hisilicon/hibmc: add dp hw moduel in hibmc driver")

No, the tag is incorrect. Offending code was added in a different commit.

> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  | 1 +
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c  | 5 +++++
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 9 +++++++--
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index 274feabe7df0..ca8502e2760c 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -69,6 +69,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv);
>  int hibmc_vdac_init(struct hibmc_drm_private *priv);
>  
>  int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
> +void hibmc_ddc_del(struct hibmc_vdac *vdac);
>  
>  int hibmc_dp_init(struct hibmc_drm_private *priv);
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> index 99b3b77b5445..44860011855e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> @@ -95,3 +95,8 @@ int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *vdac)
>  
>  	return i2c_bit_add_bus(&vdac->adapter);
>  }
> +
> +void hibmc_ddc_del(struct hibmc_vdac *vdac)
> +{
> +	i2c_del_adapter(&vdac->adapter);
> +}

Then hibmc_connector_destroy() also needs to use this helper.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index e8a527ede854..36401b46034c 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -110,7 +110,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
>  	if (ret) {
>  		drm_err(dev, "failed to init encoder: %d\n", ret);
> -		return ret;
> +		goto err;
>  	}
>  
>  	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
> @@ -121,7 +121,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  					  &vdac->adapter);
>  	if (ret) {
>  		drm_err(dev, "failed to init connector: %d\n", ret);
> -		return ret;
> +		goto err;
>  	}
>  
>  	drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
> @@ -131,4 +131,9 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  
>  	return 0;
> +
> +err:
> +	hibmc_ddc_del(vdac);
> +
> +	return ret;
>  }
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:24PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@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 <libaihan@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?

> +		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.

> +		}
>  	} 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.

> -
>  	drm_dev_exit(idx);
>  
>  	return IRQ_HANDLED;
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 03/10] drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:25PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> The local variable of irq name is passed to devm_request_threaded_irq(),
> which will make request_irq failed. Using the global irq name instead
> of it to fix.

This doesn't explain, why does it fail and which IRQ name is actually
expected.

> 
> Fixes: b11bc1ae4658 ("drm/hisilicon/hibmc: Add MSI irq getting and requesting for HPD")
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 768b97f9e74a..4cdcc34070ee 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -32,7 +32,7 @@
>  
>  DEFINE_DRM_GEM_FOPS(hibmc_fops);
>  
> -static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "vblank", "hpd" };
> +static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "hibmc-vblank", "hibmc-hpd" };

Please point to the corresponding IRQ names as currently implemented in
the upstream kernel.

>  
>  static irqreturn_t hibmc_interrupt(int irq, void *arg)
>  {

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 05/10] drm/hisilicon/hibmc: fix rare monitors cannot display problem

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:27PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> DP Link training successful at 8.1Gbps with some monitors' max link rate
> are 2.7Gbps. So change the default 8.1Gbps link rate to the rate that reads
> from devices' capabilities.

I've hard time understanding this message.

> 
> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")

No, the tag is incorrect. Mentioned commit is not related.

> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  4 ++-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 +---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 +++++++++++++------
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 12 -------
>  4 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> index 676059d4c1e6..8191233aa965 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> @@ -57,15 +57,3 @@ int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
>  
>  	return 0;
>  }
> -
> -int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp)
> -{
> -	dp->serdes_base = dp->base + HIBMC_DP_HOST_OFFSET;
> -
> -	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
> -	       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET);
> -	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
> -	       dp->serdes_base + HIBMC_DP_PMA_LANE1_OFFSET);

Where did these two writes go?

> -
> -	return hibmc_dp_serdes_rate_switch(DP_SERDES_BW_8_1, dp);
> -}
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 06/10] drm/hisilicon/hibmc: add dp mode valid check

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:28PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> If DP is connected, add mode check and BW check in mode_valid_ctx() to
> ensure DP's cfg is usable.
> 
> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 10 ++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  7 +++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 59 +++++++++++++++++++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index ee0b543afd7f..4f93d60b932b 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -259,6 +259,16 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp)
>  	dp->dp_dev->link.status.channel_equalized = false;
>  }
>  
> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.link_rate;
> +}
> +
> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.lanes;
> +}
> +
>  static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>  	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>  	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> index 68867475508c..ebc7256ad006 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -12,6 +12,10 @@
>  #include <drm/drm_print.h>
>  #include <drm/display/drm_dp_helper.h>
>  
> +/* 27 * 10000000 * 80% = 216000000 */
> +#define DP_MODE_VALI_CAL	216000000
> +#define BPP_24				24
> +
>  struct hibmc_dp_dev;
>  
>  enum hibmc_dp_cbar_pattern {
> @@ -51,6 +55,7 @@ struct hibmc_dp {
>  	struct hibmc_dp_cbar_cfg cfg;
>  	u32 irq_status;
>  	int hpd_status;
> +	bool is_connected;
>  };
>  
>  int hibmc_dp_hw_init(struct hibmc_dp *dp);
> @@ -61,5 +66,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
>  void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>  void hibmc_dp_enable_int(struct hibmc_dp *dp);
>  void hibmc_dp_disable_int(struct hibmc_dp *dp);
> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index 191fb434baa7..e4b13f21ccb3 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -16,8 +16,31 @@
>  #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>  #define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
>  
> +struct hibmc_dp_disp_clk {
> +	u16 hdisplay;
> +	u16 vdisplay;
> +	u32 clock;
> +};
> +
> +static const struct hibmc_dp_disp_clk hibmc_dp_clk_table[] = {
> +	{640, 480, 25175}, /* 25175 khz */
> +	{800, 600, 40000}, /* 40000 khz */
> +	{1024, 768, 65000}, /* 65000 khz */
> +	{1152, 864, 80000}, /* 80000 khz */
> +	{1280, 768, 79500}, /* 79500 khz */
> +	{1280, 720, 74250}, /* 74250 khz */
> +	{1280, 960, 108000}, /* 108000 khz */
> +	{1280, 1024, 108000}, /* 108000 khz */
> +	{1440, 900, 106500}, /* 106500 khz */
> +	{1600, 900, 108000}, /* 108000 khz */
> +	{1600, 1200, 162000}, /* 162000 khz */
> +	{1920, 1080, 148500}, /* 148500 khz */
> +	{1920, 1200, 193250}, /* 193250 khz */
> +};
> +
>  static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>  {
> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>  	const struct drm_edid *drm_edid;
>  	int count;
>  
> @@ -27,6 +50,8 @@ static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>  
>  	count = drm_edid_connector_add_modes(connector);
>  
> +	dp->is_connected = !!count;
> +
>  	drm_edid_free(drm_edid);
>  
>  	return count;
> @@ -43,9 +68,43 @@ static int hibmc_dp_detect(struct drm_connector *connector,
>  		return connector_status_disconnected;
>  }
>  
> +static int hibmc_dp_mode_valid(struct drm_connector *connector,
> +			       const struct drm_display_mode *mode,
> +			       struct drm_modeset_acquire_ctx *ctx,
> +			       enum drm_mode_status *status)
> +{
> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
> +	u64 cur_val, max_val;
> +
> +	if (!dp->is_connected) {
> +		*status = MODE_OK;

No, mode_valid should not depend on DP being connected.

> +		return 0;
> +	}
> +
> +	/* check DP link BW */
> +	cur_val = (u64)mode->htotal * mode->vtotal * drm_mode_vrefresh(mode) * BPP_24;
> +	max_val = (u64)hibmc_dp_get_link_rate(dp) * DP_MODE_VALI_CAL * hibmc_dp_get_lanes(dp);
> +	if (cur_val > max_val)
> +		*status = MODE_CLOCK_HIGH;
> +	else
> +		*status = MODE_OK;
> +
> +	/* check the clock */
> +	for (size_t i = 0; i < ARRAY_SIZE(hibmc_dp_clk_table); i++) {
> +		if (hibmc_dp_clk_table[i].hdisplay == mode->hdisplay &&
> +		    hibmc_dp_clk_table[i].vdisplay == mode->vdisplay) {
> +			if (hibmc_dp_clk_table[i].clock != mode->clock)
> +				*status = MODE_CLOCK_RANGE;

Why?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>  	.get_modes = hibmc_dp_connector_get_modes,
>  	.detect_ctx = hibmc_dp_detect,
> +	.mode_valid_ctx = hibmc_dp_mode_valid,
>  };
>  
>  static int hibmc_dp_late_register(struct drm_connector *connector)
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 07/10] drm/hisilicon/hibmc: add dp encoder modeset

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:29PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Add GPU display control enable in dp_mode_set(), which is already
> in vdac's mode_set, however, if vdac is not connected, GPU
> cannot work.
> 
> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")

Typically, incorrect tag.

> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index e4b13f21ccb3..d9ae7567ebb7 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -11,6 +11,7 @@
>  #include <drm/drm_edid.h>
>  
>  #include "hibmc_drm_drv.h"
> +#include "hibmc_drm_regs.h"
>  #include "dp/dp_hw.h"
>  
>  #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
> @@ -169,9 +170,26 @@ static void hibmc_dp_encoder_disable(struct drm_encoder *drm_encoder,
>  	hibmc_dp_display_en(dp, false);
>  }
>  
> +static void hibmc_dp_encoder_mode_set(struct drm_encoder *encoder,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	u32 reg;
> +
> +	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> +	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
> +	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);

This is a c&p of the corresponding VDAC code. Please move it to a common
function instead.

BTW: what does it mean that the GPU cannot work? Do you mean that the
display hardware doesn't work or that the GL calls do not work?

> +}
> +
>  static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>  	.atomic_enable = hibmc_dp_encoder_enable,
>  	.atomic_disable = hibmc_dp_encoder_disable,
> +	.atomic_mode_set = hibmc_dp_encoder_mode_set,
>  };
>  
>  irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 08/10] drm/hisilicon/hibmc: fix DP no showing after HPD with VGA connected

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:30PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> If the system started with VGA connected, the desktop like GDM cannot get
> DP's CRTC when DP device is plugged in, because there is only one crtc
> sharing use of VGA and DP. So change VGA to disconnected when DP is
> connected.

NAK. I think we discussed this when the previous patch was submitted.

VGA and DP are independent. It should be user's choice whether to use
VGA or DP if both are connected.

> 
> Fixes: 4c962bc929f1 ("drm/hisilicon/hibmc: Add vga connector detect functions")
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 09/10] drm/hisilicon/hibmc: fix HPD no showing with VGA para of GRUB

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:31PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> In early OS versions， there is a bug in hibmc-drm driver previously,

Which OS? What does that mean? Why do we need to workaround userspace
issues in the kernel?

> so some OS add a VGA parameter in GRUB(video=VGA-1:640x480-32@60me) to
> fix the bug, that will config a force VGA mode to drm driver. However, the
> HPD problem exists that mentioned in previous patch, so change VGA's status
> in force() to compatible with some older OS versions.
> 
> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index 73dd3d5fc26c..d609ccda2f2a 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -61,6 +61,19 @@ static int hibmc_vdac_detect(struct drm_connector *connector, struct drm_modeset
>  	return connector_status_connected;
>  }
>  
> +static void hibmc_vdac_force(struct drm_connector *connector)
> +{
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(connector->dev);
> +	struct hibmc_dp *dp = &priv->dp;
> +
> +	if (dp->hpd_status) {
> +		connector->status = connector_status_disconnected;
> +		return;
> +	}
> +
> +	connector->status = connector_status_connected;

You are again trying to tie VGA and DP status, so NAK.

> +}
> +
>  static void hibmc_connector_destroy(struct drm_connector *connector)
>  {
>  	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
> @@ -81,6 +94,7 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>  	.reset = drm_atomic_helper_connector_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +	.force = hibmc_vdac_force,
>  };
>  
>  static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 10/10] drm/hisilicon/hibmc: fix no showing problem with loading hibmc manually

[Dmitry Baryshkov]

On Fri, May 30, 2025 at 05:54:32PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> When using command rmmod and insmod, there is no showing in second time
> insmoding. Because DP controller won't send HPD signals, if connection
> doesn't change or controller isn't reset. So add reset before unreset
> in hibmc_dp_hw_init().
> 
> Fixes: 94ee73ee3020 ("drm/hisilicon/hibmc: add dp hw moduel in hibmc driver")

Technically... yes and no. The function was written this way in that
commit, however HPD signals were not handled until the latter commit.

> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index 4f93d60b932b..e1b9589ce639 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -172,13 +172,15 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>  	dp_dev->link.cap.lanes = 0x2;
>  	dp_dev->link.cap.link_rate = DP_LINK_BW_8_1;
>  
> -	/* hdcp data */
> -	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
>  	/* int init */
>  	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>  	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>  	/* rst */
> +	writel(0, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
> +	usleep_range(30, 50);
>  	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
> +	/* hdcp data */
> +	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);

There are two independent changes here. Split them into two commits.

>  	/* clock enable */
>  	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
>  
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 01/10] drm/hisilicon/hibmc: fix the i2c device resource leak when vdac init failed

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:23PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Currently the driver missed to clean the i2c adapter when vdac init failed.
>> It may cause resource leak.
>>
>> Fixes: 94ee73ee3020 ("drm/hisilicon/hibmc: add dp hw moduel in hibmc driver")
> No, the tag is incorrect. Offending code was added in a different commit.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  | 1 +
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c  | 5 +++++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 9 +++++++--
>>   3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 274feabe7df0..ca8502e2760c 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -69,6 +69,7 @@ int hibmc_de_init(struct hibmc_drm_private *priv);
>>   int hibmc_vdac_init(struct hibmc_drm_private *priv);
>>   
>>   int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
>> +void hibmc_ddc_del(struct hibmc_vdac *vdac);
>>   
>>   int hibmc_dp_init(struct hibmc_drm_private *priv);
>>   
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
>> index 99b3b77b5445..44860011855e 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
>> @@ -95,3 +95,8 @@ int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *vdac)
>>   
>>   	return i2c_bit_add_bus(&vdac->adapter);
>>   }
>> +
>> +void hibmc_ddc_del(struct hibmc_vdac *vdac)
>> +{
>> +	i2c_del_adapter(&vdac->adapter);
>> +}
> Then hibmc_connector_destroy() also needs to use this helper.
>
Thanks for correcting, I'll fix them.
Baihan.


>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index e8a527ede854..36401b46034c 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -110,7 +110,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>   	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
>>   	if (ret) {
>>   		drm_err(dev, "failed to init encoder: %d\n", ret);
>> -		return ret;
>> +		goto err;
>>   	}
>>   
>>   	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>> @@ -121,7 +121,7 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>   					  &vdac->adapter);
>>   	if (ret) {
>>   		drm_err(dev, "failed to init connector: %d\n", ret);
>> -		return ret;
>> +		goto err;
>>   	}
>>   
>>   	drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
>> @@ -131,4 +131,9 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>   	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>>   
>>   	return 0;
>> +
>> +err:
>> +	hibmc_ddc_del(vdac);
>> +
>> +	return ret;
>>   }
>> -- 
>> 2.33.0
>>

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 <libaihan@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 <libaihan@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
>>

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

[Dmitry Baryshkov]

On 09/06/2025 17:47, Yongbang Shi wrote:
> 
>> On Fri, May 30, 2025 at 05:54:24PM +0800, Yongbang Shi wrote:
>>> From: Baihan Li <libaihan@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 <libaihan@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?
> 

No, I was thinking about drm_err_ratelimited() in case something gets 
stuck in the hw.

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 03/10] drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:25PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> The local variable of irq name is passed to devm_request_threaded_irq(),
>> which will make request_irq failed. Using the global irq name instead
>> of it to fix.
> This doesn't explain, why does it fail and which IRQ name is actually
> expected.

The local variable is passed in request_irq (), and there will be use after free problem.


>> Fixes: b11bc1ae4658 ("drm/hisilicon/hibmc: Add MSI irq getting and requesting for HPD")
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 768b97f9e74a..4cdcc34070ee 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -32,7 +32,7 @@
>>   
>>   DEFINE_DRM_GEM_FOPS(hibmc_fops);
>>   
>> -static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "vblank", "hpd" };
>> +static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "hibmc-vblank", "hibmc-hpd" };
> Please point to the corresponding IRQ names as currently implemented in
> the upstream kernel.

Ok.


>>   
>>   static irqreturn_t hibmc_interrupt(int irq, void *arg)
>>   {

Re: [PATCH drm-dp 03/10] drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local

[Dmitry Baryshkov]

On 09/06/2025 17:50, Yongbang Shi wrote:
> 
>> On Fri, May 30, 2025 at 05:54:25PM +0800, Yongbang Shi wrote:
>>> From: Baihan Li <libaihan@huawei.com>
>>>
>>> The local variable of irq name is passed to devm_request_threaded_irq(),
>>> which will make request_irq failed. Using the global irq name instead
>>> of it to fix.
>> This doesn't explain, why does it fail and which IRQ name is actually
>> expected.
> 
> The local variable is passed in request_irq (), and there will be use 
> after free problem.

This needs to be explained (in details) in the commit message.

> 
> 
>>> Fixes: b11bc1ae4658 ("drm/hisilicon/hibmc: Add MSI irq getting and 
>>> requesting for HPD")
>>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>>> ---
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 10 +++-------
>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/ 
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index 768b97f9e74a..4cdcc34070ee 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -32,7 +32,7 @@
>>>   DEFINE_DRM_GEM_FOPS(hibmc_fops);
>>> -static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "vblank", 
>>> "hpd" };
>>> +static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "hibmc- 
>>> vblank", "hibmc-hpd" };
>> Please point to the corresponding IRQ names as currently implemented in
>> the upstream kernel.
> 
> Ok.

I was thinking in terms of IRQ lookup. You can ignore this comment (it 
makes me wonder, how did you understand it, if you responded with Ok).

> 
> 
>>>   static irqreturn_t hibmc_interrupt(int irq, void *arg)
>>>   {


-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 05/10] drm/hisilicon/hibmc: fix rare monitors cannot display problem

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:27PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> DP Link training successful at 8.1Gbps with some monitors' max link rate
>> are 2.7Gbps. So change the default 8.1Gbps link rate to the rate that reads
>> from devices' capabilities.
> I've hard time understanding this message.

Sorry for misunderstanding. The problem is that dp link training success at 8.1Gbps, however,

the sink 's maximum supported rate is less than 8.1G.


>> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
> No, the tag is incorrect. Mentioned commit is not related.

Ok.


>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  4 ++-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 +---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 +++++++++++++------
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 12 -------
>>   4 files changed, 27 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>> index 676059d4c1e6..8191233aa965 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>> @@ -57,15 +57,3 @@ int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
>>   
>>   	return 0;
>>   }
>> -
>> -int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp)
>> -{
>> -	dp->serdes_base = dp->base + HIBMC_DP_HOST_OFFSET;
>> -
>> -	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
>> -	       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET);
>> -	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
>> -	       dp->serdes_base + HIBMC_DP_PMA_LANE1_OFFSET);
> Where did these two writes go?

It's the same as the cfg in hibmc_dp_serdes_set_tx_cfg(), and this function will be called certainly.


>> -
>> -	return hibmc_dp_serdes_rate_switch(DP_SERDES_BW_8_1, dp);
>> -}
>> -- 
>> 2.33.0
>>

Re: [PATCH drm-dp 06/10] drm/hisilicon/hibmc: add dp mode valid check

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:28PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> If DP is connected, add mode check and BW check in mode_valid_ctx() to
>> ensure DP's cfg is usable.
>>
>> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 10 ++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  7 +++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 59 +++++++++++++++++++
>>   3 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index ee0b543afd7f..4f93d60b932b 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -259,6 +259,16 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp)
>>   	dp->dp_dev->link.status.channel_equalized = false;
>>   }
>>   
>> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
>> +{
>> +	return dp->dp_dev->link.cap.link_rate;
>> +}
>> +
>> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
>> +{
>> +	return dp->dp_dev->link.cap.lanes;
>> +}
>> +
>>   static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>>   	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>>   	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 68867475508c..ebc7256ad006 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -12,6 +12,10 @@
>>   #include <drm/drm_print.h>
>>   #include <drm/display/drm_dp_helper.h>
>>   
>> +/* 27 * 10000000 * 80% = 216000000 */
>> +#define DP_MODE_VALI_CAL	216000000
>> +#define BPP_24				24
>> +
>>   struct hibmc_dp_dev;
>>   
>>   enum hibmc_dp_cbar_pattern {
>> @@ -51,6 +55,7 @@ struct hibmc_dp {
>>   	struct hibmc_dp_cbar_cfg cfg;
>>   	u32 irq_status;
>>   	int hpd_status;
>> +	bool is_connected;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> @@ -61,5 +66,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
>>   void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>>   void hibmc_dp_enable_int(struct hibmc_dp *dp);
>>   void hibmc_dp_disable_int(struct hibmc_dp *dp);
>> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
>> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index 191fb434baa7..e4b13f21ccb3 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -16,8 +16,31 @@
>>   #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>>   #define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
>>   
>> +struct hibmc_dp_disp_clk {
>> +	u16 hdisplay;
>> +	u16 vdisplay;
>> +	u32 clock;
>> +};
>> +
>> +static const struct hibmc_dp_disp_clk hibmc_dp_clk_table[] = {
>> +	{640, 480, 25175}, /* 25175 khz */
>> +	{800, 600, 40000}, /* 40000 khz */
>> +	{1024, 768, 65000}, /* 65000 khz */
>> +	{1152, 864, 80000}, /* 80000 khz */
>> +	{1280, 768, 79500}, /* 79500 khz */
>> +	{1280, 720, 74250}, /* 74250 khz */
>> +	{1280, 960, 108000}, /* 108000 khz */
>> +	{1280, 1024, 108000}, /* 108000 khz */
>> +	{1440, 900, 106500}, /* 106500 khz */
>> +	{1600, 900, 108000}, /* 108000 khz */
>> +	{1600, 1200, 162000}, /* 162000 khz */
>> +	{1920, 1080, 148500}, /* 148500 khz */
>> +	{1920, 1200, 193250}, /* 193250 khz */
>> +};
>> +
>>   static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>   {
>> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>>   	const struct drm_edid *drm_edid;
>>   	int count;
>>   
>> @@ -27,6 +50,8 @@ static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>   
>>   	count = drm_edid_connector_add_modes(connector);
>>   
>> +	dp->is_connected = !!count;
>> +
>>   	drm_edid_free(drm_edid);
>>   
>>   	return count;
>> @@ -43,9 +68,43 @@ static int hibmc_dp_detect(struct drm_connector *connector,
>>   		return connector_status_disconnected;
>>   }
>>   
>> +static int hibmc_dp_mode_valid(struct drm_connector *connector,
>> +			       const struct drm_display_mode *mode,
>> +			       struct drm_modeset_acquire_ctx *ctx,
>> +			       enum drm_mode_status *status)
>> +{
>> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>> +	u64 cur_val, max_val;
>> +
>> +	if (!dp->is_connected) {
>> +		*status = MODE_OK;
> No, mode_valid should not depend on DP being connected.

Okay!


>> +		return 0;
>> +	}
>> +
>> +	/* check DP link BW */
>> +	cur_val = (u64)mode->htotal * mode->vtotal * drm_mode_vrefresh(mode) * BPP_24;
>> +	max_val = (u64)hibmc_dp_get_link_rate(dp) * DP_MODE_VALI_CAL * hibmc_dp_get_lanes(dp);
>> +	if (cur_val > max_val)
>> +		*status = MODE_CLOCK_HIGH;
>> +	else
>> +		*status = MODE_OK;
>> +
>> +	/* check the clock */
>> +	for (size_t i = 0; i < ARRAY_SIZE(hibmc_dp_clk_table); i++) {
>> +		if (hibmc_dp_clk_table[i].hdisplay == mode->hdisplay &&
>> +		    hibmc_dp_clk_table[i].vdisplay == mode->vdisplay) {
>> +			if (hibmc_dp_clk_table[i].clock != mode->clock)
>> +				*status = MODE_CLOCK_RANGE;
> Why?

Because the clock will be different in different standards, but our GPU cannot generate these clock frequencies.


>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>>   	.get_modes = hibmc_dp_connector_get_modes,
>>   	.detect_ctx = hibmc_dp_detect,
>> +	.mode_valid_ctx = hibmc_dp_mode_valid,
>>   };
>>   
>>   static int hibmc_dp_late_register(struct drm_connector *connector)
>> -- 
>> 2.33.0
>>

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

[Dmitry Baryshkov]

On Mon, Jun 09, 2025 at 10:47:50PM +0800, Yongbang Shi wrote:
> 
> > On Fri, May 30, 2025 at 05:54:24PM +0800, Yongbang Shi wrote:
> > > From: Baihan Li <libaihan@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 <libaihan@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.

I don't think HPD pulse needs to be enabled specially. It is documented
as IRQ that Source needs to respond to by rereading DPCD Link / Sink
Status bits and acting accordingly.

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 05/10] drm/hisilicon/hibmc: fix rare monitors cannot display problem

[Dmitry Baryshkov]

On Mon, Jun 09, 2025 at 10:56:09PM +0800, Yongbang Shi wrote:
> 
> > On Fri, May 30, 2025 at 05:54:27PM +0800, Yongbang Shi wrote:
> > > From: Baihan Li <libaihan@huawei.com>
> > > 
> > > DP Link training successful at 8.1Gbps with some monitors' max link rate
> > > are 2.7Gbps. So change the default 8.1Gbps link rate to the rate that reads
> > > from devices' capabilities.
> > I've hard time understanding this message.
> 
> Sorry for misunderstanding. The problem is that dp link training success at 8.1Gbps, however,
> 
> the sink 's maximum supported rate is less than 8.1G.

okay, this is better. Please update the commit message.

> 
> 
> > > Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
> > No, the tag is incorrect. Mentioned commit is not related.
> 
> Ok.
> 
> 
> > > Signed-off-by: Baihan Li <libaihan@huawei.com>
> > > ---
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  4 ++-
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 +---
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 +++++++++++++------
> > >   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 12 -------
> > >   4 files changed, 27 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> > > index 676059d4c1e6..8191233aa965 100644
> > > --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> > > +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> > > @@ -57,15 +57,3 @@ int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
> > >   	return 0;
> > >   }
> > > -
> > > -int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp)
> > > -{
> > > -	dp->serdes_base = dp->base + HIBMC_DP_HOST_OFFSET;
> > > -
> > > -	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
> > > -	       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET);
> > > -	writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, DP_SERDES_VOL0_PRE0),
> > > -	       dp->serdes_base + HIBMC_DP_PMA_LANE1_OFFSET);
> > Where did these two writes go?
> 
> It's the same as the cfg in hibmc_dp_serdes_set_tx_cfg(), and this function will be called certainly.

ack.

> 
> 
> > > -
> > > -	return hibmc_dp_serdes_rate_switch(DP_SERDES_BW_8_1, dp);
> > > -}
> > > -- 
> > > 2.33.0
> > > 

-- 
With best wishes
Dmitry

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

[Yongbang Shi]

>>
>>
>>>> +        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?
>>
>
> No, I was thinking about drm_err_ratelimited() in case something gets 
> stuck in the hw.
>
Okay, I will try to use it.

Re: [PATCH drm-dp 03/10] drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local

[Yongbang Shi]

> On 09/06/2025 17:50, Yongbang Shi wrote:
>>
>>> On Fri, May 30, 2025 at 05:54:25PM +0800, Yongbang Shi wrote:
>>>> From: Baihan Li <libaihan@huawei.com>
>>>>
>>>> The local variable of irq name is passed to 
>>>> devm_request_threaded_irq(),
>>>> which will make request_irq failed. Using the global irq name instead
>>>> of it to fix.
>>> This doesn't explain, why does it fail and which IRQ name is actually
>>> expected.
>>
>> The local variable is passed in request_irq (), and there will be use 
>> after free problem.
>
> This needs to be explained (in details) in the commit message.

Okay.

>
>>
>>
>>>> Fixes: b11bc1ae4658 ("drm/hisilicon/hibmc: Add MSI irq getting and 
>>>> requesting for HPD")
>>>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>>>> ---
>>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 10 +++-------
>>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/ 
>>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>>> index 768b97f9e74a..4cdcc34070ee 100644
>>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>>> @@ -32,7 +32,7 @@
>>>>   DEFINE_DRM_GEM_FOPS(hibmc_fops);
>>>> -static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { 
>>>> "vblank", "hpd" };
>>>> +static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "hibmc- 
>>>> vblank", "hibmc-hpd" };
>>> Please point to the corresponding IRQ names as currently implemented in
>>> the upstream kernel.
>>
>> Ok.
>
> I was thinking in terms of IRQ lookup. You can ignore this comment (it 
> makes me wonder, how did you understand it, if you responded with Ok).
>
I thought you were suggesting naming the irq referring to the previous name.


>>
>>
>>>>   static irqreturn_t hibmc_interrupt(int irq, void *arg)
>>>>   {
>
>

Re: [PATCH drm-dp 07/10] drm/hisilicon/hibmc: add dp encoder modeset

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:29PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Add GPU display control enable in dp_mode_set(), which is already
>> in vdac's mode_set, however, if vdac is not connected, GPU
>> cannot work.
>>
>> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
> Typically, incorrect tag.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index e4b13f21ccb3..d9ae7567ebb7 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -11,6 +11,7 @@
>>   #include <drm/drm_edid.h>
>>   
>>   #include "hibmc_drm_drv.h"
>> +#include "hibmc_drm_regs.h"
>>   #include "dp/dp_hw.h"
>>   
>>   #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>> @@ -169,9 +170,26 @@ static void hibmc_dp_encoder_disable(struct drm_encoder *drm_encoder,
>>   	hibmc_dp_display_en(dp, false);
>>   }
>>   
>> +static void hibmc_dp_encoder_mode_set(struct drm_encoder *encoder,
>> +				      struct drm_crtc_state *crtc_state,
>> +				      struct drm_connector_state *conn_state)
>> +{
>> +	struct drm_device *dev = encoder->dev;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	u32 reg;
>> +
>> +	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>> +	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
>> +	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
>> +	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
>> +	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
>> +	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> This is a c&p of the corresponding VDAC code. Please move it to a common
> function instead.
>
> BTW: what does it mean that the GPU cannot work? Do you mean that the
> display hardware doesn't work or that the GL calls do not work?

Okay, I will delete it here.

It's about hardware, which may cause no signal.


>> +}
>> +
>>   static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>>   	.atomic_enable = hibmc_dp_encoder_enable,
>>   	.atomic_disable = hibmc_dp_encoder_disable,
>> +	.atomic_mode_set = hibmc_dp_encoder_mode_set,
>>   };
>>   
>>   irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
>> -- 
>> 2.33.0
>>

Re: [PATCH drm-dp 08/10] drm/hisilicon/hibmc: fix DP no showing after HPD with VGA connected

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:30PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> If the system started with VGA connected, the desktop like GDM cannot get
>> DP's CRTC when DP device is plugged in, because there is only one crtc
>> sharing use of VGA and DP. So change VGA to disconnected when DP is
>> connected.
> NAK. I think we discussed this when the previous patch was submitted.
>
> VGA and DP are independent. It should be user's choice whether to use
> VGA or DP if both are connected.

Okay, I can use GDM to set which connectors to use, or config Xorg conf file.
But I have an another problem, I think our driver only support one of them displaying, can we realize
the clone displaying at the same time? To make DP and VGA shows up simultaneous when DP is plugged in,
If the system started with VGA connected.

Thanks,
Baihan.


>> Fixes: 4c962bc929f1 ("drm/hisilicon/hibmc: Add vga connector detect functions")
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)

Re: [PATCH drm-dp 08/10] drm/hisilicon/hibmc: fix DP no showing after HPD with VGA connected

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:30PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> If the system started with VGA connected, the desktop like GDM cannot get
>> DP's CRTC when DP device is plugged in, because there is only one crtc
>> sharing use of VGA and DP. So change VGA to disconnected when DP is
>> connected.
> NAK. I think we discussed this when the previous patch was submitted.
>
> VGA and DP are independent. It should be user's choice whether to use
> VGA or DP if both are connected.

Acutally, although we are developing drm drivers, our requirements
and testing department's goal is to displaying desktop systems(like GDM), so we have to
solve different OS or display managers problems, and let the driver handle more situations.


>> Fixes: 4c962bc929f1 ("drm/hisilicon/hibmc: Add vga connector detect functions")
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)

Re: [PATCH drm-dp 10/10] drm/hisilicon/hibmc: fix no showing problem with loading hibmc manually

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:32PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> When using command rmmod and insmod, there is no showing in second time
>> insmoding. Because DP controller won't send HPD signals, if connection
>> doesn't change or controller isn't reset. So add reset before unreset
>> in hibmc_dp_hw_init().
>>
>> Fixes: 94ee73ee3020 ("drm/hisilicon/hibmc: add dp hw moduel in hibmc driver")
> Technically... yes and no. The function was written this way in that
> commit, however HPD signals were not handled until the latter commit.

Alright, can I put 2 commits here?


>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index 4f93d60b932b..e1b9589ce639 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -172,13 +172,15 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   	dp_dev->link.cap.lanes = 0x2;
>>   	dp_dev->link.cap.link_rate = DP_LINK_BW_8_1;
>>   
>> -	/* hdcp data */
>> -	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
>>   	/* int init */
>>   	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>>   	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>>   	/* rst */
>> +	writel(0, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> +	usleep_range(30, 50);
>>   	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> +	/* hdcp data */
>> +	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
> There are two independent changes here. Split them into two commits.

Okay!


>>   	/* clock enable */
>>   	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
>>   
>> -- 
>> 2.33.0
>>

Re: [PATCH drm-dp 09/10] drm/hisilicon/hibmc: fix HPD no showing with VGA para of GRUB

[Yongbang Shi]

> On Fri, May 30, 2025 at 05:54:31PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> In early OS versions， there is a bug in hibmc-drm driver previously,
> Which OS? What does that mean? Why do we need to workaround userspace
> issues in the kernel?

We use OpenEuler 22.03, there is a VGA cfg(video=VGA-1:640x480-32@60me) in GRUB args.
If it exists, it will affect DP HPD.


>> so some OS add a VGA parameter in GRUB(video=VGA-1:640x480-32@60me) to
>> fix the bug, that will config a force VGA mode to drm driver. However, the
>> HPD problem exists that mentioned in previous patch, so change VGA's status
>> in force() to compatible with some older OS versions.
>>
>> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 73dd3d5fc26c..d609ccda2f2a 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -61,6 +61,19 @@ static int hibmc_vdac_detect(struct drm_connector *connector, struct drm_modeset
>>   	return connector_status_connected;
>>   }
>>   
>> +static void hibmc_vdac_force(struct drm_connector *connector)
>> +{
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(connector->dev);
>> +	struct hibmc_dp *dp = &priv->dp;
>> +
>> +	if (dp->hpd_status) {
>> +		connector->status = connector_status_disconnected;
>> +		return;
>> +	}
>> +
>> +	connector->status = connector_status_connected;
> You are again trying to tie VGA and DP status, so NAK.
>
>> +}
>> +
>>   static void hibmc_connector_destroy(struct drm_connector *connector)
>>   {
>>   	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>> @@ -81,6 +94,7 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>>   	.reset = drm_atomic_helper_connector_reset,
>>   	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +	.force = hibmc_vdac_force,
>>   };
>>   
>>   static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
>> -- 
>> 2.33.0
>>

Re: [PATCH drm-dp 03/10] drm/hisilicon/hibmc: fix irq_request()'s irq name variable is local

[Zenghui Yu]

On 2025/6/9 22:53, Dmitry Baryshkov wrote:
> On 09/06/2025 17:50, Yongbang Shi wrote:
> >
> > > On Fri, May 30, 2025 at 05:54:25PM +0800, Yongbang Shi wrote:
> > > > From: Baihan Li <libaihan@huawei.com>
> > > >
> > > > The local variable of irq name is passed to
> > > > devm_request_threaded_irq(),
> > > > which will make request_irq failed. Using the global irq name instead
> > > > of it to fix.
> > > This doesn't explain, why does it fail and which IRQ name is actually
> > > expected.
> >
> > The local variable is passed in request_irq (), and there will be use
> > after free problem.
> 
> This needs to be explained (in details) in the commit message.

+1. And I hope this can be fixed as soon as possible. I've run into
several OOPS with that.

Thanks,
Zenghui