[Qemu-devel] [PATCH 0/3] ui: improve feedback if gl setup fails

[Qemu-devel] [PATCH 0/3] ui: improve feedback if gl setup fails

[Cole Robinson]

patch 1 is just a cleanup, patch 2-3 improves feedback for invalid
gl configs (which would have saved me an hour of head scratching)

Cole Robinson (3):
  ui: egl: Replace fprintf with error_report
  ui: spice: Exit if gl=on EGL init fails
  virtio-gpu: Warn if UI config will disable virgl

 hw/display/virtio-gpu.c |  5 +++++
 ui/egl-helpers.c        | 27 ++++++++++++++-------------
 ui/spice-core.c         |  6 ++++--
 3 files changed, 23 insertions(+), 15 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] ui: egl: Replace fprintf with error_report

[Cole Robinson]

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 ui/egl-helpers.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 558edfd..6555f5f 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -2,6 +2,7 @@
 #include <glob.h>
 #include <dirent.h>
 
+#include "qemu/error-report.h"
 #include "ui/egl-helpers.h"
 
 EGLDisplay *qemu_egl_display;
@@ -77,13 +78,13 @@ int egl_rendernode_init(void)
 
     qemu_egl_rn_fd = qemu_egl_rendernode_open();
     if (qemu_egl_rn_fd == -1) {
-        fprintf(stderr, "egl: no drm render node available\n");
+        error_report("egl: no drm render node available");
         goto err;
     }
 
     qemu_egl_rn_gbm_dev = gbm_create_device(qemu_egl_rn_fd);
     if (!qemu_egl_rn_gbm_dev) {
-        fprintf(stderr, "egl: gbm_create_device failed\n");
+        error_report("egl: gbm_create_device failed");
         goto err;
     }
 
@@ -91,18 +92,18 @@ int egl_rendernode_init(void)
 
     if (!epoxy_has_egl_extension(qemu_egl_display,
                                  "EGL_KHR_surfaceless_context")) {
-        fprintf(stderr, "egl: EGL_KHR_surfaceless_context not supported\n");
+        error_report("egl: EGL_KHR_surfaceless_context not supported");
         goto err;
     }
     if (!epoxy_has_egl_extension(qemu_egl_display,
                                  "EGL_MESA_image_dma_buf_export")) {
-        fprintf(stderr, "egl: EGL_MESA_image_dma_buf_export not supported\n");
+        error_report("egl: EGL_MESA_image_dma_buf_export not supported");
         goto err;
     }
 
     qemu_egl_rn_ctx = qemu_egl_init_ctx();
     if (!qemu_egl_rn_ctx) {
-        fprintf(stderr, "egl: egl_init_ctx failed\n");
+        error_report("egl: egl_init_ctx failed");
         goto err;
     }
 
@@ -159,13 +160,13 @@ EGLSurface qemu_egl_init_surface_x11(EGLContext ectx, Window win)
                                       qemu_egl_config,
                                       (EGLNativeWindowType)win, NULL);
     if (esurface == EGL_NO_SURFACE) {
-        fprintf(stderr, "egl: eglCreateWindowSurface failed\n");
+        error_report("egl: eglCreateWindowSurface failed");
         return NULL;
     }
 
     b = eglMakeCurrent(qemu_egl_display, esurface, esurface, ectx);
     if (b == EGL_FALSE) {
-        fprintf(stderr, "egl: eglMakeCurrent failed\n");
+        error_report("egl: eglMakeCurrent failed");
         return NULL;
     }
 
@@ -207,21 +208,21 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug)
     egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
     qemu_egl_display = eglGetDisplay(dpy);
     if (qemu_egl_display == EGL_NO_DISPLAY) {
-        fprintf(stderr, "egl: eglGetDisplay failed\n");
+        error_report("egl: eglGetDisplay failed");
         return -1;
     }
 
     egl_dbg("eglInitialize ...\n");
     b = eglInitialize(qemu_egl_display, &major, &minor);
     if (b == EGL_FALSE) {
-        fprintf(stderr, "egl: eglInitialize failed\n");
+        error_report("egl: eglInitialize failed");
         return -1;
     }
 
     egl_dbg("eglBindAPI ...\n");
     b = eglBindAPI(gles ? EGL_OPENGL_ES_API : EGL_OPENGL_API);
     if (b == EGL_FALSE) {
-        fprintf(stderr, "egl: eglBindAPI failed\n");
+        error_report("egl: eglBindAPI failed");
         return -1;
     }
 
@@ -230,7 +231,7 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug)
                         gles ? conf_att_gles : conf_att_gl,
                         &qemu_egl_config, 1, &n);
     if (b == EGL_FALSE || n != 1) {
-        fprintf(stderr, "egl: eglChooseConfig failed\n");
+        error_report("egl: eglChooseConfig failed");
         return -1;
     }
 
@@ -255,13 +256,13 @@ EGLContext qemu_egl_init_ctx(void)
     ectx = eglCreateContext(qemu_egl_display, qemu_egl_config, EGL_NO_CONTEXT,
                             egl_gles ? ctx_att_gles : ctx_att_gl);
     if (ectx == EGL_NO_CONTEXT) {
-        fprintf(stderr, "egl: eglCreateContext failed\n");
+        error_report("egl: eglCreateContext failed");
         return NULL;
     }
 
     b = eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, ectx);
     if (b == EGL_FALSE) {
-        fprintf(stderr, "egl: eglMakeCurrent failed\n");
+        error_report("egl: eglMakeCurrent failed");
         return NULL;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] ui: egl: Replace fprintf with error_report

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On 05/18/2016 10:40 AM, Cole Robinson wrote:
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  ui/egl-helpers.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] ui: egl: Replace fprintf with error_report

[Marc-André Lureau]

Hi

On Wed, May 18, 2016 at 6:40 PM, Cole Robinson <crobinso@redhat.com> wrote:
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  ui/egl-helpers.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> index 558edfd..6555f5f 100644
> --- a/ui/egl-helpers.c
> +++ b/ui/egl-helpers.c
> @@ -2,6 +2,7 @@
>  #include <glob.h>
>  #include <dirent.h>
>
> +#include "qemu/error-report.h"
>  #include "ui/egl-helpers.h"
>
>  EGLDisplay *qemu_egl_display;
> @@ -77,13 +78,13 @@ int egl_rendernode_init(void)
>
>      qemu_egl_rn_fd = qemu_egl_rendernode_open();
>      if (qemu_egl_rn_fd == -1) {
> -        fprintf(stderr, "egl: no drm render node available\n");
> +        error_report("egl: no drm render node available");
>          goto err;
>      }
>
>      qemu_egl_rn_gbm_dev = gbm_create_device(qemu_egl_rn_fd);
>      if (!qemu_egl_rn_gbm_dev) {
> -        fprintf(stderr, "egl: gbm_create_device failed\n");
> +        error_report("egl: gbm_create_device failed");
>          goto err;
>      }
>
> @@ -91,18 +92,18 @@ int egl_rendernode_init(void)
>
>      if (!epoxy_has_egl_extension(qemu_egl_display,
>                                   "EGL_KHR_surfaceless_context")) {
> -        fprintf(stderr, "egl: EGL_KHR_surfaceless_context not supported\n");
> +        error_report("egl: EGL_KHR_surfaceless_context not supported");
>          goto err;
>      }
>      if (!epoxy_has_egl_extension(qemu_egl_display,
>                                   "EGL_MESA_image_dma_buf_export")) {
> -        fprintf(stderr, "egl: EGL_MESA_image_dma_buf_export not supported\n");
> +        error_report("egl: EGL_MESA_image_dma_buf_export not supported");
>          goto err;
>      }
>
>      qemu_egl_rn_ctx = qemu_egl_init_ctx();
>      if (!qemu_egl_rn_ctx) {
> -        fprintf(stderr, "egl: egl_init_ctx failed\n");
> +        error_report("egl: egl_init_ctx failed");
>          goto err;
>      }
>
> @@ -159,13 +160,13 @@ EGLSurface qemu_egl_init_surface_x11(EGLContext ectx, Window win)
>                                        qemu_egl_config,
>                                        (EGLNativeWindowType)win, NULL);
>      if (esurface == EGL_NO_SURFACE) {
> -        fprintf(stderr, "egl: eglCreateWindowSurface failed\n");
> +        error_report("egl: eglCreateWindowSurface failed");
>          return NULL;
>      }
>
>      b = eglMakeCurrent(qemu_egl_display, esurface, esurface, ectx);
>      if (b == EGL_FALSE) {
> -        fprintf(stderr, "egl: eglMakeCurrent failed\n");
> +        error_report("egl: eglMakeCurrent failed");
>          return NULL;
>      }
>
> @@ -207,21 +208,21 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug)
>      egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
>      qemu_egl_display = eglGetDisplay(dpy);
>      if (qemu_egl_display == EGL_NO_DISPLAY) {
> -        fprintf(stderr, "egl: eglGetDisplay failed\n");
> +        error_report("egl: eglGetDisplay failed");
>          return -1;
>      }
>
>      egl_dbg("eglInitialize ...\n");
>      b = eglInitialize(qemu_egl_display, &major, &minor);
>      if (b == EGL_FALSE) {
> -        fprintf(stderr, "egl: eglInitialize failed\n");
> +        error_report("egl: eglInitialize failed");
>          return -1;
>      }
>
>      egl_dbg("eglBindAPI ...\n");
>      b = eglBindAPI(gles ? EGL_OPENGL_ES_API : EGL_OPENGL_API);
>      if (b == EGL_FALSE) {
> -        fprintf(stderr, "egl: eglBindAPI failed\n");
> +        error_report("egl: eglBindAPI failed");
>          return -1;
>      }
>
> @@ -230,7 +231,7 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug)
>                          gles ? conf_att_gles : conf_att_gl,
>                          &qemu_egl_config, 1, &n);
>      if (b == EGL_FALSE || n != 1) {
> -        fprintf(stderr, "egl: eglChooseConfig failed\n");
> +        error_report("egl: eglChooseConfig failed");
>          return -1;
>      }
>
> @@ -255,13 +256,13 @@ EGLContext qemu_egl_init_ctx(void)
>      ectx = eglCreateContext(qemu_egl_display, qemu_egl_config, EGL_NO_CONTEXT,
>                              egl_gles ? ctx_att_gles : ctx_att_gl);
>      if (ectx == EGL_NO_CONTEXT) {
> -        fprintf(stderr, "egl: eglCreateContext failed\n");
> +        error_report("egl: eglCreateContext failed");
>          return NULL;
>      }
>
>      b = eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, ectx);
>      if (b == EGL_FALSE) {
> -        fprintf(stderr, "egl: eglMakeCurrent failed\n");
> +        error_report("egl: eglMakeCurrent failed");
>          return NULL;
>      }
>
> --
> 2.7.4
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

