Patchwork [12/16] vmware_vga: remove !EMBED_STDVGA code

login
register
mail settings
Submitter Juan Quintela
Date Oct. 14, 2009, 5:35 p.m.
Message ID <aa493ca196bc4ae428fb1781558def4aab7757a3.1255541443.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/36014/
State New
Headers show

Comments

Juan Quintela - Oct. 14, 2009, 5:35 p.m.
It don't compile.  And the trivial fixes (change vga.foo field to foo field
don't work either.  No output

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/vmware_vga.c |   25 -------------------------
 1 files changed, 0 insertions(+), 25 deletions(-)
andrzej zaborowski - Oct. 14, 2009, 6:15 p.m.
2009/10/14 Juan Quintela <quintela@redhat.com>:
> It don't compile.  And the trivial fixes (change vga.foo field to foo field
> don't work either.  No output

Just a note that there's supposed to be no output because your guest
OS (and BIOS) expect the standard VGA functionality to be there.  The
define is only useful for debugging vmware_vga or OS driver.  It's ok
to drop it if you want.

Cheers
Juan Quintela - Oct. 14, 2009, 7:28 p.m.
andrzej zaborowski <balrogg@gmail.com> wrote:
> 2009/10/14 Juan Quintela <quintela@redhat.com>:
>> It don't compile.  And the trivial fixes (change vga.foo field to foo field
>> don't work either.  No output
>
> Just a note that there's supposed to be no output because your guest
> OS (and BIOS) expect the standard VGA functionality to be there.  The
> define is only useful for debugging vmware_vga or OS driver.  It's ok
> to drop it if you want.

I preffer to drop it because it has no chance of working (suspend/resume
code is not there for instance).

Once that we are there.  I did a fast try at enabling DIRECT_VRAM with
the same not working result.  Any idea if it would make things
better/fast/... whatever?

The reason that I ask is because the driver is smaller than cirrus_vga,
and it supports already lots of resolutions.

Later, Juan.
andrzej zaborowski - Oct. 14, 2009, 7:42 p.m.
2009/10/14 Juan Quintela <quintela@redhat.com>:
> andrzej zaborowski <balrogg@gmail.com> wrote:
>> 2009/10/14 Juan Quintela <quintela@redhat.com>:
>>> It don't compile.  And the trivial fixes (change vga.foo field to foo field
>>> don't work either.  No output
>>
>> Just a note that there's supposed to be no output because your guest
>> OS (and BIOS) expect the standard VGA functionality to be there.  The
>> define is only useful for debugging vmware_vga or OS driver.  It's ok
>> to drop it if you want.
>
> I preffer to drop it because it has no chance of working (suspend/resume
> code is not there for instance).
>
> Once that we are there.  I did a fast try at enabling DIRECT_VRAM with
> the same not working result.  Any idea if it would make things
> better/fast/... whatever?

Currently it probably makes things slower.  I have not digged through
the newer SDL code deep enough, to tell if it's possible to create a
SDL surface directly from guest RAM provided it's contiguously mapped
on host -- this is what VMware does and it's one of the expected
benefits from using vmware_vga that isn't there, and I think using DGA
saves them another unneeded copy.  This would need reimplementing the
pieces guarded by #ifdef DIRECT_VRAM.

Cheers
Juan Quintela - Oct. 14, 2009, 8:04 p.m.
andrzej zaborowski <balrogg@gmail.com> wrote:
> 2009/10/14 Juan Quintela <quintela@redhat.com>:
>> andrzej zaborowski <balrogg@gmail.com> wrote:
>>> 2009/10/14 Juan Quintela <quintela@redhat.com>:
>>>> It don't compile.  And the trivial fixes (change vga.foo field to foo field
>>>> don't work either.  No output
>>>
>>> Just a note that there's supposed to be no output because your guest
>>> OS (and BIOS) expect the standard VGA functionality to be there.  The
>>> define is only useful for debugging vmware_vga or OS driver.  It's ok
>>> to drop it if you want.
>>
>> I preffer to drop it because it has no chance of working (suspend/resume
>> code is not there for instance).
>>
>> Once that we are there.  I did a fast try at enabling DIRECT_VRAM with
>> the same not working result.  Any idea if it would make things
>> better/fast/... whatever?
>
> Currently it probably makes things slower.  I have not digged through
> the newer SDL code deep enough, to tell if it's possible to create a
> SDL surface directly from guest RAM provided it's contiguously mapped
> on host -- this is what VMware does and it's one of the expected
> benefits from using vmware_vga that isn't there, and I think using DGA
> saves them another unneeded copy.  This would need reimplementing the
> pieces guarded by #ifdef DIRECT_VRAM.

