[PATCH v2 00/18] Make Pixman an optional dependency

[PATCH v2 00/18] Make Pixman an optional dependency

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

QEMU system emulators can be made to compile and work without Pixman.

Only a few devices and options actually require it (VNC, Gtk, Spice for ex) and
will have to be compiled out.

However, most of QEMU graphics-related code is based on pixman_image_t and
format. If we want to provide mostly compatible QEMU machines with or without
Pixman, all we need to do is to have a small compatibility header with just the
bare minimum for those types (see "ui: add pixman-compat.h"). There are a
limited number of operations related to geometry that are slightly better
implemented in QEMU (without Pixman, see "virtio-gpu: replace PIXMAN for
region/rect test").

Without this simple compatibility header approach, QEMU at runtime becomes a
very different emulator (without graphics device/board, display etc) and full of
"if PIXMAN" conditions in the code. This is a much worse outcome imho, compared
to this small header maintainance and compatibility story.

Fixes:
https://gitlab.com/qemu-project/qemu/-/issues/1172

Marc-André Lureau (18):
  build-sys: add a "pixman" feature
  ui: compile out some qemu-pixman functions when !PIXMAN
  ui: add pixman-compat.h
  ui/console: allow to override the default VC
  ui/vc: console-vc requires PIXMAN
  qmp/hmp: disable screendump if PIXMAN is missing
  virtio-gpu: replace PIXMAN for region/rect test
  ui/console: when PIXMAN is unavailable, don't draw placeholder msg
  vhost-user-gpu: skip VHOST_USER_GPU_UPDATE when !PIXMAN
  ui/gl: opengl doesn't require PIXMAN
  ui/vnc: VNC requires PIXMAN
  ui/spice: SPICE/QXL requires PIXMAN
  ui/gtk: -display gtk requires PIXMAN
  ui/dbus: do not require PIXMAN
  arm/kconfig: XLNX_ZYNQMP_ARM depends on PIXMAN
  hw/sm501: allow compiling without PIXMAN
  hw/display: make ATI_VGA depend on PIXMAN
  build-sys: make pixman actually optional

 configs/devices/mips64el-softmmu/default.mak |   2 +-
 meson.build                                  |  25 ++-
 qapi/ui.json                                 |   3 +-
 include/ui/console.h                         |   2 +
 include/ui/pixman-compat.h                   | 190 +++++++++++++++++++
 include/ui/qemu-pixman.h                     |  11 +-
 include/ui/rect.h                            |  55 ++++++
 hw/display/sm501.c                           |  19 +-
 hw/display/vhost-user-gpu.c                  |   2 +
 hw/display/virtio-gpu.c                      |  30 ++-
 softmmu/vl.c                                 |  45 +++--
 ui/console-vc-stubs.c                        |  32 ++++
 ui/console.c                                 |  20 ++
 ui/dbus-listener.c                           |  88 ++++++---
 ui/qemu-pixman.c                             |   6 +
 ui/ui-hmp-cmds.c                             |   2 +
 ui/ui-qmp-cmds.c                             |   2 +
 Kconfig.host                                 |   3 +
 hmp-commands.hx                              |   2 +
 hw/arm/Kconfig                               |   3 +-
 hw/display/Kconfig                           |   9 +-
 hw/display/meson.build                       |   4 +-
 meson_options.txt                            |   2 +
 scripts/meson-buildoptions.sh                |   3 +
 ui/meson.build                               |  22 +--
 25 files changed, 487 insertions(+), 95 deletions(-)
 create mode 100644 include/ui/pixman-compat.h
 create mode 100644 include/ui/rect.h
 create mode 100644 ui/console-vc-stubs.c

-- 
2.41.0

[PATCH v2 01/18] build-sys: add a "pixman" feature

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

For now, pixman is mandatory, but we set config_host.h and Kconfig.
Once compilation is fixed, "pixman" will become actually optional.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build                   | 10 ++++++++--
 include/ui/qemu-pixman.h      |  2 ++
 Kconfig.host                  |  3 +++
 meson_options.txt             |  2 ++
 scripts/meson-buildoptions.sh |  3 +++
 5 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 5150a74831..e870b039cc 100644
--- a/meson.build
+++ b/meson.build
@@ -828,10 +828,14 @@ if 'ust' in get_option('trace_backends')
                      method: 'pkg-config')
 endif
 pixman = not_found
-if have_system or have_tools
-  pixman = dependency('pixman-1', required: have_system, version:'>=0.21.8',
+if not get_option('pixman').auto() or have_system or have_tools
+  pixman = dependency('pixman-1', required: get_option('pixman'), version:'>=0.21.8',
                       method: 'pkg-config')
 endif
+if not pixman.found()
+  error('FIXME: pixman is currently required')
+endif
+
 zlib = dependency('zlib', required: true)
 
 libaio = not_found
@@ -2124,6 +2128,7 @@ config_host_data.set('CONFIG_SECCOMP', seccomp.found())
 if seccomp.found()
   config_host_data.set('CONFIG_SECCOMP_SYSRAWRC', seccomp_has_sysrawrc)
 endif
+config_host_data.set('CONFIG_PIXMAN', pixman.found())
 config_host_data.set('CONFIG_SNAPPY', snappy.found())
 config_host_data.set('CONFIG_SOLARIS', targetos == 'sunos')
 if get_option('tcg').allowed()
@@ -2843,6 +2848,7 @@ have_ivshmem = config_host_data.get('CONFIG_EVENTFD')
 host_kconfig = \
   (get_option('fuzzing') ? ['CONFIG_FUZZ=y'] : []) + \
   (have_tpm ? ['CONFIG_TPM=y'] : []) + \
+  (pixman.found() ? ['CONFIG_PIXMAN=y'] : []) + \
   (spice.found() ? ['CONFIG_SPICE=y'] : []) + \
   (have_ivshmem ? ['CONFIG_IVSHMEM=y'] : []) + \
   (opengl.found() ? ['CONFIG_OPENGL=y'] : []) + \
diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 51f8709327..b3379f6625 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -6,11 +6,13 @@
 #ifndef QEMU_PIXMAN_H
 #define QEMU_PIXMAN_H
 
+#ifdef CONFIG_PIXMAN
 /* pixman-0.16.0 headers have a redundant declaration */
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wredundant-decls"
 #include <pixman.h>
 #pragma GCC diagnostic pop
+#endif
 
 /*
  * pixman image formats are defined to be native endian,
diff --git a/Kconfig.host b/Kconfig.host
index d763d89269..b6ac2b9316 100644
--- a/Kconfig.host
+++ b/Kconfig.host
@@ -11,6 +11,9 @@ config OPENGL
 config X11
     bool
 
+config PIXMAN
+    bool
+
 config SPICE
     bool
 
diff --git a/meson_options.txt b/meson_options.txt
index f82d88b7c6..1aaec02d68 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -218,6 +218,8 @@ option('l2tpv3', type : 'feature', value : 'auto',
        description: 'l2tpv3 network backend support')
 option('netmap', type : 'feature', value : 'auto',
        description: 'netmap network backend support')
+option('pixman', type : 'feature', value : 'auto',
+       description: 'pixman support')
 option('slirp', type: 'feature', value: 'auto',
        description: 'libslirp user mode network backend support')
 option('vde', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index e1d178370c..d016caf819 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -147,6 +147,7 @@ meson_options_help() {
   printf "%s\n" '  pa              PulseAudio sound support'
   printf "%s\n" '  parallels       parallels image format support'
   printf "%s\n" '  pipewire        PipeWire sound support'
+  printf "%s\n" '  pixman          pixman support'
   printf "%s\n" '  png             PNG support with libpng'
   printf "%s\n" '  pvrdma          Enable PVRDMA support'
   printf "%s\n" '  qcow1           qcow1 image format support'
@@ -398,6 +399,8 @@ _meson_option_parse() {
     --disable-parallels) printf "%s" -Dparallels=disabled ;;
     --enable-pipewire) printf "%s" -Dpipewire=enabled ;;
     --disable-pipewire) printf "%s" -Dpipewire=disabled ;;
+    --enable-pixman) printf "%s" -Dpixman=enabled ;;
+    --disable-pixman) printf "%s" -Dpixman=disabled ;;
     --with-pkgversion=*) quote_sh "-Dpkgversion=$2" ;;
     --enable-plugins) printf "%s" -Dplugins=true ;;
     --disable-plugins) printf "%s" -Dplugins=false ;;
-- 
2.41.0

[PATCH v2 02/18] ui: compile out some qemu-pixman functions when !PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Those functions require the PIXMAN library.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/qemu-pixman.h | 7 +++++--
 ui/qemu-pixman.c         | 6 ++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index b3379f6625..539c089f1d 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -72,17 +72,17 @@ pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
 pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format);
 uint32_t qemu_pixman_to_drm_format(pixman_format_code_t pixman);
 int qemu_pixman_get_type(int rshift, int gshift, int bshift);
-pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
 bool qemu_pixman_check_format(DisplayChangeListener *dcl,
                               pixman_format_code_t format);
 
+#ifdef CONFIG_PIXMAN
+pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
 pixman_image_t *qemu_pixman_linebuf_create(pixman_format_code_t format,
                                            int width);
 void qemu_pixman_linebuf_fill(pixman_image_t *linebuf, pixman_image_t *fb,
                               int width, int x, int y);
 pixman_image_t *qemu_pixman_mirror_create(pixman_format_code_t format,
                                           pixman_image_t *image);
-void qemu_pixman_image_unref(pixman_image_t *image);
 
 pixman_image_t *qemu_pixman_glyph_from_vgafont(int height, const uint8_t *font,
                                                unsigned int ch);
@@ -91,6 +91,9 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
                               pixman_color_t *fgcol,
                               pixman_color_t *bgcol,
                               int x, int y, int cw, int ch);
+#endif
+
+void qemu_pixman_image_unref(pixman_image_t *image);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
 
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index be00a96340..4272a43744 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -143,6 +143,7 @@ int qemu_pixman_get_type(int rshift, int gshift, int bshift)
     return type;
 }
 
+#ifdef CONFIG_PIXMAN
 pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf)
 {
     pixman_format_code_t format;
@@ -156,6 +157,7 @@ pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf)
     }
     return format;
 }
+#endif
 
 /*
  * Return true for known-good pixman conversions.
@@ -184,6 +186,7 @@ bool qemu_pixman_check_format(DisplayChangeListener *dcl,
     }
 }
 
+#ifdef CONFIG_PIXMAN
 pixman_image_t *qemu_pixman_linebuf_create(pixman_format_code_t format,
                                            int width)
 {
@@ -209,6 +212,7 @@ pixman_image_t *qemu_pixman_mirror_create(pixman_format_code_t format,
                                     NULL,
                                     pixman_image_get_stride(image));
 }
+#endif
 
 void qemu_pixman_image_unref(pixman_image_t *image)
 {
@@ -218,6 +222,7 @@ void qemu_pixman_image_unref(pixman_image_t *image)
     pixman_image_unref(image);
 }
 
+#ifdef CONFIG_PIXMAN
 pixman_image_t *qemu_pixman_glyph_from_vgafont(int height, const uint8_t *font,
                                                unsigned int ch)
 {
@@ -260,3 +265,4 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
     pixman_image_unref(ifg);
     pixman_image_unref(ibg);
 }
+#endif /* CONFIG_PIXMAN */
-- 
2.41.0

[PATCH v2 03/18] ui: add pixman-compat.h

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This is a tiny subset of PIXMAN API that is used pervasively in QEMU
codebase to manage images and identify the underlying format.

It doesn't seems worth to wrap this in a QEMU-specific API.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/pixman-compat.h | 190 +++++++++++++++++++++++++++++++++++++
 include/ui/qemu-pixman.h   |   2 +
 2 files changed, 192 insertions(+)
 create mode 100644 include/ui/pixman-compat.h

diff --git a/include/ui/pixman-compat.h b/include/ui/pixman-compat.h
new file mode 100644
index 0000000000..e511c8b946
--- /dev/null
+++ b/include/ui/pixman-compat.h
@@ -0,0 +1,190 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Tiny subset of PIXMAN API commonly used by QEMU.
+ *
+ * Copyright 1987, 1988, 1989, 1998  The Open Group
+ * Copyright 1987, 1988, 1989 Digital Equipment Corporation
+ * Copyright 1999, 2004, 2008 Keith Packard
+ * Copyright 2000 SuSE, Inc.
+ * Copyright 2000 Keith Packard, member of The XFree86 Project, Inc.
+ * Copyright 2004, 2005, 2007, 2008, 2009, 2010 Red Hat, Inc.
+ * Copyright 2004 Nicholas Miell
+ * Copyright 2005 Lars Knoll & Zack Rusin, Trolltech
+ * Copyright 2005 Trolltech AS
+ * Copyright 2007 Luca Barbato
+ * Copyright 2008 Aaron Plattner, NVIDIA Corporation
+ * Copyright 2008 Rodrigo Kumpera
+ * Copyright 2008 André Tupinambá
+ * Copyright 2008 Mozilla Corporation
+ * Copyright 2008 Frederic Plourde
+ * Copyright 2009, Oracle and/or its affiliates. All rights reserved.
+ * Copyright 2009, 2010 Nokia Corporation
+ *
+ * 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 (including the next
+ * paragraph) 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.
+ */
+
+#ifndef PIXMAN_COMPAT_H
+#define PIXMAN_COMPAT_H
+
+#define PIXMAN_TYPE_OTHER       0
+#define PIXMAN_TYPE_ARGB        2
+#define PIXMAN_TYPE_ABGR        3
+#define PIXMAN_TYPE_BGRA        8
+#define PIXMAN_TYPE_RGBA        9
+
+#define PIXMAN_FORMAT(bpp,type,a,r,g,b) (((bpp) << 24) |  \
+                                         ((type) << 16) | \
+                                         ((a) << 12) |    \
+                                         ((r) << 8) |     \
+                                         ((g) << 4) |     \
+                                         ((b)))
+
+#define PIXMAN_FORMAT_RESHIFT(val, ofs, num)                            \
+        (((val >> (ofs)) & ((1 << (num)) - 1)) << ((val >> 22) & 3))
+
+#define PIXMAN_FORMAT_BPP(f)    PIXMAN_FORMAT_RESHIFT(f, 24, 8)
+#define PIXMAN_FORMAT_TYPE(f)   (((f) >> 16) & 0x3f)
+#define PIXMAN_FORMAT_A(f)      PIXMAN_FORMAT_RESHIFT(f, 12, 4)
+#define PIXMAN_FORMAT_R(f)      PIXMAN_FORMAT_RESHIFT(f, 8, 4)
+#define PIXMAN_FORMAT_G(f)      PIXMAN_FORMAT_RESHIFT(f, 4, 4)
+#define PIXMAN_FORMAT_B(f)      PIXMAN_FORMAT_RESHIFT(f, 0, 4)
+#define PIXMAN_FORMAT_DEPTH(f)  (PIXMAN_FORMAT_A(f) +   \
+                                 PIXMAN_FORMAT_R(f) +   \
+                                 PIXMAN_FORMAT_G(f) +   \
+                                 PIXMAN_FORMAT_B(f))
+
+typedef enum {
+    /* 32bpp formats */
+    PIXMAN_a8r8g8b8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_ARGB,8,8,8,8),
+    PIXMAN_x8r8g8b8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_ARGB,0,8,8,8),
+    PIXMAN_a8b8g8r8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_ABGR,8,8,8,8),
+    PIXMAN_x8b8g8r8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_ABGR,0,8,8,8),
+    PIXMAN_b8g8r8a8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_BGRA,8,8,8,8),
+    PIXMAN_b8g8r8x8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_BGRA,0,8,8,8),
+    PIXMAN_r8g8b8a8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_RGBA,8,8,8,8),
+    PIXMAN_r8g8b8x8 =    PIXMAN_FORMAT(32,PIXMAN_TYPE_RGBA,0,8,8,8),
+    /* 24bpp formats */
+    PIXMAN_r8g8b8 =      PIXMAN_FORMAT(24,PIXMAN_TYPE_ARGB,0,8,8,8),
+    PIXMAN_b8g8r8 =      PIXMAN_FORMAT(24,PIXMAN_TYPE_ABGR,0,8,8,8),
+    /* 16bpp formats */
+    PIXMAN_r5g6b5 =      PIXMAN_FORMAT(16,PIXMAN_TYPE_ARGB,0,5,6,5),
+    PIXMAN_a1r5g5b5 =    PIXMAN_FORMAT(16,PIXMAN_TYPE_ARGB,1,5,5,5),
+    PIXMAN_x1r5g5b5 =    PIXMAN_FORMAT(16,PIXMAN_TYPE_ARGB,0,5,5,5),
+} pixman_format_code_t;
+
+typedef struct pixman_image pixman_image_t;
+
+typedef void (* pixman_image_destroy_func_t)(pixman_image_t *image, void *data);
+
+struct pixman_image {
+    int ref_count;
+    pixman_format_code_t format;
+    int width;
+    int height;
+    int stride;
+    uint8_t *data;
+    pixman_image_destroy_func_t destroy_func;
+    void *destroy_data;
+};
+
+typedef struct pixman_color {
+    uint16_t    red;
+    uint16_t    green;
+    uint16_t    blue;
+    uint16_t    alpha;
+} pixman_color_t;
+
+static inline pixman_image_t *pixman_image_create_bits(pixman_format_code_t format,
+                                                       int width,
+                                                       int height,
+                                                       uint32_t *bits,
+                                                       int rowstride_bytes)
+{
+    pixman_image_t *i = g_new0(pixman_image_t, 1);
+
+    i->width = width;
+    i->height = height;
+    i->stride = rowstride_bytes ?: width * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
+    i->format = format;
+    i->data = bits ?: g_malloc0(rowstride_bytes * height);
+    i->ref_count = 1;
+
+    return i;
+}
+
+static inline pixman_image_t *pixman_image_ref(pixman_image_t *i)
+{
+    i->ref_count++;
+    return i;
+}
+
+static inline bool pixman_image_unref(pixman_image_t *i)
+{
+    i->ref_count--;
+
+    if (i->ref_count == 0) {
+        if (i->destroy_func) {
+            i->destroy_func (i, i->destroy_data);
+            g_free(i->data);
+            g_free(i);
+        }
+
+        return true;
+    }
+
+    return false;
+}
+
+static inline void pixman_image_set_destroy_function(pixman_image_t *i,
+                                                     pixman_image_destroy_func_t func,
+                                                     void *data)
+
+{
+    i->destroy_func = func;
+    i->destroy_data = data;
+}
+
+static inline uint8_t* pixman_image_get_data(pixman_image_t *i)
+{
+    return i->data;
+}
+
+static inline int pixman_image_get_height(pixman_image_t *i)
+{
+    return i->height;
+}
+
+static inline int pixman_image_get_width(pixman_image_t *i)
+{
+    return i->width;
+}
+
+static inline int pixman_image_get_stride(pixman_image_t *i)
+{
+    return i->stride;
+}
+
+static inline pixman_format_code_t pixman_image_get_format(pixman_image_t *i)
+{
+    return i->format;
+}
+
+#endif /* PIXMAN_COMPAT_H */
diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 539c089f1d..9c693df8dd 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -12,6 +12,8 @@
 #pragma GCC diagnostic ignored "-Wredundant-decls"
 #include <pixman.h>
 #pragma GCC diagnostic pop
+#else
+#include "pixman-compat.h"
 #endif
 
 /*
-- 
2.41.0

[PATCH v2 04/18] ui/console: allow to override the default VC

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

If a display is backed by a specialized VC, allow to override the
default "vc:80Cx24C". For that, set the dpy.type just before creating
the default serial/parallel/monitor.

(the next patch makes it create a "null" backend by default if !PIXMAN)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/console.h |  2 ++
 softmmu/vl.c         | 45 +++++++++++++++++++++++---------------------
 ui/console.c         | 14 ++++++++++++++
 3 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 28882f15a5..08c0f0dc70 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -463,12 +463,14 @@ struct QemuDisplay {
     DisplayType type;
     void (*early_init)(DisplayOptions *opts);
     void (*init)(DisplayState *ds, DisplayOptions *opts);
+    const char *vc;
 };
 
 void qemu_display_register(QemuDisplay *ui);
 bool qemu_display_find_default(DisplayOptions *opts);
 void qemu_display_early_init(DisplayOptions *opts);
 void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
+const char *qemu_display_get_vc(DisplayOptions *opts);
 void qemu_display_help(void);
 
 /* vnc.c */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3db4fd2680..8850cd3121 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1361,6 +1361,23 @@ static void qemu_create_default_devices(void)
         }
     }
 
+#if defined(CONFIG_VNC)
+    if (!QTAILQ_EMPTY(&(qemu_find_opts("vnc")->head))) {
+        display_remote++;
+    }
+#endif
+    if (dpy.type == DISPLAY_TYPE_DEFAULT && !display_remote) {
+        if (!qemu_display_find_default(&dpy)) {
+            dpy.type = DISPLAY_TYPE_NONE;
+#if defined(CONFIG_VNC)
+            vnc_parse("localhost:0,to=99,id=default");
+#endif
+        }
+    }
+    if (dpy.type == DISPLAY_TYPE_DEFAULT) {
+        dpy.type = DISPLAY_TYPE_NONE;
+    }
+
     if (nographic) {
         if (default_parallel)
             add_device_config(DEV_PARALLEL, "null");
@@ -1373,12 +1390,15 @@ static void qemu_create_default_devices(void)
                 monitor_parse("stdio", "readline", false);
         }
     } else {
-        if (default_serial)
-            add_device_config(DEV_SERIAL, "vc:80Cx24C");
+        const char *vc = qemu_display_get_vc(&dpy);
+
+        if (default_serial) {
+            add_device_config(DEV_SERIAL, vc);
+        }
         if (default_parallel)
-            add_device_config(DEV_PARALLEL, "vc:80Cx24C");
+            add_device_config(DEV_PARALLEL, vc);
         if (default_monitor)
-            monitor_parse("vc:80Cx24C", "readline", false);
+            monitor_parse(vc, "readline", false);
     }
 
     if (default_net) {
@@ -1389,23 +1409,6 @@ static void qemu_create_default_devices(void)
 #endif
     }
 
-#if defined(CONFIG_VNC)
-    if (!QTAILQ_EMPTY(&(qemu_find_opts("vnc")->head))) {
-        display_remote++;
-    }
-#endif
-    if (dpy.type == DISPLAY_TYPE_DEFAULT && !display_remote) {
-        if (!qemu_display_find_default(&dpy)) {
-            dpy.type = DISPLAY_TYPE_NONE;
-#if defined(CONFIG_VNC)
-            vnc_parse("localhost:0,to=99,id=default");
-#endif
-        }
-    }
-    if (dpy.type == DISPLAY_TYPE_DEFAULT) {
-        dpy.type = DISPLAY_TYPE_NONE;
-    }
-
     /* If no default VGA is requested, the default is "none".  */
     if (default_vga) {
         vga_model = get_default_vga_model(machine_class);
diff --git a/ui/console.c b/ui/console.c
index 4a4f19ed33..a38d24f075 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1677,6 +1677,20 @@ void qemu_display_init(DisplayState *ds, DisplayOptions *opts)
     dpys[opts->type]->init(ds, opts);
 }
 
+const char *qemu_display_get_vc(DisplayOptions *opts)
+{
+    assert(opts->type < DISPLAY_TYPE__MAX);
+    if (opts->type == DISPLAY_TYPE_NONE) {
+        return NULL;
+    }
+    assert(dpys[opts->type] != NULL);
+    if (dpys[opts->type]->vc) {
+        return dpys[opts->type]->vc;
+    } else {
+        return "vc:80Cx24C";
+    }
+}
+
 void qemu_display_help(void)
 {
     int idx;
-- 
2.41.0

[PATCH v2 05/18] ui/vc: console-vc requires PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add stubs for the fallback paths.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console-vc-stubs.c | 32 ++++++++++++++++++++++++++++++++
 ui/console.c          |  4 ++++
 ui/meson.build        |  2 +-
 3 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 ui/console-vc-stubs.c

diff --git a/ui/console-vc-stubs.c b/ui/console-vc-stubs.c
new file mode 100644
index 0000000000..0827961b2b
--- /dev/null
+++ b/ui/console-vc-stubs.c
@@ -0,0 +1,32 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * QEMU VC stubs
+ */
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/option.h"
+#include "chardev/char.h"
+#include "ui/console-priv.h"
+
+void qemu_text_console_select(QemuTextConsole *c)
+{
+}
+
+const char * qemu_text_console_get_label(QemuTextConsole *c)
+{
+    return NULL;
+}
+
+void qemu_text_console_update_cursor(void)
+{
+}
+
+void qemu_text_console_handle_keysym(QemuTextConsole *s, int keysym)
+{
+}
+
+void qemu_console_early_init(void)
+{
+}
diff --git a/ui/console.c b/ui/console.c
index a38d24f075..8b5c40ddd7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1687,7 +1687,11 @@ const char *qemu_display_get_vc(DisplayOptions *opts)
     if (dpys[opts->type]->vc) {
         return dpys[opts->type]->vc;
     } else {
+#ifdef CONFIG_PIXMAN
         return "vc:80Cx24C";
+#else
+        return "null";
+#endif
     }
 }
 
diff --git a/ui/meson.build b/ui/meson.build
index 0a1e8272a3..3085e10a72 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -6,7 +6,6 @@ system_ss.add(png)
 system_ss.add(files(
   'clipboard.c',
   'console.c',
-  'console-vc.c',
   'cursor.c',
   'input-keymap.c',
   'input-legacy.c',
@@ -19,6 +18,7 @@ system_ss.add(files(
   'ui-qmp-cmds.c',
   'util.c',
 ))
+system_ss.add(when: pixman, if_true: files('console-vc.c'), if_false: files('console-vc-stubs.c'))
 if dbus_display
   system_ss.add(files('dbus-module.c'))
 endif
-- 
2.41.0

[PATCH v2 06/18] qmp/hmp: disable screendump if PIXMAN is missing

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The command requires color conversion and line-by-line feeding. We could
have a simple fallback for simple formats though.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/ui.json     | 3 ++-
 ui/ui-hmp-cmds.c | 2 ++
 ui/ui-qmp-cmds.c | 2 ++
 hmp-commands.hx  | 2 ++
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 006616aa77..e74cc3efb6 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -200,7 +200,8 @@
 { 'command': 'screendump',
   'data': {'filename': 'str', '*device': 'str', '*head': 'int',
            '*format': 'ImageFormat'},
-  'coroutine': true }
+  'coroutine': true,
+  'if': 'CONFIG_PIXMAN' }
 
 ##
 # == Spice
diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
index c671389473..26c8ced1f2 100644
--- a/ui/ui-hmp-cmds.c
+++ b/ui/ui-hmp-cmds.c
@@ -437,6 +437,7 @@ void sendkey_completion(ReadLineState *rs, int nb_args, const char *str)
     }
 }
 
+#ifdef CONFIG_PIXMAN
 void coroutine_fn
 hmp_screendump(Monitor *mon, const QDict *qdict)
 {
@@ -458,6 +459,7 @@ hmp_screendump(Monitor *mon, const QDict *qdict)
 end:
     hmp_handle_error(mon, err);
 }
+#endif
 
 void hmp_client_migrate_info(Monitor *mon, const QDict *qdict)
 {
diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
index debc07d678..d772e1cb7f 100644
--- a/ui/ui-qmp-cmds.c
+++ b/ui/ui-qmp-cmds.c
@@ -212,6 +212,7 @@ void qmp_client_migrate_info(const char *protocol, const char *hostname,
     error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", "'spice'");
 }
 
+#ifdef CONFIG_PIXMAN
 #ifdef CONFIG_PNG
 /**
  * png_save: Take a screenshot as PNG
@@ -391,3 +392,4 @@ qmp_screendump(const char *filename, const char *device,
         }
     }
 }
+#endif /* CONFIG_PIXMAN */
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2cbd0f77a0..bfb2e2d5e2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -252,6 +252,7 @@ SRST
 
 ERST
 
+#ifdef CONFIG_PIXMAN
     {
         .name       = "screendump",
         .args_type  = "filename:F,format:-fs,device:s?,head:i?",
@@ -267,6 +268,7 @@ SRST
 ``screendump`` *filename*
   Save screen into PPM image *filename*.
 ERST
+#endif
 
     {
         .name       = "logfile",
-- 
2.41.0

[PATCH v2 07/18] virtio-gpu: replace PIXMAN for region/rect test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Use a simpler implementation for rectangle geometry & intersect, drop
the need for (more complex) PIXMAN functions.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/rect.h       | 55 +++++++++++++++++++++++++++++++++++++++++
 hw/display/virtio-gpu.c | 30 +++++++++-------------
 2 files changed, 66 insertions(+), 19 deletions(-)
 create mode 100644 include/ui/rect.h

diff --git a/include/ui/rect.h b/include/ui/rect.h
new file mode 100644
index 0000000000..63edf7a39d
--- /dev/null
+++ b/include/ui/rect.h
@@ -0,0 +1,55 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef QEMU_RECT_H
+#define QEMU_RECT_H
+
+#include <stdint.h>
+#include <stdbool.h>
+
+typedef struct QemuRect {
+    int16_t x;
+    int16_t y;
+    uint16_t width;
+    uint16_t height;
+} QemuRect;
+
+static inline void qemu_rect_init(QemuRect* rect, int16_t x, int16_t y, uint16_t width, uint16_t height)
+{
+    rect->x = x;
+    rect->y = x;
+    rect->width = width;
+    rect->height = height;
+}
+
+static inline void qemu_rect_translate(QemuRect* rect, int16_t dx, int16_t dy)
+{
+    rect->x += dx;
+    rect->y += dy;
+}
+
+static inline bool qemu_rect_intersect(const QemuRect* a, const QemuRect* b, QemuRect *res)
+{
+    int16_t x1, x2, y1, y2;
+
+    x1 = MAX(a->x, b->x);
+    y1 = MAX(a->y, b->y);
+    x2 = MIN(a->x + a->width, b->x + b->width);
+    y2 = MIN(a->y + a->height, b->y + b->height);
+
+    if (x1 >= x2 || y1 >= y2) {
+        if (res) {
+            qemu_rect_init(res, 0, 0, 0, 0);
+        }
+
+        return false;
+    }
+
+    if (res) {
+        qemu_rect_init(res, x1, y1, x2 - x1, y2 - y1);
+    }
+
+    return true;
+}
+
+#endif
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 93857ad523..fdb7b25a70 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -16,6 +16,7 @@
 #include "qemu/iov.h"
 #include "sysemu/cpus.h"
 #include "ui/console.h"
+#include "ui/rect.h"
 #include "trace.h"
 #include "sysemu/dma.h"
 #include "sysemu/sysemu.h"
@@ -507,7 +508,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
     struct virtio_gpu_simple_resource *res;
     struct virtio_gpu_resource_flush rf;
     struct virtio_gpu_scanout *scanout;
-    pixman_region16_t flush_region;
+    QemuRect flush_rect;
     bool within_bounds = false;
     bool update_submitted = false;
     int i;
@@ -569,34 +570,25 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
         return;
     }
 
-    pixman_region_init_rect(&flush_region,
-                            rf.r.x, rf.r.y, rf.r.width, rf.r.height);
+    qemu_rect_init(&flush_rect, rf.r.x, rf.r.y, rf.r.width, rf.r.height);
     for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
-        pixman_region16_t region, finalregion;
-        pixman_box16_t *extents;
+        QemuRect rect;
 
         if (!(res->scanout_bitmask & (1 << i))) {
             continue;
         }
         scanout = &g->parent_obj.scanout[i];
 
-        pixman_region_init(&finalregion);
-        pixman_region_init_rect(&region, scanout->x, scanout->y,
-                                scanout->width, scanout->height);
+        qemu_rect_init(&rect, scanout->x, scanout->y,
+                       scanout->width, scanout->height);
 
-        pixman_region_intersect(&finalregion, &flush_region, &region);
-        pixman_region_translate(&finalregion, -scanout->x, -scanout->y);
-        extents = pixman_region_extents(&finalregion);
         /* work out the area we need to update for each console */
-        dpy_gfx_update(g->parent_obj.scanout[i].con,
-                       extents->x1, extents->y1,
-                       extents->x2 - extents->x1,
-                       extents->y2 - extents->y1);
-
-        pixman_region_fini(&region);
-        pixman_region_fini(&finalregion);
+        if (qemu_rect_intersect(&flush_rect, &rect, &rect)) {
+            qemu_rect_translate(&rect, -scanout->x, -scanout->y);
+            dpy_gfx_update(g->parent_obj.scanout[i].con,
+                           rect.x, rect.y, rect.width, rect.height);
+        }
     }
-    pixman_region_fini(&flush_region);
 }
 
 static void virtio_unref_resource(pixman_image_t *image, void *data)
