[PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[Peter Maydell]

For a long time now the UI layer has guaranteed that the console
surface is always 32 bits per pixel, RGB. The TCX code already
assumes 32bpp, but it still has some checks of is_surface_bgr()
in an attempt to support 32bpp BGR. is_surface_bgr() will always
return false for the qemu_console_surface(), unless the display
device itself has deliberately created an alternate-format
surface via a function like qemu_create_displaysurface_from().

Drop the never-used BGR-handling code, and assert that we have
a 32-bit surface rather than just doing nothing if it isn't.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Tested by booting Linux on an SS-5.
---
 hw/display/tcx.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 965f92ff6b7..d3db3046572 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -128,15 +128,10 @@ static int tcx_check_dirty(TCXState *s, DirtyBitmapSnapshot *snap,
 
 static void update_palette_entries(TCXState *s, int start, int end)
 {
-    DisplaySurface *surface = qemu_console_surface(s->con);
     int i;
 
     for (i = start; i < end; i++) {
-        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]);
-        }
+        s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
     }
     tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
 }
@@ -181,21 +176,18 @@ static void tcx_draw_cursor32(TCXState *s1, uint8_t *d,
 }
 
 /*
-  XXX Could be much more optimal:
-  * detect if line/page/whole screen is in 24 bit mode
-  * if destination is also BGR, use memcpy
-  */
+ * XXX Could be much more optimal:
+ * detect if line/page/whole screen is in 24 bit mode
+ */
 static inline void tcx24_draw_line32(TCXState *s1, uint8_t *d,
                                      const uint8_t *s, int width,
                                      const uint32_t *cplane,
                                      const uint32_t *s24)
 {
-    DisplaySurface *surface = qemu_console_surface(s1->con);
-    int x, bgr, r, g, b;
+    int x, r, g, b;
     uint8_t val, *p8;
     uint32_t *p = (uint32_t *)d;
     uint32_t dval;
-    bgr = is_surface_bgr(surface);
     for(x = 0; x < width; x++, s++, s24++) {
         if (be32_to_cpu(*cplane) & 0x03000000) {
             /* 24-bit direct, BGR order */
@@ -204,10 +196,7 @@ static inline void tcx24_draw_line32(TCXState *s1, uint8_t *d,
             b = *p8++;
             g = *p8++;
             r = *p8;
-            if (bgr)
-                dval = rgb_to_pixel32bgr(r, g, b);
-            else
-                dval = rgb_to_pixel32(r, g, b);
+            dval = rgb_to_pixel32(r, g, b);
         } else {
             /* 8-bit pseudocolor */
             val = *s;
@@ -230,9 +219,7 @@ static void tcx_update_display(void *opaque)
     int y, y_start, dd, ds;
     uint8_t *d, *s;
 
-    if (surface_bits_per_pixel(surface) != 32) {
-        return;
-    }
+    assert(surface_bits_per_pixel(surface) == 32);
 
     page = 0;
     y_start = -1;
@@ -283,9 +270,7 @@ static void tcx24_update_display(void *opaque)
     uint8_t *d, *s;
     uint32_t *cptr, *s24;
 
-    if (surface_bits_per_pixel(surface) != 32) {
-            return;
-    }
+    assert(surface_bits_per_pixel(surface) == 32);
 
     page = 0;
     y_start = -1;
-- 
2.20.1

Re: [PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[Mark Cave-Ayland]

On 15/02/2021 10:21, Peter Maydell wrote:

> For a long time now the UI layer has guaranteed that the console
> surface is always 32 bits per pixel, RGB. The TCX code already
> assumes 32bpp, but it still has some checks of is_surface_bgr()
> in an attempt to support 32bpp BGR. is_surface_bgr() will always
> return false for the qemu_console_surface(), unless the display
> device itself has deliberately created an alternate-format
> surface via a function like qemu_create_displaysurface_from().
> 
> Drop the never-used BGR-handling code, and assert that we have
> a 32-bit surface rather than just doing nothing if it isn't.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Tested by booting Linux on an SS-5.
> ---
>   hw/display/tcx.c | 31 ++++++++-----------------------
>   1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 965f92ff6b7..d3db3046572 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -128,15 +128,10 @@ static int tcx_check_dirty(TCXState *s, DirtyBitmapSnapshot *snap,
>   
>   static void update_palette_entries(TCXState *s, int start, int end)
>   {
> -    DisplaySurface *surface = qemu_console_surface(s->con);
>       int i;
>   
>       for (i = start; i < end; i++) {
> -        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]);
> -        }
> +        s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
>       }
>       tcx_set_dirty(s, 0, memory_region_size(&s->vram_mem));
>   }
> @@ -181,21 +176,18 @@ static void tcx_draw_cursor32(TCXState *s1, uint8_t *d,
>   }
>   
>   /*
> -  XXX Could be much more optimal:
> -  * detect if line/page/whole screen is in 24 bit mode
> -  * if destination is also BGR, use memcpy
> -  */
> + * XXX Could be much more optimal:
> + * detect if line/page/whole screen is in 24 bit mode
> + */
>   static inline void tcx24_draw_line32(TCXState *s1, uint8_t *d,
>                                        const uint8_t *s, int width,
>                                        const uint32_t *cplane,
>                                        const uint32_t *s24)
>   {
> -    DisplaySurface *surface = qemu_console_surface(s1->con);
> -    int x, bgr, r, g, b;
> +    int x, r, g, b;
>       uint8_t val, *p8;
>       uint32_t *p = (uint32_t *)d;
>       uint32_t dval;
> -    bgr = is_surface_bgr(surface);
>       for(x = 0; x < width; x++, s++, s24++) {
>           if (be32_to_cpu(*cplane) & 0x03000000) {
>               /* 24-bit direct, BGR order */
> @@ -204,10 +196,7 @@ static inline void tcx24_draw_line32(TCXState *s1, uint8_t *d,
>               b = *p8++;
>               g = *p8++;
>               r = *p8;
> -            if (bgr)
> -                dval = rgb_to_pixel32bgr(r, g, b);
> -            else
> -                dval = rgb_to_pixel32(r, g, b);
> +            dval = rgb_to_pixel32(r, g, b);
>           } else {
>               /* 8-bit pseudocolor */
>               val = *s;
> @@ -230,9 +219,7 @@ static void tcx_update_display(void *opaque)
>       int y, y_start, dd, ds;
>       uint8_t *d, *s;
>   
> -    if (surface_bits_per_pixel(surface) != 32) {
> -        return;
> -    }
> +    assert(surface_bits_per_pixel(surface) == 32);
>   
>       page = 0;
>       y_start = -1;
> @@ -283,9 +270,7 @@ static void tcx24_update_display(void *opaque)
>       uint8_t *d, *s;
>       uint32_t *cptr, *s24;
>   
> -    if (surface_bits_per_pixel(surface) != 32) {
> -            return;
> -    }
> +    assert(surface_bits_per_pixel(surface) == 32);
>   
>       page = 0;
>       y_start = -1;

Thanks Peter - looks good to me.

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

Would you like this to go via a qemu-sparc PR or is it easier to go as part of a 
group alongside your other display surface patches via target-arm.next?


ATB,

Mark.

Re: [PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[Richard Henderson]

On 2/15/21 2:21 AM, Peter Maydell wrote:
> For a long time now the UI layer has guaranteed that the console
> surface is always 32 bits per pixel, RGB. The TCX code already
> assumes 32bpp, but it still has some checks of is_surface_bgr()
> in an attempt to support 32bpp BGR. is_surface_bgr() will always
> return false for the qemu_console_surface(), unless the display
> device itself has deliberately created an alternate-format
> surface via a function like qemu_create_displaysurface_from().
> 
> Drop the never-used BGR-handling code, and assert that we have
> a 32-bit surface rather than just doing nothing if it isn't.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Tested by booting Linux on an SS-5.
> ---
>  hw/display/tcx.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[Peter Maydell]

On Mon, 15 Feb 2021 at 22:41, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 15/02/2021 10:21, Peter Maydell wrote:
>
> > For a long time now the UI layer has guaranteed that the console
> > surface is always 32 bits per pixel, RGB. The TCX code already
> > assumes 32bpp, but it still has some checks of is_surface_bgr()
> > in an attempt to support 32bpp BGR. is_surface_bgr() will always
> > return false for the qemu_console_surface(), unless the display
> > device itself has deliberately created an alternate-format
> > surface via a function like qemu_create_displaysurface_from().
> >
> > Drop the never-used BGR-handling code, and assert that we have
> > a 32-bit surface rather than just doing nothing if it isn't.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> Thanks Peter - looks good to me.
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Would you like this to go via a qemu-sparc PR or is it easier to go as part of a
> group alongside your other display surface patches via target-arm.next?

I'm happy either way -- if you don't happen to have anything else
queued up for sparc I can just put it in with the arm queue.

thanks
-- PMM

Re: [PATCH] hw/display/tcx: Drop unnecessary code for handling BGR format outputs

[Mark Cave-Ayland]

On 16/02/2021 10:11, Peter Maydell wrote:

>> Would you like this to go via a qemu-sparc PR or is it easier to go as part of a
>> group alongside your other display surface patches via target-arm.next?
> 
> I'm happy either way -- if you don't happen to have anything else
> queued up for sparc I can just put it in with the arm queue.

Nothing at the moment. I'm not sure whether the ESP patches will go via qemu-sparc or 
Laurent's m68k queue, and the ESP series is probably large enough by itself already. 
So if you've got a pending PR feel free to add it in, otherwise I'll have a look 
post-ESP :)


ATB,

Mark.