[Qemu-devel] [PATCH 00/10] make display updates thread safe.

[Qemu-devel] [PATCH 00/10] make display updates thread safe.

[Gerd Hoffmann]

  Hi,

Second round.  Helper API should be solid now, after patch discussions
and initial testing.  API is documented now in the header file.
Additionally to vga the sparc display adapters (cg3, tcx) are converted
too.  Still quite some work until we've got them all converted.  Still
enough progress that I think posting the new revision is useful.

cheers,
  Gerd

Gerd Hoffmann (5):
  bitmap: add bitmap_copy_and_clear_atomic
  memory: add support getting and using a dirty bitmap copy.
  vga: add vga_scanline_invalidated helper
  vga: make display updates thread safe.
  [testing] console: remove do_safe_dpy_refresh

Mark Cave-Ayland (5):
  cg3: remove TARGET_PAGE_SIZE rounding on dirty page detection
  cg3: fix up size parameter for memory_region_get_dirty()
  cg3: make display updates thread safe
  tcx: introduce tcx_check_dirty() function
  tcx: make display updates thread safe

 include/exec/memory.h   | 47 +++++++++++++++++++++++++++
 include/exec/ram_addr.h |  7 ++++
 include/qemu/bitmap.h   |  2 ++
 include/qemu/typedefs.h |  1 +
 exec.c                  | 75 ++++++++++++++++++++++++++++++++++++++++++
 hw/display/cg3.c        | 40 +++++++++++------------
 hw/display/tcx.c        | 86 ++++++++++++++++++-------------------------------
 hw/display/vga.c        | 50 +++++++++++++++-------------
 memory.c                | 17 ++++++++++
 ui/console.c            | 25 +-------------
 util/bitmap.c           | 11 +++++++
 11 files changed, 241 insertions(+), 120 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 01/10] bitmap: add bitmap_copy_and_clear_atomic

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qemu/bitmap.h |  2 ++
 util/bitmap.c         | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 63ea2d0..c318da1 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -220,6 +220,8 @@ void bitmap_set(unsigned long *map, long i, long len);
 void bitmap_set_atomic(unsigned long *map, long i, long len);
 void bitmap_clear(unsigned long *map, long start, long nr);
 bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr);
+void bitmap_copy_and_clear_atomic(unsigned long *dst, unsigned long *src,
+                                  long nr);
 unsigned long bitmap_find_next_zero_area(unsigned long *map,
                                          unsigned long size,
                                          unsigned long start,
diff --git a/util/bitmap.c b/util/bitmap.c
index c1a84ca..efced9a 100644
--- a/util/bitmap.c
+++ b/util/bitmap.c
@@ -287,6 +287,17 @@ bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
     return dirty != 0;
 }
 
