Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected
From: Thomas Zimmermann
Date: Mon Apr 27 2026 - 03:22:38 EST
Hi,
sorry for not getting to the review earlier. See my comments below.
Am 23.04.26 um 08:32 schrieb Yongbang Shi:
[...]
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 31316fe1ea8d..dfaeabd05d46 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -55,6 +55,7 @@ struct hibmc_dp {
struct drm_dp_aux aux;
struct hibmc_dp_cbar_cfg cfg;
u32 irq_status;
+ int phys_state;
I think the correct term is 'status' instead of 'state'.
};
int hibmc_dp_hw_init(struct hibmc_dp *dp);
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 35dff7bfbf76..8fe2eb51a0b3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -61,27 +61,38 @@ static int hibmc_dp_detect(struct drm_connector *connector,
{
struct hibmc_dp *dp = to_hibmc_dp(connector);
struct hibmc_dp_dev *dp_dev = dp->dp_dev;
- int ret;
+ int ret = connector_status_disconnected;
if (dp->irq_status) {
- if (dp_dev->hpd_status != HIBMC_HPD_IN)
- return connector_status_disconnected;
+ if (dp_dev->hpd_status != HIBMC_HPD_IN) {
+ ret = connector_status_disconnected;
+ goto exit;
+ }
}
- if (!hibmc_dp_get_dpcd(dp_dev))
- return connector_status_disconnected;
+ if (!hibmc_dp_get_dpcd(dp_dev)) {
+ ret = connector_status_disconnected;
+ goto exit;
+ }
- if (!dp_dev->is_branch)
- return connector_status_connected;
+ if (!dp_dev->is_branch) {
+ ret = connector_status_connected;
+ goto exit;
+ }
if (drm_dp_read_sink_count_cap(connector, dp_dev->dpcd, &dp_dev->desc) &&
dp_dev->downstream_ports[0] & DP_DS_PORT_HPD) {
ret = drm_dp_read_sink_count(dp_dev->aux);
- if (ret > 0)
- return connector_status_connected;
+ if (ret > 0) {
+ ret = connector_status_connected;
+ goto exit;
+ }
}
- return connector_status_disconnected;
+exit:
+ dp->phys_state = ret;
+
+ return ret;
}
static int hibmc_dp_mode_valid(struct drm_connector *connector,
@@ -243,5 +254,7 @@ int hibmc_dp_init(struct hibmc_drm_private *priv)
connector->polled = DRM_CONNECTOR_POLL_HPD;
+ dp->phys_state = connector_status_disconnected;
+
return 0;
}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 289304500ab0..d409efc34215 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -24,6 +24,7 @@
#include <drm/drm_managed.h>
#include <drm/drm_module.h>
#include <drm/drm_vblank.h>
+#include <drm/drm_probe_helper.h>
#include "hibmc_drm_drv.h"
#include "hibmc_drm_regs.h"
@@ -162,6 +163,8 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
drm_for_each_encoder(encoder, dev)
encoder->possible_clones = clone_mask;
+ drm_kms_helper_poll_init(dev);
So I think this is too early. This function will periodically poll connector, but they may have not been set up fully. Rather call this function after drm_mode_config_reset() in hibmc_load().
+
return 0;
}
@@ -279,6 +282,7 @@ static int hibmc_hw_init(struct hibmc_drm_private *priv)
static void hibmc_unload(struct drm_device *dev)
{
+ drm_kms_helper_poll_fini(dev);
Init with drmm_kms_helper_poll_init() [1] and avoid this clean-up call entirely. In DRM we generally prefer managed init/cleanup over manual one.
[1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L963
drm_atomic_helper_shutdown(dev);
}
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index ca8502e2760c..6abb49b5107b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -31,6 +31,7 @@ struct hibmc_vdac {
struct drm_connector connector;
struct i2c_adapter adapter;
struct i2c_algo_bit_data bit_data;
+ int phys_state;
'phys_status' would again be more appropriate.
};
struct hibmc_drm_private {
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 841e81f47b68..dc40018dbd21 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -25,27 +25,20 @@
static int hibmc_connector_get_modes(struct drm_connector *connector)
{
struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
- const struct drm_edid *drm_edid;
- int count;
+ int count = 0;
- drm_edid = drm_edid_read_ddc(connector, &vdac->adapter);
+ if (vdac->phys_state == connector_status_connected)
+ count = drm_connector_helper_get_modes(connector);
- drm_edid_connector_update(connector, drm_edid);
-
- if (drm_edid) {
- count = drm_edid_connector_add_modes(connector);
- if (count)
- goto out;
- }
+ if (count > 0)
+ return count;
There's a problem with the overall logic here: if the physical status is 'connected' and the helper could not retrieve any modes, the helper should return 0 here. Then the DRM helpers do the right thing with setting up a few modes or an EDID override as a fallback. See [2]. For example, on my broken test system, I'd be able to provide my display's EDID and get the correct output.
[2] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L436
Any code below is BMC setup and should run in an else branch, just like in ast.
+ drm_edid_connector_update(connector, NULL);
count = drm_add_modes_noedid(connector,
connector->dev->mode_config.max_width,
connector->dev->mode_config.max_height);
drm_set_preferred_mode(connector, 1024, 768);
-out:
- drm_edid_free(drm_edid);
-
return count;
}
@@ -57,10 +50,32 @@ static void hibmc_connector_destroy(struct drm_connector *connector)
drm_connector_cleanup(connector);
}
+static int hibmc_vdac_detect(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx,
+ bool force)
+{
+ struct hibmc_drm_private *priv = to_hibmc_drm_private(connector->dev);
+ int state = drm_connector_helper_detect_from_ddc(connector, ctx,
+ force);
'state' -> 'status'
+ struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
+
+ if (priv->dp.phys_state == connector_status_connected)
+ return vdac->phys_state = state;
Please only one statement per line. First assign, then return.
+
+ if (state != vdac->phys_state)
+ ++connector->epoch_counter;
+ vdac->phys_state = state;
+
+ /* If both the DP and VDAC physical states are disconnected,
+ * the "connected" status is returned to support KVM display.
+ */
+ return connector_status_connected;
I haven't tried, but I think this should also resolve the problems on my test systems. Great, thanks a lot! I might just get default resolutions for now, but that's OK.
Best regards
Thomas
+}
+
static const struct drm_connector_helper_funcs
hibmc_connector_helper_funcs = {
.get_modes = hibmc_connector_get_modes,
- .detect_ctx = drm_connector_helper_detect_from_ddc,
+ .detect_ctx = hibmc_vdac_detect,
};
static const struct drm_connector_funcs hibmc_connector_funcs = {
@@ -130,6 +145,8 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+ vdac->phys_state = connector_status_connected;
+
return 0;
err:
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)