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

login
register
mail settings
Submitter Gerd Hoffmann
Date March 25, 2013, 9:10 a.m.
Message ID <51501473.3010608@redhat.com>
Download mbox | patch
Permalink /patch/230592/
State New
Headers show

Comments

Gerd Hoffmann - March 25, 2013, 9:10 a.m.
On 03/25/13 09:40, Jan Kiszka wrote:
> On 2013-03-25 09:39, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> Any hints in the X server log?
>>>
>>> "vmwlegacy(0): Weight given (565) is inconsistent with the depth
>>> (24)"
>>
>> Weight hints depth 16 indeed.  What depth used the server to run at?
>> 16 or 24?
> 
> 24

As expected.  Puzzling where the 565 weight comes from ...

Can you apply the attached patch, enable vmware_* + displaysurface_*
tracepoints + send a log?

thanks,
  Gerd
From 40ced618a80d70f579aecb397d448a45fd499c63 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Mon, 25 Mar 2013 09:53:35 +0100
Subject: [PATCH] vmware vga: trace value read+write

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/vmware_vga.c |  100 +++++++++++++++++++++++++++++++++++++------------------
 trace-events    |    2 ++
 2 files changed, 70 insertions(+), 32 deletions(-)
Jan Kiszka - March 25, 2013, 9:32 a.m.
On 2013-03-25 10:10, Gerd Hoffmann wrote:
> On 03/25/13 09:40, Jan Kiszka wrote:
>> On 2013-03-25 09:39, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>>> Any hints in the X server log?
>>>>
>>>> "vmwlegacy(0): Weight given (565) is inconsistent with the depth
>>>> (24)"
>>>
>>> Weight hints depth 16 indeed.  What depth used the server to run at?
>>> 16 or 24?
>>
>> 24
> 
> As expected.  Puzzling where the 565 weight comes from ...
> 
> Can you apply the attached patch, enable vmware_* + displaysurface_*
> tracepoints + send a log?

[shrinking CC list at this chance]

read: index 1, value 0x0
read: index 2, value 0x320
read: index 3, value 0x258
read: index 7, value 0x20
read: index 0, value 0x90000002
read: index 20, value 0x0
write: index 0, value 0x90000002
read: index 0, value 0x90000002
read: index 17, value 0x3
read: index 28, value 0x20
read: index 6, value 0x20
read: index 15, value 0x1000000
read: index 13, value 0xfd000000
read: index 4, value 0x938
read: index 5, value 0x6ea
read: index 9, value 0xf800
read: index 10, value 0x7e0
read: index 11, value 0x1f
read: index 8, value 0x0
read: index 1, value 0x0
read: index 2, value 0x320
read: index 3, value 0x258
read: index 7, value 0x20
read: index 0, value 0x90000002
read: index 20, value 0x0
write: index 0, value 0x90000002
read: index 0, value 0x90000002
read: index 17, value 0x3
read: index 28, value 0x20
read: index 6, value 0x20
read: index 15, value 0x1000000
read: index 13, value 0xfd000000
read: index 4, value 0x938
read: index 5, value 0x6ea
read: index 9, value 0xf800
read: index 10, value 0x7e0
read: index 11, value 0x1f
read: index 8, value 0x0

Jan
Gerd Hoffmann - March 25, 2013, 9:48 a.m.
On 03/25/13 10:32, Jan Kiszka wrote:
> On 2013-03-25 10:10, Gerd Hoffmann wrote:
>> On 03/25/13 09:40, Jan Kiszka wrote:
>>> On 2013-03-25 09:39, Gerd Hoffmann wrote:
>>>> Hi,
>>>> 
>>>>>> Any hints in the X server log?
>>>>> 
>>>>> "vmwlegacy(0): Weight given (565) is inconsistent with the
>>>>> depth (24)"
>>>> 
>>>> Weight hints depth 16 indeed.  What depth used the server to
>>>> run at? 16 or 24?
>>> 
>>> 24
>> 
>> As expected.  Puzzling where the 565 weight comes from ...
>> 
>> Can you apply the attached patch, enable vmware_* +
>> displaysurface_* tracepoints + send a log?
> 
> [shrinking CC list at this chance]

Hmm, no displaysurface_* tracepoints?

> read: index 1, value 0x0 read: index 2, value 0x320 read: index 3,
> value 0x258

800x600

> read: index 7, value 0x20

4 bytes per pixel

> read: index 9, value 0xf800 read: index 10, value 0x7e0 read: index
> 11, value 0x1f

16bpp r/g/b masks.  Hmm.

No attempt to initialize the display (set res etc).  Maybe because the
x11 server driver errors out early.

