[PATCH for drm-misc-fixes v5 0/4] Fix some bugs in the hibmc driver

[PATCH for drm-misc-fixes v5 0/4] Fix some bugs in the hibmc driver

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

There are some bugfix for hibmc-drm driver.

---
ChangeLog:
v4 -> v5:
  - The 'epoch_counter' of the connector is incremented when the physical
    status changes.
  - The polling mechanism for the KMS helper is enabled.
v3 -> v4:
  - fix incorrect cover-letter subject prefix in v3.
v2 -> v3:
  - remove unused macro CLOCK_TOLERANCE.
v1 -> v2:
  - fix the checkpatch.pl warning "Unknown commit ID 'e6c7c59da494'".
  - fix the checkpatch.pl warning "line length of 83/85 exceeds 80 columns".
  - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
    be given in public.
  - add 'drm-misc-fixes' in subject prefix.
---

Lin He (4):
  drm/hisilicon/hibmc: add updating link cap in DP detect()
  drm/hisilicon/hibmc: fix no showing when no connectors connected
  drm/hisilicon/hibmc: move display contrl config to hibmc_probe()
  drm/hisilicon/hibmc: use clock to look up the PLL value

 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h  |  1 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  1 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c  |  2 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 78 +++++++++++--------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 35 ++++++---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 18 +++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 67 ++++++++--------
 8 files changed, 122 insertions(+), 81 deletions(-)

-- 
2.33.0

[PATCH for drm-misc-fixes v5 1/4] drm/hisilicon/hibmc: add updating link cap in DP detect()

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

In the past, the link cap is updated in link training at encoder enable
stage, but the hibmc_dp_mode_valid() is called before it, which will use
DP link's rate and lanes. So add the hibmc_dp_update_caps() in
hibmc_dp_update_caps() to avoid some potential risks.

Fixes: 607805abfb74 ("drm/hisilicon/hibmc: add dp mode valid check")
Signed-off-by: Lin He <helin52@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v1 -> v2:
  - fix the checkpatch.pl warning "Unknown commit ID 'e6c7c59da494'".
  - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
    be given in public.
  - add 'drm-misc-fixes' in subject prefix.
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h   | 1 +
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c   | 2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c | 2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
index f9ee7ebfec55..f53dac256ee0 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_comm.h
@@ -69,5 +69,6 @@ int hibmc_dp_link_training(struct hibmc_dp_dev *dp);
 int hibmc_dp_serdes_init(struct hibmc_dp_dev *dp);
 int hibmc_dp_serdes_rate_switch(u8 rate, struct hibmc_dp_dev *dp);
 int hibmc_dp_serdes_set_tx_cfg(struct hibmc_dp_dev *dp, u8 train_set[HIBMC_DP_LANE_NUM_MAX]);
+void hibmc_dp_update_caps(struct hibmc_dp_dev *dp);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
index 0726cb5b736e..8c53f16db516 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_link.c
@@ -325,7 +325,7 @@ static int hibmc_dp_link_downgrade_training_eq(struct hibmc_dp_dev *dp)
 	return hibmc_dp_link_reduce_rate(dp);
 }
 
-static void hibmc_dp_update_caps(struct hibmc_dp_dev *dp)
+void hibmc_dp_update_caps(struct hibmc_dp_dev *dp)
 {
 	dp->link.cap.link_rate = dp->dpcd[DP_MAX_LINK_RATE];
 	if (dp->link.cap.link_rate > DP_LINK_BW_8_1 || !dp->link.cap.link_rate)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 616821e3c933..35dff7bfbf76 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -41,6 +41,8 @@ static bool hibmc_dp_get_dpcd(struct hibmc_dp_dev *dp_dev)
 	if (ret)
 		return false;
 
+	hibmc_dp_update_caps(dp_dev);
+
 	dp_dev->is_branch = drm_dp_is_branch(dp_dev->dpcd);
 
 	ret = drm_dp_read_desc(dp_dev->aux, &dp_dev->desc, dp_dev->is_branch);
-- 
2.33.0

[PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

Our chip support KVM over IP feature, so hibmc driver need to support
displaying without any connectors plugged in. If no connectors are
connected, the vdac connector status should be set to 'connected' to
ensure proper KVM display functionality. Additionally, for
previous-generation products that may lack hardware link support and
thus cannot detect the monitor, the same approach should be applied
to ensure VGA display functionality.

* Add phys_state in the struct of dp and vdac to check physical outputs.

* The 'epoch_counter' of the vdac connector is incremented when the
physical status changes.

For get_modes: using BMC modes for connector if no display is attached to
phys VGA cable, otherwise use EDID modes by drm_connector_helper_get_modes,
because KVM doesn't provide EDID reads.

The polling mechanism for the KMS helper is enabled.

Fixes: 4c962bc929f1 ("drm/hisilicon/hibmc: Add vga connector detect functions")
Reported-by: Thomas Zimmermann <tzimmermann@suse.de>
Closes: https://lore.kernel.org/all/0eb5c509-2724-4c57-87ad-74e4270d5a5a@suse.de/
Signed-off-by: Lin He <helin52@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v4 -> v5:
  - The 'epoch_counter' of the vdac connector is incremented when the physical
    status changes.
  - The polling mechanism for the KMS helper is enabled.
v1 -> v2:
  - fix the checkpatch.pl warning "line length of 83/85 exceeds 80 columns".
  - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
    be given in public.
  - add 'drm-misc-fixes' in subject prefix.
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 33 +++++++++-----
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  4 ++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  1 +
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 45 +++++++++++++------
 5 files changed, 60 insertions(+), 24 deletions(-)

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;
 };
 
 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);
+
 	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);
 	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;
 };
 
 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;
 
+	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);
+	struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
+
+	if (priv->dp.phys_state == connector_status_connected)
+		return vdac->phys_state = state;
+
+	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;
+}
+
 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:
-- 
2.33.0

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Thomas Zimmermann]

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)

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Yongbang Shi]

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

Okay. The `status` is a better fit, and it corresponds to `drm_connector_status`.

>>   };
>>     int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index 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().
> 

You're right; calling `drm_kms_helper_poll_init` immediately starts the timer, so this call should be placed after
`drm_mode_config_reset`.

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

Okay.

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

Okay.

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

The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from firmware:
  * Debugfs: Requires the user to manually perform file operations to configure it;
  * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
              `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
              parameters;

The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
methods require cooperation from the user or the distribution's operating system, making implementation relatively
difficult.

Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
would be a more universal solution. Similarly, on your test system, this approach would ensure basic display functionality.

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

Okay.

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

Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.


Thanks,
Yongbang.

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

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Thomas Zimmermann]

Hi

Am 28.04.26 um 05:58 schrieb Yongbang Shi:
[...]
>> 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.
>>
> The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from firmware:
>    * Debugfs: Requires the user to manually perform file operations to configure it;
>    * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
>                `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
>                parameters;
>
> The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
> methods require cooperation from the user or the distribution's operating system, making implementation relatively
> difficult.

Yes, it's the established way for users to override the EDID.

>
> Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
> connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
> would be a more universal solution. Similarly, on your test system, this approach would ensure basic display functionality.

If no modes could be detected on the VGA and no EDID has been provided 
by the user, DRM helpers will install a set of default modes that are 
suitable for graphics cards. See [1]. The modes installed for the KVM 
are not all compatible with VGA.  If you rely on DRM helpers, you'll 
always get correct modes for VGA and KVM.

Generally speaking, the decision of VGA-vs-KVM should be in ->detect. 
Having phys_state/phys_status does this very well. The ->get_modes 
function should then use whatever has been detected.

[1] 
https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L653

Best regards
Thomas

>
>>>    +    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'
>>
> Okay.
>
>>> +    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.
>>
> Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.
>
>
> Thanks,
> Yongbang.
>
>>> +
>>> +    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)

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Yongbang Shi]

> Hi
> 
> Am 28.04.26 um 05:58 schrieb Yongbang Shi:
> [...]
>>> 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.
>>>
>> The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from firmware:
>>    * Debugfs: Requires the user to manually perform file operations to configure it;
>>    * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
>>                `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
>>                parameters;
>>
>> The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
>> methods require cooperation from the user or the distribution's operating system, making implementation relatively
>> difficult.
> 
> Yes, it's the established way for users to override the EDID.
> 
>>
>> Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
>> connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
>> would be a more universal solution. Similarly, on your test system, this approach would ensure basic display
>> functionality.
> 
> If no modes could be detected on the VGA and no EDID has been provided by the user, DRM helpers will install a set of
> default modes that are suitable for graphics cards. See [1]. The modes installed for the KVM are not all compatible with
> VGA.  If you rely on DRM helpers, you'll always get correct modes for VGA and KVM.
> 
> Generally speaking, the decision of VGA-vs-KVM should be in ->detect. Having phys_state/phys_status does this very well.
> The ->get_modes function should then use whatever has been detected.
> 
> [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L653
> 

Yes, I understand what you mean. But there's another issue here; we've also tested the scenario where we directly return
0. However, we found that only three resolutions remain supported: 1024*768, 800*600 and 640*480. This is because the
resolution added via `drm_add_modes_noedid` in the helper has a maximum of 1024*768. This doesn't look good—there are
too few resolutions available for users to choose from in KVM.

With the current implementation, when `count == 0`, we set the resolution to the highest resolution supported by our
CRTC, allowing users more options.

Of course, this implementation has its drawbacks; it doesn't allow users to provide a custom resolution list via debugfs
using the override method when `count == 0`. But this method isn't very important for how our product is used.

Thanks,
Yongbang.

> Best regards
> Thomas
> 
>>
>>>>    +    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'
>>>
>> Okay.
>>
>>>> +    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.
>>>
>> Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.
>>
>>
>> Thanks,
>> Yongbang.
>>
>>>> +
>>>> +    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:
> 

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Thomas Zimmermann]

Hi,

sorry for the late reply. I'm fairly busy.

Am 28.04.26 um 14:53 schrieb Yongbang Shi:
>> Hi
>>
>> Am 28.04.26 um 05:58 schrieb Yongbang Shi:
>> [...]
>>>> 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.
>>>>
>>> The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from firmware:
>>>     * Debugfs: Requires the user to manually perform file operations to configure it;
>>>     * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
>>>                 `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
>>>                 parameters;
>>>
>>> The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
>>> methods require cooperation from the user or the distribution's operating system, making implementation relatively
>>> difficult.
>> Yes, it's the established way for users to override the EDID.
>>
>>> Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
>>> connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
>>> would be a more universal solution. Similarly, on your test system, this approach would ensure basic display
>>> functionality.
>> If no modes could be detected on the VGA and no EDID has been provided by the user, DRM helpers will install a set of
>> default modes that are suitable for graphics cards. See [1]. The modes installed for the KVM are not all compatible with
>> VGA.  If you rely on DRM helpers, you'll always get correct modes for VGA and KVM.
>>
>> Generally speaking, the decision of VGA-vs-KVM should be in ->detect. Having phys_state/phys_status does this very well.
>> The ->get_modes function should then use whatever has been detected.
>>
>> [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L653
>>
> Yes, I understand what you mean. But there's another issue here; we've also tested the scenario where we directly return
> 0. However, we found that only three resolutions remain supported: 1024*768, 800*600 and 640*480. This is because the
> resolution added via `drm_add_modes_noedid` in the helper has a maximum of 1024*768. This doesn't look good—there are
> too few resolutions available for users to choose from in KVM.
>
> With the current implementation, when `count == 0`, we set the resolution to the highest resolution supported by our
> CRTC, allowing users more options.

But why do you second-guess the results from ->detect?  Th call to 
->detect already established that there's a display connected.  If 
->get_modes cannot find useful modes, then don't treat it as a BMC now. 
Return 0 and let DRM choose some same defaults.  These 3 low-res modes 
act as a safe base line. Reporting high-res BMC modes might accidentally 
fry someone's monitor.

>
> Of course, this implementation has its drawbacks; it doesn't allow users to provide a custom resolution list via debugfs
> using the override method when `count == 0`. But this method isn't very important for how our product is used.

The upstream Linux kernel is not a product, but a shared commodity. It 
needs to work in everyone's best interest.

Best regards
Thomas

>
> Thanks,
> Yongbang.
>
>> Best regards
>> Thomas
>>
>>>>>     +    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'
>>>>
>>> Okay.
>>>
>>>>> +    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.
>>>>
>>> Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.
>>>
>>>
>>> Thanks,
>>> Yongbang.
>>>
>>>>> +
>>>>> +    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)

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Yongbang Shi]

> Hi,
> 
> sorry for the late reply. I'm fairly busy.
> 

