Re: [PATCH] qemu: add sdl opengl support

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

 




On 05/10/2018 05:25 AM, Maciej Wolny wrote:
> 
> 
> On 04/05/18 00:08, John Ferlan wrote:
>>> 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.
> 
> Unfortunately, the SPICE GL property has a rendernode attribute, while
> the SDL one doesn't.. I think this prevents a sensible name definition
> which would fit both use cases.
> 

Fair enough - it was more a suggestion. I'll look at v2 in a bit.

>>
>>>          </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);
>>
>>
> 
> Without that 'if', it'll append an empty, quoted string ("''") to the
> command line. The lexical scope in there was indeed unnecessary.
>  

I'll look again in v2 - the command line building can be awkward for
those that don't do it often.

>>> +
>>>          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.
> 
> I haven't managed to make that working with DO_TEST_CAPS_LATEST.

Right as I noted in the response to your other posting here - that's
because you only modified the 2.4 capabilities and using the "latest"
probably won't work similarly.

Again I'll look at v2 and we can go from there.

John

> I need more information on how QEMU capabilities work.. Am I right to
> think that the actual capabilities will depend not only on the version
> of QEMU but also other packages in the system (say, virgl will require
> libvirglrenderer)? If so, do the files in tests/qemucapabilitiesdata
> only list basic capabilities that are always provided by that version of
> QEMU? Adding QEMU_CAPS_SDL_GL to the latest caps *.xml file isn't enough,
> that test will also need virgl (which is not listed in that file).
> 
>>
>> 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