Re: [Qemu-devel] [RFC PATCH] vga: Start supporting resolution not multiple of 16 correctly.

["Andreas Färber" <afaerber@suse.de>]

Am 15.03.2013 19:14, schrieb Frediano Ziglio:
> Modern notebook support 136x768 resolution. The resolution width is

1366?

> not multiple of 16 causing some problems.

"a multiple"? (me not a native English speaker)

> 
> Qemu VGA emulation require width resolution to be multiple of 8.

"QEMU"

> 
> VNC implementation require width resolution to be multiple of 16.

"requires" or "implementations"

> 
> This patch remove these limits. Was tested with a Windows machine with
> standard vga and 1366x768 as resolution. I had to update vgabios as
> version in qemu (pc-bios/vgabios-stdvga.bin) is quite old. I also had
> to add some patches on top of VGABIOS 0.7a to add some new
> resolutions.
> 
> I have some doubt about this patch
> - are other UI (sdl, cocoa, qxl) happy if resolution is not multiple of 16 ?

SDL and Gtk+ should be easily testable; if you CC Peter Maydell or me we
can try to test Cocoa. CC'ing QXL guys.

> - scanline is computed exactly without any alignment (so 1366 8 bit is
> 1366 bytes) while getting vesa information from a laptop it seems to
> use some kind of alignment (if became 0x580 which is 1408 bytes).
> Perhaps should I change either VGABIOS and Qemu to make this
> alignment?

Concerns and personal comments are better placed below ---. :)

> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> 
> ---
>  hw/vga.c |    2 +-

File has moved to hw/display/.

>  ui/vnc.c |   27 +++++++++++++--------------
>  2 files changed, 14 insertions(+), 15 deletions(-)

I don't see VGABIOS being updated here despite being mentioned above? Is
that done in a different patch? Still needed?

> diff --git a/hw/vga.c b/hw/vga.c
> index 1caf23d..d229f06 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -651,7 +651,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t
> addr, uint32_t val)
>              }
>              break;
>          case VBE_DISPI_INDEX_XRES:
> -            if ((val <= VBE_DISPI_MAX_XRES) && ((val & 7) == 0)) {
> +            if ((val <= VBE_DISPI_MAX_XRES) && ((val & 1) == 0)) {
>                  s->vbe_regs[s->vbe_index] = val;
>              }
>              break;
> diff --git a/ui/vnc.c b/ui/vnc.c
> index ff4e2ae..328d14d 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -907,26 +907,27 @@ static int vnc_update_client(VncState *vs, int has_dirty)
>          for (y = 0; y < height; y++) {
>              int x;
>              int last_x = -1;
> -            for (x = 0; x < width / 16; x++) {
> -                if (test_and_clear_bit(x, vs->dirty[y])) {
> +            for (x = 0; x < width; x += 16) {
> +                if (test_and_clear_bit(x/16, vs->dirty[y])) {
[snip]

Please check if scripts/checkpatch.pl complains about missing spaces
around operators.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg