| Message ID | 1406716032-21795-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Instead, add a boolean variable to indicate the presence of the region. > Patch is good. Can you add a sentence on why you are doing this though? Is it just the save on the repeated malloc and free (which is sane in its own right) or something bigger in the context of your series? > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Otherwise, Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > hw/display/vga.c | 24 ++++++++++-------------- > hw/display/vga_int.h | 3 ++- > 2 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/hw/display/vga.c b/hw/display/vga.c > index 4b089a3..68cfee2 100644 > --- a/hw/display/vga.c > +++ b/hw/display/vga.c > @@ -168,15 +168,18 @@ static uint8_t expand4to8[16]; > > static void vga_update_memory_access(VGACommonState *s) > { > - MemoryRegion *region, *old_region = s->chain4_alias; > hwaddr base, offset, size; > > if (s->legacy_address_space == NULL) { > return; > } > > - s->chain4_alias = NULL; > - > + if (s->has_chain4_alias) { > + memory_region_del_subregion(s->legacy_address_space, &s->chain4_alias); > + memory_region_destroy(&s->chain4_alias); > + s->has_chain4_alias = false; > + s->plane_updated = 0xf; > + } > if ((s->sr[VGA_SEQ_PLANE_WRITE] & VGA_SR02_ALL_PLANES) == > VGA_SR02_ALL_PLANES && s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { > offset = 0; > @@ -201,18 +204,11 @@ static void vga_update_memory_access(VGACommonState *s) > break; > } > base += isa_mem_base; > - region = g_malloc(sizeof(*region)); > - memory_region_init_alias(region, memory_region_owner(&s->vram), > + memory_region_init_alias(&s->chain4_alias, memory_region_owner(&s->vram), > "vga.chain4", &s->vram, offset, size); > memory_region_add_subregion_overlap(s->legacy_address_space, base, > - region, 2); > - s->chain4_alias = region; > - } > - if (old_region) { > - memory_region_del_subregion(s->legacy_address_space, old_region); > - memory_region_destroy(old_region); > - g_free(old_region); > - s->plane_updated = 0xf; > + &s->chain4_alias, 2); > + s->has_chain4_alias = true; > } > } > > @@ -1321,7 +1317,7 @@ static void vga_draw_text(VGACommonState *s, int full_update) > s->font_offsets[1] = offset; > full_update = 1; > } > - if (s->plane_updated & (1 << 2) || s->chain4_alias) { > + if (s->plane_updated & (1 << 2) || s->has_chain4_alias) { > /* if the plane 2 was modified since the last display, it > indicates the font may have been modified */ > s->plane_updated = 0; > diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h > index 5320abd..641f8f4 100644 > --- a/hw/display/vga_int.h > +++ b/hw/display/vga_int.h > @@ -94,7 +94,8 @@ typedef struct VGACommonState { > uint32_t vram_size; > uint32_t vram_size_mb; /* property */ > uint32_t latch; > - MemoryRegion *chain4_alias; > + bool has_chain4_alias; > + MemoryRegion chain4_alias; > uint8_t sr_index; > uint8_t sr[256]; > uint8_t gr_index; > -- > 1.8.3.1 > > >
Il 31/07/2014 14:01, Peter Crosthwaite ha scritto: > On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Instead, add a boolean variable to indicate the presence of the region. >> > > Patch is good. Can you add a sentence on why you are doing this > though? Is it just the save on the repeated malloc and free (which is > sane in its own right) or something bigger in the context of your > series? Even more than the repeated malloc and free, it's the repeated add_child/unparent. Paolo >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Otherwise, > > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > >> --- >> hw/display/vga.c | 24 ++++++++++-------------- >> hw/display/vga_int.h | 3 ++- >> 2 files changed, 12 insertions(+), 15 deletions(-) >> >> diff --git a/hw/display/vga.c b/hw/display/vga.c >> index 4b089a3..68cfee2 100644 >> --- a/hw/display/vga.c >> +++ b/hw/display/vga.c >> @@ -168,15 +168,18 @@ static uint8_t expand4to8[16]; >> >> static void vga_update_memory_access(VGACommonState *s) >> { >> - MemoryRegion *region, *old_region = s->chain4_alias; >> hwaddr base, offset, size; >> >> if (s->legacy_address_space == NULL) { >> return; >> } >> >> - s->chain4_alias = NULL; >> - >> + if (s->has_chain4_alias) { >> + memory_region_del_subregion(s->legacy_address_space, &s->chain4_alias); >> + memory_region_destroy(&s->chain4_alias); >> + s->has_chain4_alias = false; >> + s->plane_updated = 0xf; >> + } >> if ((s->sr[VGA_SEQ_PLANE_WRITE] & VGA_SR02_ALL_PLANES) == >> VGA_SR02_ALL_PLANES && s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { >> offset = 0; >> @@ -201,18 +204,11 @@ static void vga_update_memory_access(VGACommonState *s) >> break; >> } >> base += isa_mem_base; >> - region = g_malloc(sizeof(*region)); >> - memory_region_init_alias(region, memory_region_owner(&s->vram), >> + memory_region_init_alias(&s->chain4_alias, memory_region_owner(&s->vram), >> "vga.chain4", &s->vram, offset, size); >> memory_region_add_subregion_overlap(s->legacy_address_space, base, >> - region, 2); >> - s->chain4_alias = region; >> - } >> - if (old_region) { >> - memory_region_del_subregion(s->legacy_address_space, old_region); >> - memory_region_destroy(old_region); >> - g_free(old_region); >> - s->plane_updated = 0xf; >> + &s->chain4_alias, 2); >> + s->has_chain4_alias = true; >> } >> } >> >> @@ -1321,7 +1317,7 @@ static void vga_draw_text(VGACommonState *s, int full_update) >> s->font_offsets[1] = offset; >> full_update = 1; >> } >> - if (s->plane_updated & (1 << 2) || s->chain4_alias) { >> + if (s->plane_updated & (1 << 2) || s->has_chain4_alias) { >> /* if the plane 2 was modified since the last display, it >> indicates the font may have been modified */ >> s->plane_updated = 0; >> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h >> index 5320abd..641f8f4 100644 >> --- a/hw/display/vga_int.h >> +++ b/hw/display/vga_int.h >> @@ -94,7 +94,8 @@ typedef struct VGACommonState { >> uint32_t vram_size; >> uint32_t vram_size_mb; /* property */ >> uint32_t latch; >> - MemoryRegion *chain4_alias; >> + bool has_chain4_alias; >> + MemoryRegion chain4_alias; >> uint8_t sr_index; >> uint8_t sr[256]; >> uint8_t gr_index; >> -- >> 1.8.3.1 >> >> >>
diff --git a/hw/display/vga.c b/hw/display/vga.c index 4b089a3..68cfee2 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -168,15 +168,18 @@ static uint8_t expand4to8[16]; static void vga_update_memory_access(VGACommonState *s) { - MemoryRegion *region, *old_region = s->chain4_alias; hwaddr base, offset, size; if (s->legacy_address_space == NULL) { return; } - s->chain4_alias = NULL; - + if (s->has_chain4_alias) { + memory_region_del_subregion(s->legacy_address_space, &s->chain4_alias); + memory_region_destroy(&s->chain4_alias); + s->has_chain4_alias = false; + s->plane_updated = 0xf; + } if ((s->sr[VGA_SEQ_PLANE_WRITE] & VGA_SR02_ALL_PLANES) == VGA_SR02_ALL_PLANES && s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { offset = 0; @@ -201,18 +204,11 @@ static void vga_update_memory_access(VGACommonState *s) break; } base += isa_mem_base; - region = g_malloc(sizeof(*region)); - memory_region_init_alias(region, memory_region_owner(&s->vram), + memory_region_init_alias(&s->chain4_alias, memory_region_owner(&s->vram), "vga.chain4", &s->vram, offset, size); memory_region_add_subregion_overlap(s->legacy_address_space, base, - region, 2); - s->chain4_alias = region; - } - if (old_region) { - memory_region_del_subregion(s->legacy_address_space, old_region); - memory_region_destroy(old_region); - g_free(old_region); - s->plane_updated = 0xf; + &s->chain4_alias, 2); + s->has_chain4_alias = true; } } @@ -1321,7 +1317,7 @@ static void vga_draw_text(VGACommonState *s, int full_update) s->font_offsets[1] = offset; full_update = 1; } - if (s->plane_updated & (1 << 2) || s->chain4_alias) { + if (s->plane_updated & (1 << 2) || s->has_chain4_alias) { /* if the plane 2 was modified since the last display, it indicates the font may have been modified */ s->plane_updated = 0; diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h index 5320abd..641f8f4 100644 --- a/hw/display/vga_int.h +++ b/hw/display/vga_int.h @@ -94,7 +94,8 @@ typedef struct VGACommonState { uint32_t vram_size; uint32_t vram_size_mb; /* property */ uint32_t latch; - MemoryRegion *chain4_alias; + bool has_chain4_alias; + MemoryRegion chain4_alias; uint8_t sr_index; uint8_t sr[256]; uint8_t gr_index;
Instead, add a boolean variable to indicate the presence of the region. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/display/vga.c | 24 ++++++++++-------------- hw/display/vga_int.h | 3 ++- 2 files changed, 12 insertions(+), 15 deletions(-)