[Spice-devel] [PATCH v2 spice-protocol 2/2] Add unix GL scanout messages

[Frediano Ziglio]

> 
> Add 2 new messages to the display channel to stream pre-rendered GL
> images of the display. This is only possible when the client supports
> SPICE_DISPLAY_CAP_GL_SCANOUT capability.
> 
> The first message, SPICE_MSG_DISPLAY_GL_SCANOUT_UNIX, sends a gl image
> file handle via socket ancillary data, and can be imported in a GL
> context with the help of eglCreateImageKHR() (as with the 2d canvas, the
> SPICE_MSG_DISPLAY_MONITORS_CONFIG will give the monitors
> coordinates (x/y/w/h) within the image). There can be only one scanount
> per display channel.
> 

Scan-out is a terminology quite similar to the old frame buffer, right?
I think is meant to distinguish buffers to hold texture or other 3d stuff
and buffers rendered to a screen.
I imagine the SCANOUT is sent on client connection or when resolution
change.
What happen with multiple monitors? Is it just not supported or a single
scan-out is used for multiple monitors? Would be sensible to add the number
of the scan-out to support multiple monitors?

About the parameters that are currently used for the scan-out.
Beside width, height and stripe which are quite obvious.
fd points to a dma buffer exported by the Linux DRM layer.
format is specified as the format of fd data, as in your comment the fourcc
types, defined in drm_fourcc.h.
The extension for eglCreateImageKHR for this are specified at
www.kronos.org/registry/egl, EGL_EXT_image_dma_buf_import.

Can we export a dma_buf for normal 2d operations? Would it make sense to use
the same protocol to share the 2d frame buffer between spice-server and the
client if the client is on the same machine?
If Linux drm dma buffers are not strictly bound to (e)GL perhaps we could
avoid the use of gl in the names.
I was also wondering if instead of passing just some fixed attributes and
then build the attribute list for eglCreateImageKHR we could pass a similar
set of attribute-type+attribute to be extensible.
Specifically I was thinking about the possible usage of the offset attribute
in a 2d context sharing the frame buffer.

What will happen if a client try to connect from a remote machine?
A normal video stream is automatically used?

> A SPICE_MSG_DISPLAY_GL_DRAW message is sent with the coordinate of the
> region within the scanount to (re)draw on the client display. For each
> draw, once the client is done with the rendering, it must acknowldge it
> by sending a SPICE_MSGC_DISPLAY_GL_DRAW_DONE message, in order to
> release the context (it is expected to improve this in the future with a
> cross-process GL fence).
> 

Are these messages serialized or not?
I mean, can the server send multiple draw commands to client without
waiting every time for a done?
Would be sensible to add a number of acknowledges to the draw_done
message?

> The relation with the existing display channel messages is that all
> other messages are unchanged: the last drawing command received must be
> displayed. However the scanout display is all or nothing. Consequently,
> if a 2d canvas draw is received, the display must be switched to the
> drawn canvas. In other words, if the last message received is a GL draw
> the display should switch to the GL display, if it's a 2d draw message
> the display should be switched to the client 2d canvas.
> 

Thinking about frame buffer sharing even on 2d I was thinking about a way
to have both commands but possibly in this case you want to do all
rendering at spice-server level and send updates with a protocol similar
to the "gl" one.

> (there will probably be a stipped-down "gl-only" channel in the future,
> or support for other streaming methods, but this protocol change should
> be enough for basic virgl or other gpu-accelerated support)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>  spice.proto      | 25 ++++++++++++++++++++++++-
>  spice/enums.h    |  9 +++++++++
>  spice/protocol.h |  1 +
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/spice.proto b/spice.proto
> index 3bca900..0756c6c 100644
> --- a/spice.proto
> +++ b/spice.proto
> @@ -678,6 +678,10 @@ struct Head {
>      uint32 flags;
>  };
>  
> +flags8 gl_scanout_flags {
> +    Y0TOP
> +};
> +

I think is no harm to use flags32 here for possible future extensions.

>  channel DisplayChannel : BaseChannel {
>   server:
>      message {
> @@ -915,7 +919,23 @@ channel DisplayChannel : BaseChannel {
>          uint32 timeout_ms;
>      } stream_activate_report;
>  
> - client:
> +    message {
> +        unix_fd fd;
> +        uint32 width;
> +        uint32 height;
> +        uint32 stride;
> +        uint32 format; /* drm fourcc */

Would be sensible to use drm_dma_buf_fd and drm_fourcc_format
just to be more specific?

> +        gl_scanout_flags flags;
> +    } gl_scanout_unix;
> +
> +    message {
> +        uint32 x;
> +        uint32 y;
> +        uint32 w;
> +        uint32 h;
> +    } gl_draw;
> +

Can't we use a rect?

> +client:
>      message {
>  	uint8 pixmap_cache_id;
>  	int64 pixmap_cache_size; //in pixels
> @@ -937,6 +957,9 @@ channel DisplayChannel : BaseChannel {
>      message {
>          image_compression image_compression;
>      } preferred_compression;
> +
> +    message {
> +    } gl_draw_done;
>  };
>  
>  flags16 keyboard_modifier_flags {
> diff --git a/spice/enums.h b/spice/enums.h
> index 16885ac..613db52 100644
> --- a/spice/enums.h
> +++ b/spice/enums.h
> @@ -303,6 +303,12 @@ typedef enum SpiceResourceType {
>      SPICE_RESOURCE_TYPE_ENUM_END
>  } SpiceResourceType;
>  
> +typedef enum SpiceGlScanoutFlags {
> +    SPICE_GL_SCANOUT_FLAGS_Y0TOP = (1 << 0),
> +
> +    SPICE_GL_SCANOUT_FLAGS_MASK = 0x1
> +} SpiceGlScanoutFlags;
> +
>  typedef enum SpiceKeyboardModifierFlags {
>      SPICE_KEYBOARD_MODIFIER_FLAGS_SCROLL_LOCK = (1 << 0),
>      SPICE_KEYBOARD_MODIFIER_FLAGS_NUM_LOCK = (1 << 1),
> @@ -503,6 +509,8 @@ enum {
>      SPICE_MSG_DISPLAY_MONITORS_CONFIG,
>      SPICE_MSG_DISPLAY_DRAW_COMPOSITE,
>      SPICE_MSG_DISPLAY_STREAM_ACTIVATE_REPORT,
> +    SPICE_MSG_DISPLAY_GL_SCANOUT_UNIX,
> +    SPICE_MSG_DISPLAY_GL_DRAW,
>  
>      SPICE_MSG_END_DISPLAY
>  };
> @@ -511,6 +519,7 @@ enum {
>      SPICE_MSGC_DISPLAY_INIT = 101,
>      SPICE_MSGC_DISPLAY_STREAM_REPORT,
>      SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION,
> +    SPICE_MSGC_DISPLAY_GL_DRAW_DONE,
>  
>      SPICE_MSGC_END_DISPLAY
>  };
> diff --git a/spice/protocol.h b/spice/protocol.h
> index 0c265ee..3e6c624 100644
> --- a/spice/protocol.h
> +++ b/spice/protocol.h
> @@ -136,6 +136,7 @@ enum {
>      SPICE_DISPLAY_CAP_STREAM_REPORT,
>      SPICE_DISPLAY_CAP_LZ4_COMPRESSION,
>      SPICE_DISPLAY_CAP_PREF_COMPRESSION,
> +    SPICE_DISPLAY_CAP_GL_SCANOUT,
>  };
>  
>  enum {

Frediano