The 800x600 video mode was probably set from vgabios.  That doesn't
explain the inconsistency though.

Do you boot with vesafb enabled?  Which video mode?  Probably 800x600?
 What depth?

cheers,
  Gerd
Jan Kiszka - March 25, 2013, 9:55 a.m.
On 2013-03-25 10:48, Gerd Hoffmann wrote:
> On 03/25/13 10:32, Jan Kiszka wrote:
>> On 2013-03-25 10:10, Gerd Hoffmann wrote:
>>> On 03/25/13 09:40, Jan Kiszka wrote:
>>>> On 2013-03-25 09:39, Gerd Hoffmann wrote:
>>>>> Hi,
>>>>>
>>>>>>> Any hints in the X server log?
>>>>>>
>>>>>> "vmwlegacy(0): Weight given (565) is inconsistent with the
>>>>>> depth (24)"
>>>>>
>>>>> Weight hints depth 16 indeed.  What depth used the server to
>>>>> run at? 16 or 24?
>>>>
>>>> 24
>>>
>>> As expected.  Puzzling where the 565 weight comes from ...
>>>
>>> Can you apply the attached patch, enable vmware_* +
>>> displaysurface_* tracepoints + send a log?
>>
>> [shrinking CC list at this chance]
> 
> Hmm, no displaysurface_* tracepoints?

Setting up a full trace was a bit too unhandy (it still lacks some
ad-hoc configuration). If you need it, I'll redo.

> 
>> read: index 1, value 0x0 read: index 2, value 0x320 read: index 3,
>> value 0x258
> 
> 800x600
> 
>> read: index 7, value 0x20
> 
> 4 bytes per pixel
> 
>> read: index 9, value 0xf800 read: index 10, value 0x7e0 read: index
>> 11, value 0x1f
> 
> 16bpp r/g/b masks.  Hmm.
> 
> No attempt to initialize the display (set res etc).  Maybe because the
> x11 server driver errors out early.
> 
> The 800x600 video mode was probably set from vgabios.  That doesn't
> explain the inconsistency though.
> 
> Do you boot with vesafb enabled?  Which video mode?  Probably 800x600?
>  What depth?

I'm booting with vga=0x314.

Jan
Jan Kiszka - March 25, 2013, 10 a.m.
On 2013-03-25 10:55, Jan Kiszka wrote:
> On 2013-03-25 10:48, Gerd Hoffmann wrote:
>> On 03/25/13 10:32, Jan Kiszka wrote:
>>> On 2013-03-25 10:10, Gerd Hoffmann wrote:
>>>> On 03/25/13 09:40, Jan Kiszka wrote:
>>>>> On 2013-03-25 09:39, Gerd Hoffmann wrote:
>>>>>> Hi,
>>>>>>
>>>>>>>> Any hints in the X server log?
>>>>>>>
>>>>>>> "vmwlegacy(0): Weight given (565) is inconsistent with the
>>>>>>> depth (24)"
>>>>>>
>>>>>> Weight hints depth 16 indeed.  What depth used the server to
>>>>>> run at? 16 or 24?
>>>>>
>>>>> 24
>>>>
>>>> As expected.  Puzzling where the 565 weight comes from ...
>>>>
>>>> Can you apply the attached patch, enable vmware_* +
>>>> displaysurface_* tracepoints + send a log?
>>>
>>> [shrinking CC list at this chance]
>>
>> Hmm, no displaysurface_* tracepoints?
> 
> Setting up a full trace was a bit too unhandy (it still lacks some
> ad-hoc configuration). If you need it, I'll redo.

Ah, just found one, though I'm unsure if it is intended to work like
this: -trace events=., then "trace-event vmware*/display* on" from the
monitor.

[5424.260045] displaysurface_create surface=0x7ff3158c33b0, 720x400
[5424.260332] displaysurface_free surface=0x7ff3158f6250
[5425.580115] displaysurface_create_from surface=0x7ff315d3df40, 800x600, bpp 16, bswap 0
[5425.580257] displaysurface_free surface=0x7ff3158c33b0
[5431.993087] vmware_value_read index 1, value 0x0
[5431.993147] vmware_value_read index 2, value 0x320
[5431.993165] vmware_value_read index 3, value 0x258
[5431.993180] vmware_value_read index 7, value 0x20
[5431.993196] vmware_value_read index 0, value 0x90000002
[5431.993211] vmware_value_read index 20, value 0x0
[5431.993226] vmware_value_write index 0, value 0x90000002
[5431.993240] vmware_value_read index 0, value 0x90000002
[5431.993256] vmware_value_read index 17, value 0x3
[5431.993272] vmware_value_read index 28, value 0x20
[5431.993287] vmware_value_read index 6, value 0x20
[5431.993301] vmware_value_read index 15, value 0x1000000
[5431.993317] vmware_value_read index 13, value 0xfd000000
[5431.993333] vmware_value_read index 4, value 0x938
[5431.993348] vmware_value_read index 5, value 0x6ea
[5431.993465] vmware_value_read index 9, value 0xf800
[5431.993486] vmware_value_read index 10, value 0x7e0
[5431.993502] vmware_value_read index 11, value 0x1f
[5431.993517] vmware_value_read index 8, value 0x0
[5432.270084] displaysurface_create_from surface=0x7ff3158f6250, 800x600, bpp 16, bswap 0
[5432.270156] displaysurface_free surface=0x7ff315d3df40
[5433.467540] vmware_value_read index 1, value 0x0
[5433.467584] vmware_value_read index 2, value 0x320
[5433.467621] vmware_value_read index 3, value 0x258
[5433.467637] vmware_value_read index 7, value 0x20
[5433.467652] vmware_value_read index 0, value 0x90000002
[5433.467667] vmware_value_read index 20, value 0x0
[5433.467681] vmware_value_write index 0, value 0x90000002
[5433.467695] vmware_value_read index 0, value 0x90000002
[5433.467713] vmware_value_read index 17, value 0x3
[5433.467727] vmware_value_read index 28, value 0x20
[5433.467742] vmware_value_read index 6, value 0x20
[5433.467757] vmware_value_read index 15, value 0x1000000
[5433.467772] vmware_value_read index 13, value 0xfd000000
[5433.467786] vmware_value_read index 4, value 0x938
[5433.467801] vmware_value_read index 5, value 0x6ea
[5433.467934] vmware_value_read index 9, value 0xf800
[5433.467957] vmware_value_read index 10, value 0x7e0
[5433.467977] vmware_value_read index 11, value 0x1f
[5433.467993] vmware_value_read index 8, value 0x0
[5433.470111] displaysurface_create_from surface=0x7ff315d3df40, 800x600, bpp 16, bswap 0
[5433.470171] displaysurface_free surface=0x7ff3158f6250
[5433.650177] displaysurface_create_from surface=0x7ff315c41750, 800x600, bpp 16, bswap 0
[5433.650231] displaysurface_free surface=0x7ff315d3df40
[5434.710649] vmware_value_read index 1, value 0x0
[5434.710690] vmware_value_read index 2, value 0x320
[5434.710713] vmware_value_read index 3, value 0x258
[5434.710735] vmware_value_read index 7, value 0x20
...

Jan
Gerd Hoffmann - March 25, 2013, 10:37 a.m.
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
Igor Mitsyanko - March 25, 2013, 1:56 p.m.
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?

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 4ebfe17..d317b4e 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -721,61 +721,79 @@  static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
     uint32_t caps;
     struct vmsvga_state_s *s = opaque;
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
+    uint32_t ret;
 
     switch (s->index) {
     case SVGA_REG_ID:
-        return s->svgaid;
+        ret = s->svgaid;
+        break;
 
     case SVGA_REG_ENABLE:
-        return s->enable;
+        ret = s->enable;
+        break;
 
     case SVGA_REG_WIDTH:
-        return surface_width(surface);
+        ret = surface_width(surface);
+        break;
 
     case SVGA_REG_HEIGHT:
-        return surface_height(surface);
+        ret = surface_height(surface);
+        break;
 
     case SVGA_REG_MAX_WIDTH:
-        return SVGA_MAX_WIDTH;
+        ret = SVGA_MAX_WIDTH;
+        break;
 
     case SVGA_REG_MAX_HEIGHT:
-        return SVGA_MAX_HEIGHT;
+        ret = SVGA_MAX_HEIGHT;
+        break;
 
     case SVGA_REG_DEPTH:
-        return s->depth;
+        ret = s->depth;
+        break;
 
     case SVGA_REG_BITS_PER_PIXEL:
-        return (s->depth + 7) & ~7;
+        ret = (s->depth + 7) & ~7;
+        break;
 
     case SVGA_REG_PSEUDOCOLOR:
-        return 0x0;
+        ret = 0x0;
+        break;
 
     case SVGA_REG_RED_MASK:
-        return surface->pf.rmask;
+        ret = surface->pf.rmask;
+        break;
 
     case SVGA_REG_GREEN_MASK:
-        return surface->pf.gmask;
+        ret = surface->pf.gmask;
+        break;
 
     case SVGA_REG_BLUE_MASK:
-        return surface->pf.bmask;
+        ret = surface->pf.bmask;
+        break;
 
     case SVGA_REG_BYTES_PER_LINE:
-        return s->bypp * s->new_width;
+        ret = s->bypp * s->new_width;
+        break;
 
     case SVGA_REG_FB_START: {
         struct pci_vmsvga_state_s *pci_vmsvga
             = container_of(s, struct pci_vmsvga_state_s, chip);
-        return pci_get_bar_addr(&pci_vmsvga->card, 1);
+        ret = pci_get_bar_addr(&pci_vmsvga->card, 1);
+        break;
     }
 
     case SVGA_REG_FB_OFFSET:
-        return 0x0;
+        ret = 0x0;
+        break;
 
     case SVGA_REG_VRAM_SIZE:
-        return s->vga.vram_size; /* No physical VRAM besides the framebuffer */
+        ret = s->vga.vram_size; /* No physical VRAM besides the framebuffer */
+        break;
 
     case SVGA_REG_FB_SIZE:
-        return s->vga.vram_size;
+        ret = s->vga.vram_size;
+        break;
 
     case SVGA_REG_CAPABILITIES:
         caps = SVGA_CAP_NONE;
@@ -791,66 +809,84 @@  static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
                     SVGA_CAP_CURSOR_BYPASS;
         }
 #endif
-        return caps;
+        ret = caps;
+        break;
 
     case SVGA_REG_MEM_START: {
         struct pci_vmsvga_state_s *pci_vmsvga
             = container_of(s, struct pci_vmsvga_state_s, chip);
-        return pci_get_bar_addr(&pci_vmsvga->card, 2);
+        ret = pci_get_bar_addr(&pci_vmsvga->card, 2);
+        break;
     }
 
     case SVGA_REG_MEM_SIZE:
-        return s->fifo_size;
+        ret = s->fifo_size;
+        break;
 
     case SVGA_REG_CONFIG_DONE:
-        return s->config;
+        ret = s->config;
+        break;
 
     case SVGA_REG_SYNC:
     case SVGA_REG_BUSY:
-        return s->syncing;
+        ret = s->syncing;
+        break;
 
     case SVGA_REG_GUEST_ID:
-        return s->guest;
+        ret = s->guest;
+        break;
 
     case SVGA_REG_CURSOR_ID:
-        return s->cursor.id;
+        ret = s->cursor.id;
+        break;
 
     case SVGA_REG_CURSOR_X:
-        return s->cursor.x;
+        ret = s->cursor.x;
+        break;
 
     case SVGA_REG_CURSOR_Y:
-        return s->cursor.x;
+        ret = s->cursor.x;
+        break;
 
     case SVGA_REG_CURSOR_ON:
-        return s->cursor.on;
+        ret = s->cursor.on;
+        break;
 
     case SVGA_REG_HOST_BITS_PER_PIXEL:
-        return (s->depth + 7) & ~7;
+        ret = (s->depth + 7) & ~7;
+        break;
 
     case SVGA_REG_SCRATCH_SIZE:
-        return s->scratch_size;
+        ret = s->scratch_size;
+        break;
 
     case SVGA_REG_MEM_REGS:
     case SVGA_REG_NUM_DISPLAYS:
     case SVGA_REG_PITCHLOCK:
     case SVGA_PALETTE_BASE ... SVGA_PALETTE_END:
-        return 0;
+        ret = 0;
+        break;
 
     default:
         if (s->index >= SVGA_SCRATCH_BASE &&
             s->index < SVGA_SCRATCH_BASE + s->scratch_size) {
-            return s->scratch[s->index - SVGA_SCRATCH_BASE];
+            ret = s->scratch[s->index - SVGA_SCRATCH_BASE];
+            break;
         }
         printf("%s: Bad register %02x\n", __func__, s->index);
+        ret = 0;
+        break;
     }
 
-    return 0;
+    trace_vmware_value_read(s->index, ret);
+    return ret;
 }
 
 static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
 {
     struct vmsvga_state_s *s = opaque;
 
+    trace_vmware_value_write(s->index, value);
     switch (s->index) {
     case SVGA_REG_ID:
         if (value == SVGA_ID_2 || value == SVGA_ID_1 || value == SVGA_ID_0) {
diff --git a/trace-events b/trace-events
index 88b1070..ea3bdf2 100644
--- a/trace-events
+++ b/trace-events
@@ -970,6 +970,8 @@  displaychangelistener_unregister(void *dcl, const char *name) "%p [ %s ]"
 
 # vga.c
 ppm_save(const char *filename, void *display_surface) "%s surface=%p"
+vmware_value_read(uint32_t index, uint32_t value) "index %d, value 0x%x"
+vmware_value_write(uint32_t index, uint32_t value) "index %d, value 0x%x"
 
 # savevm.c