[PATCH 07/15] drm/ast: Always validate H/V sync flags

[Jocelyn Falempe]

On 24/01/2025 08:57, Thomas Zimmermann wrote:
> The ast driver matches DRM display modes against an internal list of
> modes supported by the VBIOS. Matching H/V sync flags between modes is
> preferred, but optional. If sync flags are not matching, the driver
> would program the VBIOS settings to hardware and let the display handle
> the difference.
> 
> DRM modes are generated from attached displays or standard mode lines.
> Therefore differences to the VBIOS modes are not just cosmetical, but
> signal possible incompatibility with the display hardware.
> 
> Hence make matching H/V sync flags mandatory. If the VBIOS does not
> support a certain mode, we should report it as unsupported. Note that
> the VBIOS mode tables all appear to refer to standard modes.
> 
> (If sync flags really make no difference to the VBIOS, the ast driver
> shouldn't match them in the first place.)

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe at redhat.com>
-- 

Jocelyn

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 45 ++++++++++++++--------------------
>   1 file changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index fca625518a873..48b46dc3a618e 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -113,8 +113,8 @@ static bool ast_get_vbios_mode_info(const struct drm_format_info *format,
>   {
>   	u32 refresh_rate_index = 0, refresh_rate;
>   	const struct ast_vbios_enhtable *best = NULL;
> +	const struct ast_vbios_enhtable *loop;
>   	u32 hborder, vborder;
> -	bool check_sync;
>   
>   	switch (format->cpp[0] * 8) {
>   	case 8:
> @@ -176,36 +176,27 @@ static bool ast_get_vbios_mode_info(const struct drm_format_info *format,
>   	}
>   
>   	refresh_rate = drm_mode_vrefresh(mode);
> -	check_sync = vbios_mode->enh_table->flags & WideScreenMode;
> -
> -	while (1) {
> -		const struct ast_vbios_enhtable *loop = vbios_mode->enh_table;
> -
> -		while (loop->refresh_rate != 0xff) {
> -			if ((check_sync) &&
> -			    (((mode->flags & DRM_MODE_FLAG_NVSYNC)  &&
> -			      (loop->flags & PVSync))  ||
> -			     ((mode->flags & DRM_MODE_FLAG_PVSYNC)  &&
> -			      (loop->flags & NVSync))  ||
> -			     ((mode->flags & DRM_MODE_FLAG_NHSYNC)  &&
> -			      (loop->flags & PHSync))  ||
> -			     ((mode->flags & DRM_MODE_FLAG_PHSYNC)  &&
> -			      (loop->flags & NHSync)))) {
> -				loop++;
> -				continue;
> -			}
> -			if (loop->refresh_rate <= refresh_rate
> -			    && (!best || loop->refresh_rate > best->refresh_rate))
> -				best = loop;
> +
> +	loop = vbios_mode->enh_table;
> +
> +	while (loop->refresh_rate != 0xff) {
> +		if (((mode->flags & DRM_MODE_FLAG_NVSYNC) && (loop->flags & PVSync))  ||
> +		    ((mode->flags & DRM_MODE_FLAG_PVSYNC) && (loop->flags & NVSync))  ||
> +		    ((mode->flags & DRM_MODE_FLAG_NHSYNC) && (loop->flags & PHSync))  ||
> +		    ((mode->flags & DRM_MODE_FLAG_PHSYNC) && (loop->flags & NHSync))) {
>   			loop++;
> +			continue;
>   		}
> -		if (best || !check_sync)
> -			break;
> -		check_sync = 0;
> +		if (loop->refresh_rate <= refresh_rate &&
> +		    (!best || loop->refresh_rate > best->refresh_rate))
> +			best = loop;
> +		loop++;
>   	}
>   
> -	if (best)
> -		vbios_mode->enh_table = best;
> +	if (!best)
> +		return false;
> +
> +	vbios_mode->enh_table = best;
>   
>   	hborder = (vbios_mode->enh_table->flags & HBorder) ? 8 : 0;
>   	vborder = (vbios_mode->enh_table->flags & VBorder) ? 8 : 0;