drmif: add ABI for para-virtual DRM/KMS
PATCH 1/1 drmif: add ABI for para-virtual DRM/KMS
Hi, all!
This patch adds support for para-virtualized DRM/KMS. Comments, ideas are more than welcome.
With best regards,
Oleksandr Andrushchenko
Oleksandr Grytsov
Oleksandr Andrushchenko (1):
drmif: add ABI for para-virtual DRM/KMS
include/xen/interface/io/drmif.h | 505 +++++++++++++++++++++++++++++++++ include/xen/interface/io/drmif_linux.h | 142 +++++++++ 2 files changed, 647 insertions(+) create mode 100644 include/xen/interface/io/drmif.h create mode 100644 include/xen/interface/io/drmif_linux.h
Signature · 7 lines
2.7.4 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xen.org/xen-devel
PATCH 1/1 drmif: add ABI for para-virtual DRM/KMS
This is the ABI for the two halves of a para-virtualized DRM/KMS driver.
--- include/xen/interface/io/drmif.h | 505 +++++++++++++++++++++++++++++++++ include/xen/interface/io/drmif_linux.h | 142 +++++++++ 2 files changed, 647 insertions(+) create mode 100644 include/xen/interface/io/drmif.h create mode 100644 include/xen/interface/io/drmif_linux.h
include/xen/interface/io/drmif.h new file +505 -0
@@ -0,0 +1,505 @@ +/****************************************************************************** +* drmif.h +* +* Unified DRM-device I/O interface for Xen guest OSes. +* +* Permission is hereby granted, free of charge, to any person obtaining a copy +* of this software and associated documentation files (the "Software"), to +* deal in the Software without restriction, including without limitation the +* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +* sell copies of the Software, and to permit persons to whom the Software is +* furnished to do so, subject to the following conditions: +* +* The above copyright notice and this permission notice shall be included in +* all copies or substantial portions of the Software. +* +* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +* DEALINGS IN THE SOFTWARE. +* +* Copyright (C) 2016 EPAM Systems Inc. +* +* Authors: Oleksandr Andrushchenko <[email protected]> +* Oleksandr Grytsov <[email protected]> +*/ + +#ifndef __XEN_PUBLIC_IO_XENDRM_H__ +#define __XEN_PUBLIC_IO_XENDRM_H__ + +/* +* Front->back notifications: When enqueuing a new request, sending a +* notification can be made conditional on req_event (i.e., the generic +* hold-off mechanism provided by the ring macros). Backends must set +* req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()). +* +* Back->front notifications: When enqueuing a new response, sending a +* notification can be made conditional on rsp_event (i.e., the generic +* hold-off mechanism provided by the ring macros). Frontends must set +* rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()). +*/ + +/* +* Feature and Parameter Negotiation +* ================================= +* The two halves of a para-virtual DRM driver utilize nodes within the +* XenStore to communicate capabilities and to negotiate operating parameters. +* This section enumerates these nodes which reside in the respective front and +* backend portions of the XenStore, following the XenBus convention. +* +* All data in the XenStore is stored as strings. Nodes specifying numeric +* values are encoded in decimal. Integer value ranges listed below are +* expressed as fixed sized integer types capable of storing the conversion +* of a properly formated node string, without loss of information. +* +***************************************************************************** +* Backend XenBus Nodes +***************************************************************************** +* +*-------------------------------- Addressing --------------------------------- +* +* Indices used to address frontends, driver instances, cards, +* devices and streams. +* +* frontend_id +* Values: <uint> +* +* Domain ID of the sound frontend. +* +* drv_idx +* Values: <uint> +* +* Zero based contiguous index of the virtualized DRM driver instance in +* this domain. Multiple PV drivers are allowed in the domain +* at the same time. +* +* conn_id +* Values: <uint> +* +* Zero based contiguous index of the connector within the card. +* +*----------------------------- Connector settings ----------------------------- +* resolution +* Values: <[width]x[height]> +* +* Width and height for the connector in pixels separated by +* XENDRM_RESOLUTION_SEPARATOR. For example, +* vdrm/0/connector/0/resolution = "800x600" +* +* +***************************************************************************** +* Frontend XenBus Nodes +***************************************************************************** +* +*----------------------- Request Transport Parameters ----------------------- +* +* These are per stream. +* +* ctrl-channel +* Values: <uint> +* +* The identifier of the Xen connector's control event channel +* used to signal activity in the ring buffer. +* +* ctrl-ring-ref +* Values: <uint> +* +* The Xen grant reference granting permission for the backend to map +* a sole page in a single page sized connector's control ring buffer. +* +* event-channel +* Values: <uint> +* +* The identifier of the Xen connector's event channel +* used to signal activity in the ring buffer. +* +* event-ring-ref +* Values: <uint> +* +* The Xen grant reference granting permission for the backend to map +* a sole page in a single page sized connector's event ring buffer. +*/ + +/* +* STATE DIAGRAMS +* +***************************************************************************** +* Startup * +***************************************************************************** +* +* Tool stack creates front and back state nodes with initial state +* XenbusStateInitialising. +* Tool stack creates and sets up frontend sound configuration nodes per domain. +* +* Front Back +* ================================= ===================================== +* XenbusStateInitialising XenbusStateInitialising +* o Query backend device identification +* data. +* o Open and validate backend device. +* | +* | +* V +* XenbusStateInitWait +* +* o Query frontend configuration +* o Allocate and initialize +* event channels and buffers +* o Publish transport parameters +* that will be in effect during +* this connection. +* | +* | +* V +* XenbusStateInitialised +* +* o Query frontend transport parameters. +* o Connect to the event channels. +* | +* | +* V +* XenbusStateConnected +* +* o Create and initialize OS +* virtual DRM as per configuration. +* | +* | +* V +* XenbusStateConnected +* +* XenbusStateUnknown +* XenbusStateClosed +* XenbusStateClosing +* o Remove virtual sound device +* o Remove event channels +* | +* | +* V +* XenbusStateClosed +* +*/ + +/* +* REQUEST CODES. +*/ +#define XENDRM_OP_DUMB_CREATE 0 +#define XENDRM_OP_DUMB_DESTROY 1 +#define XENDRM_OP_FB_CREATE 2 +#define XENDRM_OP_FB_DESTROY 3 +#define XENDRM_OP_SET_CONFIG 4 +#define XENDRM_OP_PG_FLIP 5 + +/* +* EVENT CODES. +*/ +#define XENDRM_EVT_PG_FLIP 0 + +/* +* XENSTORE FIELD AND PATH NAME STRINGS, HELPERS. +*/ +#define XENDRM_DRIVER_NAME "vdrm" + +#define XENDRM_RESOLUTION_SEPARATOR "x" +/* Field names */ +#define XENDRM_FIELD_CTRL_RING_REF "ctrl-ring-ref" +#define XENDRM_FIELD_CTRL_CHANNEL "ctrl-channel" +#define XENDRM_FIELD_EVT_RING_REF "event-ring-ref" +#define XENDRM_FIELD_EVT_CHANNEL "event-channel" +#define XENDRM_FIELD_RESOLUTION "resolution" + +/* +* STATUS RETURN CODES. +*/ +/* Operation failed for some unspecified reason (e. g. -EIO). */ +#define XENDRM_RSP_ERROR (-1) +/* Operation completed successfully. */ +#define XENDRM_RSP_OKAY 0 + +/* Path entries */ +#define XENDRM_PATH_CONNECTOR "connector" + +/* +* Description of the protocol between frontend and backend driver. +* +* The two halves of a Para-virtual sound driver communicates with +* each other using a shared page and an event channel. +* Shared page contains a ring with request/response packets. +* +***************************************************************************** +* Frontend to backend requests +***************************************************************************** +* +* All request packets have the same length (64 octets) +* +* +* Request dumb creation - request creation of a DRM dumb buffer. +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | operation | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | dumb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | dumb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | width | +* +-----------------+-----------------+-----------------+-----------------+ +* | height | +* +-----------------+-----------------+-----------------+-----------------+ +* | bpp | +* +-----------------+-----------------+-----------------+-----------------+ +* | gref_directory_start | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* id - uint16_t, private guest value, echoed in response +* operation - uint8_t, XENDRM_OP_DUMB_CREATE +* dumb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote dumb to local in requests +* width - uint32_t, width in pixels +* height - uint32_t, height in pixels +* bpp - uint32_t, bits per pixel +* gref_directory_start - grant_ref_t, a reference to the first shared page +* describing shared buffer references. At least one page exists. If shared +* buffer size exceeds what can be addressed by this single page, then +* reference to the next page must be supplied (gref_dir_next_page below +* is not NULL) +* +* Shared page for XENDRM_OP_DUMB_CREATE buffer descriptor (gref_directory in +* the request) employs a list of pages, describing all pages of the shared +* data buffer: +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | gref_dir_next_page | +* +-----------------+-----------------+-----------------+-----------------+ +* | num_grefs | +* +-----------------+-----------------+-----------------+-----------------+ +* | gref[0] | +* +-----------------+-----------------+-----------------+-----------------+ +* | gref[1] | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | gref[N -1] | +* +-----------------+-----------------+-----------------+-----------------+ +* +* gref_dir_next_page - grant_ref_t, reference to the next page describing +* page directory +* num_grefs - number of grefs in this page +* gref[i] - grant_ref_t, reference to a shared page of the dumb buffer +* allocated at XENDRM_OP_DUMB_CREATE +* +* +* Request dumb destruction - destroy a previously allocated dumb buffer: +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | operation | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | dumb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | dumb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* id - uint16_t, private guest value, echoed in response +* operation - uint8_t, XENDRM_OP_DUMB_DESTROY +* dumb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote dumb to local in requests +* +* +* Request framebuffer creation - request creation of a DRM framebuffer. +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | operation | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | dumb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | dumb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | width | +* +-----------------+-----------------+-----------------+-----------------+ +* | height | +* +-----------------+-----------------+-----------------+-----------------+ +* | pixel_format | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* id - uint16_t, private guest value, echoed in response +* operation - uint8_t, XENDRM_OP_FB_CREATE +* dumb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote dumb to local in requests +* fb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote framebuffer to local in requests +* width - uint32_t, width in pixels +* height - uint32_t, height in pixels +* pixel_format - uint32_t, pixel format of the framebuffer +* +* +* Request framebuffer destruction - destroy a previously +* allocated framebuffer buffer: +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | operation | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* id - uint16_t, private guest value, echoed in response +* operation - uint8_t, XENDRM_OP_FB_DESTROY +* dumb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote dumb to local in requests +* +* +* Request configuration set/reset - request to set or reset +* the configuration/mode on CRTC: +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | operation | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | x | +* +-----------------+-----------------+-----------------+-----------------+ +* | y | +* +-----------------+-----------------+-----------------+-----------------+ +* | width | +* +-----------------+-----------------+-----------------+-----------------+ +* | height | +* +-----------------+-----------------+-----------------+-----------------+ +* | bpp | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* Pass all zeros to reset, otherwise command is treated as +* CRTC configuration set. +* +* id - uint16_t, private guest value, echoed in response +* operation - uint8_t, XENDRM_OP_SET_CONFIG +* fb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote framebuffer to local in requests +* x - uint32_t, starting position in pixels by X axis +* y - uint32_t, starting position in pixels by Y axis +* width - uint32_t, width in pixels +* height - uint32_t, height in pixels +* bpp - uint32_t, bits per pixel +* +* +* Request page flip - request to flip a page identified by the framebuffer +* cookie: +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | operation | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | crtc_idx | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* id - uint16_t, private guest value, echoed in response +* operation - uint8_t, XENDRM_OP_PG_FLIP +* crtc_idx - uint32_t, private guest value, echoed in response by the back +* fb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote framebuffer to local in requests +* +***************************************************************************** +* Backend to frontend events +***************************************************************************** +* +* All event packets have the same length (64 octets) +* Events are sent via a shared page allocated by the front and propagated by +* event-channel/event-ring-ref XenStore entries +* +* +* Page flip complete event - event from back to front on page flip completed: +* 0 1 2 3 octet +* +-----------------+-----------------+-----------------+-----------------+ +* | id | type | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* | crtc_idx | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie low 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | fb_cookie high 32-bit | +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| +* +-----------------+-----------------+-----------------+-----------------+ +* | reserved | +* +-----------------+-----------------+-----------------+-----------------+ +* +* id - uint16_t, event id, may be used by front +* type - uint8_t, XENDRM_EVT_PG_FLIP +* crtc_idx - uint32_t, echoed value received in XENDRM_OP_PG_FLIP request +* fb_cookie - uint64_t, unique to guest domain value used by the backend +* to map remote framebuffer to local in requests +* +*/ + +struct xendrm_request { +uint8_t raw[64 ]; +}; + +struct xendrm_response { +uint8_t raw[64 ]; +}; + +struct xendrm_event { +uint8_t raw[64 ]; +}; + +struct xendrm_event_page { +union { +struct xendrm_event raw; +struct { +uint32_t in_cons; +uint32_t in_prod; + } ring; + } u; +}; + +#endif /* __XEN_PUBLIC_IO_XENDRM_H__ */
include/xen/interface/io/drmif_linux.h new file +142 -0
142 lines not on this page · raw message
Signature · 7 lines
2.7.4 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xen.org/xen-devel
Quoted 37 lines
Quoted 1 line
Quoted 1 line · On 24.11.16 at 12:30, <[email protected]> wrote:
include/xen/interface/io/drmif_linux.h new file +33
-0@@ -0,0 +1,142 @@ +/****************************************************************************** +* drmif_linux.h +* +* Unified DRM-device I/O interface for Xen guest OSes +* +* Permission is hereby granted, free of charge, to any person obtaining a copy +* of this software and associated documentation files (the "Software"), to +* deal in the Software without restriction, including without limitation the +* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +* sell copies of the Software, and to permit persons to whom the Software is +* furnished to do so, subject to the following conditions: +* +* The above copyright notice and this permission notice shall be included in +* all copies or substantial portions of the Software. +* +* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +* DEALINGS IN THE SOFTWARE. +* +* Copyright (C) 2016 EPAM Systems Inc. +* +* Authors: Oleksandr Andrushchenko <[email protected]> +* Oleksandr Grytsov <[email protected]> +*/ + +#ifndef __XEN_PUBLIC_IO_XENSND_LINUX_H__ +#define __XEN_PUBLIC_IO_XENSND_LINUX_H__ + +#include <xen/interface/io/ring.h>
include/xen/interface/io/drmif_linux.h line 43
Quoted 10 lines
include/xen/interface/io/drmif_linux.h new file +10
-0+#include <xen/interface/io/drmif.h> +#include <xen/interface/grant_table.h> + +struct xendrm_dumb_create_req { +uint64_t dumb_cookie; +uint32_t width; +uint32_t height; +uint32_t bpp; +grant_ref_t gref_directory_start; +} __packed;
So looking a little more closely at the split here - what's the Linux specific part of the above structure? I.e. perhaps my sndif related comment was then also wrong, and instead you mean to just have everything in a single file? At least that's what it looks like here. May I suggest that you try to follow the fundamental model the other, pre-existing interface headers use? And along that line please keep in mind that frontend and backend may anyway be running in different OSes, so the split done here isn't really architecturally correct.
Also please note that __packed is undefined here.
Quoted 8 lines
include/xen/interface/io/drmif_linux.h new file +8
-0+struct xendrm_req { + union { + struct xendrm_request raw; + struct { + uint16_t id; + uint8_t operation; + uint8_t reserved; + union {
include/xen/interface/io/drmif_linux.h line 98
Quoted 10 lines
include/xen/interface/io/drmif_linux.h new file +10
-0+struct xendrm_dumb_create_req dumb_create; +struct xendrm_dumb_destroy_req dumb_destroy; +struct xendrm_fb_create_req fb_create; +struct xendrm_fb_destroy_req fb_destroy; +struct xendrm_set_config_req set_config; +struct xendrm_page_flip_req pg_flip; + } op; + }data __packed; + } u; +};
A structure whose only member is a union is odd - please make this a union itself.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Hi Oleksandr,
Quoted 8 lines · Oleksandr Andrushchenko
This is the ABI for the two halves of a para-virtualized DRM/KMS driver.
Signed-off-by Oleksandr Andrushchenko <[email protected]>Signed-off-by Oleksandr Grytsov <[email protected]>--- include/xen/interface/io/drmif.h | 505 +++++++++++++++++++++++++++++++++ include/xen/interface/io/drmif_linux.h | 142 +++++++++
Same question as for sndif, any people who wants to use the PV driver for other OS than Linux will have to do it. So, may I ask why don't you write a platform agnostic header?
[...]
Quoted 34 lines
include/xen/interface/io/drmif_linux.h new file +28
-0@@ -0,0 +1,142 @@ +/****************************************************************************** +* drmif_linux.h +* +* Unified DRM-device I/O interface for Xen guest OSes +* +* Permission is hereby granted, free of charge, to any person obtaining a copy +* of this software and associated documentation files (the "Software"), to +* deal in the Software without restriction, including without limitation the +* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +* sell copies of the Software, and to permit persons to whom the Software is +* furnished to do so, subject to the following conditions: +* +* The above copyright notice and this permission notice shall be included in +* all copies or substantial portions of the Software. +* +* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +* DEALINGS IN THE SOFTWARE. +* +* Copyright (C) 2016 EPAM Systems Inc. +* +* Authors: Oleksandr Andrushchenko <[email protected]> +* Oleksandr Grytsov <[email protected]> +*/
include/xen/interface/io/drmif_linux.h line 38
Quoted 10 lines
include/xen/interface/io/drmif_linux.h new file +10
-0+ +#ifndef __XEN_PUBLIC_IO_XENSND_LINUX_H__ +#define __XEN_PUBLIC_IO_XENSND_LINUX_H__ + +#include <xen/interface/io/ring.h> +#include <xen/interface/io/drmif.h> +#include <xen/interface/grant_table.h> + +struct xendrm_dumb_create_req { + uint64_t dumb_cookie;
If I am not mistaken, this field will not be aligned to 64-bit in the final structure (xendrm_req). Which means that you will do an unaligned access. At best the processor will do an unaligned access but slowly (some processors will do 2 memory access), at worst the kernel will crash if alignment check was enabled.
So please re-order the field to avoid this drawbacks.
include/xen/interface/io/drmif_linux.h line 46
Quoted 8 lines
include/xen/interface/io/drmif_linux.h new file +8
-0+uint32_t width; +uint32_t height; +uint32_t bpp; +grant_ref_t gref_directory_start; +} __packed; + +struct xendrm_dumb_destroy_req { +uint64_t dumb_cookie;
Ditto, although here it could be solved by adding a padding before hand.
include/xen/interface/io/drmif_linux.h line 50
Quoted 4 lines
include/xen/interface/io/drmif_linux.h new file +4
-0+} __packed; + +struct xendrm_fb_create_req { +uint64_t dumb_cookie;
Ditto.
include/xen/interface/io/drmif_linux.h line 51
Quoted 1 line
include/xen/interface/io/drmif_linux.h new file +1
-0+uint64_t fb_cookie;
Ditto.
include/xen/interface/io/drmif_linux.h line 58
Quoted 7 lines
include/xen/interface/io/drmif_linux.h new file +7
-0+uint32_t width; +uint32_t height; +uint32_t pixel_format; +} __packed; + +struct xendrm_fb_destroy_req { +uint64_t fb_cookie;
Ditto, although there it could be solved by adding a padding before hand.
include/xen/interface/io/drmif_linux.h line 62
Quoted 4 lines
include/xen/interface/io/drmif_linux.h new file +4
-0+} __packed; + +struct xendrm_set_config_req { +uint64_t fb_cookie;
Ditto.
include/xen/interface/io/drmif_linux.h line 68
Quoted 6 lines
include/xen/interface/io/drmif_linux.h new file +6
-0+uint32_t x; +uint32_t y; +uint32_t width; +uint32_t height; +uint32_t bpp; +} __packed;
Regards,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xen.org/xen-devel
I believe these concerns are exactly the same as for para-virtual audio, so I would suggest that these being addressed after sndif has some resolution on packaging and structure (one or two files) Other than that I would also hear comments on the protocol itself, e.g. related to DRM/KMS
Thank you,
Oleksandr
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 24.11.16 at 15:42, <[email protected]> wrote:
Other than that I would also hear comments on the protocol itself, e.g. related to DRM/KMS
One thing which I think you should have clarified up front is why the existing fbfront is neither sufficient nor possible to extend suitably. Which gets me to a second aspect: The chosen name is rather Linux centric - DRM has quite different a meaning in the Windows world afaik.
And then a more general request: Please stop deleting all context of the mails you reply to. As much as keeping all context (especially when replying to long mails), deleting all of it makes reading harder.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Quoted 3 lines
One thing which I think you should have clarified up front is why the existing fbfront is neither sufficient nor possible to extend suitably.
Framebuffer device is rather outdated way for Linux user-space to draw. All modern software expects DRM/KMS drivers and as
a fallback *may* use fbdev. For that reason, there is a demand on DRM support for guests. So, it doesn't replace fbdev, but rather extend
Quoted 4 lines
Which gets me to a second aspect: The chosen name is rather Linux centric - DRM has quite different a meaning in the Windows world afaik.
Well, that was my intent: define ABI for *Linux DRM/KMS* protocol That said, it is still possible to implement back or front on Windows with this protocol if need be
Quoted 4 lines
And then a more general request: Please stop deleting all context of the mails you reply to. As much as keeping all context (especially when replying to long mails), deleting all of it makes reading harder.
Sorry about that, I am new to Open Source and sometimes do things wrong ;)
Quoted 2 lines
Jan
Signature · 7 lines
Best regards, Oleksandr Andrushchenko _______________________________________________ Xen-devel mailing list [email protected] https://lists.xen.org/xen-devel
Quoted 8 lines
Quoted 4 lines
Quoted 1 line · On 24.11.16 at 16:44, <[email protected]> wrote:
One thing which I think you should have clarified up front is why
the existing fbfront is neither sufficient nor possible to extend suitably.Framebuffer device is rather outdated way for Linux user-space to draw. All modern software expects DRM/KMS drivers and as
a fallback *may* use fbdev. For that reason, there is a demand on DRM support for guests. So, it doesn't replace fbdev, but rather extend
Quoted 7 lines
Quoted 4 lines
Which gets me to a second aspect: The chosen name is rather Linux centric - DRM has quite different a meaning in the Windows world afaik.
Well, that was my intent: define ABI for *Linux DRM/KMS* protocol That said, it is still possible to implement back or front on Windows with this protocol if need be
Hmm, I think you want a PV Linux DRM/KMS driver, but that doesn't mean you want/need a protocol by that name. The interface has to describe virtual hardware, and I don't think you'd call a graphics card "DRM/KMS card"? Hence also the question whether the existing fbfront protocol couldn't be extended - after all modern graphics (3D) cards have also evolved from simple frame buffer (2D) ones.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
One thing which I think you should have clarified up front is why
the existing fbfront is neither sufficient nor possible to extend suitably.Framebuffer device is rather outdated way for Linux user-space to draw. All modern software expects DRM/KMS drivers and as
a fallback *may* use fbdev. For that reason, there is a demand on DRM support for guests. So, it doesn't replace fbdev, but rather extendQuoted 4 lines
Which gets me to a second aspect: The chosen name is rather Linux centric - DRM has quite different a meaning in the Windows world afaik.
Quoted 8 lines
Quoted 3 lines
Well, that was my intent: define ABI for *Linux DRM/KMS* protocol That said, it is still possible to implement back or front on Windows with this protocol if need be
Hmm, I think you want a PV Linux DRM/KMS driver, but that doesn't mean you want/need a protocol by that name. The interface has to describe virtual hardware, and I don't think you'd call a graphics card "DRM/KMS card"?
Good point, then I would suggest we name it dspl for display (PV display), e.g. vdspl, not vdrm.
Quoted 4 lines
Hence also the question whether the existing
fbfront protocol couldn't be extended - after all modern graphics (3D) cards have also evolved from simple frame buffer (2D) ones.
The proposed protocol is almost totally diferent from what existing framebuffer offers. So that was the reason to create a new one which better fits modern graphics and doesn't alter fbif. What is more, real DRM drivers usually support framebuffer emulation, so I was thinking of some flexible solution:
1. If FB is not needed then only DRM/DSPL is in use
2. If also FB is needed then we use existing protocol to add this functionality to guest along with DSPL Nothing tells me that these couldn't be different
back and front drivers/applications for even better flexibility.
Quoted 2 lines
Jan
Best regards,
Oleksandr Andrushchenko
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Quoted 9 lines · Oleksandr Andrushchenko
Quoted 5 lines
Hmm, I think you want a PV Linux DRM/KMS driver, but that doesn't mean you want/need a protocol by that name. The interface has to describe virtual hardware, and I don't think you'd call a graphics
card "DRM/KMS card"?Good point, then I would suggest we name it dspl for display (PV display),
e.g. vdspl, not vdrm.
What about DISPL / vDISPL, which is a little easier to pronounce, remember, and map back to 'display' (while dspl sounds to me a lot like the name of one of those obscure ACPI tables, or something like that! :-P).
Or, if you want it to be consonant only, how about GFX / vGFX, for 'graphics' ('graphix').
Just my 2 cents.
Quoted 4 lines
Quoted 4 lines
Hence also the question whether the existing
fbfront protocol couldn't be extended - after all modern graphics (3D) cards have also evolved from simple frame buffer (2D) ones.
Quoted 11 lines
The proposed protocol is almost totally diferent from what existing framebuffer offers. So that was the reason to create a new one which better fits modern graphics and doesn't alter fbif. What is more, real DRM drivers usually support framebuffer emulation, so I was thinking of some flexible solution:
1. If FB is not needed then only DRM/DSPL is in use
2. If also FB is needed then we use existing protocol to add this functionality to guest along with DSPL Nothing tells me that these couldn't be different
back and frontdrivers/applicationsfor even better flexibility.
FWIW, this seems to me something that could be nice to have.
But I'm no expert in this, and I still have to look closely at the patch and at the protocol and fully make up my mind.
Regards,
Dario
Signature · 4 lines
<<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 24.11.16 at 23:14, <[email protected]> wrote:
Quoted 9 lines · Oleksandr Andrushchenko
Quoted 5 lines
Hmm, I think you want a PV Linux DRM/KMS driver, but that doesn't mean you want/need a protocol by that name. The interface has to describe virtual hardware, and I don't think you'd call a graphics
card "DRM/KMS card"?Good point, then I would suggest we name it dspl for display (PV display),
e.g. vdspl, not vdrm.
Quoted 7 lines
What about DISPL / vDISPL, which is a little easier to pronounce, remember, and map back to 'display' (while dspl sounds to me a lot like the name of one of those obscure ACPI tables, or something like that! :-P).
Or, if you want it to be consonant only, how about GFX / vGFX, for 'graphics' ('graphix').
Yeah, I'd personally like gfx (and hence gfxif.h) better. But again I'll leave the final word to the slated maintainer.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Quoted 16 lines · Jan Beulich
Quoted 16 lines
Quoted 1 line
Quoted 1 line · On 24.11.16 at 23:14, <[email protected]> wrote:
Quoted 9 lines · Oleksandr Andrushchenko
Quoted 5 lines
Hmm, I think you want a PV Linux DRM/KMS driver, but that doesn't mean you want/need a protocol by that name. The interface has to describe virtual hardware, and I don't think you'd call a graphics
card "DRM/KMS card"?Good point, then I would suggest we name it dspl for display (PV display),
e.g. vdspl, not vdrm.What about DISPL / vDISPL, which is a little easier to pronounce, remember, and map back to 'display' (while dspl sounds to me a lot like the name of one of those obscure ACPI tables, or something like that! :-P).
Quoted 4 lines
Quoted 2 lines
Or, if you want it to be consonant only, how about GFX / vGFX, for 'graphics' ('graphix').
Yeah, I'd personally like gfx (and hence
gfxif.h) better. But again I'll leave the final word to the slated maintainer.
I think that GFX may mislead and make people think it is somehow related to what GPU normally does.
So, I would vote for DISPL/vDISPL
Quoted 3 lines
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Oleksandr,
Quoted 5 lines · Oleksandr Andrushchenko
This is the ABI for the two halves of a para-virtualized DRM/KMS driver.
Signed-off-by Oleksandr Andrushchenko <[email protected]>Signed-off-by Oleksandr Grytsov <[email protected]>
...
Quoted 6 lines
+ *
+ * Copyright (C) 2016 EPAM Systems Inc.
+ *
+ * Authors: Oleksandr Andrushchenko <[email protected]>
+ * Oleksandr Grytsov <[email protected]>
+ */
I have a slight concern here about the use of private e-mail addresses in the Signed-off-by tags and authors field in the (c) notice. It contradicts "Copyright (C) 2016 EPAM Systems Inc." and will make it very hard to execute licenses changes, if that needs to be done in future.
The Signed-off-by tag is a way to identify the (c) holder of a patch. I am assuming that in this case the (c) holder would be EPAM. Is this correct?
If yes, please change the Signed-off-by: and author fields to an EPAM e-mail address. You can still send from gmail addresses though. If not, please change the "Copyright (C) 2016 EPAM Systems Inc." accordingly
I am assuming there is a similar issue in the other patches. Please make sure you fix all of them.
Best Regards
Lars
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Sorry about that, will fix
Quoted 40 lines · Lars Kurth
Oleksandr,
Quoted 5 lines · Oleksandr Andrushchenko
This is the ABI for the two halves of a para-virtualized DRM/KMS driver.
Signed-off-by Oleksandr Andrushchenko <[email protected]>Signed-off-by Oleksandr Grytsov <[email protected]>...
Quoted 6 lines
+ *
+ * Copyright (C) 2016 EPAM Systems Inc.
+ *
+ * Authors: Oleksandr Andrushchenko <[email protected]>
+ * Oleksandr Grytsov <[email protected]>
+ */I have a slight concern here about the use of private e-mail addresses in the Signed-off-by tags and authors field in the (c) notice. It contradicts "Copyright (C) 2016 EPAM Systems Inc." and will make it very hard to execute licenses changes, if that needs to be done in future.
The Signed-off-by tag is a way to identify the (c) holder of a patch. I am assuming that in this case the (c) holder would be EPAM. Is this correct?
If yes, please change the Signed-off-by: and author fields to an EPAM e-mail address. You can still send from gmail addresses though. If not, please change the "Copyright (C) 2016 EPAM Systems Inc." accordingly
I am assuming there is a similar issue in the other patches. Please make sure you fix all of them.
Best Regards
Lars
Signature · 7 lines
Best regards, Oleksandr Andrushchenko _______________________________________________ Xen-devel mailing list [email protected] https://lists.xen.org/xen-devel