[Qemu-devel] [PATCH v2 0/2] Use SDL to create an OpenGL ES context for virglrenderer.

[Qemu-devel] [PATCH v2 0/2] Use SDL to create an OpenGL ES context for virglrenderer.

[Elie Tournier]

Hello everyone,

I submitted the v1 of this series before kraxel's display system rework.

Currently, virglrenderer [1] support OpenGL ES 2.0 on the guest side
and OpenGL ES 3.0 on the host side.
Thanks to this work, we are able to run QEMU on system that only support OpenGL ES.

The support of OpenGL ES 3.0 on the guest is limited.
We are working on it, so stay tune!
For the most curious of you, the development branch is available on
our gitlab instance [2].

In order to use this feature in QEMU, we need to create an OpenGL ES context.
This is possible thanks to the following option `-display sdl,gl=es`.

Have a nice day,
Elie

[1] https://cgit.freedesktop.org/virglrenderer
[2] https://gitlab.collabora.com/virgl-es/virglrenderer-gles/tree/hacks

Elie Tournier (2):
  qapi: Parameter gl of DisplayType now accept an enum
  sdl: Allow OpenGL ES context creation

 include/ui/sdl2.h |  1 +
 qapi/ui.json      | 21 ++++++++++++++++++++-
 qemu-options.hx   |  2 +-
 ui/sdl2-gl.c      | 17 +++++++++++++++--
 ui/sdl2.c         |  1 +
 vl.c              | 14 +++++++++-----
 6 files changed, 47 insertions(+), 9 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v2 1/2] qapi: Parameter gl of DisplayType now accept an enum

[Elie Tournier]

Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
---
 qapi/ui.json | 21 ++++++++++++++++++++-
 vl.c         | 10 +++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 5d01ad4304..c8005867e5 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1019,6 +1019,25 @@
 { 'struct'  : 'DisplayGTK',
   'data'    : { '*grab-on-hover' : 'bool' } }
 
+ ##
+ # @DisplayGLMode:
+ #
+ # Display OpenGL mode.
+ #
+ # 'off'   Disable OpenGL (default).
+ # 'on'    Use OpenGL, pick context type automatically.
+ #         Would better be named 'auto' but is called 'on' for backward
+ #         compatibility with bool type.
+ # 'core'  Use OpenGL with Core (desktop) Context.
+ # 'es'    Use OpenGL with ES (embedded systems) Context.
+ #
+ # Since: 2.13
+ #
+ ##
+ { 'enum'    : 'DisplayGLMode',
+   'data'    : [ 'off', 'on', 'core', 'es' ] }
+
+
 ##
 # @DisplayType:
 #
@@ -1048,7 +1067,7 @@
   'base'    : { 'type'           : 'DisplayType',
                 '*full-screen'   : 'bool',
                 '*window-close'  : 'bool',
-                '*gl'            : 'bool' },
+                '*gl'            : 'DisplayGLMode' },
   'discriminator' : 'type',
   'data'    : { 'default'        : 'DisplayNoOpts',
                 'none'           : 'DisplayNoOpts',
diff --git a/vl.c b/vl.c
index fce1fd12d8..7809a15caf 100644
--- a/vl.c
+++ b/vl.c
@@ -2142,9 +2142,9 @@ static void parse_display(const char *p)
                 opts = nextopt;
                 dpy.has_gl = true;
                 if (strstart(opts, "on", &nextopt)) {
-                    dpy.gl = true;
+                    dpy.gl = DISPLAYGL_MODE_ON;
                 } else if (strstart(opts, "off", &nextopt)) {
-                    dpy.gl = false;
+                    dpy.gl = DISPLAYGL_MODE_OFF;
                 } else {
                     goto invalid_sdl_args;
                 }
@@ -2185,9 +2185,9 @@ static void parse_display(const char *p)
                 opts = nextopt;
                 dpy.has_gl = true;
                 if (strstart(opts, "on", &nextopt)) {
-                    dpy.gl = true;
+                    dpy.gl = DISPLAYGL_MODE_ON;
                 } else if (strstart(opts, "off", &nextopt)) {
-                    dpy.gl = false;
+                    dpy.gl = DISPLAYGL_MODE_OFF;
                 } else {
                     goto invalid_gtk_args;
                 }
@@ -4343,7 +4343,7 @@ int main(int argc, char **argv, char **envp)
     qemu_display_early_init(&dpy);
     qemu_console_early_init();
 
-    if (dpy.has_gl && dpy.gl && display_opengl == 0) {
+    if (dpy.has_gl && !dpy.gl == DISPLAYGL_MODE_OFF && display_opengl == 0) {
 #if defined(CONFIG_OPENGL)
         error_report("OpenGL is not supported by the display");
 #else
-- 
2.17.0

[Qemu-devel] [PATCH v2 2/2] sdl: Allow OpenGL ES context creation

[Elie Tournier]

Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
---
 include/ui/sdl2.h |  1 +
 qemu-options.hx   |  2 +-
 ui/sdl2-gl.c      | 17 +++++++++++++++--
 ui/sdl2.c         |  1 +
 vl.c              |  4 ++++
 5 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
index 51084e6320..8495795e48 100644
--- a/include/ui/sdl2.h
+++ b/include/ui/sdl2.h
@@ -22,6 +22,7 @@ struct sdl2_console {
     int x, y, w, h;
     int hidden;
     int opengl;
+    DisplayGLMode gl_mode;
     int updates;
     int idle_counter;
     int ignore_hotkeys;
diff --git a/qemu-options.hx b/qemu-options.hx
index ca4e412f2f..333dd1f1c8 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1240,7 +1240,7 @@ ETEXI
 
 DEF("display", HAS_ARG, QEMU_OPTION_display,
     "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
-    "            [,window_close=on|off][,gl=on|off]\n"
+    "            [,window_close=on|off][,gl=on|core|es|off]\n"
     "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
     "-display vnc=<display>[,<optargs>]\n"
     "-display curses\n"
diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index c3683e6b65..4755314afe 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -140,12 +140,25 @@ QEMUGLContext sdl2_gl_create_context(DisplayChangeListener *dcl,
     SDL_GL_MakeCurrent(scon->real_window, scon->winctx);
 
     SDL_GL_SetAttribute(SDL_GL_SHARE_WITH_CURRENT_CONTEXT, 1);
-    SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,
-                        SDL_GL_CONTEXT_PROFILE_CORE);
+    if (scon->gl_mode ==  DISPLAYGL_MODE_ON || scon->gl_mode == DISPLAYGL_MODE_CORE)
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,
+                           SDL_GL_CONTEXT_PROFILE_CORE);
+    else if (scon->gl_mode == DISPLAYGL_MODE_ES)
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,
+                           SDL_GL_CONTEXT_PROFILE_ES);
     SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, params->major_ver);
     SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, params->minor_ver);
 
     ctx = SDL_GL_CreateContext(scon->real_window);
+
+    /* If SDL fail to create a GL context and we use the "on" flag,
+     * then try to fallback to GLES.
+     */
+    if (!ctx && scon->gl_mode == DISPLAYGL_MODE_ON) {
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,
+                           SDL_GL_CONTEXT_PROFILE_ES);
+       ctx = SDL_GL_CreateContext(scon->real_window);
+    }
     return (QEMUGLContext)ctx;
 }
 
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 83b917fa37..29bf8e36ad 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -815,6 +815,7 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
         sdl2_console[i].idx = i;
 #ifdef CONFIG_OPENGL
         sdl2_console[i].opengl = display_opengl;
+        sdl2_console[i].gl_mode = o->gl;
         sdl2_console[i].dcl.ops = display_opengl ? &dcl_gl_ops : &dcl_2d_ops;
 #else
         sdl2_console[i].opengl = 0;
diff --git a/vl.c b/vl.c
index 7809a15caf..5694c23742 100644
--- a/vl.c
+++ b/vl.c
@@ -2143,6 +2143,10 @@ static void parse_display(const char *p)
                 dpy.has_gl = true;
                 if (strstart(opts, "on", &nextopt)) {
                     dpy.gl = DISPLAYGL_MODE_ON;
+                } else if (strstart(opts, "core", &nextopt)) {
+                    dpy.gl = DISPLAYGL_MODE_CORE;
+                } else if (strstart(opts, "es", &nextopt)) {
+                    dpy.gl = DISPLAYGL_MODE_ES;
                 } else if (strstart(opts, "off", &nextopt)) {
                     dpy.gl = DISPLAYGL_MODE_OFF;
                 } else {
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 0/2] Use SDL to create an OpenGL ES context for virglrenderer.

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180410120222.31845-1-tournier.elie@gmail.com
Subject: [Qemu-devel] [PATCH v2 0/2] Use SDL to create an OpenGL ES context for virglrenderer.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   1e7e92e2ef..fb4fe32d5b  master     -> master
 * [new tag]               patchew/20180410120222.31845-1-tournier.elie@gmail.com -> patchew/20180410120222.31845-1-tournier.elie@gmail.com
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
Switched to a new branch 'test'
fa8911b71b sdl: Allow OpenGL ES context creation
6bbe6ad468 qapi: Parameter gl of DisplayType now accept an enum

=== OUTPUT BEGIN ===
Checking PATCH 1/2: qapi: Parameter gl of DisplayType now accept an enum...
Checking PATCH 2/2: sdl: Allow OpenGL ES context creation...
WARNING: line over 80 characters
#44: FILE: ui/sdl2-gl.c:143:
+    if (scon->gl_mode ==  DISPLAYGL_MODE_ON || scon->gl_mode == DISPLAYGL_MODE_CORE)

ERROR: suspect code indent for conditional statements (4, 7)
#44: FILE: ui/sdl2-gl.c:143:
+    if (scon->gl_mode ==  DISPLAYGL_MODE_ON || scon->gl_mode == DISPLAYGL_MODE_CORE)
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,

ERROR: suspect code indent for conditional statements (4, 7)
#47: FILE: ui/sdl2-gl.c:146:
+    else if (scon->gl_mode == DISPLAYGL_MODE_ES)
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,