(btw, I wonder why there is no check in checkpath for this)

-- 
Marc-André Lureau

[Qemu-devel] [PATCH 2/3] ui: spice: Exit if gl=on EGL init fails

[Cole Robinson]

The user explicitly requested spice GL, so if we know it isn't
going to work we should exit

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
For example, trying to use spice GL with libvirt qemu:///system will
fail here, since the the VM will lack permissions to access
/dev/dir/renderD*

 ui/spice-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 61db3c1..da05054 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -833,9 +833,11 @@ void qemu_spice_init(void)
                          "incompatible with -spice port/tls-port");
             exit(1);
         }
-        if (egl_rendernode_init() == 0) {
-            display_opengl = 1;
+        if (egl_rendernode_init() != 0) {
+            error_report("Failed to initialize EGL render node for SPICE GL");
+            exit(1);
         }
+        display_opengl = 1;
     }
 #endif
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] ui: spice: Exit if gl=on EGL init fails

[Marc-André Lureau]

Hi

On Wed, May 18, 2016 at 6:40 PM, Cole Robinson <crobinso@redhat.com> wrote:
> The user explicitly requested spice GL, so if we know it isn't
> going to work we should exit
>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
> For example, trying to use spice GL with libvirt qemu:///system will
> fail here, since the the VM will lack permissions to access
> /dev/dir/renderD*
>
>  ui/spice-core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 61db3c1..da05054 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -833,9 +833,11 @@ void qemu_spice_init(void)
>                           "incompatible with -spice port/tls-port");
>              exit(1);
>          }
> -        if (egl_rendernode_init() == 0) {
> -            display_opengl = 1;
> +        if (egl_rendernode_init() != 0) {
> +            error_report("Failed to initialize EGL render node for SPICE GL");
> +            exit(1);
>          }
> +        display_opengl = 1;
>      }
>  #endif
>  }
> --
> 2.7.4
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH 3/3] virtio-gpu: Warn if UI config will disable virgl

[Cole Robinson]

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(+)

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

Re: [Qemu-devel] [PATCH 3/3] virtio-gpu: Warn if UI config will disable virgl

[Marc-André Lureau]

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>




-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 3/3] virtio-gpu: Warn if UI config will disable virgl

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH 3/3] virtio-gpu: Warn if UI config will disable virgl

[Cole Robinson]

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

Re: [Qemu-devel] [PATCH 3/3] virtio-gpu: Warn if UI config will disable virgl

[Gerd Hoffmann]

  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