[Qemu-devel] [PATCH] vmware_vga: do not cache depth and bypp

[Qemu-devel] [PATCH] vmware_vga: do not cache depth and bypp

[Alex DAMIAN]

From: Alexandru DAMIAN <alexandru.damian@intel.com>

Do not cache depth and bypp information in the device state.

This resolves a bug where Xorg video-vmare driver refuses
to start up because the depth value read is the one cached from the
device start (default 32 from ui/console.c) and it is not consistent
with the graphical console depth, which may be different from
the default depth.

Signed-off-by: Alexandru DAMIAN <alexandru.damian@intel.com>
---
 hw/vmware_vga.c |   20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 5b9ce8f..75ef404 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -39,8 +39,6 @@ struct vmsvga_state_s {
     VGACommonState vga;
 
     int invalidated;
-    int depth;
-    int bypp;
     int enable;
     int config;
     struct {
@@ -742,10 +740,10 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return SVGA_MAX_HEIGHT;
 
     case SVGA_REG_DEPTH:
-        return s->depth;
+        return surface_bits_per_pixel(surface);
 
     case SVGA_REG_BITS_PER_PIXEL:
-        return (s->depth + 7) & ~7;
+        return (surface_bits_per_pixel(surface) + 7) & ~7;
 
     case SVGA_REG_PSEUDOCOLOR:
         return 0x0;
@@ -760,7 +758,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return surface->pf.bmask;
 
     case SVGA_REG_BYTES_PER_LINE:
-        return s->bypp * s->new_width;
+        return surface_bytes_per_pixel(surface) * s->new_width;
 
     case SVGA_REG_FB_START: {
         struct pci_vmsvga_state_s *pci_vmsvga
@@ -825,7 +823,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return s->cursor.on;
 
     case SVGA_REG_HOST_BITS_PER_PIXEL:
-        return (s->depth + 7) & ~7;
+        return (surface_bits_per_pixel(surface) + 7) & ~7;
 
     case SVGA_REG_SCRATCH_SIZE:
         return s->scratch_size;
@@ -888,7 +886,7 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
         break;
 
     case SVGA_REG_BITS_PER_PIXEL:
-        if (value != s->depth) {
+        if (value != surface_bits_per_pixel(qemu_console_surface(s->vga.con))) {
             printf("%s: Bad bits per pixel: %i bits\n", __func__, value);
             s->config = 0;
         }
@@ -1113,7 +1111,6 @@ static const VMStateDescription vmstate_vmware_vga_internal = {
     .minimum_version_id_old = 0,
     .post_load = vmsvga_post_load,
     .fields      = (VMStateField[]) {
-        VMSTATE_INT32_EQUAL(depth, struct vmsvga_state_s),
         VMSTATE_INT32(enable, struct vmsvga_state_s),
         VMSTATE_INT32(config, struct vmsvga_state_s),
         VMSTATE_INT32(cursor.id, struct vmsvga_state_s),
@@ -1149,8 +1146,6 @@ static const VMStateDescription vmstate_vmware_vga = {
 static void vmsvga_init(struct vmsvga_state_s *s,
                         MemoryRegion *address_space, MemoryRegion *io)
 {
-    DisplaySurface *surface;
-
     s->scratch_size = SVGA_SCRATCH_SIZE;
     s->scratch = g_malloc(s->scratch_size * 4);
 
@@ -1158,7 +1153,6 @@ static void vmsvga_init(struct vmsvga_state_s *s,
                                       vmsvga_invalidate_display,
                                       vmsvga_screen_dump,
                                       vmsvga_text_update, s);
-    surface = qemu_console_surface(s->vga.con);
 
     s->fifo_size = SVGA_FIFO_SIZE;
     memory_region_init_ram(&s->fifo_ram, "vmsvga.fifo", s->fifo_size);
@@ -1168,10 +1162,6 @@ static void vmsvga_init(struct vmsvga_state_s *s,
     vga_common_init(&s->vga);
     vga_init(&s->vga, address_space, io, true);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
-    /* Save some values here in case they are changed later.
-     * This is suspicious and needs more though why it is needed. */
-    s->depth = surface_bits_per_pixel(surface);
-    s->bypp = surface_bytes_per_pixel(surface);
 }
 
 static uint64_t vmsvga_io_read(void *opaque, hwaddr addr, unsigned size)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] vmware_vga: do not cache depth and bypp

[Gerd Hoffmann]

On 03/29/13 17:28, Alex DAMIAN wrote:
> From: Alexandru DAMIAN <alexandru.damian@intel.com>
> 
> Do not cache depth and bypp information in the device state.
> 
> This resolves a bug where Xorg video-vmare driver refuses
> to start up because the depth value read is the one cached from the
> device start (default 32 from ui/console.c) and it is not consistent
> with the graphical console depth, which may be different from
> the default depth.

Does it actually work?  /me posted a simliar patch and according to Jan
(Cc'ed) it doesn't fix the issue.

> @@ -1113,7 +1111,6 @@ static const VMStateDescription vmstate_vmware_vga_internal = {
>      .minimum_version_id_old = 0,
>      .post_load = vmsvga_post_load,
>      .fields      = (VMStateField[]) {
> -        VMSTATE_INT32_EQUAL(depth, struct vmsvga_state_s),

This breaks live migration.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vmware_vga: do not cache depth and bypp

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

It fixes my test case, that is starting up with 32 bits depth console and
then switching to 16 bits depth -
since the depth is cached, xf64-video-vmware gets 565-weight but 32 bit
depth, and refuses to start.

There is another similar bug I'm tackling now - when the window resizes,
qemu_console_resize() creates
a new console with a hardcoded 32-bit depth, instead of current depth,
which breaks the display.

I'm gonna test the live migration too.

Alex



On Wed, Apr 3, 2013 at 12:49 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 03/29/13 17:28, Alex DAMIAN wrote:
> > From: Alexandru DAMIAN <alexandru.damian@intel.com>
> >
> > Do not cache depth and bypp information in the device state.
> >
> > This resolves a bug where Xorg video-vmare driver refuses
> > to start up because the depth value read is the one cached from the
> > device start (default 32 from ui/console.c) and it is not consistent
> > with the graphical console depth, which may be different from
> > the default depth.
>
> Does it actually work?  /me posted a simliar patch and according to Jan
> (Cc'ed) it doesn't fix the issue.
>
> > @@ -1113,7 +1111,6 @@ static const VMStateDescription
> vmstate_vmware_vga_internal = {
> >      .minimum_version_id_old = 0,
> >      .post_load = vmsvga_post_load,
> >      .fields      = (VMStateField[]) {
> > -        VMSTATE_INT32_EQUAL(depth, struct vmsvga_state_s),
>
> This breaks live migration.
>
> cheers,
>   Gerd
>
>
>

[-- Attachment #2: Type: text/html, Size: 2101 bytes --]

Re: [Qemu-devel] [PATCH] vmware_vga: do not cache depth and bypp

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 7902 bytes --]

Something like this, please comment, not ready to submit.


From: Alexandru DAMIAN <alexandru.damian@intel.com>
Date: Wed, 3 Apr 2013 13:43:31 +0300
Subject: [PATCH] qemu console: Add depth preservation on allocation

Since there are multiple bit depths supported for consoles,
we must preserve the depth when creating new consoles for
resizing, etc.

Signed-off-by: Alexandru DAMIAN <alexandru.damian@intel.com>
---
 hw/vga.c             |    6 +++---
 hw/vmware_vga.c      |    2 +-
 include/ui/console.h |    4 ++--
 ui/console.c         |   17 ++++++++++-------
 4 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index f9292ed..204efa0 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1324,7 +1324,7 @@ static void vga_draw_text(VGACommonState *s, int
full_update)
         cw != s->last_cw || cheight != s->last_ch || s->last_depth) {
         s->last_scr_width = width * cw;
         s->last_scr_height = height * cheight;
-        qemu_console_resize(s->con, s->last_scr_width, s->last_scr_height);
+        qemu_console_resize(s->con, s->last_scr_width, s->last_scr_height,
s->last_depth);
         surface = qemu_console_surface(s->con);
         dpy_text_resize(s->con, width, height);
         s->last_depth = 0;
@@ -1677,7 +1677,7 @@ static void vga_draw_graphic(VGACommonState *s, int
full_update)
                     s->vram_ptr + (s->start_addr * 4), byteswap);
             dpy_gfx_replace_surface(s->con, surface);
         } else {
-            qemu_console_resize(s->con, disp_width, height);
+            qemu_console_resize(s->con, disp_width, height, depth);
             surface = qemu_console_surface(s->con);
         }
         s->last_scr_width = disp_width;
@@ -2049,7 +2049,7 @@ static void vga_update_text(void *opaque,
console_ch_t *chardata)
             cw != s->last_cw || cheight != s->last_ch) {
             s->last_scr_width = width * cw;
             s->last_scr_height = height * cheight;
-            qemu_console_resize(s->con, s->last_scr_width,
s->last_scr_height);
+            qemu_console_resize(s->con, s->last_scr_width,
s->last_scr_height, s->last_depth);
             dpy_text_resize(s->con, width, height);
             s->last_depth = 0;
             s->last_width = width;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 75ef404..85696e1 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -985,7 +985,7 @@ static inline void vmsvga_check_size(struct
vmsvga_state_s *s)

     if (s->new_width != surface_width(surface) ||
         s->new_height != surface_height(surface)) {
-        qemu_console_resize(s->vga.con, s->new_width, s->new_height);
+        qemu_console_resize(s->vga.con, s->new_width, s->new_height,
surface_bits_per_pixel(surface));
         s->invalidated = 1;
     }
 }
diff --git a/include/ui/console.h b/include/ui/console.h
index a234c72..868c068 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -201,7 +201,7 @@ DisplaySurface* qemu_create_displaysurface_from(int
width, int height, int bpp,
 PixelFormat qemu_different_endianness_pixelformat(int bpp);
 PixelFormat qemu_default_pixelformat(int bpp);

-DisplaySurface *qemu_create_displaysurface(int width, int height);
+DisplaySurface *qemu_create_displaysurface(int width, int height, int
depth);
 void qemu_free_displaysurface(DisplaySurface *surface);

 static inline int is_surface_bgr(DisplaySurface *surface)
@@ -302,7 +302,7 @@ int is_fixedsize_console(void);
 void text_consoles_set_display(DisplayState *ds);
 void console_select(unsigned int index);
 void console_color_init(DisplayState *ds);
-void qemu_console_resize(QemuConsole *con, int width, int height);
+void qemu_console_resize(QemuConsole *con, int width, int height, int
depth);
 void qemu_console_copy(QemuConsole *con, int src_x, int src_y,
                        int dst_x, int dst_y, int w, int h);
 DisplaySurface *qemu_console_surface(QemuConsole *con);
diff --git a/ui/console.c b/ui/console.c
index e84ba8b..cc35cad 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1059,7 +1059,7 @@ void console_select(unsigned int index)
         }
         active_console = s;
         if (ds->have_gfx) {
-            surface = qemu_create_displaysurface(s->g_width, s->g_height);
+            surface = qemu_create_displaysurface(s->g_width, s->g_height,
32);
             dpy_gfx_replace_surface(s, surface);
         }
         if (ds->have_text) {
@@ -1266,14 +1266,17 @@ static void qemu_alloc_display(DisplaySurface
*surface, int width, int height,
 #endif
 }

-DisplaySurface *qemu_create_displaysurface(int width, int height)
+DisplaySurface *qemu_create_displaysurface(int width, int height, int
depth)
 {
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
     int linesize = width * 4;

+    // when first allocating a display, depth comes through as 0 from
s->last_depth
+    if (depth == 0) depth = 32;
+
     trace_displaysurface_create(surface, width, height);
     qemu_alloc_display(surface, width, height, linesize,
-                       qemu_default_pixelformat(32), 0);
+                       qemu_default_pixelformat(depth), 0);
     return surface;
 }

@@ -1472,7 +1475,7 @@ static void dumb_display_init(void)
         width = active_console->g_width;
         height = active_console->g_height;
     }
-    ds->surface = qemu_create_displaysurface(width, height);
+    ds->surface = qemu_create_displaysurface(width, height, 32);

     register_displaystate(ds);
 }
@@ -1515,7 +1518,7 @@ QemuConsole *graphic_console_init(vga_hw_update_ptr
update,
     s->hw_text_update = text_update;
     s->hw = opaque;

-    ds->surface = qemu_create_displaysurface(640, 480);
+    ds->surface = qemu_create_displaysurface(640, 480, 32);

     register_displaystate(ds);
     return s;
@@ -1668,13 +1671,13 @@ void text_consoles_set_display(DisplayState *ds)
     }
 }

-void qemu_console_resize(QemuConsole *s, int width, int height)
+void qemu_console_resize(QemuConsole *s, int width, int height, int depth)
 {
     s->g_width = width;
     s->g_height = height;
     if (is_graphic_console()) {
         DisplaySurface *surface;
-        surface = qemu_create_displaysurface(width, height);
+        surface = qemu_create_displaysurface(width, height, depth);
         dpy_gfx_replace_surface(s, surface);
     }
 }
-- 
1.7.10.4





On Wed, Apr 3, 2013 at 1:13 PM, Damian, Alexandru <
alexandru.damian@intel.com> wrote:

> It fixes my test case, that is starting up with 32 bits depth console and
> then switching to 16 bits depth -
> since the depth is cached, xf64-video-vmware gets 565-weight but 32 bit
> depth, and refuses to start.
>
> There is another similar bug I'm tackling now - when the window resizes,
> qemu_console_resize() creates
> a new console with a hardcoded 32-bit depth, instead of current depth,
> which breaks the display.
>
> I'm gonna test the live migration too.
>
> Alex
>
>
>
> On Wed, Apr 3, 2013 at 12:49 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>> On 03/29/13 17:28, Alex DAMIAN wrote:
>> > From: Alexandru DAMIAN <alexandru.damian@intel.com>
>> >
>> > Do not cache depth and bypp information in the device state.
>> >
>> > This resolves a bug where Xorg video-vmare driver refuses
>> > to start up because the depth value read is the one cached from the
>> > device start (default 32 from ui/console.c) and it is not consistent
>> > with the graphical console depth, which may be different from
>> > the default depth.
>>
>> Does it actually work?  /me posted a simliar patch and according to Jan
>> (Cc'ed) it doesn't fix the issue.
>>
>> > @@ -1113,7 +1111,6 @@ static const VMStateDescription
>> vmstate_vmware_vga_internal = {
>> >      .minimum_version_id_old = 0,
>> >      .post_load = vmsvga_post_load,
>> >      .fields      = (VMStateField[]) {
>> > -        VMSTATE_INT32_EQUAL(depth, struct vmsvga_state_s),
>>
>> This breaks live migration.
>>
>> cheers,
>>   Gerd
>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 11226 bytes --]

Re: [Qemu-devel] [PATCH] vmware_vga: do not cache depth and bypp

[Gerd Hoffmann]

On 04/03/13 12:13, Damian, Alexandru wrote:
> It fixes my test case, that is starting up with 32 bits depth console and
> then switching to 16 bits depth -
> since the depth is cached, xf64-video-vmware gets 565-weight but 32 bit
> depth, and refuses to start.

Jan's case is booting with vesafb @ 16bpp, then start x (then get
inconsistent values too).

> There is another similar bug I'm tackling now - when the window resizes,
> qemu_console_resize() creates
> a new console with a hardcoded 32-bit depth, instead of current depth,
> which breaks the display.

vmware vga should not use qemu_console_resize in the first place ...

cheers,
  Gerd