[v1] Fix hibmc driver bugs

[PATCH drm-dp 06/10] drm/hisilicon/hibmc: add dp mode valid check

Posted by Yongbang Shi 6 months, 3 weeks ago

From: Baihan Li <libaihan@huawei.com>

If DP is connected, add mode check and BW check in mode_valid_ctx() to
ensure DP's cfg is usable.

Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
Signed-off-by: Baihan Li <libaihan@huawei.com>
---
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 10 ++++
 drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  7 +++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 59 +++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
index ee0b543afd7f..4f93d60b932b 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
@@ -259,6 +259,16 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp)
 	dp->dp_dev->link.status.channel_equalized = false;
 }
 
+u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.link_rate;
+}
+
+u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
+{
+	return dp->dp_dev->link.cap.lanes;
+}
+
 static const struct hibmc_dp_color_raw g_rgb_raw[] = {
 	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
 	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
index 68867475508c..ebc7256ad006 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
@@ -12,6 +12,10 @@
 #include <drm/drm_print.h>
 #include <drm/display/drm_dp_helper.h>
 
+/* 27 * 10000000 * 80% = 216000000 */
+#define DP_MODE_VALI_CAL	216000000
+#define BPP_24				24
+
 struct hibmc_dp_dev;
 
 enum hibmc_dp_cbar_pattern {
@@ -51,6 +55,7 @@ struct hibmc_dp {
 	struct hibmc_dp_cbar_cfg cfg;
 	u32 irq_status;
 	int hpd_status;
+	bool is_connected;
 };
 
 int hibmc_dp_hw_init(struct hibmc_dp *dp);
@@ -61,5 +66,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
 void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
 void hibmc_dp_enable_int(struct hibmc_dp *dp);
 void hibmc_dp_disable_int(struct hibmc_dp *dp);
+u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
+u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
 
 #endif
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
index 191fb434baa7..e4b13f21ccb3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
@@ -16,8 +16,31 @@
 #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
 #define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
 
+struct hibmc_dp_disp_clk {
+	u16 hdisplay;
+	u16 vdisplay;
+	u32 clock;
+};
+
+static const struct hibmc_dp_disp_clk hibmc_dp_clk_table[] = {
+	{640, 480, 25175}, /* 25175 khz */
+	{800, 600, 40000}, /* 40000 khz */
+	{1024, 768, 65000}, /* 65000 khz */
+	{1152, 864, 80000}, /* 80000 khz */
+	{1280, 768, 79500}, /* 79500 khz */
+	{1280, 720, 74250}, /* 74250 khz */
+	{1280, 960, 108000}, /* 108000 khz */
+	{1280, 1024, 108000}, /* 108000 khz */
+	{1440, 900, 106500}, /* 106500 khz */
+	{1600, 900, 108000}, /* 108000 khz */
+	{1600, 1200, 162000}, /* 162000 khz */
+	{1920, 1080, 148500}, /* 148500 khz */
+	{1920, 1200, 193250}, /* 193250 khz */
+};
+
 static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
 {
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
 	const struct drm_edid *drm_edid;
 	int count;
 
@@ -27,6 +50,8 @@ static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
 
 	count = drm_edid_connector_add_modes(connector);
 
+	dp->is_connected = !!count;
+
 	drm_edid_free(drm_edid);
 
 	return count;
@@ -43,9 +68,43 @@ static int hibmc_dp_detect(struct drm_connector *connector,
 		return connector_status_disconnected;
 }
 
+static int hibmc_dp_mode_valid(struct drm_connector *connector,
+			       const struct drm_display_mode *mode,
+			       struct drm_modeset_acquire_ctx *ctx,
+			       enum drm_mode_status *status)
+{
+	struct hibmc_dp *dp = to_hibmc_dp(connector);
+	u64 cur_val, max_val;
+
+	if (!dp->is_connected) {
+		*status = MODE_OK;
+		return 0;
+	}
+
+	/* check DP link BW */
+	cur_val = (u64)mode->htotal * mode->vtotal * drm_mode_vrefresh(mode) * BPP_24;
+	max_val = (u64)hibmc_dp_get_link_rate(dp) * DP_MODE_VALI_CAL * hibmc_dp_get_lanes(dp);
+	if (cur_val > max_val)
+		*status = MODE_CLOCK_HIGH;
+	else
+		*status = MODE_OK;
+
+	/* check the clock */
+	for (size_t i = 0; i < ARRAY_SIZE(hibmc_dp_clk_table); i++) {
+		if (hibmc_dp_clk_table[i].hdisplay == mode->hdisplay &&
+		    hibmc_dp_clk_table[i].vdisplay == mode->vdisplay) {
+			if (hibmc_dp_clk_table[i].clock != mode->clock)
+				*status = MODE_CLOCK_RANGE;
+		}
+	}
+
+	return 0;
+}
+
 static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
 	.get_modes = hibmc_dp_connector_get_modes,
 	.detect_ctx = hibmc_dp_detect,
+	.mode_valid_ctx = hibmc_dp_mode_valid,
 };
 
 static int hibmc_dp_late_register(struct drm_connector *connector)
-- 
2.33.0

Re: [PATCH drm-dp 06/10] drm/hisilicon/hibmc: add dp mode valid check

Posted by Dmitry Baryshkov 6 months, 1 week ago

On Fri, May 30, 2025 at 05:54:28PM +0800, Yongbang Shi wrote:
> From: Baihan Li <libaihan@huawei.com>
> 
> If DP is connected, add mode check and BW check in mode_valid_ctx() to
> ensure DP's cfg is usable.
> 
> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
> Signed-off-by: Baihan Li <libaihan@huawei.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 10 ++++
>  drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  7 +++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 59 +++++++++++++++++++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> index ee0b543afd7f..4f93d60b932b 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
> @@ -259,6 +259,16 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp)
>  	dp->dp_dev->link.status.channel_equalized = false;
>  }
>  
> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.link_rate;
> +}
> +
> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
> +{
> +	return dp->dp_dev->link.cap.lanes;
> +}
> +
>  static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>  	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>  	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> index 68867475508c..ebc7256ad006 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
> @@ -12,6 +12,10 @@
>  #include <drm/drm_print.h>
>  #include <drm/display/drm_dp_helper.h>
>  
> +/* 27 * 10000000 * 80% = 216000000 */
> +#define DP_MODE_VALI_CAL	216000000
> +#define BPP_24				24
> +
>  struct hibmc_dp_dev;
>  
>  enum hibmc_dp_cbar_pattern {
> @@ -51,6 +55,7 @@ struct hibmc_dp {
>  	struct hibmc_dp_cbar_cfg cfg;
>  	u32 irq_status;
>  	int hpd_status;
> +	bool is_connected;
>  };
>  
>  int hibmc_dp_hw_init(struct hibmc_dp *dp);
> @@ -61,5 +66,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
>  void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>  void hibmc_dp_enable_int(struct hibmc_dp *dp);
>  void hibmc_dp_disable_int(struct hibmc_dp *dp);
> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>  
>  #endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> index 191fb434baa7..e4b13f21ccb3 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
> @@ -16,8 +16,31 @@
>  #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>  #define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
>  
> +struct hibmc_dp_disp_clk {
> +	u16 hdisplay;
> +	u16 vdisplay;
> +	u32 clock;
> +};
> +
> +static const struct hibmc_dp_disp_clk hibmc_dp_clk_table[] = {
> +	{640, 480, 25175}, /* 25175 khz */
> +	{800, 600, 40000}, /* 40000 khz */
> +	{1024, 768, 65000}, /* 65000 khz */
> +	{1152, 864, 80000}, /* 80000 khz */
> +	{1280, 768, 79500}, /* 79500 khz */
> +	{1280, 720, 74250}, /* 74250 khz */
> +	{1280, 960, 108000}, /* 108000 khz */
> +	{1280, 1024, 108000}, /* 108000 khz */
> +	{1440, 900, 106500}, /* 106500 khz */
> +	{1600, 900, 108000}, /* 108000 khz */
> +	{1600, 1200, 162000}, /* 162000 khz */
> +	{1920, 1080, 148500}, /* 148500 khz */
> +	{1920, 1200, 193250}, /* 193250 khz */
> +};
> +
>  static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>  {
> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>  	const struct drm_edid *drm_edid;
>  	int count;
>  
> @@ -27,6 +50,8 @@ static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>  
>  	count = drm_edid_connector_add_modes(connector);
>  
> +	dp->is_connected = !!count;
> +
>  	drm_edid_free(drm_edid);
>  
>  	return count;
> @@ -43,9 +68,43 @@ static int hibmc_dp_detect(struct drm_connector *connector,
>  		return connector_status_disconnected;
>  }
>  
> +static int hibmc_dp_mode_valid(struct drm_connector *connector,
> +			       const struct drm_display_mode *mode,
> +			       struct drm_modeset_acquire_ctx *ctx,
> +			       enum drm_mode_status *status)
> +{
> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
> +	u64 cur_val, max_val;
> +
> +	if (!dp->is_connected) {
> +		*status = MODE_OK;

No, mode_valid should not depend on DP being connected.

> +		return 0;
> +	}
> +
> +	/* check DP link BW */
> +	cur_val = (u64)mode->htotal * mode->vtotal * drm_mode_vrefresh(mode) * BPP_24;
> +	max_val = (u64)hibmc_dp_get_link_rate(dp) * DP_MODE_VALI_CAL * hibmc_dp_get_lanes(dp);
> +	if (cur_val > max_val)
> +		*status = MODE_CLOCK_HIGH;
> +	else
> +		*status = MODE_OK;
> +
> +	/* check the clock */
> +	for (size_t i = 0; i < ARRAY_SIZE(hibmc_dp_clk_table); i++) {
> +		if (hibmc_dp_clk_table[i].hdisplay == mode->hdisplay &&
> +		    hibmc_dp_clk_table[i].vdisplay == mode->vdisplay) {
> +			if (hibmc_dp_clk_table[i].clock != mode->clock)
> +				*status = MODE_CLOCK_RANGE;

Why?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>  	.get_modes = hibmc_dp_connector_get_modes,
>  	.detect_ctx = hibmc_dp_detect,
> +	.mode_valid_ctx = hibmc_dp_mode_valid,
>  };
>  
>  static int hibmc_dp_late_register(struct drm_connector *connector)
> -- 
> 2.33.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH drm-dp 06/10] drm/hisilicon/hibmc: add dp mode valid check

Posted by Yongbang Shi 6 months, 1 week ago

> On Fri, May 30, 2025 at 05:54:28PM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> If DP is connected, add mode check and BW check in mode_valid_ctx() to
>> ensure DP's cfg is usable.
>>
>> Fixes: f9698f802e50 ("drm/hisilicon/hibmc: Restructuring the header dp_reg.h")
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    | 10 ++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  7 +++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 59 +++++++++++++++++++
>>   3 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index ee0b543afd7f..4f93d60b932b 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -259,6 +259,16 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp)
>>   	dp->dp_dev->link.status.channel_equalized = false;
>>   }
>>   
>> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp)
>> +{
>> +	return dp->dp_dev->link.cap.link_rate;
>> +}
>> +
>> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp)
>> +{
>> +	return dp->dp_dev->link.cap.lanes;
>> +}
>> +
>>   static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>>   	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>>   	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 68867475508c..ebc7256ad006 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -12,6 +12,10 @@
>>   #include <drm/drm_print.h>
>>   #include <drm/display/drm_dp_helper.h>
>>   
>> +/* 27 * 10000000 * 80% = 216000000 */
>> +#define DP_MODE_VALI_CAL	216000000
>> +#define BPP_24				24
>> +
>>   struct hibmc_dp_dev;
>>   
>>   enum hibmc_dp_cbar_pattern {
>> @@ -51,6 +55,7 @@ struct hibmc_dp {
>>   	struct hibmc_dp_cbar_cfg cfg;
>>   	u32 irq_status;
>>   	int hpd_status;
>> +	bool is_connected;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>> @@ -61,5 +66,7 @@ void hibmc_dp_reset_link(struct hibmc_dp *dp);
>>   void hibmc_dp_hpd_cfg(struct hibmc_dp *dp);
>>   void hibmc_dp_enable_int(struct hibmc_dp *dp);
>>   void hibmc_dp_disable_int(struct hibmc_dp *dp);
>> +u8 hibmc_dp_get_link_rate(struct hibmc_dp *dp);
>> +u8 hibmc_dp_get_lanes(struct hibmc_dp *dp);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> index 191fb434baa7..e4b13f21ccb3 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>> @@ -16,8 +16,31 @@
>>   #define HIBMC_DP_MASKED_SINK_HPD_PLUG_INT	BIT(2)
>>   #define HIBMC_DP_MASKED_SINK_HPD_UNPLUG_INT	BIT(3)
>>   
>> +struct hibmc_dp_disp_clk {
>> +	u16 hdisplay;
>> +	u16 vdisplay;
>> +	u32 clock;
>> +};
>> +
>> +static const struct hibmc_dp_disp_clk hibmc_dp_clk_table[] = {
>> +	{640, 480, 25175}, /* 25175 khz */
>> +	{800, 600, 40000}, /* 40000 khz */
>> +	{1024, 768, 65000}, /* 65000 khz */
>> +	{1152, 864, 80000}, /* 80000 khz */
>> +	{1280, 768, 79500}, /* 79500 khz */
>> +	{1280, 720, 74250}, /* 74250 khz */
>> +	{1280, 960, 108000}, /* 108000 khz */
>> +	{1280, 1024, 108000}, /* 108000 khz */
>> +	{1440, 900, 106500}, /* 106500 khz */
>> +	{1600, 900, 108000}, /* 108000 khz */
>> +	{1600, 1200, 162000}, /* 162000 khz */
>> +	{1920, 1080, 148500}, /* 148500 khz */
>> +	{1920, 1200, 193250}, /* 193250 khz */
>> +};
>> +
>>   static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>   {
>> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>>   	const struct drm_edid *drm_edid;
>>   	int count;
>>   
>> @@ -27,6 +50,8 @@ static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>   
>>   	count = drm_edid_connector_add_modes(connector);
>>   
>> +	dp->is_connected = !!count;
>> +
>>   	drm_edid_free(drm_edid);
>>   
>>   	return count;
>> @@ -43,9 +68,43 @@ static int hibmc_dp_detect(struct drm_connector *connector,
>>   		return connector_status_disconnected;
>>   }
>>   
>> +static int hibmc_dp_mode_valid(struct drm_connector *connector,
>> +			       const struct drm_display_mode *mode,
>> +			       struct drm_modeset_acquire_ctx *ctx,
>> +			       enum drm_mode_status *status)
>> +{
>> +	struct hibmc_dp *dp = to_hibmc_dp(connector);
>> +	u64 cur_val, max_val;
>> +
>> +	if (!dp->is_connected) {
>> +		*status = MODE_OK;
> No, mode_valid should not depend on DP being connected.

Okay!


>> +		return 0;
>> +	}
>> +
>> +	/* check DP link BW */
>> +	cur_val = (u64)mode->htotal * mode->vtotal * drm_mode_vrefresh(mode) * BPP_24;
>> +	max_val = (u64)hibmc_dp_get_link_rate(dp) * DP_MODE_VALI_CAL * hibmc_dp_get_lanes(dp);
>> +	if (cur_val > max_val)
>> +		*status = MODE_CLOCK_HIGH;
>> +	else
>> +		*status = MODE_OK;
>> +
>> +	/* check the clock */
>> +	for (size_t i = 0; i < ARRAY_SIZE(hibmc_dp_clk_table); i++) {
>> +		if (hibmc_dp_clk_table[i].hdisplay == mode->hdisplay &&
>> +		    hibmc_dp_clk_table[i].vdisplay == mode->vdisplay) {
>> +			if (hibmc_dp_clk_table[i].clock != mode->clock)
>> +				*status = MODE_CLOCK_RANGE;
> Why?

Because the clock will be different in different standards, but our GPU cannot generate these clock frequencies.


>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct drm_connector_helper_funcs hibmc_dp_conn_helper_funcs = {
>>   	.get_modes = hibmc_dp_connector_get_modes,
>>   	.detect_ctx = hibmc_dp_detect,
>> +	.mode_valid_ctx = hibmc_dp_mode_valid,
>>   };
>>   
>>   static int hibmc_dp_late_register(struct drm_connector *connector)
>> -- 
>> 2.33.0
>>