[Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[iwona260909]

Removing code for unsupproted DEPTH. This time I looked for it in mailing list archive and haven't found, hope it wasn't done.

[Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[iwona260909]

From: Iwona Kotlarska <iwona260909@gmail.com>

Signed-off-by: Iwona Kotlarska <iwona260909@gmail.com>
---
 hw/display/cirrus_vga.c      |  3 ---
 hw/display/cirrus_vga_rop.h  |  9 ---------
 hw/display/cirrus_vga_rop2.h | 46 ++------------------------------------------
 3 files changed, 2 insertions(+), 56 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index b9e7cb1df1..efa9609ccd 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -478,9 +478,6 @@ static const cirrus_bitblt_rop_t cirrus_bkwd_transp_rop[16][2] = {
 };
 
 #define ROP2(name) {\
-    name ## _8,\
-    name ## _16,\
-    name ## _24,\
     name ## _32,\
         }
 
diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
index 0925a009fe..f175c808cc 100644
--- a/hw/display/cirrus_vga_rop.h
+++ b/hw/display/cirrus_vga_rop.h
@@ -189,15 +189,6 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
     }
 }
 
-#define DEPTH 8
-#include "cirrus_vga_rop2.h"
-
-#define DEPTH 16
-#include "cirrus_vga_rop2.h"
-
-#define DEPTH 24
-#include "cirrus_vga_rop2.h"
-
 #define DEPTH 32
 #include "cirrus_vga_rop2.h"
 
diff --git a/hw/display/cirrus_vga_rop2.h b/hw/display/cirrus_vga_rop2.h
index d28bcc6f25..adf0f30a94 100644
--- a/hw/display/cirrus_vga_rop2.h
+++ b/hw/display/cirrus_vga_rop2.h
@@ -22,15 +22,8 @@
  * THE SOFTWARE.
  */
 
-#if DEPTH == 8
-#define PUTPIXEL()    ROP_OP(&d[0], col)
-#elif DEPTH == 16
-#define PUTPIXEL()    ROP_OP_16((uint16_t *)&d[0], col)
-#elif DEPTH == 24
-#define PUTPIXEL()    ROP_OP(&d[0], col);        \
-                      ROP_OP(&d[1], (col >> 8)); \
-                      ROP_OP(&d[2], (col >> 16))
-#elif DEPTH == 32
+
+#if DEPTH == 32
 #define PUTPIXEL()    ROP_OP_32(((uint32_t *)&d[0]), col)
 #else
 #error unsupported DEPTH
@@ -47,41 +40,16 @@ glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
     int x, y, pattern_y, pattern_pitch, pattern_x;
     unsigned int col;
     const uint8_t *src1;
-#if DEPTH == 24
-    int skipleft = s->vga.gr[0x2f] & 0x1f;
-#else
     int skipleft = (s->vga.gr[0x2f] & 0x07) * (DEPTH / 8);
-#endif
-
-#if DEPTH == 8
-    pattern_pitch = 8;
-#elif DEPTH == 16
-    pattern_pitch = 16;
-#else
     pattern_pitch = 32;
