Re: [PATCH v2] drm/ast: Fix long time waiting on s3/s4 resume

[Jammy Huang <jammy_huang@aspeedtech.com>]

Hi Thomas,

Could you help review this patch?

This is an issue leading to kernel panic found by Intel. Wendy has 
confirmed issue resolved by this patch.

On 2023/4/14 下午 03:42, Jammy Huang wrote:
> In resume, DP's launch function, ast_dp_launch, could wait at most 30
> seconds before timeout to check if DP is enabled.
>
> To avoid this problem, we only check if DP enable or not at driver probe.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217278
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> ---
>   v2 changes:
>    - Fix build error.
> ---
>   drivers/gpu/drm/ast/ast_dp.c   | 55 +++++++++++-----------------------
>   drivers/gpu/drm/ast/ast_drv.h  |  2 +-
>   drivers/gpu/drm/ast/ast_main.c | 11 +++++--
>   drivers/gpu/drm/ast/ast_post.c |  3 +-
>   4 files changed, 29 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 56483860306b..eee2f264c880 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -119,53 +119,32 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>   /*
>    * Launch Aspeed DP
>    */
> -void ast_dp_launch(struct drm_device *dev, u8 bPower)
> +void ast_dp_launch(struct drm_device *dev)
>   {
> -	u32 i = 0, j = 0, WaitCount = 1;
> -	u8 bDPTX = 0;
> +	u32 i = 0;
>   	u8 bDPExecute = 1;
> -
>   	struct ast_private *ast = to_ast_private(dev);
> -	// S3 come back, need more time to wait BMC ready.
> -	if (bPower)
> -		WaitCount = 300;
> -
> -
> -	// Wait total count by different condition.
> -	for (j = 0; j < WaitCount; j++) {
> -		bDPTX = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, TX_TYPE_MASK);
> -
> -		if (bDPTX)
> -			break;
>   
> +	// Wait one second then timeout.
> +	while (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, COPROCESSOR_LAUNCH) !=
> +		COPROCESSOR_LAUNCH) {
> +		i++;
> +		// wait 100 ms
>   		msleep(100);
> -	}
>   
> -	// 0xE : ASTDP with DPMCU FW handling
> -	if (bDPTX == ASTDP_DPMCU_TX) {
> -		// Wait one second then timeout.
> -		i = 0;
> -
> -		while (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, COPROCESSOR_LAUNCH) !=
> -			COPROCESSOR_LAUNCH) {
> -			i++;
> -			// wait 100 ms
> -			msleep(100);
> -
> -			if (i >= 10) {
> -				// DP would not be ready.
> -				bDPExecute = 0;
> -				break;
> -			}
> +		if (i >= 10) {
> +			// DP would not be ready.
> +			bDPExecute = 0;
> +			break;
>   		}
> +	}
>   
> -		if (bDPExecute)
> -			ast->tx_chip_types |= BIT(AST_TX_ASTDP);
> +	if (!bDPExecute)
> +		drm_err(dev, "Wait DPMCU executing timeout\n");
>   
> -		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE5,
> -							(u8) ~ASTDP_HOST_EDID_READ_DONE_MASK,
> -							ASTDP_HOST_EDID_READ_DONE);
> -	}
> +	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE5,
> +			       (u8) ~ASTDP_HOST_EDID_READ_DONE_MASK,
> +			       ASTDP_HOST_EDID_READ_DONE);
>   }
>   
>   
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index d51b81fea9c8..15e86394be4f 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -498,7 +498,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
>   
>   /* aspeed DP */
>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
> -void ast_dp_launch(struct drm_device *dev, u8 bPower);
> +void ast_dp_launch(struct drm_device *dev);
>   void ast_dp_power_on_off(struct drm_device *dev, bool no);
>   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_main.c b/drivers/gpu/drm/ast/ast_main.c
> index f83ce77127cb..8ecddf20113f 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -254,8 +254,13 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>   		case 0x0c:
>   			ast->tx_chip_types = AST_TX_DP501_BIT;
>   		}
> -	} else if (ast->chip == AST2600)
> -		ast_dp_launch(&ast->base, 0);
> +	} else if (ast->chip == AST2600) {
> +		if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, TX_TYPE_MASK) ==
> +		    ASTDP_DPMCU_TX) {
> +			ast->tx_chip_types = AST_TX_ASTDP_BIT;
> +			ast_dp_launch(&ast->base);
> +		}
> +	}
>   
>   	/* Print stuff for diagnostic purposes */
>   	if (ast->tx_chip_types & AST_TX_NONE_BIT)
> @@ -264,6 +269,8 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>   		drm_info(dev, "Using Sil164 TMDS transmitter\n");
>   	if (ast->tx_chip_types & AST_TX_DP501_BIT)
>   		drm_info(dev, "Using DP501 DisplayPort transmitter\n");
> +	if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
> +		drm_info(dev, "Using ASPEED DisplayPort transmitter\n");
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index 82fd3c8adee1..90e40f59aff7 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -380,7 +380,8 @@ void ast_post_gpu(struct drm_device *dev)
>   	ast_set_def_ext_reg(dev);
>   
>   	if (ast->chip == AST2600) {
> -		ast_dp_launch(dev, 1);
> +		if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
> +			ast_dp_launch(dev);
>   	} else if (ast->config_mode == ast_use_p2a) {
>   		if (ast->chip == AST2500)
>   			ast_post_chip_2500(dev);
>
> base-commit: e62252bc55b6d4eddc6c2bdbf95a448180d6a08d

-- 
Best Regards
Jammy