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

["Reimar Döffinger" <Reimar.Doeffinger@gmx.de>]

[-- 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;