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

[PATCH v2 drm-dp 0/4] 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:
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.
---

 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  |  11 +-
 .../gpu/drm/hisilicon/hibmc/dp/dp_config.h    |   2 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 103 ++++++++++++++-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  47 +++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  |  92 +++++++++----
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |   5 +
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    |  74 +++++++++++
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    |  36 +++++
 .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 124 ++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |  88 ++++++++++---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  86 +++++++++---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  21 +++
 14 files changed, 640 insertions(+), 68 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 v2 drm-dp 1/7] drm/hisilicon/hibmc: Add dp phy cfg to adjust serdes rate, voltage and pre-emphasis

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

This serdes is inited and configured for dp, and integrating them into dp
init and dp 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:
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    |  4 +
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 74 +++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    | 36 +++++++++
 5 files changed, 116 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..39fd3687efca 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)
 {
@@ -165,6 +166,9 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 
 	hibmc_dp_aux_init(dp_dev);
 
+	if (hibmc_dp_serdes_init(dp_dev))
+		return -EAGAIN;
+
 	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..66586db2268a
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2024 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])
+{
+	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]) {
+			cfg[i] = DP_SERDES_VOL3_PRE0;
+			drm_warn(dp->dev, "dp serdes cfg beyonds the allowable range\n");
+		}
+
+		/* 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_err(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_err(dp->dev, "dp serdes rate switching failed\n");
+		return -EAGAIN;
+	}
+
+	if (rate < DP_SERDES_BW_8_1)
+		drm_warn(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..57f7f054f2b7
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
@@ -0,0 +1,36 @@
+/* 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)
+
+#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_BW_5_4		0x2
+#define DP_SERDES_BW_2_7		0x1
+#define DP_SERDES_BW_1_62		0x0
+
+#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 v2 drm-dp 1/7] drm/hisilicon/hibmc: Add dp phy cfg to adjust serdes rate, voltage and pre-emphasis

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:53PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> This serdes is inited and configured for dp, and integrating them into dp
> init and dp 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:
> 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.

Nit: subject still mentions phy.

Nit #2: The s/phy/serdes/ doesn't change the fact that you are still
doing PHY-like programming. As such, please mention in the commit
message that using PHY subsystem is impossible for you.

> ---
>  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    |  4 +
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 74 +++++++++++++++++++
>  .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    | 36 +++++++++
>  5 files changed, 116 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..39fd3687efca 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)
>  {
> @@ -165,6 +166,9 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>  
>  	hibmc_dp_aux_init(dp_dev);
>  
> +	if (hibmc_dp_serdes_init(dp_dev))
> +		return -EAGAIN;

Can you simply propagate the return value here?

> +
>  	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..66586db2268a
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (c) 2024 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])
> +{
> +	u32 serdes_tx_cfg[4][4] = { {DP_SERDES_VOL0_PRE0, DP_SERDES_VOL0_PRE1,

static const

> +				     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]) {
> +			cfg[i] = DP_SERDES_VOL3_PRE0;
> +			drm_warn(dp->dev, "dp serdes cfg beyonds the allowable range\n");

Is it something that you should be mitigating? Can you return -EINVAL
instead?

> +		}
> +
> +		/* 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_err(dp->dev, "dp serdes cfg failed\n");

drm_dbg?

> +		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_err(dp->dev, "dp serdes rate switching failed\n");
> +		return -EAGAIN;
> +	}
> +
> +	if (rate < DP_SERDES_BW_8_1)
> +		drm_warn(dp->dev, "reducing serdes rate to :%d\n",
> +			 rate ? rate * HIBMC_DP_LINK_RATE_CAL * 10 : 162);

drm_dbg

> +
> +	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..57f7f054f2b7
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
> @@ -0,0 +1,36 @@
> +/* 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)
> +
> +#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

These need some explanation.

> +#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
> +
> +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 v2 drm-dp 1/7] drm/hisilicon/hibmc: Add dp phy cfg to adjust serdes rate, voltage and pre-emphasis

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:53PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> This serdes is inited and configured for dp, and integrating them into dp
>> init and dp 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:
>> 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.
> Nit: subject still mentions phy.
>
> Nit #2: The s/phy/serdes/ doesn't change the fact that you are still
> doing PHY-like programming. As such, please mention in the commit
> message that using PHY subsystem is impossible for you.

Hi Dmitry,

Thanks for the review. Our phy 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.

I will add in commit too.


Sincerely,

Baihan Li


>> ---
>>   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    |  4 +
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.c    | 74 +++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/dp/dp_serdes.h    | 36 +++++++++
>>   5 files changed, 116 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..39fd3687efca 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)
>>   {
>> @@ -165,6 +166,9 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   
>>   	hibmc_dp_aux_init(dp_dev);
>>   
>> +	if (hibmc_dp_serdes_init(dp_dev))
>> +		return -EAGAIN;
> Can you simply propagate the return value here?
>
>> +
>>   	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..66586db2268a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.c
>> @@ -0,0 +1,74 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2024 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])
>> +{
>> +	u32 serdes_tx_cfg[4][4] = { {DP_SERDES_VOL0_PRE0, DP_SERDES_VOL0_PRE1,
> static const
>
>> +				     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]) {
>> +			cfg[i] = DP_SERDES_VOL3_PRE0;
>> +			drm_warn(dp->dev, "dp serdes cfg beyonds the allowable range\n");
> Is it something that you should be mitigating? Can you return -EINVAL
> instead?

Yeah, I just consider that if it happens we still have chances to try link training.

Alright, I'll just return err.


>> +		}
>> +
>> +		/* 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_err(dp->dev, "dp serdes cfg failed\n");
> drm_dbg?
>
>> +		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_err(dp->dev, "dp serdes rate switching failed\n");
>> +		return -EAGAIN;
>> +	}
>> +
>> +	if (rate < DP_SERDES_BW_8_1)
>> +		drm_warn(dp->dev, "reducing serdes rate to :%d\n",
>> +			 rate ? rate * HIBMC_DP_LINK_RATE_CAL * 10 : 162);
> drm_dbg
>
>> +
>> +	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..57f7f054f2b7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_serdes.h
>> @@ -0,0 +1,36 @@
>> +/* 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)
>> +
>> +#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
> These need some explanation.
>
>> +#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
>> +
>> +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 v2 drm-dp 2/7] 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. We 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:
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    |  6 +++-
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
 5 files changed, 38 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 39fd3687efca..ee1ff205ff56 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -3,6 +3,7 @@
 
 #include <linux/io.h>
 #include <linux/delay.h>
+#include <drm/drm_managed.h>
 #include "dp_config.h"
 #include "dp_comm.h"
 #include "dp_reg.h"
@@ -73,6 +74,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)
@@ -170,7 +174,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
 		return -EAGAIN;
 
 	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..43a4b656493f 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_warn(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 4a515c726d52..f2fa9807d8ab 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -72,5 +72,6 @@
 #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_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index e6de6d5edf6b..bade693d9730 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
+#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
 
 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 v2 drm-dp 2/7] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:54PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Add dp serdes cfg in link training process, and related adapting
> and modificating. We change some init values about training,

Imperative language, please. Use 'change', not 'we change'.

> 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:
> 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    |  6 +++-
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
>  5 files changed, 38 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 39fd3687efca..ee1ff205ff56 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -3,6 +3,7 @@
>  
>  #include <linux/io.h>
>  #include <linux/delay.h>
> +#include <drm/drm_managed.h>
>  #include "dp_config.h"
>  #include "dp_comm.h"
>  #include "dp_reg.h"
> @@ -73,6 +74,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)
> @@ -170,7 +174,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>  		return -EAGAIN;
>  
>  	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..43a4b656493f 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);

This looks like:

if (dp->link.cap.link_rate == DP_LINK_BW_1_62)
    return -EINVAL;

dp->link.cap.link_rate++;
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_warn(dp->dev, "dp link training reduce to 1 lane\n");

drm_dbg

>  		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 4a515c726d52..f2fa9807d8ab 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -72,5 +72,6 @@
>  #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_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)

Please consider restructuring the header so that it becomes more
obvious, which register are these masks for.

>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index e6de6d5edf6b..bade693d9730 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
> +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c

Unnecessary whitespace change.

Why HIBMC_DP_HOST_SERDES_CTRL is not in the dp_reg.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
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 drm-dp 2/7] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:54PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Add dp serdes cfg in link training process, and related adapting
>> and modificating. We change some init values about training,
> Imperative language, please. Use 'change', not 'we change'.
>
>> 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:
>> 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    |  6 +++-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
>>   5 files changed, 38 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 39fd3687efca..ee1ff205ff56 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -3,6 +3,7 @@
>>   
>>   #include <linux/io.h>
>>   #include <linux/delay.h>
>> +#include <drm/drm_managed.h>
>>   #include "dp_config.h"
>>   #include "dp_comm.h"
>>   #include "dp_reg.h"
>> @@ -73,6 +74,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)
>> @@ -170,7 +174,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>>   		return -EAGAIN;
>>   
>>   	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..43a4b656493f 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);
> This looks like:
>
> if (dp->link.cap.link_rate == DP_LINK_BW_1_62)
>      return -EINVAL;
>
> dp->link.cap.link_rate++;
> return hibmc_dp_serdes_rate_switch(rate, dp);

We need map DP_LINK_BW_2_7 to DP_SERDES_BW's value firstly, which is for our SERDES' register cfg.
This value is not like 0x6, 0xa, etc.


>>   }
>>   
>>   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_warn(dp->dev, "dp link training reduce to 1 lane\n");
> drm_dbg
>
>>   		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 4a515c726d52..f2fa9807d8ab 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -72,5 +72,6 @@
>>   #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_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
> Please consider restructuring the header so that it becomes more
> obvious, which register are these masks for.
>
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index e6de6d5edf6b..bade693d9730 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
>> +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
> Unnecessary whitespace change.
>
> Why HIBMC_DP_HOST_SERDES_CTRL is not in the dp_reg.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 v2 drm-dp 2/7] drm/hisilicon/hibmc: Add dp serdes cfg in dp process

[Dmitry Baryshkov]

On Wed, Feb 19, 2025 at 03:39:10PM +0800, Yongbang Shi wrote:
> > On Mon, Feb 10, 2025 at 10:49:54PM +0800, Yongbang Shi wrote:
> > > From: Baihan Li <libaihan@huawei.com>
> > > 
> > > Add dp serdes cfg in link training process, and related adapting
> > > and modificating. We change some init values about training,
> > Imperative language, please. Use 'change', not 'we change'.
> > 
> > > 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:
> > > 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    |  6 +++-
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  | 33 ++++++++++++++++---
> > >   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  1 +
> > >   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  8 ++---
> > >   5 files changed, 38 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 39fd3687efca..ee1ff205ff56 100644
> > > --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> > > +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> > > @@ -3,6 +3,7 @@
> > >   #include <linux/io.h>
> > >   #include <linux/delay.h>
> > > +#include <drm/drm_managed.h>
> > >   #include "dp_config.h"
> > >   #include "dp_comm.h"
> > >   #include "dp_reg.h"
> > > @@ -73,6 +74,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)
> > > @@ -170,7 +174,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
> > >   		return -EAGAIN;
> > >   	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..43a4b656493f 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);
> > This looks like:
> > 
> > if (dp->link.cap.link_rate == DP_LINK_BW_1_62)
> >      return -EINVAL;
> > 
> > dp->link.cap.link_rate++;
> > return hibmc_dp_serdes_rate_switch(rate, dp);
> 
> We need map DP_LINK_BW_2_7 to DP_SERDES_BW's value firstly, which is for our SERDES' register cfg.
> This value is not like 0x6, 0xa, etc.

Ack

> 
> 
> > >   }
> > >   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_warn(dp->dev, "dp link training reduce to 1 lane\n");
> > drm_dbg
> > 
> > >   		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 4a515c726d52..f2fa9807d8ab 100644
> > > --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> > > +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> > > @@ -72,5 +72,6 @@
> > >   #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_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
> > Please consider restructuring the header so that it becomes more
> > obvious, which register are these masks for.
> > 
> > >   #endif
> > > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> > > index e6de6d5edf6b..bade693d9730 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
> > > +#define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
> > Unnecessary whitespace change.
> > 
> > Why HIBMC_DP_HOST_SERDES_CTRL is not in the dp_reg.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
> > > 

-- 
With best wishes
Dmitry

[PATCH v2 drm-dp 3/7] drm/hisilicon/hibmc: Getting connector info and edid by using aux channel

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Registering drm_aux and using it to get connector edid with drm functions.
Refactoring some structs to fit aux's register framework. We need change
a lot about getting member of drm_aux, because we change it's postions from
struct hibmc_dp_dev to struct hibmc_dp.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
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   | 16 ++++++----
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  7 +++--
 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 +++++++-------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 30 +++++++++++--------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  5 ++++
 7 files changed, 52 insertions(+), 32 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..e0bb9b14d9d8 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,16 @@ 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;
+	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/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index e7746bc4b592..d613da8b544c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -11,8 +11,11 @@
 #include <linux/kernel.h>
 #include <linux/bitfield.h>
 #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 +35,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 +61,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 ee1ff205ff56..77f02d5151f7 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);
 
 	if (hibmc_dp_serdes_init(dp_dev))
 		return -EAGAIN;
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 43a4b656493f..4a99a9b7e3c4 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");
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 603d6b198a54..795c5b1a6b99 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -13,20 +13,24 @@
 #include "hibmc_drm_drv.h"
 #include "dp/dp_hw.h"
 
-static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
-{
-	int count;
+static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
+	.get_modes = drm_connector_helper_get_modes,
+	.detect_ctx = drm_connector_helper_detect_from_ddc,
+};
 
-	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
+static int hibmc_dp_late_register(struct drm_connector *connector)
+{
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
 
-	return count;
+	return drm_dp_aux_register(&dp->aux);
 }
 
-static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
-	.get_modes = hibmc_dp_connector_get_modes,
-};
+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,
@@ -34,6 +38,8 @@ static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
 	.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 +109,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 v2 drm-dp 3/7] drm/hisilicon/hibmc: Getting connector info and edid by using aux channel

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:55PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Registering drm_aux and using it to get connector edid with drm functions.
> Refactoring some structs to fit aux's register framework. We need change

Nit: EDID, AUX.

> a lot about getting member of drm_aux, because we change it's postions from
> struct hibmc_dp_dev to struct hibmc_dp.

Why? Please describe the reasons, not what is being done. It might be
beneficial to split the patch into two parts:
 - move the drm_aux + make it compile
 - the rest of the changes.

> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> 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   | 16 ++++++----
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  7 +++--
>  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 +++++++-------
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 30 +++++++++++--------
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  5 ++++
>  7 files changed, 52 insertions(+), 32 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..e0bb9b14d9d8 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,16 @@ 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;
> +	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/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> index e7746bc4b592..d613da8b544c 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> @@ -11,8 +11,11 @@
>  #include <linux/kernel.h>
>  #include <linux/bitfield.h>
>  #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 +35,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 +61,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 ee1ff205ff56..77f02d5151f7 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);
>  
>  	if (hibmc_dp_serdes_init(dp_dev))
>  		return -EAGAIN;
> 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 43a4b656493f..4a99a9b7e3c4 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");
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index 603d6b198a54..795c5b1a6b99 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -13,20 +13,24 @@
>  #include "hibmc_drm_drv.h"
>  #include "dp/dp_hw.h"
>  
> -static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
> -{
> -	int count;
> +static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
> +	.get_modes = drm_connector_helper_get_modes,
> +	.detect_ctx = drm_connector_helper_detect_from_ddc,
> +};
>  
> -	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
> +static int hibmc_dp_late_register(struct drm_connector *connector)
> +{
> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>  
> -	return count;
> +	return drm_dp_aux_register(&dp->aux);
>  }
>  
> -static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
> -	.get_modes = hibmc_dp_connector_get_modes,
> -};
> +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,
> @@ -34,6 +38,8 @@ static const struct drm_connector_funcs hibmc_dp_conn_funcs = {
>  	.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 +109,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
> 

-- 
With best wishes
Dmitry

[PATCH v2 drm-dp 4/7] drm/hisilicon/hibmc: Add colorbar-cfg: set color bar cfg

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

This is a DP IP controller's feature. It can be used as a debug method
which can check DP controller is working good. The colorbar displaying
doesn't rely on other IPs work in the chip, like: GPU or DDR (vram) and
so on, because colorbar diplaying data is generated by controller itself
inside the DP IP.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v1 -> v2:
  - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
  - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
---
 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 |  2 +
 3 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index 77f02d5151f7..8adace0befde 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -226,3 +226,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..621a0a1d7eb7 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 {
+	bool enable;
+	bool 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 f2fa9807d8ab..c43ad6b30c2c 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -23,6 +23,8 @@
 #define HIBMC_DP_VIDEO_MSA1			0x11c
 #define HIBMC_DP_VIDEO_MSA2			0x120
 #define HIBMC_DP_VIDEO_HORIZONTAL_SIZE		0X124
+#define HIBMC_DP_COLOR_BAR_CTRL			0x260
+#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
 #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
 #define HIBMC_DP_TIMING_GEN_CONFIG2		0x274
 #define HIBMC_DP_TIMING_GEN_CONFIG3		0x278
-- 
2.33.0

Re: [PATCH v2 drm-dp 4/7] drm/hisilicon/hibmc: Add colorbar-cfg: set color bar cfg

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:56PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> This is a DP IP controller's feature. It can be used as a debug method
> which can check DP controller is working good. The colorbar displaying
> doesn't rely on other IPs work in the chip, like: GPU or DDR (vram) and
> so on, because colorbar diplaying data is generated by controller itself
> inside the DP IP.

You are describing it in a pretty strange manner. Does this sound
better?

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

This also requires having corresponding debugfs entry here.

> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v1 -> v2:
>   - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
>   - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
> ---
>  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 |  2 +
>  3 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index 77f02d5151f7..8adace0befde 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -226,3 +226,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..621a0a1d7eb7 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 {
> +	bool enable;
> +	bool 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 f2fa9807d8ab..c43ad6b30c2c 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -23,6 +23,8 @@
>  #define HIBMC_DP_VIDEO_MSA1			0x11c
>  #define HIBMC_DP_VIDEO_MSA2			0x120
>  #define HIBMC_DP_VIDEO_HORIZONTAL_SIZE		0X124
> +#define HIBMC_DP_COLOR_BAR_CTRL			0x260
> +#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
>  #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
>  #define HIBMC_DP_TIMING_GEN_CONFIG2		0x274
>  #define HIBMC_DP_TIMING_GEN_CONFIG3		0x278
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 drm-dp 4/7] drm/hisilicon/hibmc: Add colorbar-cfg: set color bar cfg

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:56PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> This is a DP IP controller's feature. It can be used as a debug method
>> which can check DP controller is working good. The colorbar displaying
>> doesn't rely on other IPs work in the chip, like: GPU or DDR (vram) and
>> so on, because colorbar diplaying data is generated by controller itself
>> inside the DP IP.
> You are describing it in a pretty strange manner. Does this sound
> better?
>
> DP controller can support generating a color bar signal over the
> DisplayPort interface. This can be useful to check for possible memory
> or GPU problems, as the signal generator resides completely in the DP
> block. Add debugfs file that controls colorbar generator.
>
> This also requires having corresponding debugfs entry here.

Hi Dmitry,
Thank you for your guidance, I got it.


>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v1 -> v2:
>>    - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
>>    - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
>> ---
>>   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 |  2 +
>>   3 files changed, 74 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index 77f02d5151f7..8adace0befde 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -226,3 +226,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..621a0a1d7eb7 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 {
>> +	bool enable;
>> +	bool 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 f2fa9807d8ab..c43ad6b30c2c 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -23,6 +23,8 @@
>>   #define HIBMC_DP_VIDEO_MSA1			0x11c
>>   #define HIBMC_DP_VIDEO_MSA2			0x120
>>   #define HIBMC_DP_VIDEO_HORIZONTAL_SIZE		0X124
>> +#define HIBMC_DP_COLOR_BAR_CTRL			0x260
>> +#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
>>   #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
>>   #define HIBMC_DP_TIMING_GEN_CONFIG2		0x274
>>   #define HIBMC_DP_TIMING_GEN_CONFIG3		0x278
>> -- 
>> 2.33.0
>>

[PATCH v2 drm-dp 5/7] drm/hisilicon/hibmc: Get link status and dpcd caps

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

Prepare the hibmc_dp_get_foo() functions for debugfs using in
next patch. We also add dpcd's if statement in link training process,
because we have the dpcd.

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  3 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c   | 15 ++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h   |  3 ++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c | 39 ++++++++++++++++----
 4 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index d613da8b544c..8eb1659c7685 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -26,6 +26,9 @@ struct hibmc_link_status {
 struct hibmc_link_cap {
 	u8 link_rate;
 	u8 lanes;
+	int rx_dpcd_revision;
+	bool is_tps3;
+	bool is_tps4;
 };
 
 struct hibmc_dp_link {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index 8adace0befde..5e889c377117 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -227,6 +227,21 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
 	return 0;
 }
 
+u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.link_rate;
+}
+
+u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.lanes;
+}
+
+int hibmc_dp_get_dpcd(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.rx_dpcd_revision;
+}
+
 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 621a0a1d7eb7..823544b8008b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -54,6 +54,9 @@ struct hibmc_dp {
 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);
+int hibmc_dp_get_dpcd(struct hibmc_dp *dp);
+u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
+u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
 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_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
index 4a99a9b7e3c4..39345fc78c06 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -7,6 +7,7 @@
 #include "dp_comm.h"
 #include "dp_reg.h"
 #include "dp_serdes.h"
+#include "dp_config.h"
 
 #define HIBMC_EQ_MAX_RETRY 5
 
@@ -42,11 +43,7 @@ static int hibmc_dp_link_training_configure(struct hibmc_dp_dev *dp)
 		return ret >= 0 ? -EIO : ret;
 	}
 
-	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);
-
-	return ret;
+	return 0;
 }
 
 static int hibmc_dp_link_set_pattern(struct hibmc_dp_dev *dp, int pattern)
@@ -189,15 +186,17 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
 	bool level_changed;
 	u32 voltage_tries;
 	u32 cr_tries;
+	u32 max_cr;
 	int ret;
 
 	/*
 	 * DP 1.4 spec define 10 for maxtries value, for pre DP 1.4 version set a limit of 80
 	 * (4 voltage levels x 4 preemphasis levels x 5 identical voltage retries)
 	 */