ERROR: suspect code indent for conditional statements (4, 7)
#58: FILE: ui/sdl2-gl.c:157:
+    if (!ctx && scon->gl_mode == DISPLAYGL_MODE_ON) {
+       SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK,

total: 3 errors, 1 warnings, 59 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Parameter gl of DisplayType now accept an enum

[Gerd Hoffmann]

> -    if (dpy.has_gl && dpy.gl && display_opengl == 0) {
> +    if (dpy.has_gl && !dpy.gl == DISPLAYGL_MODE_OFF && display_opengl == 0) {

That should be "... && !(dpy.gl == DISPLAYGL_MODE_OFF) && ..." to work
correctly.  Or just "dpy.gl != DISPLAYGL_MODE_OFF" ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 2/2] sdl: Allow OpenGL ES context creation

[Gerd Hoffmann]

On Tue, Apr 10, 2018 at 01:02:22PM +0100, Elie Tournier wrote:
> Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
> ---
>  include/ui/sdl2.h |  1 +
>  qemu-options.hx   |  2 +-
>  ui/sdl2-gl.c      | 17 +++++++++++++++--
>  ui/sdl2.c         |  1 +
>  vl.c              |  4 ++++
>  5 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
> index 51084e6320..8495795e48 100644
> --- a/include/ui/sdl2.h
> +++ b/include/ui/sdl2.h
> @@ -22,6 +22,7 @@ struct sdl2_console {
>      int x, y, w, h;
>      int hidden;
>      int opengl;
> +    DisplayGLMode gl_mode;

I'd suggest to move the "opts" global in sdl2.c to this struct instead,
then you can use scon->opts->gl.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Eric Blake]

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

On 04/10/2018 07:02 AM, Elie Tournier wrote:
> Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
> ---
>  qapi/ui.json | 21 ++++++++++++++++++++-
>  vl.c         | 10 +++++-----
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5d01ad4304..c8005867e5 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1019,6 +1019,25 @@
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool' } }
>  
> + ##
> + # @DisplayGLMode:
> + #
> + # Display OpenGL mode.
> + #
> + # 'off'   Disable OpenGL (default).

Wrong format; this should be:

# @off: Disable OpenGL (default).

> + # 'on'    Use OpenGL, pick context type automatically.
> + #         Would better be named 'auto' but is called 'on' for backward
> + #         compatibility with bool type.

See below...

> + # 'core'  Use OpenGL with Core (desktop) Context.
> + # 'es'    Use OpenGL with ES (embedded systems) Context.
> + #
> + # Since: 2.13
> + #
> + ##
> + { 'enum'    : 'DisplayGLMode',
> +   'data'    : [ 'off', 'on', 'core', 'es' ] }
> +
> +
>  ##
>  # @DisplayType:
>  #
> @@ -1048,7 +1067,7 @@
>    'base'    : { 'type'           : 'DisplayType',
>                  '*full-screen'   : 'bool',
>                  '*window-close'  : 'bool',
> -                '*gl'            : 'bool' },
> +                '*gl'            : 'DisplayGLMode' },

DisplayOptions was added in 2.12.  This is a backwards-incompatible
change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
releases, because the on-the-wire representation differs; pre-patch it
would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
and we want it, this patch either HAS to go in 2.12, or we have to have
more finesse (perhaps by using an 'alternate' type in the QAPI) so that
it remains backwards-compatible.

/me goes and looks at introspection output...

You may be in luck - there is no instance of 'window-close' in the
introspection output, which means 'DisplayType' exists only for ease of
command-line parsing and is not currently used by QMP, so tweaks here
are not affecting the command line.

That said, you can STILL name the enum value something smarter than 'on'
IF you make the change now, for 2.12, given that the QAPI type was only
introduced in 2.12 (you only have to worry about backwards compatibility
if 2.11 already parsed gl=on).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Gerd Hoffmann]

> # @off: Disable OpenGL (default).
> 
> > + # 'on'    Use OpenGL, pick context type automatically.
> > + #         Would better be named 'auto' but is called 'on' for backward
> > + #         compatibility with bool type.
> 
> See below...

> DisplayOptions was added in 2.12.  This is a backwards-incompatible
> change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
> releases, because the on-the-wire representation differs; pre-patch it
> would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
> and we want it, this patch either HAS to go in 2.12, or we have to have
> more finesse (perhaps by using an 'alternate' type in the QAPI) so that
> it remains backwards-compatible.
> 
> /me goes and looks at introspection output...
> 
> You may be in luck - there is no instance of 'window-close' in the
> introspection output, which means 'DisplayType' exists only for ease of
> command-line parsing and is not currently used by QMP, so tweaks here
> are not affecting the command line.

Yes, right now the struct is only used to store the parsed command line
opts, so no effect on QMP.

Plan for the future is to also parse command line options with generic
qapi/json code instead of the home-grown parser, but that switch didn't
happen yet.

> That said, you can STILL name the enum value something smarter than 'on'
> IF you make the change now, for 2.12, given that the QAPI type was only
> introduced in 2.12 (you only have to worry about backwards compatibility
> if 2.11 already parsed gl=on).

gl=on is older, so that must continue to work.  Making both "on" and
"auto" work isn't a problem for our home-grown parser (aka
parse_display() in vl.c).  But having quirks like this makes the switch
to generic parser code more difficuilt, so I'd prefer to avoid that ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Elie Tournier]

