Hi Frediano
On Thu, Dec 17, 2015 at 11:03 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> 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.
It can be sent at any time, for ex, switching front/back buffers.
> 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?
The protocol is compatible with usage of
SPICE_MSG_DISPLAY_MONITORS_CONFIG. In fact, qemu sends a
MONITORS_CONFIG to know which region within the scanout is the
monitor. But currently, virgl/qemu doesn't support multi-monitor.
> 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.
Thanks, I will add this comment to the commit message too.
> 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?
Well, that's an interesting idea, but I am not sure we should try to
reuse the same message at this point.
> 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.
Again, that sounds overly complicated to me.
> What will happen if a client try to connect from a remote machine?
> A normal video stream is automatically used?
That's not covered by this 2 local-only gl messages. Imho, the server
could convert the gl stream to a video stream, and reuse the existing
video messages. (a second later approah is to stream audio/video with
rtp for lower latency/sync)
>> 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?
Indeed, these are 2 possibilities. Can we have concurrent gl-draw, or
not? Currently, with virgl renderer, you can't. But do we have to
specifiy this in the protocol? I would say it's not necessary. The
protocol could just say the client must acknowledge each draw. No?
> Would be sensible to add a number of acknowledges to the draw_done
> message?
Yes, if we had multiple concurrent gl-draw. It could also be a future
extension But this is local only, so it's not performance critical
here (even at 100fps)
>> 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.
What is more likely to happen sooner than later is that qemu will do
the 2d rendering and only use GL scanouts (with virtio-gpus, but
possibly others). But yes, it could be interesting to extend the
concept to 2d canvas. However, I would simply introduce a different
message (or extend an existing surface message)
>> (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@xxxxxxxxxx>
>> ---
>> 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?
sounds ok to me
>
>> + gl_scanout_flags flags;
>> + } gl_scanout_unix;
>> +
>> + message {
>> + uint32 x;
>> + uint32 y;
>> + uint32 w;
>> + uint32 h;
>> + } gl_draw;
>> +
>
> Can't we use a rect?
I guess we could, but Rect is weird: top/bottom/left/right i32 instead
of more conventional x/y/w/h u32. I don't see the benefit of reusing
SpiceRect here.
>> +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
Thanks a lot for you review!
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel