[PATCH 0/7] qemu: Rework qemuBuildDeviceVideoStr() slightly
The most important patch is 7/7, IMO. The rest is just prep work. Michal Prívozník (7): qemuDomainSupportsVideoVga: Fix const correctness qemuBuildDeviceVideoStr: Separate out video module selection qemuDeviceVideoGetModel: Deduplicate a check qemu_command: Switch from VIR_ENUM_IMPL(qemuDeviceVideo) to explicit switch() qemu_command: Switch from VIR_ENUM_IMPL(qemuDeviceVideoSecondary) to explicit switch() qemuBuildDeviceVideoStr: Move logic wrapping qemuBuildVirtioDevStr() into qemuDeviceVideoGetModel() qemuBuildDeviceVideoStr: Don't overwrite @model src/qemu/qemu_command.c | 159 +++++++++++++++++++++++++--------------- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 +- 3 files changed, 102 insertions(+), 61 deletions(-) -- 2.31.1
This function doesn't modify passed video definition. Make the argument const. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7f57cae00e..6bcd67611e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10163,7 +10163,7 @@ qemuDomainCheckMonitor(virQEMUDriver *driver, bool -qemuDomainSupportsVideoVga(virDomainVideoDef *video, +qemuDomainSupportsVideoVga(const virDomainVideoDef *video, virQEMUCaps *qemuCaps) { if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9cebf9e426..0394eb45a6 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -914,7 +914,7 @@ int qemuDomainCheckMonitor(virQEMUDriver *driver, virDomainObj *vm, qemuDomainAsyncJob asyncJob); -bool qemuDomainSupportsVideoVga(virDomainVideoDef *video, +bool qemuDomainSupportsVideoVga(const virDomainVideoDef *video, virQEMUCaps *qemuCaps); bool qemuDomainNeedsVFIO(const virDomainDef *def); -- 2.31.1
The code that decides video card model is going to be reworked and expanded. Separate it out into a function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4b676fe8fc..7ca099bb39 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4195,18 +4195,11 @@ qemuBuildSoundCommandLine(virCommand *cmd, } - -static char * -qemuBuildDeviceVideoStr(const virDomainDef *def, - virDomainVideoDef *video, - virQEMUCaps *qemuCaps) +static const char * +qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, + const virDomainVideoDef *video) { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *model = NULL; - virTristateSwitch accel3d = VIR_TRISTATE_SWITCH_ABSENT; - - if (video->accel) - accel3d = video->accel->accel3d; /* We try to chose the best model for primary video device by preferring * model with VGA compatibility mode. For some video devices on some @@ -4231,6 +4224,25 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, return NULL; } + return model; +} + + +static char * +qemuBuildDeviceVideoStr(const virDomainDef *def, + virDomainVideoDef *video, + virQEMUCaps *qemuCaps) +{ + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + const char *model = NULL; + virTristateSwitch accel3d = VIR_TRISTATE_SWITCH_ABSENT; + + if (video->accel) + accel3d = video->accel->accel3d; + + if (!(model = qemuDeviceVideoGetModel(qemuCaps, video))) + return NULL; + if (STREQ(model, "virtio-gpu") || STREQ(model, "vhost-user-gpu")) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_GL_PCI) && accel3d == VIR_TRISTATE_SWITCH_ON && -- 2.31.1
There is the same check written twice (whether given video card is primary one and whether it supports VGA mode). Write it just once and store it in a boolean variable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7ca099bb39..9d8e41f4cd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4200,18 +4200,22 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, const virDomainVideoDef *video) { const char *model = NULL; + bool primaryVga = false; + + if (video->primary && qemuDomainSupportsVideoVga(video, qemuCaps)) + primaryVga = true; /* We try to chose the best model for primary video device by preferring * model with VGA compatibility mode. For some video devices on some * architectures there might not be such model so fallback to one * without VGA compatibility mode. */ if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) { - if (video->primary && qemuDomainSupportsVideoVga(video, qemuCaps)) + if (primaryVga) model = "vhost-user-vga"; else model = "vhost-user-gpu"; } else { - if (video->primary && qemuDomainSupportsVideoVga(video, qemuCaps)) + if (primaryVga) model = qemuDeviceVideoTypeToString(video->type); else model = qemuDeviceVideoSecondaryTypeToString(video->type); -- 2.31.1
This may look like a step backwards, but it isn't. The point is that in near future the chosen model will depend on more than just video type. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 56 +++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9d8e41f4cd..0a22036002 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -104,25 +104,6 @@ VIR_ENUM_IMPL(qemuVideo, "", /* ramfb can't be used with -vga */ ); -VIR_ENUM_DECL(qemuDeviceVideo); - -VIR_ENUM_IMPL(qemuDeviceVideo, - VIR_DOMAIN_VIDEO_TYPE_LAST, - "", /* default value, we shouldn't see this */ - "VGA", - "cirrus-vga", - "vmware-svga", - "", /* don't support xen */ - "", /* don't support vbox */ - "qxl-vga", - "", /* don't support parallels */ - "virtio-vga", - "" /* don't support gop */, - "" /* 'none' doesn't make sense here */, - "bochs-display", - "ramfb", -); - VIR_ENUM_DECL(qemuDeviceVideoSecondary); VIR_ENUM_IMPL(qemuDeviceVideoSecondary, @@ -4215,10 +4196,41 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, else model = "vhost-user-gpu"; } else { - if (primaryVga) - model = qemuDeviceVideoTypeToString(video->type); - else + if (primaryVga) { + switch (video->type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + model = "VGA"; + break; + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + model = "cirrus-vga"; + break; + case VIR_DOMAIN_VIDEO_TYPE_VMVGA: + model = "vmware-svga"; + break; + case VIR_DOMAIN_VIDEO_TYPE_QXL: + model = "qxl-vga"; + break; + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: + model = "virtio-vga"; + break; + case VIR_DOMAIN_VIDEO_TYPE_BOCHS: + model = "bochs-display"; + break; + case VIR_DOMAIN_VIDEO_TYPE_RAMFB: + model = "ramfb"; + break; + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + case VIR_DOMAIN_VIDEO_TYPE_VBOX: + case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_GOP: + case VIR_DOMAIN_VIDEO_TYPE_NONE: + case VIR_DOMAIN_VIDEO_TYPE_LAST: + break; + } + } else { model = qemuDeviceVideoSecondaryTypeToString(video->type); + } } if (!model || STREQ(model, "")) { -- 2.31.1
On Fri, Jun 11, 2021 at 17:04:57 +0200, Michal Privoznik wrote:
This may look like a step backwards, but it isn't. The point is that in near future the chosen model will depend on more than just video type.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 56 +++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 22 deletions(-)
@@ -4215,10 +4196,41 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, else model = "vhost-user-gpu"; } else { - if (primaryVga) - model = qemuDeviceVideoTypeToString(video->type); - else + if (primaryVga) { + switch (video->type) {
video->type is declared as: int type; /* enum virDomainVideoType */ Since it's not implicit, you need to use the proper typecast in the switch to ensure that it's always populated.
+ case VIR_DOMAIN_VIDEO_TYPE_VGA: + model = "VGA"; + break; + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
This may look like a step backwards, but it isn't. The point is that in near future the chosen model will depend on more than just video type. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0a22036002..71f0b498c1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -104,25 +104,6 @@ VIR_ENUM_IMPL(qemuVideo, "", /* ramfb can't be used with -vga */ ); -VIR_ENUM_DECL(qemuDeviceVideoSecondary); - -VIR_ENUM_IMPL(qemuDeviceVideoSecondary, - VIR_DOMAIN_VIDEO_TYPE_LAST, - "", /* default value, we shouldn't see this */ - "", /* no secondary device for VGA */ - "", /* no secondary device for cirrus-vga */ - "", /* no secondary device for vmware-svga */ - "", /* don't support xen */ - "", /* don't support vbox */ - "qxl", - "", /* don't support parallels */ - "virtio-gpu", - "" /* don't support gop */, - "" /* 'none' doesn't make sense here */, - "" /* no secondary device for bochs */, - "" /* no secondary device for ramfb */, -); - VIR_ENUM_IMPL(qemuSoundCodec, VIR_DOMAIN_SOUND_CODEC_TYPE_LAST, "hda-duplex", @@ -4229,7 +4210,27 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, break; } } else { - model = qemuDeviceVideoSecondaryTypeToString(video->type); + switch (video->type) { + case VIR_DOMAIN_VIDEO_TYPE_QXL: + model = "qxl"; + break; + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: + model = "virtio-gpu"; + break; + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + case VIR_DOMAIN_VIDEO_TYPE_VMVGA: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + case VIR_DOMAIN_VIDEO_TYPE_VBOX: + case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_GOP: + case VIR_DOMAIN_VIDEO_TYPE_NONE: + case VIR_DOMAIN_VIDEO_TYPE_BOCHS: + case VIR_DOMAIN_VIDEO_TYPE_RAMFB: + case VIR_DOMAIN_VIDEO_TYPE_LAST: + break; + } } } -- 2.31.1
On Fri, Jun 11, 2021 at 17:04:58 +0200, Michal Privoznik wrote:
This may look like a step backwards, but it isn't. The point is that in near future the chosen model will depend on more than just video type.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) @@ -4229,7 +4210,27 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, break; } } else { - model = qemuDeviceVideoSecondaryTypeToString(video->type); + switch (video->type) {
Same as previous patch.
+ case VIR_DOMAIN_VIDEO_TYPE_QXL: + model = "qxl"; + break; + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: + model = "virtio-gpu"; + break; + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
We want to call qemuBuildVirtioDevStr() from qemuBuildDeviceVideoStr() but only for some models (currently "virtio-gpu" and "vhost-user-gpu"), not all of them. Move this logic into qemuDeviceVideoGetModel() because this logic will be refined. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 71f0b498c1..c9e152324d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4159,11 +4159,14 @@ qemuBuildSoundCommandLine(virCommand *cmd, static const char * qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, - const virDomainVideoDef *video) + const virDomainVideoDef *video, + bool *virtio) { const char *model = NULL; bool primaryVga = false; + *virtio = false; + if (video->primary && qemuDomainSupportsVideoVga(video, qemuCaps)) primaryVga = true; @@ -4172,10 +4175,12 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, * architectures there might not be such model so fallback to one * without VGA compatibility mode. */ if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) { - if (primaryVga) + if (primaryVga) { model = "vhost-user-vga"; - else + } else { model = "vhost-user-gpu"; + *virtio = true; + } } else { if (primaryVga) { switch (video->type) { @@ -4216,6 +4221,7 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, break; case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: model = "virtio-gpu"; + *virtio = true; break; case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: case VIR_DOMAIN_VIDEO_TYPE_VGA: @@ -4253,14 +4259,15 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *model = NULL; virTristateSwitch accel3d = VIR_TRISTATE_SWITCH_ABSENT; + bool virtio = false; if (video->accel) accel3d = video->accel->accel3d; - if (!(model = qemuDeviceVideoGetModel(qemuCaps, video))) + if (!(model = qemuDeviceVideoGetModel(qemuCaps, video, &virtio))) return NULL; - if (STREQ(model, "virtio-gpu") || STREQ(model, "vhost-user-gpu")) { + if (virtio) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_GL_PCI) && accel3d == VIR_TRISTATE_SWITCH_ON && STREQ(model, "virtio-gpu")) -- 2.31.1
Now we have everything prepared so that @model doesn't have to be rewritten. The correct model can be chosen right from the beginning. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c9e152324d..22cc383f33 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4164,6 +4164,12 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, { const char *model = NULL; bool primaryVga = false; + virTristateSwitch accel3d = VIR_TRISTATE_SWITCH_ABSENT; + + *virtio = false; + + if (video->accel) + accel3d = video->accel->accel3d; *virtio = false; @@ -4197,7 +4203,11 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, model = "qxl-vga"; break; case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: - model = "virtio-vga"; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_VGA_GL) && + accel3d == VIR_TRISTATE_SWITCH_ON) + model = "virtio-vga-gl"; + else + model = "virtio-vga"; break; case VIR_DOMAIN_VIDEO_TYPE_BOCHS: model = "bochs-display"; @@ -4220,7 +4230,11 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, model = "qxl"; break; case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: - model = "virtio-gpu"; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_GL_PCI) && + accel3d == VIR_TRISTATE_SWITCH_ON) + model = "virtio-gpu-gl"; + else + model = "virtio-gpu"; *virtio = true; break; case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: @@ -4268,21 +4282,12 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, return NULL; if (virtio) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_GL_PCI) && - accel3d == VIR_TRISTATE_SWITCH_ON && - STREQ(model, "virtio-gpu")) - model = "virtio-gpu-gl"; - if (qemuBuildVirtioDevStr(&buf, model, qemuCaps, VIR_DOMAIN_DEVICE_VIDEO, video) < 0) { return NULL; } } else { virBufferAsprintf(&buf, "%s", model); - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_VGA_GL) && - accel3d == VIR_TRISTATE_SWITCH_ON && - STREQ(model, "virtio-vga")) - virBufferAddLit(&buf, "-gl"); } virBufferAsprintf(&buf, ",id=%s", video->info.alias); -- 2.31.1
On Fri, Jun 11, 2021 at 17:05:00 +0200, Michal Privoznik wrote:
Now we have everything prepared so that @model doesn't have to be rewritten. The correct model can be chosen right from the beginning.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c9e152324d..22cc383f33 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4164,6 +4164,12 @@ qemuDeviceVideoGetModel(virQEMUCaps *qemuCaps, { const char *model = NULL; bool primaryVga = false; + virTristateSwitch accel3d = VIR_TRISTATE_SWITCH_ABSENT; + + *virtio = false;
You've already have this ...
+ + if (video->accel) + accel3d = video->accel->accel3d;
*virtio = false;
... here.
On Fri, Jun 11, 2021 at 17:04:53 +0200, Michal Privoznik wrote:
The most important patch is 7/7, IMO. The rest is just prep work.
Michal Prívozník (7): qemuDomainSupportsVideoVga: Fix const correctness qemuBuildDeviceVideoStr: Separate out video module selection qemuDeviceVideoGetModel: Deduplicate a check qemu_command: Switch from VIR_ENUM_IMPL(qemuDeviceVideo) to explicit switch() qemu_command: Switch from VIR_ENUM_IMPL(qemuDeviceVideoSecondary) to explicit switch() qemuBuildDeviceVideoStr: Move logic wrapping qemuBuildVirtioDevStr() into qemuDeviceVideoGetModel() qemuBuildDeviceVideoStr: Don't overwrite @model
src/qemu/qemu_command.c | 159 +++++++++++++++++++++++++--------------- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 +- 3 files changed, 102 insertions(+), 61 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa