[Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Pooja Dhannawat]

Removing support for DEPTH != 32 from blizzard template header
and file that includes it, as macro DEPTH == 32 only used.

Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
---
 hw/display/blizzard.c          | 41 ++++-------------------------------------
 hw/display/blizzard_template.h | 30 +-----------------------------
 2 files changed, 5 insertions(+), 66 deletions(-)

diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c
index c231960..5684e49 100644
--- a/hw/display/blizzard.c
+++ b/hw/display/blizzard.c
@@ -925,14 +925,6 @@ static void blizzard_update_display(void *opaque)
     s->my[1] = 0;
 }
 
-#define DEPTH 8
-#include "blizzard_template.h"
-#define DEPTH 15
-#include "blizzard_template.h"
-#define DEPTH 16
-#include "blizzard_template.h"
-#define DEPTH 24
-#include "blizzard_template.h"
 #define DEPTH 32
 #include "blizzard_template.h"
 
@@ -951,35 +943,10 @@ void *s1d13745_init(qemu_irq gpio_int)
     s->con = graphic_console_init(NULL, 0, &blizzard_ops, s);
     surface = qemu_console_surface(s->con);
 
-    switch (surface_bits_per_pixel(surface)) {
-    case 0:
-        s->line_fn_tab[0] = s->line_fn_tab[1] =
-                g_malloc0(sizeof(blizzard_fn_t) * 0x10);
-        break;
-    case 8:
-        s->line_fn_tab[0] = blizzard_draw_fn_8;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_8;
-        break;
-    case 15:
-        s->line_fn_tab[0] = blizzard_draw_fn_15;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_15;
-        break;
-    case 16:
-        s->line_fn_tab[0] = blizzard_draw_fn_16;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_16;
-        break;
-    case 24:
-        s->line_fn_tab[0] = blizzard_draw_fn_24;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_24;
-        break;
-    case 32:
-        s->line_fn_tab[0] = blizzard_draw_fn_32;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_32;
-        break;
-    default:
-        fprintf(stderr, "%s: Bad color depth\n", __FUNCTION__);
-        exit(1);
-    }
+    assert(surface_bits_per_pixel(surface) == 32)
+
+    s->line_fn_tab[0] = blizzard_draw_fn_32;
+    s->line_fn_tab[1] = blizzard_draw_fn_r_32;
 
     blizzard_reset(s);
 
diff --git a/hw/display/blizzard_template.h b/hw/display/blizzard_template.h
index b7ef27c..bc38d7a 100644
--- a/hw/display/blizzard_template.h
+++ b/hw/display/blizzard_template.h
@@ -19,31 +19,7 @@
  */
 
 #define SKIP_PIXEL(to)         (to += deststep)
-#if DEPTH == 8
-# define PIXEL_TYPE            uint8_t
-# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
-# define COPY_PIXEL1(to, from) (*to++ = from)
-#elif DEPTH == 15 || DEPTH == 16
-# define PIXEL_TYPE            uint16_t
-# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
-# define COPY_PIXEL1(to, from) (*to++ = from)
-#elif DEPTH == 24
-# define PIXEL_TYPE            uint8_t
-# define COPY_PIXEL(to, from) \
-    do {                      \
-        to[0] = from;         \
-        to[1] = (from) >> 8;  \
-        to[2] = (from) >> 16; \
-        SKIP_PIXEL(to);       \
-    } while (0)
-
-# define COPY_PIXEL1(to, from) \
-    do {                       \
-        *to++ = from;          \
-        *to++ = (from) >> 8;   \
-        *to++ = (from) >> 16;  \
-    } while (0)
-#elif DEPTH == 32
+#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)
@@ -58,9 +34,6 @@
 static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
                 const uint16_t *src, unsigned int width)
 {
-#if !defined(SWAP_WORDS) && DEPTH == 16
-    memcpy(dest, src, width);
-#else
     uint16_t data;
     unsigned int r, g, b;
     const uint16_t *end = (const void *) src + width;
@@ -74,7 +47,6 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
         data >>= 5;
         COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b));
     }
-#endif
 }
 
 static void glue(blizzard_draw_line24mode1_, DEPTH)(PIXEL_TYPE *dest,
-- 
2.5.0

Re: [Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Eric Blake]

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

On 03/24/2016 12:18 PM, Pooja Dhannawat wrote:
> Removing support for DEPTH != 32 from blizzard template header
> and file that includes it, as macro DEPTH == 32 only used.
> 
> Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
> ---

>  #define DEPTH 32
>  #include "blizzard_template.h"
>  

> +    assert(surface_bits_per_pixel(surface) == 32)

So this confirms that we only ever cared about DEPTH == 32.

> +
> +    s->line_fn_tab[0] = blizzard_draw_fn_32;
> +    s->line_fn_tab[1] = blizzard_draw_fn_r_32;

But now we are in the situation of having to look in the
blizzard_template.h file...

> +++ b/hw/display/blizzard_template.h
> @@ -19,31 +19,7 @@
>   */
>  
>  #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)
> @@ -58,9 +34,6 @@
>  static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
>                  const uint16_t *src, unsigned int width)

...and to reconstruct the results of the glue() macros to find the
actual function definitions that we are using.  I think this patch is
okay as a first step, but I think we can go one step further in a
followup patch: remove the glue() magic, delete blizzard_template.h, and
just declare the used functions directly in blizzard.c.

> @@ -74,7 +47,6 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
>          data >>= 5;
>          COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b));

And even without getting rid of the .h, there are some cleanups you can
do now that DEPTH is a constant.  For example, this line can shorten to:

COPY_PIXEL1(dest, rgb_to_pixel32(r, g, b));

which in turn becomes a bit more legible as:

*dest++ = rgb_to_pixel32(r, g, b);

Since what you have is incrementally better, I'm fine if you add:
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 v4] blizzard: Remove support for DEPTH != 32

[Fam Zheng]

On Thu, 03/24 23:48, Pooja Dhannawat wrote:
> Removing support for DEPTH != 32 from blizzard template header
> and file that includes it, as macro DEPTH == 32 only used.
> 
> Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>

Hi Pooja, a meta-comment: in the future, when post a subsequent revision,
please include what is changed since previous revision, in the cover letter if
there are multiple patches, or under a "---" line in the commit message if
there is no cover letter. (Remember that the Signed-off-by line must still stay
above the "---" line).

The reason to use "---" line is for maintainers to avoid "git am" applying the
revision changelog into git history (apparently it is meaningless out of patch
review context).

In this case, it would be like:

----8<---

    Removing support for DEPTH != 32 from blizzard template header
    and file that includes it, as macro DEPTH == 32 only used.

    Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>

    ---

    v4: Changed foo to bar, and baz to qux. [$name_of_suggester]

---->8---

Re: [Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Pooja Dhannawat]

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

On Fri, Mar 25, 2016 at 8:39 AM, Fam Zheng <famz@redhat.com> wrote:

> On Thu, 03/24 23:48, Pooja Dhannawat wrote:
> > Removing support for DEPTH != 32 from blizzard template header
> > and file that includes it, as macro DEPTH == 32 only used.
> >
> > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
>
> Hi Pooja, a meta-comment: in the future, when post a subsequent revision,
> please include what is changed since previous revision, in the cover
> letter if
> there are multiple patches, or under a "---" line in the commit message if
> there is no cover letter. (Remember that the Signed-off-by line must still
> stay
> above the "---" line).
>
> The reason to use "---" line is for maintainers to avoid "git am" applying
> the
> revision changelog into git history (apparently it is meaningless out of
> patch
> review context).
>
> In this case, it would be like:
>
> ----8<---
>
>     Removing support for DEPTH != 32 from blizzard template header
>     and file that includes it, as macro DEPTH == 32 only used.
>
>     Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
>
>     ---
>
>     v4: Changed foo to bar, and baz to qux. [$name_of_suggester]
>
> ---->8---
>

