diff mbox

[v2,04/14] sm501: Get rid of base address in draw_hwc_line

Message ID 7c6e8c071cbd47d6d9065635e97dd88a7197b5be.1488068248.git.balaton@eik.bme.hu
State New
Headers show

Commit Message

BALATON Zoltan Feb. 25, 2017, 6:23 p.m. UTC
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(-)

Comments

Peter Maydell March 2, 2017, 7:14 p.m. UTC | #1
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
BALATON Zoltan March 2, 2017, 8:06 p.m. UTC | #2
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 mbox

Patch

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;