[PATCH v1 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[PATCH v1 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Vivek Kasireddy]

To address the limitation that this option is incompatible with
remote clients, this patch series adds an option to select a
preferred codec and also enable gl=on option for clients that
are connected via the network. In other words, with this option
enabled (and the below linked Spice series merged), it would be
possible to have Qemu share a dmabuf fd with Spice, which would
then forward it to a hardware or software based encoder and
eventually send the data associated with the fd to a client that
could be located on a different machine.

Tested with: -device virtio-vga,max_outputs=1,xres=1920,yres=1080
             -spice port=3001,gl=on,disable-ticketing=on,preferred-codec=gstreamer:h264

and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.

---
Associated Spice server patches can be found here:
https://lists.freedesktop.org/archives/spice-devel/2023-December/053288.html

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>

Vivek Kasireddy (7):
  ui/spice: Add an option for users to provide a preferred codec
  ui/spice: Enable gl=on option for non-local or remote clients
  ui/spice: Submit the gl_draw requests at 60 FPS for remote clients
  ui/console-gl: Add an option to override a surface's glformat
  ui/spice: Override the surface's glformat when gl=on is enabled
  ui/console-gl: Add a helper to create a texture with linear memory
    layout
  ui/spice: Create another texture with linear layout when gl=on is
    enabled

 include/ui/console.h       |  2 +
 include/ui/spice-display.h |  1 +
 include/ui/surface.h       |  1 +
 qemu-options.hx            |  5 +++
 ui/console-gl.c            | 32 ++++++++++++++++
 ui/spice-core.c            | 16 ++++++++
 ui/spice-display.c         | 75 +++++++++++++++++++++++++++++++++-----
 7 files changed, 122 insertions(+), 10 deletions(-)

-- 
2.39.2

[PATCH v1 1/7] ui/spice: Add an option for users to provide a preferred codec

[Vivek Kasireddy]

Giving users an option to choose a particular codec will enable
them to make an appropriate decision based on their hardware and
use-case.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

---
v2:
- Don't override the default Spice codec if preferred-codec is not
  provided (Frediano)
---
 qemu-options.hx |  5 +++++
 ui/spice-core.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index b66570ae00..caaafe01d5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2260,6 +2260,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
     "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
     "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
+    "       [,preferred-codec=<encoder>:<codec>\n"
     "       [,gl=[on|off]][,rendernode=<file>]\n"
     "                enable spice\n"
     "                at least one of {port, tls-port} is mandatory\n",
@@ -2348,6 +2349,10 @@ SRST
     ``seamless-migration=[on|off]``
         Enable/disable spice seamless migration. Default is off.
 
+    ``preferred-codec=<encoder>:<codec>``
+        Provide the preferred codec the Spice server should use.
+        Default would be spice:mjpeg.
+
     ``gl=[on|off]``
         Enable/disable OpenGL context. Default is off.
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index db21db2c94..13bfbe4e89 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -488,6 +488,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "streaming-video",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "preferred-codec",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "agent-mouse",
             .type = QEMU_OPT_BOOL,
@@ -663,6 +666,7 @@ static void qemu_spice_init(void)
     char *x509_key_file = NULL,
         *x509_cert_file = NULL,
         *x509_cacert_file = NULL;
+    const char *preferred_codec = NULL;
     int port, tls_port, addr_flags;
     spice_image_compression_t compression;
     spice_wan_compression_t wan_compr;
@@ -802,6 +806,14 @@ static void qemu_spice_init(void)
         spice_server_set_streaming_video(spice_server, SPICE_STREAM_VIDEO_OFF);
     }
 
+    preferred_codec = qemu_opt_get(opts, "preferred-codec");
+    if (preferred_codec) {
+        if (spice_server_set_video_codecs(spice_server, preferred_codec)) {
+            error_report("Preferred codec name is not valid");
+            exit(1);
+        }
+    }
+
     spice_server_set_agent_mouse
         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
     spice_server_set_playback_compression
-- 
2.39.2

Re: [PATCH v1 1/7] ui/spice: Add an option for users to provide a preferred codec

[Marc-André Lureau]

Hi

On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> Giving users an option to choose a particular codec will enable
> them to make an appropriate decision based on their hardware and
> use-case.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>
> ---
> v2:
> - Don't override the default Spice codec if preferred-codec is not
>   provided (Frediano)
> ---
>  qemu-options.hx |  5 +++++
>  ui/spice-core.c | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b66570ae00..caaafe01d5 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2260,6 +2260,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
>      "       [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
>      "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
>      "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
> +    "       [,preferred-codec=<encoder>:<codec>\n"

The SPICE API is "spice_server_set_video_codecs()", let's name the
option: "video-codecs" to avoid confusions.

>      "       [,gl=[on|off]][,rendernode=<file>]\n"
>      "                enable spice\n"
>      "                at least one of {port, tls-port} is mandatory\n",
> @@ -2348,6 +2349,10 @@ SRST
>      ``seamless-migration=[on|off]``
>          Enable/disable spice seamless migration. Default is off.
>
> +    ``preferred-codec=<encoder>:<codec>``
> +        Provide the preferred codec the Spice server should use.
> +        Default would be spice:mjpeg.

The SPICE API says:
 * @codecs: a codec string in the following format: encoder:codec;encoder:codec

But the doc doesn't say whether the order is important, and doesn't
give more details on the "encoder:codec" format.

Also reading the code, it seems "auto" has a special meaning for
default video codecs.