Thank you Fam. I will keep this in mind from onward.

[-- Attachment #2: Type: text/html, Size: 1878 bytes --]

[Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Pooja Dhannawat]

Removing support for DEPTH != 32 from blizzard template header
and file that includes it, as macro DEPTH == 32 only used.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
---
 hw/display/blizzard.c          | 41 ++++-------------------------------------
 hw/display/blizzard_template.h | 30 +-----------------------------
 2 files changed, 5 insertions(+), 66 deletions(-)

diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c
index c231960..5684e49 100644
--- a/hw/display/blizzard.c
+++ b/hw/display/blizzard.c
@@ -925,14 +925,6 @@ static void blizzard_update_display(void *opaque)
     s->my[1] = 0;
 }
 
-#define DEPTH 8
-#include "blizzard_template.h"
-#define DEPTH 15
-#include "blizzard_template.h"
-#define DEPTH 16
-#include "blizzard_template.h"
-#define DEPTH 24
-#include "blizzard_template.h"
 #define DEPTH 32
 #include "blizzard_template.h"
 
@@ -951,35 +943,10 @@ void *s1d13745_init(qemu_irq gpio_int)
     s->con = graphic_console_init(NULL, 0, &blizzard_ops, s);
     surface = qemu_console_surface(s->con);
 
-    switch (surface_bits_per_pixel(surface)) {
-    case 0:
-        s->line_fn_tab[0] = s->line_fn_tab[1] =
-                g_malloc0(sizeof(blizzard_fn_t) * 0x10);
-        break;
-    case 8:
-        s->line_fn_tab[0] = blizzard_draw_fn_8;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_8;
-        break;
-    case 15:
-        s->line_fn_tab[0] = blizzard_draw_fn_15;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_15;
-        break;
-    case 16:
-        s->line_fn_tab[0] = blizzard_draw_fn_16;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_16;
-        break;
-    case 24:
-        s->line_fn_tab[0] = blizzard_draw_fn_24;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_24;
-        break;
-    case 32:
-        s->line_fn_tab[0] = blizzard_draw_fn_32;
-        s->line_fn_tab[1] = blizzard_draw_fn_r_32;
-        break;
-    default:
-        fprintf(stderr, "%s: Bad color depth\n", __FUNCTION__);
-        exit(1);
-    }
+    assert(surface_bits_per_pixel(surface) == 32)
+
+    s->line_fn_tab[0] = blizzard_draw_fn_32;
+    s->line_fn_tab[1] = blizzard_draw_fn_r_32;
 
     blizzard_reset(s);
 
diff --git a/hw/display/blizzard_template.h b/hw/display/blizzard_template.h
index b7ef27c..bc38d7a 100644
--- a/hw/display/blizzard_template.h
+++ b/hw/display/blizzard_template.h
@@ -19,31 +19,7 @@
  */
 
 #define SKIP_PIXEL(to)         (to += deststep)
-#if DEPTH == 8
-# define PIXEL_TYPE            uint8_t
-# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
-# define COPY_PIXEL1(to, from) (*to++ = from)
-#elif DEPTH == 15 || DEPTH == 16
-# define PIXEL_TYPE            uint16_t
-# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
-# define COPY_PIXEL1(to, from) (*to++ = from)
-#elif DEPTH == 24
-# define PIXEL_TYPE            uint8_t
-# define COPY_PIXEL(to, from) \
-    do {                      \
-        to[0] = from;         \
-        to[1] = (from) >> 8;  \
-        to[2] = (from) >> 16; \
-        SKIP_PIXEL(to);       \
-    } while (0)
-
-# define COPY_PIXEL1(to, from) \
-    do {                       \
-        *to++ = from;          \
-        *to++ = (from) >> 8;   \
-        *to++ = (from) >> 16;  \
-    } while (0)
-#elif DEPTH == 32
+#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)
@@ -58,9 +34,6 @@
 static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
                 const uint16_t *src, unsigned int width)
 {
-#if !defined(SWAP_WORDS) && DEPTH == 16
-    memcpy(dest, src, width);
-#else
     uint16_t data;
     unsigned int r, g, b;
     const uint16_t *end = (const void *) src + width;
@@ -74,7 +47,6 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
         data >>= 5;
         COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b));
     }
