[PATCH v3 drm-dp 0/8] Add HPD, getting EDID, colorbar features in DP function

[PATCH v3 drm-dp 0/8] Add HPD, getting EDID, colorbar features in DP function

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

To support DP HPD, edid printing, and colorbar display features based on
the Hisislcon DP devices. 

---
ChangeLog:
v2 -> v3:
  - restructuring the header p_reg.h, suggested by Dmitry Baryshkov.
  - add commit log about dp serdes, suggested by Dmitry Baryshkov.
  - return value in hibmc_dp_serdes_init(), suggested by Dmitry Baryshkov.
  - add static const in the array of serdes_tx_cfg[], suggested by Dmitry Baryshkov.
  - change drm_warn to drm_dbg_dp, suggested by Dmitry Baryshkov.
  - add explanations about dp serdes macros, suggested by Dmitry Baryshkov.
  - change commit to an imperative sentence, suggested by Dmitry Baryshkov.
  - put HIBMC_DP_HOST_SERDES_CTRL in dp_serdes.h, suggested by Dmitry Baryshkov.
  - split the patch into two parts, suggested by Dmitry Baryshkov.
  - Capitalized EDID and AUX, suggested by Dmitry Baryshkov.
  - rewrite the commit log, suggested by Dmitry Baryshkov.
  - move colorbar debugfs entry to this patch, suggested by Dmitry Baryshkov.
  - change binary format to integer format, suggested by Dmitry Baryshkov.
  - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
  - remove enble_display in ISR, suggested by Dmitry Baryshkov.
  - change drm_kms_helper_connector_hotplug_event() to
    drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
  - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
  - remove struct irqs, suggested by Dmitry Baryshkov.
  - split this patch into two parts, suggested by Dmitry Baryshkov.
v1 -> v2:
  - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
  - changing all names of dp phy to dp serdes.
  - deleting type conversion, suggested by Dmitry Baryshkov.
  - deleting hibmc_dp_connector_get_modes() and using drm_connector_helper_get_modes(), suggested by Dmitry Baryshkov.
  - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
  - deleting edid decoder and its debugfs, suggested by Dmitry Baryshkov.
  - using debugfs_init() callback, suggested by Dmitry Baryshkov.
  - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
  - optimizing the description in commit message, suggested by Dmitry Baryshkov.
  - add mdelay(100) comments, suggested by Dmitry Baryshkov.
  - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
---

Baihan Li (8):
  drm/hisilicon/hibmc: Restructuring the header dp_reg.h
  drm/hisilicon/hibmc: Add dp phy cfg to adjust serdes rate, voltage and
    pre-emphasis
  drm/hisilicon/hibmc: Add dp serdes cfg in dp process
  drm/hisilicon/hibmc: Refactor the member of drm_aux in struct hibmc_dp
  drm/hisilicon/hibmc: Getting connector info and edid by using aux
    channel
  drm/hisilicon/hibmc: Add colorbar-cfg feature and its debugfs file
  drm/hisilicon/hibmc: Enable this hot plug detect of irq feature
  drm/hisilicon/hibmc: Add MSI irq getting and requesting for HPD

 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   |  16 ++-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |   7 +-
 .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |   2 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  78 ++++++++++++-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  37 +++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  |  55 +++++++---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   | 103 +++++++++++-------
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    |  72 ++++++++++++
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  40 +++++++
 .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 100 +++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |  95 +++++++++++++++-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  85 +++++++++++----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  12 ++
 14 files changed, 617 insertions(+), 88 deletions(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c

-- 
2.33.0

[PATCH v3 drm-dp 1/8] drm/hisilicon/hibmc: Restructuring the header dp_reg.h

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Move the macros below their corresponding registers to make
them more obvious.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - restructuring the header dp_reg.h, suggested by Dmitry Baryshkov.
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h | 98 +++++++++++++--------
 1 file changed, 60 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index 4a515c726d52..dc2bd3f80b70 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -5,72 +5,94 @@
 #define DP_REG_H
 
 #define HIBMC_DP_AUX_CMD_ADDR			0x50
+
 #define HIBMC_DP_AUX_WR_DATA0			0x54
 #define HIBMC_DP_AUX_WR_DATA1			0x58
 #define HIBMC_DP_AUX_WR_DATA2			0x5c
 #define HIBMC_DP_AUX_WR_DATA3			0x60
 #define HIBMC_DP_AUX_RD_DATA0			0x64
+
 #define HIBMC_DP_AUX_REQ			0x74
+#define HIBMC_DP_CFG_AUX_REQ			BIT(0)
+#define HIBMC_DP_CFG_AUX_SYNC_LEN_SEL		BIT(1)
+#define HIBMC_DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
+#define HIBMC_DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
+
 #define HIBMC_DP_AUX_STATUS			0x78
+#define HIBMC_DP_CFG_AUX_TIMEOUT		BIT(0)
+#define HIBMC_DP_CFG_AUX_STATUS			GENMASK(11, 4)
+#define HIBMC_DP_CFG_AUX_READY_DATA_BYTE	GENMASK(16, 12)
+#define HIBMC_DP_CFG_AUX			GENMASK(24, 17)
+
 #define HIBMC_DP_PHYIF_CTRL0			0xa0
+#define HIBMC_DP_CFG_SCRAMBLE_EN		BIT(0)
+#define HIBMC_DP_CFG_PAT_SEL			GENMASK(7, 4)
+#define HIBMC_DP_CFG_LANE_DATA_EN		GENMASK(11, 8)
+
 #define HIBMC_DP_VIDEO_CTRL			0x100
+#define HIBMC_DP_CFG_STREAM_RGB_ENABLE		BIT(1)
+#define HIBMC_DP_CFG_STREAM_VIDEO_MAPPING	GENMASK(5, 2)
+#define HIBMC_DP_CFG_STREAM_FRAME_MODE		BIT(6)
+#define HIBMC_DP_CFG_STREAM_HSYNC_POLARITY	BIT(7)
+#define HIBMC_DP_CFG_STREAM_VSYNC_POLARITY	BIT(8)
+
 #define HIBMC_DP_VIDEO_CONFIG0			0x104
+#define HIBMC_DP_CFG_STREAM_HACTIVE		GENMASK(31, 16)
+#define HIBMC_DP_CFG_STREAM_HBLANK		GENMASK(15, 0)
+
 #define HIBMC_DP_VIDEO_CONFIG1			0x108
+#define HIBMC_DP_CFG_STREAM_VACTIVE		GENMASK(31, 16)
+#define HIBMC_DP_CFG_STREAM_VBLANK		GENMASK(15, 0)
+
 #define HIBMC_DP_VIDEO_CONFIG2			0x10c
+#define HIBMC_DP_CFG_STREAM_HSYNC_WIDTH		GENMASK(15, 0)
+
 #define HIBMC_DP_VIDEO_CONFIG3			0x110
+#define HIBMC_DP_CFG_STREAM_VSYNC_WIDTH		GENMASK(15, 0)
+#define HIBMC_DP_CFG_STREAM_VFRONT_PORCH	GENMASK(31, 16)
+
 #define HIBMC_DP_VIDEO_PACKET			0x114
+#define HIBMC_DP_CFG_STREAM_TU_SYMBOL_SIZE	GENMASK(5, 0)
+#define HIBMC_DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE	GENMASK(9, 6)
+
 #define HIBMC_DP_VIDEO_MSA0			0x118
+#define HIBMC_DP_CFG_STREAM_VSTART		GENMASK(31, 16)
+#define HIBMC_DP_CFG_STREAM_HSTART		GENMASK(15, 0)
+
 #define HIBMC_DP_VIDEO_MSA1			0x11c
 #define HIBMC_DP_VIDEO_MSA2			0x120
+
 #define HIBMC_DP_VIDEO_HORIZONTAL_SIZE		0X124
+#define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
+#define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
+
 #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
+#define HIBMC_DP_CFG_TIMING_GEN0_HACTIVE	GENMASK(31, 16)
+#define HIBMC_DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
+
 #define HIBMC_DP_TIMING_GEN_CONFIG2		0x274
+#define HIBMC_DP_CFG_TIMING_GEN0_VACTIVE	GENMASK(31, 16)
+#define HIBMC_DP_CFG_TIMING_GEN0_VBLANK		GENMASK(15, 0)
+
 #define HIBMC_DP_TIMING_GEN_CONFIG3		0x278
+#define HIBMC_DP_CFG_TIMING_GEN0_VFRONT_PORCH	GENMASK(31, 16)
+
 #define HIBMC_DP_HDCP_CFG			0x600
+
 #define HIBMC_DP_DPTX_RST_CTRL			0x700
+#define HIBMC_DP_CFG_AUX_RST_N			BIT(4)
+
 #define HIBMC_DP_DPTX_CLK_CTRL			0x704
+
 #define HIBMC_DP_DPTX_GCTL0			0x708
+#define HIBMC_DP_CFG_PHY_LANE_NUM		GENMASK(2, 1)
+
 #define HIBMC_DP_INTR_ENABLE			0x720
 #define HIBMC_DP_INTR_ORIGINAL_STATUS		0x728
-#define HIBMC_DP_TIMING_MODEL_CTRL		0x884
-#define HIBMC_DP_TIMING_SYNC_CTRL		0xFF0
 
-#define HIBMC_DP_CFG_AUX_SYNC_LEN_SEL		BIT(1)
-#define HIBMC_DP_CFG_AUX_TIMER_TIMEOUT		BIT(2)
-#define HIBMC_DP_CFG_STREAM_FRAME_MODE		BIT(6)
-#define HIBMC_DP_CFG_AUX_MIN_PULSE_NUM		GENMASK(13, 9)
-#define HIBMC_DP_CFG_LANE_DATA_EN		GENMASK(11, 8)
-#define HIBMC_DP_CFG_PHY_LANE_NUM		GENMASK(2, 1)
-#define HIBMC_DP_CFG_AUX_REQ			BIT(0)
-#define HIBMC_DP_CFG_AUX_RST_N			BIT(4)
-#define HIBMC_DP_CFG_AUX_TIMEOUT		BIT(0)
-#define HIBMC_DP_CFG_AUX_READY_DATA_BYTE	GENMASK(16, 12)
-#define HIBMC_DP_CFG_AUX			GENMASK(24, 17)
-#define HIBMC_DP_CFG_AUX_STATUS			GENMASK(11, 4)
-#define HIBMC_DP_CFG_SCRAMBLE_EN		BIT(0)
-#define HIBMC_DP_CFG_PAT_SEL			GENMASK(7, 4)
-#define HIBMC_DP_CFG_TIMING_GEN0_HACTIVE	GENMASK(31, 16)
-#define HIBMC_DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
-#define HIBMC_DP_CFG_TIMING_GEN0_VACTIVE	GENMASK(31, 16)
-#define HIBMC_DP_CFG_TIMING_GEN0_VBLANK		GENMASK(15, 0)
-#define HIBMC_DP_CFG_TIMING_GEN0_VFRONT_PORCH	GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_HACTIVE		GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_HBLANK		GENMASK(15, 0)
-#define HIBMC_DP_CFG_STREAM_HSYNC_WIDTH		GENMASK(15, 0)
-#define HIBMC_DP_CFG_STREAM_VACTIVE		GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_VBLANK		GENMASK(15, 0)
-#define HIBMC_DP_CFG_STREAM_VFRONT_PORCH	GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_VSYNC_WIDTH		GENMASK(15, 0)
-#define HIBMC_DP_CFG_STREAM_VSTART		GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_HSTART		GENMASK(15, 0)
-#define HIBMC_DP_CFG_STREAM_VSYNC_POLARITY	BIT(8)
-#define HIBMC_DP_CFG_STREAM_HSYNC_POLARITY	BIT(7)
-#define HIBMC_DP_CFG_STREAM_RGB_ENABLE		BIT(1)
-#define HIBMC_DP_CFG_STREAM_VIDEO_MAPPING	GENMASK(5, 2)
+#define HIBMC_DP_TIMING_MODEL_CTRL		0x884
 #define HIBMC_DP_CFG_PIXEL_NUM_TIMING_MODE_SEL1	GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_TU_SYMBOL_SIZE	GENMASK(5, 0)
-#define HIBMC_DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE	GENMASK(9, 6)
-#define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
-#define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
+
+#define HIBMC_DP_TIMING_SYNC_CTRL		0xFF0
 
 #endif
-- 
2.33.0

Re: [PATCH v3 drm-dp 1/8] drm/hisilicon/hibmc: Restructuring the header dp_reg.h

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:50:54AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Move the macros below their corresponding registers to make
> them more obvious.
> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - restructuring the header dp_reg.h, suggested by Dmitry Baryshkov.
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h | 98 +++++++++++++--------
>  1 file changed, 60 insertions(+), 38 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH v3 drm-dp 2/8] drm/hisilicon/hibmc: Add dp serdes cfg to adjust serdes rate, voltage and pre-emphasis

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

This dp controller need features of digital-to-analog conversion and
high-speed transmission in chip by its extern serdes controller. Our
serdes cfg is relatively simple, just need two register configurations.
Don't need too much functions, like: power on/off, initialize, and some
complex configurations, so I'm not going to use the phy framework.
This serdes is inited and configured in dp initialization, and also
integrating them into link training process.

For rate changing, we can change from 1.62-8.2Gpbs by cfg reg.
For voltage and pre-emphasis levels changing, we can cfg different
serdes ffe value.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 - > v3:
  - add commit log about dp serdes, suggested by Dmitry Baryshkov.
  - return value in hibmc_dp_serdes_init(), suggested by Dmitry Baryshkov.
  - add static const in the array of serdes_tx_cfg[], suggested by Dmitry Baryshkov.
  - change drm_warn to drm_dbg_dp, suggested by Dmitry Baryshkov.
  - add explanations about dp serdes macros, suggested by Dmitry Baryshkov.
v1 -> v2:
  - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
  - changing all names of dp phy to dp serdes.
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile      |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 ++
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 72 +++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    | 34 +++++++++
 5 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index 95a4ed599d98..43de077d6769 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
-	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
+	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index 2c52a4476c4d..e7746bc4b592 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -38,6 +38,7 @@ struct hibmc_dp_dev {
 	struct mutex lock; /* protects concurrent RW in hibmc_dp_reg_write_field() */
 	struct hibmc_dp_link link;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
+	void __iomem *serdes_base;
 };
 
 #define dp_field_modify(reg_value, mask, val)				\
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index a8d543881c09..59078483ec55 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -7,6 +7,7 @@
 #include "dp_comm.h"
 #include "dp_reg.h"
 #include "dp_hw.h"
+#include "dp_serdes.h"
 
 static void hibmc_dp_set_tu(struct hibmc_dp_dev *dp, struct drm_display_mode *mode)
 {
@@ -151,6 +152,7 @@ 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)
@@ -165,6 +167,10 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 
 	hibmc_dp_aux_init(dp_dev);
 
+	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_2_7;
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
new file mode 100644
index 000000000000..241b9ef782b0
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2025 Hisilicon Limited.
+
+#include <linux/delay.h>
+#include <drm/drm_device.h>
+#include <drm/drm_print.h>
+#include "dp_comm.h"
+#include "dp_config.h"
+#include "dp_reg.h"
+#include "dp_serdes.h"
+
+int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX])
+{
+	static const u32 serdes_tx_cfg[4][4] = { {DP_SERDES_VOL0_PRE0, DP_SERDES_VOL0_PRE1,
+						  DP_SERDES_VOL0_PRE2, DP_SERDES_VOL0_PRE3},
+						 {DP_SERDES_VOL1_PRE0, DP_SERDES_VOL1_PRE1,
+						  DP_SERDES_VOL1_PRE2}, {DP_SERDES_VOL2_PRE0,
+						  DP_SERDES_VOL2_PRE1}, {DP_SERDES_VOL3_PRE0}};
+	int cfg[2];
+	int i;
+
+	for (i = 0; i < HIBMC_DP_LANE_NUM_MAX; i++) {
+		cfg[i] = serdes_tx_cfg[(train_set[i] & 0x3)]
+			 [(train_set[i] << DP_TRAIN_PRE_EMPHASIS_SHIFT & 0x3)];
+		if (!cfg[i])
+			return -EINVAL;
+
+		/* lane1 offset is 4 */
+		writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, cfg[i]),
+		       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET + i * 4);
+	}
+
+	usleep_range(300, 500);
+
+	if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
+		drm_dbg_dp(dp->dev, "dp serdes cfg failed\n");
+		return -EAGAIN;
+	}
+
+	return 0;
+}
+
+int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
+{
+	writel(rate, dp->serdes_base + HIBMC_DP_LANE0_RATE_OFFSET);
+	writel(rate, dp->serdes_base + HIBMC_DP_LANE1_RATE_OFFSET);
+
+	usleep_range(300, 500);
+
+	if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
+		drm_dbg_dp(dp->dev, "dp serdes rate switching failed\n");
+		return -EAGAIN;
+	}
+
+	if (rate < DP_SERDES_BW_8_1)
+		drm_dbg_dp(dp->dev, "reducing serdes rate to :%d\n",
+			   rate ? rate * HIBMC_DP_LINK_RATE_CAL * 10 : 162);
+
+	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);
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
new file mode 100644
index 000000000000..812d0794543c
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (c) 2025 Hisilicon Limited. */
+
+#ifndef DP_SERDES_H
+#define DP_SERDES_H
+
+#define HIBMC_DP_HOST_OFFSET		0x10000
+#define HIBMC_DP_LANE0_RATE_OFFSET	0x4
+#define HIBMC_DP_LANE1_RATE_OFFSET	0xc
+#define HIBMC_DP_LANE_STATUS_OFFSET	0x10
+#define HIBMC_DP_PMA_LANE0_OFFSET	0x18
+#define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
+#define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
+
+/* dp serdes TX-Deempth Configuration */
+#define DP_SERDES_VOL0_PRE0		0x280
+#define DP_SERDES_VOL0_PRE1		0x2300
+#define DP_SERDES_VOL0_PRE2		0x53c0
+#define DP_SERDES_VOL0_PRE3		0x8400
+#define DP_SERDES_VOL1_PRE0		0x380
+#define DP_SERDES_VOL1_PRE1		0x3440
+#define DP_SERDES_VOL1_PRE2		0x6480
+#define DP_SERDES_VOL2_PRE0		0x500
+#define DP_SERDES_VOL2_PRE1		0x4500
+#define DP_SERDES_VOL3_PRE0		0x600
+#define DP_SERDES_BW_8_1		0x3
+
+#define DP_SERDES_DONE			0x3
+
+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]);
+
+#endif
-- 
2.33.0

Re: [PATCH v3 drm-dp 2/8] drm/hisilicon/hibmc: Add dp serdes cfg to adjust serdes rate, voltage and pre-emphasis

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:50:55AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> This dp controller need features of digital-to-analog conversion and
> high-speed transmission in chip by its extern serdes controller. Our
> serdes cfg is relatively simple, just need two register configurations.
> Don't need too much functions, like: power on/off, initialize, and some
> complex configurations, so I'm not going to use the phy framework.
> This serdes is inited and configured in dp initialization, and also
> integrating them into link training process.
> 
> For rate changing, we can change from 1.62-8.2Gpbs by cfg reg.
> For voltage and pre-emphasis levels changing, we can cfg different
> serdes ffe value.
> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 - > v3:
>   - add commit log about dp serdes, suggested by Dmitry Baryshkov.
>   - return value in hibmc_dp_serdes_init(), suggested by Dmitry Baryshkov.
>   - add static const in the array of serdes_tx_cfg[], suggested by Dmitry Baryshkov.
>   - change drm_warn to drm_dbg_dp, suggested by Dmitry Baryshkov.
>   - add explanations about dp serdes macros, suggested by Dmitry Baryshkov.
> v1 -> v2:
>   - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
>   - changing all names of dp phy to dp serdes.
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile      |  2 +-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 +
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 ++
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 72 +++++++++++++++++++
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    | 34 +++++++++
>  5 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 95a4ed599d98..43de077d6769 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> index 2c52a4476c4d..e7746bc4b592 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> @@ -38,6 +38,7 @@ struct hibmc_dp_dev {
>  	struct mutex lock; /* protects concurrent RW in hibmc_dp_reg_write_field() */
>  	struct hibmc_dp_link link;
>  	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> +	void __iomem *serdes_base;
>  };
>  
>  #define dp_field_modify(reg_value, mask, val)				\
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index a8d543881c09..59078483ec55 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -7,6 +7,7 @@
>  #include "dp_comm.h"
>  #include "dp_reg.h"
>  #include "dp_hw.h"
> +#include "dp_serdes.h"
>  
>  static void hibmc_dp_set_tu(struct hibmc_dp_dev *dp, struct drm_display_mode *mode)
>  {
> @@ -151,6 +152,7 @@ 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)
> @@ -165,6 +167,10 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>  
>  	hibmc_dp_aux_init(dp_dev);
>  
> +	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_2_7;
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> new file mode 100644
> index 000000000000..241b9ef782b0
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2025 Hisilicon Limited.
> +
> +#include <linux/delay.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_print.h>
> +#include "dp_comm.h"
> +#include "dp_config.h"
> +#include "dp_reg.h"
> +#include "dp_serdes.h"
> +
> +int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX])
> +{
> +	static const u32 serdes_tx_cfg[4][4] = { {DP_SERDES_VOL0_PRE0, DP_SERDES_VOL0_PRE1,
> +						  DP_SERDES_VOL0_PRE2, DP_SERDES_VOL0_PRE3},
> +						 {DP_SERDES_VOL1_PRE0, DP_SERDES_VOL1_PRE1,
> +						  DP_SERDES_VOL1_PRE2}, {DP_SERDES_VOL2_PRE0,
> +						  DP_SERDES_VOL2_PRE1}, {DP_SERDES_VOL3_PRE0}};
> +	int cfg[2];
> +	int i;
> +
> +	for (i = 0; i < HIBMC_DP_LANE_NUM_MAX; i++) {
> +		cfg[i] = serdes_tx_cfg[(train_set[i] & 0x3)]
> +			 [(train_set[i] << DP_TRAIN_PRE_EMPHASIS_SHIFT & 0x3)];

I think this will not work as expected. There should be no need to shift
train_set[i], otherwise first '&' is executed, reducing array index to
(train_set[i]) << 3), which then is out-of-boundaries for the array.

Most likely you meant:

cfg[i] = serdes_tx_cfg[FIELD_GET(DP_TRAIN_VOLTAGE_SWING_MASK, train_set[i]]
		      [FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK, train_set[i]];


> +		if (!cfg[i])
> +			return -EINVAL;
> +
> +		/* lane1 offset is 4 */
> +		writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, cfg[i]),
> +		       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET + i * 4);
> +	}
> +
> +	usleep_range(300, 500);
> +
> +	if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
> +		drm_dbg_dp(dp->dev, "dp serdes cfg failed\n");
> +		return -EAGAIN;
> +	}
> +
> +	return 0;
> +}
> +
> +int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
> +{
> +	writel(rate, dp->serdes_base + HIBMC_DP_LANE0_RATE_OFFSET);
> +	writel(rate, dp->serdes_base + HIBMC_DP_LANE1_RATE_OFFSET);
> +
> +	usleep_range(300, 500);
> +
> +	if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
> +		drm_dbg_dp(dp->dev, "dp serdes rate switching failed\n");
> +		return -EAGAIN;
> +	}
> +
> +	if (rate < DP_SERDES_BW_8_1)
> +		drm_dbg_dp(dp->dev, "reducing serdes rate to :%d\n",
> +			   rate ? rate * HIBMC_DP_LINK_RATE_CAL * 10 : 162);
> +
> +	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);
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> new file mode 100644
> index 000000000000..812d0794543c
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (c) 2025 Hisilicon Limited. */
> +
> +#ifndef DP_SERDES_H
> +#define DP_SERDES_H
> +

Why all these values are not a part of the dp_reg.h?

> +#define HIBMC_DP_HOST_OFFSET		0x10000
> +#define HIBMC_DP_LANE0_RATE_OFFSET	0x4
> +#define HIBMC_DP_LANE1_RATE_OFFSET	0xc
> +#define HIBMC_DP_LANE_STATUS_OFFSET	0x10
> +#define HIBMC_DP_PMA_LANE0_OFFSET	0x18
> +#define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
> +#define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
> +
> +/* dp serdes TX-Deempth Configuration */
> +#define DP_SERDES_VOL0_PRE0		0x280
> +#define DP_SERDES_VOL0_PRE1		0x2300
> +#define DP_SERDES_VOL0_PRE2		0x53c0
> +#define DP_SERDES_VOL0_PRE3		0x8400
> +#define DP_SERDES_VOL1_PRE0		0x380
> +#define DP_SERDES_VOL1_PRE1		0x3440
> +#define DP_SERDES_VOL1_PRE2		0x6480
> +#define DP_SERDES_VOL2_PRE0		0x500
> +#define DP_SERDES_VOL2_PRE1		0x4500
> +#define DP_SERDES_VOL3_PRE0		0x600
> +#define DP_SERDES_BW_8_1		0x3
> +
> +#define DP_SERDES_DONE			0x3
> +
> +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]);
> +
> +#endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 2/8] drm/hisilicon/hibmc: Add dp serdes cfg to adjust serdes rate, voltage and pre-emphasis

[Yongbang Shi]

> On Sat, Feb 22, 2025 at 10:50:55AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> This dp controller need features of digital-to-analog conversion and
>> high-speed transmission in chip by its extern serdes controller. Our
>> serdes cfg is relatively simple, just need two register configurations.
>> Don't need too much functions, like: power on/off, initialize, and some
>> complex configurations, so I'm not going to use the phy framework.
>> This serdes is inited and configured in dp initialization, and also
>> integrating them into link training process.
>>
>> For rate changing, we can change from 1.62-8.2Gpbs by cfg reg.
>> For voltage and pre-emphasis levels changing, we can cfg different
>> serdes ffe value.
>>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 - > v3:
>>    - add commit log about dp serdes, suggested by Dmitry Baryshkov.
>>    - return value in hibmc_dp_serdes_init(), suggested by Dmitry Baryshkov.
>>    - add static const in the array of serdes_tx_cfg[], suggested by Dmitry Baryshkov.
>>    - change drm_warn to drm_dbg_dp, suggested by Dmitry Baryshkov.
>>    - add explanations about dp serdes macros, suggested by Dmitry Baryshkov.
>> v1 -> v2:
>>    - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
>>    - changing all names of dp phy to dp serdes.
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile      |  2 +-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 +
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  6 ++
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 72 +++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    | 34 +++++++++
>>   5 files changed, 114 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 95a4ed599d98..43de077d6769 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,5 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o hibmc_drm_dp.o
>> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
>>   
>>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> index 2c52a4476c4d..e7746bc4b592 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
>> @@ -38,6 +38,7 @@ struct hibmc_dp_dev {
>>   	struct mutex lock; /* protects concurrent RW in hibmc_dp_reg_write_field() */
>>   	struct hibmc_dp_link link;
>>   	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>> +	void __iomem *serdes_base;
>>   };
>>   
>>   #define dp_field_modify(reg_value, mask, val)				\
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index a8d543881c09..59078483ec55 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -7,6 +7,7 @@
>>   #include "dp_comm.h"
>>   #include "dp_reg.h"
>>   #include "dp_hw.h"
>> +#include "dp_serdes.h"
>>   
>>   static void hibmc_dp_set_tu(struct hibmc_dp_dev *dp, struct drm_display_mode *mode)
>>   {
>> @@ -151,6 +152,7 @@ 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)
>> @@ -165,6 +167,10 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   
>>   	hibmc_dp_aux_init(dp_dev);
>>   
>> +	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_2_7;
>>   
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>> new file mode 100644
>> index 000000000000..241b9ef782b0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>> @@ -0,0 +1,72 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2025 Hisilicon Limited.
>> +
>> +#include <linux/delay.h>
>> +#include <drm/drm_device.h>
>> +#include <drm/drm_print.h>
>> +#include "dp_comm.h"
>> +#include "dp_config.h"
>> +#include "dp_reg.h"
>> +#include "dp_serdes.h"
>> +
>> +int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX])
>> +{
>> +	static const u32 serdes_tx_cfg[4][4] = { {DP_SERDES_VOL0_PRE0, DP_SERDES_VOL0_PRE1,
>> +						  DP_SERDES_VOL0_PRE2, DP_SERDES_VOL0_PRE3},
>> +						 {DP_SERDES_VOL1_PRE0, DP_SERDES_VOL1_PRE1,
>> +						  DP_SERDES_VOL1_PRE2}, {DP_SERDES_VOL2_PRE0,
>> +						  DP_SERDES_VOL2_PRE1}, {DP_SERDES_VOL3_PRE0}};
>> +	int cfg[2];
>> +	int i;
>> +
>> +	for (i = 0; i < HIBMC_DP_LANE_NUM_MAX; i++) {
>> +		cfg[i] = serdes_tx_cfg[(train_set[i] & 0x3)]
>> +			 [(train_set[i] << DP_TRAIN_PRE_EMPHASIS_SHIFT & 0x3)];
> I think this will not work as expected. There should be no need to shift
> train_set[i], otherwise first '&' is executed, reducing array index to
> (train_set[i]) << 3), which then is out-of-boundaries for the array.
>
> Most likely you meant:
>
> cfg[i] = serdes_tx_cfg[FIELD_GET(DP_TRAIN_VOLTAGE_SWING_MASK, train_set[i]]
> 		      [FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK, train_set[i]];

Yeah, that's right, I'll change it. Thanks for your correcting！


>
>> +		if (!cfg[i])
>> +			return -EINVAL;
>> +
>> +		/* lane1 offset is 4 */
>> +		writel(FIELD_PREP(HIBMC_DP_PMA_TXDEEMPH, cfg[i]),
>> +		       dp->serdes_base + HIBMC_DP_PMA_LANE0_OFFSET + i * 4);
>> +	}
>> +
>> +	usleep_range(300, 500);
>> +
>> +	if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
>> +		drm_dbg_dp(dp->dev, "dp serdes cfg failed\n");
>> +		return -EAGAIN;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp)
>> +{
>> +	writel(rate, dp->serdes_base + HIBMC_DP_LANE0_RATE_OFFSET);
>> +	writel(rate, dp->serdes_base + HIBMC_DP_LANE1_RATE_OFFSET);
>> +
>> +	usleep_range(300, 500);
>> +
>> +	if (readl(dp->serdes_base + HIBMC_DP_LANE_STATUS_OFFSET) != DP_SERDES_DONE) {
>> +		drm_dbg_dp(dp->dev, "dp serdes rate switching failed\n");
>> +		return -EAGAIN;
>> +	}
>> +
>> +	if (rate < DP_SERDES_BW_8_1)
>> +		drm_dbg_dp(dp->dev, "reducing serdes rate to :%d\n",
>> +			   rate ? rate * HIBMC_DP_LINK_RATE_CAL * 10 : 162);
>> +
>> +	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);
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>> new file mode 100644
>> index 000000000000..812d0794543c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/* Copyright (c) 2025 Hisilicon Limited. */
>> +
>> +#ifndef DP_SERDES_H
>> +#define DP_SERDES_H
>> +
> Why all these values are not a part of the dp_reg.h?

Okay, I'll merged together.


>> +#define HIBMC_DP_HOST_OFFSET		0x10000
>> +#define HIBMC_DP_LANE0_RATE_OFFSET	0x4
>> +#define HIBMC_DP_LANE1_RATE_OFFSET	0xc
>> +#define HIBMC_DP_LANE_STATUS_OFFSET	0x10
>> +#define HIBMC_DP_PMA_LANE0_OFFSET	0x18
>> +#define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
>> +#define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
>> +
>> +/* dp serdes TX-Deempth Configuration */
>> +#define DP_SERDES_VOL0_PRE0		0x280
>> +#define DP_SERDES_VOL0_PRE1		0x2300
>> +#define DP_SERDES_VOL0_PRE2		0x53c0
>> +#define DP_SERDES_VOL0_PRE3		0x8400
>> +#define DP_SERDES_VOL1_PRE0		0x380
>> +#define DP_SERDES_VOL1_PRE1		0x3440
>> +#define DP_SERDES_VOL1_PRE2		0x6480
>> +#define DP_SERDES_VOL2_PRE0		0x500
>> +#define DP_SERDES_VOL2_PRE1		0x4500
>> +#define DP_SERDES_VOL3_PRE0		0x600
>> +#define DP_SERDES_BW_8_1		0x3
>> +
>> +#define DP_SERDES_DONE			0x3
>> +
>> +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]);
>> +
>> +#endif
>> -- 
>> 2.33.0
>>

[PATCH v3 drm-dp 3/8] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Add dp serdes cfg in link training process, and related adapting
and modificating. Change some init values about training,
because we want completely to negotiation process, so we start with
the maximum rate and the electrical characteristic level is 0.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - change commit to an imperative sentence, suggested by Dmitry Baryshkov.
  - put HIBMC_DP_HOST_SERDES_CTRL in dp_serdes.h, suggested by Dmitry Baryshkov.
v1 -> v2:
  - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
---
 .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  5 ++-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  6 ++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
 6 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
index 74dd9956144e..c5feef8dc27d 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
@@ -15,5 +15,6 @@
 #define HIBMC_DP_CLK_EN			0x7
 #define HIBMC_DP_SYNC_EN_MASK		0x3
 #define HIBMC_DP_LINK_RATE_CAL		27
+#define HIBMC_DP_SYNC_DELAY(lanes)	((lanes) == 0x2 ? 86 : 46)
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index 59078483ec55..ad3e5c527e39 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -73,6 +73,9 @@ static void hibmc_dp_set_sst(struct hibmc_dp_dev *dp, struct drm_display_mode *m
 				 HIBMC_DP_CFG_STREAM_HTOTAL_SIZE, htotal_size);
 	hibmc_dp_reg_write_field(dp, HIBMC_DP_VIDEO_HORIZONTAL_SIZE,
 				 HIBMC_DP_CFG_STREAM_HBLANK_SIZE, hblank_size);
+	hibmc_dp_reg_write_field(dp, HIBMC_DP_VIDEO_PACKET,
+				 HIBMC_DP_CFG_STREAM_SYNC_CALIBRATION,
+				 HIBMC_DP_SYNC_DELAY(dp->link.cap.lanes));
 }
 
 static void hibmc_dp_link_cfg(struct hibmc_dp_dev *dp, struct drm_display_mode *mode)
@@ -172,7 +175,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 		return ret;
 
 	dp_dev->link.cap.lanes = 0x2;
-	dp_dev->link.cap.link_rate = DP_LINK_BW_2_7;
+	dp_dev->link.cap.link_rate = DP_LINK_BW_8_1;
 
 	/* hdcp data */
 	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
index f6355c16cc0a..e2bee7500226 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -6,6 +6,7 @@
 #include <drm/drm_print.h>
 #include "dp_comm.h"
 #include "dp_reg.h"
+#include "dp_serdes.h"
 
 #define HIBMC_EQ_MAX_RETRY 5
 
@@ -108,7 +109,11 @@ static int hibmc_dp_link_training_cr_pre(struct hibmc_dp_dev *dp)
 		return ret;
 
 	for (i = 0; i < dp->link.cap.lanes; i++)