-#endif
     pattern_y = s->cirrus_blt_srcaddr & 7;
     for(y = 0; y < bltheight; y++) {
         pattern_x = skipleft;
         d = dst + skipleft;
         src1 = src + pattern_y * pattern_pitch;
         for (x = skipleft; x < bltwidth; x += (DEPTH / 8)) {
-#if DEPTH == 8
-            col = src1[pattern_x];
-            pattern_x = (pattern_x + 1) & 7;
-#elif DEPTH == 16
-            col = ((uint16_t *)(src1 + pattern_x))[0];
-            pattern_x = (pattern_x + 2) & 15;
-#elif DEPTH == 24
-            {
-                const uint8_t *src2 = src1 + pattern_x * 3;
-                col = src2[0] | (src2[1] << 8) | (src2[2] << 16);
-                pattern_x = (pattern_x + 1) & 7;
-            }
-#else
             col = ((uint32_t *)(src1 + pattern_x))[0];
             pattern_x = (pattern_x + 4) & 31;
-#endif
             PUTPIXEL();
             d += (DEPTH / 8);
         }
@@ -104,13 +72,8 @@ glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
     unsigned int col;
     unsigned bitmask;
     unsigned index;
-#if DEPTH == 24
-    int dstskipleft = s->vga.gr[0x2f] & 0x1f;
-    int srcskipleft = dstskipleft / 3;
-#else
     int srcskipleft = s->vga.gr[0x2f] & 0x07;
     int dstskipleft = srcskipleft * (DEPTH / 8);
-#endif
 
     if (s->cirrus_blt_modeext & CIRRUS_BLTMODEEXT_COLOREXPINV) {
         bits_xor = 0xff;
@@ -187,13 +150,8 @@ glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
     int x, y, bitpos, pattern_y;
     unsigned int bits, bits_xor;
     unsigned int col;
-#if DEPTH == 24
-    int dstskipleft = s->vga.gr[0x2f] & 0x1f;
-    int srcskipleft = dstskipleft / 3;
-#else
     int srcskipleft = s->vga.gr[0x2f] & 0x07;
     int dstskipleft = srcskipleft * (DEPTH / 8);
-#endif
 
     if (s->cirrus_blt_modeext & CIRRUS_BLTMODEEXT_COLOREXPINV) {
         bits_xor = 0xff;
-- 
2.12.0

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Peter Maydell]

On 16 March 2017 at 12:01,  <iwona260909@gmail.com> wrote:
> From: Iwona Kotlarska <iwona260909@gmail.com>
>
> Signed-off-by: Iwona Kotlarska <iwona260909@gmail.com>

Thanks -- cc'ing Gerd who's just done some cirrus patches and
is probably best placed to review this.

PS: if you put "cirrus_vga:" in your patch subject then it
helps people to know what part of QEMU your patch is affecting.

-- PMM

> ---
>  hw/display/cirrus_vga.c      |  3 ---
>  hw/display/cirrus_vga_rop.h  |  9 ---------
>  hw/display/cirrus_vga_rop2.h | 46 ++------------------------------------------
>  3 files changed, 2 insertions(+), 56 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index b9e7cb1df1..efa9609ccd 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -478,9 +478,6 @@ static const cirrus_bitblt_rop_t cirrus_bkwd_transp_rop[16][2] = {
>  };
>
>  #define ROP2(name) {\
> -    name ## _8,\
> -    name ## _16,\
> -    name ## _24,\
>      name ## _32,\
>          }
>
> diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
> index 0925a009fe..f175c808cc 100644
> --- a/hw/display/cirrus_vga_rop.h
> +++ b/hw/display/cirrus_vga_rop.h
> @@ -189,15 +189,6 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
>      }
>  }
>
> -#define DEPTH 8
> -#include "cirrus_vga_rop2.h"
> -
> -#define DEPTH 16
> -#include "cirrus_vga_rop2.h"
> -
> -#define DEPTH 24
> -#include "cirrus_vga_rop2.h"
> -
>  #define DEPTH 32
>  #include "cirrus_vga_rop2.h"
>
> diff --git a/hw/display/cirrus_vga_rop2.h b/hw/display/cirrus_vga_rop2.h
> index d28bcc6f25..adf0f30a94 100644
> --- a/hw/display/cirrus_vga_rop2.h
> +++ b/hw/display/cirrus_vga_rop2.h
> @@ -22,15 +22,8 @@
>   * THE SOFTWARE.
>   */
>
> -#if DEPTH == 8
> -#define PUTPIXEL()    ROP_OP(&d[0], col)
> -#elif DEPTH == 16
> -#define PUTPIXEL()    ROP_OP_16((uint16_t *)&d[0], col)
> -#elif DEPTH == 24
> -#define PUTPIXEL()    ROP_OP(&d[0], col);        \
> -                      ROP_OP(&d[1], (col >> 8)); \
> -                      ROP_OP(&d[2], (col >> 16))
> -#elif DEPTH == 32
> +
> +#if DEPTH == 32
>  #define PUTPIXEL()    ROP_OP_32(((uint32_t *)&d[0]), col)
>  #else
>  #error unsupported DEPTH
> @@ -47,41 +40,16 @@ glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
>      int x, y, pattern_y, pattern_pitch, pattern_x;
>      unsigned int col;
>      const uint8_t *src1;
> -#if DEPTH == 24
> -    int skipleft = s->vga.gr[0x2f] & 0x1f;
> -#else
>      int skipleft = (s->vga.gr[0x2f] & 0x07) * (DEPTH / 8);
> -#endif
> -
> -#if DEPTH == 8
> -    pattern_pitch = 8;
> -#elif DEPTH == 16
> -    pattern_pitch = 16;
> -#else
>      pattern_pitch = 32;
> -#endif
>      pattern_y = s->cirrus_blt_srcaddr & 7;
>      for(y = 0; y < bltheight; y++) {
>          pattern_x = skipleft;
>          d = dst + skipleft;
>          src1 = src + pattern_y * pattern_pitch;
>          for (x = skipleft; x < bltwidth; x += (DEPTH / 8)) {
> -#if DEPTH == 8
> -            col = src1[pattern_x];
> -            pattern_x = (pattern_x + 1) & 7;
> -#elif DEPTH == 16
> -            col = ((uint16_t *)(src1 + pattern_x))[0];
> -            pattern_x = (pattern_x + 2) & 15;
> -#elif DEPTH == 24
> -            {
> -                const uint8_t *src2 = src1 + pattern_x * 3;
> -                col = src2[0] | (src2[1] << 8) | (src2[2] << 16);
> -                pattern_x = (pattern_x + 1) & 7;
> -            }
> -#else
>              col = ((uint32_t *)(src1 + pattern_x))[0];
>              pattern_x = (pattern_x + 4) & 31;
> -#endif
>              PUTPIXEL();
>              d += (DEPTH / 8);
>          }
> @@ -104,13 +72,8 @@ glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
>      unsigned int col;
>      unsigned bitmask;
>      unsigned index;
> -#if DEPTH == 24
> -    int dstskipleft = s->vga.gr[0x2f] & 0x1f;
> -    int srcskipleft = dstskipleft / 3;
> -#else
>      int srcskipleft = s->vga.gr[0x2f] & 0x07;
>      int dstskipleft = srcskipleft * (DEPTH / 8);
> -#endif
>
>      if (s->cirrus_blt_modeext & CIRRUS_BLTMODEEXT_COLOREXPINV) {
>          bits_xor = 0xff;
> @@ -187,13 +150,8 @@ glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
>      int x, y, bitpos, pattern_y;
>      unsigned int bits, bits_xor;
>      unsigned int col;
> -#if DEPTH == 24
> -    int dstskipleft = s->vga.gr[0x2f] & 0x1f;
> -    int srcskipleft = dstskipleft / 3;
> -#else
>      int srcskipleft = s->vga.gr[0x2f] & 0x07;
>      int dstskipleft = srcskipleft * (DEPTH / 8);
> -#endif
>
>      if (s->cirrus_blt_modeext & CIRRUS_BLTMODEEXT_COLOREXPINV) {
>          bits_xor = 0xff;
> --
> 2.12.0
>
>

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Gerd Hoffmann]

  Hi,

> > -#define DEPTH 8
> > -#include "cirrus_vga_rop2.h"
> > -
> > -#define DEPTH 16
> > -#include "cirrus_vga_rop2.h"
> > -
> > -#define DEPTH 24
> > -#include "cirrus_vga_rop2.h"
> > -
> >  #define DEPTH 32
> >  #include "cirrus_vga_rop2.h"

No.  It isn't *that* simple.  The cirrus blitter operates on the guest
framebuffer, which can have any format the guest wishes it to have.
This is *not* about rendering into a 32bpp DisplaySurface.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Paolo Bonzini]

On 16/03/2017 15:12, Gerd Hoffmann wrote:
>   Hi,
> 
>>> -#define DEPTH 8
>>> -#include "cirrus_vga_rop2.h"
>>> -
>>> -#define DEPTH 16
>>> -#include "cirrus_vga_rop2.h"
>>> -
>>> -#define DEPTH 24
>>> -#include "cirrus_vga_rop2.h"
>>> -
>>>  #define DEPTH 32
>>>  #include "cirrus_vga_rop2.h"
> 
> No.  It isn't *that* simple.  The cirrus blitter operates on the guest
> framebuffer, which can have any format the guest wishes it to have.
> This is *not* about rendering into a 32bpp DisplaySurface.

Besides that the patch is incomplete, since it doesn't adjust the users
of the ROP2 macro---this will probably apply to other display adapters
where you can do this kind of dead code removal.

Paolo

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Stefan Hajnoczi]

On Thu, Mar 16, 2017 at 2:12 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > -#define DEPTH 8
>> > -#include "cirrus_vga_rop2.h"
>> > -
>> > -#define DEPTH 16
>> > -#include "cirrus_vga_rop2.h"
>> > -
>> > -#define DEPTH 24
>> > -#include "cirrus_vga_rop2.h"
>> > -
>> >  #define DEPTH 32
>> >  #include "cirrus_vga_rop2.h"
>
> No.  It isn't *that* simple.  The cirrus blitter operates on the guest
> framebuffer, which can have any format the guest wishes it to have.
> This is *not* about rendering into a 32bpp DisplaySurface.

Gerd, please reply if this is wrong, but here is a summary of the task:

Some emulated graphics cards use qemu_console_surface() and that
surface is always in 32 bits per pixel format.  Therefore, code for
dealing with other pixel formats can be removed.

When looking for emulated graphics cards where this cleanup is
possible, keep in mind that some of the code operates on the guest's
frame buffer, whose pixel format is not under QEMU's control.
Therefore we cannot assume that 32bpp is always used and this cleanup
doesn't apply there.  Limit yourself to code that uses
qemu_console_surface() exclusively to access the surface and you
should be fine.  Other code is likely to need support for additional
pixel formats.

One example is hw/display/milkymist-vgafb.c:

static void vgafb_update_display(void *opaque)
{
    DisplaySurface *surface = qemu_console_surface(s->con);
    ...
    switch (surface_bits_per_pixel(surface)) {
    case 0:
        return;
    case 8:
        fn = draw_line_8;
        break;
    case 15:
        fn = draw_line_15;
        dest_width *= 2;
        break;
    case 16:
        fn = draw_line_16;
        dest_width *= 2;
        break;
    case 24:
        fn = draw_line_24;
        dest_width *= 3;
        break;
    case 32:
        fn = draw_line_32;
        dest_width *= 4;
        break;

All cases except for 32 are dead code (they will never execute).  The
hw/display/milkymist-vgafb_template.h file can be deleted.
draw_line_32() can be macro expanded and moved into
hw/display/milkymist-vgafb.c.

Stefan

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Peter Maydell]

On 24 March 2017 at 13:36, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Gerd, please reply if this is wrong, but here is a summary of the task:
>
> Some emulated graphics cards use qemu_console_surface() and that
> surface is always in 32 bits per pixel format.  Therefore, code for
> dealing with other pixel formats can be removed.
>
> When looking for emulated graphics cards where this cleanup is
> possible, keep in mind that some of the code operates on the guest's
> frame buffer, whose pixel format is not under QEMU's control.
> Therefore we cannot assume that 32bpp is always used and this cleanup
> doesn't apply there.  Limit yourself to code that uses
> qemu_console_surface() exclusively to access the surface and you
> should be fine.  Other code is likely to need support for additional
> pixel formats.

Also worth pointing at an example of a patchset that does
this for some other device, eg Gerd's recent pl110 set,
as a guide to what's getting removed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Gerd Hoffmann]

  Hi,

> Some emulated graphics cards use qemu_console_surface() and that
> surface is always in 32 bits per pixel format.  Therefore, code for
> dealing with other pixel formats can be removed.

qemu_create_displaysurface() creates a displaysurface with the default
depth 32bpp.  qemu_console_resize() will create a new displaysurface,
with the given size, also with the default depth 32bpp.

qemu_console_surface() just asks for the current surface, that alone
isn't a good indicator, but often display drivers use
qemu_console_surface + qemu_console_resize.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Gerd Hoffmann]

  Hi,

> Also worth pointing at an example of a patchset that does
> this for some other device, eg Gerd's recent pl110 set,
> as a guide to what's getting removed.

that was sm501 ;)

https://patchwork.ozlabs.org/patch/735753/
https://patchwork.ozlabs.org/patch/735764/

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] Dead code removal: removing code for unsupported DEPTH.

[Iwona Kotlarska]

Hi,
I wanted to say that I think this patch is beyond me, I tried working on 
it having considered the additional feedback, but it needs a bit more 
understanding of the code and I won't make it on time (I wanted to apply 
to Outreachy internships), so I'm very sorry for bothering you without 
reason.
Iwona Kotlarska