| Message ID | 543c98f507a30a92c42f1d240b47ad90f0efdad3.1463588606.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi On Wed, May 18, 2016 at 6:40 PM, Cole Robinson <crobinso@redhat.com> wrote: > Give users a hint if their config is wrong. > > Signed-off-by: Cole Robinson <crobinso@redhat.com> > --- > If virgl support is built into qemu, virgl=on is the default, so this > could be noisy in cases where people don't even care about virgl. So > I won't object if this is dropped. > > The message also pops up once via make check, probably from > tests/display-vga-test.c , but doesn't cause a failure or anything. > > Is there a way to check that user explicitly specified virgl= ? Good point, there should be a way by parsing options (but would that work with monitor created devices?). I don't see anything from qdev_property_add_static/object_property_set that would differentiate the default value from explicit value. > > hw/display/virtio-gpu.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index c181fb3..d3c567f 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -13,6 +13,7 @@ > > #include "qemu/osdep.h" > #include "qemu-common.h" > +#include "qemu/error-report.h" > #include "qemu/iov.h" > #include "ui/console.h" > #include "trace.h" > @@ -944,6 +945,10 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > have_virgl = display_opengl; > #endif > if (!have_virgl) { > + if (virtio_gpu_virgl_enabled(g->conf)) { > + error_report("Display isn't configured for GL support. " > + "Disabling virgl"); > + } > g->conf.flags &= ~(1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED); > } > > -- > 2.7.4 > > I don't mind the error_report() so, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
On Mi, 2016-05-18 at 12:40 -0400, Cole Robinson wrote: > Give users a hint if their config is wrong. > > Signed-off-by: Cole Robinson <crobinso@redhat.com> > --- > If virgl support is built into qemu, virgl=on is the default, so this > could be noisy in cases where people don't even care about virgl. So > I won't object if this is dropped. Yes, it's enabled by default, so users have to flip one switch only (-spice gl=on) to enable 3d, not two. So warning here is a bad thing IMO. We could turn the virgl switch into tristate (OnOffAuto), have it default to Auto, then fail (not warn) in case it is set to On without 3d support being available. I'll go cherry-pick #1+#2. cheers, Gerd
On 05/20/2016 01:53 AM, Gerd Hoffmann wrote: > On Mi, 2016-05-18 at 12:40 -0400, Cole Robinson wrote: >> Give users a hint if their config is wrong. >> >> Signed-off-by: Cole Robinson <crobinso@redhat.com> >> --- >> If virgl support is built into qemu, virgl=on is the default, so this >> could be noisy in cases where people don't even care about virgl. So >> I won't object if this is dropped. > > Yes, it's enabled by default, so users have to flip one switch only > (-spice gl=on) to enable 3d, not two. So warning here is a bad thing > IMO. > > We could turn the virgl switch into tristate (OnOffAuto), have it > default to Auto, then fail (not warn) in case it is set to On without 3d > support being available. > Property settings are part of the migration format, right? Is there an easy way to change the 'virgl' property like that in a backwards compatible way? Or decouple the command line handling from the Property value? Thanks, Cole
Hi, > > We could turn the virgl switch into tristate (OnOffAuto), have it > > default to Auto, then fail (not warn) in case it is set to On without 3d > > support being available. > > > > Property settings are part of the migration format, right? Is there an easy > way to change the 'virgl' property like that in a backwards compatible way? Or > decouple the command line handling from the Property value? No worries for now, virtio-gpu doesn't support live migration (patch for live migration in 2d mode exists but was too late for 2.6 release). cheers, Gerd
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index c181fb3..d3c567f 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -13,6 +13,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" +#include "qemu/error-report.h" #include "qemu/iov.h" #include "ui/console.h" #include "trace.h" @@ -944,6 +945,10 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) have_virgl = display_opengl; #endif if (!have_virgl) { + if (virtio_gpu_virgl_enabled(g->conf)) { + error_report("Display isn't configured for GL support. " + "Disabling virgl"); + } g->conf.flags &= ~(1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED); }
Give users a hint if their config is wrong. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- If virgl support is built into qemu, virgl=on is the default, so this could be noisy in cases where people don't even care about virgl. So I won't object if this is dropped. The message also pops up once via make check, probably from tests/display-vga-test.c , but doesn't cause a failure or anything. Is there a way to check that user explicitly specified virgl= ? hw/display/virtio-gpu.c | 5 +++++ 1 file changed, 5 insertions(+)