-		train_set[i] = DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
+		train_set[i] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
+
+	ret = hibmc_dp_serdes_set_tx_cfg(dp, dp->link.train_set);
+	if (ret)
+		return ret;
 
 	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set, dp->link.cap.lanes);
 	if (ret != dp->link.cap.lanes) {
@@ -137,21 +142,28 @@ static bool hibmc_dp_link_get_adjust_train(struct hibmc_dp_dev *dp,
 	return false;
 }
 
-static inline int hibmc_dp_link_reduce_rate(struct hibmc_dp_dev *dp)
+static int hibmc_dp_link_reduce_rate(struct hibmc_dp_dev *dp)
 {
+	u8 rate = 0;
+
 	switch (dp->link.cap.link_rate) {
 	case DP_LINK_BW_2_7:
 		dp->link.cap.link_rate = DP_LINK_BW_1_62;
-		return 0;
+		rate = DP_SERDES_BW_1_62;
+		break;
 	case DP_LINK_BW_5_4:
 		dp->link.cap.link_rate = DP_LINK_BW_2_7;
-		return 0;
+		rate = DP_SERDES_BW_2_7;
+		break;
 	case DP_LINK_BW_8_1:
 		dp->link.cap.link_rate = DP_LINK_BW_5_4;
-		return 0;
+		rate = DP_SERDES_BW_5_4;
+		break;
 	default:
 		return -EINVAL;
 	}
+
+	return hibmc_dp_serdes_rate_switch(rate, dp);
 }
 
 static inline int hibmc_dp_link_reduce_lane(struct hibmc_dp_dev *dp)
@@ -159,6 +171,7 @@ static inline int hibmc_dp_link_reduce_lane(struct hibmc_dp_dev *dp)
 	switch (dp->link.cap.lanes) {
 	case 0x2:
 		dp->link.cap.lanes--;
+		drm_dbg_dp(dp->dev, "dp link training reduce to 1 lane\n");
 		break;
 	case 0x1:
 		drm_err(dp->dev, "dp link training reduce lane failed, already reach minimum\n");
@@ -206,6 +219,11 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
 		}
 
 		level_changed = hibmc_dp_link_get_adjust_train(dp, lane_status);
+
+		ret = hibmc_dp_serdes_set_tx_cfg(dp, dp->link.train_set);
+		if (ret)
+			return ret;
+
 		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->link.train_set,
 					dp->link.cap.lanes);
 		if (ret != dp->link.cap.lanes) {
@@ -255,6 +273,11 @@ static int hibmc_dp_link_training_channel_eq(struct hibmc_dp_dev *dp)
 		}
 
 		hibmc_dp_link_get_adjust_train(dp, lane_status);
+
+		ret = hibmc_dp_serdes_set_tx_cfg(dp, dp->link.train_set);
+		if (ret)
+			return ret;
+
 		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
 					dp->link.train_set, dp->link.cap.lanes);
 		if (ret != dp->link.cap.lanes) {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index dc2bd3f80b70..b75ac13a5ead 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -54,6 +54,7 @@
 #define HIBMC_DP_VIDEO_PACKET			0x114
 #define HIBMC_DP_CFG_STREAM_TU_SYMBOL_SIZE	GENMASK(5, 0)
 #define HIBMC_DP_CFG_STREAM_TU_SYMBOL_FRAC_SIZE	GENMASK(9, 6)
+#define HIBMC_DP_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
 
 #define HIBMC_DP_VIDEO_MSA0			0x118
 #define HIBMC_DP_CFG_STREAM_VSTART		GENMASK(31, 16)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
index 812d0794543c..e0537cc9af41 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
@@ -4,12 +4,15 @@
 #ifndef DP_SERDES_H
 #define DP_SERDES_H
 
+#include "dp_comm.h"
+
 #define HIBMC_DP_HOST_OFFSET		0x10000
 #define HIBMC_DP_LANE0_RATE_OFFSET	0x4
 #define HIBMC_DP_LANE1_RATE_OFFSET	0xc
 #define HIBMC_DP_LANE_STATUS_OFFSET	0x10
 #define HIBMC_DP_PMA_LANE0_OFFSET	0x18
 #define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
+#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
 #define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
 
 /* dp serdes TX-Deempth Configuration */
@@ -24,6 +27,9 @@
 #define DP_SERDES_VOL2_PRE1		0x4500
 #define DP_SERDES_VOL3_PRE0		0x600
 #define DP_SERDES_BW_8_1		0x3
+#define DP_SERDES_BW_5_4		0x2
+#define DP_SERDES_BW_2_7		0x1
+#define DP_SERDES_BW_1_62		0x0
 
 #define DP_SERDES_DONE			0x3
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index e6de6d5edf6b..67d39e258cac 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -28,9 +28,7 @@
 #include "hibmc_drm_drv.h"
 #include "hibmc_drm_regs.h"
 
-#define HIBMC_DP_HOST_SERDES_CTRL		0x1f001c
-#define HIBMC_DP_HOST_SERDES_CTRL_VAL		0x8a00
-#define HIBMC_DP_HOST_SERDES_CTRL_MASK		0x7ffff
+#include "dp/dp_serdes.h"
 
 DEFINE_DRM_GEM_FOPS(hibmc_fops);
 
@@ -122,8 +120,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
 	}
 
 	/* if DP existed, init DP */
-	if ((readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL) &
-	     HIBMC_DP_HOST_SERDES_CTRL_MASK) == HIBMC_DP_HOST_SERDES_CTRL_VAL) {
+	ret = readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL);
+	if (ret) {
 		ret = hibmc_dp_init(priv);
 		if (ret)
 			drm_err(dev, "failed to init dp: %d\n", ret);
-- 
2.33.0

Re: [PATCH v3 drm-dp 3/8] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:50:56AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Add dp serdes cfg in link training process, and related adapting
> and modificating. Change some init values about training,
> because we want completely to negotiation process, so we start with
> the maximum rate and the electrical characteristic level is 0.
> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - change commit to an imperative sentence, suggested by Dmitry Baryshkov.
>   - put HIBMC_DP_HOST_SERDES_CTRL in dp_serdes.h, suggested by Dmitry Baryshkov.
> v1 -> v2:
>   - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
> ---
>  .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  5 ++-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  6 ++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
>  6 files changed, 43 insertions(+), 11 deletions(-)
> 

Mostly LGTM.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> index 812d0794543c..e0537cc9af41 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> @@ -4,12 +4,15 @@
>  #ifndef DP_SERDES_H
>  #define DP_SERDES_H
>  
> +#include "dp_comm.h"

No, please include it directly, where required. This simplifies possible
inter-header dependencies.

> +
>  #define HIBMC_DP_HOST_OFFSET		0x10000
>  #define HIBMC_DP_LANE0_RATE_OFFSET	0x4
>  #define HIBMC_DP_LANE1_RATE_OFFSET	0xc
>  #define HIBMC_DP_LANE_STATUS_OFFSET	0x10
>  #define HIBMC_DP_PMA_LANE0_OFFSET	0x18
>  #define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
> +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
>  #define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
>  
>  /* dp serdes TX-Deempth Configuration */
> @@ -24,6 +27,9 @@
>  #define DP_SERDES_VOL2_PRE1		0x4500
>  #define DP_SERDES_VOL3_PRE0		0x600
>  #define DP_SERDES_BW_8_1		0x3
> +#define DP_SERDES_BW_5_4		0x2
> +#define DP_SERDES_BW_2_7		0x1
> +#define DP_SERDES_BW_1_62		0x0
>  
>  #define DP_SERDES_DONE			0x3
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index e6de6d5edf6b..67d39e258cac 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -28,9 +28,7 @@
>  #include "hibmc_drm_drv.h"
>  #include "hibmc_drm_regs.h"
>  
> -#define HIBMC_DP_HOST_SERDES_CTRL		0x1f001c
> -#define HIBMC_DP_HOST_SERDES_CTRL_VAL		0x8a00
> -#define HIBMC_DP_HOST_SERDES_CTRL_MASK		0x7ffff
> +#include "dp/dp_serdes.h"
>  
>  DEFINE_DRM_GEM_FOPS(hibmc_fops);
>  
> @@ -122,8 +120,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>  	}
>  
>  	/* if DP existed, init DP */
> -	if ((readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL) &
> -	     HIBMC_DP_HOST_SERDES_CTRL_MASK) == HIBMC_DP_HOST_SERDES_CTRL_VAL) {
> +	ret = readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL);
> +	if (ret) {

Why?

>  		ret = hibmc_dp_init(priv);
>  		if (ret)
>  			drm_err(dev, "failed to init dp: %d\n", ret);
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 3/8] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Yongbang Shi]

> On Sat, Feb 22, 2025 at 10:50:56AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Add dp serdes cfg in link training process, and related adapting
>> and modificating. Change some init values about training,
>> because we want completely to negotiation process, so we start with
>> the maximum rate and the electrical characteristic level is 0.
>>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - change commit to an imperative sentence, suggested by Dmitry Baryshkov.
>>    - put HIBMC_DP_HOST_SERDES_CTRL in dp_serdes.h, suggested by Dmitry Baryshkov.
>> v1 -> v2:
>>    - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
>> ---
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  5 ++-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  6 ++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
>>   6 files changed, 43 insertions(+), 11 deletions(-)
>>
> Mostly LGTM.
>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>> index 812d0794543c..e0537cc9af41 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>> @@ -4,12 +4,15 @@
>>   #ifndef DP_SERDES_H
>>   #define DP_SERDES_H
>>   
>> +#include "dp_comm.h"
> No, please include it directly, where required. This simplifies possible
> inter-header dependencies.

Okay.


