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
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 > >
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.