-- 
2.41.0

[PATCH v2 08/18] ui/console: when PIXMAN is unavailable, don't draw placeholder msg

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

When we can't draw text, simply show a blank display.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ui/console.c b/ui/console.c
index 8b5c40ddd7..1c710a6d5e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -584,6 +584,7 @@ DisplaySurface *qemu_create_placeholder_surface(int w, int h,
                                                 const char *msg)
 {
     DisplaySurface *surface = qemu_create_displaysurface(w, h);
+#ifdef CONFIG_PIXMAN
     pixman_color_t bg = QEMU_PIXMAN_COLOR_BLACK;
     pixman_color_t fg = QEMU_PIXMAN_COLOR_GRAY;
     pixman_image_t *glyph;
@@ -598,6 +599,7 @@ DisplaySurface *qemu_create_placeholder_surface(int w, int h,
                                  x+i, y, FONT_WIDTH, FONT_HEIGHT);
         qemu_pixman_image_unref(glyph);
     }
+#endif
     surface->flags |= QEMU_PLACEHOLDER_FLAG;
     return surface;
 }
-- 
2.41.0

[PATCH v2 09/18] vhost-user-gpu: skip VHOST_USER_GPU_UPDATE when !PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This simply means that 2d drawing updates won't be handled, but 3d
should work.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/vhost-user-gpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 1150521d9d..709c8a02a1 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -307,6 +307,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
         dpy_gl_update(con, m->x, m->y, m->width, m->height);
         break;
     }
+#ifdef CONFIG_PIXMAN
     case VHOST_USER_GPU_UPDATE: {
         VhostUserGpuUpdate *m = &msg->payload.update;
 
@@ -334,6 +335,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
         }
         break;
     }
+#endif
     default:
         g_warning("unhandled message %d %d", msg->request, msg->size);
     }
-- 
2.41.0

[PATCH v2 10/18] ui/gl: opengl doesn't require PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The QEMU fallback covers the requirements. We still need the flags of
header inclusion with CONFIG_PIXMAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/meson.build | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/meson.build b/ui/meson.build
index 3085e10a72..7c99613950 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -60,8 +60,8 @@ endif
 system_ss.add(opengl)
 if opengl.found()
   opengl_ss = ss.source_set()
-  opengl_ss.add(gbm)
-  opengl_ss.add(when: [opengl, pixman],
+  opengl_ss.add(gbm, pixman)
+  opengl_ss.add(when: [opengl],
                if_true: files('shader.c', 'console-gl.c', 'egl-helpers.c', 'egl-context.c'))
   ui_modules += {'opengl' : opengl_ss}
 endif
-- 
2.41.0

[PATCH v2 11/18] ui/vnc: VNC requires PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build    | 6 +++++-
 ui/meson.build | 4 ++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index e870b039cc..8106ab39ac 100644
--- a/meson.build
+++ b/meson.build
@@ -1545,7 +1545,11 @@ endif
 vnc = not_found
 jpeg = not_found
 sasl = not_found
-if get_option('vnc').allowed() and have_system
+if get_option('vnc') \
+             .disable_auto_if(not have_system) \
+             .require(pixman.found(),
+                      error_message: 'cannot enable VNC if pixman is not available') \
+             .allowed()
   vnc = declare_dependency() # dummy dependency
   jpeg = dependency('libjpeg', required: get_option('vnc_jpeg'),
                     method: 'pkg-config')
diff --git a/ui/meson.build b/ui/meson.build
index 7c99613950..b3525ef064 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -46,8 +46,8 @@ vnc_ss.add(files(
 ))
 vnc_ss.add(zlib, jpeg, gnutls)
 vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
-system_ss.add_all(when: vnc, if_true: vnc_ss)
-system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
+system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
+system_ss.add(when: [vnc, pixman], if_false: files('vnc-stubs.c'))
 
 ui_modules = {}
 
-- 
2.41.0

[PATCH v2 12/18] ui/spice: SPICE/QXL requires PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build        |  6 +++++-
 hw/display/Kconfig |  2 +-
 ui/meson.build     | 10 +++++-----
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index 8106ab39ac..1ef57ebda2 100644
--- a/meson.build
+++ b/meson.build
@@ -1030,7 +1030,11 @@ if not get_option('spice_protocol').auto() or have_system
                               method: 'pkg-config')
 endif
 spice = not_found
-if not get_option('spice').auto() or have_system
+if get_option('spice') \
+             .disable_auto_if(not have_system) \
+             .require(pixman.found(),
+                      error_message: 'cannot enable SPICE if pixman is not available') \
+             .allowed()
   spice = dependency('spice-server', version: '>=0.14.0',
                      required: get_option('spice'),
                      method: 'pkg-config')
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 7b3da68d1c..4d8b0cec40 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -93,7 +93,7 @@ config VGA
 
 config QXL
     bool
-    depends on SPICE && PCI
+    depends on SPICE && PCI && PIXMAN
     select VGA
 
 config VIRTIO_GPU
diff --git a/ui/meson.build b/ui/meson.build
index b3525ef064..7f806a6d48 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -141,12 +141,12 @@ if spice.found()
     'spice-display.c'
   ))
   ui_modules += {'spice-core' : spice_core_ss}
-endif
 
-if spice.found() and gio.found()
-  spice_ss = ss.source_set()
-  spice_ss.add(spice, gio, pixman, files('spice-app.c'))
-  ui_modules += {'spice-app': spice_ss}
+  if gio.found()
+    spice_ss = ss.source_set()
+    spice_ss.add(spice, gio, pixman, files('spice-app.c'))
+    ui_modules += {'spice-app': spice_ss}
+  endif
 endif
 
 keymaps = [
-- 
2.41.0

[PATCH v2 13/18] ui/gtk: -display gtk requires PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 1ef57ebda2..74bdfa92b7 100644
--- a/meson.build
+++ b/meson.build
@@ -1516,7 +1516,11 @@ gtkx11 = not_found
 vte = not_found
 have_gtk_clipboard = get_option('gtk_clipboard').enabled()
 
-if not get_option('gtk').auto() or have_system
+if get_option('gtk') \
+             .disable_auto_if(not have_system) \
+             .require(pixman.found(),
+                      error_message: 'cannot enable GTK if pixman is not available') \
+             .allowed()
   gtk = dependency('gtk+-3.0', version: '>=3.22.0',
                    method: 'pkg-config',
                    required: get_option('gtk'))
-- 
2.41.0

[PATCH v2 14/18] ui/dbus: do not require PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Implement a fallback path for region 2D update.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 88 ++++++++++++++++++++++++++++++++--------------
 ui/meson.build     |  2 +-
 2 files changed, 62 insertions(+), 28 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 36548a7f52..350988c70d 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -26,9 +26,6 @@
 #include "qapi/error.h"
 #include "sysemu/sysemu.h"
 #include "dbus.h"
-#ifdef CONFIG_OPENGL
-#include <pixman.h>
-#endif
 #ifdef G_OS_UNIX
 #include <gio/gunixfdlist.h>
 #endif
@@ -41,6 +38,7 @@
 #include "ui/shader.h"
 #include "ui/egl-helpers.h"
 #include "ui/egl-context.h"
+#include "ui/qemu-pixman.h"
 #endif
 #include "trace.h"
 
@@ -62,9 +60,11 @@ struct _DBusDisplayListener {
 
     QemuDBusDisplay1Listener *proxy;
 
-#ifdef CONFIG_OPENGL
+#ifdef CONFIG_PIXMAN
     /* Keep track of the damage region */
     pixman_region32_t gl_damage;
+#else
+    int gl_damage;
 #endif
 
     DisplayChangeListener dcl;
@@ -545,6 +545,7 @@ static void dbus_gl_refresh(DisplayChangeListener *dcl)
         return;
     }
 
+#ifdef CONFIG_PIXMAN
     int n_rects = pixman_region32_n_rects(&ddl->gl_damage);
 
     for (int i = 0; i < n_rects; i++) {
@@ -555,6 +556,13 @@ static void dbus_gl_refresh(DisplayChangeListener *dcl)
                             box->x2 - box->x1, box->y2 - box->y1);
     }
     pixman_region32_clear(&ddl->gl_damage);
+#else
+    if (ddl->gl_damage) {
+        dbus_call_update_gl(dcl, 0, 0,
+                            surface_width(ddl->ds), surface_height(ddl->ds));
+        ddl->gl_damage = 0;
+    }
+#endif
 }
 #endif /* OPENGL */
 
@@ -569,20 +577,62 @@ static void dbus_gl_gfx_update(DisplayChangeListener *dcl,
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
 
+#ifdef CONFIG_PIXMAN
     pixman_region32_t rect_region;
     pixman_region32_init_rect(&rect_region, x, y, w, h);
     pixman_region32_union(&ddl->gl_damage, &ddl->gl_damage, &rect_region);
     pixman_region32_fini(&rect_region);
+#else
+    ddl->gl_damage++;
+#endif
 }
 #endif
 
+static void dbus_gfx_update_sub(DBusDisplayListener *ddl,
+                                int x, int y, int w, int h)
+{
+    pixman_image_t *img;
+    size_t stride;
+    GVariant *v_data;
+
+    /* make a copy, since gvariant only handles linear data */
+    stride = w * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(surface_format(ddl->ds)), 8);
+    img = pixman_image_create_bits(surface_format(ddl->ds),
+                                   w, h, NULL, stride);
+#ifdef CONFIG_PIXMAN
+    pixman_image_composite(PIXMAN_OP_SRC, ddl->ds->image, NULL, img,
+                           x, y, 0, 0, 0, 0, w, h);
+#else
+    {
+        uint8_t *src = pixman_image_get_data(ddl->ds->image);
+        uint8_t *dst = pixman_image_get_data(img);
+        int bp = PIXMAN_FORMAT_BPP(surface_format(ddl->ds)) / 8;
+        int hh;
+
+        for (hh = 0; hh < h; hh++) {
+            memcpy(&dst[stride * hh], &src[surface_stride(ddl->ds) * (hh + y) + x * bp], stride);
+        }
+    }
+#endif
+    v_data = g_variant_new_from_data(
+        G_VARIANT_TYPE("ay"),
+        pixman_image_get_data(img),
+        pixman_image_get_stride(img) * h,
+        TRUE,
+        (GDestroyNotify)pixman_image_unref,
+        img);
+    qemu_dbus_display1_listener_call_update(ddl->proxy,
+        x, y, w, h, pixman_image_get_stride(img), pixman_image_get_format(img),
+        v_data,
+        G_DBUS_CALL_FLAGS_NONE,
+        DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+}
+
 static void dbus_gfx_update(DisplayChangeListener *dcl,
                             int x, int y, int w, int h)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
-    pixman_image_t *img;
     GVariant *v_data;
-    size_t stride;
 
     assert(ddl->ds);
 
@@ -619,25 +669,7 @@ static void dbus_gfx_update(DisplayChangeListener *dcl,
         return;
     }
 
-    /* make a copy, since gvariant only handles linear data */
-    stride = w * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(surface_format(ddl->ds)), 8);
-    img = pixman_image_create_bits(surface_format(ddl->ds),
-                                   w, h, NULL, stride);
-    pixman_image_composite(PIXMAN_OP_SRC, ddl->ds->image, NULL, img,
-                           x, y, 0, 0, 0, 0, w, h);
-
-    v_data = g_variant_new_from_data(
-        G_VARIANT_TYPE("ay"),
-        pixman_image_get_data(img),
-        pixman_image_get_stride(img) * h,
-        TRUE,
-        (GDestroyNotify)pixman_image_unref,
-        img);
-    qemu_dbus_display1_listener_call_update(ddl->proxy,
-        x, y, w, h, pixman_image_get_stride(img), pixman_image_get_format(img),
-        v_data,
-        G_DBUS_CALL_FLAGS_NONE,
-        DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+    dbus_gfx_update_sub(ddl, x, y, w, h);
 }
 
 #ifdef CONFIG_OPENGL
@@ -751,8 +783,10 @@ dbus_display_listener_dispose(GObject *object)
     g_clear_object(&ddl->map_proxy);
     g_clear_object(&ddl->d3d11_proxy);
     g_clear_pointer(&ddl->peer_process, CloseHandle);
-#ifdef CONFIG_OPENGL
+#ifdef CONFIG_PIXMAN
     pixman_region32_fini(&ddl->gl_damage);
+#endif
+#ifdef CONFIG_OPENGL
     egl_fb_destroy(&ddl->fb);
 #endif
 #endif
@@ -787,7 +821,7 @@ dbus_display_listener_class_init(DBusDisplayListenerClass *klass)
 static void
 dbus_display_listener_init(DBusDisplayListener *ddl)
 {
-#ifdef CONFIG_OPENGL
+#ifdef CONFIG_PIXMAN
     pixman_region32_init(&ddl->gl_damage);
 #endif
 }
diff --git a/ui/meson.build b/ui/meson.build
index 7f806a6d48..521843c356 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -93,7 +93,7 @@ if dbus_display
                                           '--generate-c-code', '@BASENAME@'])
   dbus_display1_lib = static_library('dbus-display1', dbus_display1, dependencies: gio)
   dbus_display1_dep = declare_dependency(link_with: dbus_display1_lib, include_directories: include_directories('.'))
-  dbus_ss.add(when: [gio, pixman, dbus_display1_dep],
+  dbus_ss.add(when: [gio, dbus_display1_dep],
               if_true: [files(
                 'dbus-chardev.c',
                 'dbus-clipboard.c',
-- 
2.41.0

[PATCH v2 15/18] arm/kconfig: XLNX_ZYNQMP_ARM depends on PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The Display Port has some strong PIXMAN dependency.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/arm/Kconfig         | 3 ++-
 hw/display/Kconfig     | 5 +++++
 hw/display/meson.build | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 7e68348440..57178bc299 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -451,7 +451,7 @@ config STM32F405_SOC
 config XLNX_ZYNQMP_ARM
     bool
     default y
-    depends on TCG && AARCH64
+    depends on TCG && AARCH64 && PIXMAN
     select AHCI
     select ARM_GIC
     select CADENCE
@@ -463,6 +463,7 @@ config XLNX_ZYNQMP_ARM
     select XILINX_AXI
     select XILINX_SPIPS
     select XLNX_CSU_DMA
+    select XLNX_DISPLAYPORT
     select XLNX_ZYNQMP
     select XLNX_ZDMA
     select USB_DWC3
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 4d8b0cec40..1aafe1923d 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -134,3 +134,8 @@ config MACFB
     bool
     select FRAMEBUFFER
     depends on NUBUS
+
+config XLNX_DISPLAYPORT
+    bool
+    # defaults to "N", enabled by specific boards
+    depends on PIXMAN
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 413ba4ab24..a7166b29d5 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -57,7 +57,7 @@ if config_all_devices.has_key('CONFIG_QXL')
 endif
 
 system_ss.add(when: 'CONFIG_DPCD', if_true: files('dpcd.c'))
-system_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true: files('xlnx_dp.c'))
+system_ss.add(when: 'CONFIG_XLNX_DISPLAYPORT', if_true: files('xlnx_dp.c'))
 
 system_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
 
-- 
2.41.0

[PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Drop the "x-pixman" property and use fallback path in such case.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/sm501.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 0eecd00701..a897c82f04 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
     switch (cmd) {
     case 0: /* BitBlt */
     {
-        static uint32_t tmp_buf[16384];
         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
         unsigned int src_y = s->twoD_source & 0xFFFF;
         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
@@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
                 de = db + (width + (height - 1) * dst_pitch) * bypp;
                 overlap = (db < se && sb < de);
             }
+#ifdef CONFIG_PIXMAN
             if (overlap && (s->use_pixman & BIT(2))) {
                 /* pixman can't do reverse blit: copy via temporary */
                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
+                static uint32_t tmp_buf[16384];
                 uint32_t *tmp = tmp_buf;
 
                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
@@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
                                        dst_pitch * bypp / sizeof(uint32_t),
                                        8 * bypp, 8 * bypp, src_x, src_y,
                                        dst_x, dst_y, width, height);
-            } else {
+            } else
+#else
+            {
                 fallback = true;
             }
+#endif
             if (fallback) {
                 uint8_t *sp = s->local_mem + src_base;
                 uint8_t *d = s->local_mem + dst_base;
@@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
             color = cpu_to_le16(color);
         }
 
+#ifdef CONFIG_PIXMAN
         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
-                         dst_x, dst_y, width, height, color)) {
+                         dst_x, dst_y, width, height, color))
+#endif
+        {
             /* fallback when pixman failed or we don't want to call it */
             uint8_t *d = s->local_mem + dst_base;
             unsigned int x, y, i;
@@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
 
 static Property sm501_sysbus_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
+#ifdef CONFIG_PIXMAN
     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2126,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
 
 static Property sm501_pci_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
+#ifdef CONFIG_PIXMAN
     DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2169,8 +2180,10 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
 
 static void sm501_pci_init(Object *o)
 {
+#ifdef CONFIG_PIXMAN
     object_property_set_description(o, "x-pixman", "Use pixman for: "
                                     "1: fill, 2: blit, 4: overlap blit");
+#endif
 }
 
 static const TypeInfo sm501_pci_info = {
-- 
2.41.0

[PATCH v2 17/18] hw/display: make ATI_VGA depend on PIXMAN

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configs/devices/mips64el-softmmu/default.mak | 2 +-
 hw/display/Kconfig                           | 2 +-
 hw/display/meson.build                       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/configs/devices/mips64el-softmmu/default.mak b/configs/devices/mips64el-softmmu/default.mak
index d5188f7ea5..8d85607571 100644
--- a/configs/devices/mips64el-softmmu/default.mak
+++ b/configs/devices/mips64el-softmmu/default.mak
@@ -3,7 +3,7 @@
 include ../mips-softmmu/common.mak
 CONFIG_FULOONG=y
 CONFIG_LOONGSON3V=y
-CONFIG_ATI_VGA=y
+# CONFIG_ATI_VGA=n
 CONFIG_RTL8139_PCI=y
 CONFIG_JAZZ=y
 CONFIG_VT82C686=y
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 1aafe1923d..4d8a6c4af8 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -125,7 +125,7 @@ config DPCD
 config ATI_VGA
     bool
     default y if PCI_DEVICES
-    depends on PCI
+    depends on PCI && PIXMAN
     select VGA
     select BITBANG_I2C
     select DDC
diff --git a/hw/display/meson.build b/hw/display/meson.build
index a7166b29d5..3aabf10226 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -61,7 +61,7 @@ system_ss.add(when: 'CONFIG_XLNX_DISPLAYPORT', if_true: files('xlnx_dp.c'))
 
 system_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
 
-system_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: files('ati.c', 'ati_2d.c', 'ati_dbg.c'))
+system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 'ati_dbg.c'), pixman])
 
 
 if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
-- 
2.41.0

[PATCH v2 18/18] build-sys: make pixman actually optional

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/meson.build b/meson.build
index 74bdfa92b7..bcf7424440 100644
--- a/meson.build
+++ b/meson.build
@@ -832,9 +832,6 @@ if not get_option('pixman').auto() or have_system or have_tools
   pixman = dependency('pixman-1', required: get_option('pixman'), version:'>=0.21.8',
                       method: 'pkg-config')
 endif
-if not pixman.found()
-  error('FIXME: pixman is currently required')
-endif
 
 zlib = dependency('zlib', required: true)
 
-- 
2.41.0

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[BALATON Zoltan]

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

