Re: [libvirt] [PATCH] qemu: add sdl opengl support

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 3569b9212..a2ef93c09 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -3031,6 +3031,14 @@ > <ref name="virYesNo"/> > </attribute> > </optional> > + <optional> > + <element name="gl"> > + <attribute name="enable"> > + <ref name="virYesNo"/> > + </attribute> > + <empty/> > + </element> > + </optional> I assume this is a copy of the spice gl - to reduce cut-paste-copy, you could create a "name" for it and share that name between spice and sdl. It's a common thing to do. Not required, but not difficult either.

> </group> > <group> > <attribute name="type"> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index b0257068d..7d65ca9df 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -13448,6 +13448,7 @@ static int > virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def, > xmlNodePtr node) > { > + xmlNodePtr cur; > char *fullscreen = virXMLPropString(node, "fullscreen"); > int ret = -1; > > @@ -13468,6 +13469,34 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def, > def->data.sdl.xauth = virXMLPropString(node, "xauth"); > def->data.sdl.display = virXMLPropString(node, "display"); > > + cur = node->children; > + while (cur != NULL) { > + if (cur->type == XML_ELEMENT_NODE) { > + if (virXMLNodeNameEqual(cur, "gl")) { > + char *enable = virXMLPropString(cur, "enable"); > + int enableVal; > + > + if (! enable) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("sdl gl element missing enable")); > + goto cleanup; > + } > + > + enableVal = virTristateBoolTypeFromString(enable); > + if (enableVal < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown enable value '%s'"), enable); > + VIR_FREE(enable); > + goto cleanup; > + } > + VIR_FREE(enable); > + > + def->data.sdl.gl = enableVal; > + } > + } > + cur = cur->next; > + } > + I see this is just a copy of what Spice does, which probably needed some adjustment anyway... IIRC: Peter Krempa recently went through an exercise with the storage to change a number of these while loops using virXMLNodeNameEqual into more direct XML searches. Since this is only one element and attribute, I don't really think this loop is useful. I know it's possible to get the data via another means and some of the code already does that. The exact lines I'll leave to you to hash out. > ret = 0; > cleanup: > VIR_FREE(fullscreen); > @@ -25652,6 +25681,18 @@ virDomainGraphicsDefFormat(virBufferPtr buf, > if (def->data.sdl.fullscreen) > virBufferAddLit(buf, " fullscreen='yes'"); > > + if (!children && def->data.sdl.gl) { This should be a comparison such as "def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT" (I know it's logically the same, but daata.sdl.gl is not a boolean or a pointer). > + virBufferAddLit(buf, ">\n"); > + virBufferAdjustIndent(buf, 2); > + children = true; > + } > + > + if (def->data.sdl.gl) { Similarly... yes, I know spice.gl isn't doing that. > + virBufferAsprintf(buf, "<gl enable='%s'", > + virTristateBoolTypeToString(def->data.sdl.gl)); > + virBufferAddLit(buf, "/>\n"); > + } > + > break; > > case VIR_DOMAIN_GRAPHICS_TYPE_RDP: > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 3c7eccb8c..90071d9c0 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1601,6 +1601,7 @@ struct _virDomainGraphicsDef { > char *display; > char *xauth; > bool fullscreen; > + virTristateBool gl; > } sdl; > struct { > int port; > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index aa8d350f5..02680502e 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -474,6 +474,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "query-cpus-fast", > "disk-write-cache", > "nbd-tls", > + "sdl-gl", > ); > > > @@ -2451,6 +2452,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { > { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, > { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, > { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, > + { "sdl", "gl", QEMU_CAPS_SDL_GL }, > }; > > static int > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index 2afe7ef58..e36611e2a 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -458,6 +458,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ > QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */ > QEMU_CAPS_NBD_TLS, /* NBD server supports TLS transport */ > + QEMU_CAPS_SDL_GL, /* -sdl gl */ > > QEMU_CAPS_LAST /* this must always be the last item */ > } virQEMUCapsFlags; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 418729b98..29214e806 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -7988,6 +7988,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, > virQEMUCapsPtr qemuCaps, > virDomainGraphicsDefPtr graphics) > { > + virBuffer opt = VIR_BUFFER_INITIALIZER; > switch (graphics->type) { > case VIR_DOMAIN_GRAPHICS_TYPE_SDL: > if (graphics->data.sdl.xauth) > @@ -8009,6 +8010,24 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, Suggestion... Create a patch prior to this one that moves the code for VIR_DOMAIN_GRAPHICS_TYPE_SDL into it's own helper - such as qemuBuildGraphicsSDLCommandLine. > * default, since the default changes :-( */ The above comment can be removed in a separate patch since commit id '4e8993a2' removed the check for QEMU_CAPS_SDL, although it took until commit id '649a9dd7a' to "fix" the QEMU_CAPS_SDL to be X_QEMU_CAPS_SDL since the QEMU 1.5 is our new minimum version supported. > virCommandAddArg(cmd, "-sdl"); > > + if (graphics->data.sdl.gl == VIR_TRISTATE_BOOL_YES) { > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("This QEMU doesn't support SDL OpenGL")); > + return -1; > + > + } > + > + virBufferAsprintf(&opt, "gl=%s", > + virTristateSwitchTypeToString(graphics->data.sdl.gl)); > + } > + > + { > + const char *optContent = virBufferCurrentContent(&opt); > + if (optContent && STRNEQ(optContent, "")) > + virCommandAddArgBuffer(cmd, &opt); > + } With it's own helper ^^this^^ awkward definition in the middle is unnecessary I think... In fact, I don't even know why it matters - I would think you could just do the virCommandAddArgBuffer right after the virBufferAsprintf for "gl=%s"... I think the separate helper will be good though in case there's more things added for sdl. e.g.: virCommandAddArgBuffer(cmd, &opt); virBufferFreeAndReset(&opt);

> + > break; > > case VIR_DOMAIN_GRAPHICS_TYPE_VNC: > diff --git a/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args > new file mode 100644 > index 000000000..4172320ed > --- /dev/null > +++ b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.args > @@ -0,0 +1,28 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +/usr/bin/qemu-system-i686 \ > +-name QEMUGuest1 \ > +-S \ > +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ > +-m 1024 \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-no-user-config \ > +-nodefaults \ > +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ > +server,nowait \ > +-mon chardev=charmonitor,id=monitor,mode=control \ > +-rtc base=utc \ > +-no-shutdown \ > +-no-acpi \ > +-boot c \ > +-usb \ > +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ > +id=drive-ide0-0-0,cache=none \ > +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > +-sdl gl=on \ > +-device virtio-gpu-pci,id=video0,virgl=on,bus=pci.0,addr=0x2 \ > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml > new file mode 100644 > index 000000000..9dea73fbe > --- /dev/null > +++ b/tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml > @@ -0,0 +1,38 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>1048576</memory> > + <currentMemory unit='KiB'>1048576</currentMemory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='i686' machine='pc'>hvm</type> > + <boot dev='hd'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>destroy</on_crash> > + <devices> > + <emulator>/usr/bin/qemu-system-i686</emulator> > + <disk type='file' device='disk'> > + <driver name='qemu' type='qcow2' cache='none'/> > + <source file='/var/lib/libvirt/images/QEMUGuest1'/> > + <target dev='hda' bus='ide'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <controller type='ide' index='0'/> > + <controller type='usb' index='0'/> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <graphics type='sdl'> > + <gl enable='yes'/> > + </graphics> > + <video> > + <model type='virtio' heads='1'> > + <acceleration accel3d='yes'/> > + </model> > + </video> > + <memballoon model='virtio'/> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 5b3bd4a99..0b06699f0 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1924,6 +1924,11 @@ mymain(void) > QEMU_CAPS_SPICE_GL, > QEMU_CAPS_SPICE_RENDERNODE, > QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > + DO_TEST("video-virtio-gpu-sdl-gl", > + QEMU_CAPS_DEVICE_VIRTIO_GPU, > + QEMU_CAPS_VIRTIO_GPU_VIRGL, > + QEMU_CAPS_SDL_GL, > + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); Newer code should use DO_TEST_CAPS_LATEST, but I think that'll fail based on there being no capability defined in latest.

The only reason we're printing the "-sdl gl=on" in the .args file is that you've passed the capability here. > DO_TEST("video-virtio-gpu-secondary", > QEMU_CAPS_DEVICE_VIRTIO_GPU, > QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > There needs to be an adjustment to qemuxml2xmltest.c as well. That'll cause an output file to be generated/compared against as well. Hint, use VIR_TEST_REGENERATE_OUTPUT=1 after touching the output file. John