[Qemu-devel] [PATCH for-2.4 v2 0/2] framebuffer: automatically set DIRTY_MEMORY_VGA

[Qemu-devel] [PATCH for-2.4 v2 0/2] framebuffer: automatically set DIRTY_MEMORY_VGA

[Paolo Bonzini]

The pxa2xx_lcd, omap_lcdc, pl110 and milkymist-vgafb devices use
framebuffer.c to render an image from a shared memory framebuffer.
With KVM, DIRTY_MEMORY_VGA always had to be enabled explicitly
on RAM memory regions that can be used for the framebuffer, and
the 2.4 changes to dirty bitmap handling made that mandatory for
TCG as well.

If the board does not set DIRTY_MEMORY_VGA, framebuffer.c detects this
(commit d55d420, framebuffer: check memory_region_is_logging,
2015-03-23) and always invalidates the screen; this is inefficient,
to the point that blocking X11 calls on slow network connections
can prevent the guest from making progress.  To fix this, the patch
makes the framebuffer set the flag.  Because there could be multiple
framebuffers, patch 1 allows nesting of memory_region_set_log calls.

Paolo

Paolo Bonzini (2):
  memory: count number of active VGA logging clients
  framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the
    framebuffer

 hw/display/framebuffer.c     | 75 ++++++++++++++++++++++++--------------------
 hw/display/framebuffer.h     | 44 ++++++++++++++++++++++++--
 hw/display/milkymist-vgafb.c | 15 +++++++--
 hw/display/omap_lcdc.c       | 12 +++++--
 hw/display/pl110.c           | 13 ++++++--
 hw/display/pxa2xx_lcd.c      | 29 ++++++++++++-----
 include/exec/memory.h        |  1 +
 memory.c                     |  7 +++++
 8 files changed, 145 insertions(+), 51 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] memory: count number of active VGA logging clients

[Paolo Bonzini]

