From: Baihan Li <libaihan@huawei.com>
Enable HPD feature and add its isr and event function. Add a drm client
dev and realized the hotplug callback in it.
Signed-off-by: Baihan Li <libaihan@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
- remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
- remove enble_display in ISR, suggested by Dmitry Baryshkov.
- change drm_kms_helper_connector_hotplug_event() to
drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
- move macros to dp_reg.h, suggested by Dmitry Baryshkov.
- remove struct irqs, suggested by Dmitry Baryshkov.
- split this patch into two parts, suggested by Dmitry Baryshkov.
- add a drm client dev to handle HPD event.
v1 -> v2:
- optimizing the description in commit message, suggested by Dmitry Baryshkov.
- add mdelay(100) comments, suggested by Dmitry Baryshkov.
- deleting display enable in hpd event, suggested by Dmitry Baryshkov.
---
.../gpu/drm/hisilicon/hibmc/dp/dp_config.h | 1 +
drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 22 +++++++
drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h | 6 ++
.../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 61 +++++++++++++++++++
.../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +
5 files changed, 92 insertions(+)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
index c5feef8dc27d..08f9e1caf7fc 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
@@ -16,5 +16,6 @@
#define HIBMC_DP_SYNC_EN_MASK 0x3
#define HIBMC_DP_LINK_RATE_CAL 27
#define HIBMC_DP_SYNC_DELAY(lanes) ((lanes) == 0x2 ? 86 : 46)
+#define HIBMC_DP_INT_ENABLE 0xc
#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index a921b98dbf50..b2116395b8dd 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
/* int init */
writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
+ writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
/* rst */
writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
/* clock enable */
@@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
return 0;
}
+void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
+{
+ struct hibmc_dp_dev *dp_dev = dp->dp_dev;
+
+ hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
+ hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
+ hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
+ writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
+ writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
+ writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
+ writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
+ writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
+ writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
+}
+
void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
{
struct hibmc_dp_dev *dp_dev = dp->dp_dev;
@@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
return 0;
}
+void hibmc_dp_reset_link(struct hibmc_dp *dp)
+{
+ dp->dp_dev->link.status.clock_recovered = false;
+ dp->dp_dev->link.status.channel_equalized = false;
+}
+
static const struct hibmc_dp_color_raw g_rgb_raw[] = {
{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
{CBAR_WHITE, 0xfff, 0xfff, 0xfff},
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 83a53dae8012..a55d66d53966 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -11,6 +11,7 @@
#include <drm/drm_connector.h>
#include <drm/drm_print.h>
#include <drm/display/drm_dp_helper.h>
+#include <drm/drm_client.h>
struct hibmc_dp_dev;
@@ -49,11 +50,16 @@ struct hibmc_dp {
void __iomem *mmio;
struct drm_dp_aux aux;
struct hibmc_dp_cbar_cfg cfg;
+ u32 irq_status;
+ u32 hpd_status;
+ struct drm_client_dev client;
};
int hibmc_dp_hw_init(struct hibmc_dp *dp);
int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
+void hibmc_dp_reset_link(struct hibmc_dp *dp);
+void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
#endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index a7f611e82f73..40a3ebb8ac4b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -9,10 +9,13 @@
#include <drm/drm_modes.h>
#include <drm/drm_drv.h>
#include <drm/drm_edid.h>
+#include <drm/drm_client.h>
#include "hibmc_drm_drv.h"
#include "dp/dp_hw.h"
+#define DP_MASKED_SINK_HPD_PLUG_INT BIT(2)
+
static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
{
struct hibmc_dp *dp = to_hibmc_dp(connector);
@@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
.atomic_disable = hibmc_dp_encoder_disable,
};
+irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
+{
+ struct drm_device *dev = (struct drm_device *)arg;
+ struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
+ int idx;
+
+ if (!drm_dev_enter(dev, &idx))
+ return -ENODEV;
+
+ if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
+ drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
+ priv->dp.hpd_status = 1;
+ } else {
+ drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
+ priv->dp.hpd_status = 0;
+ }
+
+ if (dev->registered)
+ drm_connector_helper_hpd_irq_event(&priv->dp.connector);
+
+ drm_dev_exit(idx);
+
+ return IRQ_HANDLED;
+}
+
+static int hibmc_dp_hpd_event(struct drm_client_dev *client)
+{
+ struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
+ struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
+ struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
+ int ret;
+
+ if (dp->hpd_status) {
+ hibmc_dp_hpd_cfg(&priv->dp);
+ ret = hibmc_dp_prepare(dp, mode);
+ if (ret)
+ return ret;
+
+ hibmc_dp_display_en(dp, true);
+ } else {
+ hibmc_dp_display_en(dp, false);
+ hibmc_dp_reset_link(&priv->dp);
+ }
+
+ return 0;
+}
+
+static const struct drm_client_funcs hibmc_dp_client_funcs = {
+ .hotplug = hibmc_dp_hpd_event,
+ .unregister = drm_client_release,
+};
+
int hibmc_dp_init(struct hibmc_drm_private *priv)
{
struct drm_device *dev = &priv->dev;
@@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
drm_connector_attach_encoder(connector, encoder);
+ ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
+ if (ret)
+ return ret;
+
+ drm_client_register(&dp->client);
+
return 0;
}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index bc89e4b9f4e3..daed1330b961 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
+irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
+
#endif
--
2.33.0Hi Yongbang, kernel test robot noticed the following build errors: [auto build test ERROR on linus/master] [also build test ERROR on v6.14-rc3 next-20250221] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Yongbang-Shi/drm-hisilicon-hibmc-Restructuring-the-header-dp_reg-h/20250222-110052 base: linus/master patch link: https://lore.kernel.org/r/20250222025102.1519798-8-shiyongbang%40huawei.com patch subject: [PATCH v3 drm-dp 7/8] drm/hisilicon/hibmc: Enable this hot plug detect of irq feature config: arm64-randconfig-004-20250223 (https://download.01.org/0day-ci/archive/20250223/202502231304.BCzV4Y8D-lkp@intel.com/config) compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 204dcafec0ecf0db81d420d2de57b02ada6b09ec) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250223/202502231304.BCzV4Y8D-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202502231304.BCzV4Y8D-lkp@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "drm_client_init" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined! >> ERROR: modpost: "drm_client_register" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined! >> ERROR: modpost: "drm_client_release" [drivers/gpu/drm/hisilicon/hibmc/hibmc-drm.ko] undefined! -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
On Sat, Feb 22, 2025 at 10:51:00AM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
>
> Enable HPD feature and add its isr and event function. Add a drm client
> dev and realized the hotplug callback in it.
What for? There should be no need to add a separate drm client just for
the hotplug.
>
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
> - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
> - remove enble_display in ISR, suggested by Dmitry Baryshkov.
> - change drm_kms_helper_connector_hotplug_event() to
> drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
> - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
> - remove struct irqs, suggested by Dmitry Baryshkov.
> - split this patch into two parts, suggested by Dmitry Baryshkov.
> - add a drm client dev to handle HPD event.
> v1 -> v2:
> - optimizing the description in commit message, suggested by Dmitry Baryshkov.
> - add mdelay(100) comments, suggested by Dmitry Baryshkov.
> - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
> ---
> .../gpu/drm/hisilicon/hibmc/dp/dp_config.h | 1 +
> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 22 +++++++
> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h | 6 ++
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 61 +++++++++++++++++++
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +
> 5 files changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> index c5feef8dc27d..08f9e1caf7fc 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
> @@ -16,5 +16,6 @@
> #define HIBMC_DP_SYNC_EN_MASK 0x3
> #define HIBMC_DP_LINK_RATE_CAL 27
> #define HIBMC_DP_SYNC_DELAY(lanes) ((lanes) == 0x2 ? 86 : 46)
> +#define HIBMC_DP_INT_ENABLE 0xc
>
> #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index a921b98dbf50..b2116395b8dd 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
> /* int init */
> writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
> writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
> + writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
> /* rst */
> writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
> /* clock enable */
> @@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
> return 0;
> }
>
> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
> +{
> + struct hibmc_dp_dev *dp_dev = dp->dp_dev;
> +
> + hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
> + hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
> + hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
> + writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
> + writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
> + writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
> + writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
> + writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
> + writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
> +}
> +
> void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
> {
> struct hibmc_dp_dev *dp_dev = dp->dp_dev;
> @@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
> return 0;
> }
>
> +void hibmc_dp_reset_link(struct hibmc_dp *dp)
> +{
> + dp->dp_dev->link.status.clock_recovered = false;
> + dp->dp_dev->link.status.channel_equalized = false;
Could you please point out, where the link rate and number of lanes are
reset?
> +}
> +
> static const struct hibmc_dp_color_raw g_rgb_raw[] = {
> {CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
> {CBAR_WHITE, 0xfff, 0xfff, 0xfff},
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> index 83a53dae8012..a55d66d53966 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -11,6 +11,7 @@
> #include <drm/drm_connector.h>
> #include <drm/drm_print.h>
> #include <drm/display/drm_dp_helper.h>
> +#include <drm/drm_client.h>
>
> struct hibmc_dp_dev;
>
> @@ -49,11 +50,16 @@ struct hibmc_dp {
> void __iomem *mmio;
> struct drm_dp_aux aux;
> struct hibmc_dp_cbar_cfg cfg;
> + u32 irq_status;
> + u32 hpd_status;
> + struct drm_client_dev client;
> };
>
> int hibmc_dp_hw_init(struct hibmc_dp *dp);
> int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
> void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
> void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
> +void hibmc_dp_reset_link(struct hibmc_dp *dp);
> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>
> #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index a7f611e82f73..40a3ebb8ac4b 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -9,10 +9,13 @@
> #include <drm/drm_modes.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_edid.h>
> +#include <drm/drm_client.h>
>
> #include "hibmc_drm_drv.h"
> #include "dp/dp_hw.h"
>
> +#define DP_MASKED_SINK_HPD_PLUG_INT BIT(2)
> +
> static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
> {
> struct hibmc_dp *dp = to_hibmc_dp(connector);
> @@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
> .atomic_disable = hibmc_dp_encoder_disable,
> };
>
> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
> +{
> + struct drm_device *dev = (struct drm_device *)arg;
> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> + int idx;
> +
> + if (!drm_dev_enter(dev, &idx))
> + return -ENODEV;
> +
> + if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
> + drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
> + priv->dp.hpd_status = 1;
> + } else {
> + drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
> + priv->dp.hpd_status = 0;
> + }
> +
> + if (dev->registered)
> + drm_connector_helper_hpd_irq_event(&priv->dp.connector);
> +
> + drm_dev_exit(idx);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> +{
> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> + int ret;
> +
> + if (dp->hpd_status) {
> + hibmc_dp_hpd_cfg(&priv->dp);
> + ret = hibmc_dp_prepare(dp, mode);
> + if (ret)
> + return ret;
> +
> + hibmc_dp_display_en(dp, true);
> + } else {
> + hibmc_dp_display_en(dp, false);
> + hibmc_dp_reset_link(&priv->dp);
> + }
If I understand this correctly, you are using a separate drm_client to
enable and disable the link & display. Why is it necessary? Existing
drm_clients and userspace compositors use drm framework, they should be
able to turn the display on and off as required.
> +
> + return 0;
> +}
> +
> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
> + .hotplug = hibmc_dp_hpd_event,
> + .unregister = drm_client_release,
> +};
> +
> int hibmc_dp_init(struct hibmc_drm_private *priv)
> {
> struct drm_device *dev = &priv->dev;
> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>
> drm_connector_attach_encoder(connector, encoder);
>
> + ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
> + if (ret)
> + return ret;
> +
> + drm_client_register(&dp->client);
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index bc89e4b9f4e3..daed1330b961 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>
> void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>
> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
> +
> #endif
> --
> 2.33.0
>
--
With best wishes
Dmitry
> On Sat, Feb 22, 2025 at 10:51:00AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Enable HPD feature and add its isr and event function. Add a drm client
>> dev and realized the hotplug callback in it.
> What for? There should be no need to add a separate drm client just for
> the hotplug.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>> - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
>> - remove enble_display in ISR, suggested by Dmitry Baryshkov.
>> - change drm_kms_helper_connector_hotplug_event() to
>> drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
>> - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
>> - remove struct irqs, suggested by Dmitry Baryshkov.
>> - split this patch into two parts, suggested by Dmitry Baryshkov.
>> - add a drm client dev to handle HPD event.
>> v1 -> v2:
>> - optimizing the description in commit message, suggested by Dmitry Baryshkov.
>> - add mdelay(100) comments, suggested by Dmitry Baryshkov.
>> - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
>> ---
>> .../gpu/drm/hisilicon/hibmc/dp/dp_config.h | 1 +
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 22 +++++++
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h | 6 ++
>> .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 61 +++++++++++++++++++
>> .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +
>> 5 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> index c5feef8dc27d..08f9e1caf7fc 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> @@ -16,5 +16,6 @@
>> #define HIBMC_DP_SYNC_EN_MASK 0x3
>> #define HIBMC_DP_LINK_RATE_CAL 27
>> #define HIBMC_DP_SYNC_DELAY(lanes) ((lanes) == 0x2 ? 86 : 46)
>> +#define HIBMC_DP_INT_ENABLE 0xc
>>
>> #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index a921b98dbf50..b2116395b8dd 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>> /* int init */
>> writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> + writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> /* rst */
>> writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> /* clock enable */
>> @@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>> return 0;
>> }
>>
>> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
>> +{
>> + struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> +
>> + hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> + hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> + hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
>> + writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
>> + writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> + writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> + writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> + writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> + writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
>> +}
>> +
>> void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
>> {
>> struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> @@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>> return 0;
>> }
>>
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp)
>> +{
>> + dp->dp_dev->link.status.clock_recovered = false;
>> + dp->dp_dev->link.status.channel_equalized = false;
> Could you please point out, where the link rate and number of lanes are
> reset?
Thanks for your reminder, I will reset link rate and lanes here too.
>> +}
>> +
>> static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>> {CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>> {CBAR_WHITE, 0xfff, 0xfff, 0xfff},
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 83a53dae8012..a55d66d53966 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -11,6 +11,7 @@
>> #include <drm/drm_connector.h>
>> #include <drm/drm_print.h>
>> #include <drm/display/drm_dp_helper.h>
>> +#include <drm/drm_client.h>
>>
>> struct hibmc_dp_dev;
>>
>> @@ -49,11 +50,16 @@ struct hibmc_dp {
>> void __iomem *mmio;
>> struct drm_dp_aux aux;
>> struct hibmc_dp_cbar_cfg cfg;
>> + u32 irq_status;
>> + u32 hpd_status;
>> + struct drm_client_dev client;
>> };
>>
>> int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>> void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>> void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp);
>> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>>
>> #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index a7f611e82f73..40a3ebb8ac4b 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -9,10 +9,13 @@
>> #include <drm/drm_modes.h>
>> #include <drm/drm_drv.h>
>> #include <drm/drm_edid.h>
>> +#include <drm/drm_client.h>
>>
>> #include "hibmc_drm_drv.h"
>> #include "dp/dp_hw.h"
>>
>> +#define DP_MASKED_SINK_HPD_PLUG_INT BIT(2)
>> +
>> static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>> {
>> struct hibmc_dp *dp = to_hibmc_dp(connector);
>> @@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>> .atomic_disable = hibmc_dp_encoder_disable,
>> };
>>
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
>> +{
>> + struct drm_device *dev = (struct drm_device *)arg;
>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> + int idx;
>> +
>> + if (!drm_dev_enter(dev, &idx))
>> + return -ENODEV;
>> +
>> + if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
>> + drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
>> + priv->dp.hpd_status = 1;
>> + } else {
>> + drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
>> + priv->dp.hpd_status = 0;
>> + }
>> +
>> + if (dev->registered)
>> + drm_connector_helper_hpd_irq_event(&priv->dp.connector);
>> +
>> + drm_dev_exit(idx);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>> +{
>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>> + int ret;
>> +
>> + if (dp->hpd_status) {
>> + hibmc_dp_hpd_cfg(&priv->dp);
>> + ret = hibmc_dp_prepare(dp, mode);
>> + if (ret)
>> + return ret;
>> +
>> + hibmc_dp_display_en(dp, true);
>> + } else {
>> + hibmc_dp_display_en(dp, false);
>> + hibmc_dp_reset_link(&priv->dp);
>> + }
> If I understand this correctly, you are using a separate drm_client to
> enable and disable the link & display. Why is it necessary? Existing
> drm_clients and userspace compositors use drm framework, they should be
> able to turn the display on and off as required.
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
>> + .hotplug = hibmc_dp_hpd_event,
>> + .unregister = drm_client_release,
>> +};
>> +
>> int hibmc_dp_init(struct hibmc_drm_private *priv)
>> {
>> struct drm_device *dev = &priv->dev;
>> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>
>> drm_connector_attach_encoder(connector, encoder);
>>
>> + ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
>> + if (ret)
>> + return ret;
>> +
>> + drm_client_register(&dp->client);
>> +
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index bc89e4b9f4e3..daed1330b961 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>>
>> void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
>> +
>> #endif
>> --
>> 2.33.0
>>
>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>> +{
>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>> + int ret;
>> +
>> + if (dp->hpd_status) {
>> + hibmc_dp_hpd_cfg(&priv->dp);
>> + ret = hibmc_dp_prepare(dp, mode);
>> + if (ret)
>> + return ret;
>> +
>> + hibmc_dp_display_en(dp, true);
>> + } else {
>> + hibmc_dp_display_en(dp, false);
>> + hibmc_dp_reset_link(&priv->dp);
>> + }
> If I understand this correctly, you are using a separate drm_client to
> enable and disable the link & display. Why is it necessary? Existing
> drm_clients and userspace compositors use drm framework, they should be
> able to turn the display on and off as required.
>
Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
the different video modes into DP registers.
>> +
>> + return 0;
>> +}
>> +
>> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
>> + .hotplug = hibmc_dp_hpd_event,
>> + .unregister = drm_client_release,
>> +};
>> +
>> int hibmc_dp_init(struct hibmc_drm_private *priv)
>> {
>> struct drm_device *dev = &priv->dev;
>> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>
>> drm_connector_attach_encoder(connector, encoder);
>>
>> + ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
>> + if (ret)
>> + return ret;
>> +
>> + drm_client_register(&dp->client);
>> +
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index bc89e4b9f4e3..daed1330b961 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>>
>> void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
>> +
>> #endif
>> --
>> 2.33.0
>>
On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > +{
> > > + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > + int ret;
> > > +
> > > + if (dp->hpd_status) {
> > > + hibmc_dp_hpd_cfg(&priv->dp);
> > > + ret = hibmc_dp_prepare(dp, mode);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + hibmc_dp_display_en(dp, true);
> > > + } else {
> > > + hibmc_dp_display_en(dp, false);
> > > + hibmc_dp_reset_link(&priv->dp);
> > > + }
> > If I understand this correctly, you are using a separate drm_client to
> > enable and disable the link & display. Why is it necessary? Existing
> > drm_clients and userspace compositors use drm framework, they should be
> > able to turn the display on and off as required.
> >
> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> the different video modes into DP registers.
Why? The link training and mode programming should happen during
pre_enable / enable stage (legacy or atomic).
>
>
--
With best wishes
Dmitry
> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>> +{
>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>> + int ret;
>>>> +
>>>> + if (dp->hpd_status) {
>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + hibmc_dp_display_en(dp, true);
>>>> + } else {
>>>> + hibmc_dp_display_en(dp, false);
>>>> + hibmc_dp_reset_link(&priv->dp);
>>>> + }
>>> If I understand this correctly, you are using a separate drm_client to
>>> enable and disable the link & display. Why is it necessary? Existing
>>> drm_clients and userspace compositors use drm framework, they should be
>>> able to turn the display on and off as required.
>>>
>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>> the different video modes into DP registers.
> Why? The link training and mode programming should happen during
> pre_enable / enable stage (legacy or atomic).
Hi Dmitry,
Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.
>>
On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
> > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> >>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> >>>> +{
> >>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> >>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> >>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> >>>> + int ret;
> >>>> +
> >>>> + if (dp->hpd_status) {
> >>>> + hibmc_dp_hpd_cfg(&priv->dp);
> >>>> + ret = hibmc_dp_prepare(dp, mode);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + hibmc_dp_display_en(dp, true);
> >>>> + } else {
> >>>> + hibmc_dp_display_en(dp, false);
> >>>> + hibmc_dp_reset_link(&priv->dp);
> >>>> + }
> >>> If I understand this correctly, you are using a separate drm_client to
> >>> enable and disable the link & display. Why is it necessary? Existing
> >>> drm_clients and userspace compositors use drm framework, they should be
> >>> able to turn the display on and off as required.
> >>>
> >> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> >> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> >> the different video modes into DP registers.
> > Why? The link training and mode programming should happen during
> > pre_enable / enable stage (legacy or atomic).
>
> Hi Dmitry,
>
> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
It should be userspace, who triggers the enable/disable (or it should
be the in-kernel fbdev / fbcon, which interface through the generic
drm_fbdev client).
--
With best wishes
Dmitry
> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>> +{
>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (dp->hpd_status) {
>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>> + } else {
>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>> + }
>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>> able to turn the display on and off as required.
>>>>>
>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>> the different video modes into DP registers.
>>> Why? The link training and mode programming should happen during
>>> pre_enable / enable stage (legacy or atomic).
>> Hi Dmitry,
>>
>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> It should be userspace, who triggers the enable/disable (or it should
> be the in-kernel fbdev / fbcon, which interface through the generic
> drm_fbdev client).
Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
by user, but it won't call when HPD triggered .
On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > +{
> > > > > > > + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (dp->hpd_status) {
> > > > > > > + hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > + ret = hibmc_dp_prepare(dp, mode);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + hibmc_dp_display_en(dp, true);
> > > > > > > + } else {
> > > > > > > + hibmc_dp_display_en(dp, false);
> > > > > > > + hibmc_dp_reset_link(&priv->dp);
> > > > > > > + }
> > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > able to turn the display on and off as required.
> > > > > >
> > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > the different video modes into DP registers.
> > > > Why? The link training and mode programming should happen during
> > > > pre_enable / enable stage (legacy or atomic).
> > > Hi Dmitry,
> > >
> > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > It should be userspace, who triggers the enable/disable (or it should
> > be the in-kernel fbdev / fbcon, which interface through the generic
> > drm_fbdev client).
>
> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> by user, but it won't call when HPD triggered .
- Is HPD even properly delivered to userspace? What kind of compsitor
are you using? Is .detect working properly and reporting a correct
plug-in state?
--
With best wishes
Dmitry
> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>> +{
>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + if (dp->hpd_status) {
>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>> + if (ret)
>>>>>>>> + return ret;
>>>>>>>> +
>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>> + } else {
>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>> + }
>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>> able to turn the display on and off as required.
>>>>>>>
>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>> the different video modes into DP registers.
>>>>> Why? The link training and mode programming should happen during
>>>>> pre_enable / enable stage (legacy or atomic).
>>>> Hi Dmitry,
>>>>
>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>> It should be userspace, who triggers the enable/disable (or it should
>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>> drm_fbdev client).
>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>> by user, but it won't call when HPD triggered .
> - Is HPD even properly delivered to userspace? What kind of compsitor
> are you using? Is .detect working properly and reporting a correct
> plug-in state?
Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
I use Xorg, and the display service is GDM.
The .detect is called and the getting modes info is correct.
I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>
> > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > +{
> > > > > > > > > + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > + int ret;
> > > > > > > > > +
> > > > > > > > > + if (dp->hpd_status) {
> > > > > > > > > + hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > + ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > + if (ret)
> > > > > > > > > + return ret;
> > > > > > > > > +
> > > > > > > > > + hibmc_dp_display_en(dp, true);
> > > > > > > > > + } else {
> > > > > > > > > + hibmc_dp_display_en(dp, false);
> > > > > > > > > + hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > + }
> > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > able to turn the display on and off as required.
> > > > > > > >
> > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > the different video modes into DP registers.
> > > > > > Why? The link training and mode programming should happen during
> > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > Hi Dmitry,
> > > > >
> > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > It should be userspace, who triggers the enable/disable (or it should
> > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > drm_fbdev client).
> > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > by user, but it won't call when HPD triggered .
> > - Is HPD even properly delivered to userspace? What kind of compsitor
> > are you using? Is .detect working properly and reporting a correct
> > plug-in state?
>
> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> I use Xorg, and the display service is GDM.
> The .detect is called and the getting modes info is correct.
> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
You can go to the display settings in GDM. It would be interesting to
observe if it notes the second monitor or not. Last, but not least, you
can use a simple tool like 'xrandr' under your XOrg session to set the
display resolution.
--
With best wishes
Dmitry
> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>> +{
>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>> + int ret;
>>>>>>>>>> +
>>>>>>>>>> + if (dp->hpd_status) {
>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>> + if (ret)
>>>>>>>>>> + return ret;
>>>>>>>>>> +
>>>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>>>> + } else {
>>>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>> + }
>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>
>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>> the different video modes into DP registers.
>>>>>>> Why? The link training and mode programming should happen during
>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>> drm_fbdev client).
>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>> by user, but it won't call when HPD triggered .
>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>> are you using? Is .detect working properly and reporting a correct
>>> plug-in state?
>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>> I use Xorg, and the display service is GDM.
>> The .detect is called and the getting modes info is correct.
>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> You can go to the display settings in GDM. It would be interesting to
> observe if it notes the second monitor or not. Last, but not least, you
> can use a simple tool like 'xrandr' under your XOrg session to set the
> display resolution.
Thank you for your advice!
Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
So do I need to clear the vga connector, if dp is plugged in?
And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
but there are errs when set some low resolutions.
On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>
> > On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> > > > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > > > +{
> > > > > > > > > > > + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > > > + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > > > + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > > > + int ret;
> > > > > > > > > > > +
> > > > > > > > > > > + if (dp->hpd_status) {
> > > > > > > > > > > + hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > > > + ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > > > + if (ret)
> > > > > > > > > > > + return ret;
> > > > > > > > > > > +
> > > > > > > > > > > + hibmc_dp_display_en(dp, true);
> > > > > > > > > > > + } else {
> > > > > > > > > > > + hibmc_dp_display_en(dp, false);
> > > > > > > > > > > + hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > > > + }
> > > > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > > > able to turn the display on and off as required.
> > > > > > > > > >
> > > > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > > > the different video modes into DP registers.
> > > > > > > > Why? The link training and mode programming should happen during
> > > > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > > > It should be userspace, who triggers the enable/disable (or it should
> > > > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > > > drm_fbdev client).
> > > > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > > > by user, but it won't call when HPD triggered .
> > > > - Is HPD even properly delivered to userspace? What kind of compsitor
> > > > are you using? Is .detect working properly and reporting a correct
> > > > plug-in state?
> > > Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> > > this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> > > I use Xorg, and the display service is GDM.
> > > The .detect is called and the getting modes info is correct.
> > > I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> > You can go to the display settings in GDM. It would be interesting to
> > observe if it notes the second monitor or not. Last, but not least, you
> > can use a simple tool like 'xrandr' under your XOrg session to set the
> > display resolution.
>
> Thank you for your advice!
> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> So do I need to clear the vga connector, if dp is plugged in?
Unless your hardware can not manage two outputs at the same time, no,
you don't have to. Just check how it behaves on x86 systems. Ideally
your driver should have the same behaviour.
> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> but there are errs when set some low resolutions.
That's a separate topic, most likely related to timing or to some other
issues. You can fix that separately (but please do, switching modes
should work).
--
With best wishes
Dmitry
> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>> + int ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (dp->hpd_status) {
>>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>> + if (ret)
>>>>>>>>>>>> + return ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>> + }
>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>
>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>> drm_fbdev client).
>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>> by user, but it won't call when HPD triggered .
>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>> are you using? Is .detect working properly and reporting a correct
>>>>> plug-in state?
>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>> I use Xorg, and the display service is GDM.
>>>> The .detect is called and the getting modes info is correct.
>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>> You can go to the display settings in GDM. It would be interesting to
>>> observe if it notes the second monitor or not. Last, but not least, you
>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>> display resolution.
>> Thank you for your advice!
>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>> So do I need to clear the vga connector, if dp is plugged in?
> Unless your hardware can not manage two outputs at the same time, no,
> you don't have to. Just check how it behaves on x86 systems. Ideally
> your driver should have the same behaviour.
Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>> but there are errs when set some low resolutions.
> That's a separate topic, most likely related to timing or to some other
> issues. You can fix that separately (but please do, switching modes
> should work).
Okay!
On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
>
> > On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> >>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> >>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> >>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> >>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> >>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> >>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> >>>>>>>>>>>> + int ret;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> + if (dp->hpd_status) {
> >>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
> >>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
> >>>>>>>>>>>> + if (ret)
> >>>>>>>>>>>> + return ret;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
> >>>>>>>>>>>> + } else {
> >>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
> >>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
> >>>>>>>>>>>> + }
> >>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
> >>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
> >>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
> >>>>>>>>>>> able to turn the display on and off as required.
> >>>>>>>>>>>
> >>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> >>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> >>>>>>>>>> the different video modes into DP registers.
> >>>>>>>>> Why? The link training and mode programming should happen during
> >>>>>>>>> pre_enable / enable stage (legacy or atomic).
> >>>>>>>> Hi Dmitry,
> >>>>>>>>
> >>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> >>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> >>>>>>> It should be userspace, who triggers the enable/disable (or it should
> >>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
> >>>>>>> drm_fbdev client).
> >>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> >>>>>> by user, but it won't call when HPD triggered .
> >>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
> >>>>> are you using? Is .detect working properly and reporting a correct
> >>>>> plug-in state?
> >>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> >>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> >>>> I use Xorg, and the display service is GDM.
> >>>> The .detect is called and the getting modes info is correct.
> >>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> >>> You can go to the display settings in GDM. It would be interesting to
> >>> observe if it notes the second monitor or not. Last, but not least, you
> >>> can use a simple tool like 'xrandr' under your XOrg session to set the
> >>> display resolution.
> >> Thank you for your advice!
> >> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> >> So do I need to clear the vga connector, if dp is plugged in?
> > Unless your hardware can not manage two outputs at the same time, no,
> > you don't have to. Just check how it behaves on x86 systems. Ideally
> > your driver should have the same behaviour.
>
> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
I think registering a single CRTC is a correct way. Then it is logical
that there is no mode set on the DP when you connect it. The userspace
can not output any data. However if you disconnect VGA and connect DP
then it should become active and should output your desktop
environment.
>
> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>
> >> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> >> but there are errs when set some low resolutions.
> > That's a separate topic, most likely related to timing or to some other
> > issues. You can fix that separately (but please do, switching modes
> > should work).
>
> Okay!
>
>
--
With best wishes
Dmitry
> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>
>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>> + int ret;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (dp->hpd_status) {
>>>>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>> + if (ret)
>>>>>>>>>>>>>> + return ret;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>
>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>> drm_fbdev client).
>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>> are you using? Is .detect working properly and reporting a correct
>>>>>>> plug-in state?
>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>> I use Xorg, and the display service is GDM.
>>>>>> The .detect is called and the getting modes info is correct.
>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>> display resolution.
>>>> Thank you for your advice!
>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>> So do I need to clear the vga connector, if dp is plugged in?
>>> Unless your hardware can not manage two outputs at the same time, no,
>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>> your driver should have the same behaviour.
>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> I think registering a single CRTC is a correct way. Then it is logical
> that there is no mode set on the DP when you connect it. The userspace
> can not output any data. However if you disconnect VGA and connect DP
> then it should become active and should output your desktop
> environment.
Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
userapce will active and enanble DP, right?
>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>
>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>> but there are errs when set some low resolutions.
>>> That's a separate topic, most likely related to timing or to some other
>>> issues. You can fix that separately (but please do, switching modes
>>> should work).
>> Okay!
>>
>>
>
On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>
> > On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > >
> > > > On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> > > > > > On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> > > > > > > > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > > > > > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > > > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > > > > > > > + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > > > > > > > + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > > > > > > > + int ret;
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > + if (dp->hpd_status) {
> > > > > > > > > > > > > > > + hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > > > > > > > + ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > > > > > > > + if (ret)
> > > > > > > > > > > > > > > + return ret;
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > + hibmc_dp_display_en(dp, true);
> > > > > > > > > > > > > > > + } else {
> > > > > > > > > > > > > > > + hibmc_dp_display_en(dp, false);
> > > > > > > > > > > > > > > + hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > > > > > > > able to turn the display on and off as required.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > > > > > > > the different video modes into DP registers.
> > > > > > > > > > > > Why? The link training and mode programming should happen during
> > > > > > > > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > >
> > > > > > > > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > > > > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > > > > > > > It should be userspace, who triggers the enable/disable (or it should
> > > > > > > > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > > > > > > > drm_fbdev client).
> > > > > > > > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > > > > > > > by user, but it won't call when HPD triggered .
> > > > > > > > - Is HPD even properly delivered to userspace? What kind of compsitor
> > > > > > > > are you using? Is .detect working properly and reporting a correct
> > > > > > > > plug-in state?
> > > > > > > Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> > > > > > > this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> > > > > > > I use Xorg, and the display service is GDM.
> > > > > > > The .detect is called and the getting modes info is correct.
> > > > > > > I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> > > > > > You can go to the display settings in GDM. It would be interesting to
> > > > > > observe if it notes the second monitor or not. Last, but not least, you
> > > > > > can use a simple tool like 'xrandr' under your XOrg session to set the
> > > > > > display resolution.
> > > > > Thank you for your advice!
> > > > > Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> > > > > So do I need to clear the vga connector, if dp is plugged in?
> > > > Unless your hardware can not manage two outputs at the same time, no,
> > > > you don't have to. Just check how it behaves on x86 systems. Ideally
> > > > your driver should have the same behaviour.
> > > Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> > > with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> > I think registering a single CRTC is a correct way. Then it is logical
> > that there is no mode set on the DP when you connect it. The userspace
> > can not output any data. However if you disconnect VGA and connect DP
> > then it should become active and should output your desktop
> > environment.
>
> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
> userapce will active and enanble DP, right?
Yes.
>
>
> > > And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
> > >
> > > > > And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> > > > > but there are errs when set some low resolutions.
> > > > That's a separate topic, most likely related to timing or to some other
> > > > issues. You can fix that separately (but please do, switching modes
> > > > should work).
> > > Okay!
> > >
> > >
> >
--
With best wishes
Dmitry
> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>>>> + int ret;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (dp->hpd_status) {
>>>>>>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>>>> + if (ret)
>>>>>>>>>>>>>>>> + return ret;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>
>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>>>> drm_fbdev client).
>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>>> are you using? Is .detect working properly and reporting a correct
>>>>>>>>> plug-in state?
>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>>>> I use Xorg, and the display service is GDM.
>>>>>>>> The .detect is called and the getting modes info is correct.
>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>>>> display resolution.
>>>>>> Thank you for your advice!
>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>>>> So do I need to clear the vga connector, if dp is plugged in?
>>>>> Unless your hardware can not manage two outputs at the same time, no,
>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>>>> your driver should have the same behaviour.
>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
>>> I think registering a single CRTC is a correct way. Then it is logical
>>> that there is no mode set on the DP when you connect it. The userspace
>>> can not output any data. However if you disconnect VGA and connect DP
>>> then it should become active and should output your desktop
>>> environment.
>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
>> userapce will active and enanble DP, right?
> Yes.
I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
>>
>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>>>
>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>>>> but there are errs when set some low resolutions.
>>>>> That's a separate topic, most likely related to timing or to some other
>>>>> issues. You can fix that separately (but please do, switching modes
>>>>> should work).
>>>> Okay!
>>>>
>>>>
On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
> > On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
> > > > On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> > > > > > > > On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> > > > > > > > > > On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > > On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> > > > > > > > > > > > > > On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> > > > > > > > > > > > > > > > > +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> > > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > > + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> > > > > > > > > > > > > > > > > + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> > > > > > > > > > > > > > > > > + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> > > > > > > > > > > > > > > > > + int ret;
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > + if (dp->hpd_status) {
> > > > > > > > > > > > > > > > > + hibmc_dp_hpd_cfg(&priv->dp);
> > > > > > > > > > > > > > > > > + ret = hibmc_dp_prepare(dp, mode);
> > > > > > > > > > > > > > > > > + if (ret)
> > > > > > > > > > > > > > > > > + return ret;
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > + hibmc_dp_display_en(dp, true);
> > > > > > > > > > > > > > > > > + } else {
> > > > > > > > > > > > > > > > > + hibmc_dp_display_en(dp, false);
> > > > > > > > > > > > > > > > > + hibmc_dp_reset_link(&priv->dp);
> > > > > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > > > If I understand this correctly, you are using a separate drm_client to
> > > > > > > > > > > > > > > > enable and disable the link & display. Why is it necessary? Existing
> > > > > > > > > > > > > > > > drm_clients and userspace compositors use drm framework, they should be
> > > > > > > > > > > > > > > > able to turn the display on and off as required.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> > > > > > > > > > > > > > > We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> > > > > > > > > > > > > > > the different video modes into DP registers.
> > > > > > > > > > > > > > Why? The link training and mode programming should happen during
> > > > > > > > > > > > > > pre_enable / enable stage (legacy or atomic).
> > > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> > > > > > > > > > > > > And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> > > > > > > > > > > > It should be userspace, who triggers the enable/disable (or it should
> > > > > > > > > > > > be the in-kernel fbdev / fbcon, which interface through the generic
> > > > > > > > > > > > drm_fbdev client).
> > > > > > > > > > > Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> > > > > > > > > > > by user, but it won't call when HPD triggered .
> > > > > > > > > > - Is HPD even properly delivered to userspace? What kind of compsitor
> > > > > > > > > > are you using? Is .detect working properly and reporting a correct
> > > > > > > > > > plug-in state?
> > > > > > > > > Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> > > > > > > > > this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> > > > > > > > > I use Xorg, and the display service is GDM.
> > > > > > > > > The .detect is called and the getting modes info is correct.
> > > > > > > > > I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> > > > > > > > You can go to the display settings in GDM. It would be interesting to
> > > > > > > > observe if it notes the second monitor or not. Last, but not least, you
> > > > > > > > can use a simple tool like 'xrandr' under your XOrg session to set the
> > > > > > > > display resolution.
> > > > > > > Thank you for your advice!
> > > > > > > Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> > > > > > > So do I need to clear the vga connector, if dp is plugged in?
> > > > > > Unless your hardware can not manage two outputs at the same time, no,
> > > > > > you don't have to. Just check how it behaves on x86 systems. Ideally
> > > > > > your driver should have the same behaviour.
> > > > > Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> > > > > with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> > > > I think registering a single CRTC is a correct way. Then it is logical
> > > > that there is no mode set on the DP when you connect it. The userspace
> > > > can not output any data. However if you disconnect VGA and connect DP
> > > > then it should become active and should output your desktop
> > > > environment.
> > > Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
> > > userapce will active and enanble DP, right?
> > Yes.
>
> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
Huh? You should implement hibmc_connector_funcs.detect() or
.detect_ctx() to report VGA connector's status. Use
ast_vga_connector_helper_detect_ctx() as an inspiration.
> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
drm_connector_cleanup() should only be callsed if the connector is going
to be destroyed. You should not be callign that function.
>
>
> > >
> > > > > And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
> > > > >
> > > > > > > And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> > > > > > > but there are errs when set some low resolutions.
> > > > > > That's a separate topic, most likely related to timing or to some other
> > > > > > issues. You can fix that separately (but please do, switching modes
> > > > > > should work).
> > > > > Okay!
> > > > >
> > > > >
--
With best wishes
Dmitry
> On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
>>> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>>>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>>>>>> + int ret;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (dp->hpd_status) {
>>>>>>>>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>>>>>> + if (ret)
>>>>>>>>>>>>>>>>>> + return ret;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>>>>>> drm_fbdev client).
>>>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>>>>> are you using? Is .detect working properly and reporting a correct
>>>>>>>>>>> plug-in state?
>>>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>>>>>> I use Xorg, and the display service is GDM.
>>>>>>>>>> The .detect is called and the getting modes info is correct.
>>>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>>>>>> display resolution.
>>>>>>>> Thank you for your advice!
>>>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>>>>>> So do I need to clear the vga connector, if dp is plugged in?
>>>>>>> Unless your hardware can not manage two outputs at the same time, no,
>>>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>>>>>> your driver should have the same behaviour.
>>>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>>>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
>>>>> I think registering a single CRTC is a correct way. Then it is logical
>>>>> that there is no mode set on the DP when you connect it. The userspace
>>>>> can not output any data. However if you disconnect VGA and connect DP
>>>>> then it should become active and should output your desktop
>>>>> environment.
>>>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
>>>> userapce will active and enanble DP, right?
>>> Yes.
>> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
> Huh? You should implement hibmc_connector_funcs.detect() or
> .detect_ctx() to report VGA connector's status. Use
> ast_vga_connector_helper_detect_ctx() as an inspiration.
Okay, thanks for your correcting, I'll try to add drm_connector_helper_detect_from_ddc() in VGA's detect_ctx.
And also, I wanna make sure that do I need to unplug the VGA monitor manually when I plug in DP, or just
set the status of VGA connector to disconnected in interrupt handler.
>> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
> drm_connector_cleanup() should only be callsed if the connector is going
> to be destroyed. You should not be callign that function.
>
>>
>>>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>>>>>
>>>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>>>>>> but there are errs when set some low resolutions.
>>>>>>> That's a separate topic, most likely related to timing or to some other
>>>>>>> issues. You can fix that separately (but please do, switching modes
>>>>>>> should work).
>>>>>> Okay!
>>>>>>
>>>>>>
On Wed, 5 Mar 2025 at 08:29, Yongbang Shi <shiyongbang@huawei.com> wrote:
>
>
> > On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
> >>> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
> >>>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
> >>>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
> >>>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
> >>>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
> >>>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
> >>>>>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
> >>>>>>>>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
> >>>>>>>>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
> >>>>>>>>>>>>>>>>>> + int ret;
> >>>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>>> + if (dp->hpd_status) {
> >>>>>>>>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
> >>>>>>>>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
> >>>>>>>>>>>>>>>>>> + if (ret)
> >>>>>>>>>>>>>>>>>> + return ret;
> >>>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
> >>>>>>>>>>>>>>>>>> + } else {
> >>>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
> >>>>>>>>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
> >>>>>>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
> >>>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
> >>>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
> >>>>>>>>>>>>>>>>> able to turn the display on and off as required.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
> >>>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
> >>>>>>>>>>>>>>>> the different video modes into DP registers.
> >>>>>>>>>>>>>>> Why? The link training and mode programming should happen during
> >>>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
> >>>>>>>>>>>>>> Hi Dmitry,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
> >>>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
> >>>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
> >>>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
> >>>>>>>>>>>>> drm_fbdev client).
> >>>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
> >>>>>>>>>>>> by user, but it won't call when HPD triggered .
> >>>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
> >>>>>>>>>>> are you using? Is .detect working properly and reporting a correct
> >>>>>>>>>>> plug-in state?
> >>>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
> >>>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
> >>>>>>>>>> I use Xorg, and the display service is GDM.
> >>>>>>>>>> The .detect is called and the getting modes info is correct.
> >>>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
> >>>>>>>>> You can go to the display settings in GDM. It would be interesting to
> >>>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
> >>>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
> >>>>>>>>> display resolution.
> >>>>>>>> Thank you for your advice!
> >>>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
> >>>>>>>> So do I need to clear the vga connector, if dp is plugged in?
> >>>>>>> Unless your hardware can not manage two outputs at the same time, no,
> >>>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
> >>>>>>> your driver should have the same behaviour.
> >>>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
> >>>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
> >>>>> I think registering a single CRTC is a correct way. Then it is logical
> >>>>> that there is no mode set on the DP when you connect it. The userspace
> >>>>> can not output any data. However if you disconnect VGA and connect DP
> >>>>> then it should become active and should output your desktop
> >>>>> environment.
> >>>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
> >>>> userapce will active and enanble DP, right?
> >>> Yes.
> >> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
> > Huh? You should implement hibmc_connector_funcs.detect() or
> > .detect_ctx() to report VGA connector's status. Use
> > ast_vga_connector_helper_detect_ctx() as an inspiration.
>
> Okay, thanks for your correcting, I'll try to add drm_connector_helper_detect_from_ddc() in VGA's detect_ctx.
> And also, I wanna make sure that do I need to unplug the VGA monitor manually when I plug in DP, or just
> set the status of VGA connector to disconnected in interrupt handler.
If the HPD reports the status of the DP connector, why do you want to
change the status of the VGA connector?
>
>
> >> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
> > drm_connector_cleanup() should only be callsed if the connector is going
> > to be destroyed. You should not be callign that function.
> >
> >>
> >>>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
> >>>>>>
> >>>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
> >>>>>>>> but there are errs when set some low resolutions.
> >>>>>>> That's a separate topic, most likely related to timing or to some other
> >>>>>>> issues. You can fix that separately (but please do, switching modes
> >>>>>>> should work).
> >>>>>> Okay!
> >>>>>>
> >>>>>>
--
With best wishes
Dmitry
> On Wed, 5 Mar 2025 at 08:29, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>
>>> On Tue, Mar 04, 2025 at 10:23:14AM +0800, Yongbang Shi wrote:
>>>>> On Mon, Mar 03, 2025 at 01:02:44PM +0800, Yongbang Shi wrote:
>>>>>>> On Sat, 1 Mar 2025 at 11:54, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>> On Sat, Mar 01, 2025 at 04:45:40PM +0800, Yongbang Shi wrote:
>>>>>>>>>>> On Thu, Feb 27, 2025 at 09:46:10PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>> On Tue, Feb 25, 2025 at 09:57:17PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>> On Mon, 24 Feb 2025 at 16:03, Yongbang Shi <shiyongbang@huawei.com> wrote:
>>>>>>>>>>>>>>>>> On Sat, Feb 22, 2025 at 06:35:48PM +0800, Yongbang Shi wrote:
>>>>>>>>>>>>>>>>>>>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>>>>>>>>>>>>>>>>>>>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>>>>>>>>>>>>>>>>>>>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>>>>>>>>>>>>>>>>>>>> + int ret;
>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>> + if (dp->hpd_status) {
>>>>>>>>>>>>>>>>>>>> + hibmc_dp_hpd_cfg(&priv->dp);
>>>>>>>>>>>>>>>>>>>> + ret = hibmc_dp_prepare(dp, mode);
>>>>>>>>>>>>>>>>>>>> + if (ret)
>>>>>>>>>>>>>>>>>>>> + return ret;
>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, true);
>>>>>>>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>>>>>>>> + hibmc_dp_display_en(dp, false);
>>>>>>>>>>>>>>>>>>>> + hibmc_dp_reset_link(&priv->dp);
>>>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>>> If I understand this correctly, you are using a separate drm_client to
>>>>>>>>>>>>>>>>>>> enable and disable the link & display. Why is it necessary? Existing
>>>>>>>>>>>>>>>>>>> drm_clients and userspace compositors use drm framework, they should be
>>>>>>>>>>>>>>>>>>> able to turn the display on and off as required.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks for your asking, there are cfg/reset process when the connector 's pluging in/out.
>>>>>>>>>>>>>>>>>> We want to cfg DP registers again when the connector changes. Not only dp link training, but also cfg
>>>>>>>>>>>>>>>>>> the different video modes into DP registers.
>>>>>>>>>>>>>>>>> Why? The link training and mode programming should happen during
>>>>>>>>>>>>>>>>> pre_enable / enable stage (legacy or atomic).
>>>>>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Right, that's what I'm curious about. It won't call encoder enble/disable functions when I triggered HPD.
>>>>>>>>>>>>>>>> And I'm sure the drm_connector_helper_hpd_irq_event() is called. So I add a drm_client for it.I
>>>>>>>>>>>>>>> It should be userspace, who triggers the enable/disable (or it should
>>>>>>>>>>>>>>> be the in-kernel fbdev / fbcon, which interface through the generic
>>>>>>>>>>>>>>> drm_fbdev client).
>>>>>>>>>>>>>> Right, I knew it. When I insmode my driver firstly (or restart display service), it will call disable, modeset and enable,
>>>>>>>>>>>>>> by user, but it won't call when HPD triggered .
>>>>>>>>>>>>> - Is HPD even properly delivered to userspace? What kind of compsitor
>>>>>>>>>>>>> are you using? Is .detect working properly and reporting a correct
>>>>>>>>>>>>> plug-in state?
>>>>>>>>>>>> Thanks for your answering. I'm not very good understanding about userspace in framework. In my opinion, when I call
>>>>>>>>>>>> this drm_connector_helper_hpd_irq_event(), the HPD will deliver to userspace.
>>>>>>>>>>>> I use Xorg, and the display service is GDM.
>>>>>>>>>>>> The .detect is called and the getting modes info is correct.
>>>>>>>>>>>> I find that it would only trigger(disable, modeset and enable), when I changed resolutions, restart display service and insmod driver.
>>>>>>>>>>> You can go to the display settings in GDM. It would be interesting to
>>>>>>>>>>> observe if it notes the second monitor or not. Last, but not least, you
>>>>>>>>>>> can use a simple tool like 'xrandr' under your XOrg session to set the
>>>>>>>>>>> display resolution.
>>>>>>>>>> Thank you for your advice!
>>>>>>>>>> Right, there are DP and VGA two monitors. I tried to totally remove the vga connector in driver, the problem is gone.
>>>>>>>>>> So do I need to clear the vga connector, if dp is plugged in?
>>>>>>>>> Unless your hardware can not manage two outputs at the same time, no,
>>>>>>>>> you don't have to. Just check how it behaves on x86 systems. Ideally
>>>>>>>>> your driver should have the same behaviour.
>>>>>>>> Our hardware cannot support two outputs with different timing, so I used the one crtc and one plane that DP and VGA share. And just add a new DP connector
>>>>>>>> with a encoder, just like the previous VGA's code logic. But the HPD problem makes me feel confused, should I change the framwork structure to slove this problem?
>>>>>>> I think registering a single CRTC is a correct way. Then it is logical
>>>>>>> that there is no mode set on the DP when you connect it. The userspace
>>>>>>> can not output any data. However if you disconnect VGA and connect DP
>>>>>>> then it should become active and should output your desktop
>>>>>>> environment.
>>>>>> Okay, Thank you for your guidance. So I need to disconnect VGA when I get the HPD (plugged in) , then
>>>>>> userapce will active and enanble DP, right?
>>>>> Yes.
>>>> I'm sorry for that, just wanna make sure one more thing. I found if I only set the VGA connector's status to disconnected when HPD plugged in, it won't be active.
>>> Huh? You should implement hibmc_connector_funcs.detect() or
>>> .detect_ctx() to report VGA connector's status. Use
>>> ast_vga_connector_helper_detect_ctx() as an inspiration.
>> Okay, thanks for your correcting, I'll try to add drm_connector_helper_detect_from_ddc() in VGA's detect_ctx.
>> And also, I wanna make sure that do I need to unplug the VGA monitor manually when I plug in DP, or just
>> set the status of VGA connector to disconnected in interrupt handler.
> If the HPD reports the status of the DP connector, why do you want to
> change the status of the VGA connector?
I'm not sure how VGA is influenced the userspace active and enable, because you said "disconnect VGA and connect DP then it should become active", so I try to do it.
After just adding VGA's detect, it works good in Xorg session, but when I start GDM, it won't enable in HPD plugged in. It's kind of complicated to me, (different Xorg usages,
different results). So I wanna send the patches first, and then follow up on this issue whether the configuration of GDM is correct.
>>
>>>> And If I add drm_cleanup_connector() for VGA, it work. So is this okay that I use this cleanup in HPD?
>>> drm_connector_cleanup() should only be callsed if the connector is going
>>> to be destroyed. You should not be callign that function.
>>>
>>>>>>>> And also, I will check whether this driver works good on the x86 server. Right now, I'm testing on arm64 server.
>>>>>>>>
>>>>>>>>>> And also, I used xrandr to set modes after 'startx'. Changing resolutions works,
>>>>>>>>>> but there are errs when set some low resolutions.
>>>>>>>>> That's a separate topic, most likely related to timing or to some other
>>>>>>>>> issues. You can fix that separately (but please do, switching modes
>>>>>>>>> should work).
>>>>>>>> Okay!
>>>>>>>>
>>>>>>>>
>
>
> On Sat, Feb 22, 2025 at 10:51:00AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> Enable HPD feature and add its isr and event function. Add a drm client
>> dev and realized the hotplug callback in it.
> What for? There should be no need to add a separate drm client just for
> the hotplug.
>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>> - remove mdelay(100) hpd function in ISR, suggested by Dmitry Baryshkov.
>> - remove enble_display in ISR, suggested by Dmitry Baryshkov.
>> - change drm_kms_helper_connector_hotplug_event() to
>> drm_connector_helper_hpd_irq_event(), suggested by Dmitry Baryshkov.
>> - move macros to dp_reg.h, suggested by Dmitry Baryshkov.
>> - remove struct irqs, suggested by Dmitry Baryshkov.
>> - split this patch into two parts, suggested by Dmitry Baryshkov.
>> - add a drm client dev to handle HPD event.
>> v1 -> v2:
>> - optimizing the description in commit message, suggested by Dmitry Baryshkov.
>> - add mdelay(100) comments, suggested by Dmitry Baryshkov.
>> - deleting display enable in hpd event, suggested by Dmitry Baryshkov.
>> ---
>> .../gpu/drm/hisilicon/hibmc/dp/dp_config.h | 1 +
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c | 22 +++++++
>> drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h | 6 ++
>> .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 61 +++++++++++++++++++
>> .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +
>> 5 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> index c5feef8dc27d..08f9e1caf7fc 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_config.h
>> @@ -16,5 +16,6 @@
>> #define HIBMC_DP_SYNC_EN_MASK 0x3
>> #define HIBMC_DP_LINK_RATE_CAL 27
>> #define HIBMC_DP_SYNC_DELAY(lanes) ((lanes) == 0x2 ? 86 : 46)
>> +#define HIBMC_DP_INT_ENABLE 0xc
>>
>> #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index a921b98dbf50..b2116395b8dd 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -182,6 +182,7 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>> /* int init */
>> writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> + writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> /* rst */
>> writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> /* clock enable */
>> @@ -190,6 +191,21 @@ int hibmc_dp_hw_init(struct hibmc_dp *dp)
>> return 0;
>> }
>>
>> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp)
>> +{
>> + struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> +
>> + hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_SYNC_LEN_SEL, 0x0);
>> + hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_TIMER_TIMEOUT, 0x1);
>> + hibmc_dp_reg_write_field(dp->dp_dev, HIBMC_DP_AUX_REQ, HIBMC_DP_CFG_AUX_MIN_PULSE_NUM, 0x9);
>> + writel(HIBMC_DP_HDCP, dp_dev->base + HIBMC_DP_HDCP_CFG);
>> + writel(0, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> + writel(HIBMC_DP_INT_RST, dp_dev->base + HIBMC_DP_INTR_ORIGINAL_STATUS);
>> + writel(HIBMC_DP_INT_ENABLE, dp_dev->base + HIBMC_DP_INTR_ENABLE);
>> + writel(HIBMC_DP_DPTX_RST, dp_dev->base + HIBMC_DP_DPTX_RST_CTRL);
>> + writel(HIBMC_DP_CLK_EN, dp_dev->base + HIBMC_DP_DPTX_CLK_CTRL);
>> +}
>> +
>> void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable)
>> {
>> struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> @@ -228,6 +244,12 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>> return 0;
>> }
>>
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp)
>> +{
>> + dp->dp_dev->link.status.clock_recovered = false;
>> + dp->dp_dev->link.status.channel_equalized = false;
> Could you please point out, where the link rate and number of lanes are
> reset?
Okay, I'll add comments here. It's mostly used when the connector is out.
>> +}
>> +
>> static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>> {CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>> {CBAR_WHITE, 0xfff, 0xfff, 0xfff},
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 83a53dae8012..a55d66d53966 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -11,6 +11,7 @@
>> #include <drm/drm_connector.h>
>> #include <drm/drm_print.h>
>> #include <drm/display/drm_dp_helper.h>
>> +#include <drm/drm_client.h>
>>
>> struct hibmc_dp_dev;
>>
>> @@ -49,11 +50,16 @@ struct hibmc_dp {
>> void __iomem *mmio;
>> struct drm_dp_aux aux;
>> struct hibmc_dp_cbar_cfg cfg;
>> + u32 irq_status;
>> + u32 hpd_status;
>> + struct drm_client_dev client;
>> };
>>
>> int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>> void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>> void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>> +void hibmc_dp_reset_link(struct hibmc_dp *dp);
>> +void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>>
>> #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index a7f611e82f73..40a3ebb8ac4b 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -9,10 +9,13 @@
>> #include <drm/drm_modes.h>
>> #include <drm/drm_drv.h>
>> #include <drm/drm_edid.h>
>> +#include <drm/drm_client.h>
>>
>> #include "hibmc_drm_drv.h"
>> #include "dp/dp_hw.h"
>>
>> +#define DP_MASKED_SINK_HPD_PLUG_INT BIT(2)
>> +
>> static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>> {
>> struct hibmc_dp *dp = to_hibmc_dp(connector);
>> @@ -98,6 +101,58 @@ static const struct drm_encoder_helper_funcs hibmc_dp_encoder_helper_funcs = {
>> .atomic_disable = hibmc_dp_encoder_disable,
>> };
>>
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg)
>> +{
>> + struct drm_device *dev = (struct drm_device *)arg;
>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> + int idx;
>> +
>> + if (!drm_dev_enter(dev, &idx))
>> + return -ENODEV;
>> +
>> + if (priv->dp.irq_status & DP_MASKED_SINK_HPD_PLUG_INT) {
>> + drm_dbg_dp(&priv->dev, "HPD IN isr occur!\n");
>> + priv->dp.hpd_status = 1;
>> + } else {
>> + drm_dbg_dp(&priv->dev, "HPD OUT isr occur!\n");
>> + priv->dp.hpd_status = 0;
>> + }
>> +
>> + if (dev->registered)
>> + drm_connector_helper_hpd_irq_event(&priv->dp.connector);
>> +
>> + drm_dev_exit(idx);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int hibmc_dp_hpd_event(struct drm_client_dev *client)
>> +{
>> + struct hibmc_dp *dp = container_of(client, struct hibmc_dp, client);
>> + struct hibmc_drm_private *priv = to_hibmc_drm_private(dp->drm_dev);
>> + struct drm_display_mode *mode = &priv->crtc.state->adjusted_mode;
>> + int ret;
>> +
>> + if (dp->hpd_status) {
>> + hibmc_dp_hpd_cfg(&priv->dp);
>> + ret = hibmc_dp_prepare(dp, mode);
>> + if (ret)
>> + return ret;
>> +
>> + hibmc_dp_display_en(dp, true);
>> + } else {
>> + hibmc_dp_display_en(dp, false);
>> + hibmc_dp_reset_link(&priv->dp);
>> + }
> If I understand this correctly, you are using a separate drm_client to
> enable and disable the link & display. Why is it necessary? Existing
> drm_clients and userspace compositors use drm framework, they should be
> able to turn the display on and off as required.
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct drm_client_funcs hibmc_dp_client_funcs = {
>> + .hotplug = hibmc_dp_hpd_event,
>> + .unregister = drm_client_release,
>> +};
>> +
>> int hibmc_dp_init(struct hibmc_drm_private *priv)
>> {
>> struct drm_device *dev = &priv->dev;
>> @@ -138,5 +193,11 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
>>
>> drm_connector_attach_encoder(connector, encoder);
>>
>> + ret = drm_client_init(dev, &dp->client, "hibmc-DP-HPD", &hibmc_dp_client_funcs);
>> + if (ret)
>> + return ret;
>> +
>> + drm_client_register(&dp->client);
>> +
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index bc89e4b9f4e3..daed1330b961 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -71,4 +71,6 @@ int hibmc_dp_init(struct hibmc_drm_private *priv);
>>
>> void hibmc_debugfs_init(struct drm_connector *connector, struct dentry *root);
>>
>> +irqreturn_t hibmc_dp_hpd_isr(int irq, void *arg);
>> +
>> #endif
>> --
>> 2.33.0
>>
© 2016 - 2026 Red Hat, Inc.