-#endif
 }
 
 static void glue(blizzard_draw_line24mode1_, DEPTH)(PIXEL_TYPE *dest,
-- 
2.5.0

Re: [Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Pooja Dhannawat]

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

CCing Eric Blake and Gerd Hoffmann.

On Sat, Mar 26, 2016 at 11:27 AM, Pooja Dhannawat <dhannawatpooja1@gmail.com
> wrote:

> Removing support for DEPTH != 32 from blizzard template header
> and file that includes it, as macro DEPTH == 32 only used.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>
> ---
>  hw/display/blizzard.c          | 41
> ++++-------------------------------------
>  hw/display/blizzard_template.h | 30 +-----------------------------
>  2 files changed, 5 insertions(+), 66 deletions(-)
>
> diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c
> index c231960..5684e49 100644
> --- a/hw/display/blizzard.c
> +++ b/hw/display/blizzard.c
> @@ -925,14 +925,6 @@ static void blizzard_update_display(void *opaque)
>      s->my[1] = 0;
>  }
>
> -#define DEPTH 8
> -#include "blizzard_template.h"
> -#define DEPTH 15
> -#include "blizzard_template.h"
> -#define DEPTH 16
> -#include "blizzard_template.h"
> -#define DEPTH 24
> -#include "blizzard_template.h"
>  #define DEPTH 32
>  #include "blizzard_template.h"
>
> @@ -951,35 +943,10 @@ void *s1d13745_init(qemu_irq gpio_int)
>      s->con = graphic_console_init(NULL, 0, &blizzard_ops, s);
>      surface = qemu_console_surface(s->con);
>
> -    switch (surface_bits_per_pixel(surface)) {
> -    case 0:
> -        s->line_fn_tab[0] = s->line_fn_tab[1] =
> -                g_malloc0(sizeof(blizzard_fn_t) * 0x10);
> -        break;
> -    case 8:
> -        s->line_fn_tab[0] = blizzard_draw_fn_8;
> -        s->line_fn_tab[1] = blizzard_draw_fn_r_8;
> -        break;
> -    case 15:
> -        s->line_fn_tab[0] = blizzard_draw_fn_15;
> -        s->line_fn_tab[1] = blizzard_draw_fn_r_15;
> -        break;
> -    case 16:
> -        s->line_fn_tab[0] = blizzard_draw_fn_16;
> -        s->line_fn_tab[1] = blizzard_draw_fn_r_16;
> -        break;
> -    case 24:
> -        s->line_fn_tab[0] = blizzard_draw_fn_24;
> -        s->line_fn_tab[1] = blizzard_draw_fn_r_24;
> -        break;
> -    case 32:
> -        s->line_fn_tab[0] = blizzard_draw_fn_32;
> -        s->line_fn_tab[1] = blizzard_draw_fn_r_32;
> -        break;
> -    default:
> -        fprintf(stderr, "%s: Bad color depth\n", __FUNCTION__);
> -        exit(1);
> -    }
> +    assert(surface_bits_per_pixel(surface) == 32)
> +
> +    s->line_fn_tab[0] = blizzard_draw_fn_32;
> +    s->line_fn_tab[1] = blizzard_draw_fn_r_32;
>
>      blizzard_reset(s);
>
> diff --git a/hw/display/blizzard_template.h
> b/hw/display/blizzard_template.h
> index b7ef27c..bc38d7a 100644
> --- a/hw/display/blizzard_template.h
> +++ b/hw/display/blizzard_template.h
> @@ -19,31 +19,7 @@
>   */
>
>  #define SKIP_PIXEL(to)         (to += deststep)
> -#if DEPTH == 8
> -# define PIXEL_TYPE            uint8_t
> -# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while
> (0)
> -# define COPY_PIXEL1(to, from) (*to++ = from)
> -#elif DEPTH == 15 || DEPTH == 16
> -# define PIXEL_TYPE            uint16_t
> -# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while
> (0)
> -# define COPY_PIXEL1(to, from) (*to++ = from)
> -#elif DEPTH == 24
> -# define PIXEL_TYPE            uint8_t
> -# define COPY_PIXEL(to, from) \
> -    do {                      \
> -        to[0] = from;         \
> -        to[1] = (from) >> 8;  \
> -        to[2] = (from) >> 16; \
> -        SKIP_PIXEL(to);       \
> -    } while (0)
> -
> -# define COPY_PIXEL1(to, from) \
> -    do {                       \
> -        *to++ = from;          \
> -        *to++ = (from) >> 8;   \
> -        *to++ = (from) >> 16;  \
> -    } while (0)
> -#elif DEPTH == 32
> +#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)
> @@ -58,9 +34,6 @@
>  static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest,
>                  const uint16_t *src, unsigned int width)
>  {
> -#if !defined(SWAP_WORDS) && DEPTH == 16
> -    memcpy(dest, src, width);
> -#else
>      uint16_t data;
>      unsigned int r, g, b;
>      const uint16_t *end = (const void *) src + width;
> @@ -74,7 +47,6 @@ static void glue(blizzard_draw_line16_,
> DEPTH)(PIXEL_TYPE *dest,
>          data >>= 5;
>          COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b));
>      }
> -#endif
>  }
>
>  static void glue(blizzard_draw_line24mode1_, DEPTH)(PIXEL_TYPE *dest,
> --
> 2.5.0
>
>

[-- Attachment #2: Type: text/html, Size: 5748 bytes --]

Re: [Qemu-devel] [PATCH v4] blizzard: Remove support for DEPTH != 32

[Peter Maydell]

On 26 March 2016 at 05:57, Pooja Dhannawat <dhannawatpooja1@gmail.com> wrote:
> Removing support for DEPTH != 32 from blizzard template header
> and file that includes it, as macro DEPTH == 32 only used.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com>

Hi. Apologies for not getting to this patch earlier -- I've been
busy with a bunch of other work and the QEMU 2.6 release.

In general this patch looks good, but unfortunately it doesn't compile:

hw/display/blizzard.c: In function ‘s1d13745_init’:
hw/display/blizzard.c:948:5: error: expected ‘;’ before ‘s’
     s->line_fn_tab[0] = blizzard_draw_fn_32;
     ^
In file included from hw/display/blizzard.c:929:0:
hw/display/blizzard.c: At top level:
hw/display/blizzard_template.h:86:22: error: ‘blizzard_draw_fn_32’
defined but not used [-Werror=unused-variable]
 static blizzard_fn_t glue(blizzard_draw_fn_, DEPTH)[0x10] = {
                      ^
cc1: all warnings being treated as errors

> +    assert(surface_bits_per_pixel(surface) == 32)

This is because you're missing a semicolon here.

It's important to at least compile and preferably also test
any patch you send to the list. (Occasionally it's not possible
if the code in question requires an obscure host OS or
library combination or something, in which case you should
note in the patch remarks below the "---" line that you weren't
able to compile test.)

In this case (and since it took me so long to get back to this
patch) I've just fixed up this error and applied the result to
target-arm.next.

There are a couple of nice further cleanups we can do on top
of this to remove blizzard_template.h altogether; I'll post
those patches in a moment.

thanks
-- PMM