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

[Fabio Fantoni <fantonifabio@tiscali.it>]

[-- Attachment #1.1: Type: text/plain, Size: 8937 bytes --]

Il 18/06/2013 12:17, Frediano Ziglio ha scritto:
> Modern notebook support 1366x768 resolution. The resolution width is
> not multiple of 16 causing some problems.
>
> QEMU VGA emulation requires width resolution to be multiple of 8.
>
> VNC implementation requires width resolution to be multiple of 16.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com 
> <mailto:frediano.ziglio@citrix.com>>

Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz>

I tested it for a long time with spice on xen (because qxl will be fully 
working only after adding SSE support on hvm domUs). It works, I think 
it is good to add this and the respective vgabios patch on upstream.

> ---
>  hw/display/vga.c |    2 +-
>  ui/vnc.c         |   34 +++++++++++++++++++---------------
>  2 files changed, 20 insertions(+), 16 deletions(-)
>
> Updates:
> - rebased
> - fixed style problems
> - fixed typos in comment
>
> Attached patch for last vgabios in order to get some new resolutions.
> Still had no time to test deeply.
>
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 21a108d..0053b0f 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -648,7 +648,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 dfc7459..2a2bb90 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -911,26 +911,30 @@ 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])) {
>                      if (last_x == -1) {
>                          last_x = x;
>                      }
>                  } else {
>                      if (last_x != -1) {
> -                        int h = find_and_clear_dirty_height(vs, y, 
> last_x, x,
> - height);
> +                        int h = find_and_clear_dirty_height(vs, y, 
> last_x/16,
> + x/16, height);
>
> -                        n += vnc_job_add_rect(job, last_x * 16, y,
> -                                              (x - last_x) * 16, h);
> +                        n += vnc_job_add_rect(job, last_x, y,
> +                                              (x - last_x), h);
>                      }
>                      last_x = -1;
>                  }
>              }
>              if (last_x != -1) {
> -                int h = find_and_clear_dirty_height(vs, y, last_x, x, 
> height);
> -                n += vnc_job_add_rect(job, last_x * 16, y,
> -                                      (x - last_x) * 16, h);
> +                int h = find_and_clear_dirty_height(vs, y, last_x/16, 
> x/16,
> + height);
> +                if (x > width) {
> +                    x = width;
> +                }
> +                n += vnc_job_add_rect(job, last_x, y,
> +                                      (x - last_x), h);
>              }
>          }
>
> @@ -1861,7 +1865,7 @@ static void framebuffer_update_request(VncState 
> *vs, int incremental,
>                                         int w, int h)
>  {
>      int i;
> -    const size_t width = surface_width(vs->vd->ds) / 16;
> +    const size_t width = (surface_width(vs->vd->ds)+15) / 16;
>      const size_t height = surface_height(vs->vd->ds);
>
>      if (y_position > height) {
> @@ -2686,10 +2690,6 @@ static int 
> vnc_refresh_server_surface(VncDisplay *vd)
>       * Check and copy modified bits from guest to server surface.
>       * Update server dirty map.
>       */
> -    cmp_bytes = 64;
> -    if (cmp_bytes > vnc_server_fb_stride(vd)) {
> -        cmp_bytes = vnc_server_fb_stride(vd);
> -    }
>      if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
>          int width = pixman_image_get_width(vd->server);
>          tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
> @@ -2710,8 +2710,12 @@ static int 
> vnc_refresh_server_surface(VncDisplay *vd)
>              }
>              server_ptr = server_row;
>
> -            for (x = 0; x + 15 < width;
> +            cmp_bytes = 64;
> +            for (x = 0; x < width;
>                      x += 16, guest_ptr += cmp_bytes, server_ptr += 
> cmp_bytes) {
> +                if (width - x < 16) {
> +                    cmp_bytes = 4 * (width - x);
> +                }
>                  if (!test_and_clear_bit((x / 16), vd->guest.dirty[y]))
>                      continue;
>                  if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0)
> -- 
> 1.7.10.4
>
>
>
> 2013/5/10 Andreas Färber <afaerber@suse.de <mailto: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
>     <mailto: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 <http://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
>
>
>
>
> Nessun virus nel messaggio.
> Controllato da AVG - www.avg.com <http://www.avg.com>
> Versione: 2013.0.3345 / Database dei virus: 3199/6420 - Data di 
> rilascio: 18/06/2013
>


[-- Attachment #1.2: Type: text/html, Size: 17146 bytes --]

[-- Attachment #2: Firma crittografica S/MIME --]
[-- Type: application/pkcs7-signature, Size: 4510 bytes --]