+void bitmap_copy_and_clear_atomic(unsigned long *dst, unsigned long *src,
+                                  long nr)
+{
+    while (nr > 0) {
+        *dst = atomic_xchg(src, 0);
+        dst++;
+        src++;
+        nr -= BITS_PER_LONG;
+    }
+}
+
 #define ALIGN_MASK(x,mask)      (((x)+(mask))&~(mask))
 
 /**
-- 
2.9.3

[Qemu-devel] [PATCH 02/10] memory: add support getting and using a dirty bitmap copy.

[Gerd Hoffmann]

This patch adds support for getting and using a local copy of the dirty
bitmap.

memory_region_copy_and_clear_dirty() will create a copy of the dirty
bitmap for the specified range, clear the dirty bitmap and return the
copy.  The returned bitmap can be a bit larger than requested, the range
is expanded so the code can copy unsigned longs from the bitmap and
avoid atomic bit update operations.

memory_region_copy_get_dirty() will return the dirty status of pages,
pretty much like memory_region_get_dirty(), but using the copy returned
by memory_region_copy_and_clear_dirty().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/exec/memory.h   | 47 +++++++++++++++++++++++++++++++
 include/exec/ram_addr.h |  7 +++++
 include/qemu/typedefs.h |  1 +
 exec.c                  | 75 +++++++++++++++++++++++++++++++++++++++++++++++++
 memory.c                | 17 +++++++++++
 5 files changed, 147 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e39256a..2d82983 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -871,6 +871,53 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr,
  */
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                                         hwaddr size, unsigned client);
+
+/**
+ * memory_region_snapshot_and_clear_dirty: Get a snapshot of the dirty
+ *                                         bitmap and clear it.
+ *
+ * Creates a snapshot of the dirty bitmap, clears the dirty bitmap and
+ * returns the snapshot.  The snapshot can then be used to query dirty
+ * status, using memory_region_snapshot_get_dirty.  Unlike
+ * memory_region_test_and_clear_dirty this allows to query the same
+ * page multiple times, which is especially useful for display updates
+ * where the scanlines often are not page aligned.
+ *
+ * The dirty bitmap region which gets copyed into the snapshot (and
+ * cleared afterwards) can be larger than requested.  The boundaries
+ * are rounded up/down so complete bitmap longs (covering 64 pages on
+ * 64bit hosts) can be copied over into the bitmap snapshot.  Which
+ * isn't a problem for display updates as the extra pages are outside
+ * the visible area, and in case the visible area changes a full
+ * display redraw is due anyway.  Should other use cases for this
+ * function emerge we might have to revisit this implementation
+ * detail.
+ *
+ * Use g_free to release DirtyBitmapSnapshot.
+ *
+ * @mr: the memory region being queried.
+ * @addr: the address (relative to the start of the region) being queried.
+ * @size: the size of the range being queried.
+ * @client: the user of the logging information; typically %DIRTY_MEMORY_VGA.
+ */
+DirtyBitmapSnapshot *memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
+                                                            hwaddr addr,
+                                                            hwaddr size,
+                                                            unsigned client);
+
+/**
+ * memory_region_snapshot_get_dirty: Check whether a range of bytes is dirty
+ *                                   in the specified dirty bitmap snapshot.
+ *
+ * @mr: the memory region being queried.
+ * @snap: the dirty bitmap snapshot
+ * @addr: the address (relative to the start of the region) being queried.
+ * @size: the size of the range being queried.
+ */
+bool memory_region_snapshot_get_dirty(MemoryRegion *mr,
+                                      DirtyBitmapSnapshot *snap,
+                                      hwaddr addr, hwaddr size);
+
 /**
  * memory_region_sync_dirty_bitmap: Synchronize a region's dirty bitmap with
  *                                  any external TLBs (e.g. kvm)
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index b05dc84..2b63d7f 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -343,6 +343,13 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                               ram_addr_t length,
                                               unsigned client);
 
+DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
+    (ram_addr_t start, ram_addr_t length, unsigned client);
+
+bool cpu_physical_memory_snapshot_get_dirty(DirtyBitmapSnapshot *snap,
+                                            ram_addr_t start,
+                                            ram_addr_t length);
+
 static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
                                                          ram_addr_t length)
 {
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index e95f28c..f08d327 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -23,6 +23,7 @@ typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct CPUState CPUState;
 typedef struct DeviceListener DeviceListener;
 typedef struct DeviceState DeviceState;
+typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
 typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DisplayState DisplayState;
 typedef struct DisplaySurface DisplaySurface;
diff --git a/exec.c b/exec.c
index e57a8a2..0b654b2 100644
--- a/exec.c
+++ b/exec.c
@@ -223,6 +223,12 @@ struct CPUAddressSpace {
     MemoryListener tcg_as_listener;
 };
 
+struct DirtyBitmapSnapshot {
+    ram_addr_t start;
+    ram_addr_t end;
+    unsigned long dirty[];
+};
+
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
@@ -1061,6 +1067,75 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
     return dirty;
 }
 
+DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
+     (ram_addr_t start, ram_addr_t length, unsigned client)
+{
+    DirtyMemoryBlocks *blocks;
+    unsigned long align = 1UL << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
+    ram_addr_t first = QEMU_ALIGN_DOWN(start, align);
+    ram_addr_t last  = QEMU_ALIGN_UP(start + length, align);
+    DirtyBitmapSnapshot *snap;
+    unsigned long page, end, dest;
+
+    snap = g_malloc0(sizeof(*snap) +
+                     ((last - first) >> (TARGET_PAGE_BITS + 3)));
+    snap->start = first;
+    snap->end   = last;
+
+    page = first >> TARGET_PAGE_BITS;
+    end  = last  >> TARGET_PAGE_BITS;
+    dest = 0;
+
+    rcu_read_lock();
+
+    blocks = atomic_rcu_read(&ram_list.dirty_memory[client]);
+
+    while (page < end) {
+        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
+
+        assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
+        assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
+        offset >>= BITS_PER_LEVEL;
+
+        bitmap_copy_and_clear_atomic(snap->dirty + dest,
+                                     blocks->blocks[idx] + offset,
+                                     num);
+        page += num;
+        dest += num >> BITS_PER_LEVEL;
+    }
+
+    rcu_read_unlock();
+
+    if (tcg_enabled()) {
+        tlb_reset_dirty_range_all(start, length);
+    }
+
+    return snap;
+}
+
+bool cpu_physical_memory_snapshot_get_dirty(DirtyBitmapSnapshot *snap,
+                                            ram_addr_t start,
+                                            ram_addr_t length)
+{
+    unsigned long page, end;
+
+    assert(start >= snap->start);
+    assert(start + length <= snap->end);
+
+    end = TARGET_PAGE_ALIGN(start + length - snap->start) >> TARGET_PAGE_BITS;
+    page = (start - snap->start) >> TARGET_PAGE_BITS;
+
+    while (page < end) {
+        if (test_bit(page, snap->dirty)) {
+            return true;
+        }
+        page++;
+    }
+    return false;
+}
+
 /* Called from RCU critical section */
 hwaddr memory_region_section_get_iotlb(CPUState *cpu,
                                        MemoryRegionSection *section,
diff --git a/memory.c b/memory.c
index 4c95aaf..8a06485 100644
--- a/memory.c
+++ b/memory.c
@@ -1716,6 +1716,23 @@ bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr,
                 memory_region_get_ram_addr(mr) + addr, size, client);
 }
 
+DirtyBitmapSnapshot *memory_region_snapshot_and_clear_dirty(MemoryRegion *mr,
+                                                            hwaddr addr,
+                                                            hwaddr size,
+                                                            unsigned client)
+{
+    assert(mr->ram_block);
+    return cpu_physical_memory_snapshot_and_clear_dirty(
+                memory_region_get_ram_addr(mr) + addr, size, client);
+}
+
+bool memory_region_snapshot_get_dirty(MemoryRegion *mr, DirtyBitmapSnapshot *snap,
+                                      hwaddr addr, hwaddr size)
+{
+    assert(mr->ram_block);
+    return cpu_physical_memory_snapshot_get_dirty(snap,
+                memory_region_get_ram_addr(mr) + addr, size);
+}
 
 void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
 {
-- 
2.9.3

[Qemu-devel] [PATCH 03/10] vga: add vga_scanline_invalidated helper

[Gerd Hoffmann]

Add vga_scanline_invalidated helper to check whenever a scanline was
invalidated.  Add a sanity check to fix OOB read access for display
heights larger than 2048.

Only cirrus uses this, for hardware cursor rendering, so having this
work properly for the first 2048 scanlines only shouldn't be a problem
as the cirrus can't handle large resolutions anyway.  Also changing the
invalidated_y_table size would break live migration.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..f320946 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1434,6 +1434,14 @@ void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2)
     }
 }
 