Thank you so much for taking the time to review our code. No worries at all—we'd really appreciate it if you could take
a look whenever you have time.

> Am 28.04.26 um 14:53 schrieb Yongbang Shi:
>>> Hi
>>>
>>> Am 28.04.26 um 05:58 schrieb Yongbang Shi:
>>> [...]
>>>>> 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.
>>>>>
>>>> The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from
>>>> firmware:
>>>>     * Debugfs: Requires the user to manually perform file operations to configure it;
>>>>     * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
>>>>                 `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
>>>>                 parameters;
>>>>
>>>> The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
>>>> methods require cooperation from the user or the distribution's operating system, making implementation relatively
>>>> difficult.
>>> Yes, it's the established way for users to override the EDID.
>>>
>>>> Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
>>>> connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
>>>> would be a more universal solution. Similarly, on your test system, this approach would ensure basic display
>>>> functionality.
>>> If no modes could be detected on the VGA and no EDID has been provided by the user, DRM helpers will install a set of
>>> default modes that are suitable for graphics cards. See [1]. The modes installed for the KVM are not all compatible with
>>> VGA.  If you rely on DRM helpers, you'll always get correct modes for VGA and KVM.
>>>
>>> Generally speaking, the decision of VGA-vs-KVM should be in ->detect. Having phys_state/phys_status does this very well.
>>> The ->get_modes function should then use whatever has been detected.
>>>
>>> [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L653
>>>
>> Yes, I understand what you mean. But there's another issue here; we've also tested the scenario where we directly return
>> 0. However, we found that only three resolutions remain supported: 1024*768, 800*600 and 640*480. This is because the
>> resolution added via `drm_add_modes_noedid` in the helper has a maximum of 1024*768. This doesn't look good—there are
>> too few resolutions available for users to choose from in KVM.
>>
>> With the current implementation, when `count == 0`, we set the resolution to the highest resolution supported by our
>> CRTC, allowing users more options.
> 
> But why do you second-guess the results from ->detect?  Th call to ->detect already established that there's a display
> connected.  If ->get_modes cannot find useful modes, then don't treat it as a BMC now. Return 0 and let DRM choose some
> same defaults.  These 3 low-res modes act as a safe base line. Reporting high-res BMC modes might accidentally fry
> someone's monitor.
> 

Sorry, my mistake. My previous reply was not quite right. I'm a little confused about this situation. I focused on
optimizing the KVM display when no monitor is connected, but when no monitor is connected, `vdac->phys_status` is set to
`disconnected`. Following the AST implementation you recommended[1], it takes the `else` branch and sets a custom list.

`vdac->phys_state == connected` and `drm_connector_helper_get_modes` returns 0, in this case, either the returned EDID
does not match any available options, or there is an issue with the display. In such situations, it's good to let DRM
choose some defaults.

I will follow the AST implementation[1] to make the modification in v6.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/ast/ast_vga.c?h=v7.1-rc2#L31

>>
>> Of course, this implementation has its drawbacks; it doesn't allow users to provide a custom resolution list via debugfs
>> using the override method when `count == 0`. But this method isn't very important for how our product is used.
> 
> The upstream Linux kernel is not a product, but a shared commodity. It needs to work in everyone's best interest.
> 

Yes, we'll keep that in mind; when developing community code, we should consider the overall architecture.

Thanks,
Yongbang.

> Best regards
> Thomas
> 
>>
>> Thanks,
>> Yongbang.
>>
>>> Best regards
>>> Thomas
>>>
>>>>>>     +    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'
>>>>>
>>>> Okay.
>>>>
>>>>>> +    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.
>>>>>
>>>> Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.
>>>>
>>>>
>>>> Thanks,
>>>> Yongbang.
>>>>
>>>>>> +
>>>>>> +    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:
> 

Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Thomas Zimmermann]

Hi

Am 06.05.26 um 10:56 schrieb Yongbang Shi:
>> Hi,
>>
>> sorry for the late reply. I'm fairly busy.
>>
> Thank you so much for taking the time to review our code. No worries at all—we'd really appreciate it if you could take
> a look whenever you have time.
>
>> Am 28.04.26 um 14:53 schrieb Yongbang Shi:
>>>> Hi
>>>>
>>>> Am 28.04.26 um 05:58 schrieb Yongbang Shi:
>>>> [...]
>>>>>> 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.
>>>>>>
>>>>> The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from
>>>>> firmware:
>>>>>      * Debugfs: Requires the user to manually perform file operations to configure it;
>>>>>      * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
>>>>>                  `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
>>>>>                  parameters;
>>>>>
>>>>> The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
>>>>> methods require cooperation from the user or the distribution's operating system, making implementation relatively
>>>>> difficult.
>>>> Yes, it's the established way for users to override the EDID.
>>>>
>>>>> Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
>>>>> connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
>>>>> would be a more universal solution. Similarly, on your test system, this approach would ensure basic display
>>>>> functionality.
>>>> If no modes could be detected on the VGA and no EDID has been provided by the user, DRM helpers will install a set of
>>>> default modes that are suitable for graphics cards. See [1]. The modes installed for the KVM are not all compatible with
>>>> VGA.  If you rely on DRM helpers, you'll always get correct modes for VGA and KVM.
>>>>
>>>> Generally speaking, the decision of VGA-vs-KVM should be in ->detect. Having phys_state/phys_status does this very well.
>>>> The ->get_modes function should then use whatever has been detected.
>>>>
>>>> [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L653
>>>>
>>> Yes, I understand what you mean. But there's another issue here; we've also tested the scenario where we directly return
>>> 0. However, we found that only three resolutions remain supported: 1024*768, 800*600 and 640*480. This is because the
>>> resolution added via `drm_add_modes_noedid` in the helper has a maximum of 1024*768. This doesn't look good—there are
>>> too few resolutions available for users to choose from in KVM.
>>>
>>> With the current implementation, when `count == 0`, we set the resolution to the highest resolution supported by our
>>> CRTC, allowing users more options.
>> But why do you second-guess the results from ->detect?  Th call to ->detect already established that there's a display
>> connected.  If ->get_modes cannot find useful modes, then don't treat it as a BMC now. Return 0 and let DRM choose some
>> same defaults.  These 3 low-res modes act as a safe base line. Reporting high-res BMC modes might accidentally fry
>> someone's monitor.
>>
> Sorry, my mistake. My previous reply was not quite right. I'm a little confused about this situation. I focused on
> optimizing the KVM display when no monitor is connected, but when no monitor is connected, `vdac->phys_status` is set to
> `disconnected`. Following the AST implementation you recommended[1], it takes the `else` branch and sets a custom list.
>
> `vdac->phys_state == connected` and `drm_connector_helper_get_modes` returns 0, in this case, either the returned EDID
> does not match any available options, or there is an issue with the display. In such situations, it's good to let DRM
> choose some defaults.
>
> I will follow the AST implementation[1] to make the modification in v6.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/ast/ast_vga.c?h=v7.1-rc2#L31
>
>>> Of course, this implementation has its drawbacks; it doesn't allow users to provide a custom resolution list via debugfs
>>> using the override method when `count == 0`. But this method isn't very important for how our product is used.
>> The upstream Linux kernel is not a product, but a shared commodity. It needs to work in everyone's best interest.
>>
> Yes, we'll keep that in mind; when developing community code, we should consider the overall architecture.
>
> Thanks,
> Yongbang.

Sounds good. Looking forward to the next revision.

Best regards
Thomas

>
>> Best regards
>> Thomas
>>
>>> Thanks,
>>> Yongbang.
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>>>      +    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'
>>>>>>
>>>>> Okay.
>>>>>
>>>>>>> +    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.
>>>>>>
>>>>> Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Yongbang.
>>>>>
>>>>>>> +
>>>>>>> +    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)

[PATCH for drm-misc-fixes v5 3/4] drm/hisilicon/hibmc: move display contrl config to hibmc_probe()

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

If there's no VGA output, this encoder modeset won't be called, which
will cause displaying data from GPU being cut off. It's actually a
common display config for DP and VGA, so move the vdac encoder modeset
to driver load stage.

Fixes: 5294967f4ae4 ("drm/hisilicon/hibmc: Add support for VDAC")
Signed-off-by: Lin He <helin52@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v1 -> v2:
  - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
    be given in public.
  - add 'drm-misc-fixes' in subject prefix.
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 14 ++++++++++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 22 -------------------
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index d409efc34215..f8ab471f92e7 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -217,6 +217,18 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv, unsigned int gate)
 	writel(gate, mmio + gate_reg);
 }
 
+static void hibmc_display_ctrl(struct hibmc_drm_private *priv)
+{
+	u32 reg;
+
+	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
+	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
+	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
+	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
+	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
+	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
+}
+
 static void hibmc_hw_config(struct hibmc_drm_private *priv)
 {
 	u32 reg;
@@ -248,6 +260,8 @@ static void hibmc_hw_config(struct hibmc_drm_private *priv)
 	reg |= HIBMC_MSCCTL_LOCALMEM_RESET(1);
 
 	writel(reg, priv->mmio + HIBMC_MISC_CTRL);
+
+	hibmc_display_ctrl(priv);
 }
 
 static int hibmc_hw_map(struct hibmc_drm_private *priv)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index dc40018dbd21..9d924740c110 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -86,26 +86,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
-				   struct drm_display_mode *mode,
-				   struct drm_display_mode *adj_mode)
-{
-	u32 reg;
-	struct drm_device *dev = encoder->dev;
-	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
-
-	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
-	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
-	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
-	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
-	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
-	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
-}
-
-static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
-	.mode_set = hibmc_encoder_mode_set,
-};
-
 int hibmc_vdac_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = &priv->dev;
@@ -128,8 +108,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
 		goto err;
 	}
 
-	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
-
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &hibmc_connector_funcs,
 					  DRM_MODE_CONNECTOR_VGA,
-- 
2.33.0

Re: [PATCH for drm-misc-fixes v5 3/4] drm/hisilicon/hibmc: move display contrl config to hibmc_probe()

[Thomas Zimmermann]

Hi

Am 23.04.26 um 08:32 schrieb Yongbang Shi:
> From: Lin He <helin52@huawei.com>
>
> If there's no VGA output, this encoder modeset won't be called, which
> will cause displaying data from GPU being cut off. It's actually a
> common display config for DP and VGA, so move the vdac encoder modeset
> to driver load stage.
>
> Fixes: 5294967f4ae4 ("drm/hisilicon/hibmc: Add support for VDAC")
> Signed-off-by: Lin He <helin52@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v1 -> v2:
>    - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
>      be given in public.
>    - add 'drm-misc-fixes' in subject prefix.
> ---
>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 14 ++++++++++++
>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 22 -------------------
>   2 files changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index d409efc34215..f8ab471f92e7 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -217,6 +217,18 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv, unsigned int gate)
>   	writel(gate, mmio + gate_reg);
>   }
>   
> +static void hibmc_display_ctrl(struct hibmc_drm_private *priv)
> +{
> +	u32 reg;
> +
> +	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> +	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);

That might be stupid question: IDK what these bits mean, but the FPVDDEN 
and FPEN sound like they enable something. If you do this 
unconditionally in HW init, does it prevent power saving?  And is the 
system goes through suspend/resume,  does it need to be re-enabled?

My gut feeling would be to put this code into the CRTC.

Best regards
Thomas

> +	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> +}
> +
>   static void hibmc_hw_config(struct hibmc_drm_private *priv)
>   {
>   	u32 reg;
> @@ -248,6 +260,8 @@ static void hibmc_hw_config(struct hibmc_drm_private *priv)
>   	reg |= HIBMC_MSCCTL_LOCALMEM_RESET(1);
>   
>   	writel(reg, priv->mmio + HIBMC_MISC_CTRL);
> +
> +	hibmc_display_ctrl(priv);
>   }
>   
>   static int hibmc_hw_map(struct hibmc_drm_private *priv)
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index dc40018dbd21..9d924740c110 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -86,26 +86,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>   };
>   
> -static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
> -				   struct drm_display_mode *mode,
> -				   struct drm_display_mode *adj_mode)
> -{
> -	u32 reg;
> -	struct drm_device *dev = encoder->dev;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> -
> -	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> -	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
> -	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
> -	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
> -	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
> -	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> -}
> -
> -static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
> -	.mode_set = hibmc_encoder_mode_set,
> -};
> -
>   int hibmc_vdac_init(struct hibmc_drm_private *priv)
>   {
>   	struct drm_device *dev = &priv->dev;
> @@ -128,8 +108,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>   		goto err;
>   	}
>   
> -	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
> -
>   	ret = drm_connector_init_with_ddc(dev, connector,
>   					  &hibmc_connector_funcs,
>   					  DRM_MODE_CONNECTOR_VGA,

-- 
--
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)

