Re: [Qemu-devel] vga optmization

[Glauber Costa]

On Wed, Nov 05, 2008 at 02:42:22PM +0000, Stefano Stabellini wrote:
> 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.
I'm not opposed to the changes. I'll redo a patch and resend it today.
However, I'm not planning to include the last optimization you sent.
It is better that we do it in a separate commit. If you can get numbers,
even better. I'll try it myself too.


> 
> 
> 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;                                                   \