diff mbox series

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

Message ID 20240717143319.104012-5-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/ast: Fix DP hotplugging and clean up | expand

Commit Message

Thomas Zimmermann July 17, 2024, 2:24 p.m. UTC 
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.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 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(-)

Comments

Jocelyn Falempe July 30, 2024, 7:41 a.m. UTC | #1 
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@redhat.com>


> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   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)
>
diff mbox series

Patch

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)