Re: [PATCH for drm-misc-fixes v5 3/4] drm/hisilicon/hibmc: move display contrl config to hibmc_probe()

[Yongbang Shi]

> Hi
> 
> Am 23.04.26 um 08:32 schrieb Yongbang Shi:
>> From: Lin He <helin52@huawei.com>
>>
>> If there's no VGA output, this encoder modeset won't be called, which
>> will cause displaying data from GPU being cut off. It's actually a
>> common display config for DP and VGA, so move the vdac encoder modeset
>> to driver load stage.
>>
>> Fixes: 5294967f4ae4 ("drm/hisilicon/hibmc: Add support for VDAC")
>> Signed-off-by: Lin He <helin52@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v1 -> v2:
>>    - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
>>      be given in public.
>>    - add 'drm-misc-fixes' in subject prefix.
>> ---
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 14 ++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 22 -------------------
>>   2 files changed, 14 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index d409efc34215..f8ab471f92e7 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -217,6 +217,18 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv, unsigned int gate)
>>       writel(gate, mmio + gate_reg);
>>   }
>>   +static void hibmc_display_ctrl(struct hibmc_drm_private *priv)
>> +{
>> +    u32 reg;
>> +
>> +    reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>> +    reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
>> +    reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
>> +    reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
>> +    reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
> 
> That might be stupid question: IDK what these bits mean, but the FPVDDEN and FPEN sound like they enable something. If
> you do this unconditionally in HW init, does it prevent power saving?  And is the system goes through suspend/resume, 
> does it need to be re-enabled?
> 
> My gut feeling would be to put this code into the CRTC.
> 

We discussed this with the chip design team and confirmed, after reviewing the chip's RTL code, that FPVDDEN, FPEN and
VBIASEN are not used in the chip's RTL implementation; only PANELDATE is used, and this bit controls the output of the
data, hsync and vsync signals simultaneously. We will remove FPVDDEN, FPEN and VBIASEN in v6, and do some tests.

This PANELDATE should be enabled during initialization. In older versions of the driver, since there was only one
encoder-connector(VGA), placing it in `encoder_helper` posed no practical issues. Now that it has been moved to the
initialization phase, it is enabled only once during initialization, in accordance with the chip's original design
intent, which better aligns with the original design.

Regarding suspend/resume operations and power saving, we have implemented `atomic_enable` and `atomic_disable` for CRTC.
These hook functions configure hsync and vsync for the DPMS switch[1]. Moving the `DISPLAY_CONTROL_HISILE.PANELDATE`
enable flag to the initialization phase does not affect power saving.

[1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L190

Thanks,
Yongbang.

> Best regards
> Thomas
> 
>> +    writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>> +}
>> +
>>   static void hibmc_hw_config(struct hibmc_drm_private *priv)
>>   {
>>       u32 reg;
>> @@ -248,6 +260,8 @@ static void hibmc_hw_config(struct hibmc_drm_private *priv)
>>       reg |= HIBMC_MSCCTL_LOCALMEM_RESET(1);
>>         writel(reg, priv->mmio + HIBMC_MISC_CTRL);
>> +
>> +    hibmc_display_ctrl(priv);
>>   }
>>     static int hibmc_hw_map(struct hibmc_drm_private *priv)
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index dc40018dbd21..9d924740c110 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -86,26 +86,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>>       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>   };
>>   -static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
>> -                   struct drm_display_mode *mode,
>> -                   struct drm_display_mode *adj_mode)
>> -{
>> -    u32 reg;
>> -    struct drm_device *dev = encoder->dev;
>> -    struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>> -
>> -    reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>> -    reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
>> -    reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
>> -    reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
>> -    reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
>> -    writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>> -}
>> -
>> -static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
>> -    .mode_set = hibmc_encoder_mode_set,
>> -};
>> -
>>   int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>   {
>>       struct drm_device *dev = &priv->dev;
>> @@ -128,8 +108,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>           goto err;
>>       }
>>   -    drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>> -
>>       ret = drm_connector_init_with_ddc(dev, connector,
>>                         &hibmc_connector_funcs,
>>                         DRM_MODE_CONNECTOR_VGA,
> 

Re: [PATCH for drm-misc-fixes v5 3/4] drm/hisilicon/hibmc: move display contrl config to hibmc_probe()

[Thomas Zimmermann]

Hi

Am 28.04.26 um 09:55 schrieb Yongbang Shi:
>> Hi
>>
>> Am 23.04.26 um 08:32 schrieb Yongbang Shi:
>>> From: Lin He <helin52@huawei.com>
>>>
>>> If there's no VGA output, this encoder modeset won't be called, which
>>> will cause displaying data from GPU being cut off. It's actually a
>>> common display config for DP and VGA, so move the vdac encoder modeset
>>> to driver load stage.
>>>
>>> Fixes: 5294967f4ae4 ("drm/hisilicon/hibmc: Add support for VDAC")
>>> Signed-off-by: Lin He <helin52@huawei.com>
>>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>>> ---
>>> ChangeLog:
>>> v1 -> v2:
>>>     - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
>>>       be given in public.
>>>     - add 'drm-misc-fixes' in subject prefix.
>>> ---
>>>    .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 14 ++++++++++++
>>>    .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 22 -------------------
>>>    2 files changed, 14 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index d409efc34215..f8ab471f92e7 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -217,6 +217,18 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv, unsigned int gate)
>>>        writel(gate, mmio + gate_reg);
>>>    }
>>>    +static void hibmc_display_ctrl(struct hibmc_drm_private *priv)
>>> +{
>>> +    u32 reg;
>>> +
>>> +    reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>>> +    reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
>>> +    reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
>>> +    reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
>>> +    reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
>> That might be stupid question: IDK what these bits mean, but the FPVDDEN and FPEN sound like they enable something. If
>> you do this unconditionally in HW init, does it prevent power saving?  And is the system goes through suspend/resume,
>> does it need to be re-enabled?
>>
>> My gut feeling would be to put this code into the CRTC.
>>
> We discussed this with the chip design team and confirmed, after reviewing the chip's RTL code, that FPVDDEN, FPEN and
> VBIASEN are not used in the chip's RTL implementation; only PANELDATE is used, and this bit controls the output of the
> data, hsync and vsync signals simultaneously. We will remove FPVDDEN, FPEN and VBIASEN in v6, and do some tests.
>
> This PANELDATE should be enabled during initialization. In older versions of the driver, since there was only one
> encoder-connector(VGA), placing it in `encoder_helper` posed no practical issues. Now that it has been moved to the
> initialization phase, it is enabled only once during initialization, in accordance with the chip's original design
> intent, which better aligns with the original design.
>
> Regarding suspend/resume operations and power saving, we have implemented `atomic_enable` and `atomic_disable` for CRTC.
> These hook functions configure hsync and vsync for the DPMS switch[1]. Moving the `DISPLAY_CONTROL_HISILE.PANELDATE`
> enable flag to the initialization phase does not affect power saving.

Sounds good. Thanks for following up on this.

Best regards
Thomas

>
> [1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L190
>
> Thanks,
> Yongbang.
>
>> Best regards
>> Thomas
>>
>>> +    writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>>> +}
>>> +
>>>    static void hibmc_hw_config(struct hibmc_drm_private *priv)
>>>    {
>>>        u32 reg;
>>> @@ -248,6 +260,8 @@ static void hibmc_hw_config(struct hibmc_drm_private *priv)
>>>        reg |= HIBMC_MSCCTL_LOCALMEM_RESET(1);
>>>          writel(reg, priv->mmio + HIBMC_MISC_CTRL);
>>> +
>>> +    hibmc_display_ctrl(priv);
>>>    }
>>>      static int hibmc_hw_map(struct hibmc_drm_private *priv)
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>>> index dc40018dbd21..9d924740c110 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>>> @@ -86,26 +86,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>>>        .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>    };
>>>    -static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
>>> -                   struct drm_display_mode *mode,
>>> -                   struct drm_display_mode *adj_mode)
>>> -{
>>> -    u32 reg;
>>> -    struct drm_device *dev = encoder->dev;
>>> -    struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
>>> -
>>> -    reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>>> -    reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
>>> -    reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
>>> -    reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
>>> -    reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
>>> -    writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
>>> -}
>>> -
>>> -static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
>>> -    .mode_set = hibmc_encoder_mode_set,
>>> -};
>>> -
>>>    int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>>    {
>>>        struct drm_device *dev = &priv->dev;
>>> @@ -128,8 +108,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>>            goto err;
>>>        }
>>>    -    drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>>> -
>>>        ret = drm_connector_init_with_ddc(dev, connector,
>>>                          &hibmc_connector_funcs,
>>>                          DRM_MODE_CONNECTOR_VGA,

-- 
--
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)

[PATCH for drm-misc-fixes v5 4/4] drm/hisilicon/hibmc: use clock to look up the PLL value

[Yongbang Shi]

From: Lin He <helin52@huawei.com>

In the past, we use width and height to look up our PLL value.
But actually the actual clock check is also necessnary. There are
some resolutions that width and height same, but its clock different.
Add the clock check when using pll_table to determine the PLL value.

Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
Signed-off-by: Lin He <helin52@huawei.com>
Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
---
ChangeLog:
v2 -> v3:
  - remove unused macro CLOCK_TOLERANCE.
v1 -> v2:
  - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
    be given in public.
  - add 'drm-misc-fixes' in subject prefix.
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 78 +++++++++++--------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 89bed78f1466..1a07e8146eee 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -32,26 +32,43 @@ struct hibmc_display_panel_pll {
 struct hibmc_dislay_pll_config {
 	u64 hdisplay;
 	u64 vdisplay;
+	int clock;
 	u32 pll1_config_value;
 	u32 pll2_config_value;
 };
 
 static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
-	{640, 480, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
-	{800, 600, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
-	{1024, 768, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
-	{1152, 864, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
-	{1280, 768, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
-	{1280, 720, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
-	{1280, 960, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
-	{1280, 1024, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
-	{1440, 900, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
-	{1600, 900, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
-	{1600, 1200, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
-	{1920, 1080, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
-	{1920, 1200, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
+	{640, 480, 25000, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
+	{800, 600, 40000, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
+	{1024, 768, 65000, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
+	{1152, 864, 78750, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
+	{1280, 768, 80000, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
+	{1280, 720, 74375, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
+	{1280, 960, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
+	{1280, 1024, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
+	{1440, 900, 105952, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
+	{1600, 900, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
+	{1600, 1200, 162500, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
+	{1920, 1080, 148750, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
+	{1920, 1200, 193750, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
 };
 
+static int hibmc_get_best_clock_idx(const struct drm_display_mode *mode)
+{
+	int i, diff;
+
+	for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
+		if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
+		    hibmc_pll_table[i].vdisplay == mode->vdisplay) {
+			diff = abs(mode->clock - hibmc_pll_table[i].clock);
+			if (diff < mode->clock / 100) /* tolerance 1/100 */
+				return i;
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static int hibmc_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_atomic_state *state)
 {
@@ -214,17 +231,13 @@ static enum drm_mode_status
 hibmc_crtc_mode_valid(struct drm_crtc *crtc,
 		      const struct drm_display_mode *mode)
 {
-	size_t i = 0;
 	int vrefresh = drm_mode_vrefresh(mode);
 
 	if (vrefresh < 59 || vrefresh > 61)
 		return MODE_NOCLOCK;
 
-	for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
-		if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
-		    hibmc_pll_table[i].vdisplay == mode->vdisplay)
-			return MODE_OK;
-	}
+	if (hibmc_get_best_clock_idx(mode) >= 0)
+		return MODE_OK;
 
 	return MODE_BAD;
 }
@@ -281,23 +294,20 @@ static void set_vclock_hisilicon(struct drm_device *dev, u64 pll)
 	writel(val, priv->mmio + CRT_PLL1_HS);
 }
 
-static void get_pll_config(u64 x, u64 y, u32 *pll1, u32 *pll2)
+static void get_pll_config(struct drm_display_mode *mode, u32 *pll1, u32 *pll2)
 {
-	size_t i;
-	size_t count = ARRAY_SIZE(hibmc_pll_table);
-
-	for (i = 0; i < count; i++) {
-		if (hibmc_pll_table[i].hdisplay == x &&
-		    hibmc_pll_table[i].vdisplay == y) {
-			*pll1 = hibmc_pll_table[i].pll1_config_value;
-			*pll2 = hibmc_pll_table[i].pll2_config_value;
-			return;
-		}
+	int idx;
+
+	idx = hibmc_get_best_clock_idx(mode);
+	if (idx < 0) {
+		/* if found none, we use default value */
+		*pll1 = CRT_PLL1_HS_25MHZ;
+		*pll2 = CRT_PLL2_HS_25MHZ;
+		return;
 	}
 
-	/* if found none, we use default value */
-	*pll1 = CRT_PLL1_HS_25MHZ;
-	*pll2 = CRT_PLL2_HS_25MHZ;
+	*pll1 = hibmc_pll_table[idx].pll1_config_value;
+	*pll2 = hibmc_pll_table[idx].pll2_config_value;
 }
 
 /*
@@ -319,7 +329,7 @@ static u32 display_ctrl_adjust(struct drm_device *dev,
 	x = mode->hdisplay;
 	y = mode->vdisplay;
 
-	get_pll_config(x, y, &pll1, &pll2);
+	get_pll_config(mode, &pll1, &pll2);
 	writel(pll2, priv->mmio + CRT_PLL2_HS);
 	set_vclock_hisilicon(dev, pll1);
 
-- 
2.33.0

Re: [PATCH for drm-misc-fixes v5 4/4] drm/hisilicon/hibmc: use clock to look up the PLL value

[Thomas Zimmermann]

Hi

Am 23.04.26 um 08:32 schrieb Yongbang Shi:
> From: Lin He <helin52@huawei.com>
>
> In the past, we use width and height to look up our PLL value.
> But actually the actual clock check is also necessnary. There are
> some resolutions that width and height same, but its clock different.
> Add the clock check when using pll_table to determine the PLL value.
>
> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
> Signed-off-by: Lin He <helin52@huawei.com>
> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
> ---
> ChangeLog:
> v2 -> v3:
>    - remove unused macro CLOCK_TOLERANCE.
> v1 -> v2:
>    - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
>      be given in public.
>    - add 'drm-misc-fixes' in subject prefix.
> ---
>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 78 +++++++++++--------
>   1 file changed, 44 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 89bed78f1466..1a07e8146eee 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -32,26 +32,43 @@ struct hibmc_display_panel_pll {
>   struct hibmc_dislay_pll_config {
>   	u64 hdisplay;
>   	u64 vdisplay;
> +	int clock;
>   	u32 pll1_config_value;
>   	u32 pll2_config_value;
>   };
>   
>   static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
> -	{640, 480, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
> -	{800, 600, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
> -	{1024, 768, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
> -	{1152, 864, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
> -	{1280, 768, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
> -	{1280, 720, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
> -	{1280, 960, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
> -	{1280, 1024, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
> -	{1440, 900, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
> -	{1600, 900, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
> -	{1600, 1200, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
> -	{1920, 1080, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
> -	{1920, 1200, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
> +	{640, 480, 25000, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
> +	{800, 600, 40000, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
> +	{1024, 768, 65000, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
> +	{1152, 864, 78750, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
> +	{1280, 768, 80000, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
> +	{1280, 720, 74375, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
> +	{1280, 960, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
> +	{1280, 1024, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
> +	{1440, 900, 105952, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
> +	{1600, 900, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
> +	{1600, 1200, 162500, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
> +	{1920, 1080, 148750, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
> +	{1920, 1200, 193750, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
>   };
>   
> +static int hibmc_get_best_clock_idx(const struct drm_display_mode *mode)
> +{
> +	int i, diff;
> +
> +	for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
> +		if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
> +		    hibmc_pll_table[i].vdisplay == mode->vdisplay) {
> +			diff = abs(mode->clock - hibmc_pll_table[i].clock);
> +			if (diff < mode->clock / 100) /* tolerance 1/100 */
> +				return i;
> +		}
> +	}
> +
> +	return -EOPNOTSUPP;

This errno code is for sockets.

Rather return -MODE_CLOCK_RANGE here, which you can then return from 
.mode_valid as well.

Best regards
Thomas


> +}
> +
>   static int hibmc_plane_atomic_check(struct drm_plane *plane,
>   				    struct drm_atomic_state *state)
>   {
> @@ -214,17 +231,13 @@ static enum drm_mode_status
>   hibmc_crtc_mode_valid(struct drm_crtc *crtc,
>   		      const struct drm_display_mode *mode)
>   {
> -	size_t i = 0;
>   	int vrefresh = drm_mode_vrefresh(mode);
>   
>   	if (vrefresh < 59 || vrefresh > 61)
>   		return MODE_NOCLOCK;
>   
> -	for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
> -		if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
> -		    hibmc_pll_table[i].vdisplay == mode->vdisplay)
> -			return MODE_OK;
> -	}
> +	if (hibmc_get_best_clock_idx(mode) >= 0)
> +		return MODE_OK;
>   
>   	return MODE_BAD;
>   }
> @@ -281,23 +294,20 @@ static void set_vclock_hisilicon(struct drm_device *dev, u64 pll)
>   	writel(val, priv->mmio + CRT_PLL1_HS);
>   }
>   
> -static void get_pll_config(u64 x, u64 y, u32 *pll1, u32 *pll2)
> +static void get_pll_config(struct drm_display_mode *mode, u32 *pll1, u32 *pll2)
>   {
> -	size_t i;
> -	size_t count = ARRAY_SIZE(hibmc_pll_table);
> -
> -	for (i = 0; i < count; i++) {
> -		if (hibmc_pll_table[i].hdisplay == x &&
> -		    hibmc_pll_table[i].vdisplay == y) {
> -			*pll1 = hibmc_pll_table[i].pll1_config_value;
> -			*pll2 = hibmc_pll_table[i].pll2_config_value;
> -			return;
> -		}
> +	int idx;
> +
> +	idx = hibmc_get_best_clock_idx(mode);
> +	if (idx < 0) {
> +		/* if found none, we use default value */
> +		*pll1 = CRT_PLL1_HS_25MHZ;
> +		*pll2 = CRT_PLL2_HS_25MHZ;
> +		return;
>   	}
>   
> -	/* if found none, we use default value */
> -	*pll1 = CRT_PLL1_HS_25MHZ;
> -	*pll2 = CRT_PLL2_HS_25MHZ;
> +	*pll1 = hibmc_pll_table[idx].pll1_config_value;
> +	*pll2 = hibmc_pll_table[idx].pll2_config_value;
>   }
>   
>   /*
> @@ -319,7 +329,7 @@ static u32 display_ctrl_adjust(struct drm_device *dev,
>   	x = mode->hdisplay;
>   	y = mode->vdisplay;
>   
> -	get_pll_config(x, y, &pll1, &pll2);
> +	get_pll_config(mode, &pll1, &pll2);
>   	writel(pll2, priv->mmio + CRT_PLL2_HS);
>   	set_vclock_hisilicon(dev, pll1);
>   

-- 
--
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)

Re: [PATCH for drm-misc-fixes v5 4/4] drm/hisilicon/hibmc: use clock to look up the PLL value

[Yongbang Shi]

> Hi
> 
> Am 23.04.26 um 08:32 schrieb Yongbang Shi:
>> From: Lin He <helin52@huawei.com>
>>
>> In the past, we use width and height to look up our PLL value.
>> But actually the actual clock check is also necessnary. There are
>> some resolutions that width and height same, but its clock different.
>> Add the clock check when using pll_table to determine the PLL value.
>>
>> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
>> Signed-off-by: Lin He <helin52@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - remove unused macro CLOCK_TOLERANCE.
>> v1 -> v2:
>>    - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", witch will
>>      be given in public.
>>    - add 'drm-misc-fixes' in subject prefix.
>> ---
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 78 +++++++++++--------
>>   1 file changed, 44 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> index 89bed78f1466..1a07e8146eee 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -32,26 +32,43 @@ struct hibmc_display_panel_pll {
>>   struct hibmc_dislay_pll_config {
>>       u64 hdisplay;
>>       u64 vdisplay;
>> +    int clock;
>>       u32 pll1_config_value;
>>       u32 pll2_config_value;
>>   };
>>     static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
>> -    {640, 480, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
>> -    {800, 600, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
>> -    {1024, 768, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
>> -    {1152, 864, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
>> -    {1280, 768, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
>> -    {1280, 720, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
>> -    {1280, 960, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> -    {1280, 1024, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> -    {1440, 900, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
>> -    {1600, 900, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> -    {1600, 1200, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
>> -    {1920, 1080, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
>> -    {1920, 1200, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
>> +    {640, 480, 25000, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
>> +    {800, 600, 40000, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
>> +    {1024, 768, 65000, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
>> +    {1152, 864, 78750, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
>> +    {1280, 768, 80000, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
>> +    {1280, 720, 74375, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
>> +    {1280, 960, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> +    {1280, 1024, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> +    {1440, 900, 105952, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
>> +    {1600, 900, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> +    {1600, 1200, 162500, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
>> +    {1920, 1080, 148750, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
>> +    {1920, 1200, 193750, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
>>   };
>>   +static int hibmc_get_best_clock_idx(const struct drm_display_mode *mode)
>> +{
>> +    int i, diff;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
>> +        if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
>> +            hibmc_pll_table[i].vdisplay == mode->vdisplay) {
>> +            diff = abs(mode->clock - hibmc_pll_table[i].clock);
>> +            if (diff < mode->clock / 100) /* tolerance 1/100 */
>> +                return i;
>> +        }
>> +    }
>> +
>> +    return -EOPNOTSUPP;
> 
> This errno code is for sockets.
> 
> Rather return -MODE_CLOCK_RANGE here, which you can then return from .mode_valid as well.
> 

Okay. I will fix in v6.

Thanks,
Yongbang.

> Best regards
> Thomas
> 
> 
>> +}
>> +
>>   static int hibmc_plane_atomic_check(struct drm_plane *plane,
>>                       struct drm_atomic_state *state)
>>   {
>> @@ -214,17 +231,13 @@ static enum drm_mode_status
>>   hibmc_crtc_mode_valid(struct drm_crtc *crtc,
>>                 const struct drm_display_mode *mode)
>>   {
>> -    size_t i = 0;
>>       int vrefresh = drm_mode_vrefresh(mode);
>>         if (vrefresh < 59 || vrefresh > 61)
>>           return MODE_NOCLOCK;
>>   -    for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
>> -        if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
>> -            hibmc_pll_table[i].vdisplay == mode->vdisplay)
>> -            return MODE_OK;
>> -    }
>> +    if (hibmc_get_best_clock_idx(mode) >= 0)
>> +        return MODE_OK;
>>         return MODE_BAD;
>>   }
>> @@ -281,23 +294,20 @@ static void set_vclock_hisilicon(struct drm_device *dev, u64 pll)
>>       writel(val, priv->mmio + CRT_PLL1_HS);
>>   }
>>   -static void get_pll_config(u64 x, u64 y, u32 *pll1, u32 *pll2)
>> +static void get_pll_config(struct drm_display_mode *mode, u32 *pll1, u32 *pll2)
>>   {
>> -    size_t i;
>> -    size_t count = ARRAY_SIZE(hibmc_pll_table);
>> -
>> -    for (i = 0; i < count; i++) {
>> -        if (hibmc_pll_table[i].hdisplay == x &&
>> -            hibmc_pll_table[i].vdisplay == y) {
>> -            *pll1 = hibmc_pll_table[i].pll1_config_value;
>> -            *pll2 = hibmc_pll_table[i].pll2_config_value;
>> -            return;
>> -        }
>> +    int idx;
>> +
>> +    idx = hibmc_get_best_clock_idx(mode);
>> +    if (idx < 0) {
>> +        /* if found none, we use default value */
>> +        *pll1 = CRT_PLL1_HS_25MHZ;
>> +        *pll2 = CRT_PLL2_HS_25MHZ;
>> +        return;
>>       }
>>   -    /* if found none, we use default value */
>> -    *pll1 = CRT_PLL1_HS_25MHZ;
>> -    *pll2 = CRT_PLL2_HS_25MHZ;
>> +    *pll1 = hibmc_pll_table[idx].pll1_config_value;
>> +    *pll2 = hibmc_pll_table[idx].pll2_config_value;
>>   }
>>     /*
>> @@ -319,7 +329,7 @@ static u32 display_ctrl_adjust(struct drm_device *dev,
>>       x = mode->hdisplay;
>>       y = mode->vdisplay;
>>   -    get_pll_config(x, y, &pll1, &pll2);
>> +    get_pll_config(mode, &pll1, &pll2);
>>       writel(pll2, priv->mmio + CRT_PLL2_HS);
>>       set_vclock_hisilicon(dev, pll1);
>>   
>