[Qemu-devel] [PATCH 0/2] hw/display/blizzard: remove blizzard_template.h

[Qemu-devel] [PATCH 0/2] hw/display/blizzard: remove blizzard_template.h

[Peter Maydell]

Now that we can rely on the display surface being 32 bits,
the whole machinery of blizzard_template.h being included once
per supported depth is unnecessary.

This series expands out the DEPTH-dependent macros, and then
moves the remaining code into blizzard.c and removes the header file.

It applies on top of Pooja Dhannawat's patch
"blizzard: Remove support for DEPTH != 32" (which you can
also find in my target-arm.next branch:
https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next)

thanks
-- PMM

Peter Maydell (2):
  hw/display/blizzard: Expand out macros
  hw/display/blizzard: Remove blizzard_template.h

 hw/display/blizzard.c          |  79 ++++++++++++++++++++++++++-
 hw/display/blizzard_template.h | 118 -----------------------------------------
 2 files changed, 77 insertions(+), 120 deletions(-)
 delete mode 100644 hw/display/blizzard_template.h

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] hw/display/blizzard: Expand out macros

[Peter Maydell]

Now that we can assume that only depth 32 is possible, there's no need
for the COPY_PIXEL1 and PIXEL_TYPE macros, and the SKIP_PIXEL, COPY_PIXEL
and SWAP_WORDS macros aren't used at all. Expand out COPY_PIXEL1 and
PIXEL_TYPE where they are used, delete the unused macro definitions, and
expand out the uses of glue(name_prefix, DEPTH).

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

diff --git a/hw/display/blizzard_template.h b/hw/display/blizzard_template.h
index bc38d7a..ed96092 100644
--- a/hw/display/blizzard_template.h
+++ b/hw/display/blizzard_template.h
@@ -18,21 +18,8 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#define SKIP_PIXEL(to)         (to += deststep)
-#if DEPTH == 32
-# define PIXEL_TYPE            uint32_t
-# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
-# define COPY_PIXEL1(to, from) (*to++ = from)
-#else
-# error unknown bit depth
-#endif
-
-#ifdef HOST_WORDS_BIGENDIAN
-# define SWAP_WORDS	1
-#endif
-
-static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
-                const uint16_t *src, unsigned int width)
+static void blizzard_draw_line16_32(uint32_t *dest,
+                                    const uint16_t *src, unsigned int width)
 {
     uint16_t data;
     unsigned int r, g, b;
@@ -45,12 +32,12 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
         data >>= 6;
         r = (data & 0x1f) << 3;
         data >>= 5;
-        COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b));
+        *dest++ = rgb_to_pixel32(r, g, b);
     }
 }
 
-static void glue(blizzard_draw_line24mode1_, DEPTH)(PIXEL_TYPE *dest,
-                const uint8_t *src, unsigned int width)
+static void blizzard_draw_line24mode1_32(uint32_t *dest,
+                                         const uint8_t *src, unsigned int width)
 {
     /* TODO: check if SDL 24-bit planes are not in the same format and
      * if so, use memcpy */
@@ -61,15 +48,15 @@ static void glue(blizzard_draw_line24mode1_, DEPTH)(PIXEL_TYPE *dest,
         r[0] = *src ++;
         r[1] = *src ++;
         b[0] = *src ++;
-        COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r[0], g[0], b[0]));
+        *dest++ = rgb_to_pixel32(r[0], g[0], b[0]);
         b[1] = *src ++;
         g[1] = *src ++;
-        COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r[1], g[1], b[1]));
+        *dest++ = rgb_to_pixel32(r[1], g[1], b[1]);
     }
 }
 
-static void glue(blizzard_draw_line24mode2_, DEPTH)(PIXEL_TYPE *dest,
-                const uint8_t *src, unsigned int width)
+static void blizzard_draw_line24mode2_32(uint32_t *dest,
+                                         const uint8_t *src, unsigned int width)
 {
     unsigned int r, g, b;
     const uint8_t *end = src + width;
@@ -78,24 +65,24 @@ static void glue(blizzard_draw_line24mode2_, DEPTH)(PIXEL_TYPE *dest,
         src ++;
         b = *src ++;
         g = *src ++;
-        COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b));
+        *dest++ = rgb_to_pixel32(r, g, b);
     }
 }
 
 /* No rotation */
-static blizzard_fn_t glue(blizzard_draw_fn_, DEPTH)[0x10] = {
+static blizzard_fn_t blizzard_draw_fn_32[0x10] = {
     NULL,
     /* RGB 5:6:5*/
-    (blizzard_fn_t) glue(blizzard_draw_line16_, DEPTH),
+    (blizzard_fn_t) blizzard_draw_line16_32,
     /* RGB 6:6:6 mode 1 */
-    (blizzard_fn_t) glue(blizzard_draw_line24mode1_, DEPTH),
+    (blizzard_fn_t) blizzard_draw_line24mode1_32,
     /* RGB 8:8:8 mode 1 */
-    (blizzard_fn_t) glue(blizzard_draw_line24mode1_, DEPTH),
+    (blizzard_fn_t) blizzard_draw_line24mode1_32,
     NULL, NULL,
     /* RGB 6:6:6 mode 2 */
-    (blizzard_fn_t) glue(blizzard_draw_line24mode2_, DEPTH),
+    (blizzard_fn_t) blizzard_draw_line24mode2_32,
     /* RGB 8:8:8 mode 2 */
-    (blizzard_fn_t) glue(blizzard_draw_line24mode2_, DEPTH),
+    (blizzard_fn_t) blizzard_draw_line24mode2_32,
     /* YUV 4:2:2 */
     NULL,
     /* YUV 4:2:0 */
@@ -104,15 +91,7 @@ static blizzard_fn_t glue(blizzard_draw_fn_, DEPTH)[0x10] = {
 };
 
 /* 90deg, 180deg and 270deg rotation */
-static blizzard_fn_t glue(blizzard_draw_fn_r_, DEPTH)[0x10] = {
+static blizzard_fn_t blizzard_draw_fn_r_32[0x10] = {
     /* TODO */
     [0 ... 0xf] = NULL,
 };
-
-#undef DEPTH
-#undef SKIP_PIXEL
-#undef COPY_PIXEL
-#undef COPY_PIXEL1
-#undef PIXEL_TYPE
-
-#undef SWAP_WORDS
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] hw/display/blizzard: Expand out macros

[Eric Blake]

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

On 05/04/2016 08:15 AM, Peter Maydell wrote:
> Now that we can assume that only depth 32 is possible, there's no need
> for the COPY_PIXEL1 and PIXEL_TYPE macros, and the SKIP_PIXEL, COPY_PIXEL
> and SWAP_WORDS macros aren't used at all. Expand out COPY_PIXEL1 and
> PIXEL_TYPE where they are used, delete the unused macro definitions, and
> expand out the uses of glue(name_prefix, DEPTH).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/blizzard_template.h | 55 +++++++++++++-----------------------------
>  1 file changed, 17 insertions(+), 38 deletions(-)

I might have broken into more patches, one per macro expansion; but the
final patch was still small enough to review when merged, so keep it the
way you have it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/2] hw/display/blizzard: Remove blizzard_template.h

[Peter Maydell]

We no longer need to do the "multiply include this header" trick with
blizzard_template.h, and it is only used in a single .c file, so just
put its contents inline in blizzard.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/blizzard.c          | 79 +++++++++++++++++++++++++++++++++-
 hw/display/blizzard_template.h | 97 ------------------------------------------
 2 files changed, 77 insertions(+), 99 deletions(-)
 delete mode 100644 hw/display/blizzard_template.h

diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c
index af0b51e..cbf07d1 100644
--- a/hw/display/blizzard.c
+++ b/hw/display/blizzard.c
@@ -925,8 +925,83 @@ static void blizzard_update_display(void *opaque)
     s->my[1] = 0;
 }
 