>> +
>>   #define HIBMC_DP_HOST_OFFSET		0x10000
>>   #define HIBMC_DP_LANE0_RATE_OFFSET	0x4
>>   #define HIBMC_DP_LANE1_RATE_OFFSET	0xc
>>   #define HIBMC_DP_LANE_STATUS_OFFSET	0x10
>>   #define HIBMC_DP_PMA_LANE0_OFFSET	0x18
>>   #define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
>> +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
>>   #define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
>>   
>>   /* dp serdes TX-Deempth Configuration */
>> @@ -24,6 +27,9 @@
>>   #define DP_SERDES_VOL2_PRE1		0x4500
>>   #define DP_SERDES_VOL3_PRE0		0x600
>>   #define DP_SERDES_BW_8_1		0x3
>> +#define DP_SERDES_BW_5_4		0x2
>> +#define DP_SERDES_BW_2_7		0x1
>> +#define DP_SERDES_BW_1_62		0x0
>>   
>>   #define DP_SERDES_DONE			0x3
>>   
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index e6de6d5edf6b..67d39e258cac 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -28,9 +28,7 @@
>>   #include "hibmc_drm_drv.h"
>>   #include "hibmc_drm_regs.h"
>>   
>> -#define HIBMC_DP_HOST_SERDES_CTRL		0x1f001c
>> -#define HIBMC_DP_HOST_SERDES_CTRL_VAL		0x8a00
>> -#define HIBMC_DP_HOST_SERDES_CTRL_MASK		0x7ffff
>> +#include "dp/dp_serdes.h"
>>   
>>   DEFINE_DRM_GEM_FOPS(hibmc_fops);
>>   
>> @@ -122,8 +120,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>>   	}
>>   
>>   	/* if DP existed, init DP */
>> -	if ((readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL) &
>> -	     HIBMC_DP_HOST_SERDES_CTRL_MASK) == HIBMC_DP_HOST_SERDES_CTRL_VAL) {
>> +	ret = readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL);
>> +	if (ret) {
> Why?

Becuase this is a serdes deempth configuration which is changed if we start cfg serdes.
The HIBMC_DP_HOST_SERDES_CTRL_VAL is default value, but we may change it. We can just
check if it is not zero. If it's not zero, it has dp block and we can init it.


>>   		ret = hibmc_dp_init(priv);
>>   		if (ret)
>>   			drm_err(dev, "failed to init dp: %d\n", ret);
>> -- 
>> 2.33.0
>>

Re: [PATCH v3 drm-dp 3/8] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 06:32:35PM +0800, Yongbang Shi wrote:
> > On Sat, Feb 22, 2025 at 10:50:56AM +0800, Yongbang Shi wrote:
> > > From: Baihan Li <libaihan@huawei.com>
> > > 
> > > Add dp serdes cfg in link training process, and related adapting
> > > and modificating. Change some init values about training,
> > > because we want completely to negotiation process, so we start with
> > > the maximum rate and the electrical characteristic level is 0.
> > > 
> > > Signed-off-by: Baihan Li <libaihan@huawei.com>
> > > Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> > > ---
> > > ChangeLog:
> > > v2 -> v3:
> > >    - change commit to an imperative sentence, suggested by Dmitry Baryshkov.
> > >    - put HIBMC_DP_HOST_SERDES_CTRL in dp_serdes.h, suggested by Dmitry Baryshkov.
> > > v1 -> v2:
> > >    - splittting the patch and add more detailed the changes in the commit message, suggested by Dmitry Baryshkov.
> > > ---
> > >   .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  5 ++-
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
> > >   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  6 ++++
> > >   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
> > >   6 files changed, 43 insertions(+), 11 deletions(-)
> > > 
> > Mostly LGTM.
> > 
> > > diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> > > index 812d0794543c..e0537cc9af41 100644
> > > --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> > > +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> > > @@ -4,12 +4,15 @@
> > >   #ifndef DP_SERDES_H
> > >   #define DP_SERDES_H
> > > +#include "dp_comm.h"
> > No, please include it directly, where required. This simplifies possible
> > inter-header dependencies.
> 
> Okay.
> 
> 
> > > +
> > >   #define HIBMC_DP_HOST_OFFSET		0x10000
> > >   #define HIBMC_DP_LANE0_RATE_OFFSET	0x4
> > >   #define HIBMC_DP_LANE1_RATE_OFFSET	0xc
> > >   #define HIBMC_DP_LANE_STATUS_OFFSET	0x10
> > >   #define HIBMC_DP_PMA_LANE0_OFFSET	0x18
> > >   #define HIBMC_DP_PMA_LANE1_OFFSET	0x1c
> > > +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
> > >   #define HIBMC_DP_PMA_TXDEEMPH		GENMASK(18, 1)
> > >   /* dp serdes TX-Deempth Configuration */
> > > @@ -24,6 +27,9 @@
> > >   #define DP_SERDES_VOL2_PRE1		0x4500
> > >   #define DP_SERDES_VOL3_PRE0		0x600
> > >   #define DP_SERDES_BW_8_1		0x3
> > > +#define DP_SERDES_BW_5_4		0x2
> > > +#define DP_SERDES_BW_2_7		0x1
> > > +#define DP_SERDES_BW_1_62		0x0
> > >   #define DP_SERDES_DONE			0x3
> > > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> > > index e6de6d5edf6b..67d39e258cac 100644
> > > --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> > > +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> > > @@ -28,9 +28,7 @@
> > >   #include "hibmc_drm_drv.h"
> > >   #include "hibmc_drm_regs.h"
> > > -#define HIBMC_DP_HOST_SERDES_CTRL		0x1f001c
> > > -#define HIBMC_DP_HOST_SERDES_CTRL_VAL		0x8a00
> > > -#define HIBMC_DP_HOST_SERDES_CTRL_MASK		0x7ffff
> > > +#include "dp/dp_serdes.h"
> > >   DEFINE_DRM_GEM_FOPS(hibmc_fops);
> > > @@ -122,8 +120,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
> > >   	}
> > >   	/* if DP existed, init DP */
> > > -	if ((readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL) &
> > > -	     HIBMC_DP_HOST_SERDES_CTRL_MASK) == HIBMC_DP_HOST_SERDES_CTRL_VAL) {
> > > +	ret = readl(priv->mmio + HIBMC_DP_HOST_SERDES_CTRL);
> > > +	if (ret) {
> > Why?
> 
> Becuase this is a serdes deempth configuration which is changed if we start cfg serdes.
> The HIBMC_DP_HOST_SERDES_CTRL_VAL is default value, but we may change it. We can just
> check if it is not zero. If it's not zero, it has dp block and we can init it.

=> commit message, please.

> 
> 
> > >   		ret = hibmc_dp_init(priv);
> > >   		if (ret)
> > >   			drm_err(dev, "failed to init dp: %d\n", ret);
> > > -- 
> > > 2.33.0
> > > 

-- 
With best wishes
Dmitry

[PATCH v3 drm-dp 4/8] drm/hisilicon/hibmc: Refactor the member of drm_aux in struct hibmc_dp

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Because the drm_aux of struct hibmc_dp_dev's member is not easy to get in
hibmc_drm_dp.c, we move the drm_aux to struct hibmc_dp. Then there are some
adaptations and modifications to make this patch compile.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - split the patch into two parts, suggested by Dmitry Baryshkov.
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c  | 13 +++++++-----
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  6 ++++--
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c   |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h   |  2 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c | 22 ++++++++++----------
 5 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
index 0a903cce1fa9..ded9e7ce887a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
@@ -8,6 +8,7 @@
 #include <drm/drm_print.h>
 #include "dp_comm.h"
 #include "dp_reg.h"
+#include "dp_hw.h"
 
 #define HIBMC_AUX_CMD_REQ_LEN		GENMASK(7, 4)
 #define HIBMC_AUX_CMD_ADDR		GENMASK(27, 8)
@@ -124,7 +125,8 @@ static int hibmc_dp_aux_parse_xfer(struct hibmc_dp_dev *dp, struct drm_dp_aux_ms
 /* ret >= 0 ,ret is size; ret < 0, ret is err code */
 static ssize_t hibmc_dp_aux_xfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
-	struct hibmc_dp_dev *dp = container_of(aux, struct hibmc_dp_dev, aux);
+	struct hibmc_dp *dp_priv = container_of(aux, struct hibmc_dp, aux);
+	struct hibmc_dp_dev *dp = dp_priv->dp_dev;
 	u32 aux_cmd;
 	int ret;
 	u32 val; /* val will be assigned at the beginning of readl_poll_timeout function */
@@ -151,14 +153,15 @@ static ssize_t hibmc_dp_aux_xfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *
 	return hibmc_dp_aux_parse_xfer(dp, msg);
 }
 
-void hibmc_dp_aux_init(struct hibmc_dp_dev *dp)
+void hibmc_dp_aux_init(struct hibmc_dp *dp)
 {
-	hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
-	hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
-	hibmc_dp_reg_write_field(dp, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM,
+	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
+	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
+	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM,
 				 HIBMC_DP_MIN_PULSE_NUM);
 
 	dp->aux.transfer = hibmc_dp_aux_xfer;
 	dp->aux.is_remote = 0;
 	drm_dp_aux_init(&dp->aux);
+	dp->dp_dev->aux = &dp->aux;
 }
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index e7746bc4b592..2296c7258cbb 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -13,6 +13,8 @@
 #include <linux/io.h>
 #include <drm/display/drm_dp_helper.h>
 
+#include "dp_hw.h"
+
 #define HIBMC_DP_LANE_NUM_MAX 2
 
 struct hibmc_link_status {
@@ -32,7 +34,7 @@ struct hibmc_dp_link {
 };
 
 struct hibmc_dp_dev {
-	struct drm_dp_aux aux;
+	struct drm_dp_aux *aux;
 	struct drm_device *dev;
 	void __iomem *base;
 	struct mutex lock; /* protects concurrent RW in hibmc_dp_reg_write_field() */
@@ -58,7 +60,7 @@ struct hibmc_dp_dev {
 		mutex_unlock(&_dp->lock);				\
 	} while (0)
 
-void hibmc_dp_aux_init(struct hibmc_dp_dev *dp);
+void hibmc_dp_aux_init(struct hibmc_dp *dp);
 int hibmc_dp_link_training(struct hibmc_dp_dev *dp);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index ad3e5c527e39..9d673f431a0e 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -168,7 +168,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 	dp_dev->dev = drm_dev;
 	dp_dev->base = dp->mmio + HIBMC_DP_OFFSET;
 
-	hibmc_dp_aux_init(dp_dev);
+	hibmc_dp_aux_init(dp);
 
 	ret = hibmc_dp_serdes_init(dp_dev);
 	if (ret)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 4dc13b3d9875..53b6d0beecea 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -10,6 +10,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_print.h>
+#include <drm/display/drm_dp_helper.h>
 
 struct hibmc_dp_dev;
 
@@ -19,6 +20,7 @@ struct hibmc_dp {
 	struct drm_encoder encoder;
 	struct drm_connector connector;
 	void __iomem *mmio;
+	struct drm_dp_aux aux;
 };
 
 int hibmc_dp_hw_init(struct hibmc_dp *dp);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
index e2bee7500226..add55787fbea 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -27,7 +27,7 @@ static int hibmc_dp_link_training_configure(struct hibmc_dp_dev *dp)
 	/* set rate and lane count */
 	buf[0] = dp->link.cap.link_rate;
 	buf[1] = DP_LANE_COUNT_ENHANCED_FRAME_EN | dp->link.cap.lanes;
-	ret = drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, buf, sizeof(buf));
+	ret = drm_dp_dpcd_write(dp->aux, DP_LINK_BW_SET, buf, sizeof(buf));
 	if (ret != sizeof(buf)) {
 		drm_dbg_dp(dp->dev, "dp aux write link rate and lanes failed, ret: %d\n", ret);
 		return ret >= 0 ? -EIO : ret;
@@ -36,13 +36,13 @@ static int hibmc_dp_link_training_configure(struct hibmc_dp_dev *dp)
 	/* set 8b/10b and downspread */
 	buf[0] = DP_SPREAD_AMP_0_5;
 	buf[1] = DP_SET_ANSI_8B10B;
-	ret = drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, buf, sizeof(buf));
+	ret = drm_dp_dpcd_write(dp->aux, DP_DOWNSPREAD_CTRL, buf, sizeof(buf));
 	if (ret != sizeof(buf)) {
 		drm_dbg_dp(dp->dev, "dp aux write 8b/10b and downspread failed, ret: %d\n", ret);
 		return ret >= 0 ? -EIO : ret;
 	}
 
-	ret = drm_dp_read_dpcd_caps(&dp->aux, dp->dpcd);
+	ret = drm_dp_read_dpcd_caps(dp->aux, dp->dpcd);
 	if (ret)
 		drm_err(dp->dev, "dp aux read dpcd failed, ret: %d\n", ret);
 
@@ -85,7 +85,7 @@ static int hibmc_dp_link_set_pattern(struct hibmc_dp_dev *dp, int pattern)
 
 	hibmc_dp_reg_write_field(dp, HIBMC_DP_PHYIF_CTRL0, HIBMC_DP_CFG_PAT_SEL, val);
 
-	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
+	ret = drm_dp_dpcd_write(dp->aux, DP_TRAINING_PATTERN_SET, &buf, sizeof(buf));
 	if (ret != sizeof(buf)) {
 		drm_dbg_dp(dp->dev, "dp aux write training pattern set failed\n");
 		return ret >= 0 ? -EIO : ret;
@@ -115,7 +115,7 @@ static int hibmc_dp_link_training_cr_pre(struct hibmc_dp_dev *dp)
 	if (ret)
 		return ret;
 
-	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set, dp->link.cap.lanes);
+	ret = drm_dp_dpcd_write(dp->aux, DP_TRAINING_LANE0_SET, train_set, dp->link.cap.lanes);
 	if (ret != dp->link.cap.lanes) {
 		drm_dbg_dp(dp->dev, "dp aux write training lane set failed\n");
 		return ret >= 0 ? -EIO : ret;
@@ -198,9 +198,9 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
 
 	voltage_tries = 1;
 	for (cr_tries = 0; cr_tries < 80; cr_tries++) {
-		drm_dp_link_train_clock_recovery_delay(&dp->aux, dp->dpcd);
+		drm_dp_link_train_clock_recovery_delay(dp->aux, dp->dpcd);
 
-		ret = drm_dp_dpcd_read_link_status(&dp->aux, lane_status);
+		ret = drm_dp_dpcd_read_link_status(dp->aux, lane_status);
 		if (ret != DP_LINK_STATUS_SIZE) {
 			drm_err(dp->dev, "Get lane status failed\n");
 			return ret;
@@ -224,7 +224,7 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
 		if (ret)
 			return ret;
 
-		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->link.train_set,
+		ret = drm_dp_dpcd_write(dp->aux, DP_TRAINING_LANE0_SET, dp->link.train_set,
 					dp->link.cap.lanes);
 		if (ret != dp->link.cap.lanes) {
 			drm_dbg_dp(dp->dev, "Update link training failed\n");
@@ -251,9 +251,9 @@ static int hibmc_dp_link_training_channel_eq(struct hibmc_dp_dev *dp)
 		return ret;
 
 	for (eq_tries = 0; eq_tries < HIBMC_EQ_MAX_RETRY; eq_tries++) {
-		drm_dp_link_train_channel_eq_delay(&dp->aux, dp->dpcd);
+		drm_dp_link_train_channel_eq_delay(dp->aux, dp->dpcd);
 
-		ret = drm_dp_dpcd_read_link_status(&dp->aux, lane_status);
+		ret = drm_dp_dpcd_read_link_status(dp->aux, lane_status);
 		if (ret != DP_LINK_STATUS_SIZE) {
 			drm_err(dp->dev, "get lane status failed\n");
 			break;
@@ -278,7 +278,7 @@ static int hibmc_dp_link_training_channel_eq(struct hibmc_dp_dev *dp)
 		if (ret)
 			return ret;
 
-		ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
+		ret = drm_dp_dpcd_write(dp->aux, DP_TRAINING_LANE0_SET,
 					dp->link.train_set, dp->link.cap.lanes);
 		if (ret != dp->link.cap.lanes) {
 			drm_dbg_dp(dp->dev, "Update link training failed\n");
-- 
2.33.0

Re: [PATCH v3 drm-dp 4/8] drm/hisilicon/hibmc: Refactor the member of drm_aux in struct hibmc_dp

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:50:57AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Because the drm_aux of struct hibmc_dp_dev's member is not easy to get in
> hibmc_drm_dp.c, we move the drm_aux to struct hibmc_dp. Then there are some
> adaptations and modifications to make this patch compile.

Nit: move, not 'we move'.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - split the patch into two parts, suggested by Dmitry Baryshkov.
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c  | 13 +++++++-----
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  6 ++++--
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c   |  2 +-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h   |  2 ++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c | 22 ++++++++++----------
>  5 files changed, 26 insertions(+), 19 deletions(-)

-- 
With best wishes
Dmitry

[PATCH v3 drm-dp 5/8] drm/hisilicon/hibmc: Getting connector info and EDID by using AUX channel

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Add registering drm_aux and use it to get connector edid with drm
functions. Add ddc channel in connector initialization to put drm_aux
in drm_connector. And also add detect callback to detect connector
befored call connector_get_modes.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - Capitalized EDID and AUX, suggested by Dmitry Baryshkov.
v1 -> v2:
  - deleting type conversion, suggested by Dmitry Baryshkov.
  - deleting hibmc_dp_connector_get_modes() and using drm_connector_helper_get_modes(), suggested by Dmitry Baryshkov.
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   |  3 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 33 ++++++++++++++++---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  5 +++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
index ded9e7ce887a..e0bb9b14d9d8 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c
@@ -161,7 +161,8 @@ void hibmc_dp_aux_init(struct hibmc_dp *dp)
 				 HIBMC_DP_MIN_PULSE_NUM);
 
 	dp->aux.transfer = hibmc_dp_aux_xfer;
-	dp->aux.is_remote = 0;
+	dp->aux.name = kasprintf(GFP_KERNEL, "HIBMC DRM dp aux");
+	dp->aux.drm_dev = dp->drm_dev;
 	drm_dp_aux_init(&dp->aux);
 	dp->dp_dev->aux = &dp->aux;
 }
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 603d6b198a54..1cb9c32f9ef0 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -15,25 +15,48 @@
 
 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;
 
-	count = drm_add_modes_noedid(connector, connector->dev->mode_config.max_width,
-				     connector->dev->mode_config.max_height);
-	drm_set_preferred_mode(connector, 1024, 768); // temporary implementation
+	drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
+
+	drm_edid_connector_update(connector, drm_edid);
+
+	count = drm_edid_connector_add_modes(connector);
+
+	drm_edid_free(drm_edid);
 
 	return count;
 }
 
 static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
 	.get_modes = hibmc_dp_connector_get_modes,
+	.detect_ctx = drm_connector_helper_detect_from_ddc,
 };
 
+static int hibmc_dp_late_register(struct drm_connector *connector)
+{
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
+
+	return drm_dp_aux_register(&dp->aux);
+}
+
+static void hibmc_dp_early_unregister(struct drm_connector *connector)
+{
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
+
+	drm_dp_aux_unregister(&dp->aux);
+}
+
 static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.late_register = hibmc_dp_late_register,
+	.early_unregister = hibmc_dp_early_unregister,
 };
 
 static inline int hibmc_dp_prepare(struct hibmc_dp *dp, struct drm_display_mode *mode)
@@ -103,8 +126,8 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
 
 	drm_encoder_helper_add(encoder, &hibmc_dp_encoder_helper_funcs);
 
-	ret = drm_connector_init(dev, connector, &hibmc_dp_conn_funcs,
-				 DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init_with_ddc(dev, connector, &hibmc_dp_conn_funcs,
+					  DRM_MODE_CONNECTOR_DisplayPort, &dp->aux.ddc);
 	if (ret) {
 		drm_err(dev, "init dp connector failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index d982f1e4b958..3ddd71aada66 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -47,6 +47,11 @@ static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
 	return container_of(connector, struct hibmc_vdac, connector);
 }
 
+static inline struct hibmc_dp *to_hibmc_dp(struct drm_connector *connector)
+{
+	return container_of(connector, struct hibmc_dp, connector);
+}
+
 static inline struct hibmc_drm_private *to_hibmc_drm_private(struct drm_device *dev)
 {
 	return container_of(dev, struct hibmc_drm_private, dev);
-- 
2.33.0

Re: [PATCH v3 drm-dp 5/8] drm/hisilicon/hibmc: Getting connector info and EDID by using AUX channel

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:50:58AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Add registering drm_aux and use it to get connector edid with drm
> functions. Add ddc channel in connector initialization to put drm_aux
> in drm_connector. And also add detect callback to detect connector
> befored call connector_get_modes.
> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - Capitalized EDID and AUX, suggested by Dmitry Baryshkov.
> v1 -> v2:
>   - deleting type conversion, suggested by Dmitry Baryshkov.
>   - deleting hibmc_dp_connector_get_modes() and using drm_connector_helper_get_modes(), suggested by Dmitry Baryshkov.
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_aux.c   |  3 +-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 33 ++++++++++++++++---
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  5 +++
>  3 files changed, 35 insertions(+), 6 deletions(-)
> 
>  
>  static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>  	.get_modes = hibmc_dp_connector_get_modes,
> +	.detect_ctx = drm_connector_helper_detect_from_ddc,

I think a proper .detect callback should be reading sink_count. Most
likely it will work though.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


>  };
>  

-- 
With best wishes
Dmitry

[PATCH v3 drm-dp 6/8] drm/hisilicon/hibmc: Add colorbar-cfg feature and its debugfs file

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

DP controller can support generating a color bar signal over the
DisplayPort interface. This can be useful to check for possible DDR
or GPU problems, as the signal generator resides completely in the DP
block. Add debugfs file that controls colorbar generator.

echo: config the color bar register to display
cat: print the color bar configuration

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - rewrite the commit log, suggested by Dmitry Baryshkov.
  - move colorbar debugfs entry to this patch, suggested by Dmitry Baryshkov.
  - change binary format to integer format, suggested by Dmitry Baryshkov.
v1 -> v2:
  - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
  - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
  - deleting edid decoder and its debugfs, suggested by Dmitry Baryshkov.
  - using debugfs_init() callback, suggested by Dmitry Baryshkov.
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  43 ++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  29 +++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |   3 +
 .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 100 ++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
 7 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index 43de077d6769..1f65c683282f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
-	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
+	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o \
+	       hibmc_drm_debugfs.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index 9d673f431a0e..a921b98dbf50 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -227,3 +227,46 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
 
 	return 0;
 }
+
+static const struct hibmc_dp_color_raw g_rgb_raw[] = {
+	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
+	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
+	{CBAR_RED,       0xfff, 0x000, 0x000},
+	{CBAR_ORANGE,    0xfff, 0x800, 0x000},
+	{CBAR_YELLOW,    0xfff, 0xfff, 0x000},
+	{CBAR_GREEN,     0x000, 0xfff, 0x000},
+	{CBAR_CYAN,      0x000, 0x800, 0x800},
+	{CBAR_BLUE,      0x000, 0x000, 0xfff},
+	{CBAR_PURPLE,    0x800, 0x000, 0x800},
+	{CBAR_BLACK,     0x000, 0x000, 0x000},
+};
+
+void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
+{
+	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
+	struct hibmc_dp_color_raw raw_data;
+
+	if (cfg->enable) {
+		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(9),
+					 cfg->self_timing);
+		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(8, 1),
+					 cfg->dynamic_rate);
+		if (cfg->pattern == CBAR_COLOR_BAR) {
+			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 0);
+		} else {
+			raw_data = g_rgb_raw[cfg->pattern];
+			drm_dbg_dp(dp->drm_dev, "r:%x g:%x b:%x\n", raw_data.r_value,
+				   raw_data.g_value, raw_data.b_value);
+			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 1);
+			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(23, 12),
+						 raw_data.r_value);
+			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(23, 12),
+						 raw_data.g_value);
+			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(11, 0),
+						 raw_data.b_value);
+		}
+	}
+
+	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(0), cfg->enable);
+	writel(HIBMC_DP_SYNC_EN_MASK, dp_dev->base + HIBMC_DP_TIMING_SYNC_CTRL);
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 53b6d0beecea..83a53dae8012 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -14,6 +14,33 @@
 
 struct hibmc_dp_dev;
 
