Re: [SeaBIOS] [PATCH 5/6] vgabios: generate EDID of preferred resolution

[Kevin O'Connor]

On Sat, Sep 01, 2012 at 03:09:21PM +0900, Hiroshi Miura wrote:
> Add interface to EDID part.
> Now bios initialize code can register preferred mode
> from information of host OS.

I don't really understanding what this patch does.  I think it needs
some cleanup or further clarification.

[...]
> +int vesa_set_prefered_mode(u16 x, u16 y) {
> SET_VGA(prefered_mode.x, x);
> SET_VGA(prefered_mode.y, y);
> return 0;
>
> +}

There's no reason to build a "struct prefered_mode_info" and define an
init function - one can just delcare two global variables - for
example: u16 PreferedXres VAR16, PreferedYres VAR16;
> +int vesa_get_prefered_mode(u16 seg, void *data) {
> struct prefered_mode_info *info = data;
> int a, b;
> a = GET_GLOBAL(prefered_mode.x);
> b = GET_GLOBAL(prefered_mode.y);
> SET_FARVAR(seg, info->x, a);
> SET_FARVAR(seg, info->y, b);
> return 0;
>
> +}

Dead code.
> +static void get_prefered_resolution(struct edid_detailed_timing *dtd, u16 *x, u16 *y) {
> u16 maxx, maxy;
> maxx = GET_GLOBAL(prefered_mode.x);
> maxy = GET_GLOBAL(prefered_mode.y);
> if (maxx == 0 && maxy == 0) {/* unknown, no-preference */
>    dtd = &vbe_edid_dtd_1920x1080;
>    maxx = 1920; maxy = 1080;
> } else if (maxx >= 1920 && (maxy >= 1080 || maxy == 0)){
>    dtd = &vbe_edid_dtd_1920x1080;
>    maxx = 1920; maxy = 1080;
> } else if (maxx >= 1600 && (maxy >= 1200 || maxy == 0)){
>    dtd = &vbe_edid_dtd_1600x1200;
>    maxx = 1600; maxy = 1200;
> } else {
>    dtd = &vbe_edid_dtd_1280x1024;
>    maxx = 1280; maxy = 1024;
> }
> *x = maxx; *y = maxy;
>
> +}

It's preferable to do this type of scanning at init time and store the
results somewhere in memory.

[...]
>  SET_FARVAR(seg, info->standard_timing[0], VBE_EDID_STD_640x480_85Hz);
>  SET_FARVAR(seg, info->standard_timing[1], VBE_EDID_STD_800x600_85Hz);
> SET_FARVAR(seg, info->standard_timing[2], VBE_EDID_STD_1024x768_85Hz);
> SET_FARVAR(seg, info->standard_timing[3], VBE_EDID_STD_1280x720_70Hz);
> SET_FARVAR(seg, info->standard_timing[4], VBE_EDID_STD_1280x960_60Hz);
> SET_FARVAR(seg, info->standard_timing[5], VBE_EDID_STD_1440x900_60Hz);
> SET_FARVAR(seg, info->standard_timing[6], VBE_EDID_STD_1600x1200_60Hz);
> SET_FARVAR(seg, info->standard_timing[7], VBE_EDID_STD_1680x1050_60Hz);
> /* detailed timing blocks */
> memcpy_far(seg, &(info->desc[0].dtd), get_global_seg(), &vbe_edid_dtd_1152x864,
>           sizeof (vbe_edid_dtd_1152x864));
> memcpy_far(seg, &(info->desc[1].dtd), get_global_seg(), &vbe_edid_dtd_1280x1024,
>           sizeof (vbe_edid_dtd_1280x1024));
> j=2;
> if (max_x >= 1152 && max_x < 1024) {
>    SET_FARVAR(seg, info->standard_timing[2], VBE_EDID_STD_1152x864_70Hz);
>    j=3;
> } else if (max_x >= 1024 && max_x < 1280) {
>    SET_FARVAR(seg, info->standard_timing[2], VBE_EDID_STD_1152x864_70Hz);
>    SET_FARVAR(seg, info->standard_timing[3], VBE_EDID_STD_1024x768_85Hz);
>    j=4;
> } else if (max_x >= 1280 && max_x < 1600) {
>    SET_FARVAR(seg, info->standard_timing[2], VBE_EDID_STD_1024x768_85Hz);
>    SET_FARVAR(seg, info->standard_timing[3], VBE_EDID_STD_1280x1024_60Hz);
>    j=4;
> } else if (max_x >= 1600 && max_x < 1920) {
>    SET_FARVAR(seg, info->standard_timing[3], VBE_EDID_STD_1440x900_60Hz);
>    SET_FARVAR(seg, info->standard_timing[4], VBE_EDID_STD_1600x900_60Hz);
>    j=5;

Looks odd - nothing sets info->standard_timing[2].

-Kevin