| Message ID | 7c6e8c071cbd47d6d9065635e97dd88a7197b5be.1488068248.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 February 2017 at 18:23, BALATON Zoltan <balaton@eik.bme.hu> wrote: > Do not use the base address to access data in local memory. This is in > preparation to allow chip connected via PCI where base address depends > on where the BAR is mapped so it will be unknown. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/display/sm501.c | 6 ++---- > hw/display/sm501_template.h | 8 ++++---- > 2 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index 22cac97..1cda127 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -460,7 +460,6 @@ typedef struct SM501State { > QemuConsole *con; > > /* status & internal resources */ > - hwaddr base; > uint32_t local_mem_size_index; > uint8_t *local_mem; > MemoryRegion local_mem_region; > @@ -1400,10 +1399,9 @@ static const GraphicHwOps sm501_ops = { > .gfx_update = sm501_update_display, > }; > > -static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base, > +static void sm501_init(SM501State *s, DeviceState *dev, > uint32_t local_mem_bytes) > { > - s->base = base; > s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes); > SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s), > s->local_mem_size_index); > @@ -1461,7 +1459,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > DeviceState *usb_dev; > > - sm501_init(&s->state, dev, s->base, s->vram_size); > + sm501_init(&s->state, dev, s->vram_size); > sysbus_init_mmio(sbd, &s->state.local_mem_region); > sysbus_init_mmio(sbd, &s->state.mmio_region); > > diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h > index 16e500b..832ee61 100644 > --- a/hw/display/sm501_template.h > +++ b/hw/display/sm501_template.h > @@ -103,13 +103,13 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, > uint8_t *palette, int c_y, uint8_t *d, int width) > { > int x, i; > - uint8_t bitset = 0; > + uint8_t *pixval, bitset = 0; > > /* get hardware cursor pattern */ > uint32_t cursor_addr = get_hwc_address(s, crt); > assert(0 <= c_y && c_y < SM501_HWC_HEIGHT); > cursor_addr += SM501_HWC_WIDTH * c_y / 4; /* 4 pixels per byte */ > - cursor_addr += s->base; > + pixval = s->local_mem + cursor_addr; > > /* get cursor position */ > x = get_hwc_x(s, crt); > @@ -120,8 +120,8 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, > > /* get pixel value */ > if (i % 4 == 0) { > - bitset = ldub_phys(&address_space_memory, cursor_addr); > - cursor_addr++; > + bitset = ldub_p(pixval); > + pixval++; > } > v = bitset & 3; > bitset >>= 2; > -- > 2.7.4 (The data sheet suggests that the hardware cursor can also be in "external memory", but it looks like we never supported that previously and I can't figure out how it is supposed to work either. So no problem with restricting it to be in the local video ram blob.) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Thu, 2 Mar 2017, Peter Maydell wrote: > On 25 February 2017 at 18:23, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> Do not use the base address to access data in local memory. This is in >> preparation to allow chip connected via PCI where base address depends >> on where the BAR is mapped so it will be unknown. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/display/sm501.c | 6 ++---- >> hw/display/sm501_template.h | 8 ++++---- >> 2 files changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/hw/display/sm501.c b/hw/display/sm501.c >> index 22cac97..1cda127 100644 >> --- a/hw/display/sm501.c >> +++ b/hw/display/sm501.c >> @@ -460,7 +460,6 @@ typedef struct SM501State { >> QemuConsole *con; >> >> /* status & internal resources */ >> - hwaddr base; >> uint32_t local_mem_size_index; >> uint8_t *local_mem; >> MemoryRegion local_mem_region; >> @@ -1400,10 +1399,9 @@ static const GraphicHwOps sm501_ops = { >> .gfx_update = sm501_update_display, >> }; >> >> -static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base, >> +static void sm501_init(SM501State *s, DeviceState *dev, >> uint32_t local_mem_bytes) >> { >> - s->base = base; >> s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes); >> SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s), >> s->local_mem_size_index); >> @@ -1461,7 +1459,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) >> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> DeviceState *usb_dev; >> >> - sm501_init(&s->state, dev, s->base, s->vram_size); >> + sm501_init(&s->state, dev, s->vram_size); >> sysbus_init_mmio(sbd, &s->state.local_mem_region); >> sysbus_init_mmio(sbd, &s->state.mmio_region); >> >> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h >> index 16e500b..832ee61 100644 >> --- a/hw/display/sm501_template.h >> +++ b/hw/display/sm501_template.h >> @@ -103,13 +103,13 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, >> uint8_t *palette, int c_y, uint8_t *d, int width) >> { >> int x, i; >> - uint8_t bitset = 0; >> + uint8_t *pixval, bitset = 0; >> >> /* get hardware cursor pattern */ >> uint32_t cursor_addr = get_hwc_address(s, crt); >> assert(0 <= c_y && c_y < SM501_HWC_HEIGHT); >> cursor_addr += SM501_HWC_WIDTH * c_y / 4; /* 4 pixels per byte */ >> - cursor_addr += s->base; >> + pixval = s->local_mem + cursor_addr; >> >> /* get cursor position */ >> x = get_hwc_x(s, crt); >> @@ -120,8 +120,8 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, >> >> /* get pixel value */ >> if (i % 4 == 0) { >> - bitset = ldub_phys(&address_space_memory, cursor_addr); >> - cursor_addr++; >> + bitset = ldub_p(pixval); >> + pixval++; >> } >> v = bitset & 3; >> bitset >>= 2; >> -- >> 2.7.4 > > (The data sheet suggests that the hardware cursor can also be in > "external memory", but it looks like we never supported that > previously and I can't figure out how it is supposed to work either. Yes and as you say this was not implemented before and I haven't implemented it because no known client seems to use this. If found to be needed later, the Fix hardware cursor patch in this series cleans this up so the cursor data address is passed from the caller where it could possibly pass the external address that can be handled in get_hwc_address which is also changed to return the address instead of offset so dependency on local_memory is removed. > So no problem with restricting it to be in the local video ram blob.) > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM > >
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index 22cac97..1cda127 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -460,7 +460,6 @@ typedef struct SM501State { QemuConsole *con; /* status & internal resources */ - hwaddr base; uint32_t local_mem_size_index; uint8_t *local_mem; MemoryRegion local_mem_region; @@ -1400,10 +1399,9 @@ static const GraphicHwOps sm501_ops = { .gfx_update = sm501_update_display, }; -static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base, +static void sm501_init(SM501State *s, DeviceState *dev, uint32_t local_mem_bytes) { - s->base = base; s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes); SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s), s->local_mem_size_index); @@ -1461,7 +1459,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) SysBusDevice *sbd = SYS_BUS_DEVICE(dev); DeviceState *usb_dev; - sm501_init(&s->state, dev, s->base, s->vram_size); + sm501_init(&s->state, dev, s->vram_size); sysbus_init_mmio(sbd, &s->state.local_mem_region); sysbus_init_mmio(sbd, &s->state.mmio_region); diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h index 16e500b..832ee61 100644 --- a/hw/display/sm501_template.h +++ b/hw/display/sm501_template.h @@ -103,13 +103,13 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, uint8_t *palette, int c_y, uint8_t *d, int width) { int x, i; - uint8_t bitset = 0; + uint8_t *pixval, bitset = 0; /* get hardware cursor pattern */ uint32_t cursor_addr = get_hwc_address(s, crt); assert(0 <= c_y && c_y < SM501_HWC_HEIGHT); cursor_addr += SM501_HWC_WIDTH * c_y / 4; /* 4 pixels per byte */ - cursor_addr += s->base; + pixval = s->local_mem + cursor_addr; /* get cursor position */ x = get_hwc_x(s, crt); @@ -120,8 +120,8 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt, /* get pixel value */ if (i % 4 == 0) { - bitset = ldub_phys(&address_space_memory, cursor_addr); - cursor_addr++; + bitset = ldub_p(pixval); + pixval++; } v = bitset & 3; bitset >>= 2;
Do not use the base address to access data in local memory. This is in preparation to allow chip connected via PCI where base address depends on where the BAR is mapped so it will be unknown. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/display/sm501.c | 6 ++---- hw/display/sm501_template.h | 8 ++++---- 2 files changed, 6 insertions(+), 8 deletions(-)