+	max_cr = dp->link.cap.rx_dpcd_revision >= DP_DPCD_REV_14 ? 10 : 80;
 
 	voltage_tries = 1;
-	for (cr_tries = 0; cr_tries < 80; cr_tries++) {
+	for (cr_tries = 0; cr_tries < max_cr; cr_tries++) {
 		drm_dp_link_train_clock_recovery_delay(dp->aux, dp->dpcd);
 
 		ret = drm_dp_dpcd_read_link_status(dp->aux, lane_status);
@@ -234,7 +233,7 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
 		voltage_tries = level_changed ? 1 : voltage_tries + 1;
 	}
 
-	drm_err(dp->dev, "dp link training clock recovery 80 times failed\n");
+	drm_err(dp->dev, "dp link training clock recovery %u times failed\n", max_cr);
 	dp->link.status.clock_recovered = false;
 
 	return 0;
@@ -244,9 +243,17 @@ static int hibmc_dp_link_training_channel_eq(struct hibmc_dp_dev *dp)
 {
 	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
 	u8 eq_tries;
+	int tps;
 	int ret;
 
-	ret = hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_2);
+	if (dp->link.cap.is_tps4)
+		tps = DP_TRAINING_PATTERN_4;
+	else if (dp->link.cap.is_tps3)
+		tps = DP_TRAINING_PATTERN_3;
+	else
+		tps = DP_TRAINING_PATTERN_2;
+
+	ret = hibmc_dp_link_set_pattern(dp, tps);
 	if (ret)
 		return ret;
 
@@ -313,11 +320,27 @@ static int hibmc_dp_link_downgrade_training_eq(struct hibmc_dp_dev *dp)
 	return hibmc_dp_link_reduce_rate(dp);
 }
 
