| Message ID | 1430152117-100558-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 04/27 18:28, Paolo Bonzini wrote: > This will be required soon by the memory core. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/display/cg3.c | 1 + > hw/display/exynos4210_fimd.c | 20 +++++++++++++------- > hw/display/g364fb.c | 1 + > hw/display/sm501.c | 1 + > hw/display/tcx.c | 1 + > 5 files changed, 17 insertions(+), 7 deletions(-) "git grep memory_region_init_ram hw/display/" also mentions qxl.vram, vga.vram, tc6393xb.vram and vga.vram. Why don't these need it? Fam > > diff --git a/hw/display/cg3.c b/hw/display/cg3.c > index 1e6ff2b..cbcf518 100644 > --- a/hw/display/cg3.c > +++ b/hw/display/cg3.c > @@ -309,6 +309,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp) > > memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size, > &error_abort); > + memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); > vmstate_register_ram_global(&s->vram_mem); > sysbus_init_mmio(sbd, &s->vram_mem); > > diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c > index 45c62af..72b3a1d 100644 > --- a/hw/display/exynos4210_fimd.c > +++ b/hw/display/exynos4210_fimd.c > @@ -1109,6 +1109,12 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w) > } > } > > +static void exynos4210_fimd_invalidate(void *opaque) > +{ > + Exynos4210fimdState *s = (Exynos4210fimdState *)opaque; > + s->invalidate = true; > +} > + > /* Updates specified window's MemorySection based on values of WINCON, > * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */ > static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) > @@ -1136,7 +1142,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) > /* TODO: add .exit and unref the region there. Not needed yet since sysbus > * does not support hot-unplug. > */ > - memory_region_unref(w->mem_section.mr); > + if (w->mem_section.mr) { > + memory_region_set_log(w->mem_section.mr, false, DIRTY_MEMORY_VGA); > + memory_region_unref(w->mem_section.mr); > + } > + > w->mem_section = memory_region_find(sysbus_address_space(sbd), > fb_start_addr, w->fb_len); > assert(w->mem_section.mr); > @@ -1162,6 +1172,8 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) > cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0); > goto error_return; > } > + memory_region_set_log(w->mem_section.mr, true, DIRTY_MEMORY_VGA); > + exynos4210_fimd_invalidate(s); > return; > > error_return: > @@ -1224,12 +1236,6 @@ static void exynos4210_fimd_update_irq(Exynos4210fimdState *s) > } > } > > -static void exynos4210_fimd_invalidate(void *opaque) > -{ > - Exynos4210fimdState *s = (Exynos4210fimdState *)opaque; > - s->invalidate = true; > -} > - > static void exynos4210_update_resolution(Exynos4210fimdState *s) > { > DisplaySurface *surface = qemu_console_surface(s->console); > diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c > index 6543f2f..be62dd6 100644 > --- a/hw/display/g364fb.c > +++ b/hw/display/g364fb.c > @@ -489,6 +489,7 @@ static void g364fb_init(DeviceState *dev, G364State *s) > memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram", > s->vram_size, s->vram); > vmstate_register_ram(&s->mem_vram, dev); > + memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA); > } > > #define TYPE_G364 "sysbus-g364" > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index c72154b..43f8538 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -1412,6 +1412,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, > memory_region_init_ram(&s->local_mem_region, NULL, "sm501.local", > local_mem_bytes, &error_abort); > vmstate_register_ram_global(&s->local_mem_region); > + memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA); > s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region); > memory_region_add_subregion(address_space_mem, base, &s->local_mem_region); > > diff --git a/hw/display/tcx.c b/hw/display/tcx.c > index a9f9f66..58faa96 100644 > --- a/hw/display/tcx.c > +++ b/hw/display/tcx.c > @@ -1006,6 +1006,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp) > memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram", > s->vram_size * (1 + 4 + 4), &error_abort); > vmstate_register_ram_global(&s->vram_mem); > + memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); > vram_base = memory_region_get_ram_ptr(&s->vram_mem); > > /* 10/ROM : FCode ROM */ > -- > 1.8.3.1 > >
On 26/05/2015 09:13, Fam Zheng wrote: >> > hw/display/cg3.c | 1 + >> > hw/display/exynos4210_fimd.c | 20 +++++++++++++------- >> > hw/display/g364fb.c | 1 + >> > hw/display/sm501.c | 1 + >> > hw/display/tcx.c | 1 + >> > 5 files changed, 17 insertions(+), 7 deletions(-) > "git grep memory_region_init_ram hw/display/" also mentions qxl.vram, vga.vram, > tc6393xb.vram and vga.vram. Why don't these need it? tc6393xb.c doesn't use dirty memory, and vga sets it in hw/display/vga.c. Paolo
Peter, Mark, Aurelien, can you review and ack this patch? Thanks, Paolo On 27/04/2015 18:28, Paolo Bonzini wrote: > This will be required soon by the memory core. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/display/cg3.c | 1 + > hw/display/exynos4210_fimd.c | 20 +++++++++++++------- > hw/display/g364fb.c | 1 + > hw/display/sm501.c | 1 + > hw/display/tcx.c | 1 + > 5 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/hw/display/cg3.c b/hw/display/cg3.c > index 1e6ff2b..cbcf518 100644 > --- a/hw/display/cg3.c > +++ b/hw/display/cg3.c > @@ -309,6 +309,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp) > > memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size, > &error_abort); > + memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); > vmstate_register_ram_global(&s->vram_mem); > sysbus_init_mmio(sbd, &s->vram_mem); > > diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c > index 45c62af..72b3a1d 100644 > --- a/hw/display/exynos4210_fimd.c > +++ b/hw/display/exynos4210_fimd.c > @@ -1109,6 +1109,12 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w) > } > } > > +static void exynos4210_fimd_invalidate(void *opaque) > +{ > + Exynos4210fimdState *s = (Exynos4210fimdState *)opaque; > + s->invalidate = true; > +} > + > /* Updates specified window's MemorySection based on values of WINCON, > * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */ > static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) > @@ -1136,7 +1142,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) > /* TODO: add .exit and unref the region there. Not needed yet since sysbus > * does not support hot-unplug. > */ > - memory_region_unref(w->mem_section.mr); > + if (w->mem_section.mr) { > + memory_region_set_log(w->mem_section.mr, false, DIRTY_MEMORY_VGA); > + memory_region_unref(w->mem_section.mr); > + } > + > w->mem_section = memory_region_find(sysbus_address_space(sbd), > fb_start_addr, w->fb_len); > assert(w->mem_section.mr); > @@ -1162,6 +1172,8 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) > cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0); > goto error_return; > } > + memory_region_set_log(w->mem_section.mr, true, DIRTY_MEMORY_VGA); > + exynos4210_fimd_invalidate(s); > return; > > error_return: > @@ -1224,12 +1236,6 @@ static void exynos4210_fimd_update_irq(Exynos4210fimdState *s) > } > } > > -static void exynos4210_fimd_invalidate(void *opaque) > -{ > - Exynos4210fimdState *s = (Exynos4210fimdState *)opaque; > - s->invalidate = true; > -} > - > static void exynos4210_update_resolution(Exynos4210fimdState *s) > { > DisplaySurface *surface = qemu_console_surface(s->console); > diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c > index 6543f2f..be62dd6 100644 > --- a/hw/display/g364fb.c > +++ b/hw/display/g364fb.c > @@ -489,6 +489,7 @@ static void g364fb_init(DeviceState *dev, G364State *s) > memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram", > s->vram_size, s->vram); > vmstate_register_ram(&s->mem_vram, dev); > + memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA); > } > > #define TYPE_G364 "sysbus-g364" > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index c72154b..43f8538 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -1412,6 +1412,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, > memory_region_init_ram(&s->local_mem_region, NULL, "sm501.local", > local_mem_bytes, &error_abort); > vmstate_register_ram_global(&s->local_mem_region); > + memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA); > s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region); > memory_region_add_subregion(address_space_mem, base, &s->local_mem_region); > > diff --git a/hw/display/tcx.c b/hw/display/tcx.c > index a9f9f66..58faa96 100644 > --- a/hw/display/tcx.c > +++ b/hw/display/tcx.c > @@ -1006,6 +1006,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp) > memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram", > s->vram_size * (1 + 4 + 4), &error_abort); > vmstate_register_ram_global(&s->vram_mem); > + memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); > vram_base = memory_region_get_ram_ptr(&s->vram_mem); > > /* 10/ROM : FCode ROM */ >
On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Peter, Mark, Aurelien, can you review and ack this patch?
Could you provide some explanation/documentation of when a
display device needs to set DIRTY_MEMORY_VGA (and when it
doesn't)? If we get it wrong is there any way to make qemu
assert or otherwise catch the error?
thanks
-- PMM
On 26/05/2015 13:52, Peter Maydell wrote: > On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote: >> > Peter, Mark, Aurelien, can you review and ack this patch? > Could you provide some explanation/documentation of when a > display device needs to set DIRTY_MEMORY_VGA (and when it > doesn't)? It needs to set it if it uses memory_region_get/set/clear_dirty with DIRTY_MEMORY_VGA as the last argument. > If we get it wrong is there any way to make qemu > assert or otherwise catch the error? It may be possible to check against mr->dirty_log_mask in memory_region_get/set/clear_dirty. However, it is just as likely to have some corner case that is correct but triggers the assertion. I haven't thought much about it, because a simple grep for DIRTY_MEMORY_VGA will catch the device models that need care. Paolo
On 26 May 2015 at 12:56, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 26/05/2015 13:52, Peter Maydell wrote: >> On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> > Peter, Mark, Aurelien, can you review and ack this patch? >> Could you provide some explanation/documentation of when a >> display device needs to set DIRTY_MEMORY_VGA (and when it >> doesn't)? > > It needs to set it if it uses memory_region_get/set/clear_dirty with > DIRTY_MEMORY_VGA as the last argument. OK, and which devices need to do that? -- PMM
On 26/05/2015 14:08, Peter Maydell wrote: >>> >> Could you provide some explanation/documentation of when a >>> >> display device needs to set DIRTY_MEMORY_VGA (and when it >>> >> doesn't)? >> > >> > It needs to set it if it uses memory_region_get/set/clear_dirty with >> > DIRTY_MEMORY_VGA as the last argument. > OK, and which devices need to do that? Those that care to optimize their update_display callback. It's entirely opt in. Paolo
On 26 May 2015 at 13:10, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 26/05/2015 14:08, Peter Maydell wrote: >>>> >> Could you provide some explanation/documentation of when a >>>> >> display device needs to set DIRTY_MEMORY_VGA (and when it >>>> >> doesn't)? >>> > >>> > It needs to set it if it uses memory_region_get/set/clear_dirty with >>> > DIRTY_MEMORY_VGA as the last argument. >> OK, and which devices need to do that? > > Those that care to optimize their update_display callback. It's > entirely opt in. It also appears to be entirely undocumented :-) The reason I'm asking is that the a lot of the bulk of QEMU is device models, which are often written by people without a deep understanding of QEMU's APIs and then reviewed by people who may not be familiar with all the subsystem details either. So APIs which are required to be used by devices are more in need of documentation and assertion-sanity-checks on their use than APIs which are only used by core code or even by tcg frontends. For instance, up until this thread I'd assumed that DIRTY_MEMORY_VGA was pretty much what the name implies, something that's only of interest to the PC VGA card emulation. So it's very hard for me to review something that's relating to an API that's not one I know of and which isn't documented either. thanks -- PMM
On 26/05/15 12:56, Paolo Bonzini wrote: > On 26/05/2015 13:52, Peter Maydell wrote: >> On 26 May 2015 at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> Peter, Mark, Aurelien, can you review and ack this patch? >> Could you provide some explanation/documentation of when a >> display device needs to set DIRTY_MEMORY_VGA (and when it >> doesn't)? > > It needs to set it if it uses memory_region_get/set/clear_dirty with > DIRTY_MEMORY_VGA as the last argument. > >> If we get it wrong is there any way to make qemu >> assert or otherwise catch the error? > > It may be possible to check against mr->dirty_log_mask in > memory_region_get/set/clear_dirty. However, it is just as likely to > have some corner case that is correct but triggers the assertion. > > I haven't thought much about it, because a simple grep for > DIRTY_MEMORY_VGA will catch the device models that need care. Hi Paolo, Sorry for a taking a while to get around to this. I've just tested the v3 patchset on qemu-system-sparc for both CG3 and TCX and I don't see any display issues (or at least if this were broken I'd expect to see missing video updates/strange video artifacts). So while I can't comment on the specifics, it looks reasonable based upon the patchset and doesn't break anything so: Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
diff --git a/hw/display/cg3.c b/hw/display/cg3.c index 1e6ff2b..cbcf518 100644 --- a/hw/display/cg3.c +++ b/hw/display/cg3.c @@ -309,6 +309,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp) memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size, &error_abort); + memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); vmstate_register_ram_global(&s->vram_mem); sysbus_init_mmio(sbd, &s->vram_mem); diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c index 45c62af..72b3a1d 100644 --- a/hw/display/exynos4210_fimd.c +++ b/hw/display/exynos4210_fimd.c @@ -1109,6 +1109,12 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w) } } +static void exynos4210_fimd_invalidate(void *opaque) +{ + Exynos4210fimdState *s = (Exynos4210fimdState *)opaque; + s->invalidate = true; +} + /* Updates specified window's MemorySection based on values of WINCON, * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) @@ -1136,7 +1142,11 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) /* TODO: add .exit and unref the region there. Not needed yet since sysbus * does not support hot-unplug. */ - memory_region_unref(w->mem_section.mr); + if (w->mem_section.mr) { + memory_region_set_log(w->mem_section.mr, false, DIRTY_MEMORY_VGA); + memory_region_unref(w->mem_section.mr); + } + w->mem_section = memory_region_find(sysbus_address_space(sbd), fb_start_addr, w->fb_len); assert(w->mem_section.mr); @@ -1162,6 +1172,8 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win) cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0); goto error_return; } + memory_region_set_log(w->mem_section.mr, true, DIRTY_MEMORY_VGA); + exynos4210_fimd_invalidate(s); return; error_return: @@ -1224,12 +1236,6 @@ static void exynos4210_fimd_update_irq(Exynos4210fimdState *s) } } -static void exynos4210_fimd_invalidate(void *opaque) -{ - Exynos4210fimdState *s = (Exynos4210fimdState *)opaque; - s->invalidate = true; -} - static void exynos4210_update_resolution(Exynos4210fimdState *s) { DisplaySurface *surface = qemu_console_surface(s->console); diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c index 6543f2f..be62dd6 100644 --- a/hw/display/g364fb.c +++ b/hw/display/g364fb.c @@ -489,6 +489,7 @@ static void g364fb_init(DeviceState *dev, G364State *s) memory_region_init_ram_ptr(&s->mem_vram, NULL, "vram", s->vram_size, s->vram); vmstate_register_ram(&s->mem_vram, dev); + memory_region_set_log(&s->mem_vram, true, DIRTY_MEMORY_VGA); } #define TYPE_G364 "sysbus-g364" diff --git a/hw/display/sm501.c b/hw/display/sm501.c index c72154b..43f8538 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -1412,6 +1412,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, memory_region_init_ram(&s->local_mem_region, NULL, "sm501.local", local_mem_bytes, &error_abort); vmstate_register_ram_global(&s->local_mem_region); + memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA); s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region); memory_region_add_subregion(address_space_mem, base, &s->local_mem_region); diff --git a/hw/display/tcx.c b/hw/display/tcx.c index a9f9f66..58faa96 100644 --- a/hw/display/tcx.c +++ b/hw/display/tcx.c @@ -1006,6 +1006,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp) memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram", s->vram_size * (1 + 4 + 4), &error_abort); vmstate_register_ram_global(&s->vram_mem); + memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); vram_base = memory_region_get_ram_ptr(&s->vram_mem); /* 10/ROM : FCode ROM */
This will be required soon by the memory core. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/display/cg3.c | 1 + hw/display/exynos4210_fimd.c | 20 +++++++++++++------- hw/display/g364fb.c | 1 + hw/display/sm501.c | 1 + hw/display/tcx.c | 1 + 5 files changed, 17 insertions(+), 7 deletions(-)