[Qemu-devel] [PATCH 0/2] sm501: drop obsolete code

[Qemu-devel] [PATCH 0/2] sm501: drop obsolete code

[Gerd Hoffmann]

  Hi,

This little series drops a bunch of code which is not needed any more.
Nowdays qemu console only uses 32bpp surfaces.

BALATON, this should simplify your job of getting the pixel conversions
right.  Feel free to add this to your series.

Gerd Hoffmann (2):
  sm501: drop support for anything but 32bpp
  sm501: drop sm501_template.h

 hw/display/sm501.c          | 167 +++++++++++++++++++++++---------------------
 hw/display/sm501_template.h | 145 --------------------------------------
 2 files changed, 86 insertions(+), 226 deletions(-)
 delete mode 100644 hw/display/sm501_template.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] sm501: drop support for anything but 32bpp

[Gerd Hoffmann]

The qemu console code hands out 32bpp display surfaces only, when using
qemu_console_resize or qemu_create_displaysurface (which sm501 does).
So there is no need to ever generate other output.  Drop the code.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c | 89 +++---------------------------------------------------
 1 file changed, 4 insertions(+), 85 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 040a0b9..6c7717c 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1173,89 +1173,9 @@ typedef void draw_line_func(uint8_t *d, const uint8_t *s,
 typedef void draw_hwc_line_func(SM501State * s, int crt, uint8_t * palette,
                                 int c_y, uint8_t *d, int width);
 
-#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_draw_crt(SM501State * s)
 {
     DisplaySurface *surface = qemu_console_surface(s->con);
@@ -1269,7 +1189,6 @@ static void sm501_draw_crt(SM501State * s)
     uint32_t * palette = (uint32_t *)&s->dc_palette[SM501_DC_CRT_PALETTE
 						    - SM501_DC_PANEL_PALETTE];
     uint8_t hwc_palette[3 * 3];
-    int ds_depth_index = get_depth_index(surface);
     draw_line_func * draw_line = NULL;
     draw_hwc_line_func * draw_hwc_line = NULL;
     int full_update = 0;
@@ -1282,15 +1201,15 @@ static void sm501_draw_crt(SM501State * s)
     switch (s->dc_crt_control & 3) {
     case SM501_DC_CRT_CONTROL_8BPP:
 	src_bpp = 1;
-	draw_line = draw_line8_funcs[ds_depth_index];
+	draw_line = draw_line8_32;
 	break;
     case SM501_DC_CRT_CONTROL_16BPP:
 	src_bpp = 2;
-	draw_line = draw_line16_funcs[ds_depth_index];
+	draw_line = draw_line16_32;
 	break;
     case SM501_DC_CRT_CONTROL_32BPP:
 	src_bpp = 4;
-	draw_line = draw_line32_funcs[ds_depth_index];
+	draw_line = draw_line32_32;
 	break;
     default:
 	printf("sm501 draw crt : invalid DC_CRT_CONTROL=%x.\n",
@@ -1312,7 +1231,7 @@ static void sm501_draw_crt(SM501State * s)
         }
 
         /* choose cursor draw line function */
-        draw_hwc_line = draw_hwc_line_funcs[ds_depth_index];
+        draw_hwc_line = draw_hwc_line_32;
     }
 
     /* adjust console size */
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] sm501: drop sm501_template.h

[Gerd Hoffmann]

Now that we include sm501_template.h only once there
is no need to keep this stuff in a separate file.

FIXME: as peter mentioned the TARGET_WORDS_BIGENDIAN dependency looks
       suspious.  Left as-is for now.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/sm501.c          | 110 +++++++++++++++++++++++++++++----
 hw/display/sm501_template.h | 145 --------------------------------------------
 2 files changed, 98 insertions(+), 157 deletions(-)
 delete mode 100644 hw/display/sm501_template.h

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 6c7717c..44171c1 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1170,11 +1170,101 @@ static const MemoryRegionOps sm501_2d_engine_ops = {
 typedef void draw_line_func(uint8_t *d, const uint8_t *s,
 			    int width, const uint32_t *pal);
 
-typedef void draw_hwc_line_func(SM501State * s, int crt, uint8_t * palette,
-                                int c_y, uint8_t *d, int width);
+static void draw_line8(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)[0] = rgb_to_pixel32(r, g, b);
+        s ++;
+        d += 4;
+    } while (-- width != 0);
+}
 
