[PATCH 3/6] vgabios: vbe 15h func returns constructed EDID

[Hiroshi Miura]

The Bochs VGA ROM now return an information
    about a recent WIDE monitor.

It is constructed from defitions at run time.

Returning an EDID with decent timings may be necessary
    in case we want to use a resolution
    superior to 1024x768 with Xorg.

Signed-off-by: Hiroshi Miura 
---
 vgasrc/bochsvga.c |  202
++++++++++++++++++++++++++++++++---------------------
 vgasrc/vbe_edid.h |  134 +++++++++++++++++++++++++++++++++++
 2 files changed, 255 insertions(+), 81 deletions(-)
 create mode 100644 vgasrc/vbe_edid.h

[Kevin O'Connor]

On Sat, Sep 01, 2012 at 03:11:40PM +0900, Hiroshi Miura wrote:
> The Bochs VGA ROM now return an information
> about a recent WIDE monitor.

Thanks.  See my comments below.

[...]
> { 0x187, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> { 0x188, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> { 0x189, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> { 0x18a, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> { 0x18b, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> { 0x18c, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
> { 0x187, { MM_DIRECT, 1920, 1080, 16, 8, 16, SEG_GRAPH } },
> { 0x188, { MM_DIRECT, 1920, 1080, 24, 8, 16, SEG_GRAPH } },
> { 0x189, { MM_DIRECT, 1920, 1080, 32, 8, 16, SEG_GRAPH } },
> { 0x18a, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> { 0x18b, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> { 0x18c, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> { 0x18d, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> { 0x18e, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> { 0x18f, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },

This appears to be adding three new 1080 height modes.  Unless this
is somehow tied to the edid support, I suggest moving this change to a
separate patch.

[...]
> -/****************************************************************
> EDID
> ****************************************************************/
>
> -u8 bochsvga_edid[128] VAR16 = {
> 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, /* 8-byte header */

This removes a bunch of code added in patch 2.  This makes patch
review harder.  I suggest you merge this patch with patch 2 (and patch
4) to make review easier.

[...]
> --- /dev/null
> +++ b/vgasrc/vbe_edid.h
> @@ -0,0 +1,134 @@
> +#ifndef __VBE_EDID_H
> +#define __VBE_EDID_H
>
> +#define WORDBE(x) ((((x) & 0xff) << 8 ) | (((x) >> 8) & 0xff))
> +#define DWORDBE(x) (((x) & 0xff) << 24) | ((((x) >> 8) & 0xff) << 16 ) | ((((x) >> 16) & 0xff) << 8 ) | ((((x) >> 24) & 0xff))

We don't need more macros that perform byte swabbing.  Use (and/or
enhance) the code in byteorder.h.

-Kevin

[Hiroshi Miura]

Hi,

On 2012年09月03日 06:10, Kevin O'Connor wrote:
> > { 0x187, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> > { 0x188, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> > { 0x189, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> > { 0x18a, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> > { 0x18b, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> > { 0x18c, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
> > { 0x187, { MM_DIRECT, 1920, 1080, 16, 8, 16, SEG_GRAPH } },
> > { 0x188, { MM_DIRECT, 1920, 1080, 24, 8, 16, SEG_GRAPH } },
> > { 0x189, { MM_DIRECT, 1920, 1080, 32, 8, 16, SEG_GRAPH } },
> > { 0x18a, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> > { 0x18b, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> > { 0x18c, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> > { 0x18d, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> > { 0x18e, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> > { 0x18f, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
>
> This appears to be adding three new 1080 height modes.  Unless this
> is somehow tied to the edid support, I suggest moving this change to a
> separate patch.

This is little tied with edid staff, because of preferred mode is 1080 height.
That is caused by I have a successful data only of
1920x1080 and 1600x1200 for high resolution.

Without addition of these staff, guest OS cannot use 1080 height as mode
then result would be unpredictable.
> [...]
> > -/****************************************************************
> > EDID
> > ****************************************************************/
> >
> > -u8 bochsvga_edid[128] VAR16 = {
> > 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, /* 8-byte header */
>
> This removes a bunch of code added in patch 2.  This makes patch
> review harder.  I suggest you merge this patch with patch 2 (and patch
> 4) to make review easier.

I see, I rework it.
> [...]
> > --- /dev/null
> > +++ b/vgasrc/vbe_edid.h
> > @@ -0,0 +1,134 @@
> > +#ifndef __VBE_EDID_H
> > +#define __VBE_EDID_H
> >
> > +#define WORDBE(x) ((((x) & 0xff) << 8 ) | (((x) >> 8) & 0xff))
> > +#define DWORDBE(x) (((x) & 0xff) << 24) | ((((x) >> 8) & 0xff) << 16 ) | ((((x) >> 16) & 0xff) << 8 ) | ((((x) >> 24) & 0xff))
>
> We don't need more macros that perform byte swabbing.  Use (and/or
> enhance) the code in byteorder.h.

Ok.I'd like to use byteorder.h

Hiroshi

[Hiroshi Miura]

Hi,

On 2012年09月03日 06:10, Kevin O'Connor wrote:
> > { 0x187, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> > { 0x188, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> > { 0x189, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> > { 0x18a, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> > { 0x18b, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> > { 0x18c, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
> > { 0x187, { MM_DIRECT, 1920, 1080, 16, 8, 16, SEG_GRAPH } },
> > { 0x188, { MM_DIRECT, 1920, 1080, 24, 8, 16, SEG_GRAPH } },
> > { 0x189, { MM_DIRECT, 1920, 1080, 32, 8, 16, SEG_GRAPH } },
> > { 0x18a, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> > { 0x18b, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> > { 0x18c, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> > { 0x18d, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> > { 0x18e, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> > { 0x18f, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
>
> This appears to be adding three new 1080 height modes.  Unless this
> is somehow tied to the edid support, I suggest moving this change to a
> separate patch.

This is little tied with edid staff, because of preferred mode is 1080 height.
That is caused by I have a successful data only of
1920x1080 and 1600x1200 for high resolution.

Without addition of these staff, guest OS cannot use 1080 height as mode
then result would be unpredictable.

But now I'd like to add EDID for 1920x1200.
> [...]
> > -/****************************************************************
> > EDID
> > ****************************************************************/
> >
> > -u8 bochsvga_edid[128] VAR16 = {
> > 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, /* 8-byte header */
>
> This removes a bunch of code added in patch 2.  This makes patch
> review harder.  I suggest you merge this patch with patch 2 (and patch
> 4) to make review easier.

I see, I rework it.
> [...]
> > --- /dev/null
> > +++ b/vgasrc/vbe_edid.h
> > @@ -0,0 +1,134 @@
> > +#ifndef __VBE_EDID_H
> > +#define __VBE_EDID_H
> >
> > +#define WORDBE(x) ((((x) & 0xff) << 8 ) | (((x) >> 8) & 0xff))
> > +#define DWORDBE(x) (((x) & 0xff) << 24) | ((((x) >> 8) & 0xff) << 16 ) | ((((x) >> 16) & 0xff) << 8 ) | ((((x) >> 24) & 0xff))
>
> We don't need more macros that perform byte swabbing.  Use (and/or
> enhance) the code in byteorder.h.

Ok, I see.

Hiroshi

[Hiroshi Miura]

On 2012年09月08日 23:38, Hiroshi Miura wrote:
> On 2012年09月03日 06:10, Kevin O'Connor wrote:
> > > { 0x187, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> > > { 0x188, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> > > { 0x189, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> > > { 0x18a, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> > > { 0x18b, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> > > { 0x18c, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
> > > { 0x187, { MM_DIRECT, 1920, 1080, 16, 8, 16, SEG_GRAPH } },
> > > { 0x188, { MM_DIRECT, 1920, 1080, 24, 8, 16, SEG_GRAPH } },
> > > { 0x189, { MM_DIRECT, 1920, 1080, 32, 8, 16, SEG_GRAPH } },
> > > { 0x18a, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
> > > { 0x18b, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
> > > { 0x18c, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
> > > { 0x18d, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
> > > { 0x18e, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
> > > { 0x18f, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
> >
> > This appears to be adding three new 1080 height modes.  Unless this
> > is somehow tied to the edid support, I suggest moving this change to a
> > separate patch.
>
> But now I'd like to add EDID for 1920x1200.

A patch I resend ([PATCH 2/3] bochsvga: Implement vbe 15h function) now provide
EDID for 1920x1200 resolution that make this part of modification is not needed.

Thank you for your suggestion.

Hiroshi