Re: [PATCH v2 spice-protocol 2/2] Add unix GL scanout messages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]