On Tue, Apr 10, 2018 at 08:13:00AM -0500, Eric Blake wrote:
> On 04/10/2018 07:02 AM, Elie Tournier wrote:
> > Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
> > ---
> >  qapi/ui.json | 21 ++++++++++++++++++++-
> >  vl.c         | 10 +++++-----
> >  2 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 5d01ad4304..c8005867e5 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1019,6 +1019,25 @@
> >  { 'struct'  : 'DisplayGTK',
> >    'data'    : { '*grab-on-hover' : 'bool' } }
> >  
> > + ##
> > + # @DisplayGLMode:
> > + #
> > + # Display OpenGL mode.
> > + #
> > + # 'off'   Disable OpenGL (default).
> 
> Wrong format; this should be:
> 
> # @off: Disable OpenGL (default).

Fix locally
> 
> > + # 'on'    Use OpenGL, pick context type automatically.
> > + #         Would better be named 'auto' but is called 'on' for backward
> > + #         compatibility with bool type.
> 
> See below...
> 
> > + # 'core'  Use OpenGL with Core (desktop) Context.
> > + # 'es'    Use OpenGL with ES (embedded systems) Context.
> > + #
> > + # Since: 2.13
> > + #
> > + ##
> > + { 'enum'    : 'DisplayGLMode',
> > +   'data'    : [ 'off', 'on', 'core', 'es' ] }
> > +
> > +
> >  ##
> >  # @DisplayType:
> >  #
> > @@ -1048,7 +1067,7 @@
> >    'base'    : { 'type'           : 'DisplayType',
> >                  '*full-screen'   : 'bool',
> >                  '*window-close'  : 'bool',
> > -                '*gl'            : 'bool' },
> > +                '*gl'            : 'DisplayGLMode' },
> 
> DisplayOptions was added in 2.12.  This is a backwards-incompatible
> change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
> releases, because the on-the-wire representation differs; pre-patch it
> would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
> and we want it, this patch either HAS to go in 2.12, or we have to have
> more finesse (perhaps by using an 'alternate' type in the QAPI) so that
> it remains backwards-compatible.
> 
> /me goes and looks at introspection output...
> 
> You may be in luck - there is no instance of 'window-close' in the
> introspection output, which means 'DisplayType' exists only for ease of
> command-line parsing and is not currently used by QMP, so tweaks here
> are not affecting the command line.
> 
> That said, you can STILL name the enum value something smarter than 'on'
> IF you make the change now, for 2.12, given that the QAPI type was only
> introduced in 2.12 (you only have to worry about backwards compatibility
> if 2.11 already parsed gl=on).

It sounds like making the change now is the best option.
So, is 'auto' a better choice than 'on' in the enum?
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Elie Tournier]

Hello,

On Tue, Apr 10, 2018 at 03:33:35PM +0200, Gerd Hoffmann wrote:
> > # @off: Disable OpenGL (default).

Just to be sure, I have to add @ in front of all parameter, right?

> > 
> > > + # 'on'    Use OpenGL, pick context type automatically.
> > > + #         Would better be named 'auto' but is called 'on' for backward
> > > + #         compatibility with bool type.
> > 
> > See below...
> 
> > DisplayOptions was added in 2.12.  This is a backwards-incompatible
> > change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
> > releases, because the on-the-wire representation differs; pre-patch it
> > would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
> > and we want it, this patch either HAS to go in 2.12, or we have to have
> > more finesse (perhaps by using an 'alternate' type in the QAPI) so that
> > it remains backwards-compatible.
> > 
> > /me goes and looks at introspection output...
> > 
> > You may be in luck - there is no instance of 'window-close' in the
> > introspection output, which means 'DisplayType' exists only for ease of
> > command-line parsing and is not currently used by QMP, so tweaks here
> > are not affecting the command line.
> 
> Yes, right now the struct is only used to store the parsed command line
> opts, so no effect on QMP.
> 
> Plan for the future is to also parse command line options with generic
> qapi/json code instead of the home-grown parser, but that switch didn't
> happen yet.
> 
> > That said, you can STILL name the enum value something smarter than 'on'
> > IF you make the change now, for 2.12, given that the QAPI type was only
> > introduced in 2.12 (you only have to worry about backwards compatibility
> > if 2.11 already parsed gl=on).
> 
> gl=on is older, so that must continue to work.  Making both "on" and
> "auto" work isn't a problem for our home-grown parser (aka
> parse_display() in vl.c).  But having quirks like this makes the switch
> to generic parser code more difficuilt, so I'd prefer to avoid that ...

