Re: [PATCH] Fixed 1600x900 Display Issue

[Egbert Eich <eich@freedesktop.org>]

On Tue, Aug 19, 2014 at 05:10:51PM +0800, Y.C. Chen wrote:
> From: "Y.C. Chen" <yc_chen@aspeedtech.com>
> 
> ---
>  drivers/gpu/drm/ast/ast_mode.c   | 27 +++++++++++++++++---------
>  drivers/gpu/drm/ast/ast_tables.h | 42 ++++++++++++++++++++++------------------
>  2 files changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 5389350..e7fed29 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -140,15 +140,24 @@ static bool ast_get_vbios_mode_info(struct drm_crtc *crtc, struct drm_display_mo
>  		return false;
>  	}
>  
> -	refresh_rate = drm_mode_vrefresh(mode);
> -	while (vbios_mode->enh_table->refresh_rate < refresh_rate) {
> -		vbios_mode->enh_table++;
> -		if ((vbios_mode->enh_table->refresh_rate > refresh_rate) ||
> -		    (vbios_mode->enh_table->refresh_rate == 0xff)) {
> -			vbios_mode->enh_table--;
> -			break;
> -		}
> -	}
> +    if (vbios_mode->enh_table->flags & WideScreenMode)
> +    {
> +        /* parsing for wide screen reduced blank mode */
> +        if ((mode->flags & DRM_MODE_FLAG_NVSYNC) && (mode->flags & DRM_MODE_FLAG_PHSYNC))	/* CVT RB */
> +		    vbios_mode->enh_table++;

I'm not really happy with assuming fixed list positions. Wouldn't it be better to walk the 
list until you reach an entry with:
   (vbios_mode->enh_table->flags & SyncNP) && (vbios_mode->enh_table->refresh_rate != 0xff)?
This way it would be independent of the position of the modes in the list. Assuming fixed list
postition seems to somewhat defy the sophisticated flag concept used here.

> +    }
> +    else
> +    {
> +	    refresh_rate = drm_mode_vrefresh(mode);
> +	    while (vbios_mode->enh_table->refresh_rate < refresh_rate) {
> +		    vbios_mode->enh_table++;
> +		    if ((vbios_mode->enh_table->refresh_rate > refresh_rate) ||
> +		        (vbios_mode->enh_table->refresh_rate == 0xff)) {
> +			    vbios_mode->enh_table--;
> +			    break;
> +		    }
> +	    }
> +    }

[..]

>  static struct ast_vbios_enhtable res_1600x900[] = {
> +    {2112, 1600, 88,168, 934,  900, 3, 5, VCLK118_25,	/* 60Hz CVT */
> +     (SyncPN | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 1, 0x3A },
>  	{1760, 1600, 48, 32, 926,  900, 3, 5, VCLK97_75,	/* 60Hz CVT RB */
>  	 (SyncNP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 1, 0x3A },
>  	{1760, 1600, 48, 32, 926,  900, 3, 5, VCLK97_75,	/* end */
> @@ -259,31 +264,30 @@ static struct ast_vbios_enhtable res_1920x1080[] = {
>  
>  /* 16:10 */
>  static struct ast_vbios_enhtable res_1280x800[] = {
> -	{1440, 1280, 48, 32,  823,  800, 3, 6, VCLK71,	/* 60Hz RB */
> -	 (SyncNP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 1, 35 },
> -	{1680, 1280, 72,128,  831,  800, 3, 6, VCLK83_5,	/* 60Hz */
> -	 (SyncPN | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 1, 0x35 },
> -	{1680, 1280, 72,128,  831,  800, 3, 6, VCLK83_5,	/* 60Hz */
> -	 (SyncPN | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 0xFF, 1, 0x35 },
> -
> +    {1680, 1280, 72,128,  831,  800, 3, 6, VCLK83_5,	/* 60Hz CVT */
> +     (SyncPN | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 1, 0x35 },
> +    {1440, 1280, 48, 32,  823,  800, 3, 6, VCLK71,		/* 60Hz CVT RB */
> +      (SyncNP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 60, 2, 35 },
> +    {1440, 1280, 48, 32,  823,  800, 3, 6, VCLK71,		/* 60Hz CVT RB */
> +     (SyncNP | Charx8Dot | LineCompareOff | WideScreenMode | NewModeInfo), 0xFF, 2, 35 },
>  };

       The last entry marks the end of the list by setting refresh rate to 0xff.
       Would be nice if all other entries in this list element were obviously invalid
       as well to enhance readability and clearly mark that there is no magic involved
       here.
       But you didn't introduce this, it was done this way before already.


Cheers,
	Egbert.