-#define DEPTH 32
-#include "sm501_template.h"
+static void draw_line16(uint8_t *d, const uint8_t *s,
+                        int width, const uint32_t *pal)
+{
+    uint16_t rgb565;
+    uint8_t r, g, b;
+
+    do {
+        rgb565 = lduw_p(s);
+        r = ((rgb565 >> 11) & 0x1f) << 3;
+        g = ((rgb565 >>  5) & 0x3f) << 2;
+        b = ((rgb565 >>  0) & 0x1f) << 3;
+        ((uint32_t *) d)[0] = rgb_to_pixel32(r, g, b);
+        s += 2;
+        d += 4;
+    } while (-- width != 0);
+}
+
+static void draw_line32(uint8_t *d, const uint8_t *s,
+                        int width, const uint32_t *pal)
+{
+    uint8_t r, g, b;
+
+    do {
+        ldub_p(s);
+#if defined(TARGET_WORDS_BIGENDIAN)
+        r = s[1];
+        g = s[2];
+        b = s[3];
+#else
+        b = s[0];
+        g = s[1];
+        r = s[2];
+#endif
+        ((uint32_t *) d)[0] = 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(SM501State * s, int crt,
+                          uint8_t * palette, int c_y, uint8_t *d, int width)
+{
+    int x, i;
+    uint8_t bitset = 0;
+
+    /* get hardware cursor pattern */
+    uint32_t cursor_addr = get_hwc_address(s, crt);
+    assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
+    cursor_addr += 64 * c_y / 4;  /* 4 pixels per byte */
+    cursor_addr += s->base;
+
+    /* get cursor position */
+    x = get_hwc_x(s, crt);
+    d += x * 4;
+
+    for (i = 0; i < SM501_HWC_WIDTH && x + i < width; i++) {
+        uint8_t v;
+
+        /* get pixel value */
+        if (i % 4 == 0) {
+            bitset = ldub_phys(&address_space_memory, cursor_addr);
+            cursor_addr++;
+        }
+        v = bitset & 3;
+        bitset >>= 2;
+
+        /* write pixel */
+        if (v) {
+            v--;
+            uint8_t r = palette[v * 3 + 0];
+            uint8_t g = palette[v * 3 + 1];
+            uint8_t b = palette[v * 3 + 2];
+            ((uint32_t *) d)[0] = rgb_to_pixel32(r, g, b);
+        }
+        d += 4;
+    }
+}
 
 static void sm501_draw_crt(SM501State * s)
 {
@@ -1190,7 +1280,6 @@ static void sm501_draw_crt(SM501State * s)
 						    - SM501_DC_PANEL_PALETTE];
     uint8_t hwc_palette[3 * 3];
     draw_line_func * draw_line = NULL;
-    draw_hwc_line_func * draw_hwc_line = NULL;
     int full_update = 0;
     int y_start = -1;
     ram_addr_t page_min = ~0l;
@@ -1201,15 +1290,15 @@ static void sm501_draw_crt(SM501State * s)
     switch (s->dc_crt_control & 3) {
     case SM501_DC_CRT_CONTROL_8BPP:
 	src_bpp = 1;
-	draw_line = draw_line8_32;
+	draw_line = draw_line8;
 	break;
     case SM501_DC_CRT_CONTROL_16BPP:
 	src_bpp = 2;
-	draw_line = draw_line16_32;
+	draw_line = draw_line16;
 	break;
     case SM501_DC_CRT_CONTROL_32BPP:
 	src_bpp = 4;
-	draw_line = draw_line32_32;
+	draw_line = draw_line32;
 	break;
     default:
 	printf("sm501 draw crt : invalid DC_CRT_CONTROL=%x.\n",
@@ -1229,9 +1318,6 @@ static void sm501_draw_crt(SM501State * s)
             hwc_palette[i * 3 + 1] = (rgb565 & 0x07e0) >> 3; /* green */
             hwc_palette[i * 3 + 2] = (rgb565 & 0x001f) << 3; /* blue */
         }
-
-        /* choose cursor draw line function */
-        draw_hwc_line = draw_hwc_line_32;
     }
 
     /* adjust console size */
@@ -1246,7 +1332,7 @@ static void sm501_draw_crt(SM501State * s)
     /* draw each line according to conditions */
     memory_region_sync_dirty_bitmap(&s->local_mem_region);
     for (y = 0; y < height; y++) {
-	int update_hwc = draw_hwc_line ? within_hwc_y_range(s, y, 1) : 0;
+	int update_hwc = is_hwc_enabled(s, 1) && within_hwc_y_range(s, y, 1);
 	int update = full_update || update_hwc;
         ram_addr_t page0 = offset;
         ram_addr_t page1 = offset + width * src_bpp - 1;
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
deleted file mode 100644
index f33e499..0000000
--- a/hw/display/sm501_template.h
+++ /dev/null
@@ -1,145 +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.
- */
-
-#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)
-{
-    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;
-	((PIXEL_TYPE *) d)[0] = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
-	s ++;
-	d += BPP;
-    } while (-- width != 0);
-}
-
-static void glue(draw_line16_, PIXEL_NAME)(
-		 uint8_t *d, const uint8_t *s, int width, const uint32_t *pal)
-{
-    uint16_t rgb565;
-    uint8_t r, g, b;
-
-    do {
-	rgb565 = lduw_p(s);
-	r = ((rgb565 >> 11) & 0x1f) << 3;
-	g = ((rgb565 >>  5) & 0x3f) << 2;
-	b = ((rgb565 >>  0) & 0x1f) << 3;
-	((PIXEL_TYPE *) d)[0] = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
-	s += 2;
-	d += BPP;
-    } while (-- width != 0);
-}
-
-static void glue(draw_line32_, PIXEL_NAME)(
-		 uint8_t *d, const uint8_t *s, int width, const uint32_t *pal)
-{
-    uint8_t r, g, b;
-
-    do {
-	ldub_p(s);
-#if defined(TARGET_WORDS_BIGENDIAN)
-        r = s[1];
-        g = s[2];
-        b = s[3];
-#else
-        b = s[0];
-        g = s[1];
-        r = s[2];
-#endif
-	((PIXEL_TYPE *) d)[0] = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
-	s += 4;
-	d += BPP;
-    } while (-- width != 0);
-}
-
-/**
- * Draw hardware cursor image on the given line.
- */
-static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State * s, int crt,
-                         uint8_t * palette, int c_y, uint8_t *d, int width)
-{
-    int x, i;
-    uint8_t bitset = 0;
-
-    /* get hardware cursor pattern */
-    uint32_t cursor_addr = get_hwc_address(s, crt);
-    assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
-    cursor_addr += 64 * c_y / 4;  /* 4 pixels per byte */
-    cursor_addr += s->base;
-
-    /* get cursor position */
-    x = get_hwc_x(s, crt);
-    d += x * BPP;
-
-    for (i = 0; i < SM501_HWC_WIDTH && x + i < width; i++) {
-        uint8_t v;
-
-        /* get pixel value */
-        if (i % 4 == 0) {
-            bitset = ldub_phys(&address_space_memory, cursor_addr);
-            cursor_addr++;
-        }
-        v = bitset & 3;
-        bitset >>= 2;
-
-        /* write pixel */
-        if (v) {
-            v--;
-            uint8_t r = palette[v * 3 + 0];
-            uint8_t g = palette[v * 3 + 1];
-            uint8_t b = palette[v * 3 + 2];
-            ((PIXEL_TYPE *) d)[0] = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
-        }
-        d += BPP;
-    }
-}
-
-#undef DEPTH
-#undef BPP
-#undef PIXEL_TYPE
-#undef PIXEL_NAME
-#undef BGR_FORMAT
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] sm501: drop obsolete code

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1488810230-31945-1-git-send-email-kraxel@redhat.com
Subject: [Qemu-devel] [PATCH 0/2] sm501: drop obsolete code
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1488810230-31945-1-git-send-email-kraxel@redhat.com -> patchew/1488810230-31945-1-git-send-email-kraxel@redhat.com
Switched to a new branch 'test'
6c01c06 sm501: drop sm501_template.h
1eac5db sm501: drop support for anything but 32bpp