+enum hibmc_dp_cbar_pattern {
+	CBAR_COLOR_BAR,
+	CBAR_WHITE,
+	CBAR_RED,
+	CBAR_ORANGE,
+	CBAR_YELLOW,
+	CBAR_GREEN,
+	CBAR_CYAN,
+	CBAR_BLUE,
+	CBAR_PURPLE,
+	CBAR_BLACK,
+};
+
+struct hibmc_dp_color_raw {
+	enum hibmc_dp_cbar_pattern pattern;
+	u32 r_value;
+	u32 g_value;
+	u32 b_value;
+};
+
+struct hibmc_dp_cbar_cfg {
+	u8 enable;
+	u8 self_timing;
+	u8 dynamic_rate; /* 0:static, 1-255(frame):dynamic */
+	enum hibmc_dp_cbar_pattern pattern;
+};
+
 struct hibmc_dp {
 	struct hibmc_dp_dev *dp_dev;
 	struct drm_device *drm_dev;
@@ -21,10 +48,12 @@ struct hibmc_dp {
 	struct drm_connector connector;
 	void __iomem *mmio;
 	struct drm_dp_aux aux;
+	struct hibmc_dp_cbar_cfg cfg;
 };
 
 int hibmc_dp_hw_init(struct hibmc_dp *dp);
 int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
 void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
+void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index b75ac13a5ead..4c388f633081 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -67,6 +67,9 @@
 #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
 #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
 
+#define HIBMC_DP_COLOR_BAR_CTRL			0x260
+#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
+
 #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
 #define HIBMC_DP_CFG_TIMING_GEN0_HACTIVE	GENMASK(31, 16)
 #define HIBMC_DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
new file mode 100644
index 000000000000..8d050a36946e
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 Hisilicon Limited.
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/seq_file.h>
+#include <linux/pci.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_file.h>
+#include <drm/drm_debugfs.h>
+#include <drm/drm_edid.h>
+
+#include "hibmc_drm_drv.h"
+
+#define MAX_BUF_SIZE 12
+
+static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	struct hibmc_drm_private *priv = file_inode(file)->i_private;
+	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
+	int ret, idx;
+	u8 buf[MAX_BUF_SIZE];
+
+	if (count >= MAX_BUF_SIZE)
+		return -EINVAL;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	buf[count] = '\0';
+
+	if (sscanf(buf, "%hhu %hhu %hhu %u", &cfg->enable, &cfg->self_timing,
+		   &cfg->dynamic_rate, &cfg->pattern) != 4) {
+		return -EINVAL;
+	}
+
+	if (cfg->pattern > 9 || cfg->enable > 1 || cfg->self_timing > 1)
+		return -EINVAL;
+
+	ret = drm_dev_enter(&priv->dev, &idx);
+	if (!ret)
+		return -ENODEV;
+
+	hibmc_dp_set_cbar(&priv->dp, cfg);
+
+	drm_dev_exit(idx);
+
+	return count;
+}
+
+static int hibmc_dp_dbgfs_show(struct seq_file *m, void *arg)
+{
+	struct hibmc_drm_private *priv = m->private;
+	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
+	int idx;
+
+	if (!drm_dev_enter(&priv->dev, &idx))
+		return -ENODEV;
+
+	/* 1: 0: enable colorbar, 1: disable colorbar
+	 * 2: 0: timing follows XDP, 1: internal self timing
+	 * 3: 0: static colorbar image,
+	 *    1~255: right shifting a type of color per (1~255)frames
+	 * 4: 0~9: color bar, white, red, orange,
+	 *         yellow, green, cyan, bule, pupper, black
+	 */
+
+	seq_printf(m, "hibmc dp colorbar cfg: %u %u %u %u\n", cfg->enable, cfg->self_timing,
+		   cfg->dynamic_rate, cfg->pattern);
+
+	drm_dev_exit(idx);
+
+	return 0;
+}
+
+static int hibmc_open(struct inode *inode, struct file *filp)
+{
+	return single_open(filp, hibmc_dp_dbgfs_show, inode->i_private);
+}
+
+static const struct file_operations hibmc_dbg_fops = {
+	.owner   = THIS_MODULE,
+	.write   = hibmc_control_write,
+	.read    = seq_read,
+	.open    = hibmc_open,
+	.llseek  = seq_lseek,
+	.release = single_release,
+};
+
+void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root)
+{
+	struct drm_device *dev = connector->dev;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+
+	/* create the file in drm directory, so we don't need to remove manually */
+	debugfs_create_file("colorbar-cfg", 0200,
+			    root, priv, &hibmc_dbg_fops);
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 1cb9c32f9ef0..a7f611e82f73 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -57,6 +57,7 @@ static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 	.late_register = hibmc_dp_late_register,
 	.early_unregister = hibmc_dp_early_unregister,
+	.debugfs_init = hibmc_debugfs_init,
 };
 
 static inline int hibmc_dp_prepare(struct hibmc_dp *dp, struct drm_display_mode *mode)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index 3ddd71aada66..bc89e4b9f4e3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -69,4 +69,6 @@ int hibmc_ddc_create(struct drm_device *drm_dev, struct hibmc_vdac *connector);
 
 int hibmc_dp_init(struct hibmc_drm_private *priv);
 
+void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
+
 #endif
-- 
2.33.0

Re: [PATCH v3 drm-dp 6/8] drm/hisilicon/hibmc: Add colorbar-cfg feature and its debugfs file

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:50:59AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> DP controller can support generating a color bar signal over the
> DisplayPort interface. This can be useful to check for possible DDR
> or GPU problems, as the signal generator resides completely in the DP
> block. Add debugfs file that controls colorbar generator.
> 
> echo: config the color bar register to display
> cat: print the color bar configuration
> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - rewrite the commit log, suggested by Dmitry Baryshkov.
>   - move colorbar debugfs entry to this patch, suggested by Dmitry Baryshkov.
>   - change binary format to integer format, suggested by Dmitry Baryshkov.
> v1 -> v2:
>   - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
>   - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
>   - deleting edid decoder and its debugfs, suggested by Dmitry Baryshkov.
>   - using debugfs_init() callback, suggested by Dmitry Baryshkov.
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  43 ++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  29 +++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |   3 +
>  .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 100 ++++++++++++++++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>  7 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index 43de077d6769..1f65c683282f 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o \
> +	       hibmc_drm_debugfs.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index 9d673f431a0e..a921b98dbf50 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -227,3 +227,46 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>  
>  	return 0;
>  }
> +
> +static const struct hibmc_dp_color_raw g_rgb_raw[] = {
> +	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
> +	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
> +	{CBAR_RED,       0xfff, 0x000, 0x000},
> +	{CBAR_ORANGE,    0xfff, 0x800, 0x000},
> +	{CBAR_YELLOW,    0xfff, 0xfff, 0x000},
> +	{CBAR_GREEN,     0x000, 0xfff, 0x000},
> +	{CBAR_CYAN,      0x000, 0x800, 0x800},
> +	{CBAR_BLUE,      0x000, 0x000, 0xfff},
> +	{CBAR_PURPLE,    0x800, 0x000, 0x800},
> +	{CBAR_BLACK,     0x000, 0x000, 0x000},
> +};
> +
> +void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
> +{
> +	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
> +	struct hibmc_dp_color_raw raw_data;
> +
> +	if (cfg->enable) {
> +		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(9),
> +					 cfg->self_timing);
> +		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(8, 1),
> +					 cfg->dynamic_rate);
> +		if (cfg->pattern == CBAR_COLOR_BAR) {
> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 0);
> +		} else {
> +			raw_data = g_rgb_raw[cfg->pattern];
> +			drm_dbg_dp(dp->drm_dev, "r:%x g:%x b:%x\n", raw_data.r_value,
> +				   raw_data.g_value, raw_data.b_value);
> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 1);
> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(23, 12),
> +						 raw_data.r_value);
> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(23, 12),
> +						 raw_data.g_value);
> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(11, 0),
> +						 raw_data.b_value);
> +		}
> +	}
> +
> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(0), cfg->enable);
> +	writel(HIBMC_DP_SYNC_EN_MASK, dp_dev->base + HIBMC_DP_TIMING_SYNC_CTRL);
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> index 53b6d0beecea..83a53dae8012 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -14,6 +14,33 @@
>  
>  struct hibmc_dp_dev;
>  
> +enum hibmc_dp_cbar_pattern {
> +	CBAR_COLOR_BAR,
> +	CBAR_WHITE,
> +	CBAR_RED,
> +	CBAR_ORANGE,
> +	CBAR_YELLOW,
> +	CBAR_GREEN,
> +	CBAR_CYAN,
> +	CBAR_BLUE,
> +	CBAR_PURPLE,
> +	CBAR_BLACK,
> +};
> +
> +struct hibmc_dp_color_raw {
> +	enum hibmc_dp_cbar_pattern pattern;
> +	u32 r_value;
> +	u32 g_value;
> +	u32 b_value;
> +};
> +
> +struct hibmc_dp_cbar_cfg {
> +	u8 enable;
> +	u8 self_timing;
> +	u8 dynamic_rate; /* 0:static, 1-255(frame):dynamic */
> +	enum hibmc_dp_cbar_pattern pattern;
> +};
> +
>  struct hibmc_dp {
>  	struct hibmc_dp_dev *dp_dev;
>  	struct drm_device *drm_dev;
> @@ -21,10 +48,12 @@ struct hibmc_dp {
>  	struct drm_connector connector;
>  	void __iomem *mmio;
>  	struct drm_dp_aux aux;
> +	struct hibmc_dp_cbar_cfg cfg;
>  };
>  
>  int hibmc_dp_hw_init(struct hibmc_dp *dp);
>  int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>  void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
> +void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> index b75ac13a5ead..4c388f633081 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -67,6 +67,9 @@
>  #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
>  #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>  
> +#define HIBMC_DP_COLOR_BAR_CTRL			0x260
> +#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
> +
>  #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
>  #define HIBMC_DP_CFG_TIMING_GEN0_HACTIVE	GENMASK(31, 16)
>  #define HIBMC_DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
> new file mode 100644
> index 000000000000..8d050a36946e
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2024 Hisilicon Limited.
> +
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/seq_file.h>
> +#include <linux/pci.h>
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_file.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_edid.h>
> +
> +#include "hibmc_drm_drv.h"
> +
> +#define MAX_BUF_SIZE 12
> +
> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
> +	int ret, idx;
> +	u8 buf[MAX_BUF_SIZE];
> +
> +	if (count >= MAX_BUF_SIZE)
> +		return -EINVAL;
> +
> +	if (copy_from_user(buf, user_buf, count))
> +		return -EFAULT;
> +
> +	buf[count] = '\0';
> +

There should be at least some documentation on the written values.

> +	if (sscanf(buf, "%hhu %hhu %hhu %u", &cfg->enable, &cfg->self_timing,
> +		   &cfg->dynamic_rate, &cfg->pattern) != 4) {
> +		return -EINVAL;
> +	}
> +
> +	if (cfg->pattern > 9 || cfg->enable > 1 || cfg->self_timing > 1)
> +		return -EINVAL;
> +
> +	ret = drm_dev_enter(&priv->dev, &idx);
> +	if (!ret)
> +		return -ENODEV;
> +
> +	hibmc_dp_set_cbar(&priv->dp, cfg);
> +
> +	drm_dev_exit(idx);
> +
> +	return count;
> +}

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 6/8] drm/hisilicon/hibmc: Add colorbar-cfg feature and its debugfs file

[Yongbang Shi]

> On Sat, Feb 22, 2025 at 10:50:59AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> DP controller can support generating a color bar signal over the
>> DisplayPort interface. This can be useful to check for possible DDR
>> or GPU problems, as the signal generator resides completely in the DP
>> block. Add debugfs file that controls colorbar generator.
>>
>> echo: config the color bar register to display
>> cat: print the color bar configuration
>>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - rewrite the commit log, suggested by Dmitry Baryshkov.
>>    - move colorbar debugfs entry to this patch, suggested by Dmitry Baryshkov.
>>    - change binary format to integer format, suggested by Dmitry Baryshkov.
>> v1 -> v2:
>>    - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
>>    - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
>>    - deleting edid decoder and its debugfs, suggested by Dmitry Baryshkov.
>>    - using debugfs_init() callback, suggested by Dmitry Baryshkov.
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  43 ++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  29 +++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |   3 +
>>   .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 100 ++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>>   7 files changed, 180 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 43de077d6769..1f65c683282f 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
>> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o \
>> +	       hibmc_drm_debugfs.o
>>   
>>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index 9d673f431a0e..a921b98dbf50 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -227,3 +227,46 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>>   
>>   	return 0;
>>   }
>> +
>> +static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>> +	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>> +	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
>> +	{CBAR_RED,       0xfff, 0x000, 0x000},
>> +	{CBAR_ORANGE,    0xfff, 0x800, 0x000},
>> +	{CBAR_YELLOW,    0xfff, 0xfff, 0x000},
>> +	{CBAR_GREEN,     0x000, 0xfff, 0x000},
>> +	{CBAR_CYAN,      0x000, 0x800, 0x800},
>> +	{CBAR_BLUE,      0x000, 0x000, 0xfff},
>> +	{CBAR_PURPLE,    0x800, 0x000, 0x800},
>> +	{CBAR_BLACK,     0x000, 0x000, 0x000},
>> +};
>> +
>> +void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
>> +{
>> +	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> +	struct hibmc_dp_color_raw raw_data;
>> +
>> +	if (cfg->enable) {
>> +		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(9),
>> +					 cfg->self_timing);
>> +		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(8, 1),
>> +					 cfg->dynamic_rate);
>> +		if (cfg->pattern == CBAR_COLOR_BAR) {
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 0);
>> +		} else {
>> +			raw_data = g_rgb_raw[cfg->pattern];
>> +			drm_dbg_dp(dp->drm_dev, "r:%x g:%x b:%x\n", raw_data.r_value,
>> +				   raw_data.g_value, raw_data.b_value);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 1);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(23, 12),
>> +						 raw_data.r_value);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(23, 12),
>> +						 raw_data.g_value);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(11, 0),
>> +						 raw_data.b_value);
>> +		}
>> +	}
>> +
>> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(0), cfg->enable);
>> +	writel(HIBMC_DP_SYNC_EN_MASK, dp_dev->base + HIBMC_DP_TIMING_SYNC_CTRL);
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 53b6d0beecea..83a53dae8012 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -14,6 +14,33 @@
>>   
>>   struct hibmc_dp_dev;
>>   
>> +enum hibmc_dp_cbar_pattern {
>> +	CBAR_COLOR_BAR,
>> +	CBAR_WHITE,
>> +	CBAR_RED,
>> +	CBAR_ORANGE,
>> +	CBAR_YELLOW,
>> +	CBAR_GREEN,
>> +	CBAR_CYAN,
>> +	CBAR_BLUE,
>> +	CBAR_PURPLE,
>> +	CBAR_BLACK,
>> +};
>> +
>> +struct hibmc_dp_color_raw {
>> +	enum hibmc_dp_cbar_pattern pattern;
>> +	u32 r_value;
>> +	u32 g_value;
>> +	u32 b_value;
>> +};
>> +
>> +struct hibmc_dp_cbar_cfg {
>> +	u8 enable;
>> +	u8 self_timing;
>> +	u8 dynamic_rate; /* 0:static, 1-255(frame):dynamic */
>> +	enum hibmc_dp_cbar_pattern pattern;
>> +};
>> +
>>   struct hibmc_dp {
>>   	struct hibmc_dp_dev *dp_dev;
>>   	struct drm_device *drm_dev;
>> @@ -21,10 +48,12 @@ struct hibmc_dp {
>>   	struct drm_connector connector;
>>   	void __iomem *mmio;
>>   	struct drm_dp_aux aux;
>> +	struct hibmc_dp_cbar_cfg cfg;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>>   int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>>   void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>> +void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> index b75ac13a5ead..4c388f633081 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -67,6 +67,9 @@
>>   #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
>>   #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>>   
>> +#define HIBMC_DP_COLOR_BAR_CTRL			0x260
>> +#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
>> +
>>   #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
>>   #define HIBMC_DP_CFG_TIMING_GEN0_HACTIVE	GENMASK(31, 16)
>>   #define HIBMC_DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> new file mode 100644
>> index 000000000000..8d050a36946e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2024 Hisilicon Limited.
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/device.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/pci.h>
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_file.h>
>> +#include <drm/drm_debugfs.h>
>> +#include <drm/drm_edid.h>
>> +
>> +#include "hibmc_drm_drv.h"
>> +
>> +#define MAX_BUF_SIZE 12
>> +
>> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
>> +				   size_t count, loff_t *ppos)
>> +{
>> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
>> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
>> +	int ret, idx;
>> +	u8 buf[MAX_BUF_SIZE];
>> +
>> +	if (count >= MAX_BUF_SIZE)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(buf, user_buf, count))
>> +		return -EFAULT;
>> +
>> +	buf[count] = '\0';
>> +
> There should be at least some documentation on the written values.

Okay!


>> +	if (sscanf(buf, "%hhu %hhu %hhu %u", &cfg->enable, &cfg->self_timing,
>> +		   &cfg->dynamic_rate, &cfg->pattern) != 4) {
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cfg->pattern > 9 || cfg->enable > 1 || cfg->self_timing > 1)
>> +		return -EINVAL;
>> +
>> +	ret = drm_dev_enter(&priv->dev, &idx);
>> +	if (!ret)
>> +		return -ENODEV;
>> +
>> +	hibmc_dp_set_cbar(&priv->dp, cfg);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return count;
>> +}

[PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Enable HPD feature and add its isr and event function. Add a drm client
dev and realized the hotplug callback in it.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
  - remove enble_display in ISR, suggested by Dmitry Baryshkov.
  - change drm_kms_helper_connector_hotplug_event() to
    drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
  - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
  - remove struct irqs, suggested by Dmitry Baryshkov.
  - split this patch into two parts, suggested by Dmitry Baryshkov.
  - add a drm client dev to handle HPD event.
v1 -> v2:
  - optimizing the description in commit message, suggested by Dmitry Baryshkov.
  - add mdelay(100) comments, suggested by Dmitry Baryshkov.
  - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
---
 .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 22 +++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  6 ++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 61 +++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  2 +
 5 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
index c5feef8dc27d..08f9e1caf7fc 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
@@ -16,5 +16,6 @@
 #define HIBMC_DP_SYNC_EN_MASK		0x3
 #define HIBMC_DP_LINK_RATE_CAL		27
 #define HIBMC_DP_SYNC_DELAY(lanes)	((lanes) == 0x2 ? 86 : 46)
+#define HIBMC_DP_INT_ENABLE		0xc
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index a921b98dbf50..b2116395b8dd 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 	/* int init */
 	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
 	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
+	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
 	/* rst */
 	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
 	/* clock enable */
@@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 	return 0;
 }
 
+void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
+{
+	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
+
+	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
+	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
+	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
+	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
+	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
+	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
+	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
+	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
+	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
+}
+
 void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
 {
 	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
@@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
 	return 0;
 }
 
+void hibmc_dp_reset_link(struct hibmc_dp *dp)
+{
+	dp->dp_dev->link.status.clock_recovered = false;
+	dp->dp_dev->link.status.channel_equalized = false;
+}
+
 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 83a53dae8012..a55d66d53966 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -11,6 +11,7 @@
 #include <drm/drm_connector.h>
 #include <drm/drm_print.h>
 #include <drm/display/drm_dp_helper.h>
+#include <drm/drm_client.h>
 
 struct hibmc_dp_dev;
 
@@ -49,11 +50,16 @@ struct hibmc_dp {
 	void __iomem *mmio;
 	struct drm_dp_aux aux;
 	struct hibmc_dp_cbar_cfg cfg;
+	u32 irq_status;
+	u32 hpd_status;
+	struct drm_client_dev client;
 };
 
 int hibmc_dp_hw_init(struct hibmc_dp *dp);
 int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
 void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
 void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
