Re: [Qemu-devel] vga optmization

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

Glauber Costa wrote:

>> That's also how currently qemu-xen works.
>> I am glad that we agree :)
> 
> I'm attaching a new version. Let me know if it's better this way.
> 



Yes, this is much better thanks.

I still have comments about possible improvements, so I wrote patch
(against your patch).
This patch is not meant to be applied, is only meant to be read (I believe
that C code is more meaningful than English :).

Some of the changes include:

- instead of adding cirrus_lfb_addr, add a more generic lfb_addr to
VGAState, so that can be reused in the future for possible stdvga only
mappings;

- instead of using cirrus_lfb_mapped as a boolean, use it as the
mapping address, it is more useful that way;

- instead of keeping the kvm dirty map always enabled, enable it only
when the framebuffer is linear and in graphical mode;

- look at the changes to vga.c, there is a simple check to reduce the
dirty area to sync.

It would be nice to check if the last two changes are actually a
performance improvement.


diff -r a5bcebe9e2bc hw/cirrus_vga.c
--- a/hw/cirrus_vga.c	Wed Nov 05 12:09:13 2008 +0000
+++ b/hw/cirrus_vga.c	Wed Nov 05 14:29:54 2008 +0000
@@ -249,9 +249,6 @@
     int cirrus_linear_io_addr;
     int cirrus_linear_bitblt_io_addr;
     int cirrus_mmio_io_addr;
-    uint32_t cirrus_lfb_addr;
-    uint32_t cirrus_lfb_end;
-    uint32_t cirrus_lfb_mapped;
     uint32_t cirrus_addr_mask;
     uint32_t linear_mmio_mask;
     uint8_t cirrus_shadow_gr0;
@@ -2655,9 +2652,10 @@
 static void map_linear_vram(CirrusVGAState *s)
 {
 
-    if (!s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
-        set_vram_mapping(s->cirrus_lfb_addr, s->cirrus_lfb_end, s->vram_offset);
-        s->cirrus_lfb_mapped = 1;
+    if (!s->map_addr && s->lfb_addr && s->lfb_end) {
+        set_vram_mapping(s->lfb_addr, s->lfb_end, s->vram_offset);
+        s->map_addr = s->lfb_addr;
+        s->map_end = s->lfb_end;
     }
     
     if(!(s->cirrus_srcptr != s->cirrus_srcptr_end)
@@ -2674,10 +2672,10 @@
 static void unmap_linear_vram(CirrusVGAState *s)
 {
 
-    if (s->cirrus_lfb_mapped && s->cirrus_lfb_addr && s->cirrus_lfb_end) {
-        unset_vram_mapping(s->cirrus_lfb_addr,
-                           s->cirrus_lfb_end);
-        s->cirrus_lfb_mapped = 0;
+    if (s->map_addr && s->lfb_addr && s->lfb_end) {
+        unset_vram_mapping(s->lfb_addr,
+                           s->lfb_end);
+        s->map_addr = s->map_end = 0;
     }
 
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
@@ -3331,9 +3329,9 @@
     cpu_register_physical_memory(addr + 0x1000000, 0x400000,
 				 s->cirrus_linear_bitblt_io_addr);
 
-    s->cirrus_lfb_mapped = 0;
-    s->cirrus_lfb_addr = addr;
-    s->cirrus_lfb_end = addr + VGA_RAM_SIZE;
+    s->map_addr = 0;
+    s->lfb_addr = addr;
+    s->lfb_end = addr + VGA_RAM_SIZE;
 }
 
 static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
diff -r a5bcebe9e2bc hw/vga.c
--- a/hw/vga.c	Wed Nov 05 12:09:13 2008 +0000
+++ b/hw/vga.c	Wed Nov 05 14:29:54 2008 +0000
@@ -1244,6 +1244,9 @@
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
 
+    /* disable dirty bit tracking */
+    // kvm_log_stop();
+
     full_update |= update_palette16(s);
     palette = s->last_palette;
 
@@ -1569,7 +1572,8 @@
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
 
-    qemu_physical_sync_dirty_bitmap(s->vram_offset, s->vram_offset + VGA_RAM_SIZE);
+    /* Enable dirty bit tracking */
+    // kvm_log_start
 
     full_update |= update_basic_params(s);
 
@@ -1657,6 +1661,31 @@
         s->cursor_invalidate(s);
 
     line_offset = s->line_offset;
+
+    if (s->lfb_addr) {
+        if (height - 1 > s->line_compare || multi_run || (s->cr[0x17] & 3) != 3) {
+            /* Tricky things happen, just track all video memory */
+            start = 0;
+            end = s->vram_size;
+        } else {
+            /* Tricky things won't have any effect, i.e. we are in the very simple
+             * (and very usual) case of a linear buffer. */
+            /* use page table dirty bit tracking for the LFB plus border */
+            start = (s->start_addr * 4) & TARGET_PAGE_MASK;
+            end = ((s->start_addr * 4 + height * line_offset) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
+        }
+
+        for (y = 0 ; y < start; y += TARGET_PAGE_SIZE)
+            /* We will not read that anyway. */
+            cpu_physical_memory_set_dirty(s->vram_offset + y);
+
+        qemu_physical_sync_dirty_bitmap(s->lfb_addr + y, end);
+
+        for ( ; y < s->vram_size; y += TARGET_PAGE_SIZE)
+            /* We will not read that anyway. */
+            cpu_physical_memory_set_dirty(s->vram_offset + y);
+    }
+
 #if 0
     printf("w=%d h=%d v=%d line_offset=%d cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%d sr[0x01]=0x%02x\n",
            width, height, v, line_offset, s->cr[9], s->cr[0x17], s->line_compare, s->sr[0x01]);
@@ -1746,6 +1775,10 @@
         return;
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
+
+    /* disable dirty bit tracking */
+    // kvm_log_stop();
+
     if (s->ds->depth == 8)
         val = s->rgb_to_pixel(0, 0, 0);
     else
diff -r a5bcebe9e2bc hw/vga_int.h
--- a/hw/vga_int.h	Wed Nov 05 12:09:13 2008 +0000
+++ b/hw/vga_int.h	Wed Nov 05 14:29:54 2008 +0000
@@ -106,6 +106,10 @@
     unsigned int bios_size;                                             \
     target_phys_addr_t base_ctrl;                                       \
     int it_shift;                                                       \
+    unsigned long lfb_addr;                                             \
+    unsigned long lfb_end;                                              \
+    uint32_t map_addr;                                                  \
+    uint32_t map_end;                                                   \
     PCIDevice *pci_dev;                                                 \
     uint32_t latch;                                                     \
     uint8_t sr_index;                                                   \