=== OUTPUT BEGIN ===
Checking PATCH 1/2: sm501: drop support for anything but 32bpp...
ERROR: code indent should never use tabs
#120: FILE: hw/display/sm501.c:1204:
+^Idraw_line = draw_line8_32;$

ERROR: code indent should never use tabs
#125: FILE: hw/display/sm501.c:1208:
+^Idraw_line = draw_line16_32;$

ERROR: code indent should never use tabs
#130: FILE: hw/display/sm501.c:1212:
+^Idraw_line = draw_line32_32;$

total: 3 errors, 0 warnings, 122 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: sm501: drop sm501_template.h...
ERROR: space prohibited before that '++' (ctx:WxO)
#35: FILE: hw/display/sm501.c:1183:
+        s ++;
           ^

ERROR: "foo * bar" should be "foo *bar"
#84: FILE: hw/display/sm501.c:1230:
+static void draw_hwc_line(SM501State * s, int crt,

ERROR: "foo * bar" should be "foo *bar"
#85: FILE: hw/display/sm501.c:1231:
+                          uint8_t * palette, int c_y, uint8_t *d, int width)

ERROR: code indent should never use tabs
#138: FILE: hw/display/sm501.c:1293:
+^Idraw_line = draw_line8;$

ERROR: code indent should never use tabs
#143: FILE: hw/display/sm501.c:1297:
+^Idraw_line = draw_line16;$

