| Message ID | 1457955672-28758-1-git-send-email-cfergeau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 14, 2016 at 12:41 PM, Christophe Fergeau <cfergeau@redhat.com> wrote: > Currently, virgl support has to go through a local unix socket, trying > to connect to a VM using -spice gl through spice://localhost:5900 will > only result in a black screen. > This commit errors out when the user tries to start a VM with both GL > support and a port/tls-port set. > This would fit better in spice-server, but currently QEMU does not call > into spice-server when parsing 'gl' on its command line, so we have to > do this check in QEMU instead. > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > ui/spice-core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/ui/spice-core.c b/ui/spice-core.c > index 7987a4e..94f3236 100644 > --- a/ui/spice-core.c > +++ b/ui/spice-core.c > @@ -844,6 +844,10 @@ void qemu_spice_init(void) > > #ifdef HAVE_SPICE_GL > if (qemu_opt_get_bool(opts, "gl", 0)) { > + if ((port != 0) || (tls_port != 0)) { > + error_report("SPICE GL support is local-only for now and incompatible with -spice port/tls-port"); > + exit(1); > + } > if (egl_rendernode_init() == 0) { > display_opengl = 1; > } > -- > 2.5.0 >
On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote: > Currently, virgl support has to go through a local unix socket, trying > to connect to a VM using -spice gl through spice://localhost:5900 will > only result in a black screen. > This commit errors out when the user tries to start a VM with both GL > support and a port/tls-port set. > This would fit better in spice-server, but currently QEMU does not call > into spice-server when parsing 'gl' on its command line, so we have to > do this check in QEMU instead. Hmm. It's something which we want support long-term though, by encoding those dma-bufs as video stream and send them off over tcp. I don't think this is a good idea long-term. And even as temporary stopgap: Can libvirt + virt-manager handle this? cheers, Gerd
Hi, On Mon, Mar 14, 2016 at 04:00:11PM +0100, Gerd Hoffmann wrote: > On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote: > > Currently, virgl support has to go through a local unix socket, trying > > to connect to a VM using -spice gl through spice://localhost:5900 will > > only result in a black screen. > > This commit errors out when the user tries to start a VM with both GL > > support and a port/tls-port set. > > This would fit better in spice-server, but currently QEMU does not call > > into spice-server when parsing 'gl' on its command line, so we have to > > do this check in QEMU instead. > > Hmm. It's something which we want support long-term though, by encoding > those dma-bufs as video stream and send them off over tcp. > > I don't think this is a good idea long-term. Yes, long-term we will want to remove this once spice gets support for this. Currently however, this imo makes it a bit easier to understand how everything is setup. Otherwise I expect people are going to just take their existing SPICE libvirt configuration listening on the network, add <gl enable="yes"/> to it, try remote-viewer spice://localhost:5900 and wonder why they get a black screen, and not know whether this is because their guest does not support virgl, or their host, or because of some other issue, ... Having this as a stopgap ensures that they at least be informed this is not a valid usecase. > And even as temporary stopgap: Can libvirt + virt-manager handle this? libvirt can, I haven't tried virt-manager (I believe all the patches from Marc-André haven't been pushed yet) <spice autoport="no"/> starts QEMU with -spice port=0 Christophe
On 03/14/2016 05:41 AM, Christophe Fergeau wrote: > Currently, virgl support has to go through a local unix socket, trying > to connect to a VM using -spice gl through spice://localhost:5900 will > only result in a black screen. > This commit errors out when the user tries to start a VM with both GL > support and a port/tls-port set. > This would fit better in spice-server, but currently QEMU does not call > into spice-server when parsing 'gl' on its command line, so we have to > do this check in QEMU instead. > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > --- > ui/spice-core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/ui/spice-core.c b/ui/spice-core.c > index 7987a4e..94f3236 100644 > --- a/ui/spice-core.c > +++ b/ui/spice-core.c > @@ -844,6 +844,10 @@ void qemu_spice_init(void) > > #ifdef HAVE_SPICE_GL > if (qemu_opt_get_bool(opts, "gl", 0)) { > + if ((port != 0) || (tls_port != 0)) { Overparenthesized; you could write: if (port || tls_port) { for the same effect with less typing.
On Mon, Mar 14, 2016 at 09:41:34AM -0600, Eric Blake wrote: > On 03/14/2016 05:41 AM, Christophe Fergeau wrote: > > Currently, virgl support has to go through a local unix socket, trying > > to connect to a VM using -spice gl through spice://localhost:5900 will > > only result in a black screen. > > This commit errors out when the user tries to start a VM with both GL > > support and a port/tls-port set. > > This would fit better in spice-server, but currently QEMU does not call > > into spice-server when parsing 'gl' on its command line, so we have to > > do this check in QEMU instead. > > > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> > > --- > > ui/spice-core.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/ui/spice-core.c b/ui/spice-core.c > > index 7987a4e..94f3236 100644 > > --- a/ui/spice-core.c > > +++ b/ui/spice-core.c > > @@ -844,6 +844,10 @@ void qemu_spice_init(void) > > > > #ifdef HAVE_SPICE_GL > > if (qemu_opt_get_bool(opts, "gl", 0)) { > > + if ((port != 0) || (tls_port != 0)) { > > Overparenthesized; you could write: > > if (port || tls_port) { > > for the same effect with less typing. Yeah I know I'm overly verbose with these tests, the parentheses make it explicit that there are no operator priority issues, the != 0 emphasize it's an integer type which is being handled. I'll change it to your recommendation before sending a v2. Christophe
Hi, > Yes, long-term we will want to remove this once spice gets support for > this. How can qemu figure whenever spice supports gl+tcp or not? cheers, Gerd
On Tue, Mar 15, 2016 at 02:09:47PM +0100, Gerd Hoffmann wrote: > Hi, > > > Yes, long-term we will want to remove this once spice gets support for > > this. > > How can qemu figure whenever spice supports gl+tcp or not? gl support is already enabled through a spice-server version check #if defined(CONFIG_OPENGL_DMABUF) # if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */ # define HAVE_SPICE_GL 1 # include "ui/egl-helpers.h" # include "ui/egl-context.h" # endif #endif We can do something similar once gl+tcp is available. Christophe
On Di, 2016-03-15 at 15:17 +0100, Christophe Fergeau wrote: > On Tue, Mar 15, 2016 at 02:09:47PM +0100, Gerd Hoffmann wrote: > > Hi, > > > > > Yes, long-term we will want to remove this once spice gets support for > > > this. > > > > How can qemu figure whenever spice supports gl+tcp or not? > > gl support is already enabled through a spice-server version check > > #if defined(CONFIG_OPENGL_DMABUF) > # if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */ > # define HAVE_SPICE_GL 1 > # include "ui/egl-helpers.h" > # include "ui/egl-context.h" > # endif > #endif Sure, qemu needed to figure whenever spice-server provides the gl API. > We can do something similar once gl+tcp is available. I don't expect adding gl+tcp support to spice needs changes in the spice-server API and qemu. So ifdef'ing on the spice-server version is bogous, especially as the version you compiled qemu against can be newer or older as the version used to run qemu. cheers, Gerd
On Tue, Mar 15, 2016 at 03:32:31PM +0100, Gerd Hoffmann wrote: > > We can do something similar once gl+tcp is available. > > I don't expect adding gl+tcp support to spice needs changes in the > spice-server API and qemu. So ifdef'ing on the spice-server version is > bogous, Hmm, I expected some changes, at least wrt options if the user needs to tweak the format of the video stream, hence the suggestion :) Things get trickier then. Adding a runtime spice_get_version() would not be that great either as the check would have to be updated with each spice-server release. I'd still like to have some failure when people try such configurations, gathering all the pieces is complicated enough, better to let people know when they try doing something that won't work. Christophe
On Mo, 2016-03-14 at 12:41 +0100, Christophe Fergeau wrote: > Currently, virgl support has to go through a local unix socket, trying > to connect to a VM using -spice gl through spice://localhost:5900 will > only result in a black screen. > This commit errors out when the user tries to start a VM with both GL > support and a port/tls-port set. > This would fit better in spice-server, but currently QEMU does not call > into spice-server when parsing 'gl' on its command line, so we have to > do this check in QEMU instead. > > Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> Picked up for ui patch queue. thanks, Gerd
diff --git a/ui/spice-core.c b/ui/spice-core.c index 7987a4e..94f3236 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -844,6 +844,10 @@ void qemu_spice_init(void) #ifdef HAVE_SPICE_GL if (qemu_opt_get_bool(opts, "gl", 0)) { + if ((port != 0) || (tls_port != 0)) { + error_report("SPICE GL support is local-only for now and incompatible with -spice port/tls-port"); + exit(1); + } if (egl_rendernode_init() == 0) { display_opengl = 1; }
Currently, virgl support has to go through a local unix socket, trying to connect to a VM using -spice gl through spice://localhost:5900 will only result in a black screen. This commit errors out when the user tries to start a VM with both GL support and a port/tls-port set. This would fit better in spice-server, but currently QEMU does not call into spice-server when parsing 'gl' on its command line, so we have to do this check in QEMU instead. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> --- ui/spice-core.c | 4 ++++ 1 file changed, 4 insertions(+)