+static void hibmc_dp_update_caps(struct hibmc_dp_dev *dp)
+{
+	dp->link.cap.rx_dpcd_revision = dp->dpcd[DP_DPCD_REV];
+
+	dp->link.cap.is_tps3 = (dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_13) &&
+			       (dp->dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED);
+	dp->link.cap.is_tps4 = (dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14) &&
+			       (dp->dpcd[DP_MAX_DOWNSPREAD] & DP_TPS4_SUPPORTED);
+}
+
 int hibmc_dp_link_training(struct hibmc_dp_dev *dp)
 {
 	struct hibmc_dp_link *link = &dp->link;
 	int ret;
 
+	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);
+
+	hibmc_dp_update_caps(dp);
+
 	while (true) {
 		ret = hibmc_dp_link_training_cr_pre(dp);
 		if (ret)
-- 
2.33.0

Re: [PATCH v2 drm-dp 5/7] drm/hisilicon/hibmc: Get link status and dpcd caps

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:57PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> Prepare the hibmc_dp_get_foo() functions for debugfs using in
> next patch. We also add dpcd's if statement in link training process,
> because we have the dpcd.

Unrelated, split to a separate patch.

> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h |  3 ++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c   | 15 ++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h   |  3 ++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c | 39 ++++++++++++++++----
>  4 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> index d613da8b544c..8eb1659c7685 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
> @@ -26,6 +26,9 @@ struct hibmc_link_status {
>  struct hibmc_link_cap {
>  	u8 link_rate;
>  	u8 lanes;
> +	int rx_dpcd_revision;
> +	bool is_tps3;
> +	bool is_tps4;
>  };
>  
>  struct hibmc_dp_link {
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index 8adace0befde..5e889c377117 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -227,6 +227,21 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>  	return 0;
>  }
>  
> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.link_rate;
> +}
> +
> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.lanes;
> +}
> +
> +int hibmc_dp_get_dpcd(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.rx_dpcd_revision;
> +}
> +
>  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 621a0a1d7eb7..823544b8008b 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -54,6 +54,9 @@ struct hibmc_dp {
>  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);
> +int hibmc_dp_get_dpcd(struct hibmc_dp *dp);
> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>  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_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> index 4a99a9b7e3c4..39345fc78c06 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
> @@ -7,6 +7,7 @@
>  #include "dp_comm.h"
>  #include "dp_reg.h"
>  #include "dp_serdes.h"
> +#include "dp_config.h"
>  
>  #define HIBMC_EQ_MAX_RETRY 5
>  
> @@ -42,11 +43,7 @@ static int hibmc_dp_link_training_configure(struct hibmc_dp_dev *dp)
>  		return ret >= 0 ? -EIO : ret;
>  	}
>  
> -	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);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static int hibmc_dp_link_set_pattern(struct hibmc_dp_dev *dp, int pattern)
> @@ -189,15 +186,17 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
>  	bool level_changed;
>  	u32 voltage_tries;
>  	u32 cr_tries;
> +	u32 max_cr;
>  	int ret;
>  
>  	/*
>  	 * DP 1.4 spec define 10 for maxtries value, for pre DP 1.4 version set a limit of 80
>  	 * (4 voltage levels x 4 preemphasis levels x 5 identical voltage retries)
>  	 */
> +	max_cr = dp->link.cap.rx_dpcd_revision >= DP_DPCD_REV_14 ? 10 : 80;
>  
>  	voltage_tries = 1;
> -	for (cr_tries = 0; cr_tries < 80; cr_tries++) {
> +	for (cr_tries = 0; cr_tries < max_cr; cr_tries++) {
>  		drm_dp_link_train_clock_recovery_delay(dp->aux, dp->dpcd);
>  
>  		ret = drm_dp_dpcd_read_link_status(dp->aux, lane_status);
> @@ -234,7 +233,7 @@ static int hibmc_dp_link_training_cr(struct hibmc_dp_dev *dp)
>  		voltage_tries = level_changed ? 1 : voltage_tries + 1;
>  	}
>  
> -	drm_err(dp->dev, "dp link training clock recovery 80 times failed\n");
> +	drm_err(dp->dev, "dp link training clock recovery %u times failed\n", max_cr);
>  	dp->link.status.clock_recovered = false;
>  
>  	return 0;
> @@ -244,9 +243,17 @@ static int hibmc_dp_link_training_channel_eq(struct hibmc_dp_dev *dp)
>  {
>  	u8 lane_status[DP_LINK_STATUS_SIZE] = {0};
>  	u8 eq_tries;
> +	int tps;
>  	int ret;
>  
> -	ret = hibmc_dp_link_set_pattern(dp, DP_TRAINING_PATTERN_2);
> +	if (dp->link.cap.is_tps4)
> +		tps = DP_TRAINING_PATTERN_4;
> +	else if (dp->link.cap.is_tps3)
> +		tps = DP_TRAINING_PATTERN_3;
> +	else
> +		tps = DP_TRAINING_PATTERN_2;
> +
> +	ret = hibmc_dp_link_set_pattern(dp, tps);
>  	if (ret)
>  		return ret;
>  
> @@ -313,11 +320,27 @@ static int hibmc_dp_link_downgrade_training_eq(struct hibmc_dp_dev *dp)
>  	return hibmc_dp_link_reduce_rate(dp);
>  }
>  
> +static void hibmc_dp_update_caps(struct hibmc_dp_dev *dp)
> +{
> +	dp->link.cap.rx_dpcd_revision = dp->dpcd[DP_DPCD_REV];
> +
> +	dp->link.cap.is_tps3 = (dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_13) &&
> +			       (dp->dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED);
> +	dp->link.cap.is_tps4 = (dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14) &&
> +			       (dp->dpcd[DP_MAX_DOWNSPREAD] & DP_TPS4_SUPPORTED);
> +}
> +
>  int hibmc_dp_link_training(struct hibmc_dp_dev *dp)
>  {
>  	struct hibmc_dp_link *link = &dp->link;
>  	int ret;
>  
> +	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);
> +
> +	hibmc_dp_update_caps(dp);
> +
>  	while (true) {
>  		ret = hibmc_dp_link_training_cr_pre(dp);
>  		if (ret)
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

[PATCH v2 drm-dp 6/7] drm/hisilicon/hibmc: Add drm debugfs functions

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

We use the previous two patches as our debug functions and
generate two files. "hibmc-dp" and "color-bar".
hibmc-dp: read only, print the dp link status and dpcd version
color-bar: read/write
           write: cfg color bar and enable/disable it by your input
           read: print your current cfg info of color-bar

Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v1 -> v2:
  - 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 +-
 .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 124 ++++++++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
 4 files changed, 129 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/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
new file mode 100644
index 000000000000..af2efb70d6ea
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
@@ -0,0 +1,124 @@
+// 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"
+
+static int hibmc_dp_show(struct seq_file *m, void *arg)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+	int idx;
+
+	if (!drm_dev_enter(dev, &idx))
+		return -ENODEV;
+
+	seq_printf(m, "enable lanes: %u\n", hibmc_dp_get_lanes(&priv->dp));
+	seq_printf(m, "link rate: %d\n", hibmc_dp_get_link_rate(&priv->dp) * 27);
+	seq_printf(m, "dpcd version: 0x%x\n", hibmc_dp_get_dpcd(&priv->dp));
+
+	drm_dev_exit(idx);
+
+	return 0;
+}
+
+static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
+				   size_t size, loff_t *ppos)
+{
+	struct hibmc_drm_private *priv = file_inode(file)->i_private;
+	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
+	u32 input = 0;
+	int ret, idx;
+	u8 val;
+
+	ret = kstrtou32_from_user(user_buf, size, 0, &input);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(GENMASK(13, 10), input);
+	if (val > 9)
+		return -EINVAL;
+	cfg->pattern = val;
+	cfg->enable = FIELD_GET(BIT(0), input);
+	cfg->self_timing = FIELD_GET(BIT(1), input);
+	cfg->dynamic_rate = FIELD_GET(GENMASK(9, 2), input);
+
+	ret = drm_dev_enter(&priv->dev, &idx);
+	if (!ret)
+		return -ENODEV;
+
+	hibmc_dp_set_cbar(&priv->dp, cfg);
+
+	drm_dev_exit(idx);
+
+	return size;
+}
+
+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;
+	u32 output = 0;
+	int idx;
+
+	if (!drm_dev_enter(&priv->dev, &idx))
+		return -ENODEV;
+
+	/* bit[0]: 0: enable colorbar, 1: disable colorbar
+	 * bit[1]: 0: timing follows XDP, 1: internal self timing
+	 * bit[2,9]: 0: static colorbar image,
+	 *           1~255: right shifting a type of color per (1~255)frames
+	 * bit[10,13]: 0~9: color bar, white, red, orange,
+	 *             yellow, green, cyan, bule, pupper, black
+	 */
+	output = cfg->enable | (cfg->self_timing << 1) |
+		 (cfg->dynamic_rate << 2) | (cfg->pattern << 10);
+
+	drm_dev_exit(idx);
+
+	seq_printf(m, "hibmc dp colorbar cfg: %u\n", output);
+
+	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,
+};
+
+static struct drm_info_list hibmc_debugfs_list[] = {
+	{ "hibmc-dp", hibmc_dp_show },
+};
+
+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);
+	struct drm_minor *minor = dev->primary;
+
+	/* 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);
+
+	drm_debugfs_create_files(hibmc_debugfs_list, ARRAY_SIZE(hibmc_debugfs_list),
+				 root, minor);
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 795c5b1a6b99..1f8f7b74f5b4 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -40,6 +40,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 v2 drm-dp 6/7] drm/hisilicon/hibmc: Add drm debugfs functions

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:58PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> We use the previous two patches as our debug functions and
> generate two files. "hibmc-dp" and "color-bar".
> hibmc-dp: read only, print the dp link status and dpcd version

Please define a generic DP attribute for this, handle it in
drm_dp_helper.c. Other drivers then can reuse this debugfs file.
Also note drm_dp_downstream_debug(), it might also be helpful.
Also see msm_dp_debug_show() for inspiration

> color-bar: read/write
>            write: cfg color bar and enable/disable it by your input
>            read: print your current cfg info of color-bar

This really should go into your color-bar patch.

> 
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v1 -> v2:
>   - 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 +-
>  .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 124 ++++++++++++++++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>  4 files changed, 129 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/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
> new file mode 100644
> index 000000000000..af2efb70d6ea
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
> @@ -0,0 +1,124 @@
> +// 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"
> +
> +static int hibmc_dp_show(struct seq_file *m, void *arg)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	int idx;
> +
> +	if (!drm_dev_enter(dev, &idx))
> +		return -ENODEV;
> +
> +	seq_printf(m, "enable lanes: %u\n", hibmc_dp_get_lanes(&priv->dp));
> +	seq_printf(m, "link rate: %d\n", hibmc_dp_get_link_rate(&priv->dp) * 27);
> +	seq_printf(m, "dpcd version: 0x%x\n", hibmc_dp_get_dpcd(&priv->dp));
> +
> +	drm_dev_exit(idx);
> +
> +	return 0;
> +}
> +
> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
> +				   size_t size, loff_t *ppos)
> +{
> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
> +	u32 input = 0;
> +	int ret, idx;
> +	u8 val;
> +
> +	ret = kstrtou32_from_user(user_buf, size, 0, &input);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_GET(GENMASK(13, 10), input);
> +	if (val > 9)
> +		return -EINVAL;
> +	cfg->pattern = val;
> +	cfg->enable = FIELD_GET(BIT(0), input);
> +	cfg->self_timing = FIELD_GET(BIT(1), input);
> +	cfg->dynamic_rate = FIELD_GET(GENMASK(9, 2), input);

Having a binary file format is really a sad idea. Can it be a text file
instead?

> +
> +	ret = drm_dev_enter(&priv->dev, &idx);
> +	if (!ret)
> +		return -ENODEV;
> +
> +	hibmc_dp_set_cbar(&priv->dp, cfg);
> +
> +	drm_dev_exit(idx);
> +
> +	return size;
> +}
> +

-- 
With best wishes
Dmitry

Re: [PATCH v2 drm-dp 6/7] drm/hisilicon/hibmc: Add drm debugfs functions

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:58PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> We use the previous two patches as our debug functions and
>> generate two files. "hibmc-dp" and "color-bar".
>> hibmc-dp: read only, print the dp link status and dpcd version
> Please define a generic DP attribute for this, handle it in
> drm_dp_helper.c. Other drivers then can reuse this debugfs file.
> Also note drm_dp_downstream_debug(), it might also be helpful.
> Also see msm_dp_debug_show() for inspiration

Hi Dmitry,

Thanks for your much advising! I will consider all of them and fix them with detailed explanations.
And I will try to make it in drm_dp_helper.c for here.

Sincerely,
Baihan Li


>> color-bar: read/write
>>             write: cfg color bar and enable/disable it by your input
>>             read: print your current cfg info of color-bar
> This really should go into your color-bar patch.

Ok.


>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v1 -> v2:
>>    - 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 +-
>>   .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 124 ++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>>   4 files changed, 129 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/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> new file mode 100644
>> index 000000000000..af2efb70d6ea
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> @@ -0,0 +1,124 @@
>> +// 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"
>> +
>> +static int hibmc_dp_show(struct seq_file *m, void *arg)
>> +{
>> +	struct drm_info_node *node = m->private;
>> +	struct drm_device *dev = node->minor->dev;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	int idx;
>> +
>> +	if (!drm_dev_enter(dev, &idx))
>> +		return -ENODEV;
>> +
>> +	seq_printf(m, "enable lanes: %u\n", hibmc_dp_get_lanes(&priv->dp));
>> +	seq_printf(m, "link rate: %d\n", hibmc_dp_get_link_rate(&priv->dp) * 27);
>> +	seq_printf(m, "dpcd version: 0x%x\n", hibmc_dp_get_dpcd(&priv->dp));
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
>> +				   size_t size, loff_t *ppos)
>> +{
>> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
>> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
>> +	u32 input = 0;
>> +	int ret, idx;
>> +	u8 val;
>> +
>> +	ret = kstrtou32_from_user(user_buf, size, 0, &input);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = FIELD_GET(GENMASK(13, 10), input);
>> +	if (val > 9)
>> +		return -EINVAL;
>> +	cfg->pattern = val;
>> +	cfg->enable = FIELD_GET(BIT(0), input);
>> +	cfg->self_timing = FIELD_GET(BIT(1), input);
>> +	cfg->dynamic_rate = FIELD_GET(GENMASK(9, 2), input);
> Having a binary file format is really a sad idea. Can it be a text file
> instead?
>
>> +
>> +	ret = drm_dev_enter(&priv->dev, &idx);
>> +	if (!ret)
>> +		return -ENODEV;
>> +
>> +	hibmc_dp_set_cbar(&priv->dp, cfg);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return size;
>> +}
>> +

Re: [PATCH v2 drm-dp 6/7] drm/hisilicon/hibmc: Add drm debugfs functions

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:58PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> We use the previous two patches as our debug functions and
>> generate two files. "hibmc-dp" and "color-bar".
>> hibmc-dp: read only, print the dp link status and dpcd version
> Please define a generic DP attribute for this, handle it in
> drm_dp_helper.c. Other drivers then can reuse this debugfs file.
> Also note drm_dp_downstream_debug(), it might also be helpful.
> Also see msm_dp_debug_show() for inspiration

Hi Dmitry,
Thanks four your advice! I think I will remove this link status part temporarily.
I'll post a separate patch for this generic function later.


>> color-bar: read/write
>>             write: cfg color bar and enable/disable it by your input
>>             read: print your current cfg info of color-bar
> This really should go into your color-bar patch.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v1 -> v2:
>>    - 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 +-
>>   .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 124 ++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>>   4 files changed, 129 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/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> new file mode 100644
>> index 000000000000..af2efb70d6ea
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> @@ -0,0 +1,124 @@
>> +// 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"
>> +
>> +static int hibmc_dp_show(struct seq_file *m, void *arg)
>> +{
>> +	struct drm_info_node *node = m->private;
>> +	struct drm_device *dev = node->minor->dev;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	int idx;
>> +
>> +	if (!drm_dev_enter(dev, &idx))
>> +		return -ENODEV;
>> +
>> +	seq_printf(m, "enable lanes: %u\n", hibmc_dp_get_lanes(&priv->dp));
>> +	seq_printf(m, "link rate: %d\n", hibmc_dp_get_link_rate(&priv->dp) * 27);
>> +	seq_printf(m, "dpcd version: 0x%x\n", hibmc_dp_get_dpcd(&priv->dp));
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
>> +				   size_t size, loff_t *ppos)
>> +{
>> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
>> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
>> +	u32 input = 0;
>> +	int ret, idx;
>> +	u8 val;
>> +
>> +	ret = kstrtou32_from_user(user_buf, size, 0, &input);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = FIELD_GET(GENMASK(13, 10), input);
>> +	if (val > 9)
>> +		return -EINVAL;
>> +	cfg->pattern = val;
>> +	cfg->enable = FIELD_GET(BIT(0), input);
>> +	cfg->self_timing = FIELD_GET(BIT(1), input);
>> +	cfg->dynamic_rate = FIELD_GET(GENMASK(9, 2), input);
> Having a binary file format is really a sad idea. Can it be a text file
> instead?
>
>> +
>> +	ret = drm_dev_enter(&priv->dev, &idx);
>> +	if (!ret)
>> +		return -ENODEV;
>> +
>> +	hibmc_dp_set_cbar(&priv->dp, cfg);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return size;
>> +}
>> +

Re: [PATCH v2 drm-dp 6/7] drm/hisilicon/hibmc: Add drm debugfs functions

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:58PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> We use the previous two patches as our debug functions and
>> generate two files. "hibmc-dp" and "color-bar".
>> hibmc-dp: read only, print the dp link status and dpcd version
> Please define a generic DP attribute for this, handle it in
> drm_dp_helper.c. Other drivers then can reuse this debugfs file.
> Also note drm_dp_downstream_debug(), it might also be helpful.
> Also see msm_dp_debug_show() for inspiration
>
>> color-bar: read/write
>>             write: cfg color bar and enable/disable it by your input
>>             read: print your current cfg info of color-bar
> This really should go into your color-bar patch.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v1 -> v2:
>>    - 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 +-
>>   .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 124 ++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>>   4 files changed, 129 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/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> new file mode 100644
>> index 000000000000..af2efb70d6ea
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> @@ -0,0 +1,124 @@
>> +// 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"
>> +
>> +static int hibmc_dp_show(struct seq_file *m, void *arg)
>> +{
>> +	struct drm_info_node *node = m->private;
>> +	struct drm_device *dev = node->minor->dev;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	int idx;
>> +
>> +	if (!drm_dev_enter(dev, &idx))
>> +		return -ENODEV;
>> +
>> +	seq_printf(m, "enable lanes: %u\n", hibmc_dp_get_lanes(&priv->dp));
>> +	seq_printf(m, "link rate: %d\n", hibmc_dp_get_link_rate(&priv->dp) * 27);
>> +	seq_printf(m, "dpcd version: 0x%x\n", hibmc_dp_get_dpcd(&priv->dp));
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
>> +				   size_t size, loff_t *ppos)
>> +{
>> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
>> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
>> +	u32 input = 0;
>> +	int ret, idx;
>> +	u8 val;
>> +
>> +	ret = kstrtou32_from_user(user_buf, size, 0, &input);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = FIELD_GET(GENMASK(13, 10), input);
>> +	if (val > 9)
>> +		return -EINVAL;
>> +	cfg->pattern = val;
>> +	cfg->enable = FIELD_GET(BIT(0), input);
>> +	cfg->self_timing = FIELD_GET(BIT(1), input);
>> +	cfg->dynamic_rate = FIELD_GET(GENMASK(9, 2), input);
> Having a binary file format is really a sad idea. Can it be a text file
> instead?

Okay, I'll change it to 4 int numbers as parameters, like: echo "1 2 3 4" > file


>> +
>> +	ret = drm_dev_enter(&priv->dev, &idx);
>> +	if (!ret)
>> +		return -ENODEV;
>> +
>> +	hibmc_dp_set_cbar(&priv->dp, cfg);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return size;
>> +}
>> +

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

[Yongbang Shi]

From: Baihan Li <libaihan@huawei.com>

To realize HPD feature, we have getting hpd status function, registering
irq function, interrupt handler function, and hpd event process function.
And also 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>
---
ChangeLog:
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    | 33 ++++++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    | 13 ++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  2 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 57 +++++++++++++-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 78 +++++++++++++++----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   | 14 ++++
 7 files changed, 181 insertions(+), 17 deletions(-)

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 5e889c377117..0b6ce48c5d8f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -181,6 +181,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 */
@@ -284,3 +285,35 @@ void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
 	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);
 }