For a board that has multiple framebuffer devices, both of them
might want to use DIRTY_MEMORY_VGA on the same memory region.
The lack of reference counting in memory_region_set_log makes
this very awkward to implement.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h | 1 +
 memory.c              | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 1394715..94d20ea 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -180,6 +180,7 @@ struct MemoryRegion {
     bool warning_printed; /* For reservations */
     bool flush_coalesced_mmio;
     bool global_locking;
+    uint8_t vga_logging_count;
     MemoryRegion *alias;
     hwaddr alias_offset;
     int32_t priority;
diff --git a/memory.c b/memory.c
index 0acebb1..5e5f325 100644
--- a/memory.c
+++ b/memory.c
@@ -1433,8 +1433,15 @@ void memory_region_notify_iommu(MemoryRegion *mr,
 void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 {
     uint8_t mask = 1 << client;
+    uint8_t old_logging;
 
     assert(client == DIRTY_MEMORY_VGA);
+    old_logging = mr->vga_logging_count;
+    mr->vga_logging_count += log ? 1 : -1;
+    if (!!old_logging == !!mr->vga_logging_count) {
+        return;
+    }
+
     memory_region_transaction_begin();
     mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
     memory_region_update_pending |= mr->enabled;
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer

[Paolo Bonzini]

The MemoryRegionSection contains enough information to access the
RAM region underlying the framebuffer, and can be cached inside the
display device.

By doing this, the new framebuffer_update_memory_section function can
enable dirty memory logging on the relevant RAM region.  The function
must be called whenever the stride or base of the framebuffer changes;
a simple way to cover these cases is to call it on every full frame
invalidation, which is a rare case.

framebuffer_update_display now works entirely on a MemoryRegionSection,
without going through cpu_physical_memory_map/unmap.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: add doc comments, fix pxa2xx argument order

---
 hw/display/framebuffer.c     | 75 ++++++++++++++++++++++++--------------------
 hw/display/framebuffer.h     | 44 ++++++++++++++++++++++++--
 hw/display/milkymist-vgafb.c | 15 +++++++--
 hw/display/omap_lcdc.c       | 12 +++++--
 hw/display/pl110.c           | 13 ++++++--
 hw/display/pxa2xx_lcd.c      | 29 ++++++++++++-----
 6 files changed, 137 insertions(+), 51 deletions(-)

diff --git a/hw/display/framebuffer.c b/hw/display/framebuffer.c
index 2cabced..7f075ce 100644
--- a/hw/display/framebuffer.c
+++ b/hw/display/framebuffer.c
@@ -21,12 +21,40 @@
 #include "ui/console.h"
 #include "framebuffer.h"
 
+void framebuffer_update_memory_section(
+    MemoryRegionSection *mem_section,
+    MemoryRegion *root,
+    hwaddr base,
+    unsigned rows,
+    unsigned src_width)
+{
+    hwaddr src_len = (hwaddr)rows * src_width;
+
+    if (mem_section->mr) {
+        memory_region_set_log(mem_section->mr, false, DIRTY_MEMORY_VGA);
+        memory_region_unref(mem_section->mr);
+        mem_section->mr = NULL;
+    }
+
+    *mem_section = memory_region_find(root, base, src_len);
+    if (!mem_section->mr) {
+        return;
+    }
+
+    if (int128_get64(mem_section->size) < src_len ||
+            !memory_region_is_ram(mem_section->mr)) {
+        memory_region_unref(mem_section->mr);
+        mem_section->mr = NULL;
+        return;
+    }
+
+    memory_region_set_log(mem_section->mr, true, DIRTY_MEMORY_VGA);
+}
+
 /* Render an image from a shared memory framebuffer.  */
-   
 void framebuffer_update_display(
     DisplaySurface *ds,
-    MemoryRegion *address_space,
-    hwaddr base,
+    MemoryRegionSection *mem_section,
     int cols, /* Width in pixels.  */
     int rows, /* Height in pixels.  */
     int src_width, /* Length of source line, in bytes.  */
@@ -41,51 +69,33 @@ void framebuffer_update_display(
     hwaddr src_len;
     uint8_t *dest;
     uint8_t *src;
-    uint8_t *src_base;
     int first, last = 0;
     int dirty;
     int i;
     ram_addr_t addr;
-    MemoryRegionSection mem_section;
     MemoryRegion *mem;
 
     i = *first_row;
     *first_row = -1;
     src_len = src_width * rows;
 
-    mem_section = memory_region_find(address_space, base, src_len);
-    mem = mem_section.mr;
-    if (int128_get64(mem_section.size) != src_len ||
-            !memory_region_is_ram(mem_section.mr)) {
-        goto out;
+    mem = mem_section->mr;
+    if (!mem) {
+        return;
     }
-    assert(mem);
-    assert(mem_section.offset_within_address_space == base);
-
     memory_region_sync_dirty_bitmap(mem);
-    if (!memory_region_is_logging(mem, DIRTY_MEMORY_VGA)) {
-        invalidate = true;
-    }
 
-    src_base = cpu_physical_memory_map(base, &src_len, 0);
-    /* If we can't map the framebuffer then bail.  We could try harder,
-       but it's not really worth it as dirty flag tracking will probably
-       already have failed above.  */
-    if (!src_base)
-        goto out;
-    if (src_len != src_width * rows) {
-        cpu_physical_memory_unmap(src_base, src_len, 0, 0);
-        goto out;
-    }
-    src = src_base;
+    addr = mem_section->offset_within_region;
+    src = memory_region_get_ram_ptr(mem) + addr;
+
     dest = surface_data(ds);
-    if (dest_col_pitch < 0)
+    if (dest_col_pitch < 0) {
         dest -= dest_col_pitch * (cols - 1);
+    }
     if (dest_row_pitch < 0) {
         dest -= dest_row_pitch * (rows - 1);
     }
     first = -1;
-    addr = mem_section.offset_within_region;
 
     addr += i * src_width;
     src += i * src_width;
@@ -104,14 +114,11 @@ void framebuffer_update_display(
         src += src_width;
         dest += dest_row_pitch;
     }
-    cpu_physical_memory_unmap(src_base, src_len, 0, 0);
     if (first < 0) {
-        goto out;
+        return;
     }
-    memory_region_reset_dirty(mem, mem_section.offset_within_region, src_len,
+    memory_region_reset_dirty(mem, mem_section->offset_within_region, src_len,
                               DIRTY_MEMORY_VGA);
     *first_row = first;
     *last_row = last;
-out:
-    memory_region_unref(mem);
 }
diff --git a/hw/display/framebuffer.h b/hw/display/framebuffer.h
index 6eae035..63954d4 100644
--- a/hw/display/framebuffer.h
+++ b/hw/display/framebuffer.h
@@ -7,10 +7,50 @@
 
 typedef void (*drawfn)(void *, uint8_t *, const uint8_t *, int, int);
 
+/* framebuffer_update_memory_section: Update framebuffer
+ * #MemoryRegionSection, for example if the framebuffer is switched to
+ * a different memory area.
+ *
+ * @mem_section: Output #MemoryRegionSection, to be passed to
+ * framebuffer_update_display().
+ * @root: #MemoryRegion within which the framebuffer lies
+ * @base: Base address of the framebuffer within @root.
+ * @rows: Height of the screen.
+ * @src_width: Number of bytes in framebuffer memory between two rows.
+ */
+void framebuffer_update_memory_section(
+    MemoryRegionSection *mem_section,
+    MemoryRegion *root,
+    hwaddr base,
+    unsigned rows,
+    unsigned src_width);
+
+/* framebuffer_update_display: Draw the framebuffer on a surface.
+ *
+ * @ds: #DisplaySurface to draw to.
+ * @mem_section: #MemoryRegionSection provded by
+ * framebuffer_update_memory_section().
+ * @cols: Width the screen.
+ * @rows: Height of the screen.
+ * @src_width: Number of bytes in framebuffer memory between two rows.
+ * @dest_row_pitch: Number of bytes in the surface data between two rows.
+ * Negative if the framebuffer is stored in the opposite order (e.g.
+ * bottom-to-top) compared to the framebuffer.
+ * @dest_col_pitch: Number of bytes in the surface data between two pixels.
+ * Negative if the framebuffer is stored in the opposite order (e.g.
+ * right-to-left) compared to the framebuffer.
+ * @invalidate: True if the function should redraw the whole screen
+ * without checking the DIRTY_MEMORY_VGA dirty bitmap.
+ * @fn: Drawing function to be called for each row that has to be drawn.
+ * @opaque: Opaque pointer passed to @fn.
+ * @first_row: Pointer to an integer, receives the number of the first row
+ * that was drawn (either the first dirty row, or 0 if @invalidate is true).
+ * @last_row: Pointer to an integer, receives the number of the last row that
+ * was drawn (either the last dirty row, or @rows-1 if @invalidate is true).
+ */
 void framebuffer_update_display(
     DisplaySurface *ds,
-    MemoryRegion *address_space,
-    hwaddr base,
+    MemoryRegionSection *mem_section,
     int cols,
     int rows,
     int src_width,
diff --git a/hw/display/milkymist-vgafb.c b/hw/display/milkymist-vgafb.c
index 9b35e76..ab3074f 100644
--- a/hw/display/milkymist-vgafb.c
+++ b/hw/display/milkymist-vgafb.c
@@ -71,6 +71,7 @@ struct MilkymistVgafbState {
     SysBusDevice parent_obj;
 
     MemoryRegion regs_region;
+    MemoryRegionSection fbsection;
     QemuConsole *con;
 
     int invalidate;
@@ -91,6 +92,7 @@ static void vgafb_update_display(void *opaque)
     MilkymistVgafbState *s = opaque;
     SysBusDevice *sbd;
     DisplaySurface *surface = qemu_console_surface(s->con);
+    int src_width;
     int first = 0;
     int last = 0;
     drawfn fn;
@@ -129,11 +131,18 @@ static void vgafb_update_display(void *opaque)
         break;
     }
 
-    framebuffer_update_display(surface, sysbus_address_space(sbd),
-                               s->regs[R_BASEADDRESS] + s->fb_offset,
+    src_width = s->regs[R_HRES] * 2;
+    if (s->invalidate) {
+        framebuffer_update_memory_section(&s->fbsection,
+                                          sysbus_address_space(sbd),
+                                          s->regs[R_BASEADDRESS] + s->fb_offset,
+                                          s->regs[R_VRES], src_width);
+    }
+
+    framebuffer_update_display(surface, &s->fbsection,
                                s->regs[R_HRES],
                                s->regs[R_VRES],
-                               s->regs[R_HRES] * 2,
+                               src_width,
                                dest_width,
                                0,
                                s->invalidate,
diff --git a/hw/display/omap_lcdc.c b/hw/display/omap_lcdc.c
index fda81ba..a7c6cd7 100644
--- a/hw/display/omap_lcdc.c
+++ b/hw/display/omap_lcdc.c
@@ -25,6 +25,7 @@
 struct omap_lcd_panel_s {
     MemoryRegion *sysmem;
     MemoryRegion iomem;
+    MemoryRegionSection fbsection;
     qemu_irq irq;
     QemuConsole *con;
 
@@ -215,12 +216,19 @@ static void omap_update_display(void *opaque)
 
     step = width * bpp >> 3;
     linesize = surface_stride(surface);
-    framebuffer_update_display(surface, omap_lcd->sysmem,
-                               frame_base, width, height,
+    if (omap_lcd->invalidate) {
+        framebuffer_update_memory_section(&omap_lcd->fbsection,
+                                          omap_lcd->sysmem, frame_base,
+                                          height, step);
+    }
+
+    framebuffer_update_display(surface, &omap_lcd->fbsection,
+                               width, height,
                                step, linesize, 0,
                                omap_lcd->invalidate,
                                draw_line, omap_lcd->palette,
                                &first, &last);
+
     if (first >= 0) {
         dpy_gfx_update(omap_lcd->con, 0, first, width, last - first + 1);
     }
diff --git a/hw/display/pl110.c b/hw/display/pl110.c
index c574cf1..ef1a7b1 100644
--- a/hw/display/pl110.c
+++ b/hw/display/pl110.c
@@ -46,6 +46,7 @@ typedef struct PL110State {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
+    MemoryRegionSection fbsection;
     QemuConsole *con;
 
     int version;
@@ -238,12 +239,20 @@ static void pl110_update_display(void *opaque)
     }
     dest_width *= s->cols;
     first = 0;
-    framebuffer_update_display(surface, sysbus_address_space(sbd),
-                               s->upbase, s->cols, s->rows,
+    if (s->invalidate) {
+        framebuffer_update_memory_section(&s->fbsection,
+                                          sysbus_address_space(sbd),
+                                          s->upbase,
+                                          s->rows, src_width);
+    }
+
+    framebuffer_update_display(surface, &s->fbsection,
+                               s->cols, s->rows,
                                src_width, dest_width, 0,
                                s->invalidate,
                                fn, s->palette,
                                &first, &last);
+
     if (first >= 0) {
         dpy_gfx_update(s->con, 0, first, s->cols, last - first + 1);
     }
diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index ac3c018..494700d 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -35,6 +35,7 @@ struct DMAChannel {
 struct PXA2xxLCDState {
     MemoryRegion *sysmem;
     MemoryRegion iomem;
+    MemoryRegionSection fbsection;
     qemu_irq irq;
     int irqlevel;
 
@@ -687,8 +688,11 @@ static void pxa2xx_lcdc_dma0_redraw_rot0(PXA2xxLCDState *s,
 
     dest_width = s->xres * s->dest_width;
     *miny = 0;
-    framebuffer_update_display(surface, s->sysmem,
-                               addr, s->xres, s->yres,
+    if (s->invalidated) {
+        framebuffer_update_memory_section(&s->fbsection, s->sysmem,
+                                          addr, s->yres, src_width);
+    }
+    framebuffer_update_display(surface, &s->fbsection, s->xres, s->yres,
                                src_width, dest_width, s->dest_width,
                                s->invalidated,
                                fn, s->dma_ch[0].palette, miny, maxy);
@@ -715,8 +719,11 @@ static void pxa2xx_lcdc_dma0_redraw_rot90(PXA2xxLCDState *s,
 
     dest_width = s->yres * s->dest_width;
     *miny = 0;
-    framebuffer_update_display(surface, s->sysmem,
-                               addr, s->xres, s->yres,
+    if (s->invalidated) {
+        framebuffer_update_memory_section(&s->fbsection, s->sysmem,
+                                          addr, s->yres, src_width);
+    }
+    framebuffer_update_display(surface, &s->fbsection, s->xres, s->yres,
                                src_width, s->dest_width, -dest_width,
                                s->invalidated,
                                fn, s->dma_ch[0].palette,
@@ -747,8 +754,11 @@ static void pxa2xx_lcdc_dma0_redraw_rot180(PXA2xxLCDState *s,
 
     dest_width = s->xres * s->dest_width;
     *miny = 0;
-    framebuffer_update_display(surface, s->sysmem,
-                               addr, s->xres, s->yres,
+    if (s->invalidated) {
+        framebuffer_update_memory_section(&s->fbsection, s->sysmem,
+                                          addr, s->yres, src_width);
+    }
+    framebuffer_update_display(surface, &s->fbsection, s->xres, s->yres,
                                src_width, -dest_width, -s->dest_width,
                                s->invalidated,
                                fn, s->dma_ch[0].palette, miny, maxy);
@@ -778,8 +788,11 @@ static void pxa2xx_lcdc_dma0_redraw_rot270(PXA2xxLCDState *s,
 
     dest_width = s->yres * s->dest_width;
     *miny = 0;
-    framebuffer_update_display(surface, s->sysmem,
-                               addr, s->xres, s->yres,
+    if (s->invalidated) {
+        framebuffer_update_memory_section(&s->fbsection, s->sysmem,
+                                          addr, s->yres, src_width);
+    }
+    framebuffer_update_display(surface, &s->fbsection, s->xres, s->yres,
                                src_width, -s->dest_width, dest_width,
                                s->invalidated,
                                fn, s->dma_ch[0].palette,
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer

[Peter Maydell]

On 23 July 2015 at 12:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The MemoryRegionSection contains enough information to access the
> RAM region underlying the framebuffer, and can be cached inside the
> display device.
>
> By doing this, the new framebuffer_update_memory_section function can
> enable dirty memory logging on the relevant RAM region.  The function
> must be called whenever the stride or base of the framebuffer changes;
> a simple way to cover these cases is to call it on every full frame
> invalidation, which is a rare case.
>
> framebuffer_update_display now works entirely on a MemoryRegionSection,
> without going through cpu_physical_memory_map/unmap.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> --- a/hw/display/framebuffer.h
> +++ b/hw/display/framebuffer.h
> @@ -7,10 +7,50 @@
>
>  typedef void (*drawfn)(void *, uint8_t *, const uint8_t *, int, int);
>
> +/* framebuffer_update_memory_section: Update framebuffer
> + * #MemoryRegionSection, for example if the framebuffer is switched to
> + * a different memory area.
> + *
> + * @mem_section: Output #MemoryRegionSection, to be passed to
> + * framebuffer_update_display().
> + * @root: #MemoryRegion within which the framebuffer lies
> + * @base: Base address of the framebuffer within @root.
> + * @rows: Height of the screen.
> + * @src_width: Number of bytes in framebuffer memory between two rows.
> + */
> +void framebuffer_update_memory_section(
> +    MemoryRegionSection *mem_section,
> +    MemoryRegion *root,
> +    hwaddr base,
> +    unsigned rows,
> +    unsigned src_width);
> +
> +/* framebuffer_update_display: Draw the framebuffer on a surface.
> + *
> + * @ds: #DisplaySurface to draw to.
> + * @mem_section: #MemoryRegionSection provded by

"provided"

> + * framebuffer_update_memory_section().
> + * @cols: Width the screen.
> + * @rows: Height of the screen.
> + * @src_width: Number of bytes in framebuffer memory between two rows.
> + * @dest_row_pitch: Number of bytes in the surface data between two rows.
> + * Negative if the framebuffer is stored in the opposite order (e.g.

should these be "i.e.", or do we allow the source framebuffer to be
either way round?

> + * bottom-to-top) compared to the framebuffer.
> + * @dest_col_pitch: Number of bytes in the surface data between two pixels.
> + * Negative if the framebuffer is stored in the opposite order (e.g.
> + * right-to-left) compared to the framebuffer.
> + * @invalidate: True if the function should redraw the whole screen
> + * without checking the DIRTY_MEMORY_VGA dirty bitmap.
> + * @fn: Drawing function to be called for each row that has to be drawn.
> + * @opaque: Opaque pointer passed to @fn.
> + * @first_row: Pointer to an integer, receives the number of the first row
> + * that was drawn (either the first dirty row, or 0 if @invalidate is true).
> + * @last_row: Pointer to an integer, receives the number of the last row that
> + * was drawn (either the last dirty row, or @rows-1 if @invalidate is true).
> + */

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer

[Paolo Bonzini]

On 23/07/2015 13:13, Peter Maydell wrote:
>> > + * Negative if the framebuffer is stored in the opposite order (e.g.
> should these be "i.e.", or do we allow the source framebuffer to be
> either way round?

I think we do, that's why I used "e.g.".

Paolo

>> > + * bottom-to-top) compared to the framebuffer.