9:55 a.m.
On Mon, Apr 28, 2025 at 12:33:19PM +0200, Peter Krempa wrote:
On Mon, Apr 28, 2025 at 12:22:22 +0200, Kirill Shchetiniuk via Devel
wrote:
> From: Kirill Shchetiniuk <kshcheti(a)redhat.com>
>
> Introduce GTK display support with OpenGL for the QEMU driver.
>
> - Add new XML options for GTK display type.
> - Include capability flags for the QEMU driver.
>
> Note: The `QEMU_CAPS_GTK_GL` flag cannot yet be checked, so device
> definition validation is incomplete. A placeholder is left for
> future implementation, when the GTK along with OpenGL capability
> check would be available in QEMU.
Can you elaborate? WHy can't it be checked?
So I have discussed this with Michal Privoznik, it's possible to
directly gather if GTK capability is enable, but there is now
straight way to check if OpenGL for GTK is enabled too. Michal
purposed a way to check this with a small hack, check if GTK
capability is presented along with egl-headless, but we decided
better not to check OpenGL for GTK cabability for now, as it's not
straightforward approch, and I decided to leave a placeholer
for the future check to do not forget to add this later.
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/570
>
> Signed-off-by: Kirill Shchetiniuk <kshcheti(a)redhat.com>
> ---
You will need to split this patch.
At least the capability flag needs to be a separate commit that only
adds the capability flag.
I'm also missing qemuxmlconftest test cases and docs/formatdomain.rst
documentation changes.
Sure, will this with next series, thanks!
[...]
> 36 files changed, 218 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 542d6ade91..73550f7fcf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -960,6 +960,7 @@ VIR_ENUM_IMPL(virDomainGraphics,
> "spice",
> "egl-headless",
> "dbus",
> + "gtk",
> );
>
> VIR_ENUM_IMPL(virDomainGraphicsListen,
> @@ -2054,6 +2055,12 @@ void virDomainGraphicsDefFree(virDomainGraphicsDef *def)
> g_free(def->data.dbus.rendernode);
> break;
>
> + case VIR_DOMAIN_GRAPHICS_TYPE_GTK:
> + g_free(def->data.gtk.display);
> + g_free(def->data.gtk.xauth);
> + g_free(def->data.gtk.rendernode);
> + break;
> +
> case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> break;
> }
> @@ -12199,6 +12206,39 @@ virDomainGraphicsDefParseXMLDBus(virDomainGraphicsDef
*def,
> }
>
>
> +static int
> +virDomainGraphicsDefParseXMLGTK(virDomainGraphicsDef *def,
> + xmlNodePtr node,
> + xmlXPathContextPtr ctxt)
> +{
> + VIR_XPATH_NODE_AUTORESTORE(ctxt)
> + xmlNodePtr glNode;
> + virTristateBool fullscreen;
> +
> + ctxt->node = node;
> +
> + if (virXMLPropTristateBool(node, "fullscreen", VIR_XML_PROP_NONE,
> + &fullscreen) < 0)
You parse a tristate ...
> + return -1;
> +
> + virTristateBoolToBool(fullscreen, &def->data.gtk.fullscreen);
... but store it only as boolean?
So I saw the same approach in virDomainGraphicsDefParseXMLSDL and decided to
use it too. Do you suggest virXPathBoolean instead or something else?
> + def->data.gtk.xauth = virXMLPropString(node, "xauth");
> + def->data.gtk.display = virXMLPropString(node, "display");
> +
> + if ((glNode = virXPathNode("./gl", ctxt))) {
> + def->data.gtk.rendernode = virXMLPropString(glNode,
> + "rendernode");
> +
> + if (virXMLPropTristateBool(glNode, "enable",
> + VIR_XML_PROP_REQUIRED,
> + &def->data.gtk.gl) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> virDomainGraphicsDef *
> virDomainGraphicsDefNew(virDomainXMLOption *xmlopt)
> {
[...]
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index a804335c85..de7ce95499 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -732,6 +732,8 @@ VIR_ENUM_IMPL(virQEMUCaps,
>
> /* 475 */
> "virtio-scsi.iothread-mapping", /*
QEMU_CAPS_VIRTIO_SCSI_IOTHREAD_MAPPING */
> + "gtk", /* QEMU_CAPS_GTK */
> + "gtk-gl", /* QEMU_CAPS_GTK_GL */
> );
>
>
> @@ -1602,6 +1604,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
> { "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
> { "query-cpu-model-expansion/ret-type/deprecated-props",
QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION_DEPRECATED_PROPS },
> { "migrate-incoming/arg-type/exit-on-error",
QEMU_CAPS_MIGRATE_INCOMING_EXIT_ON_ERROR },
> + { "query-display-options/ret-type/type/^gtk", QEMU_CAPS_GTK },
> };
>
> typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
> @@ -6499,6 +6502,8 @@ virQEMUCapsFillDomainDeviceGraphicsCaps(virQEMUDriverConfig
*cfg,
> VIR_DOMAIN_GRAPHICS_TYPE_RDP);
> }
> }
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GTK))
> + VIR_DOMAIN_CAPS_ENUM_SET(dev->type, VIR_DOMAIN_GRAPHICS_TYPE_GTK);
> }
>
>
[...]
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index b2c3c9e2f6..be92785d71 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -4523,6 +4523,17 @@ qemuValidateDomainDeviceDefRDPGraphics(const
virDomainGraphicsDef *graphics)
> }
>
>
> +static int
> +qemuValidateDomainDeviceDefGTKGraphics(const virDomainGraphicsDef *graphics
G_GNUC_UNUSED,
> + virQEMUCaps *qemuCaps G_GNUC_UNUSED)
> +{
> + /* TODO: OpenGL check */
> + /* For now it's not possible to check if gtk-gl capability is available in
QEMU */
> + /* Add cappability check later when will be possible to gather the capability
from QEMU */
So why does the patch add the capability then? Or is this comment no
longer applicable?
So I didn't know how to do it better, decided to leave a placeholder for future
changes.
With next series will remove the yet unused gtk-gl capability, and leave the placeholder
only.
> + return 0;
> +}
> +
> +
> static int
> qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics,
> const virDomainDef *def,