Message ID | 20230830093843.3531473-1-marcandre.lureau@redhat.com |
---|---|
Headers | show |
Series | Make pixman an optional dependency | expand |
On 30/08/2023 11.37, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Hi, > > QEMU system emulators can be made to compile and work without pixman. > > Given how pervasively pixman types and API is used in all the code base, it was > a bit difficult to figure out how to cut the dependency. > > I decided that it was important to keep VGA and graphics device working for > compatibility reasons, although some devices, such as xlnx Display Port, have > stronger dependency and have to be disabled. The ui/console code also has a lot > of pixman usage and a bit of a mess to deal with. I made large refactoring to > allow to compile out the VC code. > > The series can be roughly divded as: > - a few related preliminary cleanups > - ui/console refactoring to allow ui/vc split > - add a 'pixman' option, and a minimal pixman-compat.h > - make some parts depend on 'pixman' > > Graphic -display still work, although with some caveats. For ex, -display sdl or > cocoa don't have VCs, so starting QEMU will print the following warnings when > pixman is disabled: I just had a quick look at the series, but for me it looks like this is adding a lot of additional complexity to the code (adds lots of #ifdefs, and adds a subset of the pixman library to the code base), which seems somewhat unfortunate for such a marginal feature request. What's really so bad about requiring pixman for building QEMU? IMHO, if we really want to go down this path, I think we should rather disable all graphic related stuff in QEMU instead, i.e. disable VGA cards, Spice, SDL, etc. completely. I think this is also what has been requested in the original gitlab issue ticket where the reporter wanted to compile a text-mode only QEMU binary...? ... just my 0.02 € anyway. Thomas
Hi On Wed, Aug 30, 2023 at 2:53 PM Thomas Huth <thuth@redhat.com> wrote: > > On 30/08/2023 11.37, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Hi, > > > > QEMU system emulators can be made to compile and work without pixman. > > > > Given how pervasively pixman types and API is used in all the code base, it was > > a bit difficult to figure out how to cut the dependency. > > > > I decided that it was important to keep VGA and graphics device working for > > compatibility reasons, although some devices, such as xlnx Display Port, have > > stronger dependency and have to be disabled. The ui/console code also has a lot > > of pixman usage and a bit of a mess to deal with. I made large refactoring to > > allow to compile out the VC code. > > > > The series can be roughly divded as: > > - a few related preliminary cleanups > > - ui/console refactoring to allow ui/vc split > > - add a 'pixman' option, and a minimal pixman-compat.h > > - make some parts depend on 'pixman' > > > > Graphic -display still work, although with some caveats. For ex, -display sdl or > > cocoa don't have VCs, so starting QEMU will print the following warnings when > > pixman is disabled: > > I just had a quick look at the series, but for me it looks like this is > adding a lot of additional complexity to the code (adds lots of #ifdefs, and > adds a subset of the pixman library to the code base), which seems somewhat The #ifdef aren't so bad (at least I can bare it). Just a quick stat: $ git diff origin/master | grep "+#ifdef CONFIG_PIXMAN" | wc -l 11 > unfortunate for such a marginal feature request. What's really so bad about > requiring pixman for building QEMU? Not that a good part of the series is cleaning up ui/console.c code that really deserved it. It makes it use QOM, and split VC code. It's still worth it regardless of the outcome for pixman. > IMHO, if we really want to go down this path, I think we should rather > disable all graphic related stuff in QEMU instead, i.e. disable VGA cards, > Spice, SDL, etc. completely. I think this is also what has been requested in > The various features and devices can be disabled by other means. I think we should aim at making the different configure options orthogonal, so QEMU without pixman can still provide most gfx/vga/UI experience too, by default. > the original gitlab issue ticket where the reporter wanted to compile a > text-mode only QEMU binary...? So that is not an incompatible goal, further tuning of configure options can help.
On Wed, Aug 30, 2023 at 03:01:27PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Aug 30, 2023 at 2:53 PM Thomas Huth <thuth@redhat.com> wrote: > > > > On 30/08/2023 11.37, marcandre.lureau@redhat.com wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > Hi, > > > > > > QEMU system emulators can be made to compile and work without pixman. > > > > > > Given how pervasively pixman types and API is used in all the code base, it was > > > a bit difficult to figure out how to cut the dependency. > > > > > > I decided that it was important to keep VGA and graphics device working for > > > compatibility reasons, although some devices, such as xlnx Display Port, have > > > stronger dependency and have to be disabled. The ui/console code also has a lot > > > of pixman usage and a bit of a mess to deal with. I made large refactoring to > > > allow to compile out the VC code. > > > > > > The series can be roughly divded as: > > > - a few related preliminary cleanups > > > - ui/console refactoring to allow ui/vc split > > > - add a 'pixman' option, and a minimal pixman-compat.h > > > - make some parts depend on 'pixman' > > > > > > Graphic -display still work, although with some caveats. For ex, -display sdl or > > > cocoa don't have VCs, so starting QEMU will print the following warnings when > > > pixman is disabled: > > > > I just had a quick look at the series, but for me it looks like this is > > adding a lot of additional complexity to the code (adds lots of #ifdefs, and > > adds a subset of the pixman library to the code base), which seems somewhat > > The #ifdef aren't so bad (at least I can bare it). Just a quick stat: > > $ git diff origin/master | grep "+#ifdef CONFIG_PIXMAN" | wc -l > 11 > > > unfortunate for such a marginal feature request. What's really so bad about > > requiring pixman for building QEMU? > > Not that a good part of the series is cleaning up ui/console.c code > that really deserved it. It makes it use QOM, and split VC code. It's > still worth it regardless of the outcome for pixman. I've done a review of the start and like the cleanup patches, and the adaption to make sane use of inheritance in QOM rather than the poor man's inheritance via the console type field. I agree that's worthwhile regardless of what we think about pixman optionality. > > IMHO, if we really want to go down this path, I think we should rather > > disable all graphic related stuff in QEMU instead, i.e. disable VGA cards, > > Spice, SDL, etc. completely. I think this is also what has been requested in > > > > The various features and devices can be disabled by other means. I > think we should aim at making the different configure options > orthogonal, so QEMU without pixman can still provide most gfx/vga/UI > experience too, by default. To me where this series becomes dubious is roughly around the patch that adds pixman-compat.h providing a bunch of pixman API as stubs. If we can use Kconfig and/or meson options to simply drop the build of files in QEMU that require pixman that's reasonable. If we're adding compat APIs the provide local impls of pixman APIs it feels wrong to me. > > the original gitlab issue ticket where the reporter wanted to compile a > > text-mode only QEMU binary...? > > So that is not an incompatible goal, further tuning of configure > options can help. With regards, Daniel
Hi On Wed, Aug 30, 2023 at 3:21 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Aug 30, 2023 at 03:01:27PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Wed, Aug 30, 2023 at 2:53 PM Thomas Huth <thuth@redhat.com> wrote: > > > > > > On 30/08/2023 11.37, marcandre.lureau@redhat.com wrote: > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > > > Hi, > > > > > > > > QEMU system emulators can be made to compile and work without pixman. > > > > > > > > Given how pervasively pixman types and API is used in all the code base, it was > > > > a bit difficult to figure out how to cut the dependency. > > > > > > > > I decided that it was important to keep VGA and graphics device working for > > > > compatibility reasons, although some devices, such as xlnx Display Port, have > > > > stronger dependency and have to be disabled. The ui/console code also has a lot > > > > of pixman usage and a bit of a mess to deal with. I made large refactoring to > > > > allow to compile out the VC code. > > > > > > > > The series can be roughly divded as: > > > > - a few related preliminary cleanups > > > > - ui/console refactoring to allow ui/vc split > > > > - add a 'pixman' option, and a minimal pixman-compat.h > > > > - make some parts depend on 'pixman' > > > > > > > > Graphic -display still work, although with some caveats. For ex, -display sdl or > > > > cocoa don't have VCs, so starting QEMU will print the following warnings when > > > > pixman is disabled: > > > > > > I just had a quick look at the series, but for me it looks like this is > > > adding a lot of additional complexity to the code (adds lots of #ifdefs, and > > > adds a subset of the pixman library to the code base), which seems somewhat > > > > The #ifdef aren't so bad (at least I can bare it). Just a quick stat: > > > > $ git diff origin/master | grep "+#ifdef CONFIG_PIXMAN" | wc -l > > 11 > > > > > unfortunate for such a marginal feature request. What's really so bad about > > > requiring pixman for building QEMU? > > > > Not that a good part of the series is cleaning up ui/console.c code > > that really deserved it. It makes it use QOM, and split VC code. It's > > still worth it regardless of the outcome for pixman. > > I've done a review of the start and like the cleanup patches, and the > adaption to make sane use of inheritance in QOM rather than the poor > man's inheritance via the console type field. I agree that's worthwhile > regardless of what we think about pixman optionality. > > > > IMHO, if we really want to go down this path, I think we should rather > > > disable all graphic related stuff in QEMU instead, i.e. disable VGA cards, > > > Spice, SDL, etc. completely. I think this is also what has been requested in > > > > > > > The various features and devices can be disabled by other means. I > > think we should aim at making the different configure options > > orthogonal, so QEMU without pixman can still provide most gfx/vga/UI > > experience too, by default. > > To me where this series becomes dubious is roughly around the patch > that adds pixman-compat.h providing a bunch of pixman API as stubs. Not stubs actually... this shows that you haven't looked at it enough :) It's really stupid, nothing like pixman! it just provides a few types. See my reply on the patch. Completely removing those few pixman types from QEMU will lead to a very unpleasant and complicated result. I can give it another try, or you may also play with it and prove me wrong. At least, I can come up with a better list of arguments.
From: Marc-André Lureau <marcandre.lureau@redhat.com> Hi, QEMU system emulators can be made to compile and work without pixman. Given how pervasively pixman types and API is used in all the code base, it was a bit difficult to figure out how to cut the dependency. I decided that it was important to keep VGA and graphics device working for compatibility reasons, although some devices, such as xlnx Display Port, have stronger dependency and have to be disabled. The ui/console code also has a lot of pixman usage and a bit of a mess to deal with. I made large refactoring to allow to compile out the VC code. The series can be roughly divded as: - a few related preliminary cleanups - ui/console refactoring to allow ui/vc split - add a 'pixman' option, and a minimal pixman-compat.h - make some parts depend on 'pixman' Graphic -display still work, although with some caveats. For ex, -display sdl or cocoa don't have VCs, so starting QEMU will print the following warnings when pixman is disabled: qemu-system-x86_64: warning: compat_monitor0: this is a dummy VC driver. Use '-nographic' or a different chardev. qemu-system-x86_64: warning: serial0: this is a dummy VC driver. Use '-nographic' or a different chardev. qemu-system-x86_64: warning: parallel0: this is a dummy VC driver. Use '-nographic' or a different chardev. But -display dbus is fully functionnal, for ex, as it operates at chardev/text level. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1172 Marc-André Lureau (67): ui: remove qemu_pixman_color() helper ui: remove qemu_pixman_linebuf_copy() ui/qmp: move screendump to ui-qmp-cmds.c ui/vc: replace vc_chr_write() with generic qemu_chr_write() ui/vc: drop have_text ui/console: console_select() regardless of have_gfx ui/console: call dpy_gfx_update() regardless of have_gfx ui/console: drop have_gfx ui/console: get the DisplayState from new_console() ui/console: new_console() cannot fail ui/vc: VC always has a DisplayState now ui/vc: move VCChardev declaration at the top ui/vc: replace variable with static text attributes default ui/vc: fold text_update_xy() ui/vc: pass VCCharDev to VC-specific functions ui/vc: move VCCharDev specific fields out of QemuConsole ui/console: use OBJECT_DEFINE_TYPE for QemuConsole ui/console: change new_console() to use object initialization ui/console: introduce different console objects ui/console: instantiate a specific console type ui/console: register the console from qemu_console_init() ui/console: remove new_console() ui/console: specialize console_lookup_unused() ui/console: update the head from unused QemuConsole ui/console: allocate ui_timer in QemuConsole ui/vc: move cursor_timer initialization to QemuTextConsole class ui/console: free more QemuConsole resources ui/vc: move text fields to QemuTextConsole ui/console: move graphic fields to QemuGraphicConsole ui/vc: fold text_console_do_init() in vc_chr_open() ui/vc: move some text console initialization to qom handlers ui/console: simplify getting active_console size ui/console: remove need for g_width/g_height ui/vc: use common text console surface creation ui/console: declare console types in console.h ui/console: use QEMU_PIXMAN_COLOR helpers ui/console: rename vga_ functions → qemu_console_ ui/console: assert(surface) where appropriate ui/console: fold text_console_update_cursor_timer ui/vc: skip text console resize when possible ui/console: minor stylistic changes ui/vc: move text console invalidate in helper ui/vc: do not parse VC-specific options in Spice and GTK ui/vc: change the argument for QemuTextConsole ui/vc: remove kby_put_keysym() and update function calls ui/vc: rename kbd_put → qemu_text_console functions ui/console: remove redundant format field ui/vc: preliminary QemuTextConsole changes before split ui/vc: split off the VC part from console.c ui/console: move DisplaySurface to its own header build-sys: add optional "pixman" feature ui: compile out some qemu-pixman functions when !PIXMAN ui: add pixman-compat.h 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 ppc/kconfig: make SAM460EX depend on PPC & PIXMAN sh4/kconfig: make R2D depend on SH4 & PIXMAN display/kconfig: make SM501 depend on PIXMAN configs/devices/ppc-softmmu/default.mak | 2 +- configs/devices/sh4-softmmu/default.mak | 2 +- meson.build | 11 +- qapi/char.json | 4 + qapi/ui.json | 3 +- include/chardev/char.h | 3 - include/ui/console.h | 118 +- include/ui/pixman-compat.h | 190 +++ include/ui/qemu-pixman.h | 20 +- include/ui/rect.h | 55 + include/ui/surface.h | 91 ++ ui/console-priv.h | 43 + hw/display/vhost-user-gpu.c | 2 + hw/display/virtio-gpu.c | 30 +- ui/console-gl.c | 2 +- ui/console-vc-stubs.c | 59 + ui/console-vc.c | 1079 +++++++++++++++ ui/console.c | 1603 +++-------------------- ui/curses.c | 2 +- ui/dbus-listener.c | 62 +- ui/gtk.c | 11 +- ui/qemu-pixman.c | 25 +- ui/sdl2-input.c | 5 +- ui/sdl2.c | 5 +- ui/spice-app.c | 7 +- ui/spice-display.c | 2 +- ui/ui-hmp-cmds.c | 2 + ui/ui-qmp-cmds.c | 189 +++ ui/vnc.c | 56 +- Kconfig.host | 3 + hmp-commands.hx | 2 + hw/arm/Kconfig | 2 +- hw/display/Kconfig | 3 +- hw/ppc/Kconfig | 2 + hw/sh4/Kconfig | 2 + meson_options.txt | 2 + ui/cocoa.m | 2 +- ui/meson.build | 25 +- 38 files changed, 2093 insertions(+), 1633 deletions(-) create mode 100644 include/ui/pixman-compat.h create mode 100644 include/ui/rect.h create mode 100644 include/ui/surface.h create mode 100644 ui/console-priv.h create mode 100644 ui/console-vc-stubs.c create mode 100644 ui/console-vc.c