+static bool vga_scanline_invalidated(VGACommonState *s, int y)
+{
+    if (y >= VGA_MAX_HEIGHT) {
+        return false;
+    }
+    return s->invalidated_y_table[y >> 5] |= 1 << (y & 0x1f);
+}
+
 void vga_sync_dirty_bitmap(VGACommonState *s)
 {
     memory_region_sync_dirty_bitmap(&s->vram);
@@ -1638,8 +1646,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         page1 = addr + bwidth - 1;
         update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
                                           DIRTY_MEMORY_VGA);
-        /* explicit invalidation for the hardware cursor */
-        update |= (s->invalidated_y_table[y >> 5] >> (y & 0x1f)) & 1;
+        /* explicit invalidation for the hardware cursor (cirrus only) */
+        update |= vga_scanline_invalidated(s, y);
         if (update) {
             if (y_start < 0)
                 y_start = y;
@@ -1686,7 +1694,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
                                   page_max - page_min,
                                   DIRTY_MEMORY_VGA);
     }
-    memset(s->invalidated_y_table, 0, ((height + 31) >> 5) * 4);
+    memset(s->invalidated_y_table, 0, sizeof(s->invalidated_y_table));
 }
 
 static void vga_draw_blank(VGACommonState *s, int full_update)
-- 
2.9.3

[Qemu-devel] [PATCH 04/10] vga: make display updates thread safe.

[Gerd Hoffmann]

The vga code clears the dirty bits *after* reading the framebuffer
memory.  So if the guest framebuffer updates hits the race window
between vga reading the framebuffer and vga clearing the dirty bits
vga will miss that update

Fix it by using the new memory_region_copy_and_clear_dirty()
memory_region_copy_get_dirty() functions.  That way we clear the
dirty bitmap before reading the framebuffer.  Any guest display
updates happening in parallel will be properly tracked in the
dirty bitmap then and the next display refresh will pick them up.

Problem triggers with mttcg only.  Before mttcg was merged tcg
never ran in parallel to vga emulation.  Using kvm will hide the
problem too, due to qemu operating on a userspace copy of the
kernel's dirty bitmap.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index f320946..9bb71f1 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1465,7 +1465,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     DisplaySurface *surface = qemu_console_surface(s->con);
     int y1, y, update, linesize, y_start, double_scan, mask, depth;
     int width, height, shift_control, line_offset, bwidth, bits;
-    ram_addr_t page0, page1, page_min, page_max;
+    ram_addr_t page0, page1;
+    DirtyBitmapSnapshot *snap = NULL;
     int disp_width, multi_scan, multi_run;
     uint8_t *d;
     uint32_t v, addr1, addr;
@@ -1480,9 +1481,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
 
     full_update |= update_basic_params(s);
 
-    if (!full_update)
-        vga_sync_dirty_bitmap(s);
-
     s->get_resolution(s, &width, &height);
     disp_width = width;
 
@@ -1625,11 +1623,17 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     addr1 = (s->start_addr * 4);
     bwidth = (width * bits + 7) / 8;
     y_start = -1;
-    page_min = -1;
-    page_max = 0;
     d = surface_data(surface);
     linesize = surface_stride(surface);
     y1 = 0;
