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

[Qemu-devel] [PATCH for-2.4 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     | 10 ++++--
 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, 111 insertions(+), 51 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 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>
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

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

[Peter Maydell]

On 22 July 2015 at 13:46, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/exec/memory.h | 1 +
>  memory.c              | 7 +++++++
>  2 files changed, 8 insertions(+)

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

thanks
-- PMM

[Qemu-devel] [PATCH for-2.4 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>
---
        only tested with vexpress (pl110)

 hw/display/framebuffer.c     | 75 ++++++++++++++++++++++++--------------------
 hw/display/framebuffer.h     | 10 ++++--
 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, 103 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..e735bb7 100644
--- a/hw/display/framebuffer.h
+++ b/hw/display/framebuffer.h
@@ -7,10 +7,16 @@
 
 typedef void (*drawfn)(void *, uint8_t *, const uint8_t *, int, int);
 
+void framebuffer_update_memory_section(
+    MemoryRegionSection *mem_section,
+    MemoryRegion *root,
+    hwaddr base,
+    unsigned rows,
+    unsigned src_width);
+
 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..47dcd39 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, src_width, s->yres);
+    }
+    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, src_width, s->yres);
+    }
+    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, src_width, s->yres);
+    }
+    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, src_width, s->yres);
+    }
+    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 for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer

[Peter Maydell]

On 22 July 2015 at 13:46, 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.

I have a few minor nits below, but the patch works and looks
good overall.

At the moment the pattern in all the callsites is
 if (s->invalidate) {
     framebuffer_update_memory_section(...);
 }
 framebuffer_update_display(...);

What's the rationale for not just having framebuffer_update_display()
do this -- that we might in future want to be cleverer about how
often we call framebuffer_update_memory_section() ?

> 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>
> ---
>         only tested with vexpress (pl110)

Tested pxa2xx, pl110, milkymist. I don't have any OMAP1 images
so can't test the omap_lcdc (it's not used in the OMAP2).

(Maybe we should just drop the OMAP1 support some day...)

>  hw/display/framebuffer.c     | 75 ++++++++++++++++++++++++--------------------
>  hw/display/framebuffer.h     | 10 ++++--
>  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, 103 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 ||

The previous code was doing an '!=' test; I guess < makes more
sense, though.

> +            !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);

Not new in this patch, but isn't there technically a race
condition if the guest writes to the framebuffer memory
after we've done the memory_region_get_dirty() for that
row but before we clear all the dirty bits again here?

>      *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..e735bb7 100644
> --- a/hw/display/framebuffer.h
> +++ b/hw/display/framebuffer.h
> @@ -7,10 +7,16 @@
>
>  typedef void (*drawfn)(void *, uint8_t *, const uint8_t *, int, int);
>
> +void framebuffer_update_memory_section(
> +    MemoryRegionSection *mem_section,
> +    MemoryRegion *root,
> +    hwaddr base,
> +    unsigned rows,
> +    unsigned src_width);

A doc-comment header would be nice.

> +
>  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..47dcd39 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, src_width, s->yres);

Aren't src_width and s->yres in the wrong order here?
They should be in the same order as they are in the
(ditto in the other hunks in the patch for this file).

This doesn't actually break anything because the only thing
the function does with those two params is multiply them
together.

> +    }
> +    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, src_width, s->yres);
> +    }
> +    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, src_width, s->yres);
> +    }
> +    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, src_width, s->yres);
> +    }
> +    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,

thanks
-- PMM

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

[Paolo Bonzini]

On 23/07/2015 12:22, Peter Maydell wrote:
> I have a few minor nits below, but the patch works and looks
> good overall.
> 
> At the moment the pattern in all the callsites is
>  if (s->invalidate) {
>      framebuffer_update_memory_section(...);
>  }
>  framebuffer_update_display(...);
> 
> What's the rationale for not just having framebuffer_update_display()
> do this -- that we might in future want to be cleverer about how
> often we call framebuffer_update_memory_section() ?

Yes.  I initially set out adding framebuffer_update_memory_section calls
after the register writes, but using s->invalidate is simpler albeit
marginally less efficient.

>> +    memory_region_reset_dirty(mem, mem_section->offset_within_region, src_len,
>>                                DIRTY_MEMORY_VGA);
> 
> Not new in this patch, but isn't there technically a race
> condition if the guest writes to the framebuffer memory
> after we've done the memory_region_get_dirty() for that
> row but before we clear all the dirty bits again here?

Yes, I think you're right.

>>
>> +void framebuffer_update_memory_section(
>> +    MemoryRegionSection *mem_section,
>> +    MemoryRegion *root,
>> +    hwaddr base,
>> +    unsigned rows,
>> +    unsigned src_width);
> 
> A doc-comment header would be nice.

Ok.

>>
>> +    if (s->invalidated) {
>> +        framebuffer_update_memory_section(&s->fbsection, s->sysmem,
>> +                                          addr, src_width, s->yres);
> 
> Aren't src_width and s->yres in the wrong order here?
> They should be in the same order as they are in the
> (ditto in the other hunks in the patch for this file).

Ouch, indeed.

Paolo