+void hibmc_dp_reset_link(struct hibmc_dp *dp);
+void hibmc_dp_hpd_cfg(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 a7f611e82f73..40a3ebb8ac4b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -9,10 +9,13 @@
 #include <drm/drm_modes.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_client.h>
 
 #include "hibmc_drm_drv.h"
 #include "dp/dp_hw.h"
 
+#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
+
 static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
 {
 	struct hibmc_dp *dp = to_hibmc_dp(connector);
@@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
 	.atomic_disable = hibmc_dp_encoder_disable,
 };
 
+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);
+	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");
+		priv->dp.hpd_status = 1;
+	} else {
+		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
+		priv->dp.hpd_status = 0;
+	}
+
+	if (dev->registered)
+		drm_connector_helper_hpd_irq_event(&priv->dp.connector);
+
+	drm_dev_exit(idx);
+
+	return IRQ_HANDLED;
+}
+
+static int hibmc_dp_hpd_event(struct drm_client_dev *client)
+{
+	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
+	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
+	int ret;
+
+	if (dp->hpd_status) {
+		hibmc_dp_hpd_cfg(&priv->dp);
+		ret = hibmc_dp_prepare(dp, mode);
+		if (ret)
+			return ret;
+
+		hibmc_dp_display_en(dp, true);
+	} else {
+		hibmc_dp_display_en(dp, false);
+		hibmc_dp_reset_link(&priv->dp);
+	}
+
+	return 0;
+}
+
+static const struct drm_client_funcs hibmc_dp_client_funcs = {
+	.hotplug = hibmc_dp_hpd_event,
+	.unregister = drm_client_release,
+};
+
 int hibmc_dp_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = &priv->dev;
@@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
 
 	drm_connector_attach_encoder(connector, encoder);
 
+	ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
+	if (ret)
+		return ret;
+
+	drm_client_register(&dp->client);
+
 	return 0;
 }
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index bc89e4b9f4e3..daed1330b961 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
 
 void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
 
+irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
+
 #endif
-- 
2.33.0

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:51:00AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Enable HPD feature and add its isr and event function. Add a drm client
> dev and realized the hotplug callback in it.

What for? There should be no need to add a separate drm client just for
the hotplug.

> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>   - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
>   - remove enble_display in ISR, suggested by Dmitry Baryshkov.
>   - change drm_kms_helper_connector_hotplug_event() to
>     drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
>   - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
>   - remove struct irqs, suggested by Dmitry Baryshkov.
>   - split this patch into two parts, suggested by Dmitry Baryshkov.
>   - add a drm client dev to handle HPD event.
> v1 -> v2:
>   - optimizing the description in commit message, suggested by Dmitry Baryshkov.
>   - add mdelay(100) comments, suggested by Dmitry Baryshkov.
>   - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
> ---
>  .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 22 +++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  6 ++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 61 +++++++++++++++++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  2 +
>  5 files changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> index c5feef8dc27d..08f9e1caf7fc 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> @@ -16,5 +16,6 @@
>  #define HIBMC_DP_SYNC_EN_MASK		0x3
>  #define HIBMC_DP_LINK_RATE_CAL		27
>  #define HIBMC_DP_SYNC_DELAY(lanes)	((lanes) == 0x2 ? 86 : 46)
> +#define HIBMC_DP_INT_ENABLE		0xc
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index a921b98dbf50..b2116395b8dd 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>  	/* int init */
>  	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>  	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
> +	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>  	/* rst */
>  	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>  	/* clock enable */
> @@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>  	return 0;
>  }
>  
> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
> +{
> +	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
> +
> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
> +	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
> +	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
> +	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
> +	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
> +	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
> +	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
> +	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
> +}
> +
>  void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
>  {
>  	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
> @@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>  	return 0;
>  }
>  
> +void hibmc_dp_reset_link(struct hibmc_dp *dp)
> +{
> +	dp->dp_dev->link.status.clock_recovered = false;
> +	dp->dp_dev->link.status.channel_equalized = false;

Could you please point out, where the link rate and number of lanes are
reset?

> +}
> +
>  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 83a53dae8012..a55d66d53966 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -11,6 +11,7 @@
>  #include <drm/drm_connector.h>
>  #include <drm/drm_print.h>
>  #include <drm/display/drm_dp_helper.h>
> +#include <drm/drm_client.h>
>  
>  struct hibmc_dp_dev;
>  
> @@ -49,11 +50,16 @@ struct hibmc_dp {
>  	void __iomem *mmio;
>  	struct drm_dp_aux aux;
>  	struct hibmc_dp_cbar_cfg cfg;
> +	u32 irq_status;
> +	u32 hpd_status;
> +	struct drm_client_dev client;
>  };
>  
>  int hibmc_dp_hw_init(struct hibmc_dp *dp);
>  int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>  void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>  void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
> +void hibmc_dp_reset_link(struct hibmc_dp *dp);
> +void hibmc_dp_hpd_cfg(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 a7f611e82f73..40a3ebb8ac4b 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -9,10 +9,13 @@
>  #include <drm/drm_modes.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_client.h>
>  
>  #include "hibmc_drm_drv.h"
>  #include "dp/dp_hw.h"
>  
> +#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
> +
>  static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct hibmc_dp *dp = to_hibmc_dp(connector);
> @@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>  	.atomic_disable = hibmc_dp_encoder_disable,
>  };
>  
> +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);
> +	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");
> +		priv->dp.hpd_status = 1;
> +	} else {
> +		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
> +		priv->dp.hpd_status = 0;
> +	}
> +
> +	if (dev->registered)
> +		drm_connector_helper_hpd_irq_event(&priv->dp.connector);
> +
> +	drm_dev_exit(idx);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> +{
> +	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> +	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> +	int ret;
> +
> +	if (dp->hpd_status) {
> +		hibmc_dp_hpd_cfg(&priv->dp);
> +		ret = hibmc_dp_prepare(dp, mode);
> +		if (ret)
> +			return ret;
> +
> +		hibmc_dp_display_en(dp, true);
> +	} else {
> +		hibmc_dp_display_en(dp, false);
> +		hibmc_dp_reset_link(&priv->dp);
> +	}

If I understand this correctly, you are using a separate drm_client to
enable and disable the link & display. Why is it necessary? Existing
drm_clients and userspace compositors use drm framework, they should be
able to turn the display on and off as required.

> +
> +	return 0;
> +}
> +
> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
> +	.hotplug = hibmc_dp_hpd_event,
> +	.unregister = drm_client_release,
> +};
> +
>  int hibmc_dp_init(struct hibmc_drm_private *priv)
>  {
>  	struct drm_device *dev = &priv->dev;
> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>  
>  	drm_connector_attach_encoder(connector, encoder);
>  
> +	ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
> +	if (ret)
> +		return ret;
> +
> +	drm_client_register(&dp->client);
> +
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index bc89e4b9f4e3..daed1330b961 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>  
>  void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>  
> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
> +
>  #endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Sat, Feb 22, 2025 at 10:51:00AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Enable HPD feature and add its isr and event function. Add a drm client
>> dev and realized the hotplug callback in it.
> What for? There should be no need to add a separate drm client just for
> the hotplug.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
>>    - remove enble_display in ISR, suggested by Dmitry Baryshkov.
>>    - change drm_kms_helper_connector_hotplug_event() to
>>      drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
>>    - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
>>    - remove struct irqs, suggested by Dmitry Baryshkov.
>>    - split this patch into two parts, suggested by Dmitry Baryshkov.
>>    - add a drm client dev to handle HPD event.
>> v1 -> v2:
>>    - optimizing the description in commit message, suggested by Dmitry Baryshkov.
>>    - add mdelay(100) comments, suggested by Dmitry Baryshkov.
>>    - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
>> ---
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 22 +++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  6 ++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 61 +++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  2 +
>>   5 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> index c5feef8dc27d..08f9e1caf7fc 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> @@ -16,5 +16,6 @@
>>   #define HIBMC_DP_SYNC_EN_MASK		0x3
>>   #define HIBMC_DP_LINK_RATE_CAL		27
>>   #define HIBMC_DP_SYNC_DELAY(lanes)	((lanes) == 0x2 ? 86 : 46)
>> +#define HIBMC_DP_INT_ENABLE		0xc
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index a921b98dbf50..b2116395b8dd 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   	/* int init */
>>   	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>>   	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> +	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>>   	/* rst */
>>   	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>>   	/* clock enable */
>> @@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   	return 0;
>>   }
>>   
>> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
>> +{
>> +	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> +
>> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> +	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
>> +	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
>> +	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> +	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> +	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> +	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> +	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
>> +}
>> +
>>   void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
>>   {
>>   	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> @@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>>   	return 0;
>>   }
>>   
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp)
>> +{
>> +	dp->dp_dev->link.status.clock_recovered = false;
>> +	dp->dp_dev->link.status.channel_equalized = false;
> Could you please point out, where the link rate and number of lanes are
> reset?

Okay, I'll add comments here. It's mostly used when the connector is out.


>> +}
>> +
>>   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 83a53dae8012..a55d66d53966 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -11,6 +11,7 @@
>>   #include <drm/drm_connector.h>
>>   #include <drm/drm_print.h>
>>   #include <drm/display/drm_dp_helper.h>
>> +#include <drm/drm_client.h>
>>   
>>   struct hibmc_dp_dev;
>>   
>> @@ -49,11 +50,16 @@ struct hibmc_dp {
>>   	void __iomem *mmio;
>>   	struct drm_dp_aux aux;
>>   	struct hibmc_dp_cbar_cfg cfg;
>> +	u32 irq_status;
>> +	u32 hpd_status;
>> +	struct drm_client_dev client;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>>   int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>>   void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>>   void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp);
>> +void hibmc_dp_hpd_cfg(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 a7f611e82f73..40a3ebb8ac4b 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -9,10 +9,13 @@
>>   #include <drm/drm_modes.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_edid.h>
>> +#include <drm/drm_client.h>
>>   
>>   #include "hibmc_drm_drv.h"
>>   #include "dp/dp_hw.h"
>>   
>> +#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>> +
>>   static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>   {
>>   	struct hibmc_dp *dp = to_hibmc_dp(connector);
>> @@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>>   	.atomic_disable = hibmc_dp_encoder_disable,
>>   };
>>   
>> +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);
>> +	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");
>> +		priv->dp.hpd_status = 1;
>> +	} else {
>> +		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
>> +		priv->dp.hpd_status = 0;
>> +	}
>> +
>> +	if (dev->registered)
>> +		drm_connector_helper_hpd_irq_event(&priv->dp.connector);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>> +{
>> +	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>> +	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>> +	int ret;
>> +
>> +	if (dp->hpd_status) {
>> +		hibmc_dp_hpd_cfg(&priv->dp);
>> +		ret = hibmc_dp_prepare(dp, mode);
>> +		if (ret)
>> +			return ret;
>> +
>> +		hibmc_dp_display_en(dp, true);
>> +	} else {
>> +		hibmc_dp_display_en(dp, false);
>> +		hibmc_dp_reset_link(&priv->dp);
>> +	}
> If I understand this correctly, you are using a separate drm_client to
> enable and disable the link & display. Why is it necessary? Existing
> drm_clients and userspace compositors use drm framework, they should be
> able to turn the display on and off as required.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
>> +	.hotplug = hibmc_dp_hpd_event,
>> +	.unregister = drm_client_release,
>> +};
>> +
>>   int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   {
>>   	struct drm_device *dev = &priv->dev;
>> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   
>>   	drm_connector_attach_encoder(connector, encoder);
>>   
>> +	ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_client_register(&dp->client);
>> +
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index bc89e4b9f4e3..daed1330b961 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>>   
>>   void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>   
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
>> +
>>   #endif
>> -- 
>> 2.33.0
>>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>> +{
>> +	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>> +	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>> +	int ret;
>> +
>> +	if (dp->hpd_status) {
>> +		hibmc_dp_hpd_cfg(&priv->dp);
>> +		ret = hibmc_dp_prepare(dp, mode);
>> +		if (ret)
>> +			return ret;
>> +
>> +		hibmc_dp_display_en(dp, true);
>> +	} else {
>> +		hibmc_dp_display_en(dp, false);
>> +		hibmc_dp_reset_link(&priv->dp);
>> +	}
> If I understand this correctly, you are using a separate drm_client to
> enable and disable the link & display. Why is it necessary? Existing
> drm_clients and userspace compositors use drm framework, they should be
> able to turn the display on and off as required.
>
Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
the different video modes into DP registers.


>> +
>> +	return 0;
>> +}
>> +
>> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
>> +	.hotplug = hibmc_dp_hpd_event,
>> +	.unregister = drm_client_release,
>> +};
>> +
>>   int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   {
>>   	struct drm_device *dev = &priv->dev;
>> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   
>>   	drm_connector_attach_encoder(connector, encoder);
>>   
>> +	ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_client_register(&dp->client);
>> +
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index bc89e4b9f4e3..daed1330b961 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>>   
>>   void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>   
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
>> +
>>   #endif
>> -- 
>> 2.33.0
>>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > +{
> > > +	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > +	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > +	int ret;
> > > +
> > > +	if (dp->hpd_status) {
> > > +		hibmc_dp_hpd_cfg(&priv->dp);
> > > +		ret = hibmc_dp_prepare(dp, mode);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		hibmc_dp_display_en(dp, true);
> > > +	} else {
> > > +		hibmc_dp_display_en(dp, false);
> > > +		hibmc_dp_reset_link(&priv->dp);
> > > +	}
> > If I understand this correctly, you are using a separate drm_client to
> > enable and disable the link & display. Why is it necessary? Existing
> > drm_clients and userspace compositors use drm framework, they should be
> > able to turn the display on and off as required.
> > 
> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> the different video modes into DP registers.

Why? The link training and mode programming should happen during
pre_enable / enable stage (legacy or atomic).

> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>> +{
>>>> +	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>> +	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>> +	int ret;
>>>> +
>>>> +	if (dp->hpd_status) {
>>>> +		hibmc_dp_hpd_cfg(&priv->dp);
>>>> +		ret = hibmc_dp_prepare(dp, mode);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		hibmc_dp_display_en(dp, true);
>>>> +	} else {
>>>> +		hibmc_dp_display_en(dp, false);
>>>> +		hibmc_dp_reset_link(&priv->dp);
>>>> +	}
>>> If I understand this correctly, you are using a separate drm_client to
>>> enable and disable the link & display. Why is it necessary? Existing
>>> drm_clients and userspace compositors use drm framework, they should be
>>> able to turn the display on and off as required.
>>>
>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>> the different video modes into DP registers.
> Why? The link training and mode programming should happen during
> pre_enable / enable stage (legacy or atomic).

Hi Dmitry,

Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.


>>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
> > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> >>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> >>>> +{
> >>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> >>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> >>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> >>>> +  int ret;
> >>>> +
> >>>> +  if (dp->hpd_status) {
> >>>> +          hibmc_dp_hpd_cfg(&priv->dp);
> >>>> +          ret = hibmc_dp_prepare(dp, mode);
> >>>> +          if (ret)
> >>>> +                  return ret;
> >>>> +
> >>>> +          hibmc_dp_display_en(dp, true);
> >>>> +  } else {
> >>>> +          hibmc_dp_display_en(dp, false);
> >>>> +          hibmc_dp_reset_link(&priv->dp);
> >>>> +  }
> >>> If I understand this correctly, you are using a separate drm_client to
> >>> enable and disable the link & display. Why is it necessary? Existing
> >>> drm_clients and userspace compositors use drm framework, they should be
> >>> able to turn the display on and off as required.
> >>>
> >> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> >> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> >> the different video modes into DP registers.
> > Why? The link training and mode programming should happen during
> > pre_enable / enable stage (legacy or atomic).
>
> Hi Dmitry,
>
> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I

It should be userspace, who triggers the enable/disable (or it should
be the in-kernel fbdev / fbcon, which interface through the generic
drm_fbdev client).

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>> +{
>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>> +  int ret;
>>>>>> +
>>>>>> +  if (dp->hpd_status) {
>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>> +          if (ret)
>>>>>> +                  return ret;
>>>>>> +
>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>> +  } else {
>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>> +  }
>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>> able to turn the display on and off as required.
>>>>>
>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>> the different video modes into DP registers.
>>> Why? The link training and mode programming should happen during
>>> pre_enable / enable stage (legacy or atomic).
>> Hi Dmitry,
>>
>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> It should be userspace, who triggers the enable/disable (or it should
> be the in-kernel fbdev / fbcon, which interface through the generic
> drm_fbdev client).

Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
by user, but it won't call when HPD triggered .

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > +{
> > > > > > > +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > +  int ret;
> > > > > > > +
> > > > > > > +  if (dp->hpd_status) {
> > > > > > > +          hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > +          ret = hibmc_dp_prepare(dp, mode);
> > > > > > > +          if (ret)
> > > > > > > +                  return ret;
> > > > > > > +
> > > > > > > +          hibmc_dp_display_en(dp, true);
> > > > > > > +  } else {
> > > > > > > +          hibmc_dp_display_en(dp, false);
> > > > > > > +          hibmc_dp_reset_link(&priv->dp);
> > > > > > > +  }
> > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > able to turn the display on and off as required.
> > > > > > 
> > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > the different video modes into DP registers.
> > > > Why? The link training and mode programming should happen during
> > > > pre_enable / enable stage (legacy or atomic).
> > > Hi Dmitry,
> > > 
> > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > It should be userspace, who triggers the enable/disable (or it should
> > be the in-kernel fbdev / fbcon, which interface through the generic
> > drm_fbdev client).
> 
> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> by user, but it won't call when HPD triggered .

- Is HPD even properly delivered to userspace? What kind of compsitor
  are you using? Is .detect working properly and reporting a correct
  plug-in state?

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>> +{
>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>> +  int ret;
>>>>>>>> +
>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>> +          if (ret)
>>>>>>>> +                  return ret;
>>>>>>>> +
>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>> +  } else {
>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>> +  }
>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>> able to turn the display on and off as required.
>>>>>>>
>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>> the different video modes into DP registers.
>>>>> Why? The link training and mode programming should happen during
>>>>> pre_enable / enable stage (legacy or atomic).
>>>> Hi Dmitry,
>>>>
>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>> It should be userspace, who triggers the enable/disable (or it should
>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>> drm_fbdev client).
>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>> by user, but it won't call when HPD triggered .
> - Is HPD even properly delivered to userspace? What kind of compsitor
>    are you using? Is .detect working properly and reporting a correct
>    plug-in state?

Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
I use Xorg, and the display service is GDM.
The .detect is called and the getting modes info is correct.
I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> 
> > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > +{
> > > > > > > > > +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > +  int ret;
> > > > > > > > > +
> > > > > > > > > +  if (dp->hpd_status) {
> > > > > > > > > +          hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > +          ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > +          if (ret)
> > > > > > > > > +                  return ret;
> > > > > > > > > +
> > > > > > > > > +          hibmc_dp_display_en(dp, true);
> > > > > > > > > +  } else {
> > > > > > > > > +          hibmc_dp_display_en(dp, false);
> > > > > > > > > +          hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > +  }
> > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > able to turn the display on and off as required.
> > > > > > > > 
> > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > the different video modes into DP registers.
> > > > > > Why? The link training and mode programming should happen during
> > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > Hi Dmitry,
> > > > > 
> > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > It should be userspace, who triggers the enable/disable (or it should
> > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > drm_fbdev client).
> > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > by user, but it won't call when HPD triggered .
> > - Is HPD even properly delivered to userspace? What kind of compsitor
> >    are you using? Is .detect working properly and reporting a correct
> >    plug-in state?
> 
> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> I use Xorg, and the display service is GDM.
> The .detect is called and the getting modes info is correct.
> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.

You can go to the display settings in GDM. It would be interesting to
observe if it notes the second monitor or not. Last, but not least, you
can use a simple tool like 'xrandr' under your XOrg session to set the
display resolution.

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>> +{
>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>> +  int ret;
>>>>>>>>>> +
>>>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>> +          if (ret)
>>>>>>>>>> +                  return ret;
>>>>>>>>>> +
>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>>>> +  } else {
>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>> +  }
>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>
>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>> the different video modes into DP registers.
>>>>>>> Why? The link training and mode programming should happen during
>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>> drm_fbdev client).
>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>> by user, but it won't call when HPD triggered .
>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>     are you using? Is .detect working properly and reporting a correct
>>>     plug-in state?
>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>> I use Xorg, and the display service is GDM.
>> The .detect is called and the getting modes info is correct.
>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> You can go to the display settings in GDM. It would be interesting to
> observe if it notes the second monitor or not. Last, but not least, you
> can use a simple tool like 'xrandr' under your XOrg session to set the
> display resolution.

Thank you for your advice!
Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
So do I need to clear the vga connector, if dp is plugged in?
And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
but there are errs when set some low resolutions.

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> 
> > On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> > > > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > > > +{
> > > > > > > > > > > +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > > > +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > > > +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > > > +  int ret;
> > > > > > > > > > > +
> > > > > > > > > > > +  if (dp->hpd_status) {
> > > > > > > > > > > +          hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > > > +          ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > > > +          if (ret)
> > > > > > > > > > > +                  return ret;
> > > > > > > > > > > +
> > > > > > > > > > > +          hibmc_dp_display_en(dp, true);
> > > > > > > > > > > +  } else {
> > > > > > > > > > > +          hibmc_dp_display_en(dp, false);
> > > > > > > > > > > +          hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > > > +  }
> > > > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > > > able to turn the display on and off as required.
> > > > > > > > > > 
> > > > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > > > the different video modes into DP registers.
> > > > > > > > Why? The link training and mode programming should happen during
> > > > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > > > Hi Dmitry,
> > > > > > > 
> > > > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > > > It should be userspace, who triggers the enable/disable (or it should
> > > > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > > > drm_fbdev client).
> > > > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > > > by user, but it won't call when HPD triggered .
> > > > - Is HPD even properly delivered to userspace? What kind of compsitor
> > > >     are you using? Is .detect working properly and reporting a correct
> > > >     plug-in state?
> > > Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> > > this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> > > I use Xorg, and the display service is GDM.
> > > The .detect is called and the getting modes info is correct.
> > > I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> > You can go to the display settings in GDM. It would be interesting to
> > observe if it notes the second monitor or not. Last, but not least, you
> > can use a simple tool like 'xrandr' under your XOrg session to set the
> > display resolution.
> 
> Thank you for your advice!
> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> So do I need to clear the vga connector, if dp is plugged in?

Unless your hardware can not manage two outputs at the same time, no,
you don't have to. Just check how it behaves on x86 systems. Ideally
your driver should have the same behaviour.

> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> but there are errs when set some low resolutions.

That's a separate topic, most likely related to timing or to some other
issues. You can fix that separately (but please do, switching modes
should work).

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>> +  int ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>> +          if (ret)
>>>>>>>>>>>> +                  return ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>>>>>> +  } else {
>>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>> +  }
>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>
>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>> drm_fbdev client).
>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>> by user, but it won't call when HPD triggered .
>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>      are you using? Is .detect working properly and reporting a correct
>>>>>      plug-in state?
>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>> I use Xorg, and the display service is GDM.
>>>> The .detect is called and the getting modes info is correct.
>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>> You can go to the display settings in GDM. It would be interesting to
>>> observe if it notes the second monitor or not. Last, but not least, you
>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>> display resolution.
>> Thank you for your advice!
>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>> So do I need to clear the vga connector, if dp is plugged in?
> Unless your hardware can not manage two outputs at the same time, no,
> you don't have to. Just check how it behaves on x86 systems. Ideally
> your driver should have the same behaviour.

Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?

And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.

>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>> but there are errs when set some low resolutions.
> That's a separate topic, most likely related to timing or to some other
> issues. You can fix that separately (but please do, switching modes
> should work).

Okay!

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
>
> > On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> >>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> >>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> >>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> >>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> >>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> >>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> >>>>>>>>>>>> +  int ret;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +  if (dp->hpd_status) {
> >>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
> >>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
> >>>>>>>>>>>> +          if (ret)
> >>>>>>>>>>>> +                  return ret;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
> >>>>>>>>>>>> +  } else {
> >>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
> >>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
> >>>>>>>>>>>> +  }
> >>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
> >>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
> >>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
> >>>>>>>>>>> able to turn the display on and off as required.
> >>>>>>>>>>>
> >>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> >>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> >>>>>>>>>> the different video modes into DP registers.
> >>>>>>>>> Why? The link training and mode programming should happen during
> >>>>>>>>> pre_enable / enable stage (legacy or atomic).
> >>>>>>>> Hi Dmitry,
> >>>>>>>>
> >>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> >>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> >>>>>>> It should be userspace, who triggers the enable/disable (or it should
> >>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
> >>>>>>> drm_fbdev client).
> >>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> >>>>>> by user, but it won't call when HPD triggered .
> >>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
> >>>>>      are you using? Is .detect working properly and reporting a correct
> >>>>>      plug-in state?
> >>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> >>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> >>>> I use Xorg, and the display service is GDM.
> >>>> The .detect is called and the getting modes info is correct.
> >>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> >>> You can go to the display settings in GDM. It would be interesting to
> >>> observe if it notes the second monitor or not. Last, but not least, you
> >>> can use a simple tool like 'xrandr' under your XOrg session to set the
> >>> display resolution.
> >> Thank you for your advice!
> >> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> >> So do I need to clear the vga connector, if dp is plugged in?
> > Unless your hardware can not manage two outputs at the same time, no,
> > you don't have to. Just check how it behaves on x86 systems. Ideally
> > your driver should have the same behaviour.
>
> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?

I think registering a single CRTC is a correct way. Then it is logical
that there is no mode set on the DP when you connect it. The userspace
can not output any data. However if you disconnect VGA and connect DP
then it should become active and should output your desktop
environment.

>
> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>
> >> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> >> but there are errs when set some low resolutions.
> > That's a separate topic, most likely related to timing or to some other
> > issues. You can fix that separately (but please do, switching modes
> > should work).
>
> Okay!
>
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>
>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>> +  int ret;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>> +          if (ret)
>>>>>>>>>>>>>> +                  return ret;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>> +  } else {
>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>> +  }
>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>
>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>> drm_fbdev client).
>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>       are you using? Is .detect working properly and reporting a correct
>>>>>>>       plug-in state?
>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>> I use Xorg, and the display service is GDM.
>>>>>> The .detect is called and the getting modes info is correct.
>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>> display resolution.
>>>> Thank you for your advice!
>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>> So do I need to clear the vga connector, if dp is plugged in?
>>> Unless your hardware can not manage two outputs at the same time, no,
>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>> your driver should have the same behaviour.
>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> I think registering a single CRTC is a correct way. Then it is logical
> that there is no mode set on the DP when you connect it. The userspace
> can not output any data. However if you disconnect VGA and connect DP
> then it should become active and should output your desktop
> environment.

Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
userapce will active and enanble DP, right?


>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>
>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>> but there are errs when set some low resolutions.
>>> That's a separate topic, most likely related to timing or to some other
>>> issues. You can fix that separately (but please do, switching modes
>>> should work).
>> Okay!
>>
>>
>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
> 
> > On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > 
> > > > On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> > > > > > On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> > > > > > > > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > > > > > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > > > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > > > > > > > +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > > > > > > > +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > > > > > > > +  int ret;
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > +  if (dp->hpd_status) {
> > > > > > > > > > > > > > > +          hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > > > > > > > +          ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > > > > > > > +          if (ret)
> > > > > > > > > > > > > > > +                  return ret;
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > +          hibmc_dp_display_en(dp, true);
> > > > > > > > > > > > > > > +  } else {
> > > > > > > > > > > > > > > +          hibmc_dp_display_en(dp, false);
> > > > > > > > > > > > > > > +          hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > > > > > > > +  }
> > > > > > > > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > > > > > > > able to turn the display on and off as required.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > > > > > > > the different video modes into DP registers.
> > > > > > > > > > > > Why? The link training and mode programming should happen during
> > > > > > > > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > 
> > > > > > > > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > > > > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > > > > > > > It should be userspace, who triggers the enable/disable (or it should
> > > > > > > > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > > > > > > > drm_fbdev client).
> > > > > > > > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > > > > > > > by user, but it won't call when HPD triggered .
> > > > > > > > - Is HPD even properly delivered to userspace? What kind of compsitor
> > > > > > > >       are you using? Is .detect working properly and reporting a correct
> > > > > > > >       plug-in state?
> > > > > > > Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> > > > > > > this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> > > > > > > I use Xorg, and the display service is GDM.
> > > > > > > The .detect is called and the getting modes info is correct.
> > > > > > > I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> > > > > > You can go to the display settings in GDM. It would be interesting to
> > > > > > observe if it notes the second monitor or not. Last, but not least, you
> > > > > > can use a simple tool like 'xrandr' under your XOrg session to set the
> > > > > > display resolution.
> > > > > Thank you for your advice!
> > > > > Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> > > > > So do I need to clear the vga connector, if dp is plugged in?
> > > > Unless your hardware can not manage two outputs at the same time, no,
> > > > you don't have to. Just check how it behaves on x86 systems. Ideally
> > > > your driver should have the same behaviour.
> > > Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> > > with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> > I think registering a single CRTC is a correct way. Then it is logical
> > that there is no mode set on the DP when you connect it. The userspace
> > can not output any data. However if you disconnect VGA and connect DP
> > then it should become active and should output your desktop
> > environment.
> 
> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
> userapce will active and enanble DP, right?

Yes.

> 
> 
> > > And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
> > > 
> > > > > And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> > > > > but there are errs when set some low resolutions.
> > > > That's a separate topic, most likely related to timing or to some other
> > > > issues. You can fix that separately (but please do, switching modes
> > > > should work).
> > > Okay!
> > > 
> > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>>>> +  int ret;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>>>> +          if (ret)
>>>>>>>>>>>>>>>> +                  return ret;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>>>> +  } else {
>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>>>> +  }
>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>
>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>>>> drm_fbdev client).
>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>>>        are you using? Is .detect working properly and reporting a correct
>>>>>>>>>        plug-in state?
>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>>>> I use Xorg, and the display service is GDM.
>>>>>>>> The .detect is called and the getting modes info is correct.
>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>>>> display resolution.
>>>>>> Thank you for your advice!
>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>>>> So do I need to clear the vga connector, if dp is plugged in?
>>>>> Unless your hardware can not manage two outputs at the same time, no,
>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>>>> your driver should have the same behaviour.
>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
>>> I think registering a single CRTC is a correct way. Then it is logical
>>> that there is no mode set on the DP when you connect it. The userspace
>>> can not output any data. However if you disconnect VGA and connect DP
>>> then it should become active and should output your desktop
>>> environment.
>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
>> userapce will active and enanble DP, right?
> Yes.

I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.

And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?


>>
>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>>>
>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>>>> but there are errs when set some low resolutions.
>>>>> That's a separate topic, most likely related to timing or to some other
>>>>> issues. You can fix that separately (but please do, switching modes
>>>>> should work).
>>>> Okay!
>>>>
>>>>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
> > On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
> > > > On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> > > > > > > > On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> > > > > > > > > > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > > > > > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > > +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > > > > > > > > > +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > > > > > > > > > +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > > > > > > > > > +  int ret;
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > +  if (dp->hpd_status) {
> > > > > > > > > > > > > > > > > +          hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > > > > > > > > > +          ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > > > > > > > > > +          if (ret)
> > > > > > > > > > > > > > > > > +                  return ret;
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > +          hibmc_dp_display_en(dp, true);
> > > > > > > > > > > > > > > > > +  } else {
> > > > > > > > > > > > > > > > > +          hibmc_dp_display_en(dp, false);
> > > > > > > > > > > > > > > > > +          hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > > > > > > > > > +  }
> > > > > > > > > > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > > > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > > > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > > > > > > > > > able to turn the display on and off as required.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > > > > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > > > > > > > > > the different video modes into DP registers.
> > > > > > > > > > > > > > Why? The link training and mode programming should happen during
> > > > > > > > > > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > > > > > > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > > > > > > > > > It should be userspace, who triggers the enable/disable (or it should
> > > > > > > > > > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > > > > > > > > > drm_fbdev client).
> > > > > > > > > > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > > > > > > > > > by user, but it won't call when HPD triggered .
> > > > > > > > > > - Is HPD even properly delivered to userspace? What kind of compsitor
> > > > > > > > > >        are you using? Is .detect working properly and reporting a correct
> > > > > > > > > >        plug-in state?
> > > > > > > > > Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> > > > > > > > > this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> > > > > > > > > I use Xorg, and the display service is GDM.
> > > > > > > > > The .detect is called and the getting modes info is correct.
> > > > > > > > > I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> > > > > > > > You can go to the display settings in GDM. It would be interesting to
> > > > > > > > observe if it notes the second monitor or not. Last, but not least, you
> > > > > > > > can use a simple tool like 'xrandr' under your XOrg session to set the
> > > > > > > > display resolution.
> > > > > > > Thank you for your advice!
> > > > > > > Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> > > > > > > So do I need to clear the vga connector, if dp is plugged in?
> > > > > > Unless your hardware can not manage two outputs at the same time, no,
> > > > > > you don't have to. Just check how it behaves on x86 systems. Ideally
> > > > > > your driver should have the same behaviour.
> > > > > Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> > > > > with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> > > > I think registering a single CRTC is a correct way. Then it is logical
> > > > that there is no mode set on the DP when you connect it. The userspace
> > > > can not output any data. However if you disconnect VGA and connect DP
> > > > then it should become active and should output your desktop
> > > > environment.
> > > Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
> > > userapce will active and enanble DP, right?
> > Yes.
> 
> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.

Huh? You should implement hibmc_connector_funcs.detect() or
.detect_ctx() to report VGA connector's status. Use
ast_vga_connector_helper_detect_ctx() as an inspiration.

> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?

drm_connector_cleanup() should only be callsed if the connector is going
to be destroyed. You should not be callign that function.

> 
> 
> > > 
> > > > > And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
> > > > > 
> > > > > > > And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> > > > > > > but there are errs when set some low resolutions.
> > > > > > That's a separate topic, most likely related to timing or to some other
> > > > > > issues. You can fix that separately (but please do, switching modes
> > > > > > should work).
> > > > > Okay!
> > > > > 
> > > > > 

-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
>>> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>>>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>>>>>> +  int ret;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>>>>>> +          if (ret)
>>>>>>>>>>>>>>>>>> +                  return ret;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>>>>>> +  } else {
>>>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>>>>>> +  }
>>>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>>>>>> drm_fbdev client).
>>>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>>>>>         are you using? Is .detect working properly and reporting a correct
>>>>>>>>>>>         plug-in state?
>>>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>>>>>> I use Xorg, and the display service is GDM.
>>>>>>>>>> The .detect is called and the getting modes info is correct.
>>>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>>>>>> display resolution.
>>>>>>>> Thank you for your advice!
>>>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>>>>>> So do I need to clear the vga connector, if dp is plugged in?
>>>>>>> Unless your hardware can not manage two outputs at the same time, no,
>>>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>>>>>> your driver should have the same behaviour.
>>>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>>>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
>>>>> I think registering a single CRTC is a correct way. Then it is logical
>>>>> that there is no mode set on the DP when you connect it. The userspace
>>>>> can not output any data. However if you disconnect VGA and connect DP
>>>>> then it should become active and should output your desktop
>>>>> environment.
>>>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
>>>> userapce will active and enanble DP, right?
>>> Yes.
>> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
> Huh? You should implement hibmc_connector_funcs.detect() or
> .detect_ctx() to report VGA connector's status. Use
> ast_vga_connector_helper_detect_ctx() as an inspiration.