On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Drop the "x-pixman" property and use fallback path in such case.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/display/sm501.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 0eecd00701..a897c82f04 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
>     switch (cmd) {
>     case 0: /* BitBlt */
>     {
> -        static uint32_t tmp_buf[16384];
>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
>         unsigned int src_y = s->twoD_source & 0xFFFF;
>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>                 overlap = (db < se && sb < de);
>             }
> +#ifdef CONFIG_PIXMAN
>             if (overlap && (s->use_pixman & BIT(2))) {
>                 /* pixman can't do reverse blit: copy via temporary */
>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
> +                static uint32_t tmp_buf[16384];
>                 uint32_t *tmp = tmp_buf;
>
>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
> @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
>                                        dst_pitch * bypp / sizeof(uint32_t),
>                                        8 * bypp, 8 * bypp, src_x, src_y,
>                                        dst_x, dst_y, width, height);
> -            } else {
> +            } else
> +#else
> +            {
>                 fallback = true;
>             }
> +#endif
>             if (fallback) {
>                 uint8_t *sp = s->local_mem + src_base;
>                 uint8_t *d = s->local_mem + dst_base;
> @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
>             color = cpu_to_le16(color);
>         }
>
> +#ifdef CONFIG_PIXMAN
>         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
> -                         dst_x, dst_y, width, height, color)) {
> +                         dst_x, dst_y, width, height, color))
> +#endif
> +        {
>             /* fallback when pixman failed or we don't want to call it */
>             uint8_t *d = s->local_mem + dst_base;
>             unsigned int x, y, i;
> @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>
> static Property sm501_sysbus_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> +#ifdef CONFIG_PIXMAN
>     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
> +#endif

Do we want to omit the property when compiled without pixman? I think we 
could leave it there and it would just be ignored without pixman but the 
same command line would still work and need less ifdefs in code.

Otherwise:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2126,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>
> static Property sm501_pci_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> +#ifdef CONFIG_PIXMAN
>     DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
> +#endif
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2169,8 +2180,10 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
>
> static void sm501_pci_init(Object *o)
> {
> +#ifdef CONFIG_PIXMAN
>     object_property_set_description(o, "x-pixman", "Use pixman for: "
>                                     "1: fill, 2: blit, 4: overlap blit");
> +#endif
> }
>
> static const TypeInfo sm501_pci_info = {
>

Re: [PATCH v2 01/18] build-sys: add a "pixman" feature

[Paolo Bonzini]

On 9/18/23 15:51, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> For now, pixman is mandatory, but we set config_host.h and Kconfig.
> Once compilation is fixed, "pixman" will become actually optional.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   meson.build                   | 10 ++++++++--
>   include/ui/qemu-pixman.h      |  2 ++
>   Kconfig.host                  |  3 +++
>   meson_options.txt             |  2 ++
>   scripts/meson-buildoptions.sh |  3 +++
>   5 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 5150a74831..e870b039cc 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -828,10 +828,14 @@ if 'ust' in get_option('trace_backends')
>                        method: 'pkg-config')
>   endif
>   pixman = not_found
> -if have_system or have_tools
> -  pixman = dependency('pixman-1', required: have_system, version:'>=0.21.8',
> +if not get_option('pixman').auto() or have_system or have_tools
> +  pixman = dependency('pixman-1', required: get_option('pixman'), version:'>=0.21.8',
>                         method: 'pkg-config')

I wonder if we should _anyway_ default to "enabled" if system emulators are
enabled, even after the series.  That would be

if not get_option('pixman').auto() or have_system or have_tools
   pixman = dependency('pixman-1',
       required: have_system or have_tools ? true : get_option('pixman'),
       version:'>=0.21.8',
       method: 'pkg-config')
endif

(There is also .enable_auto_if() but that requires Meson 1.1).

>   endif
> +if not pixman.found()
> +  error('FIXME: pixman is currently required')
> +endif

This would be subsumed by the above "required" argument.

Even if we don't want to require pixman if system emulators are enabled,
this should be "if not pixman.found() and (have_system or have_tools)"
otherwise compilation with --disable-system would require pixman until
the end of this series.

No need for a v3 to fix the above.

Paolo

> +
>   zlib = dependency('zlib', required: true)
>   
>   libaio = not_found
> @@ -2124,6 +2128,7 @@ config_host_data.set('CONFIG_SECCOMP', seccomp.found())
>   if seccomp.found()
>     config_host_data.set('CONFIG_SECCOMP_SYSRAWRC', seccomp_has_sysrawrc)
>   endif
> +config_host_data.set('CONFIG_PIXMAN', pixman.found())
>   config_host_data.set('CONFIG_SNAPPY', snappy.found())
>   config_host_data.set('CONFIG_SOLARIS', targetos == 'sunos')
>   if get_option('tcg').allowed()
> @@ -2843,6 +2848,7 @@ have_ivshmem = config_host_data.get('CONFIG_EVENTFD')
>   host_kconfig = \
>     (get_option('fuzzing') ? ['CONFIG_FUZZ=y'] : []) + \
>     (have_tpm ? ['CONFIG_TPM=y'] : []) + \
> +  (pixman.found() ? ['CONFIG_PIXMAN=y'] : []) + \
>     (spice.found() ? ['CONFIG_SPICE=y'] : []) + \
>     (have_ivshmem ? ['CONFIG_IVSHMEM=y'] : []) + \
>     (opengl.found() ? ['CONFIG_OPENGL=y'] : []) + \
> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> index 51f8709327..b3379f6625 100644
> --- a/include/ui/qemu-pixman.h
> +++ b/include/ui/qemu-pixman.h
> @@ -6,11 +6,13 @@
>   #ifndef QEMU_PIXMAN_H
>   #define QEMU_PIXMAN_H
>   
> +#ifdef CONFIG_PIXMAN
>   /* pixman-0.16.0 headers have a redundant declaration */
>   #pragma GCC diagnostic push
>   #pragma GCC diagnostic ignored "-Wredundant-decls"
>   #include <pixman.h>
>   #pragma GCC diagnostic pop
> +#endif
>   
>   /*
>    * pixman image formats are defined to be native endian,
> diff --git a/Kconfig.host b/Kconfig.host
> index d763d89269..b6ac2b9316 100644
> --- a/Kconfig.host
> +++ b/Kconfig.host
> @@ -11,6 +11,9 @@ config OPENGL
>   config X11
>       bool
>   
> +config PIXMAN
> +    bool
> +
>   config SPICE
>       bool
>   
> diff --git a/meson_options.txt b/meson_options.txt
> index f82d88b7c6..1aaec02d68 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -218,6 +218,8 @@ option('l2tpv3', type : 'feature', value : 'auto',
>          description: 'l2tpv3 network backend support')
>   option('netmap', type : 'feature', value : 'auto',
>          description: 'netmap network backend support')
> +option('pixman', type : 'feature', value : 'auto',
> +       description: 'pixman support')
>   option('slirp', type: 'feature', value: 'auto',
>          description: 'libslirp user mode network backend support')
>   option('vde', type : 'feature', value : 'auto',
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index e1d178370c..d016caf819 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -147,6 +147,7 @@ meson_options_help() {
>     printf "%s\n" '  pa              PulseAudio sound support'
>     printf "%s\n" '  parallels       parallels image format support'
>     printf "%s\n" '  pipewire        PipeWire sound support'
> +  printf "%s\n" '  pixman          pixman support'
>     printf "%s\n" '  png             PNG support with libpng'
>     printf "%s\n" '  pvrdma          Enable PVRDMA support'
>     printf "%s\n" '  qcow1           qcow1 image format support'
> @@ -398,6 +399,8 @@ _meson_option_parse() {
>       --disable-parallels) printf "%s" -Dparallels=disabled ;;
>       --enable-pipewire) printf "%s" -Dpipewire=enabled ;;
>       --disable-pipewire) printf "%s" -Dpipewire=disabled ;;
> +    --enable-pixman) printf "%s" -Dpixman=enabled ;;
> +    --disable-pixman) printf "%s" -Dpixman=disabled ;;
>       --with-pkgversion=*) quote_sh "-Dpkgversion=$2" ;;
>       --enable-plugins) printf "%s" -Dplugins=true ;;
>       --disable-plugins) printf "%s" -Dplugins=false ;;

Re: [PATCH v2 05/18] ui/vc: console-vc requires PIXMAN

[Paolo Bonzini]

On 9/18/23 15:51, marcandre.lureau@redhat.com wrote:
>           return "vc:80Cx24C";
> +#else
> +        return "null";

Maybe return NULL and then add some heuristics in vl.c:

      const char *vc = qemu_display_get_vc(&dpy);
      if (nographic ||
          (!vc && !is_daemonized() && isatty(STDOUT_FILENO)) {
          ...
      } else {
         if (default_serial)
             add_device_config(DEV_SERIAL, vc ? vc : "null");
         if (default_parallel)
             add_device_config(DEV_PARALLEL, vc ? vc : "null");
         if (default_monitor && vc)
             monitor_parse(vc, "readline", false);
      }

This would use a muxed console on stdio if pixman is disabled.

Paolo

Re: [PATCH v2 09/18] vhost-user-gpu: skip VHOST_USER_GPU_UPDATE when !PIXMAN

[Paolo Bonzini]

On 9/18/23 15:51, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This simply means that 2d drawing updates won't be handled, but 3d
> should work.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

How bad is this?  Is it worth making the device dependent on pixman 
altogether?

Paolo

> ---
>   hw/display/vhost-user-gpu.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 1150521d9d..709c8a02a1 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -307,6 +307,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
>           dpy_gl_update(con, m->x, m->y, m->width, m->height);
>           break;
>       }
> +#ifdef CONFIG_PIXMAN
>       case VHOST_USER_GPU_UPDATE: {
>           VhostUserGpuUpdate *m = &msg->payload.update;
>   
> @@ -334,6 +335,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
>           }
>           break;
>       }
> +#endif
>       default:
>           g_warning("unhandled message %d %d", msg->request, msg->size);
>       }

Re: [PATCH v2 09/18] vhost-user-gpu: skip VHOST_USER_GPU_UPDATE when !PIXMAN

[Marc-André Lureau]

Hi

On Tue, Sep 19, 2023 at 4:56 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 9/18/23 15:51, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This simply means that 2d drawing updates won't be handled, but 3d
> > should work.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> How bad is this?  Is it worth making the device dependent on pixman
> altogether?
>

It will issue warnings for 2d commands, and display a blank output.
But 3d / dmabuf will be displayed.

(ideally, some day when QEMU won't do any graphics/ui, the
GPU/renderer process will issue the draw commands to a client
directly, without going through the QEMU process - using
org.qemu.Display1.Listener DBus interface)

-- 
Marc-André Lureau

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[Marc-André Lureau]

Hi Zoltan

On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Drop the "x-pixman" property and use fallback path in such case.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > hw/display/sm501.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 0eecd00701..a897c82f04 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
> >     switch (cmd) {
> >     case 0: /* BitBlt */
> >     {
> > -        static uint32_t tmp_buf[16384];
> >         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
> >         unsigned int src_y = s->twoD_source & 0xFFFF;
> >         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> > @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
> >                 de = db + (width + (height - 1) * dst_pitch) * bypp;
> >                 overlap = (db < se && sb < de);
> >             }
> > +#ifdef CONFIG_PIXMAN
> >             if (overlap && (s->use_pixman & BIT(2))) {
> >                 /* pixman can't do reverse blit: copy via temporary */
> >                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
> > +                static uint32_t tmp_buf[16384];
> >                 uint32_t *tmp = tmp_buf;
> >
> >                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
> > @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
> >                                        dst_pitch * bypp / sizeof(uint32_t),
> >                                        8 * bypp, 8 * bypp, src_x, src_y,
> >                                        dst_x, dst_y, width, height);
> > -            } else {
> > +            } else
> > +#else
> > +            {
> >                 fallback = true;
> >             }
> > +#endif
> >             if (fallback) {
> >                 uint8_t *sp = s->local_mem + src_base;
> >                 uint8_t *d = s->local_mem + dst_base;
> > @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
> >             color = cpu_to_le16(color);
> >         }
> >
> > +#ifdef CONFIG_PIXMAN
> >         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
> >             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
> >                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
> > -                         dst_x, dst_y, width, height, color)) {
> > +                         dst_x, dst_y, width, height, color))
> > +#endif
> > +        {
> >             /* fallback when pixman failed or we don't want to call it */
> >             uint8_t *d = s->local_mem + dst_base;
> >             unsigned int x, y, i;
> > @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> >
> > static Property sm501_sysbus_properties[] = {
> >     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> > +#ifdef CONFIG_PIXMAN
> >     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
> > +#endif
>
> Do we want to omit the property when compiled without pixman? I think we
> could leave it there and it would just be ignored without pixman but the
> same command line would still work and need less ifdefs in code.

That's a reasonable idea to me. At least, it can handle x-pixman=0
fine when !CONFIG_PIXMAN then.

Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
will add a TODO :)


>
> Otherwise:
>
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Regards,
> BALATON Zoltan
>
> >     DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > @@ -2126,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
> >
> > static Property sm501_pci_properties[] = {
> >     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> > +#ifdef CONFIG_PIXMAN
> >     DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
> > +#endif
> >     DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > @@ -2169,8 +2180,10 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
> >
> > static void sm501_pci_init(Object *o)
> > {
> > +#ifdef CONFIG_PIXMAN
> >     object_property_set_description(o, "x-pixman", "Use pixman for: "
> >                                     "1: fill, 2: blit, 4: overlap blit");
> > +#endif
> > }
> >
> > static const TypeInfo sm501_pci_info = {
> >



--
Marc-André Lureau

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[BALATON Zoltan]

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

On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> Hi Zoltan
>
> On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Drop the "x-pixman" property and use fallback path in such case.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>> hw/display/sm501.c | 19 ++++++++++++++++---
>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index 0eecd00701..a897c82f04 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
>>>     switch (cmd) {
>>>     case 0: /* BitBlt */
>>>     {
>>> -        static uint32_t tmp_buf[16384];
>>>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
>>>         unsigned int src_y = s->twoD_source & 0xFFFF;
>>>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
>>> @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
>>>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>>>                 overlap = (db < se && sb < de);
>>>             }
>>> +#ifdef CONFIG_PIXMAN
>>>             if (overlap && (s->use_pixman & BIT(2))) {
>>>                 /* pixman can't do reverse blit: copy via temporary */
>>>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
>>> +                static uint32_t tmp_buf[16384];
>>>                 uint32_t *tmp = tmp_buf;
>>>
>>>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
>>> @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
>>>                                        dst_pitch * bypp / sizeof(uint32_t),
>>>                                        8 * bypp, 8 * bypp, src_x, src_y,
>>>                                        dst_x, dst_y, width, height);
>>> -            } else {
>>> +            } else
>>> +#else
>>> +            {
>>>                 fallback = true;
>>>             }
>>> +#endif
>>>             if (fallback) {
>>>                 uint8_t *sp = s->local_mem + src_base;
>>>                 uint8_t *d = s->local_mem + dst_base;
>>> @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
>>>             color = cpu_to_le16(color);
>>>         }
>>>
>>> +#ifdef CONFIG_PIXMAN
>>>         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
>>>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
>>>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
>>> -                         dst_x, dst_y, width, height, color)) {
>>> +                         dst_x, dst_y, width, height, color))
>>> +#endif
>>> +        {
>>>             /* fallback when pixman failed or we don't want to call it */
>>>             uint8_t *d = s->local_mem + dst_base;
>>>             unsigned int x, y, i;
>>> @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>>>
>>> static Property sm501_sysbus_properties[] = {
>>>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>>> +#ifdef CONFIG_PIXMAN
>>>     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
>>> +#endif
>>
>> Do we want to omit the property when compiled without pixman? I think we
>> could leave it there and it would just be ignored without pixman but the
>> same command line would still work and need less ifdefs in code.
>
> That's a reasonable idea to me. At least, it can handle x-pixman=0
> fine when !CONFIG_PIXMAN then.
>
> Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
> will add a TODO :)

Erm, a bit can be 1 or 0 but the default value of it is 7. It's not a 
single but but a bitmask the enable/disable pisman for different 
operations separately so I think it can't be _BIT.

Regards,
BALATON Zoltan

>>
>> Otherwise:
>>
>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>>
>> Regards,
>> BALATON Zoltan
>>
>>>     DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> @@ -2126,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>>>
>>> static Property sm501_pci_properties[] = {
>>>     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
>>> +#ifdef CONFIG_PIXMAN
>>>     DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
>>> +#endif
>>>     DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> @@ -2169,8 +2180,10 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
>>>
>>> static void sm501_pci_init(Object *o)
>>> {
>>> +#ifdef CONFIG_PIXMAN
>>>     object_property_set_description(o, "x-pixman", "Use pixman for: "
>>>                                     "1: fill, 2: blit, 4: overlap blit");
>>> +#endif
>>> }
>>>
>>> static const TypeInfo sm501_pci_info = {
>>>
>
>
>
> --
> Marc-André Lureau
>
>

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[Marc-André Lureau]

Hi

On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> > Hi Zoltan
> >
> > On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >>
> >> On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
> >>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>
> >>> Drop the "x-pixman" property and use fallback path in such case.
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>> ---
> >>> hw/display/sm501.c | 19 ++++++++++++++++---
> >>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> >>> index 0eecd00701..a897c82f04 100644
> >>> --- a/hw/display/sm501.c
> >>> +++ b/hw/display/sm501.c
> >>> @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
> >>>     switch (cmd) {
> >>>     case 0: /* BitBlt */
> >>>     {
> >>> -        static uint32_t tmp_buf[16384];
> >>>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
> >>>         unsigned int src_y = s->twoD_source & 0xFFFF;
> >>>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> >>> @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
> >>>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
> >>>                 overlap = (db < se && sb < de);
> >>>             }
> >>> +#ifdef CONFIG_PIXMAN
> >>>             if (overlap && (s->use_pixman & BIT(2))) {
> >>>                 /* pixman can't do reverse blit: copy via temporary */
> >>>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
> >>> +                static uint32_t tmp_buf[16384];
> >>>                 uint32_t *tmp = tmp_buf;
> >>>
> >>>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
> >>> @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
> >>>                                        dst_pitch * bypp / sizeof(uint32_t),
> >>>                                        8 * bypp, 8 * bypp, src_x, src_y,
> >>>                                        dst_x, dst_y, width, height);
> >>> -            } else {
> >>> +            } else
> >>> +#else
> >>> +            {
> >>>                 fallback = true;
> >>>             }
> >>> +#endif
> >>>             if (fallback) {
> >>>                 uint8_t *sp = s->local_mem + src_base;
> >>>                 uint8_t *d = s->local_mem + dst_base;
> >>> @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
> >>>             color = cpu_to_le16(color);
> >>>         }
> >>>
> >>> +#ifdef CONFIG_PIXMAN
> >>>         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
> >>>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
> >>>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
> >>> -                         dst_x, dst_y, width, height, color)) {
> >>> +                         dst_x, dst_y, width, height, color))
> >>> +#endif
> >>> +        {
> >>>             /* fallback when pixman failed or we don't want to call it */
> >>>             uint8_t *d = s->local_mem + dst_base;
> >>>             unsigned int x, y, i;
> >>> @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> >>>
> >>> static Property sm501_sysbus_properties[] = {
> >>>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> >>> +#ifdef CONFIG_PIXMAN
> >>>     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
> >>> +#endif
> >>
> >> Do we want to omit the property when compiled without pixman? I think we
> >> could leave it there and it would just be ignored without pixman but the
> >> same command line would still work and need less ifdefs in code.
> >
> > That's a reasonable idea to me. At least, it can handle x-pixman=0
> > fine when !CONFIG_PIXMAN then.
> >
> > Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
> > will add a TODO :)
>
> Erm, a bit can be 1 or 0 but the default value of it is 7. It's not a
> single but but a bitmask the enable/disable pisman for different
> operations separately so I think it can't be _BIT.

Sure, but we could have more explicitly different BIT properties
("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit").

thanks

-- 
Marc-André Lureau

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[BALATON Zoltan]

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

On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
>>> Hi Zoltan
>>>
>>> On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>
>>>> On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> Drop the "x-pixman" property and use fallback path in such case.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>> hw/display/sm501.c | 19 ++++++++++++++++---
>>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>>>> index 0eecd00701..a897c82f04 100644
>>>>> --- a/hw/display/sm501.c
>>>>> +++ b/hw/display/sm501.c
>>>>> @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
>>>>>     switch (cmd) {
>>>>>     case 0: /* BitBlt */
>>>>>     {
>>>>> -        static uint32_t tmp_buf[16384];
>>>>>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
>>>>>         unsigned int src_y = s->twoD_source & 0xFFFF;
>>>>>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
>>>>> @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
>>>>>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>>>>>                 overlap = (db < se && sb < de);
>>>>>             }
>>>>> +#ifdef CONFIG_PIXMAN
>>>>>             if (overlap && (s->use_pixman & BIT(2))) {
>>>>>                 /* pixman can't do reverse blit: copy via temporary */
>>>>>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
>>>>> +                static uint32_t tmp_buf[16384];
>>>>>                 uint32_t *tmp = tmp_buf;
>>>>>
>>>>>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
>>>>> @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
>>>>>                                        dst_pitch * bypp / sizeof(uint32_t),
>>>>>                                        8 * bypp, 8 * bypp, src_x, src_y,
>>>>>                                        dst_x, dst_y, width, height);
>>>>> -            } else {
>>>>> +            } else
>>>>> +#else
>>>>> +            {
>>>>>                 fallback = true;
>>>>>             }
>>>>> +#endif
>>>>>             if (fallback) {
>>>>>                 uint8_t *sp = s->local_mem + src_base;
>>>>>                 uint8_t *d = s->local_mem + dst_base;
>>>>> @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
>>>>>             color = cpu_to_le16(color);
>>>>>         }
>>>>>
>>>>> +#ifdef CONFIG_PIXMAN
>>>>>         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
>>>>>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
>>>>>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
>>>>> -                         dst_x, dst_y, width, height, color)) {
>>>>> +                         dst_x, dst_y, width, height, color))
>>>>> +#endif
>>>>> +        {
>>>>>             /* fallback when pixman failed or we don't want to call it */
>>>>>             uint8_t *d = s->local_mem + dst_base;
>>>>>             unsigned int x, y, i;
>>>>> @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>>>>>
>>>>> static Property sm501_sysbus_properties[] = {
>>>>>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>>>>> +#ifdef CONFIG_PIXMAN
>>>>>     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
>>>>> +#endif
>>>>
>>>> Do we want to omit the property when compiled without pixman? I think we
>>>> could leave it there and it would just be ignored without pixman but the
>>>> same command line would still work and need less ifdefs in code.
>>>
>>> That's a reasonable idea to me. At least, it can handle x-pixman=0
>>> fine when !CONFIG_PIXMAN then.
>>>
>>> Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
>>> will add a TODO :)
>>
>> Erm, a bit can be 1 or 0 but the default value of it is 7. It's not a
>> single but but a bitmask the enable/disable pisman for different
>> operations separately so I think it can't be _BIT.
>
> Sure, but we could have more explicitly different BIT properties
> ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit").

That was also proposed when I've added it and concluded that we don't want 
that. This is a debug option for experts and adding a lot of experimental 
options for that that are also harder to type is not an improvement so 
having just a value is fine.

Regards,
BALATON Zoltan

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[Marc-André Lureau]

Hi

On Tue, Oct 10, 2023 at 2:09 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> > On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> >>> Hi Zoltan
> >>>
> >>> On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >>>>
> >>>> On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
> >>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>
> >>>>> Drop the "x-pixman" property and use fallback path in such case.
> >>>>>
> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>> ---
> >>>>> hw/display/sm501.c | 19 ++++++++++++++++---
> >>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> >>>>> index 0eecd00701..a897c82f04 100644
> >>>>> --- a/hw/display/sm501.c
> >>>>> +++ b/hw/display/sm501.c
> >>>>> @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
> >>>>>     switch (cmd) {
> >>>>>     case 0: /* BitBlt */
> >>>>>     {
> >>>>> -        static uint32_t tmp_buf[16384];
> >>>>>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
> >>>>>         unsigned int src_y = s->twoD_source & 0xFFFF;
> >>>>>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> >>>>> @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
> >>>>>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
> >>>>>                 overlap = (db < se && sb < de);
> >>>>>             }
> >>>>> +#ifdef CONFIG_PIXMAN
> >>>>>             if (overlap && (s->use_pixman & BIT(2))) {
> >>>>>                 /* pixman can't do reverse blit: copy via temporary */
> >>>>>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
> >>>>> +                static uint32_t tmp_buf[16384];
> >>>>>                 uint32_t *tmp = tmp_buf;
> >>>>>
> >>>>>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
> >>>>> @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
> >>>>>                                        dst_pitch * bypp / sizeof(uint32_t),
> >>>>>                                        8 * bypp, 8 * bypp, src_x, src_y,
> >>>>>                                        dst_x, dst_y, width, height);
> >>>>> -            } else {
> >>>>> +            } else
> >>>>> +#else
> >>>>> +            {
> >>>>>                 fallback = true;
> >>>>>             }
> >>>>> +#endif
> >>>>>             if (fallback) {
> >>>>>                 uint8_t *sp = s->local_mem + src_base;
> >>>>>                 uint8_t *d = s->local_mem + dst_base;
> >>>>> @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
> >>>>>             color = cpu_to_le16(color);
> >>>>>         }
> >>>>>
> >>>>> +#ifdef CONFIG_PIXMAN
> >>>>>         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
> >>>>>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
> >>>>>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
> >>>>> -                         dst_x, dst_y, width, height, color)) {
> >>>>> +                         dst_x, dst_y, width, height, color))
> >>>>> +#endif
> >>>>> +        {
> >>>>>             /* fallback when pixman failed or we don't want to call it */
> >>>>>             uint8_t *d = s->local_mem + dst_base;
> >>>>>             unsigned int x, y, i;
> >>>>> @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> >>>>>
> >>>>> static Property sm501_sysbus_properties[] = {
> >>>>>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> >>>>> +#ifdef CONFIG_PIXMAN
> >>>>>     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
> >>>>> +#endif
> >>>>
> >>>> Do we want to omit the property when compiled without pixman? I think we
> >>>> could leave it there and it would just be ignored without pixman but the
> >>>> same command line would still work and need less ifdefs in code.
> >>>
> >>> That's a reasonable idea to me. At least, it can handle x-pixman=0
> >>> fine when !CONFIG_PIXMAN then.
> >>>
> >>> Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
> >>> will add a TODO :)
> >>
> >> Erm, a bit can be 1 or 0 but the default value of it is 7. It's not a
> >> single but but a bitmask the enable/disable pisman for different
> >> operations separately so I think it can't be _BIT.
> >
> > Sure, but we could have more explicitly different BIT properties
> > ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit").
>
> That was also proposed when I've added it and concluded that we don't want
> that. This is a debug option for experts and adding a lot of experimental
> options for that that are also harder to type is not an improvement so
> having just a value is fine.

Ok, I'll change the comment to:
/* this a debug option, prefer UINT over PROP_BIT for simplicity */

r-b with that?
thanks

-- 
Marc-André Lureau

Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[BALATON Zoltan]

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

On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> Hi
>
> On Tue, Oct 10, 2023 at 2:09 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
>>> On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
>>>>> Hi Zoltan
>>>>>
>>>>> On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>>>
>>>>>> On Mon, 18 Sep 2023, marcandre.lureau@redhat.com wrote:
>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> Drop the "x-pixman" property and use fallback path in such case.
>>>>>>>
>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>> ---
>>>>>>> hw/display/sm501.c | 19 ++++++++++++++++---
>>>>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>>>>>> index 0eecd00701..a897c82f04 100644
>>>>>>> --- a/hw/display/sm501.c
>>>>>>> +++ b/hw/display/sm501.c
>>>>>>> @@ -730,7 +730,6 @@ static void sm501_2d_operation(SM501State *s)
>>>>>>>     switch (cmd) {
>>>>>>>     case 0: /* BitBlt */
>>>>>>>     {
>>>>>>> -        static uint32_t tmp_buf[16384];
>>>>>>>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
>>>>>>>         unsigned int src_y = s->twoD_source & 0xFFFF;
>>>>>>>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
>>>>>>> @@ -828,9 +827,11 @@ static void sm501_2d_operation(SM501State *s)
>>>>>>>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>>>>>>>                 overlap = (db < se && sb < de);
>>>>>>>             }
>>>>>>> +#ifdef CONFIG_PIXMAN
>>>>>>>             if (overlap && (s->use_pixman & BIT(2))) {
>>>>>>>                 /* pixman can't do reverse blit: copy via temporary */
>>>>>>>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
>>>>>>> +                static uint32_t tmp_buf[16384];
>>>>>>>                 uint32_t *tmp = tmp_buf;
>>>>>>>
>>>>>>>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
>>>>>>> @@ -860,9 +861,12 @@ static void sm501_2d_operation(SM501State *s)
>>>>>>>                                        dst_pitch * bypp / sizeof(uint32_t),
>>>>>>>                                        8 * bypp, 8 * bypp, src_x, src_y,
>>>>>>>                                        dst_x, dst_y, width, height);
>>>>>>> -            } else {
>>>>>>> +            } else
>>>>>>> +#else
>>>>>>> +            {
>>>>>>>                 fallback = true;
>>>>>>>             }
>>>>>>> +#endif
>>>>>>>             if (fallback) {
>>>>>>>                 uint8_t *sp = s->local_mem + src_base;
>>>>>>>                 uint8_t *d = s->local_mem + dst_base;
>>>>>>> @@ -894,10 +898,13 @@ static void sm501_2d_operation(SM501State *s)
>>>>>>>             color = cpu_to_le16(color);
>>>>>>>         }
>>>>>>>
>>>>>>> +#ifdef CONFIG_PIXMAN
>>>>>>>         if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
>>>>>>>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
>>>>>>>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
>>>>>>> -                         dst_x, dst_y, width, height, color)) {
>>>>>>> +                         dst_x, dst_y, width, height, color))
>>>>>>> +#endif
>>>>>>> +        {
>>>>>>>             /* fallback when pixman failed or we don't want to call it */
>>>>>>>             uint8_t *d = s->local_mem + dst_base;
>>>>>>>             unsigned int x, y, i;
>>>>>>> @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>>>>>>>
>>>>>>> static Property sm501_sysbus_properties[] = {
>>>>>>>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>>>>>>> +#ifdef CONFIG_PIXMAN
>>>>>>>     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
>>>>>>> +#endif
>>>>>>
>>>>>> Do we want to omit the property when compiled without pixman? I think we
>>>>>> could leave it there and it would just be ignored without pixman but the
>>>>>> same command line would still work and need less ifdefs in code.
>>>>>
>>>>> That's a reasonable idea to me. At least, it can handle x-pixman=0
>>>>> fine when !CONFIG_PIXMAN then.
>>>>>
>>>>> Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
>>>>> will add a TODO :)
>>>>
>>>> Erm, a bit can be 1 or 0 but the default value of it is 7. It's not a
>>>> single but but a bitmask the enable/disable pisman for different
>>>> operations separately so I think it can't be _BIT.
>>>
>>> Sure, but we could have more explicitly different BIT properties
>>> ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit").
>>
>> That was also proposed when I've added it and concluded that we don't want
>> that. This is a debug option for experts and adding a lot of experimental
>> options for that that are also harder to type is not an improvement so
>> having just a value is fine.
>
> Ok, I'll change the comment to:
> /* this a debug option, prefer UINT over PROP_BIT for simplicity */
>
> r-b with that?

I don't think it needs a comment but I don't mind if you add one. I'll 
review the latest version later and if you add a comment that won't change 
it.

Regards,
BALATON Zoltan