-#define DEPTH 32
-#include "blizzard_template.h"
+static void blizzard_draw_line16_32(uint32_t *dest,
+                                    const uint16_t *src, unsigned int width)
+{
+    uint16_t data;
+    unsigned int r, g, b;
+    const uint16_t *end = (const void *) src + width;
+    while (src < end) {
+        data = *src ++;
+        b = (data & 0x1f) << 3;
+        data >>= 5;
+        g = (data & 0x3f) << 2;
+        data >>= 6;
+        r = (data & 0x1f) << 3;
+        data >>= 5;
+        *dest++ = rgb_to_pixel32(r, g, b);
+    }
+}
+
+static void blizzard_draw_line24mode1_32(uint32_t *dest,
+                                         const uint8_t *src, unsigned int width)
+{
+    /* TODO: check if SDL 24-bit planes are not in the same format and
+     * if so, use memcpy */
+    unsigned int r[2], g[2], b[2];
+    const uint8_t *end = src + width;
+    while (src < end) {
+        g[0] = *src ++;
+        r[0] = *src ++;
+        r[1] = *src ++;
+        b[0] = *src ++;
+        *dest++ = rgb_to_pixel32(r[0], g[0], b[0]);
+        b[1] = *src ++;
+        g[1] = *src ++;
+        *dest++ = rgb_to_pixel32(r[1], g[1], b[1]);
+    }
+}
+
+static void blizzard_draw_line24mode2_32(uint32_t *dest,
+                                         const uint8_t *src, unsigned int width)
+{
+    unsigned int r, g, b;
+    const uint8_t *end = src + width;
+    while (src < end) {
+        r = *src ++;
+        src ++;
+        b = *src ++;
+        g = *src ++;
+        *dest++ = rgb_to_pixel32(r, g, b);
+    }
+}
+
+/* No rotation */
+static blizzard_fn_t blizzard_draw_fn_32[0x10] = {
+    NULL,
+    /* RGB 5:6:5*/
+    (blizzard_fn_t) blizzard_draw_line16_32,
+    /* RGB 6:6:6 mode 1 */
+    (blizzard_fn_t) blizzard_draw_line24mode1_32,
+    /* RGB 8:8:8 mode 1 */
+    (blizzard_fn_t) blizzard_draw_line24mode1_32,
+    NULL, NULL,
+    /* RGB 6:6:6 mode 2 */
+    (blizzard_fn_t) blizzard_draw_line24mode2_32,
+    /* RGB 8:8:8 mode 2 */
+    (blizzard_fn_t) blizzard_draw_line24mode2_32,
+    /* YUV 4:2:2 */
+    NULL,
+    /* YUV 4:2:0 */
+    NULL,
+    NULL, NULL, NULL, NULL, NULL, NULL,
+};
+
+/* 90deg, 180deg and 270deg rotation */
+static blizzard_fn_t blizzard_draw_fn_r_32[0x10] = {
+    /* TODO */
+    [0 ... 0xf] = NULL,
+};
 
 static const GraphicHwOps blizzard_ops = {
     .invalidate  = blizzard_invalidate_display,
diff --git a/hw/display/blizzard_template.h b/hw/display/blizzard_template.h
deleted file mode 100644
index ed96092..0000000
--- a/hw/display/blizzard_template.h
+++ /dev/null
@@ -1,97 +0,0 @@
-/*
- * QEMU Epson S1D13744/S1D13745 templates
- *
- * Copyright (C) 2008 Nokia Corporation
- * Written by Andrzej Zaborowski <andrew@openedhand.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 or
- * (at your option) version 3 of the License.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, see <http://www.gnu.org/licenses/>.
- */
-
-static void blizzard_draw_line16_32(uint32_t *dest,
-                                    const uint16_t *src, unsigned int width)
-{
-    uint16_t data;
-    unsigned int r, g, b;
-    const uint16_t *end = (const void *) src + width;
-    while (src < end) {
-        data = *src ++;
-        b = (data & 0x1f) << 3;
-        data >>= 5;
-        g = (data & 0x3f) << 2;
-        data >>= 6;
-        r = (data & 0x1f) << 3;
-        data >>= 5;
-        *dest++ = rgb_to_pixel32(r, g, b);
-    }
-}
-
-static void blizzard_draw_line24mode1_32(uint32_t *dest,
-                                         const uint8_t *src, unsigned int width)
-{
-    /* TODO: check if SDL 24-bit planes are not in the same format and
-     * if so, use memcpy */
-    unsigned int r[2], g[2], b[2];
-    const uint8_t *end = src + width;
-    while (src < end) {
-        g[0] = *src ++;
-        r[0] = *src ++;
-        r[1] = *src ++;
-        b[0] = *src ++;
-        *dest++ = rgb_to_pixel32(r[0], g[0], b[0]);
-        b[1] = *src ++;
-        g[1] = *src ++;
-        *dest++ = rgb_to_pixel32(r[1], g[1], b[1]);
-    }
-}
-
-static void blizzard_draw_line24mode2_32(uint32_t *dest,
-                                         const uint8_t *src, unsigned int width)
-{
-    unsigned int r, g, b;
-    const uint8_t *end = src + width;
-    while (src < end) {
-        r = *src ++;
-        src ++;
-        b = *src ++;
-        g = *src ++;
-        *dest++ = rgb_to_pixel32(r, g, b);
-    }
-}
-
-/* No rotation */
-static blizzard_fn_t blizzard_draw_fn_32[0x10] = {
-    NULL,
-    /* RGB 5:6:5*/
-    (blizzard_fn_t) blizzard_draw_line16_32,
-    /* RGB 6:6:6 mode 1 */
-    (blizzard_fn_t) blizzard_draw_line24mode1_32,
-    /* RGB 8:8:8 mode 1 */
-    (blizzard_fn_t) blizzard_draw_line24mode1_32,
-    NULL, NULL,
-    /* RGB 6:6:6 mode 2 */
-    (blizzard_fn_t) blizzard_draw_line24mode2_32,
-    /* RGB 8:8:8 mode 2 */
-    (blizzard_fn_t) blizzard_draw_line24mode2_32,
-    /* YUV 4:2:2 */
-    NULL,
-    /* YUV 4:2:0 */
-    NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL,
-};
-
-/* 90deg, 180deg and 270deg rotation */
-static blizzard_fn_t blizzard_draw_fn_r_32[0x10] = {
-    /* TODO */
-    [0 ... 0xf] = NULL,
-};
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] hw/display/blizzard: Remove blizzard_template.h

[Eric Blake]

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

On 05/04/2016 08:15 AM, Peter Maydell wrote:
> We no longer need to do the "multiply include this header" trick with
> blizzard_template.h, and it is only used in a single .c file, so just
> put its contents inline in blizzard.c.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/blizzard.c          | 79 +++++++++++++++++++++++++++++++++-
>  hw/display/blizzard_template.h | 97 ------------------------------------------
>  2 files changed, 77 insertions(+), 99 deletions(-)
>  delete mode 100644 hw/display/blizzard_template.h
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/2] hw/display/blizzard: remove blizzard_template.h

[Gerd Hoffmann]

On Mi, 2016-05-04 at 15:15 +0100, Peter Maydell wrote:
> Now that we can rely on the display surface being 32 bits,
> the whole machinery of blizzard_template.h being included once
> per supported depth is unnecessary.
> 
> This series expands out the DEPTH-dependent macros, and then
> moves the remaining code into blizzard.c and removes the header file.
> 
> It applies on top of Pooja Dhannawat's patch
> "blizzard: Remove support for DEPTH != 32" (which you can
> also find in my target-arm.next branch:
> https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next)

So you take those two through the arm queue too I guess?

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

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/2] hw/display/blizzard: remove blizzard_template.h

[Peter Maydell]

On 9 May 2016 at 08:48, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mi, 2016-05-04 at 15:15 +0100, Peter Maydell wrote:
>> Now that we can rely on the display surface being 32 bits,
>> the whole machinery of blizzard_template.h being included once
>> per supported depth is unnecessary.
>>
>> This series expands out the DEPTH-dependent macros, and then
>> moves the remaining code into blizzard.c and removes the header file.
>>
>> It applies on top of Pooja Dhannawat's patch
>> "blizzard: Remove support for DEPTH != 32" (which you can
>> also find in my target-arm.next branch:
>> https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next)
>
> So you take those two through the arm queue too I guess?

Yes.

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

Thanks for the review.

-- PMM