+
+enum hibmc_dp_hpd_status hibmc_dp_get_hpd_status(const struct hibmc_dp *dp)
+{
+	enum hibmc_dp_hpd_status status;
+	u32 val;
+
+	/* spec said the HPD signal needs to be pulled up for
+	 * more than 100 ms to enter the plug status
+	 */
+	mdelay(100);
+
+	val = readl(dp->dp_dev->base + HIBMC_DP_HPD_STATUS);
+	val = (val & HIBMC_DP_CFG_SINK_HPD_STATE_MACHINE) >> 4;
+
+	switch (val) {
+	case 0: /* 0: IDLE */
+	case 3: /* 3: DONE */
+		status = HIBMC_DP_HPD_OUT;
+		break;
+	case 1: /* 1: PLUG */
+		status = HIBMC_DP_HPD_IN;
+		break;
+	case 4: /* 4: ASSERT_LENGHT */
+		status = HIBMC_DP_HPD_DETECTING;
+		break;
+	default: /* unknown status */
+		status = HIBMC_DP_HPD_DET_FAIL;
+		break;
+	}
+
+	return status;
+}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 823544b8008b..df7b692b1998 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -41,6 +41,17 @@ struct hibmc_dp_cbar_cfg {
 	enum hibmc_dp_cbar_pattern pattern;
 };
 