> +
>      ``gl=[on|off]``
>          Enable/disable OpenGL context. Default is off.
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index db21db2c94..13bfbe4e89 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -488,6 +488,9 @@ static QemuOptsList qemu_spice_opts = {
>          },{
>              .name = "streaming-video",
>              .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "preferred-codec",
> +            .type = QEMU_OPT_STRING,
>          },{
>              .name = "agent-mouse",
>              .type = QEMU_OPT_BOOL,
> @@ -663,6 +666,7 @@ static void qemu_spice_init(void)
>      char *x509_key_file = NULL,
>          *x509_cert_file = NULL,
>          *x509_cacert_file = NULL;
> +    const char *preferred_codec = NULL;
>      int port, tls_port, addr_flags;
>      spice_image_compression_t compression;
>      spice_wan_compression_t wan_compr;
> @@ -802,6 +806,14 @@ static void qemu_spice_init(void)
>          spice_server_set_streaming_video(spice_server, SPICE_STREAM_VIDEO_OFF);
>      }
>
> +    preferred_codec = qemu_opt_get(opts, "preferred-codec");
> +    if (preferred_codec) {
> +        if (spice_server_set_video_codecs(spice_server, preferred_codec)) {

Sadly, the API just returns 0 if one of the codec was accepted, not
great if you want a specific set of codecs.

otherwise, lgtm


> +            error_report("Preferred codec name is not valid");
> +            exit(1);
> +        }
> +    }
> +
>      spice_server_set_agent_mouse
>          (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
>      spice_server_set_playback_compression
> --
> 2.39.2
>
>


--
Marc-André Lureau

RE: [PATCH v1 1/7] ui/spice: Add an option for users to provide a preferred codec

[Kasireddy, Vivek]

Hi Marc-Andre,

> 
> Hi
> 
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> <vivek.kasireddy@intel.com> wrote:
> >
> > Giving users an option to choose a particular codec will enable
> > them to make an appropriate decision based on their hardware and
> > use-case.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Frediano Ziglio <freddy77@gmail.com>
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >
> > ---
> > v2:
> > - Don't override the default Spice codec if preferred-codec is not
> >   provided (Frediano)
> > ---
> >  qemu-options.hx |  5 +++++
> >  ui/spice-core.c | 12 ++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index b66570ae00..caaafe01d5 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2260,6 +2260,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
> >      "       [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
> >      "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
> >      "       [,playback-compression=[on|off]][,seamless-
> migration=[on|off]]\n"
> > +    "       [,preferred-codec=<encoder>:<codec>\n"
> 
> The SPICE API is "spice_server_set_video_codecs()", let's name the
> option: "video-codecs" to avoid confusions.
I am ok with "video-codes" instead of "preferred-codec".

> 
> >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> >      "                enable spice\n"
> >      "                at least one of {port, tls-port} is mandatory\n",
> > @@ -2348,6 +2349,10 @@ SRST
> >      ``seamless-migration=[on|off]``
> >          Enable/disable spice seamless migration. Default is off.
> >
> > +    ``preferred-codec=<encoder>:<codec>``
> > +        Provide the preferred codec the Spice server should use.
> > +        Default would be spice:mjpeg.
> 
> The SPICE API says:
>  * @codecs: a codec string in the following format:
> encoder:codec;encoder:codec
> 
> But the doc doesn't say whether the order is important, and doesn't
> give more details on the "encoder:codec" format.
> 
> Also reading the code, it seems "auto" has a special meaning for
> default video codecs.
Although, I am using spice_server_set_video_codecs() API, my initial goal
with this option is for the user to specify one <encoder>:<codec> entry
only. I guess it might be OK to have the user specify a list as long as Spice picks
the first entry (aka preferred) and falls back to the next if it cannot create or
use the encoder associated with the first entry.

> 
> > +
> >      ``gl=[on|off]``
> >          Enable/disable OpenGL context. Default is off.
> >
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index db21db2c94..13bfbe4e89 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -488,6 +488,9 @@ static QemuOptsList qemu_spice_opts = {
> >          },{
> >              .name = "streaming-video",
> >              .type = QEMU_OPT_STRING,
> > +        },{
> > +            .name = "preferred-codec",
> > +            .type = QEMU_OPT_STRING,
> >          },{
> >              .name = "agent-mouse",
> >              .type = QEMU_OPT_BOOL,
> > @@ -663,6 +666,7 @@ static void qemu_spice_init(void)
> >      char *x509_key_file = NULL,
> >          *x509_cert_file = NULL,
> >          *x509_cacert_file = NULL;
> > +    const char *preferred_codec = NULL;
> >      int port, tls_port, addr_flags;
> >      spice_image_compression_t compression;
> >      spice_wan_compression_t wan_compr;
> > @@ -802,6 +806,14 @@ static void qemu_spice_init(void)
> >          spice_server_set_streaming_video(spice_server,
> SPICE_STREAM_VIDEO_OFF);
> >      }
> >
> > +    preferred_codec = qemu_opt_get(opts, "preferred-codec");
> > +    if (preferred_codec) {
> > +        if (spice_server_set_video_codecs(spice_server, preferred_codec)) {
> 
> Sadly, the API just returns 0 if one of the codec was accepted, not
> great if you want a specific set of codecs.
IIUC, although a user can specify a set of codecs, only one can by actively used at
any given time (at-least with Gstreamer ones, not Spice codecs) with my use-case (gl=on).

Thanks,
Vivek

> 
> otherwise, lgtm
> 
> 
> > +            error_report("Preferred codec name is not valid");
> > +            exit(1);
> > +        }
> > +    }
> > +
> >      spice_server_set_agent_mouse
> >          (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
> >      spice_server_set_playback_compression
> > --
> > 2.39.2
> >
> >
> 
> 
> --
> Marc-André Lureau

[PATCH v1 2/7] ui/spice: Enable gl=on option for non-local or remote clients

[Vivek Kasireddy]

Newer versions of Spice server should be able to accept dmabuf
fds from Qemu for clients that are connected via the network.
In other words, when this option is enabled, Qemu would share
a dmabuf fd with Spice which would encode and send the data
associated with the fd to a client that could be located on
a different machine.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h | 1 +
 ui/spice-core.c            | 4 ++++
 ui/spice-display.c         | 1 +
 3 files changed, 6 insertions(+)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index e1a9b36185..f4922dd74b 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -151,6 +151,7 @@ struct SimpleSpiceCursor {
 };
 
 extern bool spice_opengl;
+extern bool remote_client;
 
 int qemu_spice_rect_is_empty(const QXLRect* r);
 void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 13bfbe4e89..3b9a54685f 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -849,9 +849,13 @@ static void qemu_spice_init(void)
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
         if ((port != 0) || (tls_port != 0)) {
+#if SPICE_SERVER_VERSION >= 0x000f03 /* release 0.15.3 */
+            remote_client = 1;
+#else
             error_report("SPICE GL support is local-only for now and "
                          "incompatible with -spice port/tls-port");
             exit(1);
+#endif
         }
         egl_init(qemu_opt_get(opts, "rendernode"), DISPLAYGL_MODE_ON, &error_fatal);
         spice_opengl = 1;
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6eb98a5a5c..384b8508d4 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -29,6 +29,7 @@
 #include "ui/spice-display.h"
 
 bool spice_opengl;
+bool remote_client;
 
 int qemu_spice_rect_is_empty(const QXLRect* r)
 {
-- 
2.39.2

Re: [PATCH v1 2/7] ui/spice: Enable gl=on option for non-local or remote clients

[Marc-André Lureau]

Hi

On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> Newer versions of Spice server should be able to accept dmabuf
> fds from Qemu for clients that are connected via the network.
> In other words, when this option is enabled, Qemu would share
> a dmabuf fd with Spice which would encode and send the data
> associated with the fd to a client that could be located on
> a different machine.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  include/ui/spice-display.h | 1 +
>  ui/spice-core.c            | 4 ++++
>  ui/spice-display.c         | 1 +
>  3 files changed, 6 insertions(+)
>
> diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> index e1a9b36185..f4922dd74b 100644
> --- a/include/ui/spice-display.h
> +++ b/include/ui/spice-display.h
> @@ -151,6 +151,7 @@ struct SimpleSpiceCursor {
>  };
>
>  extern bool spice_opengl;
> +extern bool remote_client;
>
>  int qemu_spice_rect_is_empty(const QXLRect* r);
>  void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 13bfbe4e89..3b9a54685f 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -849,9 +849,13 @@ static void qemu_spice_init(void)
>  #ifdef HAVE_SPICE_GL
>      if (qemu_opt_get_bool(opts, "gl", 0)) {
>          if ((port != 0) || (tls_port != 0)) {
> +#if SPICE_SERVER_VERSION >= 0x000f03 /* release 0.15.3 */

(ok, we should wait for the Spice series to be merged)


> +            remote_client = 1;
> +#else
>              error_report("SPICE GL support is local-only for now and "
>                           "incompatible with -spice port/tls-port");
>              exit(1);
> +#endif
>          }
>          egl_init(qemu_opt_get(opts, "rendernode"), DISPLAYGL_MODE_ON, &error_fatal);
>          spice_opengl = 1;
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 6eb98a5a5c..384b8508d4 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -29,6 +29,7 @@
>  #include "ui/spice-display.h"
>
>  bool spice_opengl;
> +bool remote_client;
>
>  int qemu_spice_rect_is_empty(const QXLRect* r)
>  {
> --
> 2.39.2
>
>


--
Marc-André Lureau

RE: [PATCH v1 2/7] ui/spice: Enable gl=on option for non-local or remote clients

[Kasireddy, Vivek]

Hi Marc-Andre,

> 
> Hi
> 
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> <vivek.kasireddy@intel.com> wrote:
> >
> > Newer versions of Spice server should be able to accept dmabuf
> > fds from Qemu for clients that are connected via the network.
> > In other words, when this option is enabled, Qemu would share
> > a dmabuf fd with Spice which would encode and send the data
> > associated with the fd to a client that could be located on
> > a different machine.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Frediano Ziglio <freddy77@gmail.com>
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  include/ui/spice-display.h | 1 +
> >  ui/spice-core.c            | 4 ++++
> >  ui/spice-display.c         | 1 +
> >  3 files changed, 6 insertions(+)
> >
> > diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> > index e1a9b36185..f4922dd74b 100644
> > --- a/include/ui/spice-display.h
> > +++ b/include/ui/spice-display.h
> > @@ -151,6 +151,7 @@ struct SimpleSpiceCursor {
> >  };
> >
> >  extern bool spice_opengl;
> > +extern bool remote_client;
> >
> >  int qemu_spice_rect_is_empty(const QXLRect* r);
> >  void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 13bfbe4e89..3b9a54685f 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -849,9 +849,13 @@ static void qemu_spice_init(void)
> >  #ifdef HAVE_SPICE_GL
> >      if (qemu_opt_get_bool(opts, "gl", 0)) {
> >          if ((port != 0) || (tls_port != 0)) {
> > +#if SPICE_SERVER_VERSION >= 0x000f03 /* release 0.15.3 */
> 
> (ok, we should wait for the Spice series to be merged)
Sure, I'll post the v2 after the Spice series gets merged.

Thanks,
Vivek

> 
> 
> > +            remote_client = 1;
> > +#else
> >              error_report("SPICE GL support is local-only for now and "
> >                           "incompatible with -spice port/tls-port");
> >              exit(1);
> > +#endif
> >          }
> >          egl_init(qemu_opt_get(opts, "rendernode"), DISPLAYGL_MODE_ON,
> &error_fatal);
> >          spice_opengl = 1;
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 6eb98a5a5c..384b8508d4 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -29,6 +29,7 @@
> >  #include "ui/spice-display.h"
> >
> >  bool spice_opengl;
> > +bool remote_client;
> >
> >  int qemu_spice_rect_is_empty(const QXLRect* r)
> >  {
> > --
> > 2.39.2
> >
> >
> 
> 
> --
> Marc-André Lureau

[PATCH v1 3/7] ui/spice: Submit the gl_draw requests at 60 FPS for remote clients

[Vivek Kasireddy]

In the specific case where the display layer (virtio-gpu) is using
dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
it makes sense to limit the maximum (streaming) rate to 60 FPS
using the GUI timer. This matches the behavior of GTK UI where the
display updates are submitted at 60 FPS (assuming the underlying
mode is WxY@60).

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/spice-display.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 384b8508d4..90c04623ec 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -841,12 +841,31 @@ static void qemu_spice_gl_block_timer(void *opaque)
     warn_report("spice: no gl-draw-done within one second");
 }
 
+static void spice_gl_draw(SimpleSpiceDisplay *ssd,
+                           uint32_t x, uint32_t y, uint32_t w, uint32_t h)
+{
+    uint64_t cookie;
+
+    cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
+    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
+}
+
 static void spice_gl_refresh(DisplayChangeListener *dcl)
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
-    uint64_t cookie;
+    QemuDmaBuf *dmabuf = ssd->guest_dmabuf;
 
-    if (!ssd->ds || qemu_console_is_gl_blocked(ssd->dcl.con)) {
+    if (!ssd->ds) {
+        return;
+    }
+
+    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
+        if (remote_client && ssd->gl_updates && dmabuf) {
+            spice_gl_draw(ssd, 0, 0, dmabuf->width, dmabuf->height);
+            ssd->gl_updates = 0;
+            /* To stream at 60 FPS, the (GUI) timer delay needs to be ~17 ms */
+            dcl->update_interval = 1000 / (2 * GUI_REFRESH_INTERVAL_DEFAULT) + 1;
+        }
         return;
     }
 
@@ -854,11 +873,8 @@ static void spice_gl_refresh(DisplayChangeListener *dcl)
     if (ssd->gl_updates && ssd->have_surface) {
         qemu_spice_gl_block(ssd, true);
         glFlush();
-        cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
-        spice_qxl_gl_draw_async(&ssd->qxl, 0, 0,
-                                surface_width(ssd->ds),
-                                surface_height(ssd->ds),
-                                cookie);
+        spice_gl_draw(ssd, 0, 0,
+                      surface_width(ssd->ds), surface_height(ssd->ds));
         ssd->gl_updates = 0;
     }
 }
@@ -1025,7 +1041,6 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint stride = 0, fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
-    uint64_t cookie;
     int fd;
 
     if (!ssd->have_scanout) {
@@ -1098,8 +1113,11 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
     qemu_spice_gl_block(ssd, true);
     glFlush();
-    cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
-    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
+    if (remote_client) {
+        ssd->gl_updates++;
+    } else {
+        spice_gl_draw(ssd, x, y, w, h);
+    }
 }
 
 static const DisplayChangeListenerOps display_listener_gl_ops = {
-- 
2.39.2

Re: [PATCH v1 3/7] ui/spice: Submit the gl_draw requests at 60 FPS for remote clients

[Marc-André Lureau]

Hi

On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> In the specific case where the display layer (virtio-gpu) is using
> dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
> it makes sense to limit the maximum (streaming) rate to 60 FPS
> using the GUI timer. This matches the behavior of GTK UI where the
> display updates are submitted at 60 FPS (assuming the underlying
> mode is WxY@60).

One of the issues with this approach is that the DMABUF isn't owned by
the consumer. By delaying the usage of it, we risk having
damaged/invalid updates.

It would be great if we could have a mechanism for double/triple
buffering with virtio-gpu, as far as I know this is not possible yet.

I wonder if setting dpy_set_ui_info() with the fixed refresh_rate is
enough for the guest to have a fixed FPS. It will only work with gfx
hw that support ui_info() though.



>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  ui/spice-display.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 384b8508d4..90c04623ec 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -841,12 +841,31 @@ static void qemu_spice_gl_block_timer(void *opaque)
>      warn_report("spice: no gl-draw-done within one second");
>  }
>
> +static void spice_gl_draw(SimpleSpiceDisplay *ssd,
> +                           uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> +{
> +    uint64_t cookie;
> +
> +    cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> +    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> +}
> +
>  static void spice_gl_refresh(DisplayChangeListener *dcl)
>  {
>      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> -    uint64_t cookie;
> +    QemuDmaBuf *dmabuf = ssd->guest_dmabuf;
>
> -    if (!ssd->ds || qemu_console_is_gl_blocked(ssd->dcl.con)) {
> +    if (!ssd->ds) {
> +        return;
> +    }
> +
> +    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
> +        if (remote_client && ssd->gl_updates && dmabuf) {
> +            spice_gl_draw(ssd, 0, 0, dmabuf->width, dmabuf->height);
> +            ssd->gl_updates = 0;
> +            /* To stream at 60 FPS, the (GUI) timer delay needs to be ~17 ms */
> +            dcl->update_interval = 1000 / (2 * GUI_REFRESH_INTERVAL_DEFAULT) + 1;
> +        }
>          return;
>      }
>
> @@ -854,11 +873,8 @@ static void spice_gl_refresh(DisplayChangeListener *dcl)
>      if (ssd->gl_updates && ssd->have_surface) {
>          qemu_spice_gl_block(ssd, true);
>          glFlush();
> -        cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> -        spice_qxl_gl_draw_async(&ssd->qxl, 0, 0,
> -                                surface_width(ssd->ds),
> -                                surface_height(ssd->ds),
> -                                cookie);
> +        spice_gl_draw(ssd, 0, 0,
> +                      surface_width(ssd->ds), surface_height(ssd->ds));
>          ssd->gl_updates = 0;
>      }
>  }
> @@ -1025,7 +1041,6 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>      EGLint stride = 0, fourcc = 0;
>      bool render_cursor = false;
>      bool y_0_top = false; /* FIXME */
> -    uint64_t cookie;
>      int fd;
>
>      if (!ssd->have_scanout) {
> @@ -1098,8 +1113,11 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>      trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
>      qemu_spice_gl_block(ssd, true);
>      glFlush();
> -    cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> -    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> +    if (remote_client) {
> +        ssd->gl_updates++;
> +    } else {
> +        spice_gl_draw(ssd, x, y, w, h);
> +    }
>  }
>
>  static const DisplayChangeListenerOps display_listener_gl_ops = {
> --
> 2.39.2
>
>


--
Marc-André Lureau

RE: [PATCH v1 3/7] ui/spice: Submit the gl_draw requests at 60 FPS for remote clients

[Kasireddy, Vivek]

Hi Marc-Andre,

> 
> Hi
> 
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> <vivek.kasireddy@intel.com> wrote:
> >
> > In the specific case where the display layer (virtio-gpu) is using
> > dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
> > it makes sense to limit the maximum (streaming) rate to 60 FPS
> > using the GUI timer. This matches the behavior of GTK UI where the
> > display updates are submitted at 60 FPS (assuming the underlying
> > mode is WxY@60).
> 
> One of the issues with this approach is that the DMABUF isn't owned by
> the consumer. By delaying the usage of it, we risk having
> damaged/invalid updates.
This patch is only relevant with blob=true option. And, in this case, the
Guest (virtio-gpu kernel driver) is blocked (on a fence) until the Host
has consumed the buffer, which in this case happens after the encoder
signals the async to indicate that encoding is completed. Therefore,
AFAIU, there is no risk of missing or invalid updates. Ideally, the rate
should be driven by the consumer which in this case is the Spice client,
and given that it doesn't make sense to submit new frames faster than
the refresh rate, I figured it is ok to limit the producer to 60 FPS as well.
Note that Spice also does rate limiting based on network latencies and
dropped frames.


> 
> It would be great if we could have a mechanism for double/triple
> buffering with virtio-gpu, as far as I know this is not possible yet.
Given that virtio-gpu is a drm/kms driver, there can only be one buffer
in flight at any given time. And, it doesn't make sense to submit frames
faster than the refresh rate as they would simply get dropped. However,
I tried to address this issue few years ago but it did not go anywhere:
https://lore.kernel.org/all/20211014124402.46f95ebc@eldfell/T/

> 
> I wonder if setting dpy_set_ui_info() with the fixed refresh_rate is
> enough for the guest to have a fixed FPS.
I am not sure. Let me try to experiment with it and see how things work.

Thanks,
Vivek
> It will only work with gfx hw that support ui_info() though.
> 
> 
> 
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Frediano Ziglio <freddy77@gmail.com>
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  ui/spice-display.c | 38 ++++++++++++++++++++++++++++----------
> >  1 file changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 384b8508d4..90c04623ec 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -841,12 +841,31 @@ static void qemu_spice_gl_block_timer(void
> *opaque)
> >      warn_report("spice: no gl-draw-done within one second");
> >  }
> >
> > +static void spice_gl_draw(SimpleSpiceDisplay *ssd,
> > +                           uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> > +{
> > +    uint64_t cookie;
> > +
> > +    cookie =
> (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> > +    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> > +}
> > +
> >  static void spice_gl_refresh(DisplayChangeListener *dcl)
> >  {
> >      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > -    uint64_t cookie;
> > +    QemuDmaBuf *dmabuf = ssd->guest_dmabuf;
> >
> > -    if (!ssd->ds || qemu_console_is_gl_blocked(ssd->dcl.con)) {
> > +    if (!ssd->ds) {
> > +        return;
> > +    }
> > +
> > +    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
> > +        if (remote_client && ssd->gl_updates && dmabuf) {
> > +            spice_gl_draw(ssd, 0, 0, dmabuf->width, dmabuf->height);
> > +            ssd->gl_updates = 0;
> > +            /* To stream at 60 FPS, the (GUI) timer delay needs to be ~17 ms */
> > +            dcl->update_interval = 1000 / (2 *
> GUI_REFRESH_INTERVAL_DEFAULT) + 1;
> > +        }
> >          return;
> >      }
> >
> > @@ -854,11 +873,8 @@ static void spice_gl_refresh(DisplayChangeListener
> *dcl)
> >      if (ssd->gl_updates && ssd->have_surface) {
> >          qemu_spice_gl_block(ssd, true);
> >          glFlush();
> > -        cookie =
> (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> > -        spice_qxl_gl_draw_async(&ssd->qxl, 0, 0,
> > -                                surface_width(ssd->ds),
> > -                                surface_height(ssd->ds),
> > -                                cookie);
> > +        spice_gl_draw(ssd, 0, 0,
> > +                      surface_width(ssd->ds), surface_height(ssd->ds));
> >          ssd->gl_updates = 0;
> >      }
> >  }
> > @@ -1025,7 +1041,6 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> >      EGLint stride = 0, fourcc = 0;
> >      bool render_cursor = false;
> >      bool y_0_top = false; /* FIXME */
> > -    uint64_t cookie;
> >      int fd;
> >
> >      if (!ssd->have_scanout) {
> > @@ -1098,8 +1113,11 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> >      trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
> >      qemu_spice_gl_block(ssd, true);
> >      glFlush();
> > -    cookie =
> (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> > -    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> > +    if (remote_client) {
> > +        ssd->gl_updates++;
> > +    } else {
> > +        spice_gl_draw(ssd, x, y, w, h);
> > +    }
> >  }
> >
> >  static const DisplayChangeListenerOps display_listener_gl_ops = {
> > --
> > 2.39.2
> >
> >
> 
> 
> --
> Marc-André Lureau

[PATCH v1 4/7] ui/console-gl: Add an option to override a surface's glformat

[Vivek Kasireddy]

In some cases where a UI component (e.g, Spice) needs to choose
a particular glformat for a surface while creating a texture,
this new GLenum provides an option to do so. One situation
where this needs to be done is when the Host endianness is
causing issues such as interchanged R and B channels.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/surface.h | 1 +
 ui/console-gl.c      | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/ui/surface.h b/include/ui/surface.h
index 4244e0ca4a..a39fee55a2 100644
--- a/include/ui/surface.h
+++ b/include/ui/surface.h
@@ -20,6 +20,7 @@ typedef struct DisplaySurface {
     uint8_t flags;
 #ifdef CONFIG_OPENGL
     GLenum glformat;
+    GLenum target_glformat;
     GLenum gltype;
     GLuint texture;
 #endif
diff --git a/ui/console-gl.c b/ui/console-gl.c
index 103b954017..dee317f42c 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -72,6 +72,12 @@ void surface_gl_create_texture(QemuGLShader *gls,
         g_assert_not_reached();
     }
 
+    /* The caller wants to override the glformat in some specific cases */
+    if (surface->target_glformat &&
+        surface->target_glformat != surface->glformat) {
+        surface->glformat = surface->target_glformat;
+    }
+
     glGenTextures(1, &surface->texture);
     glEnable(GL_TEXTURE_2D);
     glBindTexture(GL_TEXTURE_2D, surface->texture);
-- 
2.39.2

[PATCH v1 5/7] ui/spice: Override the surface's glformat when gl=on is enabled

[Vivek Kasireddy]

When testing with gl=on on an Intel Host, it was noticed that the
R and B channels were interchanged while the Guest FB image was
displayed. This was only seen if the display layer (virtio-gpu)
did not directly share the dmabuf fd with Spice (i.e, blob=false).

One of the main differences in the case where blob=true vs blob=false
is that we create the dmabuf fd from a texture in the latter case
whereas we directly pass the fd from the display layer to Spice in
the former case. Although, the surface's format (PIXMAN_BE_b8g8r8x8)
is the same in both cases, the creation of the texture (which involves
copying data from Pixman image into a GPU buffer) appears to somehow
result in having the R and B channels interchanged. One way to ensure
correct behavior is we have glformat=GL_RGBA while creating the texture.

It looks like having glformat=GL_RGBA and gltype = GL_UNSIGNED_BYTE
should work regardless of the Host's endianness but let us limit
this change only to this specific use-case for now.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/spice-display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 90c04623ec..08b4aec921 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -900,6 +900,9 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
     }
     ssd->ds = new_surface;
     if (ssd->ds) {
+        if (remote_client && surface_format(ssd->ds) != PIXMAN_r5g6b5) {
+            ssd->ds->target_glformat = GL_RGBA;
+        }
         surface_gl_create_texture(ssd->gls, ssd->ds);
         fd = egl_get_fd_for_texture(ssd->ds->texture,
                                     &stride, &fourcc,
-- 
2.39.2

[PATCH v1 6/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Vivek Kasireddy]

There are cases where we do not want the memory layout of a texture to
be tiled as the component processing the texture memory would not know
how to de-tile either via software or hardware. Therefore, ensuring
that the memory backing the texture has a linear layout is absolutely
necessary in these situations.

Note that, requesting GL implementation to create a texture with
linear memory layout can only be done using memory objects. And, a
memory object can be created by importing a dmabuf fd.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/console.h |  2 ++
 ui/console-gl.c      | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index a4a49ffc64..e53e3ce03e 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -444,6 +444,8 @@ bool console_gl_check_format(DisplayChangeListener *dcl,
                              pixman_format_code_t format);
 void surface_gl_create_texture(QemuGLShader *gls,
                                DisplaySurface *surface);
+void surface_gl_create_texture_from_fd(DisplaySurface *surface,
+                                       int fd, GLuint *texture);
 void surface_gl_update_texture(QemuGLShader *gls,
                                DisplaySurface *surface,
                                int x, int y, int w, int h);
diff --git a/ui/console-gl.c b/ui/console-gl.c
index dee317f42c..2c1b2ae377 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -102,6 +102,32 @@ void surface_gl_create_texture(QemuGLShader *gls,
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
 }
 
+void surface_gl_create_texture_from_fd(DisplaySurface *surface,
+                                       int fd, GLuint *texture)
+{
+    unsigned long size = surface_stride(surface) * surface_height(surface);
+    GLuint mem_obj;
+
+    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
+        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
+        return;
+    }
+
+#ifdef GL_EXT_memory_object_fd
+    glCreateMemoryObjectsEXT(1, &mem_obj);
+    glImportMemoryFdEXT(mem_obj, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, fd);
+    if (!glIsMemoryObjectEXT(mem_obj)) {
+        return;
+    }
+
+    glGenTextures(1, texture);
+    glBindTexture(GL_TEXTURE_2D, *texture);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT, GL_LINEAR_TILING_EXT);
+    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8, surface_width(surface),
+                         surface_height(surface), mem_obj, 0);
+#endif
+}
+
 void surface_gl_update_texture(QemuGLShader *gls,
                                DisplaySurface *surface,
                                int x, int y, int w, int h)
-- 
2.39.2

[PATCH v1 7/7] ui/spice: Create another texture with linear layout when gl=on is enabled

[Vivek Kasireddy]

Since most encoders (invoked by Spice) may not work with tiled memory
associated with a texture, we need to create another texture that has
linear memory layout and use that instead.

Note that, there does not seem to be a direct way to indicate to the
GL implementation that a texture's backing memory needs to be linear.
Instead, we have to do it in a roundabout way where we first need to
create a tiled texture and obtain a dmabuf fd associated with it via
EGL. Next, we have to create a memory object by importing the dmabuf
fd created earlier and finally create a linear texture by using the
memory object as the texture storage mechanism.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/spice-display.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 08b4aec921..94cb378dbe 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -893,6 +893,7 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
     EGLint stride, fourcc;
+    GLuint texture = 0;
     int fd;
 
     if (ssd->ds) {
@@ -912,6 +913,38 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
             return;
         }
 
+        if (remote_client && surface_format(ssd->ds) != PIXMAN_r5g6b5) {
+            /*
+             * We really want to ensure that the memory layout of the texture
+             * is linear; otherwise, the encoder's output may show corruption.
+             */
+            surface_gl_create_texture_from_fd(ssd->ds, fd, &texture);
+
+            /*
+             * A successful return after glImportMemoryFdEXT() means that
+             * the ownership of fd has been passed to GL. In other words,
+             * the fd we got above should not be used anymore.
+             */
+            if (texture > 0) {
+                fd = egl_get_fd_for_texture(texture,
+                                            &stride, &fourcc,
+                                            NULL);
+                if (fd < 0) {
+                    glDeleteTextures(1, &texture);
+                    fd = egl_get_fd_for_texture(ssd->ds->texture,
+                                                &stride, &fourcc,
+                                                NULL);
+                    if (fd < 0) {
+                        surface_gl_destroy_texture(ssd->gls, ssd->ds);
+                        return;
+                    }
+                } else {
+                    surface_gl_destroy_texture(ssd->gls, ssd->ds);
+                    ssd->ds->texture = texture;
+                }
+            }
+        }
+
         trace_qemu_spice_gl_surface(ssd->qxl.id,
                                     surface_width(ssd->ds),
                                     surface_height(ssd->ds),
-- 
2.39.2

Re: [PATCH v1 7/7] ui/spice: Create another texture with linear layout when gl=on is enabled

[Marc-André Lureau]

Hi

On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> Since most encoders (invoked by Spice) may not work with tiled memory
> associated with a texture, we need to create another texture that has
> linear memory layout and use that instead.
>
> Note that, there does not seem to be a direct way to indicate to the
> GL implementation that a texture's backing memory needs to be linear.
> Instead, we have to do it in a roundabout way where we first need to
> create a tiled texture and obtain a dmabuf fd associated with it via
> EGL. Next, we have to create a memory object by importing the dmabuf
> fd created earlier and finally create a linear texture by using the
> memory object as the texture storage mechanism.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  ui/spice-display.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 08b4aec921..94cb378dbe 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -893,6 +893,7 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
>  {
>      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
>      EGLint stride, fourcc;
> +    GLuint texture = 0;
>      int fd;
>
>      if (ssd->ds) {
> @@ -912,6 +913,38 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
>              return;
>          }
>
> +        if (remote_client && surface_format(ssd->ds) != PIXMAN_r5g6b5) {

hmm

> +            /*
> +             * We really want to ensure that the memory layout of the texture
> +             * is linear; otherwise, the encoder's output may show corruption.
> +             */
> +            surface_gl_create_texture_from_fd(ssd->ds, fd, &texture);

What if the encoder actually supports tiled layout?

Shouldn't this conversion be done at the encoder level as necessary?

It's also strange to reuse an FD associated with a tiled texture for a
linear layout (I am uncomfortable with all this tbh)

> +
> +            /*
> +             * A successful return after glImportMemoryFdEXT() means that
> +             * the ownership of fd has been passed to GL. In other words,
> +             * the fd we got above should not be used anymore.
> +             */
> +            if (texture > 0) {
> +                fd = egl_get_fd_for_texture(texture,
> +                                            &stride, &fourcc,
> +                                            NULL);
> +                if (fd < 0) {

I suggest adding warnings or tracing, to help debug issues...

> +                    glDeleteTextures(1, &texture);
> +                    fd = egl_get_fd_for_texture(ssd->ds->texture,
> +                                                &stride, &fourcc,
> +                                                NULL);
> +                    if (fd < 0) {
> +                        surface_gl_destroy_texture(ssd->gls, ssd->ds);
> +                        return;
> +                    }
> +                } else {
> +                    surface_gl_destroy_texture(ssd->gls, ssd->ds);
> +                    ssd->ds->texture = texture;

Have you tried this series with virgl? (I doubt the renderer accepts
that the scanout texture is changed)

> +                }
> +            }
> +        }
> +
>          trace_qemu_spice_gl_surface(ssd->qxl.id,
>                                      surface_width(ssd->ds),
>                                      surface_height(ssd->ds),
> --
> 2.39.2
>
>


-- 
Marc-André Lureau

RE: [PATCH v1 7/7] ui/spice: Create another texture with linear layout when gl=on is enabled

[Kasireddy, Vivek]

Hi Marc-Andre,

> 
> Hi
> 
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> <vivek.kasireddy@intel.com> wrote:
> >
> > Since most encoders (invoked by Spice) may not work with tiled memory
> > associated with a texture, we need to create another texture that has
> > linear memory layout and use that instead.
> >
> > Note that, there does not seem to be a direct way to indicate to the
> > GL implementation that a texture's backing memory needs to be linear.
> > Instead, we have to do it in a roundabout way where we first need to
> > create a tiled texture and obtain a dmabuf fd associated with it via
> > EGL. Next, we have to create a memory object by importing the dmabuf
> > fd created earlier and finally create a linear texture by using the
> > memory object as the texture storage mechanism.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Frediano Ziglio <freddy77@gmail.com>
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  ui/spice-display.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 08b4aec921..94cb378dbe 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -893,6 +893,7 @@ static void spice_gl_switch(DisplayChangeListener
> *dcl,
> >  {
> >      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> >      EGLint stride, fourcc;
> > +    GLuint texture = 0;
> >      int fd;
> >
> >      if (ssd->ds) {
> > @@ -912,6 +913,38 @@ static void spice_gl_switch(DisplayChangeListener
> *dcl,
> >              return;
> >          }
> >
> > +        if (remote_client && surface_format(ssd->ds) != PIXMAN_r5g6b5) {
> 
> hmm
> 
> > +            /*
> > +             * We really want to ensure that the memory layout of the texture
> > +             * is linear; otherwise, the encoder's output may show corruption.
> > +             */
> > +            surface_gl_create_texture_from_fd(ssd->ds, fd, &texture);
> 
> What if the encoder actually supports tiled layout?
Right, that would be true in most cases as the Render and Encode hardware are
mostly compatible. And, my patches are not required in this case if Spice chooses
a hardware encoder. However, the choice of which encoder to use (hardware vs
software) is decided dynamically and is internal to Spice at this point. And, since
there is no easy way to know from Qemu whether an encoder chosen by Spice
would support any given tiling format, I chose to always use linear layout since it
is guaranteed to work with all types of encoders.

> 
> Shouldn't this conversion be done at the encoder level as necessary?
Yeah, that is probably the right place but a software encoder like x264enc is not
going to know how to do the conversion from various tiled formats. This is the
specific case I am trying to address given that it is not a problem with hardware
encoders. I guess we could add a Qemu ui/spice option to tweak this behavior
while launching the VM.

> 
> It's also strange to reuse an FD associated with a tiled texture for a
> linear layout (I am uncomfortable with all this tbh)
I have looked around but there doesn't seem to be a way for requesting the GL
implementation to create a texture with linear layout other than via importing
memory objects. As noted in the comments below, the fd's ownership is taken
over by the GL implementation as part of the import. 

Also, I have started a conversation to figure out if there is any other way to
create linear textures more efficiently:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27067

> 
> > +
> > +            /*
> > +             * A successful return after glImportMemoryFdEXT() means that
> > +             * the ownership of fd has been passed to GL. In other words,
> > +             * the fd we got above should not be used anymore.
> > +             */
> > +            if (texture > 0) {
> > +                fd = egl_get_fd_for_texture(texture,
> > +                                            &stride, &fourcc,
> > +                                            NULL);
> > +                if (fd < 0) {
> 
> I suggest adding warnings or tracing, to help debug issues...
Sure, will do that in v2.

> 
> > +                    glDeleteTextures(1, &texture);
> > +                    fd = egl_get_fd_for_texture(ssd->ds->texture,
> > +                                                &stride, &fourcc,
> > +                                                NULL);
> > +                    if (fd < 0) {
> > +                        surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > +                        return;
> > +                    }
> > +                } else {
> > +                    surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > +                    ssd->ds->texture = texture;
> 
> Have you tried this series with virgl? (I doubt the renderer accepts
> that the scanout texture is changed)
I have tried with virgl enabled and it mostly works because my patches
don't affect virgl codepaths such as qemu_spice_gl_scanout_texture()
which is where the texture/fd is set. My patches are only meant for
virtio-vga device (and blob=false), where the Guest desktop is rendered
using llvmpipe DRI driver.

But, your suspicion is right. That is, if we are using virgl and Spice selects a
software encoder (such as x264enc), then the encoder's output will show
corruption and we'd not be able fix it with the approach used in this patch.
As you implied, this is because virglrenderer owns the scanout texture in
this case which cannot be replaced inside Qemu UI like we do in this patch.
I am not sure how this problem can be solved if we (Qemu UI) cannot request
Virgl or GL implementation to create linear textures.

Thanks,
Vivek

> 
> > +                }
> > +            }
> > +        }
> > +
> >          trace_qemu_spice_gl_surface(ssd->qxl.id,
> >                                      surface_width(ssd->ds),
> >                                      surface_height(ssd->ds),
> > --
> > 2.39.2
> >
> >
> 
> 
> --
> Marc-André Lureau