Re: [PATCH] qemu: add sdl opengl support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/05/18 00:08, John Ferlan wrote:
> BTW: Since your patches add a capability - I would have expected a
> change to add a flag to one (or more) of the
> tests/qemucapabilitiesdata/caps_*.xml files, but none are modified.  So
> that means that the feature may not be introspectable, perhaps it's been
> part of qemu since 1.5 or it's something being added to the next qemu
> release. If it's a new feature, then when was it added? If it's been
> there since 1.5, then no capability flag is required.

I'm going to need some help understanding the QEMU capabilities test
(tests/qemucapabilitiestest.c). As I understand it, it run QEMU to get
the capabilities and then compares that to the XML file containing the
expected list of capabilities. What are the *.replies files used for?
And how is it do that on multiple architectures and QEMU versions
at the same time?

> 
> How that us determined for sdl is a mystery to me... I usually search
> through the qemu */*.json files. In this case I do find some remnants of
> 'gl' and 'sdl' in qapi/ui.json.  But how I read that output is that for
> 2.12 there's 'sdl' in DisplayOptions, but using 'DisplayNoOpts' which
> could mean no options for sdl are allowed, but I'm not sure. Maybe there
> is some change in flight - I don't follow qemu-devel that closely.
> 
> If I look back at commit id '937ebba00e' for the spice-gl addition
> (which yes, was one in one patch) - I can relate that to the similar
> spice gl fetch, but looking the recent top of qemu git tree, I don't see
> how this same mechanism can be used to determine whether the running
> qemu supports sdl using gl.

It looks like this option was introduced to QEMU in 0b71a5d5. v2.4.0 is
the earliest release which contains that commit.

> 
> So for code and other libvirt specific things...
> 
> BTW: The docs/formatdomain.html.in to add/describe the 'gl' option for
> 'sdl' needs to be provided.
> 
>> 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
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux