Re: [Qemu-devel] [PATCH v2 04/14] sm501: Get rid of base address in draw_hwc_line

[BALATON Zoltan <balaton@eik.bme.hu>]

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
>
>