+
+    if (!full_update) {
+        vga_sync_dirty_bitmap(s);
+        snap = memory_region_snapshot_and_clear_dirty(&s->vram, addr1,
+                                                      bwidth * height,
+                                                      DIRTY_MEMORY_VGA);
+    }
+
     for(y = 0; y < height; y++) {
         addr = addr1;
         if (!(s->cr[VGA_CRTC_MODE] & 1)) {
@@ -1644,17 +1648,17 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         update = full_update;
         page0 = addr;
         page1 = addr + bwidth - 1;
-        update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
-                                          DIRTY_MEMORY_VGA);
+        if (full_update) {
+            update = 1;
+        } else {
+            update = memory_region_snapshot_get_dirty(&s->vram, snap,
+                                                      page0, page1 - page0);
+        }
         /* explicit invalidation for the hardware cursor (cirrus only) */
         update |= vga_scanline_invalidated(s, y);
         if (update) {
             if (y_start < 0)
                 y_start = y;
-            if (page0 < page_min)
-                page_min = page0;
-            if (page1 > page_max)
-                page_max = page1;
             if (!(is_buffer_shared(surface))) {
                 vga_draw_line(s, d, s->vram_ptr + addr, width);
                 if (s->cursor_draw_line)
@@ -1687,13 +1691,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         dpy_gfx_update(s->con, 0, y_start,
                        disp_width, y - y_start);
     }
-    /* reset modified pages */
-    if (page_max >= page_min) {
-        memory_region_reset_dirty(&s->vram,
-                                  page_min,
-                                  page_max - page_min,
-                                  DIRTY_MEMORY_VGA);
-    }
+    g_free(snap);
     memset(s->invalidated_y_table, 0, sizeof(s->invalidated_y_table));
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH 05/10] cg3: remove TARGET_PAGE_SIZE rounding on dirty page detection

[Gerd Hoffmann]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

This was an artifact from very early versions of the code from before the
memory API and is no longer needed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 1174220..e05ca92 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -114,7 +114,7 @@ static void cg3_update_display(void *opaque)
     for (y = 0; y < height; y++) {
         int update = s->full_update;
 
-        page = (y * width) & TARGET_PAGE_MASK;
+        page = y * width;
         update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
                                           DIRTY_MEMORY_VGA);
         if (update) {
@@ -148,8 +148,7 @@ static void cg3_update_display(void *opaque)
     }
     if (page_max >= page_min) {
         memory_region_reset_dirty(&s->vram_mem,
-                              page_min, page_max - page_min + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
+                              page_min, page_max - page_min, DIRTY_MEMORY_VGA);
     }
     /* vsync interrupt? */
     if (s->regs[0] & CG3_CR_ENABLE_INTS) {
-- 
2.9.3

[Qemu-devel] [PATCH 06/10] cg3: fix up size parameter for memory_region_get_dirty()

[Gerd Hoffmann]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

The code was incorrectly calculating the end address rather than the size of
the required region.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index e05ca92..6e19da0 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -115,7 +115,7 @@ static void cg3_update_display(void *opaque)
         int update = s->full_update;
 
         page = y * width;
-        update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
+        update |= memory_region_get_dirty(&s->vram_mem, page, width - 1,
                                           DIRTY_MEMORY_VGA);
         if (update) {
             if (y_start < 0) {
-- 
2.9.3

[Qemu-devel] [PATCH 07/10] cg3: make display updates thread safe

[Gerd Hoffmann]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 6e19da0..9996567 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -95,7 +95,8 @@ static void cg3_update_display(void *opaque)
     uint32_t dval;
     int x, y, y_start;
     unsigned int width, height;
-    ram_addr_t page, page_min, page_max;
+    ram_addr_t page;
+    DirtyBitmapSnapshot *snap = NULL;
 
     if (surface_bits_per_pixel(surface) != 32) {
         return;
@@ -104,29 +105,32 @@ static void cg3_update_display(void *opaque)
     height = s->height;
 
     y_start = -1;
-    page_min = -1;
-    page_max = 0;
-    page = 0;
     pix = memory_region_get_ram_ptr(&s->vram_mem);
     data = (uint32_t *)surface_data(surface);
 
-    memory_region_sync_dirty_bitmap(&s->vram_mem);
+    if (!s->full_update) {
+        memory_region_sync_dirty_bitmap(&s->vram_mem);
+        snap = memory_region_snapshot_and_clear_dirty(&s->vram_mem, 0x0,
+                                                      memory_region_size(&s->vram_mem),
+                                                      DIRTY_MEMORY_VGA);
+    }
+    
     for (y = 0; y < height; y++) {
-        int update = s->full_update;
+        int update;
 
         page = y * width;
-        update |= memory_region_get_dirty(&s->vram_mem, page, width - 1,
-                                          DIRTY_MEMORY_VGA);
+
+        if (s->full_update) {
+            update = 1;
+        } else {
+            update = memory_region_snapshot_get_dirty(&s->vram_mem, snap, page,
+                                                      width);
+        }
+
         if (update) {
             if (y_start < 0) {
                 y_start = y;
             }
-            if (page < page_min) {
-                page_min = page;
-            }
-            if (page > page_max) {
-                page_max = page;
-            }
 
             for (x = 0; x < width; x++) {
                 dval = *pix++;
@@ -146,15 +150,12 @@ static void cg3_update_display(void *opaque)
     if (y_start >= 0) {
         dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
     }
-    if (page_max >= page_min) {
-        memory_region_reset_dirty(&s->vram_mem,
-                              page_min, page_max - page_min, DIRTY_MEMORY_VGA);
-    }
     /* vsync interrupt? */
     if (s->regs[0] & CG3_CR_ENABLE_INTS) {
         s->regs[1] |= CG3_SR_PENDING_INT;
         qemu_irq_raise(s->irq);
     }
+    g_free(snap);
 }
 
 static void cg3_invalidate_display(void *opaque)
-- 
2.9.3

[Qemu-devel] [PATCH 08/10] tcx: introduce tcx_check_dirty() function

[Gerd Hoffmann]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

This is similar to the existing tcx24_check_dirty() function and will aid
with the transition to thread-safe updates.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 8e26aae..15172f0 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -98,6 +98,12 @@ static void tcx_set_dirty(TCXState *s)
     memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
 }
 
+static inline int tcx_check_dirty(TCXState *s, ram_addr_t page)
+{
+    return memory_region_get_dirty(&s->vram_mem, page, TARGET_PAGE_SIZE,
+                                   DIRTY_MEMORY_VGA);
+}
+
 static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
                                     ram_addr_t page24, ram_addr_t cpage)
 {
@@ -359,8 +365,7 @@ static void tcx_update_display(void *opaque)
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
     for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
-        if (memory_region_get_dirty(&ts->vram_mem, page, TARGET_PAGE_SIZE,
-                                    DIRTY_MEMORY_VGA)) {
+        if (tcx_check_dirty(ts, page)) {
             if (y_start < 0)
                 y_start = y;
             if (page < page_min)
-- 
2.9.3

[Qemu-devel] [PATCH 09/10] tcx: make display updates thread safe

[Gerd Hoffmann]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c | 85 +++++++++++++++++++-------------------------------------
 1 file changed, 29 insertions(+), 56 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 15172f0..a890e49 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -98,44 +98,28 @@ static void tcx_set_dirty(TCXState *s)
     memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
 }
 
-static inline int tcx_check_dirty(TCXState *s, ram_addr_t page)
+static inline int tcx_check_dirty(TCXState *s, DirtyBitmapSnapshot *snap,
+                                  ram_addr_t page)
 {
-    return memory_region_get_dirty(&s->vram_mem, page, TARGET_PAGE_SIZE,
-                                   DIRTY_MEMORY_VGA);
+    return memory_region_snapshot_get_dirty(&s->vram_mem, snap, page,
+                                            TARGET_PAGE_SIZE - 1);
 }
 
-static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
-                                    ram_addr_t page24, ram_addr_t cpage)
+static inline int tcx24_check_dirty(TCXState *s, DirtyBitmapSnapshot *snap,
+                                    ram_addr_t page, ram_addr_t page24,
+                                    ram_addr_t cpage)
 {
     int ret;
 
-    ret = memory_region_get_dirty(&s->vram_mem, page, TARGET_PAGE_SIZE,
-                                  DIRTY_MEMORY_VGA);
-    ret |= memory_region_get_dirty(&s->vram_mem, page24, TARGET_PAGE_SIZE * 4,
-                                   DIRTY_MEMORY_VGA);
-    ret |= memory_region_get_dirty(&s->vram_mem, cpage, TARGET_PAGE_SIZE * 4,
-                                   DIRTY_MEMORY_VGA);
+    ret = memory_region_snapshot_get_dirty(&s->vram_mem, snap, page,
+                                           TARGET_PAGE_SIZE);
+    ret |= memory_region_snapshot_get_dirty(&s->vram_mem, snap, page24,
+                                            TARGET_PAGE_SIZE * 4);
+    ret |= memory_region_snapshot_get_dirty(&s->vram_mem, snap, cpage,
+                                            TARGET_PAGE_SIZE * 4);
     return ret;
 }
 
-static inline void tcx24_reset_dirty(TCXState *ts, ram_addr_t page_min,
-                               ram_addr_t page_max, ram_addr_t page24,
-                              ram_addr_t cpage)
-{
-    memory_region_reset_dirty(&ts->vram_mem,
-                              page_min,
-                              (page_max - page_min) + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
-    memory_region_reset_dirty(&ts->vram_mem,
-                              page24 + page_min * 4,
-                              (page_max - page_min) * 4 + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
-    memory_region_reset_dirty(&ts->vram_mem,
-                              cpage + page_min * 4,
-                              (page_max - page_min) * 4 + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
-}
-
 static void update_palette_entries(TCXState *s, int start, int end)
 {
     DisplaySurface *surface = qemu_console_surface(s->con);
@@ -325,7 +309,8 @@ static void tcx_update_display(void *opaque)
 {
     TCXState *ts = opaque;
     DisplaySurface *surface = qemu_console_surface(ts->con);
-    ram_addr_t page, page_min, page_max;
+    ram_addr_t page;
+    DirtyBitmapSnapshot *snap = NULL;
     int y, y_start, dd, ds;
     uint8_t *d, *s;
     void (*f)(TCXState *s1, uint8_t *dst, const uint8_t *src, int width);
@@ -337,8 +322,6 @@ static void tcx_update_display(void *opaque)
 
     page = 0;
     y_start = -1;
-    page_min = -1;
-    page_max = 0;
     d = surface_data(surface);
     s = ts->vram;
     dd = surface_stride(surface);
@@ -364,14 +347,14 @@ static void tcx_update_display(void *opaque)
     }
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
+    snap = memory_region_snapshot_and_clear_dirty(&ts->vram_mem, 0x0,
+                                                  memory_region_size(&ts->vram_mem),
+                                                  DIRTY_MEMORY_VGA);
+
     for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
-        if (tcx_check_dirty(ts, page)) {
+        if (tcx_check_dirty(ts, snap, page)) {
             if (y_start < 0)
                 y_start = y;
-            if (page < page_min)
-                page_min = page;
-            if (page > page_max)
-                page_max = page;
 
             f(ts, d, s, ts->width);
             if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
@@ -421,20 +404,15 @@ static void tcx_update_display(void *opaque)
         dpy_gfx_update(ts->con, 0, y_start,
                        ts->width, y - y_start);
     }
-    /* reset modified pages */
-    if (page_max >= page_min) {
-        memory_region_reset_dirty(&ts->vram_mem,
-                                  page_min,
-                                  (page_max - page_min) + TARGET_PAGE_SIZE,
-                                  DIRTY_MEMORY_VGA);
-    }
+    g_free(snap);
 }
 
 static void tcx24_update_display(void *opaque)
 {
     TCXState *ts = opaque;
     DisplaySurface *surface = qemu_console_surface(ts->con);
-    ram_addr_t page, page_min, page_max, cpage, page24;
+    ram_addr_t page, cpage, page24;
+    DirtyBitmapSnapshot *snap = NULL;
     int y, y_start, dd, ds;
     uint8_t *d, *s;
     uint32_t *cptr, *s24;
@@ -447,8 +425,6 @@ static void tcx24_update_display(void *opaque)
     page24 = ts->vram24_offset;
     cpage = ts->cplane_offset;
     y_start = -1;
-    page_min = -1;
-    page_max = 0;
     d = surface_data(surface);
     s = ts->vram;
     s24 = ts->vram24;
@@ -457,15 +433,15 @@ static void tcx24_update_display(void *opaque)
     ds = 1024;
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
+    snap = memory_region_snapshot_and_clear_dirty(&ts->vram_mem, 0x0,
+                                                  memory_region_size(&ts->vram_mem),
+                                                  DIRTY_MEMORY_VGA);
+
     for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE,
             page24 += TARGET_PAGE_SIZE, cpage += TARGET_PAGE_SIZE) {
-        if (tcx24_check_dirty(ts, page, page24, cpage)) {
+        if (tcx24_check_dirty(ts, snap, page, page24, cpage)) {
             if (y_start < 0)
                 y_start = y;
-            if (page < page_min)
-                page_min = page;
-            if (page > page_max)
-                page_max = page;
             tcx24_draw_line32(ts, d, s, ts->width, cptr, s24);
             if (y >= ts->cursy && y < ts->cursy+32 && ts->cursx < ts->width) {
                 tcx_draw_cursor32(ts, d, y, ts->width);
@@ -521,10 +497,7 @@ static void tcx24_update_display(void *opaque)
         dpy_gfx_update(ts->con, 0, y_start,
                        ts->width, y - y_start);
     }
-    /* reset modified pages */
-    if (page_max >= page_min) {
-        tcx24_reset_dirty(ts, page_min, page_max, page24, cpage);
-    }
+    g_free(snap);
 }
 
 static void tcx_invalidate_display(void *opaque)
-- 
2.9.3

[Qemu-devel] [PATCH 10/10] [testing] console: remove do_safe_dpy_refresh

[Gerd Hoffmann]

Drop the temporary workaround for the broken display updates.

TODO: before actually merging this we have to fix all display
      adapters ...

Converted so far: vga, cg3, tcx.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 419b098..4c70d8b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1575,36 +1575,13 @@ bool dpy_gfx_check_format(QemuConsole *con,
     return true;
 }
 
-/*
- * Safe DPY refresh for TCG guests. We use the exclusive mechanism to
- * ensure the TCG vCPUs are quiescent so we can avoid races between
- * dirty page tracking for direct frame-buffer access by the guest.
- *
- * This is a temporary stopgap until we've fixed the dirty tracking
- * races in display adapters.
- */
-static void do_safe_dpy_refresh(DisplayChangeListener *dcl)
-{
-    qemu_mutex_unlock_iothread();
-    start_exclusive();
-    qemu_mutex_lock_iothread();
-    dcl->ops->dpy_refresh(dcl);
-    qemu_mutex_unlock_iothread();
-    end_exclusive();
-    qemu_mutex_lock_iothread();
-}
-
 static void dpy_refresh(DisplayState *s)
 {
     DisplayChangeListener *dcl;
 
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (dcl->ops->dpy_refresh) {
-            if (tcg_enabled()) {
-                do_safe_dpy_refresh(dcl);
-            } else {
-                dcl->ops->dpy_refresh(dcl);
-            }
+            dcl->ops->dpy_refresh(dcl);
         }
     }
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 02/10] memory: add support getting and using a dirty bitmap copy.

[Paolo Bonzini]

On 04/04/2017 12:23, Gerd Hoffmann wrote:
> 
> memory_region_copy_and_clear_dirty() will create a copy of the dirty
> bitmap for the specified range, clear the dirty bitmap and return the
> copy.  The returned bitmap can be a bit larger than requested, the range
> is expanded so the code can copy unsigned longs from the bitmap and
> avoid atomic bit update operations.
> 
> memory_region_copy_get_dirty() will return the dirty status of pages,
> pretty much like memory_region_get_dirty(), but using the copy returned
> by memory_region_copy_and_clear_dirty().

Old names in the commit message, otherwise looks good.

This will also help KVM by the way, because there are changes in the
pipeline to limit the amount of work done by log_sync and process dirty
pages periodically in the VCPU thread.

Paolo

Re: [Qemu-devel] [PATCH 06/10] cg3: fix up size parameter for memory_region_get_dirty()

[Mark Cave-Ayland]

On 04/04/17 11:23, Gerd Hoffmann wrote:

> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> The code was incorrectly calculating the end address rather than the size of
> the required region.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/cg3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index e05ca92..6e19da0 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -115,7 +115,7 @@ static void cg3_update_display(void *opaque)
>          int update = s->full_update;
>  
>          page = y * width;
> -        update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
> +        update |= memory_region_get_dirty(&s->vram_mem, page, width - 1,
>                                            DIRTY_MEMORY_VGA);
>          if (update) {
>              if (y_start < 0) {
> 

The size here should actually be "width" rather than "width - 1" here
(this patch was originally written when the assert()s for checking the
start/end of the region size were off-by-one).


ATB,

Mark.

Re: [Qemu-devel] [PATCH 09/10] tcx: make display updates thread safe

[Mark Cave-Ayland]

On 04/04/17 11:23, Gerd Hoffmann wrote:

> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/tcx.c | 85 +++++++++++++++++++-------------------------------------
>  1 file changed, 29 insertions(+), 56 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 15172f0..a890e49 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -98,44 +98,28 @@ static void tcx_set_dirty(TCXState *s)
>      memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
>  }
>  
> -static inline int tcx_check_dirty(TCXState *s, ram_addr_t page)
> +static inline int tcx_check_dirty(TCXState *s, DirtyBitmapSnapshot *snap,
> +                                  ram_addr_t page)
>  {
> -    return memory_region_get_dirty(&s->vram_mem, page, TARGET_PAGE_SIZE,
> -                                   DIRTY_MEMORY_VGA);
> +    return memory_region_snapshot_get_dirty(&s->vram_mem, snap, page,
> +                                            TARGET_PAGE_SIZE - 1);
>  }

Again the patch preceding this is correct, however this should be left
as "TARGET_PAGE_SIZE" rather than "TARGET_PAGE SIZE - 1" since this was
also before the assert() fixes.

> -static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
> -                                    ram_addr_t page24, ram_addr_t cpage)
> +static inline int tcx24_check_dirty(TCXState *s, DirtyBitmapSnapshot *snap,
> +                                    ram_addr_t page, ram_addr_t page24,
> +                                    ram_addr_t cpage)
>  {
>      int ret;
>  
> -    ret = memory_region_get_dirty(&s->vram_mem, page, TARGET_PAGE_SIZE,
> -                                  DIRTY_MEMORY_VGA);
> -    ret |= memory_region_get_dirty(&s->vram_mem, page24, TARGET_PAGE_SIZE * 4,
> -                                   DIRTY_MEMORY_VGA);
> -    ret |= memory_region_get_dirty(&s->vram_mem, cpage, TARGET_PAGE_SIZE * 4,
> -                                   DIRTY_MEMORY_VGA);
> +    ret = memory_region_snapshot_get_dirty(&s->vram_mem, snap, page,
> +                                           TARGET_PAGE_SIZE);
> +    ret |= memory_region_snapshot_get_dirty(&s->vram_mem, snap, page24,
> +                                            TARGET_PAGE_SIZE * 4);
> +    ret |= memory_region_snapshot_get_dirty(&s->vram_mem, snap, cpage,
> +                                            TARGET_PAGE_SIZE * 4);
>      return ret;
>  }
>  
> -static inline void tcx24_reset_dirty(TCXState *ts, ram_addr_t page_min,
> -                               ram_addr_t page_max, ram_addr_t page24,
> -                              ram_addr_t cpage)
> -{
> -    memory_region_reset_dirty(&ts->vram_mem,
> -                              page_min,
> -                              (page_max - page_min) + TARGET_PAGE_SIZE,
> -                              DIRTY_MEMORY_VGA);
> -    memory_region_reset_dirty(&ts->vram_mem,
> -                              page24 + page_min * 4,
> -                              (page_max - page_min) * 4 + TARGET_PAGE_SIZE,
> -                              DIRTY_MEMORY_VGA);
> -    memory_region_reset_dirty(&ts->vram_mem,
> -                              cpage + page_min * 4,
> -                              (page_max - page_min) * 4 + TARGET_PAGE_SIZE,
> -                              DIRTY_MEMORY_VGA);
> -}
> -
>  static void update_palette_entries(TCXState *s, int start, int end)
>  {
>      DisplaySurface *surface = qemu_console_surface(s->con);
> @@ -325,7 +309,8 @@ static void tcx_update_display(void *opaque)
>  {
>      TCXState *ts = opaque;
>      DisplaySurface *surface = qemu_console_surface(ts->con);
> -    ram_addr_t page, page_min, page_max;
> +    ram_addr_t page;
> +    DirtyBitmapSnapshot *snap = NULL;
>      int y, y_start, dd, ds;
>      uint8_t *d, *s;
>      void (*f)(TCXState *s1, uint8_t *dst, const uint8_t *src, int width);
> @@ -337,8 +322,6 @@ static void tcx_update_display(void *opaque)
>  
>      page = 0;
>      y_start = -1;
> -    page_min = -1;
> -    page_max = 0;
>      d = surface_data(surface);
>      s = ts->vram;
>      dd = surface_stride(surface);
> @@ -364,14 +347,14 @@ static void tcx_update_display(void *opaque)
>      }
>  
>      memory_region_sync_dirty_bitmap(&ts->vram_mem);
> +    snap = memory_region_snapshot_and_clear_dirty(&ts->vram_mem, 0x0,
> +                                                  memory_region_size(&ts->vram_mem),
> +                                                  DIRTY_MEMORY_VGA);
> +
>      for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
> -        if (tcx_check_dirty(ts, page)) {
> +        if (tcx_check_dirty(ts, snap, page)) {
>              if (y_start < 0)
>                  y_start = y;
> -            if (page < page_min)
> -                page_min = page;
> -            if (page > page_max)
> -                page_max = page;
>  
>              f(ts, d, s, ts->width);
>              if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
> @@ -421,20 +404,15 @@ static void tcx_update_display(void *opaque)
>          dpy_gfx_update(ts->con, 0, y_start,
>                         ts->width, y - y_start);
>      }
> -    /* reset modified pages */
> -    if (page_max >= page_min) {
> -        memory_region_reset_dirty(&ts->vram_mem,
> -                                  page_min,
> -                                  (page_max - page_min) + TARGET_PAGE_SIZE,
> -                                  DIRTY_MEMORY_VGA);
> -    }
> +    g_free(snap);
>  }
>  
>  static void tcx24_update_display(void *opaque)
>  {
>      TCXState *ts = opaque;
>      DisplaySurface *surface = qemu_console_surface(ts->con);
> -    ram_addr_t page, page_min, page_max, cpage, page24;
> +    ram_addr_t page, cpage, page24;
> +    DirtyBitmapSnapshot *snap = NULL;
>      int y, y_start, dd, ds;
>      uint8_t *d, *s;
>      uint32_t *cptr, *s24;
> @@ -447,8 +425,6 @@ static void tcx24_update_display(void *opaque)
>      page24 = ts->vram24_offset;
>      cpage = ts->cplane_offset;
>      y_start = -1;
> -    page_min = -1;
> -    page_max = 0;
>      d = surface_data(surface);
>      s = ts->vram;
>      s24 = ts->vram24;
> @@ -457,15 +433,15 @@ static void tcx24_update_display(void *opaque)
>      ds = 1024;
>  
>      memory_region_sync_dirty_bitmap(&ts->vram_mem);
> +    snap = memory_region_snapshot_and_clear_dirty(&ts->vram_mem, 0x0,
> +                                                  memory_region_size(&ts->vram_mem),
> +                                                  DIRTY_MEMORY_VGA);
> +
>      for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE,
>              page24 += TARGET_PAGE_SIZE, cpage += TARGET_PAGE_SIZE) {
> -        if (tcx24_check_dirty(ts, page, page24, cpage)) {
> +        if (tcx24_check_dirty(ts, snap, page, page24, cpage)) {
>              if (y_start < 0)
>                  y_start = y;
> -            if (page < page_min)
> -                page_min = page;
> -            if (page > page_max)
> -                page_max = page;
>              tcx24_draw_line32(ts, d, s, ts->width, cptr, s24);
>              if (y >= ts->cursy && y < ts->cursy+32 && ts->cursx < ts->width) {
>                  tcx_draw_cursor32(ts, d, y, ts->width);
> @@ -521,10 +497,7 @@ static void tcx24_update_display(void *opaque)
>          dpy_gfx_update(ts->con, 0, y_start,
>                         ts->width, y - y_start);
>      }
> -    /* reset modified pages */
> -    if (page_max >= page_min) {
> -        tcx24_reset_dirty(ts, page_min, page_max, page24, cpage);
> -    }
> +    g_free(snap);
>  }
>  
>  static void tcx_invalidate_display(void *opaque)

ATB,

Mark.

Re: [Qemu-devel] [PATCH 00/10] make display updates thread safe.

[Mark Cave-Ayland]

On 04/04/17 11:23, Gerd Hoffmann wrote:

>   Hi,
> 
> Second round.  Helper API should be solid now, after patch discussions
> and initial testing.  API is documented now in the header file.
> Additionally to vga the sparc display adapters (cg3, tcx) are converted
> too.  Still quite some work until we've got them all converted.  Still
> enough progress that I think posting the new revision is useful.
> 
> cheers,
>   Gerd
> 
> Gerd Hoffmann (5):
>   bitmap: add bitmap_copy_and_clear_atomic
>   memory: add support getting and using a dirty bitmap copy.
>   vga: add vga_scanline_invalidated helper
>   vga: make display updates thread safe.
>   [testing] console: remove do_safe_dpy_refresh
> 
> Mark Cave-Ayland (5):
>   cg3: remove TARGET_PAGE_SIZE rounding on dirty page detection
>   cg3: fix up size parameter for memory_region_get_dirty()
>   cg3: make display updates thread safe
>   tcx: introduce tcx_check_dirty() function
>   tcx: make display updates thread safe
> 
>  include/exec/memory.h   | 47 +++++++++++++++++++++++++++
>  include/exec/ram_addr.h |  7 ++++
>  include/qemu/bitmap.h   |  2 ++
>  include/qemu/typedefs.h |  1 +
>  exec.c                  | 75 ++++++++++++++++++++++++++++++++++++++++++
>  hw/display/cg3.c        | 40 +++++++++++------------
>  hw/display/tcx.c        | 86 ++++++++++++++++++-------------------------------
>  hw/display/vga.c        | 50 +++++++++++++++-------------
>  memory.c                | 17 ++++++++++
>  ui/console.c            | 25 +-------------
>  util/bitmap.c           | 11 +++++++
>  11 files changed, 241 insertions(+), 120 deletions(-)

All in all, looks reasonably good here.

I've just noticed that TCX 24-bit is still leaving some artifacts, but
from looking at the code I'm fairly confident that it's bugs in the
existing TCX code where some of the accelerated features aren't
correctly invalidating the external color plane.

Will have a play and see if I can send a patch later.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 00/10] make display updates thread safe.

[Mark Cave-Ayland]

On 04/04/17 13:54, Mark Cave-Ayland wrote:

> I've just noticed that TCX 24-bit is still leaving some artifacts, but
> from looking at the code I'm fairly confident that it's bugs in the
> existing TCX code where some of the accelerated features aren't
> correctly invalidating the external color plane.
> 
> Will have a play and see if I can send a patch later.

So actually I started digging into the issues and ended up with a load
of invalidation fixes for TCX24 exposed by the thread-safe work, along
with some good clean ups.

I'll send through what I have shortly, however now that patchset has
expanded I think it's better to remove the TCX/CG3 patches from your
queue and just include the conversation to thread-safe routines.

Here's what your vga-fixes branch looks like rebased upon my patchset:
https://github.com/mcayland/qemu/commits/vga-fixes-sparc.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 03/10] vga: add vga_scanline_invalidated helper

[Richard Henderson]

On 04/04/2017 03:23 AM, Gerd Hoffmann wrote:
> +static bool vga_scanline_invalidated(VGACommonState *s, int y)
> +{
> +    if (y >= VGA_MAX_HEIGHT) {
> +        return false;
> +    }
> +    return s->invalidated_y_table[y >> 5] |= 1 << (y & 0x1f);
> +}
> +
>  void vga_sync_dirty_bitmap(VGACommonState *s)
>  {
>      memory_region_sync_dirty_bitmap(&s->vram);
> @@ -1638,8 +1646,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>          page1 = addr + bwidth - 1;
>          update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
>                                            DIRTY_MEMORY_VGA);
> -        /* explicit invalidation for the hardware cursor */
> -        update |= (s->invalidated_y_table[y >> 5] >> (y & 0x1f)) & 1;
> +        /* explicit invalidation for the hardware cursor (cirrus only) */
> +        update |= vga_scanline_invalidated(s, y);

The return stmt doesn't match what you're replacing.  Are you really intending 
to modify invalidated_y_table here?


r~

Re: [Qemu-devel] [PATCH 03/10] vga: add vga_scanline_invalidated helper

[Gerd Hoffmann]

> > +static bool vga_scanline_invalidated(VGACommonState *s, int y)
> > +{
> > +    if (y >= VGA_MAX_HEIGHT) {
> > +        return false;
> > +    }
> > +    return s->invalidated_y_table[y >> 5] |= 1 << (y & 0x1f);
> > +}

> The return stmt doesn't match what you're replacing.  Are you really intending 
> to modify invalidated_y_table here?

No.  Really stupid cut&paste bug.  Fixed.

thanks,
  Gerd