Re: [PATCH 4/5] drm/ast: astdp: Perform link training during atomic_enable

Jocelyn Falempe <jfalempe@xxxxxxxxxx> · Tue, 30 Jul 2024 09:41:23 +0200

On 17/07/2024 16:24, Thomas Zimmermann wrote:
The place for link training is in the encoder's atomic_enable
helper. Remove all related tests from other helper ASTDP functions;
especially ast_astdp_is_connected(), which tests HPD status.

DP link training is controlled by the firmware. A status flag reports
success or failure. The process can be fragile on Aspeed hardware. Moving
the test from connector detection to the atomic_enable allows for several
retries and a longer timeout.


Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>


Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---
  drivers/gpu/drm/ast/ast_dp.c   | 45 +++++++++++++++++-----------------
  drivers/gpu/drm/ast/ast_drv.h  |  1 +
  drivers/gpu/drm/ast/ast_mode.c |  2 ++
  drivers/gpu/drm/ast/ast_reg.h  |  3 +--
  4 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index c45b336baf45..6cbde46f24dc 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -11,8 +11,6 @@ bool ast_astdp_is_connected(struct ast_device *ast)
  {
  	if (!ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDF, AST_IO_VGACRDF_HPD))
  		return false;
-	if (!ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDC, ASTDP_LINK_SUCCESS))
-		return false;
  	return true;
  }
  
@@ -22,14 +20,10 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
  	u8 i = 0, j = 0;
  
  	/*
-	 * CRDC[b0]: DP link success
  	 * CRE5[b0]: Host reading EDID process is done
  	 */
-	if (!(ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDC, ASTDP_LINK_SUCCESS) &&
-		ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xE5,
-								ASTDP_HOST_EDID_READ_DONE_MASK))) {
+	if (!(ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xE5, ASTDP_HOST_EDID_READ_DONE_MASK)))
  		goto err_astdp_edid_not_ready;
-	}
  
  	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE5, (u8) ~ASTDP_HOST_EDID_READ_DONE_MASK,
  							0x00);
@@ -58,11 +52,6 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
  			 */
  			mdelay(j+1);
  
-			if (!(ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDC,
-							ASTDP_LINK_SUCCESS))) {
-				goto err_astdp_jump_out_loop_of_edid;
-			}
-
  			j++;
  			if (j > 200)
  				goto err_astdp_jump_out_loop_of_edid;
@@ -106,8 +95,6 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
  	return (~(j+256) + 1);
  
  err_astdp_edid_not_ready:
-	if (!(ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDC, ASTDP_LINK_SUCCESS)))
-		return (~0xDC + 1);
  	if (!(ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xE5, ASTDP_HOST_EDID_READ_DONE_MASK)))
  		return (~0xE5 + 1);
  
@@ -165,7 +152,22 @@ void ast_dp_power_on_off(struct drm_device *dev, bool on)
  	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE3, (u8) ~AST_DP_PHY_SLEEP, bE3);
  }
  
+void ast_dp_link_training(struct ast_device *ast)
+{
+	struct drm_device *dev = &ast->base;
+	unsigned int i = 10;
+
+	while (i--) {
+		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
  
+		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
+			break;
+		if (i)
+			msleep(100);
+	}
+	if (!i)
+		drm_err(dev, "Link training failed\n");
+}
  
  void ast_dp_set_on_off(struct drm_device *dev, bool on)
  {
@@ -176,16 +178,13 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on)
  	// Video On/Off
  	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xE3, (u8) ~AST_DP_VIDEO_ENABLE, on);
  
-	// If DP plug in and link successful then check video on / off status
-	if (ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDC, ASTDP_LINK_SUCCESS)) {
-		video_on_off <<= 4;
-		while (ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDF,
+	video_on_off <<= 4;
+	while (ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xDF,
  						ASTDP_MIRROR_VIDEO_ENABLE) != video_on_off) {
-			// wait 1 ms
-			mdelay(1);
-			if (++i > 200)
-				break;
-		}
+		// wait 1 ms
+		mdelay(1);
+		if (++i > 200)
+			break;
  	}
  }
  
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 02476eb78119..d23b98ce4359 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -474,6 +474,7 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
  int ast_dp_launch(struct ast_device *ast);
  bool ast_dp_power_is_on(struct ast_device *ast);
  void ast_dp_power_on_off(struct drm_device *dev, bool no);
+void ast_dp_link_training(struct ast_device *ast);
  void ast_dp_set_on_off(struct drm_device *dev, bool no);
  void ast_dp_set_mode(struct drm_crtc *crtc, struct ast_vbios_mode_info *vbios_mode);
  
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 049ee1477c33..ddb7696acc04 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1622,6 +1622,8 @@ static void ast_astdp_encoder_helper_atomic_enable(struct drm_encoder *encoder,
  	struct ast_device *ast = to_ast_device(dev);
  
  	ast_dp_power_on_off(dev, AST_DP_POWER_ON);
+	ast_dp_link_training(ast);
+
  	ast_wait_for_vretrace(ast);
  	ast_dp_set_on_off(dev, 1);
  }
diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
index e61954dabf1a..28bb43f6795b 100644
--- a/drivers/gpu/drm/ast/ast_reg.h
+++ b/drivers/gpu/drm/ast/ast_reg.h
@@ -38,6 +38,7 @@
  #define AST_IO_VGACRCB_HWC_ENABLED	BIT(1)
  
  #define AST_IO_VGACRD1_MCU_FW_EXECUTING	BIT(5)
+#define AST_IO_VGACRDC_LINK_SUCCESS	BIT(0)
  #define AST_IO_VGACRDF_HPD		BIT(0)
  
  #define AST_IO_VGAIR1_R			(0x5A)
@@ -70,10 +71,8 @@
  #define AST_DP_VIDEO_ENABLE		BIT(0)
  
  /*
- * CRDC[b0]: DP link success
   * CRE5[b0]: Host reading EDID process is done
   */
-#define ASTDP_LINK_SUCCESS		BIT(0)
  #define ASTDP_HOST_EDID_READ_DONE	BIT(0)
  #define ASTDP_HOST_EDID_READ_DONE_MASK	GENMASK(0, 0)
  







