Re: [PATCH v3] drm/ast: Support timings, 1280x720/1280x960/1600x900

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Jamie


Am 25.12.24 um 01:32 schrieb Jammy Huang:
> In this patch, 3 new timings are added into support list.
>
> If you want to have new timings, 1280x720 and 1280x960 on DisplayPort,
> your dp-fw should be newer than version, 20240502.

Thank you for the patch.

What happens if the firmware is not at the required version?

As it seems unreasonable to expect everyone to upgrade their DP firmware 
(or even be able to), we 'll likely need a test against the firmware 
version. So that the additional display modes are optional within the 
driver. My proposal is to get the firmware version at the end of 
ast_dp_launch() [1]. Then implement drm_encoder_helper_funcs.mode_valid 
[2] to filter out unsupported modes on older firmwares. Does that make 
sense?

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/gpu/drm/ast/ast_dp.c#L148
[2] 
https://elixir.bootlin.com/linux/v6.13-rc3/source/include/drm/drm_modeset_helper_vtables.h#L562

>
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> ---
> v2:
>   - Fix build errors.
> v3:
>   - Fix typo.
> ---
>   drivers/gpu/drm/ast/ast_dp.c     |  9 ++++++++-
>   drivers/gpu/drm/ast/ast_drv.h    |  3 +++
>   drivers/gpu/drm/ast/ast_mode.c   | 14 ++++++++++++++
>   drivers/gpu/drm/ast/ast_tables.h | 18 ++++++++++++++++++
>   4 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 0e282b7b167c..0b56f0335871 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -255,6 +255,10 @@ static void ast_dp_set_mode(struct drm_crtc *crtc, struct ast_vbios_mode_info *v
>   	case 1280:
>   		if (crtc->mode.crtc_vdisplay == 800)
>   			ModeIdx = (ASTDP_1280x800_60_RB - (u8) ulRefreshRateIndex);
> +		else if (crtc->mode.crtc_vdisplay == 720)
> +			ModeIdx = ASTDP_1280x720_60;
> +		else if (crtc->mode.crtc_vdisplay == 960)
> +			ModeIdx = ASTDP_1280x960_60;
>   		else		// 1024
>   			ModeIdx = (ASTDP_1280x1024_60 + (u8) ulRefreshRateIndex);
>   		break;
> @@ -267,7 +271,10 @@ static void ast_dp_set_mode(struct drm_crtc *crtc, struct ast_vbios_mode_info *v
>   		break;
>   	case 1600:
>   		if (crtc->mode.crtc_vdisplay == 900)
> -			ModeIdx = (ASTDP_1600x900_60_RB - (u8) ulRefreshRateIndex);
> +			if (ulRefreshRateIndex == 2)
> +				ModeIdx = ASTDP_1600x900_60_DMT;
> +			else
> +				ModeIdx = (ASTDP_1600x900_60_RB - (u8) ulRefreshRateIndex);
>   		else		//1200
>   			ModeIdx = ASTDP_1600x1200_60;
>   		break;
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 21ce3769bf0d..e7cef334b3ec 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -442,6 +442,9 @@ int ast_mode_config_init(struct ast_device *ast);
>   #define ASTDP_1600x900_60_RB	0x1D
>   #define ASTDP_1366x768_60		0x1E
>   #define ASTDP_1152x864_75		0x1F
> +#define ASTDP_1600x900_60_DMT		0x51
> +#define ASTDP_1280x720_60		0x52
> +#define ASTDP_1280x960_60		0x53
>   
>   int ast_mm_init(struct ast_device *ast);
>   
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 9d5321c81e68..48d1065846a2 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -147,6 +147,10 @@ static bool ast_get_vbios_mode_info(const struct drm_format_info *format,
>   	case 1280:
>   		if (mode->crtc_vdisplay == 800)
>   			vbios_mode->enh_table = &res_1280x800[refresh_rate_index];
> +		else if (mode->crtc_vdisplay == 720)
> +			vbios_mode->enh_table = &res_1280x720[refresh_rate_index];
> +		else if (mode->crtc_vdisplay == 960)
> +			vbios_mode->enh_table = &res_1280x960[refresh_rate_index];
>   		else
>   			vbios_mode->enh_table = &res_1280x1024[refresh_rate_index];
>   		break;
> @@ -475,6 +479,12 @@ static void ast_set_dclk_reg(struct ast_device *ast,
>   	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xbb, 0x0f,
>   			       (clk_info->param3 & 0xc0) |
>   			       ((clk_info->param3 & 0x3) << 4));
> +
> +	/* Set SEQ; Half dclk for this timing */
> +	if (vbios_mode->enh_table->flags & HalfDCLK)
> +		ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xff, 0x08);
> +	else
> +		ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x01, 0xf7, 0x00);
>   }
>   
>   static void ast_set_color_reg(struct ast_device *ast,
> @@ -1027,8 +1037,12 @@ ast_crtc_helper_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode
>   	if (ast->support_wide_screen) {
>   		if ((mode->hdisplay == 1680) && (mode->vdisplay == 1050))
>   			return MODE_OK;
> +		if ((mode->hdisplay == 1280) && (mode->vdisplay == 960))
> +			return MODE_OK;
>   		if ((mode->hdisplay == 1280) && (mode->vdisplay == 800))
>   			return MODE_OK;
> +		if ((mode->hdisplay == 1280) && (mode->vdisplay == 720))
> +			return MODE_OK;
>   		if ((mode->hdisplay == 1440) && (mode->vdisplay == 900))
>   			return MODE_OK;
>   		if ((mode->hdisplay == 1360) && (mode->vdisplay == 768))
> diff --git a/drivers/gpu/drm/ast/ast_tables.h b/drivers/gpu/drm/ast/ast_tables.h
> index 0378c9bc079b..329d6bac867b 100644
> --- a/drivers/gpu/drm/ast/ast_tables.h
> +++ b/drivers/gpu/drm/ast/ast_tables.h
> @@ -254,6 +254,13 @@ static const struct ast_vbios_enhtable res_1024x768[] = {
>   	 (SyncPP | Charx8Dot), 0xFF, 4, 0x31 },
>   };
>   
> +static const struct ast_vbios_enhtable res_1280x960[] = {
> +	{1800, 1280, 96, 112, 1000, 960, 1, 3, VCLK108, /* 60Hz */
> +	 (SyncPP | Charx8Dot), 60, 1, 0x3E },
> +	{1800, 1280, 96, 112, 1000, 960, 1, 3, VCLK108, /* end */
> +	 (SyncPP | Charx8Dot), 0xFF, 1, 0x3E },
> +};
> +
>   static const struct ast_vbios_enhtable res_1280x1024[] = {
>   	{1688, 1280, 48, 112, 1066, 1024, 1, 3, VCLK108,	/* 60Hz */
>   	 (SyncPP | Charx8Dot), 60, 1, 0x32 },
> @@ -280,6 +287,15 @@ static const struct ast_vbios_enhtable res_1152x864[] = {
>   };
>   
>   /* 16:9 */
> +static const struct ast_vbios_enhtable res_1280x720[] = {
> +	{1650, 1280, 110, 40, 750, 720, 5, 5, VCLK148_5,	/* 60Hz */
> +	 (SyncPP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo |
> +	  HalfDCLK), 60, 1, 0x3D },
> +	{1650, 1280, 110, 40, 750, 720, 5, 5, VCLK148_5,	/* end */
> +	 (SyncPP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo |
> +	  HalfDCLK), 0xFF, 1, 0x3D },
> +};
> +
>   static const struct ast_vbios_enhtable res_1360x768[] = {
>   	{1792, 1360, 64, 112, 795, 768, 3, 6, VCLK85_5,		/* 60Hz */
>   	 (SyncPP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 1, 0x39 },
> @@ -294,6 +310,8 @@ static const struct ast_vbios_enhtable res_1600x900[] = {
>   	  AST2500PreCatchCRT), 60, 1, 0x3A },
>   	{2112, 1600, 88, 168, 934, 900, 3, 5, VCLK118_25,	/* 60Hz CVT */
>   	 (SyncPN | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 2, 0x3A },
> +	{1800, 1600, 24, 80, 1000, 900, 1, 3, VCLK108,		/* 60Hz DMT */
> +	 (SyncPP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 3, 0x3A },
>   	{2112, 1600, 88, 168, 934, 900, 3, 5, VCLK118_25,	/* 60Hz CVT */
>   	 (SyncPN | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 0xFF, 2, 0x3A },
>   };
>
> base-commit: 4bbf9020becbfd8fc2c3da790855b7042fad455b

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)