+enum hibmc_dp_hpd_status {
+	HIBMC_DP_HPD_DETECTING,
+	HIBMC_DP_HPD_IN,
+	HIBMC_DP_HPD_OUT,
+	HIBMC_DP_HPD_SHORT, /* Short hpd (irq_hpd) */
+	HIBMC_DP_HPD_DET_FAIL,
+	HIBMC_DP_HPD_IN_SIMULATE,
+	HIBMC_DP_HPD_OUT_SIMULATE,
+	HIBMC_DP_HPD_SHORT_SIMULATE,
+};
+
 struct hibmc_dp {
 	struct hibmc_dp_dev *dp_dev;
 	struct drm_device *drm_dev;
@@ -49,6 +60,7 @@ struct hibmc_dp {
 	void __iomem *mmio;
 	struct drm_dp_aux aux;
 	struct hibmc_dp_cbar_cfg cfg;
+	u32 hpd_status;
 };
 
 int hibmc_dp_hw_init(struct hibmc_dp *dp);
@@ -58,5 +70,6 @@ int hibmc_dp_get_dpcd(struct hibmc_dp *dp);
 u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
 u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
 void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
+enum hibmc_dp_hpd_status hibmc_dp_get_hpd_status(const struct hibmc_dp *dp);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
index c43ad6b30c2c..6c4ba0412b15 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
@@ -12,6 +12,7 @@
 #define HIBMC_DP_AUX_RD_DATA0			0x64
 #define HIBMC_DP_AUX_REQ			0x74
 #define HIBMC_DP_AUX_STATUS			0x78
+#define HIBMC_DP_HPD_STATUS			0x98
 #define HIBMC_DP_PHYIF_CTRL0			0xa0
 #define HIBMC_DP_VIDEO_CTRL			0x100
 #define HIBMC_DP_VIDEO_CONFIG0			0x104
@@ -75,5 +76,6 @@
 #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
 #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
 #define HIBMC_DP_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
+#define HIBMC_DP_CFG_SINK_HPD_STATE_MACHINE	GENMASK(7, 4)
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 1f8f7b74f5b4..057dd3caa043 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -11,8 +11,12 @@
 #include <drm/drm_edid.h>
 
 #include "hibmc_drm_drv.h"
+#include "hibmc_drm_regs.h"
 #include "dp/dp_hw.h"
 
+#define DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
+#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
+
 static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
 	.get_modes = drm_connector_helper_get_modes,
 	.detect_ctx = drm_connector_helper_detect_from_ddc,
@@ -81,6 +85,53 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
 	.atomic_disable = hibmc_dp_encoder_disable,
 };
 
+static int hibmc_dp_cfg(struct hibmc_dp *dp)
+{
+	int ret;
+
+	ret = hibmc_dp_hw_init(dp);
+	if (ret)
+		return ret;
+
+	hibmc_dp_display_en(dp, false);
+
+	return 0;
+}
+
+irqreturn_t hibmc_dp_hpd_event_isr(int irq, void *arg)
+{
+	struct drm_device *dev = (struct drm_device *)arg;
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+	enum hibmc_dp_hpd_status status;
+	int idx;
+
+	if (!drm_dev_enter(dev, &idx))
+		return -ENODEV;
+
+	status  = hibmc_dp_get_hpd_status(&priv->dp);
+	if (status == HIBMC_DP_HPD_IN) {
+		if (!(priv->dp.hpd_status & DP_MASKED_SINK_HPD_PLUG_INT))
+			goto err;
+		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
+		hibmc_dp_cfg(&priv->dp);
+		drm_kms_helper_connector_hotplug_event(&priv->dp.connector);
+	} else if (status == HIBMC_DP_HPD_OUT) {
+		if (!(priv->dp.hpd_status & DP_MASKED_SINK_HPD_UNPLUG_INT))
+			goto err;
+		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
+		drm_kms_helper_connector_hotplug_event(&priv->dp.connector);
+	} else {
+		drm_warn(&priv->dev, "HPD unknown isr occur, status: %d\n", status);
+	}
+
+err:
+	priv->dp.hpd_status = 0;
+
+	drm_dev_exit(idx);
+
+	return IRQ_HANDLED;
+}
+
 int hibmc_dp_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = &priv->dev;
@@ -93,14 +144,12 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
 	dp->mmio = priv->mmio;
 	dp->drm_dev = dev;
 
-	ret = hibmc_dp_hw_init(&priv->dp);
+	ret = hibmc_dp_cfg(dp);
 	if (ret) {
-		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
+		drm_err(dev, "hibmc dp cfg failed: %d\n", ret);
 		return ret;
 	}
 
-	hibmc_dp_display_en(&priv->dp, false);
-
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret) {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index bade693d9730..199acb3e7a73 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -29,9 +29,13 @@
 #include "hibmc_drm_regs.h"
 
 #define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
+#define HIBMC_DP_INTSTAT		0x1e0724
+#define HIBMC_DP_INTCLR			0x1e0728
 
 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 +53,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.hpd_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 +268,52 @@ 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)
+{
+	drm_atomic_helper_shutdown(dev);
+
+	pci_disable_msi(to_pci_dev(dev->dev));
+}
+
+static int hibmc_msi_init(struct drm_device *dev)
 {
+	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	int ret;
 
-	drm_atomic_helper_shutdown(dev);
+	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;
+	}
 
-	free_irq(pdev->irq, dev);
+	priv->valid_irq_num = ret;
 
-	pci_disable_msi(to_pci_dev(dev->dev));
+	priv->irqs = drmm_kcalloc(dev, priv->valid_irq_num,
+				  sizeof(struct hibmc_irq), GFP_KERNEL);
+	if (!priv->irqs)
+		return -ENOMEM;
+
+	for (int i = 0; i < priv->valid_irq_num; i++) {
+		snprintf(priv->irqs[i].name, sizeof(priv->irqs[i].name) - 1, "%s-%s-%s",
+			 dev->driver->name, pci_name(pdev), g_irqs_names_map[i]);
+
+		priv->irqs[i].irq_num = pci_irq_vector(pdev, i);
+
+		if (i)
+			/* PCI devices require shared interrupts. */
+			ret = devm_request_threaded_irq(&pdev->dev, priv->irqs[i].irq_num,
+							hibmc_dp_interrupt, hibmc_dp_hpd_event_isr,
+							IRQF_SHARED, priv->irqs[i].name, dev);
+		else
+			ret = devm_request_irq(&pdev->dev, priv->irqs[i].irq_num, hibmc_interrupt,
+					       IRQF_SHARED, priv->irqs[i].name, dev);
+		if (ret) {
+			drm_err(dev, "install irq failed: %d\n", ret);
+			return ret;
+		}
+	}
 
 	return 0;
 }
@@ -288,15 +345,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 +424,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 bc89e4b9f4e3..764e211a8779 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -22,6 +22,14 @@
 
 #include "dp/dp_hw.h"
 
+#define HIBMC_MIN_VECTORS	1
+#define HIBMC_MAX_VECTORS	2
+
+struct hibmc_irq {
+	s32 irq_num;
+	char name[32];
+};
+
 struct hibmc_vdac {
 	struct drm_device *dev;
 	struct drm_encoder encoder;
@@ -40,6 +48,10 @@ struct hibmc_drm_private {
 	struct drm_crtc crtc;
 	struct hibmc_vdac vdac;
 	struct hibmc_dp dp;
+
+	/* irqs */
+	struct hibmc_irq *irqs;
+	u32 valid_irq_num;
 };
 
 static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
@@ -71,4 +83,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_event_isr(int irq, void *arg);
+
 #endif
-- 
2.33.0

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

[Dmitry Baryshkov]

On Mon, Feb 10, 2025 at 10:49:59PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> To realize HPD feature, we have getting hpd status function, registering
> irq function, interrupt handler function, and hpd event process function.
> And also 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>
> ---
> ChangeLog:
> 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    | 33 ++++++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    | 13 ++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  2 +
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 57 +++++++++++++-
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 78 +++++++++++++++----
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   | 14 ++++
>  7 files changed, 181 insertions(+), 17 deletions(-)
> 
> 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 5e889c377117..0b6ce48c5d8f 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -181,6 +181,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 */
> @@ -284,3 +285,35 @@ void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
>  	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);
>  }
> +
> +enum hibmc_dp_hpd_status hibmc_dp_get_hpd_status(const struct hibmc_dp *dp)
> +{
> +	enum hibmc_dp_hpd_status status;
> +	u32 val;
> +
> +	/* spec said the HPD signal needs to be pulled up for
> +	 * more than 100 ms to enter the plug status
> +	 */
> +	mdelay(100);

Please check that your code doesn't break with CONFIG_DEBUG_ATOMIC_SLEEP=y

Having a 100msec delay in the ISR handler is a really sad idea. Don't
you have HPD debouncing handled in the hardware?

> +
> +	val = readl(dp->dp_dev->base + HIBMC_DP_HPD_STATUS);
> +	val = (val & HIBMC_DP_CFG_SINK_HPD_STATE_MACHINE) >> 4;
> +
> +	switch (val) {
> +	case 0: /* 0: IDLE */
> +	case 3: /* 3: DONE */
> +		status = HIBMC_DP_HPD_OUT;
> +		break;
> +	case 1: /* 1: PLUG */
> +		status = HIBMC_DP_HPD_IN;
> +		break;
> +	case 4: /* 4: ASSERT_LENGHT */
> +		status = HIBMC_DP_HPD_DETECTING;
> +		break;
> +	default: /* unknown status */
> +		status = HIBMC_DP_HPD_DET_FAIL;
> +		break;
> +	}

Is there a way to detect a short pulse on the HPD line aka attention?
You have defined HIBMC_DP_HPD_SHORT below, but it is not being assigned
at all.

> +
> +	return status;
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> index 823544b8008b..df7b692b1998 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -41,6 +41,17 @@ struct hibmc_dp_cbar_cfg {
>  	enum hibmc_dp_cbar_pattern pattern;
>  };
>  
> +enum hibmc_dp_hpd_status {
> +	HIBMC_DP_HPD_DETECTING,
> +	HIBMC_DP_HPD_IN,
> +	HIBMC_DP_HPD_OUT,
> +	HIBMC_DP_HPD_SHORT, /* Short hpd (irq_hpd) */
> +	HIBMC_DP_HPD_DET_FAIL,
> +	HIBMC_DP_HPD_IN_SIMULATE,
> +	HIBMC_DP_HPD_OUT_SIMULATE,
> +	HIBMC_DP_HPD_SHORT_SIMULATE,
> +};
> +
>  struct hibmc_dp {
>  	struct hibmc_dp_dev *dp_dev;
>  	struct drm_device *drm_dev;
> @@ -49,6 +60,7 @@ struct hibmc_dp {
>  	void __iomem *mmio;
>  	struct drm_dp_aux aux;
>  	struct hibmc_dp_cbar_cfg cfg;
> +	u32 hpd_status;
>  };
>  
>  int hibmc_dp_hw_init(struct hibmc_dp *dp);
> @@ -58,5 +70,6 @@ int hibmc_dp_get_dpcd(struct hibmc_dp *dp);
>  u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
>  u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>  void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
> +enum hibmc_dp_hpd_status hibmc_dp_get_hpd_status(const struct hibmc_dp *dp);
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> index c43ad6b30c2c..6c4ba0412b15 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
> @@ -12,6 +12,7 @@
>  #define HIBMC_DP_AUX_RD_DATA0			0x64
>  #define HIBMC_DP_AUX_REQ			0x74
>  #define HIBMC_DP_AUX_STATUS			0x78
> +#define HIBMC_DP_HPD_STATUS			0x98
>  #define HIBMC_DP_PHYIF_CTRL0			0xa0
>  #define HIBMC_DP_VIDEO_CTRL			0x100
>  #define HIBMC_DP_VIDEO_CONFIG0			0x104
> @@ -75,5 +76,6 @@
>  #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
>  #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>  #define HIBMC_DP_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
> +#define HIBMC_DP_CFG_SINK_HPD_STATE_MACHINE	GENMASK(7, 4)
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index 1f8f7b74f5b4..057dd3caa043 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -11,8 +11,12 @@
>  #include <drm/drm_edid.h>
>  
>  #include "hibmc_drm_drv.h"
> +#include "hibmc_drm_regs.h"
>  #include "dp/dp_hw.h"
>  
> +#define DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
> +#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
> +
>  static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>  	.get_modes = drm_connector_helper_get_modes,
>  	.detect_ctx = drm_connector_helper_detect_from_ddc,
> @@ -81,6 +85,53 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>  	.atomic_disable = hibmc_dp_encoder_disable,
>  };
>  
> +static int hibmc_dp_cfg(struct hibmc_dp *dp)
> +{
> +	int ret;
> +
> +	ret = hibmc_dp_hw_init(dp);
> +	if (ret)
> +		return ret;
> +
> +	hibmc_dp_display_en(dp, false);

Does this enable video signal? You should only enable AUX and perform
link training at this point.

> +
> +	return 0;
> +}
> +
> +irqreturn_t hibmc_dp_hpd_event_isr(int irq, void *arg)
> +{
> +	struct drm_device *dev = (struct drm_device *)arg;
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> +	enum hibmc_dp_hpd_status status;
> +	int idx;
> +
> +	if (!drm_dev_enter(dev, &idx))
> +		return -ENODEV;
> +
> +	status  = hibmc_dp_get_hpd_status(&priv->dp);
> +	if (status == HIBMC_DP_HPD_IN) {
> +		if (!(priv->dp.hpd_status & DP_MASKED_SINK_HPD_PLUG_INT))
> +			goto err;
> +		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
> +		hibmc_dp_cfg(&priv->dp);
> +		drm_kms_helper_connector_hotplug_event(&priv->dp.connector);
> +	} else if (status == HIBMC_DP_HPD_OUT) {
> +		if (!(priv->dp.hpd_status & DP_MASKED_SINK_HPD_UNPLUG_INT))
> +			goto err;
> +		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
> +		drm_kms_helper_connector_hotplug_event(&priv->dp.connector);

This looks like a hand-coded drm_connector_helper_hpd_irq_event()

> +	} else {
> +		drm_warn(&priv->dev, "HPD unknown isr occur, status: %d\n", status);
> +	}
> +
> +err:
> +	priv->dp.hpd_status = 0;
> +
> +	drm_dev_exit(idx);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  int hibmc_dp_init(struct hibmc_drm_private *priv)
>  {
>  	struct drm_device *dev = &priv->dev;
> @@ -93,14 +144,12 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>  	dp->mmio = priv->mmio;
>  	dp->drm_dev = dev;
>  
> -	ret = hibmc_dp_hw_init(&priv->dp);
> +	ret = hibmc_dp_cfg(dp);
>  	if (ret) {
> -		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
> +		drm_err(dev, "hibmc dp cfg failed: %d\n", ret);
>  		return ret;
>  	}
>  
> -	hibmc_dp_display_en(&priv->dp, false);
> -
>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
>  	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index bade693d9730..199acb3e7a73 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -29,9 +29,13 @@
>  #include "hibmc_drm_regs.h"
>  
>  #define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
> +#define HIBMC_DP_INTSTAT		0x1e0724
> +#define HIBMC_DP_INTCLR			0x1e0728

Why are those defined outside of hibmc_drm_regs.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 +53,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.hpd_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 +268,52 @@ 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)
> +{
> +	drm_atomic_helper_shutdown(dev);
> +
> +	pci_disable_msi(to_pci_dev(dev->dev));
> +}
> +
> +static int hibmc_msi_init(struct drm_device *dev)
>  {
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
> +	int ret;
>  
> -	drm_atomic_helper_shutdown(dev);
> +	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;
> +	}
>  
> -	free_irq(pdev->irq, dev);
> +	priv->valid_irq_num = ret;
>  
> -	pci_disable_msi(to_pci_dev(dev->dev));
> +	priv->irqs = drmm_kcalloc(dev, priv->valid_irq_num,
> +				  sizeof(struct hibmc_irq), GFP_KERNEL);

Why do you need an extra pointer?  You know that there are no more than
HIBMC_MAX_VECTORS, so you can have this data as a part of your private
structure.

> +	if (!priv->irqs)
> +		return -ENOMEM;
> +
> +	for (int i = 0; i < priv->valid_irq_num; i++) {
> +		snprintf(priv->irqs[i].name, sizeof(priv->irqs[i].name) - 1, "%s-%s-%s",
> +			 dev->driver->name, pci_name(pdev), g_irqs_names_map[i]);
> +
> +		priv->irqs[i].irq_num = pci_irq_vector(pdev, i);
> +
> +		if (i)
> +			/* PCI devices require shared interrupts. */
> +			ret = devm_request_threaded_irq(&pdev->dev, priv->irqs[i].irq_num,
> +							hibmc_dp_interrupt, hibmc_dp_hpd_event_isr,
> +							IRQF_SHARED, priv->irqs[i].name, dev);
> +		else
> +			ret = devm_request_irq(&pdev->dev, priv->irqs[i].irq_num, hibmc_interrupt,
> +					       IRQF_SHARED, priv->irqs[i].name, dev);

This feels like "too many changes". You have MSI-related changes, devm_,
your HPD interrupt. Please split this patch into logical parts.

> +		if (ret) {
> +			drm_err(dev, "install irq failed: %d\n", ret);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> @@ -288,15 +345,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 +424,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 bc89e4b9f4e3..764e211a8779 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -22,6 +22,14 @@
>  
>  #include "dp/dp_hw.h"
>  
> +#define HIBMC_MIN_VECTORS	1
> +#define HIBMC_MAX_VECTORS	2
> +
> +struct hibmc_irq {
> +	s32 irq_num;
> +	char name[32];
> +};
> +
>  struct hibmc_vdac {
>  	struct drm_device *dev;
>  	struct drm_encoder encoder;
> @@ -40,6 +48,10 @@ struct hibmc_drm_private {
>  	struct drm_crtc crtc;
>  	struct hibmc_vdac vdac;
>  	struct hibmc_dp dp;
> +
> +	/* irqs */
> +	struct hibmc_irq *irqs;
> +	u32 valid_irq_num;
>  };
>  
>  static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
> @@ -71,4 +83,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_event_isr(int irq, void *arg);
> +
>  #endif
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

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

[Yongbang Shi]

> On Mon, Feb 10, 2025 at 10:49:59PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> To realize HPD feature, we have getting hpd status function, registering
>> irq function, interrupt handler function, and hpd event process function.
>> And also 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>
>> ---
>> ChangeLog:
>> 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    | 33 ++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    | 13 ++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |  2 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 57 +++++++++++++-
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 78 +++++++++++++++----
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   | 14 ++++
>>   7 files changed, 181 insertions(+), 17 deletions(-)
>>
>> 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 5e889c377117..0b6ce48c5d8f 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -181,6 +181,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 */
>> @@ -284,3 +285,35 @@ void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
>>   	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);
>>   }
>> +
>> +enum hibmc_dp_hpd_status hibmc_dp_get_hpd_status(const struct hibmc_dp *dp)
>> +{
>> +	enum hibmc_dp_hpd_status status;
>> +	u32 val;
>> +
>> +	/* spec said the HPD signal needs to be pulled up for
>> +	 * more than 100 ms to enter the plug status
>> +	 */
>> +	mdelay(100);
> Please check that your code doesn't break with CONFIG_DEBUG_ATOMIC_SLEEP=y
>
> Having a 100msec delay in the ISR handler is a really sad idea. Don't
> you have HPD debouncing handled in the hardware?
>
>> +
>> +	val = readl(dp->dp_dev->base + HIBMC_DP_HPD_STATUS);
>> +	val = (val & HIBMC_DP_CFG_SINK_HPD_STATE_MACHINE) >> 4;
>> +
>> +	switch (val) {
>> +	case 0: /* 0: IDLE */
>> +	case 3: /* 3: DONE */
>> +		status = HIBMC_DP_HPD_OUT;
>> +		break;
>> +	case 1: /* 1: PLUG */
>> +		status = HIBMC_DP_HPD_IN;
>> +		break;
>> +	case 4: /* 4: ASSERT_LENGHT */
>> +		status = HIBMC_DP_HPD_DETECTING;
>> +		break;
>> +	default: /* unknown status */
>> +		status = HIBMC_DP_HPD_DET_FAIL;
>> +		break;
>> +	}
> Is there a way to detect a short pulse on the HPD line aka attention?
> You have defined HIBMC_DP_HPD_SHORT below, but it is not being assigned
> at all.

Okay, I'll fix this funciton.


>> +
>> +	return status;
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 823544b8008b..df7b692b1998 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -41,6 +41,17 @@ struct hibmc_dp_cbar_cfg {
>>   	enum hibmc_dp_cbar_pattern pattern;
>>   };
>>   
>> +enum hibmc_dp_hpd_status {
>> +	HIBMC_DP_HPD_DETECTING,
>> +	HIBMC_DP_HPD_IN,
>> +	HIBMC_DP_HPD_OUT,
>> +	HIBMC_DP_HPD_SHORT, /* Short hpd (irq_hpd) */
>> +	HIBMC_DP_HPD_DET_FAIL,
>> +	HIBMC_DP_HPD_IN_SIMULATE,
>> +	HIBMC_DP_HPD_OUT_SIMULATE,
>> +	HIBMC_DP_HPD_SHORT_SIMULATE,
>> +};
>> +
>>   struct hibmc_dp {
>>   	struct hibmc_dp_dev *dp_dev;
>>   	struct drm_device *drm_dev;
>> @@ -49,6 +60,7 @@ struct hibmc_dp {
>>   	void __iomem *mmio;
>>   	struct drm_dp_aux aux;
>>   	struct hibmc_dp_cbar_cfg cfg;
>> +	u32 hpd_status;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> @@ -58,5 +70,6 @@ int hibmc_dp_get_dpcd(struct hibmc_dp *dp);
>>   u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
>>   u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>>   void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>> +enum hibmc_dp_hpd_status hibmc_dp_get_hpd_status(const struct hibmc_dp *dp);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> index c43ad6b30c2c..6c4ba0412b15 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -12,6 +12,7 @@
>>   #define HIBMC_DP_AUX_RD_DATA0			0x64
>>   #define HIBMC_DP_AUX_REQ			0x74
>>   #define HIBMC_DP_AUX_STATUS			0x78
>> +#define HIBMC_DP_HPD_STATUS			0x98
>>   #define HIBMC_DP_PHYIF_CTRL0			0xa0
>>   #define HIBMC_DP_VIDEO_CTRL			0x100
>>   #define HIBMC_DP_VIDEO_CONFIG0			0x104
>> @@ -75,5 +76,6 @@
>>   #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
>>   #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>>   #define HIBMC_DP_CFG_STREAM_SYNC_CALIBRATION	GENMASK(31, 20)
>> +#define HIBMC_DP_CFG_SINK_HPD_STATE_MACHINE	GENMASK(7, 4)
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index 1f8f7b74f5b4..057dd3caa043 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -11,8 +11,12 @@
>>   #include <drm/drm_edid.h>
>>   
>>   #include "hibmc_drm_drv.h"
>> +#include "hibmc_drm_regs.h"
>>   #include "dp/dp_hw.h"
>>   
>> +#define DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
>> +#define DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>> +
>>   static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>>   	.get_modes = drm_connector_helper_get_modes,
>>   	.detect_ctx = drm_connector_helper_detect_from_ddc,
>> @@ -81,6 +85,53 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>>   	.atomic_disable = hibmc_dp_encoder_disable,
>>   };
>>   
>> +static int hibmc_dp_cfg(struct hibmc_dp *dp)
>> +{
>> +	int ret;
>> +
>> +	ret = hibmc_dp_hw_init(dp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	hibmc_dp_display_en(dp, false);
> Does this enable video signal? You should only enable AUX and perform
> link training at this point.

It's a switch function, the parameter false.
Also I can change the function name.


>> +
>> +	return 0;
>> +}
>> +
>> +irqreturn_t hibmc_dp_hpd_event_isr(int irq, void *arg)
>> +{
>> +	struct drm_device *dev = (struct drm_device *)arg;
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> +	enum hibmc_dp_hpd_status status;
>> +	int idx;
>> +
>> +	if (!drm_dev_enter(dev, &idx))
>> +		return -ENODEV;
>> +
>> +	status  = hibmc_dp_get_hpd_status(&priv->dp);
>> +	if (status == HIBMC_DP_HPD_IN) {
>> +		if (!(priv->dp.hpd_status & DP_MASKED_SINK_HPD_PLUG_INT))
>> +			goto err;
>> +		drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
>> +		hibmc_dp_cfg(&priv->dp);
>> +		drm_kms_helper_connector_hotplug_event(&priv->dp.connector);
>> +	} else if (status == HIBMC_DP_HPD_OUT) {
>> +		if (!(priv->dp.hpd_status & DP_MASKED_SINK_HPD_UNPLUG_INT))
>> +			goto err;
>> +		drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
>> +		drm_kms_helper_connector_hotplug_event(&priv->dp.connector);
> This looks like a hand-coded drm_connector_helper_hpd_irq_event()

Ok. I will change to it.


>> +	} else {
>> +		drm_warn(&priv->dev, "HPD unknown isr occur, status: %d\n", status);
>> +	}
>> +
>> +err:
>> +	priv->dp.hpd_status = 0;
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>   int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   {
>>   	struct drm_device *dev = &priv->dev;
>> @@ -93,14 +144,12 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>   	dp->mmio = priv->mmio;
>>   	dp->drm_dev = dev;
>>   
>> -	ret = hibmc_dp_hw_init(&priv->dp);
>> +	ret = hibmc_dp_cfg(dp);
>>   	if (ret) {
>> -		drm_err(dev, "hibmc dp hw init failed: %d\n", ret);
>> +		drm_err(dev, "hibmc dp cfg failed: %d\n", ret);
>>   		return ret;
>>   	}
>>   
>> -	hibmc_dp_display_en(&priv->dp, false);
>> -
>>   	encoder->possible_crtcs = drm_crtc_mask(crtc);
>>   	ret = drmm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
>>   	if (ret) {
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index bade693d9730..199acb3e7a73 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -29,9 +29,13 @@
>>   #include "hibmc_drm_regs.h"
>>   
>>   #define HIBMC_DP_HOST_SERDES_CTRL	0x1f001c
>> +#define HIBMC_DP_INTSTAT		0x1e0724
>> +#define HIBMC_DP_INTCLR			0x1e0728
> Why are those defined outside of hibmc_drm_regs.h?

Ok, I will fix it.


>>   
>>   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 +53,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.hpd_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 +268,52 @@ 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)
>> +{
>> +	drm_atomic_helper_shutdown(dev);
>> +
>> +	pci_disable_msi(to_pci_dev(dev->dev));
>> +}
>> +
>> +static int hibmc_msi_init(struct drm_device *dev)
>>   {
>> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>> +	int ret;
>>   
>> -	drm_atomic_helper_shutdown(dev);
>> +	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;
>> +	}
>>   
>> -	free_irq(pdev->irq, dev);
>> +	priv->valid_irq_num = ret;
>>   
>> -	pci_disable_msi(to_pci_dev(dev->dev));
>> +	priv->irqs = drmm_kcalloc(dev, priv->valid_irq_num,
>> +				  sizeof(struct hibmc_irq), GFP_KERNEL);
> Why do you need an extra pointer?  You know that there are no more than
> HIBMC_MAX_VECTORS, so you can have this data as a part of your private
> structure.

Ok.


>> +	if (!priv->irqs)
>> +		return -ENOMEM;
>> +
>> +	for (int i = 0; i < priv->valid_irq_num; i++) {
>> +		snprintf(priv->irqs[i].name, sizeof(priv->irqs[i].name) - 1, "%s-%s-%s",
>> +			 dev->driver->name, pci_name(pdev), g_irqs_names_map[i]);
>> +
>> +		priv->irqs[i].irq_num = pci_irq_vector(pdev, i);
>> +
>> +		if (i)
>> +			/* PCI devices require shared interrupts. */
>> +			ret = devm_request_threaded_irq(&pdev->dev, priv->irqs[i].irq_num,
>> +							hibmc_dp_interrupt, hibmc_dp_hpd_event_isr,
>> +							IRQF_SHARED, priv->irqs[i].name, dev);
>> +		else
>> +			ret = devm_request_irq(&pdev->dev, priv->irqs[i].irq_num, hibmc_interrupt,
>> +					       IRQF_SHARED, priv->irqs[i].name, dev);
> This feels like "too many changes". You have MSI-related changes, devm_,
> your HPD interrupt. Please split this patch into logical parts.

Alright, I'll fix them and split it, thanks.


>> +		if (ret) {
>> +			drm_err(dev, "install irq failed: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -288,15 +345,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 +424,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 bc89e4b9f4e3..764e211a8779 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -22,6 +22,14 @@
>>   
>>   #include "dp/dp_hw.h"
>>   
>> +#define HIBMC_MIN_VECTORS	1
>> +#define HIBMC_MAX_VECTORS	2
>> +
>> +struct hibmc_irq {
>> +	s32 irq_num;
>> +	char name[32];
>> +};
>> +
>>   struct hibmc_vdac {
>>   	struct drm_device *dev;
>>   	struct drm_encoder encoder;
>> @@ -40,6 +48,10 @@ struct hibmc_drm_private {
>>   	struct drm_crtc crtc;
>>   	struct hibmc_vdac vdac;
>>   	struct hibmc_dp dp;
>> +
>> +	/* irqs */
>> +	struct hibmc_irq *irqs;
>> +	u32 valid_irq_num;
>>   };
>>   
>>   static inline struct hibmc_vdac *to_hibmc_vdac(struct drm_connector *connector)
>> @@ -71,4 +83,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_event_isr(int irq, void *arg);
>> +
>>   #endif
>> -- 
>> 2.33.0
>>