[Qemu-devel] [PATCH 00/14] TCX/CG3 adapter cleanups

[Qemu-devel] [PATCH 00/14] TCX/CG3 adapter cleanups

[Mark Cave-Ayland]

This started as part of the work on Gerd's thread-safe display patches
but ended up turning into something a lot more comprehensive. It contains
lots of cleanupm and fixes for display invalidation (particularly on 24-bit
TCX) exposed by testing the thread-safe patchset.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (14):
  cg3: remove TARGET_PAGE_SIZE rounding on dirty page detection
  cg3: fix up size parameter for memory_region_get_dirty()
  cg3: remove unused width and height variables
  cg3: switch to load_image_mr() and remove prom-addr hack
  tcx: alter tcx_set_dirty() to accept address and length parameters
  tcx: ensure tcx_set_dirty() also invalidates the 24-bit plane and
    cplane
  tcx: alter tcx24_check_dirty() to accept address and length
    parameters
  tcx: alter tcx24_reset_dirty() to accept address and length
    parameters
  tcx: remove page24 and cpage from tcx24_update_display()
  tcx: remove TARGET_PAGE_SIZE from tcx_update_display()
  tcx: remove TARGET_PAGE_SIZE from tcx24_update_display()
  tcx: remove primitives for non-32-bit surfaces
  tcx: use tcx_set_dirty() for accelerated ops
  tcx: switch to load_image_mr() and remove prom_addr hack

 hw/display/cg3.c |   23 ++---
 hw/display/tcx.c |  298 +++++++++++-------------------------------------------
 hw/sparc/sun4m.c |    2 -
 3 files changed, 69 insertions(+), 254 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 01/14] cg3: remove TARGET_PAGE_SIZE rounding on dirty page detection

[Mark Cave-Ayland]

This was an artifact from very early versions of the code from before the
memory API and is no longer needed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 1174220..7d43694 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -26,7 +26,6 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
-#include "cpu.h"
 #include "qemu/error-report.h"
 #include "ui/console.h"
 #include "hw/sysbus.h"
