[Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c

[Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c

[Reimar Döffinger]

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

Hello,
for what I can tell, there is no way for vmware_vga to work correctly
right now. It assumes that the framebuffer bits-per-pixel and the one
from the DisplaySurface are identical (it uses directly the VRAM from
vga.c), but it always assumes 3 bytes per pixel, which is never possible
with the current version of DisplaySurface.
Attached patch fixes that by using ds_get_bits_per_pixel.
Note that this further breaks the already broken compilation if you use
#undef EMBED_STDVGA (maybe it is time to throw away all that broken
code??).

[-- Attachment #2: vmware_depth.diff --]
[-- Type: text/plain, Size: 1131 bytes --]

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 5ceebf1..23d5fc8 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -923,7 +927,7 @@ static void vmsvga_reset(struct vmsvga_state_s *s)
     s->width = -1;
     s->height = -1;
     s->svgaid = SVGA_ID;
-    s->depth = 24;
+    s->depth = ds_get_bits_per_pixel(s->vga.ds);
     s->bypp = (s->depth + 7) >> 3;
     s->cursor.on = 0;
     s->redraw_fifo_first = 0;
@@ -1126,8 +1130,6 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
     s->scratch_size = SVGA_SCRATCH_SIZE;
     s->scratch = (uint32_t *) qemu_malloc(s->scratch_size * 4);
 
-    vmsvga_reset(s);
-
 #ifdef EMBED_STDVGA
     vga_common_init((VGAState *) s, vga_ram_size);
     vga_init((VGAState *) s);
@@ -1142,6 +1144,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
                                      vmsvga_screen_dump,
                                      vmsvga_text_update, &s->vga);
 
+    vmsvga_reset(s);
+
 #ifdef CONFIG_BOCHS_VBE
     /* XXX: use optimized standard vga accesses */
     cpu_register_physical_memory(VBE_DISPI_LFB_PHYSICAL_ADDRESS,

Re: [Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c

[andrzej zaborowski]

2009/8/17 Reimar Döffinger <Reimar.Doeffinger@gmx.de>:
> Hello,
> for what I can tell, there is no way for vmware_vga to work correctly
> right now. It assumes that the framebuffer bits-per-pixel and the one
> from the DisplaySurface are identical (it uses directly the VRAM from
> vga.c), but it always assumes 3 bytes per pixel, which is never possible
> with the current version of DisplaySurface.
> Attached patch fixes that by using ds_get_bits_per_pixel.

It was discussed at some point earlier that at the time this code runs
SDL is not initialised and the depth returned is an arbitrary value
from default allocator.  What vmware_vga really should do is ask SDL
for the host's depth and set the surface's pixelformat to that.
Unfortunately the ability to know host's pixel depth was dropped
during video API conversion and afaik hasn't been added till now.  The
also arbitrary value of 24 bits was stuffed there for some reason but
it seems to work for me.

Cheers

Re: [Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c

[Reimar Döffinger]

On Sun, Aug 23, 2009 at 07:25:32PM +0200, andrzej zaborowski wrote:
> 2009/8/17 Reimar Döffinger <Reimar.Doeffinger@gmx.de>:
> > Hello,
> > for what I can tell, there is no way for vmware_vga to work correctly
> > right now. It assumes that the framebuffer bits-per-pixel and the one
> > from the DisplaySurface are identical (it uses directly the VRAM from
> > vga.c), but it always assumes 3 bytes per pixel, which is never possible
> > with the current version of DisplaySurface.
> > Attached patch fixes that by using ds_get_bits_per_pixel.
> 
> It was discussed at some point earlier that at the time this code runs
> SDL is not initialised and the depth returned is an arbitrary value
> from default allocator.

It is not arbitrary. It matches exactly the DisplaySurface's
linesize and data buffer size.
As such I claim that my patch is correct, it may uncover some
bugs that were very carefully swept under the rug, but that only
makes it incomplete.
Also a reference to either the previous discussion or at least a proper
"bug report" about what/how/where it breaks with my patch applied would
be very helpful, e.g. which operating system/hardware driver/SDL version
(I guess there is some reason why I get a different bit depth).
As such, I want to add that the revert commit message of "was
incorrect." doesn't qualify as useful to me.

> What vmware_vga really should do is ask SDL
> for the host's depth and set the surface's pixelformat to that.

Obvious question: why shouldn't SDL ask the VGA for its depth and try
to use a surface with that format? Has the advantage that the depth
of the emulated stuff stays the same, whereas with your suggestion
if I tried a loadvm from a savevm of your machine qemu would get
in a bit in trouble.
(Though looking at sdl_setdata/sdl_update some conversion should
be done anyway, though that requires that the values in the
DisplaySurface and in the vmware_vga depth variable match, which
at least at some points in time they currently don't).

> Unfortunately the ability to know host's pixel depth was dropped
> during video API conversion and afaik hasn't been added till now.

Considering the above, I think that might count as an "accidentally
good" decision.

Greetings,
Reimar Döffinger

Re: [Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c

[andrzej zaborowski]

2009/8/24 Reimar Döffinger <Reimar.Doeffinger@gmx.de>:
> On Sun, Aug 23, 2009 at 07:25:32PM +0200, andrzej zaborowski wrote:
>> It was discussed at some point earlier that at the time this code runs
>> SDL is not initialised and the depth returned is an arbitrary value
>> from default allocator.
>
> It is not arbitrary. It matches exactly the DisplaySurface's
> linesize and data buffer size.

Only at the moment the function is called.  The value is still
hardcoded, just elsewhere.  Once display backend initialises this
value may be invalid.

> As such, I want to add that the revert commit message of "was
> incorrect." doesn't qualify as useful to me.

I wasn't intending to push this commit, instead I responded to the
thread but later noticed I had pushed it.

>
>> What vmware_vga really should do is ask SDL
>> for the host's depth and set the surface's pixelformat to that.
>
> Obvious question: why shouldn't SDL ask the VGA for its depth and try
> to use a surface with that format?

It should, the VGA should create the surface using
qemu_create_displaysurface... like in vga.c.  But, this depth is not
set by the guest, it should match the host's depth because this is how
the vmware's "specification" (if you can call it that) defines it.

Beside that it's an obvious performance gain.  The API change did not
magically remove the pixel by pixel conversion of the colour space, it
just hid it in SDL, under more indirection.

> Has the advantage that the depth
> of the emulated stuff stays the same, whereas with your suggestion
> if I tried a loadvm from a savevm of your machine qemu would get
> in a bit in trouble.

I'm not sure I understand this sentence, but apparently there's some
way vmware can communicate to the guest that the bit depth has
changed.  This is not implemented in vmware_vga.c yet.

Similarly when the window is resized, instead of zooming we could
communicate the resolution change to the guest, but it's not
implemented yet.

Cheers

Re: [Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c

[Reimar Döffinger]

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

On Tue, Aug 25, 2009 at 01:45:15AM +0200, andrzej zaborowski wrote:
> 2009/8/24 Reimar Döffinger <Reimar.Doeffinger@gmx.de>:
> > On Sun, Aug 23, 2009 at 07:25:32PM +0200, andrzej zaborowski wrote:
> >> It was discussed at some point earlier that at the time this code runs
> >> SDL is not initialised and the depth returned is an arbitrary value
> >> from default allocator.
> >
> > It is not arbitrary. It matches exactly the DisplaySurface's
> > linesize and data buffer size.
> 
> Only at the moment the function is called.  The value is still
> hardcoded, just elsewhere.  Once display backend initialises this
> value may be invalid.

Ok, I looked at the code again and it seems that the issue is that
we have to expect that the depth of the DisplayState/surface may change
any time.
I'm not sure that really was intentional and it just can't really work
in general, but vmware_vga can handle it just as well as vga by just
not storing/caching the depth and anything that depends on it.
So attached is a patch that does that.
Obviously it has to assume that the depth is not changed "under its
feet" after the guest OS has initialized a graphics mode but otherwise
it will not care.
I also have not tested it on hardware/software where the current code
works, since I don't know in which cases that would be.
And lastly, I suspect it breaks DIRECT_VRAM even more - given that
(AFAICT) it does not compile even now I'd suggest just removing that
code...

> > As such, I want to add that the revert commit message of "was
> > incorrect." doesn't qualify as useful to me.
> 
> I wasn't intending to push this commit, instead I responded to the
> thread but later noticed I had pushed it.

I can't resist pointing out that that doesn't make the commit message any
better.

> Beside that it's an obvious performance gain.  The API change did not
> magically remove the pixel by pixel conversion of the colour space, it
> just hid it in SDL, under more indirection.

I see the point, but while I don't know SDL internals well enough to
know if it does this, it might be possible to do the conversion in
hardware in some cases.


Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

[-- Attachment #2: vmware_depth2.diff --]
[-- Type: text/plain, Size: 8217 bytes --]

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 5ceebf1..d6c8831 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -44,8 +44,6 @@ struct vmsvga_state_s {
     int width;
     int height;
     int invalidated;
-    int depth;
-    int bypp;
     int enable;
     int config;
     struct {
@@ -70,11 +68,7 @@ struct vmsvga_state_s {
     int new_height;
     uint32_t guest;
     uint32_t svgaid;
-    uint32_t wred;
-    uint32_t wgreen;
-    uint32_t wblue;
     int syncing;
-    int fb_size;
 
     union {
         uint32_t *fifo;
@@ -297,11 +291,18 @@ enum {
     SVGA_CURSOR_ON_RESTORE_TO_FB = 3,
 };
 
+static int fb_size(struct vmsvga_state_s *s)
+{
+    if (!s->enable) return 0;
+    return ds_get_linesize(s->vga.ds) * ds_get_height(s->vga.ds);
+}
+
 static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
                 int x, int y, int w, int h)
 {
 #ifndef DIRECT_VRAM
     int line;
+    int bypp = ds_get_bytes_per_pixel(s->vga.ds);
     int bypl;
     int width;
     int start;
@@ -323,9 +324,9 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
     }
 
     line = h;
-    bypl = s->bypp * s->width;
-    width = s->bypp * w;
-    start = s->bypp * x + bypl * y;
+    bypl = ds_get_linesize(s->vga.ds);
+    width = bypp * w;
+    start = bypp * x + bypl * y;
     src = s->vga.vram_ptr + start;
     dst = ds_get_data(s->vga.ds) + start;
 
@@ -339,7 +340,8 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
 static inline void vmsvga_update_screen(struct vmsvga_state_s *s)
 {
 #ifndef DIRECT_VRAM
-    memcpy(ds_get_data(s->vga.ds), s->vga.vram_ptr, s->bypp * s->width * s->height);
+    int bypp = ds_get_bytes_per_pixel(s->vga.ds);
+    memcpy(ds_get_data(s->vga.ds), s->vga.vram_ptr, bypp * s->width * s->height);
 #endif
 
     dpy_update(s->vga.ds, 0, 0, s->width, s->height);
@@ -385,8 +387,9 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
 # else
     uint8_t *vram = s->vga.vram_ptr;
 # endif
-    int bypl = s->bypp * s->width;
-    int width = s->bypp * w;
+    int bypp = ds_get_bytes_per_pixel(s->vga.ds);
+    int bypl = ds_get_linesize(s->vga.ds);
+    int width = bypp * w;
     int line = h;
     uint8_t *ptr[2];
 
@@ -397,13 +400,13 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
 # endif
     {
         if (y1 > y0) {
-            ptr[0] = vram + s->bypp * x0 + bypl * (y0 + h - 1);
-            ptr[1] = vram + s->bypp * x1 + bypl * (y1 + h - 1);
+            ptr[0] = vram + bypp * x0 + bypl * (y0 + h - 1);
+            ptr[1] = vram + bypp * x1 + bypl * (y1 + h - 1);
             for (; line > 0; line --, ptr[0] -= bypl, ptr[1] -= bypl)
                 memmove(ptr[1], ptr[0], width);
         } else {
-            ptr[0] = vram + s->bypp * x0 + bypl * y0;
-            ptr[1] = vram + s->bypp * x1 + bypl * y1;
+            ptr[0] = vram + bypp * x0 + bypl * y0;
+            ptr[1] = vram + bypp * x1 + bypl * y1;
             for (; line > 0; line --, ptr[0] += bypl, ptr[1] += bypl)
                 memmove(ptr[1], ptr[0], width);
         }
@@ -422,8 +425,8 @@ static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
 # else
     uint8_t *vram = s->vga.vram_ptr;
 # endif
-    int bypp = s->bypp;
-    int bypl = bypp * s->width;
+    int bypp = ds_get_bytes_per_pixel(s->vga.ds);
+    int bypl = ds_get_linesize(s->vga.ds);
     int width = bypp * w;
     int line = h;
     int column;
@@ -664,23 +667,24 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return SVGA_MAX_HEIGHT;
 
     case SVGA_REG_DEPTH:
-        return s->depth;
+        return ds_get_bits_per_pixel(s->vga.ds);
 
+    case SVGA_REG_HOST_BITS_PER_PIXEL:
     case SVGA_REG_BITS_PER_PIXEL:
-        return (s->depth + 7) & ~7;
+        return ds_get_bytes_per_pixel(s->vga.ds) << 3;
 
     case SVGA_REG_PSEUDOCOLOR:
         return 0x0;
 
     case SVGA_REG_RED_MASK:
-        return s->wred;
+        return s->vga.ds->surface->pf.rmask;
     case SVGA_REG_GREEN_MASK:
-        return s->wgreen;
+        return s->vga.ds->surface->pf.gmask;
     case SVGA_REG_BLUE_MASK:
-        return s->wblue;
+        return s->vga.ds->surface->pf.bmask;
 
     case SVGA_REG_BYTES_PER_LINE:
-        return ((s->depth + 7) >> 3) * s->new_width;
+        return ds_get_linesize(s->vga.ds);
 
     case SVGA_REG_FB_START:
         return s->vram_base;
@@ -692,7 +696,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return s->vga.vram_size - SVGA_FIFO_SIZE;
 
     case SVGA_REG_FB_SIZE:
-        return s->fb_size;
+        return fb_size(s);
 
     case SVGA_REG_CAPABILITIES:
         caps = SVGA_CAP_NONE;
@@ -737,9 +741,6 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
     case SVGA_REG_CURSOR_ON:
         return s->cursor.on;
 
-    case SVGA_REG_HOST_BITS_PER_PIXEL:
-        return (s->depth + 7) & ~7;
-
     case SVGA_REG_SCRATCH_SIZE:
         return s->scratch_size;
 
@@ -777,8 +778,6 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
 #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;
 
     case SVGA_REG_WIDTH:
@@ -793,7 +792,7 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
 
     case SVGA_REG_DEPTH:
     case SVGA_REG_BITS_PER_PIXEL:
-        if (value != s->depth) {
+        if (value != ds_get_bits_per_pixel(s->vga.ds)) {
             printf("%s: Bad colour depth: %i bits\n", __FUNCTION__, value);
             s->config = 0;
         }
@@ -923,38 +922,9 @@ static void vmsvga_reset(struct vmsvga_state_s *s)
     s->width = -1;
     s->height = -1;
     s->svgaid = SVGA_ID;
-    s->depth = 24;
-    s->bypp = (s->depth + 7) >> 3;
     s->cursor.on = 0;
     s->redraw_fifo_first = 0;
     s->redraw_fifo_last = 0;
-    switch (s->depth) {
-    case 8:
-        s->wred   = 0x00000007;
-        s->wgreen = 0x00000038;
-        s->wblue  = 0x000000c0;
-        break;
-    case 15:
-        s->wred   = 0x0000001f;
-        s->wgreen = 0x000003e0;
-        s->wblue  = 0x00007c00;
-        break;
-    case 16:
-        s->wred   = 0x0000001f;
-        s->wgreen = 0x000007e0;
-        s->wblue  = 0x0000f800;
-        break;
-    case 24:
-        s->wred   = 0x00ff0000;
-        s->wgreen = 0x0000ff00;
-        s->wblue  = 0x000000ff;
-        break;
-    case 32:
-        s->wred   = 0x00ff0000;
-        s->wgreen = 0x0000ff00;
-        s->wblue  = 0x000000ff;
-        break;
-    }
     s->syncing = 0;
 }
 
@@ -983,7 +953,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename)
         return;
     }
 
-    if (s->depth == 32) {
+    if (ds_get_bits_per_pixel(s->vga.ds) == 32) {
         DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
                 s->height, 32, ds_get_linesize(s->vga.ds), s->vga.vram_ptr);
         ppm_save(filename, ds);
@@ -1072,7 +1042,7 @@ static CPUWriteMemoryFunc *vmsvga_vram_write[] = {
 
 static void vmsvga_save(struct vmsvga_state_s *s, QEMUFile *f)
 {
-    qemu_put_be32(f, s->depth);
+    qemu_put_be32(f, ds_get_bits_per_pixel(s->vga.ds));
     qemu_put_be32(f, s->enable);
     qemu_put_be32(f, s->config);
     qemu_put_be32(f, s->cursor.id);
@@ -1086,7 +1056,7 @@ static void vmsvga_save(struct vmsvga_state_s *s, QEMUFile *f)
     qemu_put_be32s(f, &s->guest);
     qemu_put_be32s(f, &s->svgaid);
     qemu_put_be32(f, s->syncing);
-    qemu_put_be32(f, s->fb_size);
+    qemu_put_be32(f, fb_size(s));
 }
 
 static int vmsvga_load(struct vmsvga_state_s *s, QEMUFile *f)
@@ -1106,9 +1076,9 @@ static int vmsvga_load(struct vmsvga_state_s *s, QEMUFile *f)
     qemu_get_be32s(f, &s->guest);
     qemu_get_be32s(f, &s->svgaid);
     s->syncing=qemu_get_be32(f);
-    s->fb_size=qemu_get_be32(f);
+    qemu_get_be32(f); // fb_size is not used anymore
 
-    if (s->enable && depth != s->depth) {
+    if (s->enable && depth != ds_get_bits_per_pixel(s->vga.ds)) {
         printf("%s: need colour depth of %i bits to resume operation.\n",
                         __FUNCTION__, depth);
         return -EINVAL;