Patchwork [16/18] console: stop using DisplayState in gfx hardware emulation

login
register
mail settings
Submitter Gerd Hoffmann
Date March 25, 2013, 8:30 p.m.
Message ID <5150B3F0.3080003@redhat.com>
Download mbox | patch
Permalink /patch/230984/
State New
Headers show

Comments

Gerd Hoffmann - March 25, 2013, 8:30 p.m.
On 03/25/13 14:56, Igor Mitsyanko wrote:
> On 03/25/2013 02:37 PM, Gerd Hoffmann wrote:
>>
>>>    Hi,
>>>
>>>  [5425.580115] displaysurface_create_from surface=0x7ff315d3df40,
>>>> 800x600, bpp 16, bswap 0 [5425.580257] displaysurface_free
>>>> surface=0x7ff3158c33b0
>>>>
>>>
>>> This is vga=0x314
>>>
>>> Looks like we have some funky interaction between vga and vmware.
>>>
>>> I'll go dig.  Meanwhile you can try vga=0x315 (800x600x24) or
>>> vga=normal (textmode), that has a high chance to workaround this.
>>>
>>> cheers,
>>>    Gerd
>>>
>>
>>
> 
>  Couldn't it be because wred, wgreen and wblue were removed? It seems like
> it was a workaround for some pre-existing problem, is it ok that you
> removed them but left depth and bypp intact?

No, it is not, and yes, this is where the inconsistency comes from.  We
read wred+wgreen+wblue directly from the surface whereas depth is cached
in the vmware vga state struct.  Patch attached.  Not fully tested yet.

I think the "pre-existing problem" is that the vmware vga has the bochs
dispi vbe interface enabled.  So the vga bios sets a video mode (via
bocks dispi interface) and the vmware svga emulation isn't notified ...

Making vmware vga emulation read stuff directly from the surface should
improve things a bit as the data returned to the guest reflects what the
actual gfx card state is, even when not set using the vmware vga
interface.  The change could still have unwanted side effects though.

I think the "real" fix is to disable bochs on vmware and hack seavgabios
to do modesetting using the vmware vga interfaces.

cheers,
  Gerd
From 266181a355ac8440d9143d254126244c611d1f67 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Mon, 25 Mar 2013 11:44:21 +0100
Subject: [PATCH] vmware: remove depth+bypp from state

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/vmware_vga.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)
BALATON Zoltan - March 26, 2013, 12:02 a.m.
On Mon, 25 Mar 2013, Gerd Hoffmann wrote:
> No, it is not, and yes, this is where the inconsistency comes from.  We
> read wred+wgreen+wblue directly from the surface whereas depth is cached
> in the vmware vga state struct.  Patch attached.  Not fully tested yet.

Tried that before and it didn't work that time (but maybe things have 
improved since). I could not find the "Problem" but you might look at
http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg00512.html
and commit 1f202568e0553b416483e5993f1bde219c22cf72 for the symptoms and 
why these were added back after being removed shortly. If these are not 
relevant any more just ignore this message (or use it to get rid of the 
dummy depth variable in your patch).

Regards,
BALATON Zoltan
Jan Kiszka - March 26, 2013, 8:26 a.m.
On 2013-03-25 21:30, Gerd Hoffmann wrote:
> On 03/25/13 14:56, Igor Mitsyanko wrote:
>> On 03/25/2013 02:37 PM, Gerd Hoffmann wrote:
>>>
>>>>    Hi,
>>>>
>>>>  [5425.580115] displaysurface_create_from surface=0x7ff315d3df40,
>>>>> 800x600, bpp 16, bswap 0 [5425.580257] displaysurface_free
>>>>> surface=0x7ff3158c33b0
>>>>>
>>>>
>>>> This is vga=0x314
>>>>
>>>> Looks like we have some funky interaction between vga and vmware.
>>>>
>>>> I'll go dig.  Meanwhile you can try vga=0x315 (800x600x24) or
>>>> vga=normal (textmode), that has a high chance to workaround this.
>>>>
>>>> cheers,
>>>>    Gerd
>>>>
>>>
>>>
>>
>>  Couldn't it be because wred, wgreen and wblue were removed? It seems like
>> it was a workaround for some pre-existing problem, is it ok that you
>> removed them but left depth and bypp intact?
> 
> No, it is not, and yes, this is where the inconsistency comes from.  We
> read wred+wgreen+wblue directly from the surface whereas depth is cached
> in the vmware vga state struct.  Patch attached.  Not fully tested yet.

Unfortunately, this doesn't change the picture (except for the expected
"vmsvga_value_read: Bad register 1c"). The 0x315 workaround does indeed
work.

Jan
Gerd Hoffmann - April 3, 2013, 11:50 a.m.
Hi,

>> No, it is not, and yes, this is where the inconsistency comes 
>> from.  We read wred+wgreen+wblue directly from the surface 
>> whereas depth is cached in the vmware vga state struct.  Patch 
>> attached.  Not fully tested yet.
> 
> Unfortunately, this doesn't change the picture (except for the 
> expected "vmsvga_value_read: Bad register 1c"). The 0x315 
> workaround does indeed work.

Hmm, the patch fixes it for me (boot vesafb with 800x600 or 1024x768 @
16bpp, Xorg starts successfully) ...

Can I get a full X server log?

cheers,
  Gerd
Jan Kiszka - April 10, 2013, 8:31 a.m.
On 2013-04-03 13:50, Gerd Hoffmann wrote:
>   Hi,
> 
>>> No, it is not, and yes, this is where the inconsistency comes 
>>> from.  We read wred+wgreen+wblue directly from the surface 
>>> whereas depth is cached in the vmware vga state struct.  Patch 
>>> attached.  Not fully tested yet.
>>
>> Unfortunately, this doesn't change the picture (except for the 
>> expected "vmsvga_value_read: Bad register 1c"). The 0x315 
>> workaround does indeed work.
> 
> Hmm, the patch fixes it for me (boot vesafb with 800x600 or 1024x768 @
> 16bpp, Xorg starts successfully) ...
> 
> Can I get a full X server log?

Here is the one without the patch (it no longer applies). If you want me
to rerun with that patch, please provide an update.

Thanks,
Jan
Gerd Hoffmann - April 16, 2013, 7:42 a.m.
On 04/10/13 10:31, Jan Kiszka wrote:
> On 2013-04-03 13:50, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> No, it is not, and yes, this is where the inconsistency comes 
>>>> from.  We read wred+wgreen+wblue directly from the surface 
>>>> whereas depth is cached in the vmware vga state struct.  Patch 
>>>> attached.  Not fully tested yet.
>>>
>>> Unfortunately, this doesn't change the picture (except for the 
>>> expected "vmsvga_value_read: Bad register 1c"). The 0x315 
>>> workaround does indeed work.
>>
>> Hmm, the patch fixes it for me (boot vesafb with 800x600 or 1024x768 @
>> 16bpp, Xorg starts successfully) ...
>>
>> Can I get a full X server log?
> 
> Here is the one without the patch (it no longer applies). If you want me
> to rerun with that patch, please provide an update.

Pushed latest pixman bits (including vmware fixes) to
   git://git.kraxel.org/qemu rebase/pixman

Should be working fine again.

cheers,
  Gerd
Jan Kiszka - April 20, 2013, 2:04 p.m.
On 2013-04-16 09:42, Gerd Hoffmann wrote:
> On 04/10/13 10:31, Jan Kiszka wrote:
>> On 2013-04-03 13:50, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>>> No, it is not, and yes, this is where the inconsistency comes 
>>>>> from.  We read wred+wgreen+wblue directly from the surface 
>>>>> whereas depth is cached in the vmware vga state struct.  Patch 
>>>>> attached.  Not fully tested yet.
>>>>
>>>> Unfortunately, this doesn't change the picture (except for the 
>>>> expected "vmsvga_value_read: Bad register 1c"). The 0x315 
>>>> workaround does indeed work.
>>>
>>> Hmm, the patch fixes it for me (boot vesafb with 800x600 or 1024x768 @
>>> 16bpp, Xorg starts successfully) ...
>>>
>>> Can I get a full X server log?
>>
>> Here is the one without the patch (it no longer applies). If you want me
>> to rerun with that patch, please provide an update.
> 
> Pushed latest pixman bits (including vmware fixes) to
>    git://git.kraxel.org/qemu rebase/pixman
> 
> Should be working fine again.

Yep, works again.

Thanks,
Jan

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index d317b4e..aa4d6f8 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -39,8 +39,7 @@  struct vmsvga_state_s {
     VGACommonState vga;
 
     int invalidated;
-    int depth;
-    int bypp;
+    int depth_vmstate_dummy;
     int enable;
     int config;
     struct {
@@ -749,11 +748,12 @@  static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         break;
 
     case SVGA_REG_DEPTH:
-        ret = s->depth;
+        ret = surface_bits_per_pixel(surface);
         break;
 
     case SVGA_REG_BITS_PER_PIXEL:
-        ret = (s->depth + 7) & ~7;
+    case SVGA_REG_HOST_BITS_PER_PIXEL:
+        ret = (surface_bits_per_pixel(surface) + 7) & ~7;
         break;
 
     case SVGA_REG_PSEUDOCOLOR:
@@ -773,7 +773,7 @@  static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         break;
 
     case SVGA_REG_BYTES_PER_LINE:
-        ret = s->bypp * s->new_width;
+        ret = surface_bytes_per_pixel(surface) * s->new_width;
         break;
 
     case SVGA_REG_FB_START: {
@@ -852,10 +852,6 @@  static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         ret = s->cursor.on;
         break;
 
-    case SVGA_REG_HOST_BITS_PER_PIXEL:
-        ret = (s->depth + 7) & ~7;
-        break;
-
     case SVGA_REG_SCRATCH_SIZE:
         ret = s->scratch_size;
         break;
@@ -885,6 +881,7 @@  static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
 static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
 {
     struct vmsvga_state_s *s = opaque;
+    DisplaySurface *surface = qemu_console_surface(s->vga.con);
 
     trace_vmware_value_write(s->index, value);
     switch (s->index) {
@@ -924,7 +921,7 @@  static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
         break;
 
     case SVGA_REG_BITS_PER_PIXEL:
-        if (value != s->depth) {
+        if (value != surface_bits_per_pixel(surface)) {
             printf("%s: Bad bits per pixel: %i bits\n", __func__, value);
             s->config = 0;
         }
@@ -1125,7 +1122,7 @@  static const VMStateDescription vmstate_vmware_vga_internal = {
     .minimum_version_id_old = 0,
     .post_load = vmsvga_post_load,
     .fields      = (VMStateField[]) {
-        VMSTATE_INT32_EQUAL(depth, struct vmsvga_state_s),
+        VMSTATE_INT32_EQUAL(depth_vmstate_dummy, struct vmsvga_state_s),
         VMSTATE_INT32(enable, struct vmsvga_state_s),
         VMSTATE_INT32(config, struct vmsvga_state_s),
         VMSTATE_INT32(cursor.id, struct vmsvga_state_s),
@@ -1183,10 +1180,7 @@  static void vmsvga_init(struct vmsvga_state_s *s,
     vga_common_init(&s->vga);
     vga_init(&s->vga, address_space, io, true);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
-    /* Save some values here in case they are changed later.
-     * This is suspicious and needs more though why it is needed. */
-    s->depth = surface_bits_per_pixel(surface);
-    s->bypp = surface_bytes_per_pixel(surface);
+    s->depth_vmstate_dummy = surface_bits_per_pixel(surface);
 }
 
 static uint64_t vmsvga_io_read(void *opaque, hwaddr addr, unsigned size)