[PATCH 0/3] hw/display/sm501: Tidy up template header

[PATCH 0/3] hw/display/sm501: Tidy up template header

[Peter Maydell]

For a long time now the UI layer has guaranteed that the console
surface is always 32 bits per pixel RGB, but some older display device
models still have the code to handle other formats. This patchset
cleans up that dead code for the sm501 device, which allows us
to remove the multiply-included sm501_template.h header entirely.

There have been a few attempts at doing this cleanup on this
device in the past by various people; rather than trying to
resurrect those old patchsets and get them to apply to the current
code in master, I just started from scratch.

Tested with AROS ISO image on sam460ex.

thanks
-- PMM

Peter Maydell (3):
  hw/display/sm501: Remove dead code for non-32-bit RGB surfaces
  hw/display/sm501: Expand out macros in template header
  hw/display/sm501: Inline template header into C file

 hw/display/sm501_template.h | 131 ----------------------------
 hw/display/sm501.c          | 166 ++++++++++++++++++------------------
 2 files changed, 83 insertions(+), 214 deletions(-)
 delete mode 100644 hw/display/sm501_template.h

-- 
2.20.1

[PATCH 1/3] hw/display/sm501: Remove dead code for non-32-bit RGB surfaces

[Peter Maydell]

For a long time now the UI layer has guaranteed that the console
surface is always 32 bits per pixel RGB. Remove the legacy dead
code from the sm501 display device which was handling the
possibility that the console surface was some other format.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/sm501.c | 91 +++-------------------------------------------
 1 file changed, 6 insertions(+), 85 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 8966b69bc73..aba447c18b3 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1558,89 +1558,9 @@ typedef void draw_hwc_line_func(uint8_t *d, const uint8_t *s,
                                 int width, const uint8_t *palette,
                                 int c_x, int c_y);
 
-#define DEPTH 8
-#include "sm501_template.h"
-
-#define DEPTH 15
-#include "sm501_template.h"
-
-#define BGR_FORMAT
-#define DEPTH 15
-#include "sm501_template.h"
-
-#define DEPTH 16
-#include "sm501_template.h"
-
-#define BGR_FORMAT
-#define DEPTH 16
-#include "sm501_template.h"
-
 #define DEPTH 32
 #include "sm501_template.h"
 
-#define BGR_FORMAT
-#define DEPTH 32
-#include "sm501_template.h"
-
-static draw_line_func *draw_line8_funcs[] = {
-    draw_line8_8,
-    draw_line8_15,
-    draw_line8_16,
-    draw_line8_32,
-    draw_line8_32bgr,
-    draw_line8_15bgr,
-    draw_line8_16bgr,
-};
-
-static draw_line_func *draw_line16_funcs[] = {
-    draw_line16_8,
-    draw_line16_15,
-    draw_line16_16,
-    draw_line16_32,
-    draw_line16_32bgr,
-    draw_line16_15bgr,
-    draw_line16_16bgr,
-};
-
-static draw_line_func *draw_line32_funcs[] = {
-    draw_line32_8,
-    draw_line32_15,
-    draw_line32_16,
-    draw_line32_32,
-    draw_line32_32bgr,
-    draw_line32_15bgr,
-    draw_line32_16bgr,
-};
-
-static draw_hwc_line_func *draw_hwc_line_funcs[] = {
-    draw_hwc_line_8,
-    draw_hwc_line_15,
-    draw_hwc_line_16,
-    draw_hwc_line_32,
-    draw_hwc_line_32bgr,
-    draw_hwc_line_15bgr,
-    draw_hwc_line_16bgr,
-};
-
-static inline int get_depth_index(DisplaySurface *surface)
-{
-    switch (surface_bits_per_pixel(surface)) {
-    default:
-    case 8:
-        return 0;
-    case 15:
-        return 1;
-    case 16:
-        return 2;
-    case 32:
-        if (is_surface_bgr(surface)) {
-            return 4;
-        } else {
-            return 3;
-        }
-    }
-}
-
 static void sm501_update_display(void *opaque)
 {
     SM501State *s = (SM501State *)opaque;
@@ -1652,7 +1572,6 @@ static void sm501_update_display(void *opaque)
     int height = get_height(s, crt);
     int src_bpp = get_bpp(s, crt);
     int dst_bpp = surface_bytes_per_pixel(surface);
-    int dst_depth_index = get_depth_index(surface);
     draw_line_func *draw_line = NULL;
     draw_hwc_line_func *draw_hwc_line = NULL;
     int full_update = 0;
@@ -1662,6 +1581,8 @@ static void sm501_update_display(void *opaque)
     uint8_t hwc_palette[3 * 3];
     uint8_t *hwc_src = NULL;
 
+    assert(dst_bpp == 4); /* Output is always 32-bit RGB */
+
     if (!((crt ? s->dc_crt_control : s->dc_panel_control)
           & SM501_DC_CRT_CONTROL_ENABLE)) {
         return;
@@ -1674,13 +1595,13 @@ static void sm501_update_display(void *opaque)
     /* choose draw_line function */
     switch (src_bpp) {
     case 1:
-        draw_line = draw_line8_funcs[dst_depth_index];
+        draw_line = draw_line8_32;
         break;
     case 2:
-        draw_line = draw_line16_funcs[dst_depth_index];
+        draw_line = draw_line16_32;
         break;
     case 4:
-        draw_line = draw_line32_funcs[dst_depth_index];
+        draw_line = draw_line32_32;
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "sm501: update display"
@@ -1691,7 +1612,7 @@ static void sm501_update_display(void *opaque)
     /* set up to draw hardware cursor */
     if (is_hwc_enabled(s, crt)) {
         /* choose cursor draw line function */
-        draw_hwc_line = draw_hwc_line_funcs[dst_depth_index];
+        draw_hwc_line = draw_hwc_line_32;
         hwc_src = get_hwc_address(s, crt);
         c_x = get_hwc_x(s, crt);
         c_y = get_hwc_y(s, crt);
-- 
2.20.1

[PATCH 2/3] hw/display/sm501: Expand out macros in template header

[Peter Maydell]

Now that we only include sm501_template.h for the DEPTH==32 case, we
can expand out the uses of the BPP, PIXEL_TYPE and PIXEL_NAME macros
in that header.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/sm501_template.h | 60 +++++++++++--------------------------
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index a60abad019c..28537a05d95 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -22,28 +22,8 @@
  * THE SOFTWARE.
  */
 
-#if DEPTH == 8
-#define BPP 1
-#define PIXEL_TYPE uint8_t
-#elif DEPTH == 15 || DEPTH == 16
-#define BPP 2
-#define PIXEL_TYPE uint16_t
-#elif DEPTH == 32
-#define BPP 4
-#define PIXEL_TYPE uint32_t
-#else
-#error unsupport depth
-#endif
-
-#ifdef BGR_FORMAT
-#define PIXEL_NAME glue(DEPTH, bgr)
-#else
-#define PIXEL_NAME DEPTH
-#endif /* BGR_FORMAT */
-
-
-static void glue(draw_line8_, PIXEL_NAME)(
-                 uint8_t *d, const uint8_t *s, int width, const uint32_t *pal)
+static void draw_line8_32(uint8_t *d, const uint8_t *s, int width,
+                          const uint32_t *pal)
 {
     uint8_t v, r, g, b;
     do {
@@ -51,14 +31,14 @@ static void glue(draw_line8_, PIXEL_NAME)(
         r = (pal[v] >> 16) & 0xff;
         g = (pal[v] >>  8) & 0xff;
         b = (pal[v] >>  0) & 0xff;
-        *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
+        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
         s++;
-        d += BPP;
+        d += 4;
     } while (--width != 0);
 }
 
-static void glue(draw_line16_, PIXEL_NAME)(
-                 uint8_t *d, const uint8_t *s, int width, const uint32_t *pal)
+static void draw_line16_32(uint8_t *d, const uint8_t *s, int width,
+                           const uint32_t *pal)
 {
     uint16_t rgb565;
     uint8_t r, g, b;
@@ -68,14 +48,14 @@ static void glue(draw_line16_, PIXEL_NAME)(
         r = (rgb565 >> 8) & 0xf8;
         g = (rgb565 >> 3) & 0xfc;
         b = (rgb565 << 3) & 0xf8;
-        *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
+        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
         s += 2;
-        d += BPP;
+        d += 4;
     } while (--width != 0);
 }
 
-static void glue(draw_line32_, PIXEL_NAME)(
-                 uint8_t *d, const uint8_t *s, int width, const uint32_t *pal)
+static void draw_line32_32(uint8_t *d, const uint8_t *s, int width,
+                           const uint32_t *pal)
 {
     uint8_t r, g, b;
 
@@ -83,17 +63,17 @@ static void glue(draw_line32_, PIXEL_NAME)(
         r = s[2];
         g = s[1];
         b = s[0];
-        *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
+        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
         s += 4;
-        d += BPP;
+        d += 4;
     } while (--width != 0);
 }
 
 /**
  * Draw hardware cursor image on the given line.
  */
-static void glue(draw_hwc_line_, PIXEL_NAME)(uint8_t *d, const uint8_t *s,
-                 int width, const uint8_t *palette, int c_x, int c_y)
+static void draw_hwc_line_32(uint8_t *d, const uint8_t *s, int width,
+                             const uint8_t *palette, int c_x, int c_y)
 {
     int i;
     uint8_t r, g, b, v, bitset = 0;
@@ -101,7 +81,7 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(uint8_t *d, const uint8_t *s,
     /* get cursor position */
     assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
     s += SM501_HWC_WIDTH * c_y / 4;  /* 4 pixels per byte */
-    d += c_x * BPP;
+    d += c_x * 4;
 
     for (i = 0; i < SM501_HWC_WIDTH && c_x + i < width; i++) {
         /* get pixel value */
@@ -118,14 +98,8 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(uint8_t *d, const uint8_t *s,
             r = palette[v * 3 + 0];
             g = palette[v * 3 + 1];
             b = palette[v * 3 + 2];
-            *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
+            *(uint32_t *)d = rgb_to_pixel32(r, g, b);
         }
-        d += BPP;
+        d += 4;
     }
 }
-
-#undef DEPTH
-#undef BPP
-#undef PIXEL_TYPE
-#undef PIXEL_NAME
-#undef BGR_FORMAT
-- 
2.20.1

[PATCH 3/3] hw/display/sm501: Inline template header into C file

[Peter Maydell]

We no longer need to include sm501_template.h multiple times, so
we can simply inline its contents into sm501.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/sm501_template.h | 105 ------------------------------------
 hw/display/sm501.c          |  83 +++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 107 deletions(-)
 delete mode 100644 hw/display/sm501_template.h

diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
deleted file mode 100644
index 28537a05d95..00000000000
--- a/hw/display/sm501_template.h
+++ /dev/null
@@ -1,105 +0,0 @@
-/*
- * Pixel drawing function templates for QEMU SM501 Device
- *
- * Copyright (c) 2008 Shin-ichiro KAWASAKI
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-static void draw_line8_32(uint8_t *d, const uint8_t *s, int width,
-                          const uint32_t *pal)
-{
-    uint8_t v, r, g, b;
-    do {
-        v = ldub_p(s);
-        r = (pal[v] >> 16) & 0xff;
-        g = (pal[v] >>  8) & 0xff;
-        b = (pal[v] >>  0) & 0xff;
-        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
-        s++;
-        d += 4;
-    } while (--width != 0);
-}
-
-static void draw_line16_32(uint8_t *d, const uint8_t *s, int width,
-                           const uint32_t *pal)
-{
-    uint16_t rgb565;
-    uint8_t r, g, b;
-
-    do {
-        rgb565 = lduw_le_p(s);
-        r = (rgb565 >> 8) & 0xf8;
-        g = (rgb565 >> 3) & 0xfc;
-        b = (rgb565 << 3) & 0xf8;
-        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
-        s += 2;
-        d += 4;
-    } while (--width != 0);
-}
-
-static void draw_line32_32(uint8_t *d, const uint8_t *s, int width,
-                           const uint32_t *pal)
-{
-    uint8_t r, g, b;
-
-    do {
-        r = s[2];
-        g = s[1];
-        b = s[0];
-        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
-        s += 4;
-        d += 4;
-    } while (--width != 0);
-}
-
-/**
- * Draw hardware cursor image on the given line.
- */
-static void draw_hwc_line_32(uint8_t *d, const uint8_t *s, int width,
-                             const uint8_t *palette, int c_x, int c_y)
-{
-    int i;
-    uint8_t r, g, b, v, bitset = 0;
-
-    /* get cursor position */
-    assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
-    s += SM501_HWC_WIDTH * c_y / 4;  /* 4 pixels per byte */
-    d += c_x * 4;
-
-    for (i = 0; i < SM501_HWC_WIDTH && c_x + i < width; i++) {
-        /* get pixel value */
-        if (i % 4 == 0) {
-            bitset = ldub_p(s);
-            s++;
-        }
-        v = bitset & 3;
-        bitset >>= 2;
-
-        /* write pixel */
-        if (v) {
-            v--;
-            r = palette[v * 3 + 0];
-            g = palette[v * 3 + 1];
-            b = palette[v * 3 + 2];
-            *(uint32_t *)d = rgb_to_pixel32(r, g, b);
-        }
-        d += 4;
-    }
-}
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index aba447c18b3..8789722ef27 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1558,8 +1558,87 @@ typedef void draw_hwc_line_func(uint8_t *d, const uint8_t *s,
                                 int width, const uint8_t *palette,
                                 int c_x, int c_y);
 
-#define DEPTH 32
-#include "sm501_template.h"
+static void draw_line8_32(uint8_t *d, const uint8_t *s, int width,
+                          const uint32_t *pal)
+{
+    uint8_t v, r, g, b;
+    do {
+        v = ldub_p(s);
+        r = (pal[v] >> 16) & 0xff;
+        g = (pal[v] >>  8) & 0xff;
+        b = (pal[v] >>  0) & 0xff;
+        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
+        s++;
+        d += 4;
+    } while (--width != 0);
+}
+
+static void draw_line16_32(uint8_t *d, const uint8_t *s, int width,
+                           const uint32_t *pal)
+{
+    uint16_t rgb565;
+    uint8_t r, g, b;
+
+    do {
+        rgb565 = lduw_le_p(s);
+        r = (rgb565 >> 8) & 0xf8;
+        g = (rgb565 >> 3) & 0xfc;
+        b = (rgb565 << 3) & 0xf8;
+        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
+        s += 2;
+        d += 4;
+    } while (--width != 0);
+}
+
+static void draw_line32_32(uint8_t *d, const uint8_t *s, int width,
+                           const uint32_t *pal)
+{
+    uint8_t r, g, b;
+
+    do {
+        r = s[2];
+        g = s[1];
+        b = s[0];
+        *(uint32_t *)d = rgb_to_pixel32(r, g, b);
+        s += 4;
+        d += 4;
+    } while (--width != 0);
+}
+
+/**
+ * Draw hardware cursor image on the given line.
+ */
+static void draw_hwc_line_32(uint8_t *d, const uint8_t *s, int width,
+                             const uint8_t *palette, int c_x, int c_y)
+{
+    int i;
+    uint8_t r, g, b, v, bitset = 0;
+
+    /* get cursor position */
+    assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
+    s += SM501_HWC_WIDTH * c_y / 4;  /* 4 pixels per byte */
+    d += c_x * 4;
+
+    for (i = 0; i < SM501_HWC_WIDTH && c_x + i < width; i++) {
+        /* get pixel value */
+        if (i % 4 == 0) {
+            bitset = ldub_p(s);
+            s++;
+        }
+        v = bitset & 3;
+        bitset >>= 2;
+
+        /* write pixel */
+        if (v) {
+            v--;
+            r = palette[v * 3 + 0];
+            g = palette[v * 3 + 1];
+            b = palette[v * 3 + 2];
+            *(uint32_t *)d = rgb_to_pixel32(r, g, b);
+        }
+        d += 4;
+    }
+}
 
 static void sm501_update_display(void *opaque)
 {
-- 
2.20.1

Re: [PATCH 0/3] hw/display/sm501: Tidy up template header

[BALATON Zoltan]

On Fri, 12 Feb 2021, Peter Maydell wrote:
> For a long time now the UI layer has guaranteed that the console
> surface is always 32 bits per pixel RGB, but some older display device
> models still have the code to handle other formats. This patchset
> cleans up that dead code for the sm501 device, which allows us
> to remove the multiply-included sm501_template.h header entirely.
>
> There have been a few attempts at doing this cleanup on this
> device in the past by various people; rather than trying to
> resurrect those old patchsets and get them to apply to the current
> code in master, I just started from scratch.
>
> Tested with AROS ISO image on sam460ex.
>
> thanks
> -- PMM
>
> Peter Maydell (3):
>  hw/display/sm501: Remove dead code for non-32-bit RGB surfaces
>  hw/display/sm501: Expand out macros in template header
>  hw/display/sm501: Inline template header into C file

I've tried with AmigaOS and MorphOS and those also work. Unfortunately the 
drivers for sm501 on those are restricted to 16bit modes (maybe because 
real hardware is too slow otherwise or does not have enough memory) so 
every screen update in QEMU needs conversion which makes it quite slow. 
But this was like that before and unless we want to allow other than 32bit 
surfaces again we can't use the code removed here but that was the reason 
I've kept it and not removed so far in case we want to do this 
optimisation again.

Otherwise,

Acked-by: BALATON Zoltan <balaton@eik.bme.hu>

The sm501 is also used on the SH4 r2d machine I think. Aurelien probably 
knows more about that. I've found some images for it here:

https://people.debian.org/~aurel32/qemu/sh4/
https://lists.nongnu.org/archive/html/qemu-devel/2008-08/msg01308.html

in case you want to test that too.

Regards,
BALATON Zoltan

> hw/display/sm501_template.h | 131 ----------------------------
> hw/display/sm501.c          | 166 ++++++++++++++++++------------------
> 2 files changed, 83 insertions(+), 214 deletions(-)
> delete mode 100644 hw/display/sm501_template.h
>
>

Re: [PATCH 0/3] hw/display/sm501: Tidy up template header

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

On Fri, Feb 12, 2021 at 09:28:35PM +0100, BALATON Zoltan wrote:
> On Fri, 12 Feb 2021, Peter Maydell wrote:
> > For a long time now the UI layer has guaranteed that the console
> > surface is always 32 bits per pixel RGB, but some older display device
> > models still have the code to handle other formats. This patchset
> > cleans up that dead code for the sm501 device, which allows us
> > to remove the multiply-included sm501_template.h header entirely.
> > 
> > There have been a few attempts at doing this cleanup on this
> > device in the past by various people; rather than trying to
> > resurrect those old patchsets and get them to apply to the current
> > code in master, I just started from scratch.
> > 
> > Tested with AROS ISO image on sam460ex.
> > 
> > thanks
> > -- PMM
> > 
> > Peter Maydell (3):
> >  hw/display/sm501: Remove dead code for non-32-bit RGB surfaces
> >  hw/display/sm501: Expand out macros in template header
> >  hw/display/sm501: Inline template header into C file
> 
> I've tried with AmigaOS and MorphOS and those also work. Unfortunately the
> drivers for sm501 on those are restricted to 16bit modes (maybe because real
> hardware is too slow otherwise or does not have enough memory) so every
> screen update in QEMU needs conversion which makes it quite slow. But this
> was like that before and unless we want to allow other than 32bit surfaces
> again we can't use the code removed here but that was the reason I've kept
> it and not removed so far in case we want to do this optimisation again.
> 
> Otherwise,
> 
> Acked-by: BALATON Zoltan <balaton@eik.bme.hu>
> 
> The sm501 is also used on the SH4 r2d machine I think. Aurelien probably
> knows more about that. I've found some images for it here:
> 
> https://people.debian.org/~aurel32/qemu/sh4/
> https://lists.nongnu.org/archive/html/qemu-devel/2008-08/msg01308.html
> 
> in case you want to test that too.

I've queued these to ppc-for-6.0.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]