@@ -114,7 +113,7 @@ static void cg3_update_display(void *opaque)
     for (y = 0; y < height; y++) {
         int update = s->full_update;
 
-        page = (y * width) & TARGET_PAGE_MASK;
+        page = y * width;
         update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
                                           DIRTY_MEMORY_VGA);
         if (update) {
@@ -148,8 +147,7 @@ static void cg3_update_display(void *opaque)
     }
     if (page_max >= page_min) {
         memory_region_reset_dirty(&s->vram_mem,
-                              page_min, page_max - page_min + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
+                              page_min, page_max - page_min, DIRTY_MEMORY_VGA);
     }
     /* vsync interrupt? */
     if (s->regs[0] & CG3_CR_ENABLE_INTS) {
-- 
1.7.10.4

[Qemu-devel] [PATCH 02/14] cg3: fix up size parameter for memory_region_get_dirty()

[Mark Cave-Ayland]

The code was incorrectly calculating the end address rather than the size of
the required region.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 7d43694..b42f60e 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -114,7 +114,7 @@ static void cg3_update_display(void *opaque)
         int update = s->full_update;
 
         page = y * width;
-        update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
+        update |= memory_region_get_dirty(&s->vram_mem, page, width,
                                           DIRTY_MEMORY_VGA);
         if (update) {
             if (y_start < 0) {
-- 
1.7.10.4

[Qemu-devel] [PATCH 03/14] cg3: remove unused width and height variables

[Mark Cave-Ayland]

These aren't required since we can use the display width and height directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index b42f60e..178a6dd 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -93,14 +93,11 @@ static void cg3_update_display(void *opaque)
     uint32_t *data;
     uint32_t dval;
     int x, y, y_start;
-    unsigned int width, height;
     ram_addr_t page, page_min, page_max;
 
     if (surface_bits_per_pixel(surface) != 32) {
         return;
     }
-    width = s->width;
-    height = s->height;
 
     y_start = -1;
     page_min = -1;
@@ -110,11 +107,11 @@ static void cg3_update_display(void *opaque)
     data = (uint32_t *)surface_data(surface);
 
     memory_region_sync_dirty_bitmap(&s->vram_mem);
-    for (y = 0; y < height; y++) {
+    for (y = 0; y < s->height; y++) {
         int update = s->full_update;
 
-        page = y * width;
-        update |= memory_region_get_dirty(&s->vram_mem, page, width,
+        page = y * s->width;
+        update |= memory_region_get_dirty(&s->vram_mem, page, s->width,
                                           DIRTY_MEMORY_VGA);
         if (update) {
             if (y_start < 0) {
@@ -127,7 +124,7 @@ static void cg3_update_display(void *opaque)
                 page_max = page;
             }
 
-            for (x = 0; x < width; x++) {
+            for (x = 0; x < s->width; x++) {
                 dval = *pix++;
                 dval = (s->r[dval] << 16) | (s->g[dval] << 8) | s->b[dval];
                 *data++ = dval;
@@ -137,8 +134,8 @@ static void cg3_update_display(void *opaque)
                 dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
                 y_start = -1;
             }
-            pix += width;
-            data += width;
+            pix += s->width;
+            data += s->width;
         }
     }
     s->full_update = 0;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 03/14] cg3: remove unused width and height variables

[Philippe Mathieu-Daudé]

On 04/05/2017 05:35 AM, Mark Cave-Ayland wrote:
> These aren't required since we can use the display width and height directly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/display/cg3.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index b42f60e..178a6dd 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -93,14 +93,11 @@ static void cg3_update_display(void *opaque)
>      uint32_t *data;
>      uint32_t dval;
>      int x, y, y_start;
> -    unsigned int width, height;
>      ram_addr_t page, page_min, page_max;
>
>      if (surface_bits_per_pixel(surface) != 32) {
>          return;
>      }
> -    width = s->width;
> -    height = s->height;
>
>      y_start = -1;
>      page_min = -1;
> @@ -110,11 +107,11 @@ static void cg3_update_display(void *opaque)
>      data = (uint32_t *)surface_data(surface);
>
>      memory_region_sync_dirty_bitmap(&s->vram_mem);
> -    for (y = 0; y < height; y++) {
> +    for (y = 0; y < s->height; y++) {
>          int update = s->full_update;
>
> -        page = y * width;
> -        update |= memory_region_get_dirty(&s->vram_mem, page, width,
> +        page = y * s->width;
> +        update |= memory_region_get_dirty(&s->vram_mem, page, s->width,
>                                            DIRTY_MEMORY_VGA);
>          if (update) {
>              if (y_start < 0) {
> @@ -127,7 +124,7 @@ static void cg3_update_display(void *opaque)
>                  page_max = page;
>              }
>
> -            for (x = 0; x < width; x++) {
> +            for (x = 0; x < s->width; x++) {
>                  dval = *pix++;
>                  dval = (s->r[dval] << 16) | (s->g[dval] << 8) | s->b[dval];
>                  *data++ = dval;
> @@ -137,8 +134,8 @@ static void cg3_update_display(void *opaque)
>                  dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
>                  y_start = -1;
>              }
> -            pix += width;
> -            data += width;
> +            pix += s->width;
> +            data += s->width;
>          }
>      }
>      s->full_update = 0;
>

Re: [Qemu-devel] [PATCH 03/14] cg3: remove unused width and height variables

[Richard Henderson]

On 04/05/2017 01:35 AM, Mark Cave-Ayland wrote:
> These aren't required since we can use the display width and height directly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/cg3.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index b42f60e..178a6dd 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -93,14 +93,11 @@ static void cg3_update_display(void *opaque)
>      uint32_t *data;
>      uint32_t dval;
>      int x, y, y_start;
> -    unsigned int width, height;
>      ram_addr_t page, page_min, page_max;
>
>      if (surface_bits_per_pixel(surface) != 32) {
>          return;
>      }
> -    width = s->width;
> -    height = s->height;
>
>      y_start = -1;
>      page_min = -1;
> @@ -110,11 +107,11 @@ static void cg3_update_display(void *opaque)
>      data = (uint32_t *)surface_data(surface);
>
>      memory_region_sync_dirty_bitmap(&s->vram_mem);
> -    for (y = 0; y < height; y++) {
> +    for (y = 0; y < s->height; y++) {

I suspect the generated code is much worse, since the compiler can no longer 
tell that the loop bounds are constant.

You probably do want to keep width and height in local variables across this 
function.


r~

Re: [Qemu-devel] [PATCH 03/14] cg3: remove unused width and height variables

[Mark Cave-Ayland]

On 15/04/17 11:54, Richard Henderson wrote:

> On 04/05/2017 01:35 AM, Mark Cave-Ayland wrote:
>> These aren't required since we can use the display width and height
>> directly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/display/cg3.c |   15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
>> index b42f60e..178a6dd 100644
>> --- a/hw/display/cg3.c
>> +++ b/hw/display/cg3.c
>> @@ -93,14 +93,11 @@ static void cg3_update_display(void *opaque)
>>      uint32_t *data;
>>      uint32_t dval;
>>      int x, y, y_start;
>> -    unsigned int width, height;
>>      ram_addr_t page, page_min, page_max;
>>
>>      if (surface_bits_per_pixel(surface) != 32) {
>>          return;
>>      }
>> -    width = s->width;
>> -    height = s->height;
>>
>>      y_start = -1;
>>      page_min = -1;
>> @@ -110,11 +107,11 @@ static void cg3_update_display(void *opaque)
>>      data = (uint32_t *)surface_data(surface);
>>
>>      memory_region_sync_dirty_bitmap(&s->vram_mem);
>> -    for (y = 0; y < height; y++) {
>> +    for (y = 0; y < s->height; y++) {
> 
> I suspect the generated code is much worse, since the compiler can no
> longer tell that the loop bounds are constant.
> 
> You probably do want to keep width and height in local variables across
> this function.

Can you explain a bit more about this? My thoughts were that with
optimisation enabled the compiler would assume s->width is constant
throughout the loop by default (or at least in OpenBIOS I've had to
explicitly mark variables as volatile to ensure the compiler refetches
the original value instead of reusing a previous copy from the
stack/registers)?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 03/14] cg3: remove unused width and height variables

[Richard Henderson]

On 04/18/2017 07:38 AM, Mark Cave-Ayland wrote:
> On 15/04/17 11:54, Richard Henderson wrote:
>
>> On 04/05/2017 01:35 AM, Mark Cave-Ayland wrote:
>>> These aren't required since we can use the display width and height
>>> directly.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/display/cg3.c |   15 ++++++---------
>>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
>>> index b42f60e..178a6dd 100644
>>> --- a/hw/display/cg3.c
>>> +++ b/hw/display/cg3.c
>>> @@ -93,14 +93,11 @@ static void cg3_update_display(void *opaque)
>>>      uint32_t *data;
>>>      uint32_t dval;
>>>      int x, y, y_start;
>>> -    unsigned int width, height;
>>>      ram_addr_t page, page_min, page_max;
>>>
>>>      if (surface_bits_per_pixel(surface) != 32) {
>>>          return;
>>>      }
>>> -    width = s->width;
>>> -    height = s->height;
>>>
>>>      y_start = -1;
>>>      page_min = -1;
>>> @@ -110,11 +107,11 @@ static void cg3_update_display(void *opaque)
>>>      data = (uint32_t *)surface_data(surface);
>>>
>>>      memory_region_sync_dirty_bitmap(&s->vram_mem);
>>> -    for (y = 0; y < height; y++) {
>>> +    for (y = 0; y < s->height; y++) {
>>
>> I suspect the generated code is much worse, since the compiler can no
>> longer tell that the loop bounds are constant.
>>
>> You probably do want to keep width and height in local variables across
>> this function.
>
> Can you explain a bit more about this? My thoughts were that with
> optimisation enabled the compiler would assume s->width is constant
> throughout the loop by default (or at least in OpenBIOS I've had to
> explicitly mark variables as volatile to ensure the compiler refetches
> the original value instead of reusing a previous copy from the
> stack/registers)?

With data in memory, as for foo->bar, the compiler is obliged to consider the 
memory may be changed for e.g. an intervening function call baz(), when foo is 
"global", as it is here.  You have many of those in those loops.


r~

Re: [Qemu-devel] [PATCH 03/14] cg3: remove unused width and height variables

[Mark Cave-Ayland]

On 18/04/17 15:55, Richard Henderson wrote:

> On 04/18/2017 07:38 AM, Mark Cave-Ayland wrote:
>> On 15/04/17 11:54, Richard Henderson wrote:
>>
>>> On 04/05/2017 01:35 AM, Mark Cave-Ayland wrote:
>>>> These aren't required since we can use the display width and height
>>>> directly.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  hw/display/cg3.c |   15 ++++++---------
>>>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
>>>> index b42f60e..178a6dd 100644
>>>> --- a/hw/display/cg3.c
>>>> +++ b/hw/display/cg3.c
>>>> @@ -93,14 +93,11 @@ static void cg3_update_display(void *opaque)
>>>>      uint32_t *data;
>>>>      uint32_t dval;
>>>>      int x, y, y_start;
>>>> -    unsigned int width, height;
>>>>      ram_addr_t page, page_min, page_max;
>>>>
>>>>      if (surface_bits_per_pixel(surface) != 32) {
>>>>          return;
>>>>      }
>>>> -    width = s->width;
>>>> -    height = s->height;
>>>>
>>>>      y_start = -1;
>>>>      page_min = -1;
>>>> @@ -110,11 +107,11 @@ static void cg3_update_display(void *opaque)
>>>>      data = (uint32_t *)surface_data(surface);
>>>>
>>>>      memory_region_sync_dirty_bitmap(&s->vram_mem);
>>>> -    for (y = 0; y < height; y++) {
>>>> +    for (y = 0; y < s->height; y++) {
>>>
>>> I suspect the generated code is much worse, since the compiler can no
>>> longer tell that the loop bounds are constant.
>>>
>>> You probably do want to keep width and height in local variables across
>>> this function.
>>
>> Can you explain a bit more about this? My thoughts were that with
>> optimisation enabled the compiler would assume s->width is constant
>> throughout the loop by default (or at least in OpenBIOS I've had to
>> explicitly mark variables as volatile to ensure the compiler refetches
>> the original value instead of reusing a previous copy from the
>> stack/registers)?
> 
> With data in memory, as for foo->bar, the compiler is obliged to
> consider the memory may be changed for e.g. an intervening function call
> baz(), when foo is "global", as it is here.  You have many of those in
> those loops.

Given that you know a lot more about this than I do, I'll drop this
patch for now and send a v2. I see that TCX references s->width in a
similar way, however I'm going to be AFK for a while and so I'll try and
send a PR later today since this is a blocker on Gerd's VGA fixes
patchset - TCX can be altered later in a similar way if it is deemed to
give a noticeable performance benefit.


ATB,

Mark.

[Qemu-devel] [PATCH 04/14] cg3: switch to load_image_mr() and remove prom-addr hack

[Mark Cave-Ayland]

Previous to the existence of load_image_mr(), the only way to load in the
FCode ROM image was to pass in its physical address via qdev properties
and use load_image_targphys().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/cg3.c |    4 +---
 hw/sparc/sun4m.c |    1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 178a6dd..3d36960 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -300,8 +300,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
     vmstate_register_ram_global(&s->rom);
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
     if (fcode_filename) {
-        ret = load_image_targphys(fcode_filename, s->prom_addr,
-                                  FCODE_MAX_ROM_SIZE);
+        ret = load_image_mr(fcode_filename, &s->rom);
         g_free(fcode_filename);
         if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
             error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
@@ -366,7 +365,6 @@ static Property cg3_properties[] = {
     DEFINE_PROP_UINT16("width",        CG3State, width,     -1),
     DEFINE_PROP_UINT16("height",       CG3State, height,    -1),
     DEFINE_PROP_UINT16("depth",        CG3State, depth,     -1),
-    DEFINE_PROP_UINT64("prom-addr",    CG3State, prom_addr, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 873cd7d..7a0922b 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -544,7 +544,6 @@ static void cg3_init(hwaddr addr, qemu_irq irq, int vram_size, int width,
     qdev_prop_set_uint16(dev, "width", width);
     qdev_prop_set_uint16(dev, "height", height);
     qdev_prop_set_uint16(dev, "depth", depth);
-    qdev_prop_set_uint64(dev, "prom-addr", addr);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 04/14] cg3: switch to load_image_mr() and remove prom-addr hack

[Philippe Mathieu-Daudé]

On 04/05/2017 05:35 AM, Mark Cave-Ayland wrote:
> Previous to the existence of load_image_mr(), the only way to load in the
> FCode ROM image was to pass in its physical address via qdev properties
> and use load_image_targphys().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/display/cg3.c |    4 +---
>  hw/sparc/sun4m.c |    1 -
>  2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index 178a6dd..3d36960 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -300,8 +300,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>      vmstate_register_ram_global(&s->rom);
>      fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
>      if (fcode_filename) {
> -        ret = load_image_targphys(fcode_filename, s->prom_addr,
> -                                  FCODE_MAX_ROM_SIZE);
> +        ret = load_image_mr(fcode_filename, &s->rom);
>          g_free(fcode_filename);
>          if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
>              error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
> @@ -366,7 +365,6 @@ static Property cg3_properties[] = {
>      DEFINE_PROP_UINT16("width",        CG3State, width,     -1),
>      DEFINE_PROP_UINT16("height",       CG3State, height,    -1),
>      DEFINE_PROP_UINT16("depth",        CG3State, depth,     -1),
> -    DEFINE_PROP_UINT64("prom-addr",    CG3State, prom_addr, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 873cd7d..7a0922b 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -544,7 +544,6 @@ static void cg3_init(hwaddr addr, qemu_irq irq, int vram_size, int width,
>      qdev_prop_set_uint16(dev, "width", width);
>      qdev_prop_set_uint16(dev, "height", height);
>      qdev_prop_set_uint16(dev, "depth", depth);
> -    qdev_prop_set_uint64(dev, "prom-addr", addr);
>      qdev_init_nofail(dev);
>      s = SYS_BUS_DEVICE(dev);
>
>

[Qemu-devel] [PATCH 05/14] tcx: alter tcx_set_dirty() to accept address and length parameters

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 8e26aae..d24466f 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -93,9 +93,9 @@ typedef struct TCXState {
     uint16_t cursy;
 } TCXState;
 
-static void tcx_set_dirty(TCXState *s)
+static void tcx_set_dirty(TCXState *s, ram_addr_t addr, int len)
 {
-    memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
+    memory_region_set_dirty(&s->vram_mem, addr, len);
 }
 
 static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
@@ -156,7 +156,7 @@ static void update_palette_entries(TCXState *s, int start, int end)
             break;
         }
     }
-    tcx_set_dirty(s);
+    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
 }
 
 static void tcx_draw_line32(TCXState *s1, uint8_t *d,
@@ -526,7 +526,7 @@ static void tcx_invalidate_display(void *opaque)
 {
     TCXState *s = opaque;
 
-    tcx_set_dirty(s);
+    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
     qemu_console_resize(s->con, s->width, s->height);
 }
 
@@ -534,7 +534,7 @@ static void tcx24_invalidate_display(void *opaque)
 {
     TCXState *s = opaque;
 
-    tcx_set_dirty(s);
+    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
     qemu_console_resize(s->con, s->width, s->height);
 }
 
@@ -543,7 +543,7 @@ static int vmstate_tcx_post_load(void *opaque, int version_id)
     TCXState *s = opaque;
 
     update_palette_entries(s, 0, 256);
-    tcx_set_dirty(s);
+    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
     return 0;
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 05/14] tcx: alter tcx_set_dirty() to accept address and length parameters

[Philippe Mathieu-Daudé]

Hi Mark,

On 04/05/2017 05:35 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/tcx.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 8e26aae..d24466f 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -93,9 +93,9 @@ typedef struct TCXState {
>      uint16_t cursy;
>  } TCXState;
>
> -static void tcx_set_dirty(TCXState *s)
> +static void tcx_set_dirty(TCXState *s, ram_addr_t addr, int len)

len is uint64_t

with this change:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

This function can also be inlined.

>  {
> -    memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
> +    memory_region_set_dirty(&s->vram_mem, addr, len);
>  }
>
>  static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
> @@ -156,7 +156,7 @@ static void update_palette_entries(TCXState *s, int start, int end)
>              break;
>          }
>      }
> -    tcx_set_dirty(s);
> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>  }
>
>  static void tcx_draw_line32(TCXState *s1, uint8_t *d,
> @@ -526,7 +526,7 @@ static void tcx_invalidate_display(void *opaque)
>  {
>      TCXState *s = opaque;
>
> -    tcx_set_dirty(s);
> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>      qemu_console_resize(s->con, s->width, s->height);
>  }
>
> @@ -534,7 +534,7 @@ static void tcx24_invalidate_display(void *opaque)
>  {
>      TCXState *s = opaque;
>
> -    tcx_set_dirty(s);
> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>      qemu_console_resize(s->con, s->width, s->height);
>  }
>
> @@ -543,7 +543,7 @@ static int vmstate_tcx_post_load(void *opaque, int version_id)
>      TCXState *s = opaque;
>
>      update_palette_entries(s, 0, 256);
> -    tcx_set_dirty(s);
> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>      return 0;
>  }
>
>

Re: [Qemu-devel] [PATCH 05/14] tcx: alter tcx_set_dirty() to accept address and length parameters

[Mark Cave-Ayland]

On 15/04/17 15:27, Philippe Mathieu-Daudé wrote:

Hi Philippe,

> Hi Mark,
> 
> On 04/05/2017 05:35 AM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/display/tcx.c |   12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
>> index 8e26aae..d24466f 100644
>> --- a/hw/display/tcx.c
>> +++ b/hw/display/tcx.c
>> @@ -93,9 +93,9 @@ typedef struct TCXState {
>>      uint16_t cursy;
>>  } TCXState;
>>
>> -static void tcx_set_dirty(TCXState *s)
>> +static void tcx_set_dirty(TCXState *s, ram_addr_t addr, int len)
> 
> len is uint64_t

Yes, this was deliberate in order to match the existing tcx_* functions
in the same file which tend to use int for len/width parameters. Note
that TCX is fixed 1024x768 resolution giving a maximum len of ~800K so
there is no risk of overflow here.

> with this change:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This function can also be inlined.

My current understanding is that these days compilers tend to handle
inlining themselves and inline is either a no-op or a very weak hint?

>>  {
>> -    memory_region_set_dirty(&s->vram_mem, 0, MAXX * MAXY);
>> +    memory_region_set_dirty(&s->vram_mem, addr, len);
>>  }
>>
>>  static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
>> @@ -156,7 +156,7 @@ static void update_palette_entries(TCXState *s,
>> int start, int end)
>>              break;
>>          }
>>      }
>> -    tcx_set_dirty(s);
>> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>>  }
>>
>>  static void tcx_draw_line32(TCXState *s1, uint8_t *d,
>> @@ -526,7 +526,7 @@ static void tcx_invalidate_display(void *opaque)
>>  {
>>      TCXState *s = opaque;
>>
>> -    tcx_set_dirty(s);
>> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>>      qemu_console_resize(s->con, s->width, s->height);
>>  }
>>
>> @@ -534,7 +534,7 @@ static void tcx24_invalidate_display(void *opaque)
>>  {
>>      TCXState *s = opaque;
>>
>> -    tcx_set_dirty(s);
>> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>>      qemu_console_resize(s->con, s->width, s->height);
>>  }
>>
>> @@ -543,7 +543,7 @@ static int vmstate_tcx_post_load(void *opaque, int
>> version_id)
>>      TCXState *s = opaque;
>>
>>      update_palette_entries(s, 0, 256);
>> -    tcx_set_dirty(s);
>> +    tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>>      return 0;
>>  }

ATB,

Mark.

[Qemu-devel] [PATCH 06/14] tcx: ensure tcx_set_dirty() also invalidates the 24-bit plane and cplane

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index d24466f..6817bd2 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -96,6 +96,13 @@ typedef struct TCXState {
 static void tcx_set_dirty(TCXState *s, ram_addr_t addr, int len)
 {
     memory_region_set_dirty(&s->vram_mem, addr, len);
+
+    if (s->depth == 24) {
+        memory_region_set_dirty(&s->vram_mem, s->vram24_offset + addr * 4,
+                                len * 4);
+        memory_region_set_dirty(&s->vram_mem, s->cplane_offset + addr * 4,
+                                len * 4);
+    }
 }
 
 static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
-- 
1.7.10.4

[Qemu-devel] [PATCH 07/14] tcx: alter tcx24_check_dirty() to accept address and length parameters

[Mark Cave-Ayland]

This can now be used by both the 8-bit and 24-bit display code, so rename
to tcx_check_dirty().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |   25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 6817bd2..171236a 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -105,17 +105,21 @@ static void tcx_set_dirty(TCXState *s, ram_addr_t addr, int len)
     }
 }
 
-static inline int tcx24_check_dirty(TCXState *s, ram_addr_t page,
-                                    ram_addr_t page24, ram_addr_t cpage)
+static int tcx_check_dirty(TCXState *s, ram_addr_t addr, int len)
 {
     int ret;
 
-    ret = memory_region_get_dirty(&s->vram_mem, page, TARGET_PAGE_SIZE,
-                                  DIRTY_MEMORY_VGA);
-    ret |= memory_region_get_dirty(&s->vram_mem, page24, TARGET_PAGE_SIZE * 4,
-                                   DIRTY_MEMORY_VGA);
-    ret |= memory_region_get_dirty(&s->vram_mem, cpage, TARGET_PAGE_SIZE * 4,
-                                   DIRTY_MEMORY_VGA);
+    ret = memory_region_get_dirty(&s->vram_mem, addr, len, DIRTY_MEMORY_VGA);
+
+    if (s->depth == 24) {
+        ret |= memory_region_get_dirty(&s->vram_mem,
+                                       s->vram24_offset + addr * 4, len * 4,
+                                       DIRTY_MEMORY_VGA);
+        ret |= memory_region_get_dirty(&s->vram_mem,
+                                       s->cplane_offset + addr * 4, len * 4,
+                                       DIRTY_MEMORY_VGA);
+    }
+
     return ret;
 }
 
@@ -366,8 +370,7 @@ static void tcx_update_display(void *opaque)
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
     for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
-        if (memory_region_get_dirty(&ts->vram_mem, page, TARGET_PAGE_SIZE,
-                                    DIRTY_MEMORY_VGA)) {
+        if (tcx_check_dirty(ts, page, TARGET_PAGE_SIZE)) {
             if (y_start < 0)
                 y_start = y;
             if (page < page_min)
@@ -461,7 +464,7 @@ static void tcx24_update_display(void *opaque)
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
     for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE,
             page24 += TARGET_PAGE_SIZE, cpage += TARGET_PAGE_SIZE) {
-        if (tcx24_check_dirty(ts, page, page24, cpage)) {
+        if (tcx_check_dirty(ts, page, TARGET_PAGE_SIZE)) {
             if (y_start < 0)
                 y_start = y;
             if (page < page_min)
-- 
1.7.10.4

[Qemu-devel] [PATCH 08/14] tcx: alter tcx24_reset_dirty() to accept address and length parameters

[Mark Cave-Ayland]

This can now be used by both the 8-bit and 24-bit display code, so rename
to tcx_check_dirty().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |   31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 171236a..e9056e0 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -123,22 +123,16 @@ static int tcx_check_dirty(TCXState *s, ram_addr_t addr, int len)
     return ret;
 }
 
-static inline void tcx24_reset_dirty(TCXState *ts, ram_addr_t page_min,
-                               ram_addr_t page_max, ram_addr_t page24,
-                              ram_addr_t cpage)
+static void tcx_reset_dirty(TCXState *s, ram_addr_t addr, int len)
 {
-    memory_region_reset_dirty(&ts->vram_mem,
-                              page_min,
-                              (page_max - page_min) + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
-    memory_region_reset_dirty(&ts->vram_mem,
-                              page24 + page_min * 4,
-                              (page_max - page_min) * 4 + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
-    memory_region_reset_dirty(&ts->vram_mem,
-                              cpage + page_min * 4,
-                              (page_max - page_min) * 4 + TARGET_PAGE_SIZE,
-                              DIRTY_MEMORY_VGA);
+    memory_region_reset_dirty(&s->vram_mem, addr, len, DIRTY_MEMORY_VGA);
+
+    if (s->depth == 24) {
+        memory_region_reset_dirty(&s->vram_mem, s->vram24_offset + addr * 4,
+                                  len * 4, DIRTY_MEMORY_VGA);
+        memory_region_reset_dirty(&s->vram_mem, s->cplane_offset + addr * 4,
+                                  len * 4, DIRTY_MEMORY_VGA);
+    }
 }
 
 static void update_palette_entries(TCXState *s, int start, int end)
@@ -428,10 +422,7 @@ static void tcx_update_display(void *opaque)
     }
     /* reset modified pages */
     if (page_max >= page_min) {
-        memory_region_reset_dirty(&ts->vram_mem,
-                                  page_min,
-                                  (page_max - page_min) + TARGET_PAGE_SIZE,
-                                  DIRTY_MEMORY_VGA);
+        tcx_reset_dirty(ts, page_min, page_max - page_min);
     }
 }
 
@@ -528,7 +519,7 @@ static void tcx24_update_display(void *opaque)
     }
     /* reset modified pages */
     if (page_max >= page_min) {
-        tcx24_reset_dirty(ts, page_min, page_max, page24, cpage);
+        tcx_reset_dirty(ts, page_min, page_max - page_min);
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 09/14] tcx: remove page24 and cpage from tcx24_update_display()

[Mark Cave-Ayland]

Since all of the tcx_*_dirty() functions now calculate the 24-bit and
cplane offsets themselves from the base address, these variables are no
longer needed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index e9056e0..4f4f03f 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -430,7 +430,7 @@ static void tcx24_update_display(void *opaque)
 {
     TCXState *ts = opaque;
     DisplaySurface *surface = qemu_console_surface(ts->con);
-    ram_addr_t page, page_min, page_max, cpage, page24;
+    ram_addr_t page, page_min, page_max;
     int y, y_start, dd, ds;
     uint8_t *d, *s;
     uint32_t *cptr, *s24;
@@ -440,8 +440,6 @@ static void tcx24_update_display(void *opaque)
     }
 
     page = 0;
-    page24 = ts->vram24_offset;
-    cpage = ts->cplane_offset;
     y_start = -1;
     page_min = -1;
     page_max = 0;
@@ -453,8 +451,7 @@ static void tcx24_update_display(void *opaque)
     ds = 1024;
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
-    for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE,
-            page24 += TARGET_PAGE_SIZE, cpage += TARGET_PAGE_SIZE) {
+    for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
         if (tcx_check_dirty(ts, page, TARGET_PAGE_SIZE)) {
             if (y_start < 0)
                 y_start = y;
-- 
1.7.10.4

[Qemu-devel] [PATCH 10/14] tcx: remove TARGET_PAGE_SIZE from tcx_update_display()

[Mark Cave-Ayland]

Now that page alignment is handled by the memory API, there is no need to
duplicate the code 4 times (4 * 1024 == 4096 == TARGET_PAGE_SIZE).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |   36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 4f4f03f..14a17fe 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -363,8 +363,8 @@ static void tcx_update_display(void *opaque)
     }
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
-    for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
-        if (tcx_check_dirty(ts, page, TARGET_PAGE_SIZE)) {
+    for (y = 0; y < ts->height; y++, page += ds) {
+        if (tcx_check_dirty(ts, page, ds)) {
             if (y_start < 0)
                 y_start = y;
             if (page < page_min)
@@ -376,33 +376,6 @@ static void tcx_update_display(void *opaque)
             if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
                 fc(ts, d, y, ts->width);
             }
-            d += dd;
-            s += ds;
-            y++;
-
-            f(ts, d, s, ts->width);
-            if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
-                fc(ts, d, y, ts->width);
-            }
-            d += dd;
-            s += ds;
-            y++;
-
-            f(ts, d, s, ts->width);
-            if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
-                fc(ts, d, y, ts->width);
-            }
-            d += dd;
-            s += ds;
-            y++;
-
-            f(ts, d, s, ts->width);
-            if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
-                fc(ts, d, y, ts->width);
-            }
-            d += dd;
-            s += ds;
-            y++;
         } else {
             if (y_start >= 0) {
                 /* flush to display */
@@ -410,10 +383,9 @@ static void tcx_update_display(void *opaque)
                                ts->width, y - y_start);
                 y_start = -1;
             }
-            d += dd * 4;
-            s += ds * 4;
-            y += 4;
         }
+        s += ds;
+        d += dd;
     }
     if (y_start >= 0) {
         /* flush to display */
-- 
1.7.10.4

[Qemu-devel] [PATCH 11/14] tcx: remove TARGET_PAGE_SIZE from tcx24_update_display()

[Mark Cave-Ayland]

Now that page alignment is handled by the memory API, there is no need to
duplicate the code 4 times (4 * 1024 == 4096 == TARGET_PAGE_SIZE).

Finally we have now removed all traces of TARGET_PAGE_SIZE.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |   46 ++++++----------------------------------------
 1 file changed, 6 insertions(+), 40 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 14a17fe..0dd007e 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -25,7 +25,6 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
-#include "cpu.h" /* FIXME shouldn't use TARGET_PAGE_SIZE */
 #include "ui/console.h"
 #include "ui/pixel_ops.h"
 #include "hw/loader.h"
@@ -423,8 +422,8 @@ static void tcx24_update_display(void *opaque)
     ds = 1024;
 
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
-    for (y = 0; y < ts->height; page += TARGET_PAGE_SIZE) {
-        if (tcx_check_dirty(ts, page, TARGET_PAGE_SIZE)) {
+    for (y = 0; y < ts->height; y++, page += ds) {
+        if (tcx_check_dirty(ts, page, ds)) {
             if (y_start < 0)
                 y_start = y;
             if (page < page_min)
@@ -435,38 +434,6 @@ static void tcx24_update_display(void *opaque)
             if (y >= ts->cursy && y < ts->cursy+32 && ts->cursx < ts->width) {
                 tcx_draw_cursor32(ts, d, y, ts->width);
             }
-            d += dd;
-            s += ds;
-            cptr += ds;
-            s24 += ds;
-            y++;
-            tcx24_draw_line32(ts, d, s, ts->width, cptr, s24);
-            if (y >= ts->cursy && y < ts->cursy+32 && ts->cursx < ts->width) {
-                tcx_draw_cursor32(ts, d, y, ts->width);
-            }
-            d += dd;
-            s += ds;
-            cptr += ds;
-            s24 += ds;
-            y++;
-            tcx24_draw_line32(ts, d, s, ts->width, cptr, s24);
-            if (y >= ts->cursy && y < ts->cursy+32 && ts->cursx < ts->width) {
-                tcx_draw_cursor32(ts, d, y, ts->width);
-            }
-            d += dd;
-            s += ds;
-            cptr += ds;
-            s24 += ds;
-            y++;
-            tcx24_draw_line32(ts, d, s, ts->width, cptr, s24);
-            if (y >= ts->cursy && y < ts->cursy+32 && ts->cursx < ts->width) {
-                tcx_draw_cursor32(ts, d, y, ts->width);
-            }
-            d += dd;
-            s += ds;
-            cptr += ds;
-            s24 += ds;
-            y++;
         } else {
             if (y_start >= 0) {
                 /* flush to display */
@@ -474,12 +441,11 @@ static void tcx24_update_display(void *opaque)
                                ts->width, y - y_start);
                 y_start = -1;
             }
-            d += dd * 4;
-            s += ds * 4;
-            cptr += ds * 4;
-            s24 += ds * 4;
-            y += 4;
         }
+        d += dd;
+        s += ds;
+        cptr += ds;
+        s24 += ds;
     }
     if (y_start >= 0) {
         /* flush to display */
-- 
1.7.10.4

[Qemu-devel] [PATCH 12/14] tcx: remove primitives for non-32-bit surfaces

[Mark Cave-Ayland]

As all surfaces in QEMU are now either shared or 32-bit ARGB regardless of
the guest depth, remove all non-32-bit primitives from tcx_update_display()
and consequence their implementation which are no longer required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |  126 ++++--------------------------------------------------
 1 file changed, 8 insertions(+), 118 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 0dd007e..6da6dfb 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -140,25 +140,12 @@ static void update_palette_entries(TCXState *s, int start, int end)
     int i;
 
     for (i = start; i < end; i++) {
-        switch (surface_bits_per_pixel(surface)) {
-        default:
-        case 8:
-            s->palette[i] = rgb_to_pixel8(s->r[i], s->g[i], s->b[i]);
-            break;
-        case 15:
-            s->palette[i] = rgb_to_pixel15(s->r[i], s->g[i], s->b[i]);
-            break;
-        case 16:
-            s->palette[i] = rgb_to_pixel16(s->r[i], s->g[i], s->b[i]);
-            break;
-        case 32:
-            if (is_surface_bgr(surface)) {
-                s->palette[i] = rgb_to_pixel32bgr(s->r[i], s->g[i], s->b[i]);
-            } else {
-                s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
-            }
-            break;
+        if (is_surface_bgr(surface)) {
+            s->palette[i] = rgb_to_pixel32bgr(s->r[i], s->g[i], s->b[i]);
+        } else {
+            s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
         }
+        break;
     }
     tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
 }
@@ -176,31 +163,6 @@ static void tcx_draw_line32(TCXState *s1, uint8_t *d,
     }
 }
 
-static void tcx_draw_line16(TCXState *s1, uint8_t *d,
-                            const uint8_t *s, int width)
-{
-    int x;
-    uint8_t val;
-    uint16_t *p = (uint16_t *)d;
-
-    for (x = 0; x < width; x++) {
-        val = *s++;
-        *p++ = s1->palette[val];
-    }
-}
-
-static void tcx_draw_line8(TCXState *s1, uint8_t *d,
-                           const uint8_t *s, int width)
-{
-    int x;
-    uint8_t val;
-
-    for(x = 0; x < width; x++) {
-        val = *s++;
-        *d++ = s1->palette[val];
-    }
-}
-
 static void tcx_draw_cursor32(TCXState *s1, uint8_t *d,
                               int y, int width)
 {
@@ -227,57 +189,6 @@ static void tcx_draw_cursor32(TCXState *s1, uint8_t *d,
     }
 }
 
-static void tcx_draw_cursor16(TCXState *s1, uint8_t *d,
-                              int y, int width)
-{
-    int x, len;
-    uint32_t mask, bits;
-    uint16_t *p = (uint16_t *)d;
-
-    y = y - s1->cursy;
-    mask = s1->cursmask[y];
-    bits = s1->cursbits[y];
-    len = MIN(width - s1->cursx, 32);
-    p = &p[s1->cursx];
-    for (x = 0; x < len; x++) {
-        if (mask & 0x80000000) {
-            if (bits & 0x80000000) {
-                *p = s1->palette[259];
-            } else {
-                *p = s1->palette[258];
-            }
-        }
-        p++;
-        mask <<= 1;
-        bits <<= 1;
-    }
-}
-
-static void tcx_draw_cursor8(TCXState *s1, uint8_t *d,
-                              int y, int width)
-{
-    int x, len;
-    uint32_t mask, bits;
-
-    y = y - s1->cursy;
-    mask = s1->cursmask[y];
-    bits = s1->cursbits[y];
-    len = MIN(width - s1->cursx, 32);
-    d = &d[s1->cursx];
-    for (x = 0; x < len; x++) {
-        if (mask & 0x80000000) {
-            if (bits & 0x80000000) {
-                *d = s1->palette[259];
-            } else {
-                *d = s1->palette[258];
-            }
-        }
-        d++;
-        mask <<= 1;
-        bits <<= 1;
-    }
-}
-
 /*
   XXX Could be much more optimal:
   * detect if line/page/whole screen is in 24 bit mode
@@ -326,10 +237,8 @@ static void tcx_update_display(void *opaque)
     ram_addr_t page, page_min, page_max;
     int y, y_start, dd, ds;
     uint8_t *d, *s;
-    void (*f)(TCXState *s1, uint8_t *dst, const uint8_t *src, int width);
-    void (*fc)(TCXState *s1, uint8_t *dst, int y, int width);
 
-    if (surface_bits_per_pixel(surface) == 0) {
+    if (surface_bits_per_pixel(surface) != 32) {
         return;
     }
 
@@ -342,25 +251,6 @@ static void tcx_update_display(void *opaque)
     dd = surface_stride(surface);
     ds = 1024;
 
-    switch (surface_bits_per_pixel(surface)) {
-    case 32:
-        f = tcx_draw_line32;
-        fc = tcx_draw_cursor32;
-        break;
-    case 15:
-    case 16:
-        f = tcx_draw_line16;
-        fc = tcx_draw_cursor16;
-        break;
-    default:
-    case 8:
-        f = tcx_draw_line8;
-        fc = tcx_draw_cursor8;
-        break;
-    case 0:
-        return;
-    }
-
     memory_region_sync_dirty_bitmap(&ts->vram_mem);
     for (y = 0; y < ts->height; y++, page += ds) {
         if (tcx_check_dirty(ts, page, ds)) {
@@ -371,9 +261,9 @@ static void tcx_update_display(void *opaque)
             if (page > page_max)
                 page_max = page;
 
-            f(ts, d, s, ts->width);
+            tcx_draw_line32(ts, d, s, ts->width);
             if (y >= ts->cursy && y < ts->cursy + 32 && ts->cursx < ts->width) {
-                fc(ts, d, y, ts->width);
+                tcx_draw_cursor32(ts, d, y, ts->width);
             }
         } else {
             if (y_start >= 0) {
-- 
1.7.10.4

[Qemu-devel] [PATCH 13/14] tcx: use tcx_set_dirty() for accelerated ops

[Mark Cave-Ayland]

Rather than calling memory_region_set_dirty() directly, make sure that we call
tcx_set_dirty() instead. This ensures that the 24-bit plane and cplane are
also invalidated correctly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 6da6dfb..3f00735 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -525,7 +525,7 @@ static void tcx_stip_writel(void *opaque, hwaddr addr,
                 val <<= 1;
             }
         }
-        memory_region_set_dirty(&s->vram_mem, addr, 32);
+        tcx_set_dirty(s, addr, 32);
     }
 }
 
@@ -558,7 +558,7 @@ static void tcx_rstip_writel(void *opaque, hwaddr addr,
                 val <<= 1;
             }
         }
-        memory_region_set_dirty(&s->vram_mem, addr, 32);
+        tcx_set_dirty(s, addr, 32);
     }
 }
 
@@ -616,7 +616,7 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
                 memcpy(&s->vram24[addr], &s->vram24[adsr], len * 4);
             }
         }
-        memory_region_set_dirty(&s->vram_mem, addr, len);
+        tcx_set_dirty(s, addr, len);
     }
 }
 
@@ -650,7 +650,7 @@ static void tcx_rblit_writel(void *opaque, hwaddr addr,
                 memcpy(&s->cplane[addr], &s->cplane[adsr], len * 4);
             }
         }
-        memory_region_set_dirty(&s->vram_mem, addr, len);
+        tcx_set_dirty(s, addr, len);
     }
 }
 
@@ -687,7 +687,7 @@ static void tcx_invalidate_cursor_position(TCXState *s)
     start = ymin * 1024;
     end   = ymax * 1024;
 
-    memory_region_set_dirty(&s->vram_mem, start, end-start);
+    tcx_set_dirty(s, start, end - start);
 }
 
 static uint64_t tcx_thc_readl(void *opaque, hwaddr addr,
-- 
1.7.10.4

[Qemu-devel] [PATCH 14/14] tcx: switch to load_image_mr() and remove prom_addr hack

[Mark Cave-Ayland]

Previous to the existence of load_image_mr(), the only way to load in the
FCode ROM image was to pass in its physical address via qdev properties
and use load_image_targphys().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c |    4 +---
 hw/sparc/sun4m.c |    1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 3f00735..5a1115c 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -843,8 +843,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     vmstate_register_ram_global(&s->rom);
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, TCX_ROM_FILE);
     if (fcode_filename) {
-        ret = load_image_targphys(fcode_filename, s->prom_addr,
-                                  FCODE_MAX_ROM_SIZE);
+        ret = load_image_mr(fcode_filename, &s->rom);
         g_free(fcode_filename);
         if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
             error_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
@@ -902,7 +901,6 @@ static Property tcx_properties[] = {
     DEFINE_PROP_UINT16("width",    TCXState, width,     -1),
     DEFINE_PROP_UINT16("height",   TCXState, height,    -1),
     DEFINE_PROP_UINT16("depth",    TCXState, depth,     -1),
-    DEFINE_PROP_UINT64("prom_addr", TCXState, prom_addr, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 7a0922b..5f022cc 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -491,7 +491,6 @@ static void tcx_init(hwaddr addr, qemu_irq irq, int vram_size, int width,
     qdev_prop_set_uint16(dev, "width", width);
     qdev_prop_set_uint16(dev, "height", height);
     qdev_prop_set_uint16(dev, "depth", depth);
-    qdev_prop_set_uint64(dev, "prom_addr", addr);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 14/14] tcx: switch to load_image_mr() and remove prom_addr hack

[Philippe Mathieu-Daudé]

On 04/05/2017 05:35 AM, Mark Cave-Ayland wrote:
> Previous to the existence of load_image_mr(), the only way to load in the
> FCode ROM image was to pass in its physical address via qdev properties
> and use load_image_targphys().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/display/tcx.c |    4 +---
>  hw/sparc/sun4m.c |    1 -
>  2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 3f00735..5a1115c 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -843,8 +843,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
>      vmstate_register_ram_global(&s->rom);
>      fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, TCX_ROM_FILE);
>      if (fcode_filename) {
> -        ret = load_image_targphys(fcode_filename, s->prom_addr,
> -                                  FCODE_MAX_ROM_SIZE);
> +        ret = load_image_mr(fcode_filename, &s->rom);
>          g_free(fcode_filename);
>          if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
>              error_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
> @@ -902,7 +901,6 @@ static Property tcx_properties[] = {
>      DEFINE_PROP_UINT16("width",    TCXState, width,     -1),
>      DEFINE_PROP_UINT16("height",   TCXState, height,    -1),
>      DEFINE_PROP_UINT16("depth",    TCXState, depth,     -1),
> -    DEFINE_PROP_UINT64("prom_addr", TCXState, prom_addr, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 7a0922b..5f022cc 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -491,7 +491,6 @@ static void tcx_init(hwaddr addr, qemu_irq irq, int vram_size, int width,
>      qdev_prop_set_uint16(dev, "width", width);
>      qdev_prop_set_uint16(dev, "height", height);
>      qdev_prop_set_uint16(dev, "depth", depth);
> -    qdev_prop_set_uint64(dev, "prom_addr", addr);
>      qdev_init_nofail(dev);
>      s = SYS_BUS_DEVICE(dev);
>
>

Re: [Qemu-devel] [PATCH 00/14] TCX/CG3 adapter cleanups

[Gerd Hoffmann]

On Mi, 2017-04-05 at 09:35 +0100, Mark Cave-Ayland wrote:
> This started as part of the work on Gerd's thread-safe display patches
> but ended up turning into something a lot more comprehensive. It contains
> lots of cleanupm and fixes for display invalidation (particularly on 24-bit
> TCX) exposed by testing the thread-safe patchset.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Looks all fine to me.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

You'll merge them through the sparc queue I guess?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 00/14] TCX/CG3 adapter cleanups

[Mark Cave-Ayland]

On 07/04/17 11:28, Gerd Hoffmann wrote:

> On Mi, 2017-04-05 at 09:35 +0100, Mark Cave-Ayland wrote:
>> This started as part of the work on Gerd's thread-safe display patches
>> but ended up turning into something a lot more comprehensive. It contains
>> lots of cleanupm and fixes for display invalidation (particularly on 24-bit
>> TCX) exposed by testing the thread-safe patchset.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Looks all fine to me.
> 
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> You'll merge them through the sparc queue I guess?

Yes, I can do. I think it makes sense to merge them before your display
thread-safety series, so if that still works for you I'll send a pull
request early on when the tree opens for 2.10 to clear the way for your
updates.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 00/14] TCX/CG3 adapter cleanups

[Gerd Hoffmann]

  Hi,

> Yes, I can do. I think it makes sense to merge them before your display
> thread-safety series, so if that still works for you I'll send a pull
> request early on when the tree opens for 2.10 to clear the way for your
> updates.

Sounds good.  Maybe I split the series anyway, first merge the snapshot
infrastructure patches, then all the display updates, finally revert the
temporary workaround, to reduce the ordering headaches.

cheers,
  Gerd