Is it possible to upstream this change before 2.12 release?

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Eric Blake]

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

On 04/12/2018 09:11 AM, Elie Tournier wrote:
> Hello,
> 
> On Tue, Apr 10, 2018 at 03:33:35PM +0200, Gerd Hoffmann wrote:
>>> # @off: Disable OpenGL (default).
> 
> Just to be sure, I have to add @ in front of all parameter, right?

Yes.


>>> You may be in luck - there is no instance of 'window-close' in the
>>> introspection output, which means 'DisplayType' exists only for ease of
>>> command-line parsing and is not currently used by QMP, so tweaks here
>>> are not affecting the command line.
>>
>> Yes, right now the struct is only used to store the parsed command line
>> opts, so no effect on QMP.
>>
>> Plan for the future is to also parse command line options with generic
>> qapi/json code instead of the home-grown parser, but that switch didn't
>> happen yet.
>>
>>> That said, you can STILL name the enum value something smarter than 'on'
>>> IF you make the change now, for 2.12, given that the QAPI type was only
>>> introduced in 2.12 (you only have to worry about backwards compatibility
>>> if 2.11 already parsed gl=on).
>>
>> gl=on is older, so that must continue to work.  Making both "on" and
>> "auto" work isn't a problem for our home-grown parser (aka
>> parse_display() in vl.c).  But having quirks like this makes the switch
>> to generic parser code more difficuilt, so I'd prefer to avoid that ...
> 
> Is it possible to upstream this change before 2.12 release?

At this point, no, we've missed -rc3, and it's not a big enough bug to
warrant needing -rc4 on its own.  And as Gerd said, we already parsed
'gl=on' in 2.11 (the QAPI representation is new to 2.12, but only to
simplify how the existing command line parser was coded), so that's
different than if 2.12 were introducing brand-new content in a form that
should be fixed before it is baked into existing uses.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Gerd Hoffmann]

On Thu, Apr 12, 2018 at 03:11:53PM +0100, Elie Tournier wrote:
> Hello,
> 
> On Tue, Apr 10, 2018 at 03:33:35PM +0200, Gerd Hoffmann wrote:
> > > # @off: Disable OpenGL (default).
> 
> Just to be sure, I have to add @ in front of all parameter, right?
> 
> > > 
> > > > + # 'on'    Use OpenGL, pick context type automatically.
> > > > + #         Would better be named 'auto' but is called 'on' for backward
> > > > + #         compatibility with bool type.
> > > 
> > > See below...
> > 
> > > DisplayOptions was added in 2.12.  This is a backwards-incompatible
> > > change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
> > > releases, because the on-the-wire representation differs; pre-patch it
> > > would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
> > > and we want it, this patch either HAS to go in 2.12, or we have to have
> > > more finesse (perhaps by using an 'alternate' type in the QAPI) so that
> > > it remains backwards-compatible.
> > > 
> > > /me goes and looks at introspection output...
> > > 
> > > You may be in luck - there is no instance of 'window-close' in the
> > > introspection output, which means 'DisplayType' exists only for ease of
> > > command-line parsing and is not currently used by QMP, so tweaks here
> > > are not affecting the command line.
> > 
> > Yes, right now the struct is only used to store the parsed command line
> > opts, so no effect on QMP.
> > 
> > Plan for the future is to also parse command line options with generic
> > qapi/json code instead of the home-grown parser, but that switch didn't
> > happen yet.
> > 
> > > That said, you can STILL name the enum value something smarter than 'on'
> > > IF you make the change now, for 2.12, given that the QAPI type was only
> > > introduced in 2.12 (you only have to worry about backwards compatibility
> > > if 2.11 already parsed gl=on).
> > 
> > gl=on is older, so that must continue to work.  Making both "on" and
> > "auto" work isn't a problem for our home-grown parser (aka
> > parse_display() in vl.c).  But having quirks like this makes the switch
> > to generic parser code more difficuilt, so I'd prefer to avoid that ...
> 
> Is it possible to upstream this change before 2.12 release?

No need to hurry with this.  QMP doesn't see the structs, so no
compatibility problem here.  gl=on cmd line option was present in 2.11 +
older already, so that must continue to work no matter whenever this
makes 2.12 or not.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Elie Tournier]

On Fri, Apr 13, 2018 at 08:15:29AM +0200, Gerd Hoffmann wrote:
> On Thu, Apr 12, 2018 at 03:11:53PM +0100, Elie Tournier wrote:
> > Hello,
> > 
> > On Tue, Apr 10, 2018 at 03:33:35PM +0200, Gerd Hoffmann wrote:
> > > > # @off: Disable OpenGL (default).
> > 
> > Just to be sure, I have to add @ in front of all parameter, right?
> > 
> > > > 
> > > > > + # 'on'    Use OpenGL, pick context type automatically.
> > > > > + #         Would better be named 'auto' but is called 'on' for backward
> > > > > + #         compatibility with bool type.
> > > > 
> > > > See below...
> > > 
> > > > DisplayOptions was added in 2.12.  This is a backwards-incompatible
> > > > change in QMP (you CANNOT change 'bool' to 'DisplayGLMode' across
> > > > releases, because the on-the-wire representation differs; pre-patch it
> > > > would be "gl":true, post-patch it is "gl":"on").  So if it affects QMP,
> > > > and we want it, this patch either HAS to go in 2.12, or we have to have
> > > > more finesse (perhaps by using an 'alternate' type in the QAPI) so that
> > > > it remains backwards-compatible.
> > > > 
> > > > /me goes and looks at introspection output...
> > > > 
> > > > You may be in luck - there is no instance of 'window-close' in the
> > > > introspection output, which means 'DisplayType' exists only for ease of
> > > > command-line parsing and is not currently used by QMP, so tweaks here
> > > > are not affecting the command line.
> > > 
> > > Yes, right now the struct is only used to store the parsed command line
> > > opts, so no effect on QMP.
> > > 
> > > Plan for the future is to also parse command line options with generic
> > > qapi/json code instead of the home-grown parser, but that switch didn't
> > > happen yet.
> > > 
> > > > That said, you can STILL name the enum value something smarter than 'on'
> > > > IF you make the change now, for 2.12, given that the QAPI type was only
> > > > introduced in 2.12 (you only have to worry about backwards compatibility
> > > > if 2.11 already parsed gl=on).
> > > 
> > > gl=on is older, so that must continue to work.  Making both "on" and
> > > "auto" work isn't a problem for our home-grown parser (aka
> > > parse_display() in vl.c).  But having quirks like this makes the switch
> > > to generic parser code more difficuilt, so I'd prefer to avoid that ...
> > 
> > Is it possible to upstream this change before 2.12 release?
> 
> No need to hurry with this.  QMP doesn't see the structs, so no
> compatibility problem here.  gl=on cmd line option was present in 2.11 +
> older already, so that must continue to work no matter whenever this
> makes 2.12 or not.

Alright, thanks.
Well, to be fair, I just want to see this feature upstream, I don't really care
about the release number.

Just a question before I resend the series.
Should I note "Since 2.12" or "Since 2.13" in qapi/ui.json?

BR,
Elie

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH v2 1/2 for-2.12?] qapi: Parameter gl of DisplayType now accept an enum

[Gerd Hoffmann]

> > No need to hurry with this.  QMP doesn't see the structs, so no
> > compatibility problem here.  gl=on cmd line option was present in 2.11 +
> > older already, so that must continue to work no matter whenever this
> > makes 2.12 or not.
> 
> Alright, thanks.
> Well, to be fair, I just want to see this feature upstream, I don't really care
> about the release number.
> 
> Just a question before I resend the series.
> Should I note "Since 2.12" or "Since 2.13" in qapi/ui.json?

2.13 please.

thanks,
  Gerd