Re: [Qemu-devel] [PATCH 3/3 v2] vmware_vga: Allow simple drivers to work without using the fifo

[Paolo Bonzini <pbonzini@redhat.com>]

Il 03/10/2012 11:31, BALATON Zoltan ha scritto:
> Postpone stopping the dirty log to the point where the command fifo is
> configured to allow drivers which don't use the fifo to work too.
> (Without this the picture rendered into the vram never got to the
> screen and the DIRECT_VRAM option meant to support this case was
> removed a year ago.)
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/vga.c        |    2 +-
>  hw/vga_int.h    |    1 +
>  hw/vmware_vga.c |   34 +++++++++++++++++++++++++---------
>  3 files changed, 27 insertions(+), 10 deletions(-)
> 
>  v2: Rebase to apply to current
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index 80299ea..7939a9d 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -1612,7 +1612,7 @@ void vga_invalidate_scanlines(VGACommonState *s,
> int y1, int y2)
>      }
>  }
> 
> -static void vga_sync_dirty_bitmap(VGACommonState *s)
> +void vga_sync_dirty_bitmap(VGACommonState *s)
>  {
>      memory_region_sync_dirty_bitmap(&s->vram);
>  }
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 330a32f..c01d07d 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -196,6 +196,7 @@ MemoryRegion *vga_init_io(VGACommonState *s,
>                            const MemoryRegionPortio **vbe_ports);
>  void vga_common_reset(VGACommonState *s);
> 
> +void vga_sync_dirty_bitmap(VGACommonState *s);
>  void vga_dirty_log_start(VGACommonState *s);
>  void vga_dirty_log_stop(VGACommonState *s);
> 
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 5e4786f..17b3140 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -820,11 +820,10 @@ static void vmsvga_value_write(void *opaque,
> uint32_t address, uint32_t value)
>          break;
> 
>      case SVGA_REG_ENABLE:
> -        s->enable = value;
> -        s->config &= !!value;
> +        s->enable = !!value;
>          s->invalidated = 1;
>          s->vga.invalidate(&s->vga);
> -        if (s->enable) {
> +        if (s->enable && s->config) {
>              vga_dirty_log_stop(&s->vga);
>          } else {
>              vga_dirty_log_start(&s->vga);
> @@ -860,15 +859,19 @@ static void vmsvga_value_write(void *opaque,
> uint32_t address, uint32_t value)
>          if (value) {
>              s->fifo = (uint32_t *) s->fifo_ptr;
>              /* Check range and alignment.  */
> -            if ((CMD(min) | CMD(max) |
> -                        CMD(next_cmd) | CMD(stop)) & 3)
> +            if ((CMD(min) | CMD(max) | CMD(next_cmd) | CMD(stop)) & 3) {
>                  break;
> -            if (CMD(min) < (uint8_t *) s->cmd->fifo - (uint8_t *) s->fifo)
> +            }
> +            if (CMD(min) < (uint8_t *) s->cmd->fifo - (uint8_t *)
> s->fifo) {
>                  break;
> -            if (CMD(max) > SVGA_FIFO_SIZE)
> +            }
> +            if (CMD(max) > SVGA_FIFO_SIZE) {
>                  break;
> -            if (CMD(max) < CMD(min) + 10 * 1024)
> +            }
> +            if (CMD(max) < CMD(min) + 10 * 1024) {
>                  break;
> +            }

Please keep all coding style changes in a separate patch (so you have
one patch for coding style changes in the whole series).

> +            vga_dirty_log_stop(&s->vga);
>          }
>          s->config = !!value;
>          break;
> @@ -949,6 +952,8 @@ static inline void vmsvga_check_size(struct
> vmsvga_state_s *s)
>  static void vmsvga_update_display(void *opaque)
>  {
>      struct vmsvga_state_s *s = opaque;
> +    bool dirty = false;
> +
>      if (!s->enable) {
>          s->vga.update(&s->vga);
>          return;
> @@ -963,13 +968,24 @@ static void vmsvga_update_display(void *opaque)
>       * Is it more efficient to look at vram VGA-dirty bits or wait
>       * for the driver to issue SVGA_CMD_UPDATE?
>       */
> -    if (s->invalidated) {
> +    if (memory_region_is_logging(&s->vga.vram)) {
> +        vga_sync_dirty_bitmap(&s->vga);
> +        dirty = memory_region_get_dirty(&s->vga.vram, 0,
> +            ds_get_linesize(s->vga.ds) * ds_get_height(s->vga.ds),
> +            DIRTY_MEMORY_VGA);
> +    }
> +    if (s->invalidated || dirty) {
>          s->invalidated = 0;
>          memcpy(ds_get_data(s->vga.ds), s->vga.vram_ptr,
>                 ds_get_linesize(s->vga.ds) * ds_get_height(s->vga.ds));
>          dpy_update(s->vga.ds, 0, 0,
>                 ds_get_width(s->vga.ds), ds_get_height(s->vga.ds));
>      }
> +    if (dirty) {
> +        memory_region_reset_dirty(&s->vga.vram, 0,
> +            ds_get_linesize(s->vga.ds) * ds_get_height(s->vga.ds),
> +            DIRTY_MEMORY_VGA);
> +    }
>  }
> 
>  static void vmsvga_reset(DeviceState *dev)

Otherwise looks good.

Paolo