ERROR: code indent should never use tabs
#148: FILE: hw/display/sm501.c:1301:
+^Idraw_line = draw_line32;$

ERROR: code indent should never use tabs
#167: FILE: hw/display/sm501.c:1335:
+^Iint update_hwc = is_hwc_enabled(s, 1) && within_hwc_y_range(s, y, 1);$

total: 7 errors, 0 warnings, 147 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 2/2] sm501: drop sm501_template.h

[Eric Blake]

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

On 03/06/2017 08:23 AM, Gerd Hoffmann wrote:
> Now that we include sm501_template.h only once there
> is no need to keep this stuff in a separate file.
> 
> FIXME: as peter mentioned the TARGET_WORDS_BIGENDIAN dependency looks
>        suspious.  Left as-is for now.

s/suspious/suspicious/

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/sm501.c          | 110 +++++++++++++++++++++++++++++----
>  hw/display/sm501_template.h | 145 --------------------------------------------
>  2 files changed, 98 insertions(+), 157 deletions(-)
>  delete mode 100644 hw/display/sm501_template.h
> 

-- 
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] sm501: drop obsolete code

[BALATON Zoltan]

Hello,

On Mon, 6 Mar 2017, Gerd Hoffmann wrote:
> This little series drops a bunch of code which is not needed any more.
> Nowdays qemu console only uses 32bpp surfaces.
>
> BALATON, this should simplify your job of getting the pixel conversions
> right.  Feel free to add this to your series.

Thanks but this would actually complicate my job, as applying this before 
my series would need rebasing my patches again to this change and after 
that it won't apply as it is because the draw functions are changed. Maybe 
this change could be done independently after my series gets in. Having 
the unused functions should not cause any problems other than some unused 
code in the executable so it does not seem to matter for pixel 
conversions.

Regards,
BALATON Zoltan

> Gerd Hoffmann (2):
>  sm501: drop support for anything but 32bpp
>  sm501: drop sm501_template.h
>
> hw/display/sm501.c          | 167 +++++++++++++++++++++++---------------------
> hw/display/sm501_template.h | 145 --------------------------------------
> 2 files changed, 86 insertions(+), 226 deletions(-)
> delete mode 100644 hw/display/sm501_template.h
>
>