Okay, thanks for your correcting, I'll try to add drm_connector_helper_detect_from_ddc() in VGA's detect_ctx.
And also, I wanna make sure that do I need to unplug the VGA monitor manually when I plug in DP, or just
set the status of VGA connector to disconnected in interrupt handler.


>> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
> drm_connector_cleanup() should only be callsed if the connector is going
> to be destroyed. You should not be callign that function.
>
>>
>>>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>>>>>
>>>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>>>>>> but there are errs when set some low resolutions.
>>>>>>> That's a separate topic, most likely related to timing or to some other
>>>>>>> issues. You can fix that separately (but please do, switching modes
>>>>>>> should work).
>>>>>> Okay!
>>>>>>
>>>>>>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Dmitry Baryshkov]

On Wed, 5 Mar 2025 at 08:29, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
>
> > On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
> >>> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
> >>>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
> >>>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> >>>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> >>>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> >>>>>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> >>>>>>>>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> >>>>>>>>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> >>>>>>>>>>>>>>>>>> +  int ret;
> >>>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>>> +  if (dp->hpd_status) {
> >>>>>>>>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
> >>>>>>>>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
> >>>>>>>>>>>>>>>>>> +          if (ret)
> >>>>>>>>>>>>>>>>>> +                  return ret;
> >>>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
> >>>>>>>>>>>>>>>>>> +  } else {
> >>>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
> >>>>>>>>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
> >>>>>>>>>>>>>>>>>> +  }
> >>>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
> >>>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
> >>>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
> >>>>>>>>>>>>>>>>> able to turn the display on and off as required.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> >>>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> >>>>>>>>>>>>>>>> the different video modes into DP registers.
> >>>>>>>>>>>>>>> Why? The link training and mode programming should happen during
> >>>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
> >>>>>>>>>>>>>> Hi Dmitry,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> >>>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> >>>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
> >>>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
> >>>>>>>>>>>>> drm_fbdev client).
> >>>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> >>>>>>>>>>>> by user, but it won't call when HPD triggered .
> >>>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
> >>>>>>>>>>>         are you using? Is .detect working properly and reporting a correct
> >>>>>>>>>>>         plug-in state?
> >>>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> >>>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> >>>>>>>>>> I use Xorg, and the display service is GDM.
> >>>>>>>>>> The .detect is called and the getting modes info is correct.
> >>>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> >>>>>>>>> You can go to the display settings in GDM. It would be interesting to
> >>>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
> >>>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
> >>>>>>>>> display resolution.
> >>>>>>>> Thank you for your advice!
> >>>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> >>>>>>>> So do I need to clear the vga connector, if dp is plugged in?
> >>>>>>> Unless your hardware can not manage two outputs at the same time, no,
> >>>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
> >>>>>>> your driver should have the same behaviour.
> >>>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> >>>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> >>>>> I think registering a single CRTC is a correct way. Then it is logical
> >>>>> that there is no mode set on the DP when you connect it. The userspace
> >>>>> can not output any data. However if you disconnect VGA and connect DP
> >>>>> then it should become active and should output your desktop
> >>>>> environment.
> >>>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
> >>>> userapce will active and enanble DP, right?
> >>> Yes.
> >> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
> > Huh? You should implement hibmc_connector_funcs.detect() or
> > .detect_ctx() to report VGA connector's status. Use
> > ast_vga_connector_helper_detect_ctx() as an inspiration.
>
> Okay, thanks for your correcting, I'll try to add drm_connector_helper_detect_from_ddc() in VGA's detect_ctx.
> And also, I wanna make sure that do I need to unplug the VGA monitor manually when I plug in DP, or just
> set the status of VGA connector to disconnected in interrupt handler.

If the HPD reports the status of the DP connector, why do you want to
change the status of the VGA connector?

>
>
> >> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
> > drm_connector_cleanup() should only be callsed if the connector is going
> > to be destroyed. You should not be callign that function.
> >
> >>
> >>>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
> >>>>>>
> >>>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> >>>>>>>> but there are errs when set some low resolutions.
> >>>>>>> That's a separate topic, most likely related to timing or to some other
> >>>>>>> issues. You can fix that separately (but please do, switching modes
> >>>>>>> should work).
> >>>>>> Okay!
> >>>>>>
> >>>>>>



-- 
With best wishes
Dmitry

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Wed, 5 Mar 2025 at 08:29, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>
>>> On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
>>>>> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>>>>>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>>> +  struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>>>>>>>> +  struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>>>>>>>> +  struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>>>>>>>> +  int ret;
>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>> +  if (dp->hpd_status) {
>>>>>>>>>>>>>>>>>>>> +          hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>>>>>>>> +          ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>>>>>>>> +          if (ret)
>>>>>>>>>>>>>>>>>>>> +                  return ret;
>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>>>>>>>> +  } else {
>>>>>>>>>>>>>>>>>>>> +          hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>>>>>>>> +          hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>>>>>>>> +  }
>>>>>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>>>>>>>> drm_fbdev client).
>>>>>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>>>>>>>          are you using? Is .detect working properly and reporting a correct
>>>>>>>>>>>>>          plug-in state?
>>>>>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>>>>>>>> I use Xorg, and the display service is GDM.
>>>>>>>>>>>> The .detect is called and the getting modes info is correct.
>>>>>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>>>>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>>>>>>>> display resolution.
>>>>>>>>>> Thank you for your advice!
>>>>>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>>>>>>>> So do I need to clear the vga connector, if dp is plugged in?
>>>>>>>>> Unless your hardware can not manage two outputs at the same time, no,
>>>>>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>>>>>>>> your driver should have the same behaviour.
>>>>>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>>>>>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
>>>>>>> I think registering a single CRTC is a correct way. Then it is logical
>>>>>>> that there is no mode set on the DP when you connect it. The userspace
>>>>>>> can not output any data. However if you disconnect VGA and connect DP
>>>>>>> then it should become active and should output your desktop
>>>>>>> environment.
>>>>>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
>>>>>> userapce will active and enanble DP, right?
>>>>> Yes.
>>>> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
>>> Huh? You should implement hibmc_connector_funcs.detect() or
>>> .detect_ctx() to report VGA connector's status. Use
>>> ast_vga_connector_helper_detect_ctx() as an inspiration.
>> Okay, thanks for your correcting, I'll try to add drm_connector_helper_detect_from_ddc() in VGA's detect_ctx.
>> And also, I wanna make sure that do I need to unplug the VGA monitor manually when I plug in DP, or just
>> set the status of VGA connector to disconnected in interrupt handler.
> If the HPD reports the status of the DP connector, why do you want to
> change the status of the VGA connector?

I'm not sure how VGA is influenced the userspace active and enable, because you said "disconnect VGA and connect DP then it should become active", so I try to do it.

After just adding VGA's detect, it works good in Xorg session, but when I start GDM, it won't enable in HPD plugged in. It's kind of complicated to me, (different Xorg usages,

  different results). So I wanna send the patches first, and then follow up on this issue whether the configuration of GDM is correct.


>>
>>>> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
>>> drm_connector_cleanup() should only be callsed if the connector is going
>>> to be destroyed. You should not be callign that function.
>>>
>>>>>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>>>>>>>
>>>>>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>>>>>>>> but there are errs when set some low resolutions.
>>>>>>>>> That's a separate topic, most likely related to timing or to some other
>>>>>>>>> issues. You can fix that separately (but please do, switching modes
>>>>>>>>> should work).
>>>>>>>> Okay!
>>>>>>>>
>>>>>>>>
>
>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[Yongbang Shi]

> On Sat, Feb 22, 2025 at 10:51:00AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Enable HPD feature and add its isr and event function. Add a drm client
>> dev and realized the hotplug callback in it.
> What for? There should be no need to add a separate drm client just for
> the hotplug.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
>>    - remove enble_display in ISR, suggested by Dmitry Baryshkov.
>>    - change drm_kms_helper_connector_hotplug_event() to
>>      drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
>>    - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
>>    - remove struct irqs, suggested by Dmitry Baryshkov.
>>    - split this patch into two parts, suggested by Dmitry Baryshkov.
>>    - add a drm client dev to handle HPD event.
>> v1 -> v2:
>>    - optimizing the description in commit message, suggested by Dmitry Baryshkov.
>>    - add mdelay(100) comments, suggested by Dmitry Baryshkov.
>>    - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
>> ---
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |  1 +
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 22 +++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  6 ++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 61 +++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  2 +
>>   5 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> index c5feef8dc27d..08f9e1caf7fc 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> @@ -16,5 +16,6 @@
>>   #define HIBMC_DP_SYNC_EN_MASK		0x3
>>   #define HIBMC_DP_LINK_RATE_CAL		27
>>   #define HIBMC_DP_SYNC_DELAY(lanes)	((lanes) == 0x2 ? 86 : 46)
>> +#define HIBMC_DP_INT_ENABLE		0xc
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index a921b98dbf50..b2116395b8dd 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   	/* int init */
>>   	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>>   	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> +	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>>   	/* rst */
>>   	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>>   	/* clock enable */
>> @@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   	return 0;
>>   }
>>   
>> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
>> +{
>> +	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> +
>> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> +	hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
>> +	writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
>> +	writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> +	writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> +	writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> +	writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> +	writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
>> +}
>> +
>>   void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
>>   {
>>   	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> @@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>>   	return 0;
>>   }
>>   
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp)
>> +{
>> +	dp->dp_dev->link.status.clock_recovered = false;
>> +	dp->dp_dev->link.status.channel_equalized = false;
> Could you please point out, where the link rate and number of lanes are
> reset?

Thanks for your reminder, I will reset link rate and lanes here too.


>> +}
>> +
>>   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 83a53dae8012..a55d66d53966 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -11,6 +11,7 @@
>>   #include <drm/drm_connector.h>
>>   #include <drm/drm_print.h>
>>   #include <drm/display/drm_dp_helper.h>
>> +#include <drm/drm_client.h>
>>   
>>   struct hibmc_dp_dev;
>>   
>> @@ -49,11 +50,16 @@ struct hibmc_dp {
>>   	void __iomem *mmio;
>>   	struct drm_dp_aux aux;
>>   	struct hibmc_dp_cbar_cfg cfg;
>> +	u32 irq_status;
>> +	u32 hpd_status;
>> +	struct drm_client_dev client;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>>   int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>>   void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>>   void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp);
>> +void hibmc_dp_hpd_cfg(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 a7f611e82f73..40a3ebb8ac4b 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -9,10 +9,13 @@
>>   #include <drm/drm_modes.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_edid.h>
>> +#include <drm/drm_client.h>
>>   
>>   #include "hibmc_drm_drv.h"
>>   #include "dp/dp_hw.h"
>>   
>> +#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>> +
>>   static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>   {
>>   	struct hibmc_dp *dp = to_hibmc_dp(connector);
>> @@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>>   	.atomic_disable = hibmc_dp_encoder_disable,
>>   };
>>   
>> +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);
>> +	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");
>> +		priv->dp.hpd_status = 1;
>> +	} else {
>> +		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
>> +		priv->dp.hpd_status = 0;
>> +	}
>> +
>> +	if (dev->registered)
>> +		drm_connector_helper_hpd_irq_event(&priv->dp.connector);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>> +{
>> +	struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>> +	struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>> +	int ret;
>> +
>> +	if (dp->hpd_status) {
>> +		hibmc_dp_hpd_cfg(&priv->dp);
>> +		ret = hibmc_dp_prepare(dp, mode);
>> +		if (ret)
>> +			return ret;
>> +
>> +		hibmc_dp_display_en(dp, true);
>> +	} else {
>> +		hibmc_dp_display_en(dp, false);
>> +		hibmc_dp_reset_link(&priv->dp);
>> +	}
> If I understand this correctly, you are using a separate drm_client to
> enable and disable the link & display. Why is it necessary? Existing
> drm_clients and userspace compositors use drm framework, they should be
> able to turn the display on and off as required.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
>> +	.hotplug = hibmc_dp_hpd_event,
>> +	.unregister = drm_client_release,
>> +};
>> +
>>   int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   {
>>   	struct drm_device *dev = &priv->dev;
>> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   
>>   	drm_connector_attach_encoder(connector, encoder);
>>   
>> +	ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_client_register(&dp->client);
>> +
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index bc89e4b9f4e3..daed1330b961 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>>   
>>   void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>   
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
>> +
>>   #endif
>> -- 
>> 2.33.0
>>

Re: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature

[kernel test robot]

Hi Yongbang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.14-rc3 next-20250221]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yongbang-Shi/drm-hisilicon-hibmc-Restructuring-the-header-dp_reg-h/20250222-110052
base:   linus/master
patch link:    https://lore.kernel.org/r/20250222025102.1519798-8-shiyongbang%40huawei.com
patch subject: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature
config: arm64-randconfig-004-20250223 (https://download.01.org/0day-ci/archive/20250223/202502231304.BCzV4Y8D-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 204dcafec0ecf0db81d420d2de57b02ada6b09ec)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250223/202502231304.BCzV4Y8D-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502231304.BCzV4Y8D-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "drm_client_init" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined!
>> ERROR: modpost: "drm_client_register" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined!
>> ERROR: modpost: "drm_client_release" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 drm-dp 8/8] drm/hisilicon/hibmc: Add MSI irq getting and requesting for HPD

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

To realize HPD feature, request irq for HPD , add its handler function.
We use pci_alloc_irq_vectors() to get our msi irq, because we have two
interrupts now.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  3 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 77 +++++++++++++++----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  3 +
 3 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index 4c388f633081..28d3ffd1a39a 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -99,4 +99,7 @@
 
 #define HIBMC_DP_TIMING_SYNC_CTRL		0xFF0
 
+#define HIBMC_DP_INTSTAT			0x1e0724
+#define HIBMC_DP_INTCLR				0x1e0728
+
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 67d39e258cac..84f81ee63d54 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -29,9 +29,12 @@
 #include "hibmc_drm_regs.h"
 
 #include "dp/dp_serdes.h"
+#include "dp/dp_reg.h"
 
 DEFINE_DRM_GEM_FOPS(hibmc_fops);
 
+static const char *g_irqs_names_map[HIBMC_MAX_VECTORS] = { "vblank", "hpd" };
+
 static irqreturn_t hibmc_interrupt(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *)arg;
@@ -49,6 +52,22 @@ static irqreturn_t hibmc_interrupt(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
+{
+	struct drm_device *dev = (struct drm_device *)arg;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+	u32 status;
+
+	status = readl(priv->mmio + HIBMC_DP_INTSTAT);
+	if (status) {
+		priv->dp.irq_status = status;
+		writel(status, priv->mmio + HIBMC_DP_INTCLR);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 			     struct drm_mode_create_dumb *args)
 {
@@ -248,15 +267,50 @@ static int hibmc_hw_init(struct hibmc_drm_private *priv)
 	return 0;
 }
 
-static int hibmc_unload(struct drm_device *dev)
+static void hibmc_unload(struct drm_device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-
 	drm_atomic_helper_shutdown(dev);
 
-	free_irq(pdev->irq, dev);
-
 	pci_disable_msi(to_pci_dev(dev->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;
+
+	ret = pci_alloc_irq_vectors(pdev, HIBMC_MIN_VECTORS,
+				    HIBMC_MAX_VECTORS, PCI_IRQ_MSI);
+	if (ret < 0) {
+		drm_err(dev, "enabling MSI failed: %d\n", ret);
+		return ret;
+	}
+
+	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)
+			/* PCI devices require shared interrupts. */
+			ret = devm_request_threaded_irq(&pdev->dev, irq,
+							hibmc_dp_interrupt,
+							hibmc_dp_hpd_isr,
+							IRQF_SHARED, name, dev);
+		else
+			ret = devm_request_irq(&pdev->dev, irq, hibmc_interrupt,
+					       IRQF_SHARED, name, dev);
+		if (ret) {
+			drm_err(dev, "install irq failed: %d\n", ret);
+			return ret;
+		}
+	}
 
 	return 0;
 }
@@ -288,15 +342,10 @@ static int hibmc_load(struct drm_device *dev)
 		goto err;
 	}
 
-	ret = pci_enable_msi(pdev);
+	ret = hibmc_msi_init(dev);
 	if (ret) {
-		drm_warn(dev, "enabling MSI failed: %d\n", ret);
-	} else {
-		/* PCI devices require shared interrupts. */
-		ret = request_irq(pdev->irq, hibmc_interrupt, IRQF_SHARED,
-				  dev->driver->name, dev);
-		if (ret)
-			drm_warn(dev, "install irq failed: %d\n", ret);
+		drm_err(dev, "hibmc msi init failed, ret:%d\n", ret);
+		goto err;
 	}
 
 	/* reset all the states of crtc/plane/encoder/connector */
@@ -372,7 +421,7 @@ static void hibmc_pci_remove(struct pci_dev *pdev)
 
 static void hibmc_pci_shutdown(struct pci_dev *pdev)
 {
-	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+	hibmc_pci_remove(pdev);
 }
 
 static const struct pci_device_id hibmc_pci_table[] = {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index daed1330b961..274feabe7df0 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -22,6 +22,9 @@
 
 #include "dp/dp_hw.h"
 
+#define HIBMC_MIN_VECTORS	1
+#define HIBMC_MAX_VECTORS	2
+
 struct hibmc_vdac {
 	struct drm_device *dev;
 	struct drm_encoder encoder;
-- 
2.33.0

Re: [PATCH v3 drm-dp 8/8] drm/hisilicon/hibmc: Add MSI irq getting and requesting for HPD

[Dmitry Baryshkov]

On Sat, Feb 22, 2025 at 10:51:01AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> To realize HPD feature, request irq for HPD , add its handler function.
> We use pci_alloc_irq_vectors() to get our msi irq, because we have two
> interrupts now.
> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  3 +
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 77 +++++++++++++++----
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  3 +
>  3 files changed, 69 insertions(+), 14 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

-- 
With best wishes
Dmitry