| Message ID | 20190423132004.13725-6-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user for input & GPU | expand |
On Tue, Apr 23, 2019 at 03:19:58PM +0200, Marc-André Lureau wrote: > Add a new vhost-user message to give a unix socket to a vhost-user > backend for GPU display updates. Can you split input/gpu into two patch series? > +Wire format > +=========== > + > +Unless specified differently, numbers are in the machine native byte > +order. > + > +A vhost-user-gpu request consists of 2 header fields and a payload. > + > ++---------+------+---------+ > +| request | size | payload | > ++---------+------+---------+ > + > +Header > +------ > + > +:request: ``u32``, type of the request > + > +:size: ``u32``, size of the payload > + > +A reply consists only of a payload, whose content depends on the request. I'd suggest to use the same format for replies, only with "request" meaning "status" in replies. Allows for OK/ERROR status in replies, and having a size field in replies too should make things more robust. Also allows for an empty reply (status=ok,size=0). cheers, Gerd
Hi On Fri, Apr 26, 2019 at 9:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Tue, Apr 23, 2019 at 03:19:58PM +0200, Marc-André Lureau wrote: > > Add a new vhost-user message to give a unix socket to a vhost-user > > backend for GPU display updates. > > Can you split input/gpu into two patch series? > > > +Wire format > > +=========== > > + > > +Unless specified differently, numbers are in the machine native byte > > +order. > > + > > +A vhost-user-gpu request consists of 2 header fields and a payload. > > + > > ++---------+------+---------+ > > +| request | size | payload | > > ++---------+------+---------+ > > + > > +Header > > +------ > > + > > +:request: ``u32``, type of the request > > + > > +:size: ``u32``, size of the payload > > + > > +A reply consists only of a payload, whose content depends on the request. > > I'd suggest to use the same format for replies, only with "request" > meaning "status" in replies. Allows for OK/ERROR status in replies, > and having a size field in replies too should make things more robust. > Also allows for an empty reply (status=ok,size=0). That brings more questions and ambiguity imho. Can we leave that for future protocol extensions negotiated with GET/SET_PROTOCOL_FEATURES ? -- Marc-André Lureau
Hi, > > > +Wire format > > > +=========== > > > + > > > +Unless specified differently, numbers are in the machine native byte > > > +order. > > > + > > > +A vhost-user-gpu request consists of 2 header fields and a payload. > > > + > > > ++---------+------+---------+ > > > +| request | size | payload | > > > ++---------+------+---------+ > > > + > > > +Header > > > +------ > > > + > > > +:request: ``u32``, type of the request > > > + > > > +:size: ``u32``, size of the payload > > > + > > > +A reply consists only of a payload, whose content depends on the request. > > > > I'd suggest to use the same format for replies, only with "request" > > meaning "status" in replies. Allows for OK/ERROR status in replies, > > and having a size field in replies too should make things more robust. > > Also allows for an empty reply (status=ok,size=0). > > > That brings more questions and ambiguity imho. What questions for example? > Can we leave that for future protocol extensions negotiated with > GET/SET_PROTOCOL_FEATURES ? I don't think negotiating such a basic protocol change is a good idea. cheers, Gerd
Hi On Fri, Apr 26, 2019 at 2:06 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > > +Wire format > > > > +=========== > > > > + > > > > +Unless specified differently, numbers are in the machine native byte > > > > +order. > > > > + > > > > +A vhost-user-gpu request consists of 2 header fields and a payload. > > > > + > > > > ++---------+------+---------+ > > > > +| request | size | payload | > > > > ++---------+------+---------+ > > > > + > > > > +Header > > > > +------ > > > > + > > > > +:request: ``u32``, type of the request > > > > + > > > > +:size: ``u32``, size of the payload > > > > + > > > > +A reply consists only of a payload, whose content depends on the request. > > > > > > I'd suggest to use the same format for replies, only with "request" > > > meaning "status" in replies. Allows for OK/ERROR status in replies, > > > and having a size field in replies too should make things more robust. > > > Also allows for an empty reply (status=ok,size=0). > > > > > > That brings more questions and ambiguity imho. > > What questions for example? This opens up different kind of possible replies, and error handling. With current proposal and needs, the reply (or absence of reply) is entirely driven by the request. With your proposal, should all request have a reply? which makes a lot more code synchronous, and complicates both sides unnecessarily. > > > Can we leave that for future protocol extensions negotiated with > > GET/SET_PROTOCOL_FEATURES ? > > I don't think negotiating such a basic protocol change is a good idea. Well, then I would rather focus on improving protocol negociation, rather than adding unnecessary protocol changes. Given that GET/SET_PROTOCOL_FEATURES is the first messages being sent, why couldn't it have flags indicating new protocol revision?
Hi, > > What questions for example? > > This opens up different kind of possible replies, and error handling. > > With current proposal and needs, the reply (or absence of reply) is > entirely driven by the request. > > With your proposal, should all request have a reply? Yes. > which makes a lot > more code synchronous, Why? You don't have to wait for the reply before sending the next request. Adding a request id to the messages might be useful, so it is possible to wait for a reply to a specific message without having to keeping track of all in-flight messages. > and complicates both sides unnecessarily. Having headers in the reply allows it to process them in generic code. There is a size header for the reply, so you can parse the stream without knowing what replay to expect. You can use the status field to indicate the payload, simliar to virtio-gpu which has response code OK_NODATA, some OK_$whatpayload and some ERR_$failure codes. You can dispatch based on the response/status code and run *fully* asynchronous without too much trouble. > > > Can we leave that for future protocol extensions negotiated with > > > GET/SET_PROTOCOL_FEATURES ? > > > > I don't think negotiating such a basic protocol change is a good idea. > > Well, then I would rather focus on improving protocol negociation, > rather than adding unnecessary protocol changes. > > Given that GET/SET_PROTOCOL_FEATURES is the first messages being sent, > why couldn't it have flags indicating new protocol revision? A properly structed reply allows a different approach in reply processing (see above). But that only works if it is in the protocol right from the start. As add-on feature it can't provide the benefits because the reply parser must be able to handle both protocol variants. cheers, Gerd
Hi On Mon, Apr 29, 2019 at 9:12 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > What questions for example? > > > > This opens up different kind of possible replies, and error handling. > > > > With current proposal and needs, the reply (or absence of reply) is > > entirely driven by the request. > > > > With your proposal, should all request have a reply? > > Yes. > > > which makes a lot > > more code synchronous, > > Why? You don't have to wait for the reply before sending the next > request. > > Adding a request id to the messages might be useful, so it is possible > to wait for a reply to a specific message without having to keeping > track of all in-flight messages. > > > and complicates both sides unnecessarily. > > Having headers in the reply allows it to process them in generic code. > There is a size header for the reply, so you can parse the stream > without knowing what replay to expect. You can use the status field to > indicate the payload, simliar to virtio-gpu which has response code > OK_NODATA, some OK_$whatpayload and some ERR_$failure codes. > > You can dispatch based on the response/status code and run *fully* > asynchronous without too much trouble. It's really a different level of complexity than what I needed so far. > > > > > Can we leave that for future protocol extensions negotiated with > > > > GET/SET_PROTOCOL_FEATURES ? > > > > > > I don't think negotiating such a basic protocol change is a good idea. > > > > Well, then I would rather focus on improving protocol negociation, > > rather than adding unnecessary protocol changes. > > > > Given that GET/SET_PROTOCOL_FEATURES is the first messages being sent, > > why couldn't it have flags indicating new protocol revision? > > A properly structed reply allows a different approach in reply > processing (see above). But that only works if it is in the protocol > right from the start. As add-on feature it can't provide the benefits > because the reply parser must be able to handle both protocol variants. You are asking for a full-blown protocol... could we take DBus as an alternative instead?
Hi, > > > and complicates both sides unnecessarily. > > > > Having headers in the reply allows it to process them in generic code. > > There is a size header for the reply, so you can parse the stream > > without knowing what replay to expect. You can use the status field to > > indicate the payload, simliar to virtio-gpu which has response code > > OK_NODATA, some OK_$whatpayload and some ERR_$failure codes. > > > > You can dispatch based on the response/status code and run *fully* > > asynchronous without too much trouble. > > It's really a different level of complexity than what I needed so far. That is a bit overboard, no? All I'm asking for is: (a) A header field telling what kind of payload has been sent, for both requests and replies. (b) A header field telling what size the payload has. That hardly is a different level of complexity. And that isn't up for negotiation. If you absolutely don't want ack all requests, ok. Not sending a reply in case there is no payload to send back and no error to report is something I can live with. > > A properly structed reply allows a different approach in reply > > processing (see above). But that only works if it is in the protocol > > right from the start. As add-on feature it can't provide the benefits > > because the reply parser must be able to handle both protocol variants. > > You are asking for a full-blown protocol... could we take DBus as an > alternative instead? May I suggest QMP instead? Seriously: If you want reuse something, just use the vhost-user wire format. Which is essentially what I'm asking for, plus an additional flags field in the header. cheers, Gerd
On Mon, Apr 29, 2019 at 09:12:43AM +0200, Gerd Hoffmann wrote: > Hi, > > > > What questions for example? > > > > This opens up different kind of possible replies, and error handling. > > > > With current proposal and needs, the reply (or absence of reply) is > > entirely driven by the request. > > > > With your proposal, should all request have a reply? > > Yes. > > > which makes a lot > > more code synchronous, > > Why? You don't have to wait for the reply before sending the next > request. > > Adding a request id to the messages might be useful, so it is possible > to wait for a reply to a specific message without having to keeping > track of all in-flight messages. > > > and complicates both sides unnecessarily. > > Having headers in the reply allows it to process them in generic code. > There is a size header for the reply, so you can parse the stream > without knowing what replay to expect. You can use the status field to > indicate the payload, simliar to virtio-gpu which has response code > OK_NODATA, some OK_$whatpayload and some ERR_$failure codes. > > You can dispatch based on the response/status code and run *fully* > asynchronous without too much trouble. > > > > > Can we leave that for future protocol extensions negotiated with > > > > GET/SET_PROTOCOL_FEATURES ? > > > > > > I don't think negotiating such a basic protocol change is a good idea. > > > > Well, then I would rather focus on improving protocol negociation, > > rather than adding unnecessary protocol changes. > > > > Given that GET/SET_PROTOCOL_FEATURES is the first messages being sent, > > why couldn't it have flags indicating new protocol revision? > > A properly structed reply allows a different approach in reply > processing (see above). But that only works if it is in the protocol > right from the start. As add-on feature it can't provide the benefits > because the reply parser must be able to handle both protocol variants. > > cheers, > Gerd I think it can in theory - but if we know we want a feature we should just add it as mandatory. More options does imply more overhead.
diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h index 414ceb0a2f..eac5c120de 100644 --- a/contrib/libvhost-user/libvhost-user.h +++ b/contrib/libvhost-user/libvhost-user.h @@ -94,6 +94,7 @@ typedef enum VhostUserRequest { VHOST_USER_POSTCOPY_END = 30, VHOST_USER_GET_INFLIGHT_FD = 31, VHOST_USER_SET_INFLIGHT_FD = 32, + VHOST_USER_GPU_SET_SOCKET = 33, VHOST_USER_MAX } VhostUserRequest; diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index d6632a18e6..6f6670783f 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -170,4 +170,6 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev, int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev, struct vhost_iotlb_msg *imsg); +int vhost_user_gpu_set_socket(struct vhost_dev *dev, int fd); + #endif /* VHOST_BACKEND_H */ diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index e738f6ce88..c9823a1354 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -130,6 +130,7 @@ vu_request_to_string(unsigned int req) REQ(VHOST_USER_POSTCOPY_END), REQ(VHOST_USER_GET_INFLIGHT_FD), REQ(VHOST_USER_SET_INFLIGHT_FD), + REQ(VHOST_USER_GPU_SET_SOCKET), REQ(VHOST_USER_MAX), }; #undef REQ diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 553319c7ac..4ca5b2551e 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -96,6 +96,7 @@ typedef enum VhostUserRequest { VHOST_USER_POSTCOPY_END = 30, VHOST_USER_GET_INFLIGHT_FD = 31, VHOST_USER_SET_INFLIGHT_FD = 32, + VHOST_USER_GPU_SET_SOCKET = 33, VHOST_USER_MAX } VhostUserRequest; @@ -353,6 +354,16 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, return 0; } +int vhost_user_gpu_set_socket(struct vhost_dev *dev, int fd) +{ + VhostUserMsg msg = { + .hdr.request = VHOST_USER_GPU_SET_SOCKET, + .hdr.flags = VHOST_USER_VERSION, + }; + + return vhost_user_write(dev, &msg, &fd, 1); +} + static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, struct vhost_log *log) { diff --git a/MAINTAINERS b/MAINTAINERS index 7e727299de..337eca10a3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1659,6 +1659,12 @@ F: hw/display/virtio-gpu* F: hw/display/virtio-vga.c F: include/hw/virtio/virtio-gpu.h +vhost-user-gpu +M: Marc-André Lureau <marcandre.lureau@redhat.com> +M: Gerd Hoffmann <kraxel@redhat.com> +S: Maintained +F: docs/interop/vhost-user-gpu.rst + Cirrus VGA M: Gerd Hoffmann <kraxel@redhat.com> S: Odd Fixes diff --git a/docs/interop/index.rst b/docs/interop/index.rst index 2df977dd52..28ead07466 100644 --- a/docs/interop/index.rst +++ b/docs/interop/index.rst @@ -15,4 +15,4 @@ Contents: bitmaps live-block-operations pr-helper - + vhost-user-gpu diff --git a/docs/interop/vhost-user-gpu.rst b/docs/interop/vhost-user-gpu.rst new file mode 100644 index 0000000000..9cc1106b02 --- /dev/null +++ b/docs/interop/vhost-user-gpu.rst @@ -0,0 +1,238 @@ +======================= +Vhost-user-gpu Protocol +======================= + +:Licence: This work is licensed under the terms of the GNU GPL, + version 2 or later. See the COPYING file in the top-level + directory. + +.. contents:: Table of Contents + +Introduction +============ + +The vhost-user-gpu protocol is aiming at sharing the rendering result +of a virtio-gpu, done from a vhost-user slave process to a vhost-user +master process (such as QEMU). It bears a resemblance to a display +server protocol, if you consider QEMU as the display server and the +slave as the client, but in a very limited way. Typically, it will +work by setting a scanout/display configuration, before sending flush +events for the display updates. It will also update the cursor shape +and position. + +The protocol is sent over a UNIX domain stream socket, since it uses +socket ancillary data to share opened file descriptors (DMABUF fds or +shared memory). The socket is usually obtained via +``VHOST_USER_GPU_SET_SOCKET``. + +Requests are sent by the *slave*, and the optional replies by the +*master*. + +Wire format +=========== + +Unless specified differently, numbers are in the machine native byte +order. + +A vhost-user-gpu request consists of 2 header fields and a payload. + ++---------+------+---------+ +| request | size | payload | ++---------+------+---------+ + +Header +------ + +:request: ``u32``, type of the request + +:size: ``u32``, size of the payload + +A reply consists only of a payload, whose content depends on the request. + +Payload types +------------- + +Depending on the request type, **payload** can be: + +VhostUserGpuCursorPos +^^^^^^^^^^^^^^^^^^^^^ + ++------------+---+---+ +| scanout-id | x | y | ++------------+---+---+ + +:scanout-id: ``u32``, the scanout where the cursor is located + +:x/y: ``u32``, the cursor postion + +VhostUserGpuCursorUpdate +^^^^^^^^^^^^^^^^^^^^^^^^ + ++-----+-------+-------+--------+ +| pos | hot_x | hot_y | cursor | ++-----+-------+-------+--------+ + +:pos: a ``VhostUserGpuCursorPos``, the cursor location + +:hot_x/hot_y: ``u32``, the cursor hot location + +:cursor: ``[u32; 64 * 64]``, 64x64 RGBA cursor data + +VhostUserGpuScanout +^^^^^^^^^^^^^^^^^^^ + ++------------+---+---+ +| scanout-id | w | h | ++------------+---+---+ + +:scanout-id: ``u32``, the scanout configuration to set + +:w/h: ``u32``, the scanout width/height size + +VhostUserGpuUpdate +^^^^^^^^^^^^^^^^^^ + ++------------+---+---+---+---+------+ +| scanout-id | x | y | w | h | data | ++------------+---+---+---+---+------+ + +:scanout-id: ``u32``, the scanout content to update + +:x/y/w/h: ``u32``, region of the update + +:data: RGBA data (the size is computed based on the region size, and + the request type) + +VhostUserGpuDMABUFScanout +^^^^^^^^^^^^^^^^^^^^^^^^^ + ++------------+---+---+---+---+-----+-----+--------+-------+--------+ +| scanout-id | x | y | w | h | fdw | fwh | stride | flags | fourcc | ++------------+---+---+---+---+-----+-----+--------+-------+--------+ + +:scanout-id: ``u32``, the scanout configuration to set + +:x/y: ``u32``, the location of the scanout within the DMABUF + +:w/h: ``u32``, the scanout width/height size + +:fdw/fdh/stride/flags: ``u32``, the DMABUF width/height/stride/flags + +:fourcc: ``i32``, the DMABUF fourcc + + +C structure +----------- + +In QEMU the vhost-user-gpu message is implemented with the following struct: + +.. code:: c + + typedef struct VhostUserGpuMsg { + uint32_t request; /* VhostUserGpuRequest */ + uint32_t size; /* the following payload size */ + union { + VhostUserGpuCursorPos cursor_pos; + VhostUserGpuCursorUpdate cursor_update; + VhostUserGpuScanout scanout; + VhostUserGpuUpdate update; + VhostUserGpuDMABUFScanout dmabuf_scanout; + uint64_t u64; + } payload; + } QEMU_PACKED VhostUserGpuMsg; + +Protocol features +----------------- + +None yet. + +As the protocol may need to evolve, new messages and communication +changes are negotiated thanks to preliminary +``VHOST_USER_GPU_GET_PROTOCOL_FEATURES`` and +``VHOST_USER_GPU_SET_PROTOCOL_FEATURES`` requests. + +Communication +============= + +Message types +------------- + +``VHOST_USER_GPU_GET_PROTOCOL_FEATURES`` + :id: 1 + :request payload: N/A + :reply payload: ``u64`` + + Get the supported protocol features bitmask. + +``VHOST_USER_GPU_SET_PROTOCOL_FEATURES`` + :id: 2 + :request payload: ``u64`` + :reply payload: N/A + + Enable protocol features using a bitmask. + +``VHOST_USER_GPU_GET_DISPLAY_INFO`` + :id: 3 + :request payload: N/A + :reply payload: ``struct virtio_gpu_resp_display_info`` (from virtio specification) + + Get the preferred display configuration. + +``VHOST_USER_GPU_CURSOR_POS`` + :id: 4 + :request payload: ``VhostUserGpuCursorPos`` + :reply payload: N/A + + Set/show the cursor position. + +``VHOST_USER_GPU_CURSOR_POS_HIDE`` + :id: 5 + :request payload: ``VhostUserGpuCursorPos`` + :reply payload: N/A + + Set/hide the cursor. + +``VHOST_USER_GPU_CURSOR_UPDATE`` + :id: 6 + :request payload: ``VhostUserGpuCursorUpdate`` + :reply payload: N/A + + Update the cursor shape and location. + +``VHOST_USER_GPU_SCANOUT`` + :id: 7 + :request payload: ``VhostUserGpuScanout`` + :reply payload: N/A + + Set the scanout resolution. To disable a scanout, the dimensions + width/height are set to 0. + +``VHOST_USER_GPU_UPDATE`` + :id: 8 + :request payload: ``VhostUserGpuUpdate`` + :reply payload: N/A + + Update the scanout content. The data payload contains the graphical bits. + The display should be flushed and presented. + +``VHOST_USER_GPU_DMABUF_SCANOUT`` + :id: 9 + :request payload: ``VhostUserGpuDMABUFScanout`` + :reply payload: N/A + + Set the scanout resolution/configuration, and share a DMABUF file + descriptor for the scanout content, which is passed as ancillary + data. To disable a scanout, the dimensions width/height are set + to 0, there is no file descriptor passed. + +``VHOST_USER_GPU_DMABUF_UPDATE`` + :id: 10 + :request payload: ``VhostUserGpuUpdate`` + :reply payload: ``u32`` + + The display should be flushed and presented according to updated + region from ``VhostUserGpuUpdate``. + + Note: there is no data payload, since the scanout is shared thanks + to DMABUF, that must have been set previously with + ``VHOST_USER_GPU_DMABUF_SCANOUT``. diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 4dbd530cb9..1be2c74cc7 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -1051,6 +1051,15 @@ Master message types the shared inflight buffer back to slave so that slave could get inflight I/O after a crash or restart. + * VHOST_USER_GPU_SET_SOCKET + Id: 33 + Master payload: N/A + Slave payload: N/A + + Sets the GPU protocol socket file descriptor, which is passed as + ancillary data. The GPU protocol is used to inform the master of + rendering state and updates. See vhost-user-gpu.rst for details. + Slave message types -------------------
Add a new vhost-user message to give a unix socket to a vhost-user backend for GPU display updates. Back when I started that work, I added a new GPU channel because the vhost-user protocol wasn't bidirectional. Since then, there is a vhost-user-slave channel for the slave to send requests to the master. We could extend it with GPU messages. However, the GPU protocol is quite orthogonal to vhost-user, thus I chose to have a new dedicated channel. See vhost-user-gpu.rst for the protocol details. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- contrib/libvhost-user/libvhost-user.h | 1 + include/hw/virtio/vhost-backend.h | 2 + contrib/libvhost-user/libvhost-user.c | 1 + hw/virtio/vhost-user.c | 11 ++ MAINTAINERS | 6 + docs/interop/index.rst | 2 +- docs/interop/vhost-user-gpu.rst | 238 ++++++++++++++++++++++++++ docs/interop/vhost-user.txt | 9 + 8 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 docs/interop/vhost-user-gpu.rst