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 <address@hidden>:
> > 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