[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA o
|
From: |
Peter Maydell |
|
Subject: |
Re: [Qemu-devel] [PATCH for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer |
|
Date: |
Thu, 23 Jul 2015 11:22:21 +0100 |
On 22 July 2015 at 13:46, Paolo Bonzini <address@hidden> 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 <address@hidden>
> ---
> 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