From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MfxWL-0004Lj-EW for qemu-devel@nongnu.org; Tue, 25 Aug 2009 11:01:13 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MfxWG-0004Ij-M7 for qemu-devel@nongnu.org; Tue, 25 Aug 2009 11:01:12 -0400 Received: from [199.232.76.173] (port=37786 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MfxWG-0004If-Dj for qemu-devel@nongnu.org; Tue, 25 Aug 2009 11:01:08 -0400 Received: from mail.gmx.net ([213.165.64.20]:42815) by monty-python.gnu.org with smtp (Exim 4.60) (envelope-from ) id 1MfxWF-0005Wt-CK for qemu-devel@nongnu.org; Tue, 25 Aug 2009 11:01:08 -0400 Date: Tue, 25 Aug 2009 16:54:11 +0200 From: Reimar =?iso-8859-1?Q?D=F6ffinger?= Subject: Re: [Qemu-devel] [PATCH] use corect depth from DisplaySurface in vmware_vga.c Message-ID: <20090825145411.GA13710@1und1.de> References: <20090817100828.GA22029@1und1.de> <20090824132229.GB24730@1und1.de> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="2oS5YaxWCcQjTEyO" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: andrzej zaborowski Cc: qemu-devel@nongnu.org --2oS5YaxWCcQjTEyO Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Tue, Aug 25, 2009 at 01:45:15AM +0200, andrzej zaborowski wrote: > 2009/8/24 Reimar Döffinger : > > 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 --2oS5YaxWCcQjTEyO Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: attachment; filename="vmware_depth2.diff" 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; --2oS5YaxWCcQjTEyO--