Without DIRECT_VRAM, my un-scientific test (run glxgears) show that
cirrus_vga is a bit faster (207 vs 190 fps), neither the test of the
results something to write home about.

I have another problem with the driver: depth.

I have to change s->depth to 32 in vmsvga_reset() to make it work
correctly on my setup (default qemu.git tree, i.e. nothing fancy),
running with sdl.  Any clue here?

Anthony thinks that the problem happens at the memcpy() in
vmsvga_update_rect(), but I haven't had the time to look at how to fix
it.  Any idea here?

Later, Juan.
andrzej zaborowski - Oct. 21, 2009, 2:23 a.m.
Hi,
sorry for replying late,

2009/10/14 Juan Quintela <quintela@redhat.com>:
> I have another problem with the driver: depth.
>
> I have to change s->depth to 32 in vmsvga_reset() to make it work
> correctly on my setup (default qemu.git tree, i.e. nothing fancy),
> running with sdl.  Any clue here?
>
> Anthony thinks that the problem happens at the memcpy() in
> vmsvga_update_rect(), but I haven't had the time to look at how to fix
> it.  Any idea here?

There's a thread about this issue at
http://lists.gnu.org/archive/html/qemu-devel/2009-08/msg00835.html

Cheers

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 1fb04bf..48f3a06 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -26,20 +26,15 @@ 
 #include "pci.h"

 #define VERBOSE
-#define EMBED_STDVGA
 #undef DIRECT_VRAM
 #define HW_RECT_ACCEL
 #define HW_FILL_ACCEL
 #define HW_MOUSE_ACCEL

-#ifdef EMBED_STDVGA
 # include "vga_int.h"
-#endif

 struct vmsvga_state_s {
-#ifdef EMBED_STDVGA
     VGACommonState vga;
-#endif

     int width;
     int height;
@@ -55,12 +50,6 @@  struct vmsvga_state_s {
         int on;
     } cursor;

-#ifndef EMBED_STDVGA
-    DisplayState *ds;
-    int vram_size;
-    ram_addr_t vram_offset;
-    uint8_t *vram_ptr;
-#endif
     target_phys_addr_t vram_base;

     int index;
@@ -774,9 +763,7 @@  static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
         s->width = -1;
         s->height = -1;
         s->invalidated = 1;
-#ifdef EMBED_STDVGA
         s->vga.invalidate(&s->vga);
-#endif
         if (s->enable)
             s->fb_size = ((s->depth + 7) >> 3) * s->new_width * s->new_height;
         break;
@@ -894,9 +881,7 @@  static void vmsvga_update_display(void *opaque)
 {
     struct vmsvga_state_s *s = opaque;
     if (!s->enable) {
-#ifdef EMBED_STDVGA
         s->vga.update(&s->vga);
-#endif
         return;
     }

@@ -962,9 +947,7 @@  static void vmsvga_invalidate_display(void *opaque)
 {
     struct vmsvga_state_s *s = opaque;
     if (!s->enable) {
-#ifdef EMBED_STDVGA
         s->vga.invalidate(&s->vga);
-#endif
         return;
     }

@@ -977,9 +960,7 @@  static void vmsvga_screen_dump(void *opaque, const char *filename)
 {
     struct vmsvga_state_s *s = opaque;
     if (!s->enable) {
-#ifdef EMBED_STDVGA
         s->vga.screen_dump(&s->vga, filename);
-#endif
         return;
     }

@@ -1128,15 +1109,9 @@  static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)

     vmsvga_reset(s);

-#ifdef EMBED_STDVGA
     vga_common_init(&s->vga, vga_ram_size);
     vga_init(&s->vga);
     vmstate_register(0, &vmstate_vga_common, &s->vga);
-#else
-    s->vram_size = vga_ram_size;
-    s->vram_offset = qemu_ram_alloc(vga_ram_size);
-    s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
-#endif

     s->vga.ds = graphic_console_init(vmsvga_update_display,
                                      vmsvga_invalidate_display,