|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] displif: add ABI for para-virtual display
On Wed, Feb 15, 2017 at 09:33:41AM +0200, Oleksandr Andrushchenko wrote:
> On 02/14/2017 10:27 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Feb 13, 2017 at 10:50:49AM +0200, Oleksandr Andrushchenko wrote:
> > > Hi, Konrad!
> > >
> > > Thank you for reviewing this.
> > >
> > > On 02/10/2017 11:27 PM, Konrad Rzeszutek Wilk wrote:
> > > > On Fri, Feb 10, 2017 at 09:29:58AM +0200, Oleksandr Andrushchenko wrote:
> > > > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > > >
> > > > > This is the ABI for the two halves of a para-virtualized
> > > > > display driver.
> > > > >
> > > > > This protocol aims to provide a unified protocol which fits more
> > > > > sophisticated use-cases than a framebuffer device can handle. At the
> > > > > moment basic functionality is supported with the intention to extend:
> > > > > o multiple dynamically allocated/destroyed framebuffers
> > > > > o buffers of arbitrary sizes
> > > > > o better configuration options including multiple display support
> > > > >
> > > > > Note: existing fbif can be used together with displif running at the
> > > > > same time, e.g. on Linux one provides framebuffer and another DRM/KMS
> > > > >
> > > > > Future extensions to the existing protocol may include:
> > > > > o allow display/connector cloning
> > > > > o allow allocating objects other than display buffers
> > > > > o add planes/overlays support
> > > > > o support scaling
> > > > > o support rotation
> > > > >
> > > > > ==================================================
> > > > > Rationale for introducing this protocol instead of
> > > > > using the existing fbif:
> > > > > ==================================================
> > > > >
> > > > > 1. In/out event sizes
> > > > > o fbif - 40 octets
> > > > > o displif - 40 octets
> > > > > This is only the initial version of the displif protocol
> > > > > which means that there could be requests which will not fit
> > > > > (WRT introducing some GPU related functionality
> > > > > later on). In that case we cannot alter fbif sizes as we need to
> > > > > be backward compatible an will be forced to handle those
> > > > > apart of fbif.
> > > > >
> > > > > 2. Shared page
> > > > > Displif doesn't use anything like struct xenfb_page, but
> > > > > DEFINE_RING_TYPES(xen_displif, struct xendispl_req, struct
> > > > > xendispl_resp) which is a better and more common way.
> > > > > Output events use a shared page which only has in_cons and in_prod
> > > > > and all the rest is used for incoming events. Here struct xenfb_page
> > > > > could probably be used as is despite the fact that it only has a half
> > > > > of a page for incoming events which is only 50 events. (consider
> > > > > something like 60Hz display)
> > > > >
> > > > > 3. Amount of changes.
> > > > > fbif only provides XENFB_TYPE_UPDATE and XENFB_TYPE_RESIZE
> > > > > events, so it looks like it is easier to get fb support into displif
> > > > .. would it make sense to reserve some of those values (2, 3)
> > > > in the XENDISPL_OP_ values? So that if this happens there is a nice
> > > > fit in there? Thought looking at the structure there is no easy
> > > > way to 'overlay' the xenfb_out_event structure as it is missing the
> > > > 'id'.
> > > >
> > > > I guess one can get creative.
> > > >
> > > > Or you could swap positions of 'id' and 'type'? And then it would fit
> > > > much
> > > > nicer?
> > > yeap, in order no one needs to be creative, why not
> > > explicitly define those?
> > > Anyways, it won't be possible to simply lay the structures from
> > > fbif on top of displif (different structure, size, workflow etc.),
> > > what is more that would give some room to find workarounds, rather than
> > > have well defined solution. BTW, there was an attempt [1] to update
> > > fbdev to meet modern application needs. If we decide to add FBDEV
> > > functionality into this protocol, then I can re-use already
> > > proven to work solution from [1] into DISPLIF, but defining
> > > structures and events to fit the current protocol.
> > I was thinking that since the size of the structure is 64bytes
> > you have enough space to jam in the old structures too.
> yes, I understand your intention
> >
> > Naturally the driver would need adjustments as it offsets
> > of where this goes are all wrong.
> exactly
> >
> > > What do you think?
> so, what is the decision? Options:
> 1) Leave the protocol as is, if need be it can be extended later
> 2) Implement something similar to [1]
> 3) Give room for workarounds and reserve XENDISPL_OP_XXX for
> what current fbif uses
I would do 3) so that it becomes easier to migrate the old code over.
> > > > > than vice versa. displif at the moment has 6 requests and 1 event,
> > > > > multiple connector support, etc.
> > > > >
> > > > > Changes since v2:
> > > > > * updated XenStore configuration template/pattern
> > > > > * added "Recovery flow" to state diagram description
> > > > > * renamed gref_directory_start to gref_directory
> > > > > * added missing "versions" and "version" string constants
> > > > >
> > > > > Changes since v1:
> > > > > * fixed xendispl_event_page padding size
> > > > > * added versioning support
> > > > > * explicitly define value types for XenStore fields
> > > > > * text decoration re-work
> > > > > * added offsets to ASCII box notation
> > > > >
> > > > > Changes since initial:
> > > > > * DRM changed to DISPL, protocol made generic
> > > > > * major re-work addressing issues raised for sndif
> > > > >
> > > > > Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> > > > > Signed-off-by: Oleksandr Andrushchenko
> > > > > <oleksandr_andrushchenko@xxxxxxxx>
> > > > > ---
> > > > > xen/include/public/io/displif.h | 778
> > > > > ++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 778 insertions(+)
> > > > > create mode 100644 xen/include/public/io/displif.h
> > > > >
> > > > > diff --git a/xen/include/public/io/displif.h
> > > > > b/xen/include/public/io/displif.h
> > > > > new file mode 100644
> > > > > index 000000000000..849f27fe5f1d
> > > > > --- /dev/null
> > > > > +++ b/xen/include/public/io/displif.h
> > > > > @@ -0,0 +1,778 @@
> > > > > +/******************************************************************************
> > > > > + * displif.h
> > > > > + *
> > > > > + * Unified display 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-2017 EPAM Systems Inc.
> > > > > + *
> > > > > + * Authors: Oleksandr Andrushchenko
> > > > > <oleksandr_andrushchenko@xxxxxxxx>
> > > > > + * Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> > > > > + */
> > > > > +
> > > > > +#ifndef __XEN_PUBLIC_IO_DISPLIF_H__
> > > > > +#define __XEN_PUBLIC_IO_DISPLIF_H__
> > > > > +
> > > > > +#include "ring.h"
> > > > > +#include "../grant_table.h"
> > > > > +
> > > > > +/*
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Main features provided by the protocol
> > > > > +
> > > > > ******************************************************************************
> > > > > + * This protocol aims to provide a unified protocol which fits more
> > > > > + * sophisticated use-cases than a framebuffer device can handle. At
> > > > > the
> > > > > + * moment basic functionality is supported with the intention to be
> > > > > extended:
> > > > > + * o multiple dynamically allocated/destroyed framebuffers
> > > > > + * o buffers of arbitrary sizes
> > > > > + * o buffer allocation at either back or front end
> > > > > + * o better configuration options including multiple display support
> > > > > + *
> > > > > + * Note: existing fbif can be used together with displif running at
> > > > > the
> > > > > + * same time, e.g. on Linux one provides framebuffer and another
> > > > > DRM/KMS
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Direction of improvements
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Future extensions to the existing protocol may include:
> > > > > + * o display/connector cloning
> > > > > + * o allocation of objects other than display buffers
> > > > > + * o plane/overlay support
> > > > > + * o scaling support
> > > > > + * o rotation support
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Feature and Parameter Negotiation
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + * Front->back notifications: when enqueuing a new request, sending a
> > > > > + * notification can be made conditional on xendispl_req (i.e., the
> > > > > generic
> > > > > + * hold-off mechanism provided by the ring macros). Backends must set
> > > > > + * xendispl_req 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 xendispl_resp (i.e., the
> > > > > generic
> > > > > + * hold-off mechanism provided by the ring macros). Frontends must
> > > > > set
> > > > > + * xendispl_resp appropriately (e.g., using
> > > > > RING_FINAL_CHECK_FOR_RESPONSES()).
> > > > > + *
> > > > > + * The two halves of a para-virtual display driver utilize nodes
> > > > > within
> > > > > + * XenStore to communicate capabilities and to negotiate operating
> > > > > parameters.
> > > > > + * This section enumerates these nodes which reside in the
> > > > > respective front and
> > > > > + * backend portions of XenStore, following the XenBus convention.
> > > > > + *
> > > > > + * All data in 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.
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Example configuration
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + * Note: depending on the use-case backend can expose more display
> > > > > connectors
> > > > > + * than the underlying HW physically has by employing SW graphics
> > > > > compositors
> > > > > + *
> > > > > + * This is an example of backend and frontend configuration:
> > > > > + *
> > > > > + *--------------------------------- Backend
> > > > > -----------------------------------
> > > > > + *
> > > > > + * /local/domain/0/backend/vdispl/1/0/frontend-id = "1"
> > > > > + * /local/domain/0/backend/vdispl/1/0/frontend =
> > > > > "/local/domain/1/device/vdispl/0"
> > > > > + * /local/domain/0/backend/vdispl/1/0/state = "4"
> > > > > + * /local/domain/0/backend/vdispl/1/0/versions = "1,2"
> > > > > + * /local/domain/0/backend/vdispl/1/0/features = "be_alloc"
> > > > > + *
> > > > > + *--------------------------------- Frontend
> > > > > ----------------------------------
> > > > > + *
> > > > > + * /local/domain/1/device/vdispl/0/backend-id = "0"
> > > > > + * /local/domain/1/device/vdispl/0/backend =
> > > > > "/local/domain/0/backend/vdispl/1/0"
> > > > > + * /local/domain/1/device/vdispl/0/state = "4"
> > > > > + * /local/domain/1/device/vdispl/0/version = "1"
> > > > > + *
> > > > > + *-------------------------- Connector 0 configuration
> > > > > ------------------------
> > > > > + *
> > > > > + * /local/domain/1/device/vdispl/0/0/resolution = "1920x1080"
> > > > > + * /local/domain/1/device/vdispl/0/0/ctrl-ring-ref = "2832"
> > > > > + * /local/domain/1/device/vdispl/0/0/ctrl-channel = "15"
> > > > > + * /local/domain/1/device/vdispl/0/0/event-ring-ref = "387"
> > > > > + * /local/domain/1/device/vdispl/0/0/event-channel = "16"
> > > > > + *
> > > > > + *-------------------------- Connector 1 configuration
> > > > > ------------------------
> > > > > + *
> > > > > + * /local/domain/1/device/vdispl/0/1/resolution = "800x600"
> > > > > + * /local/domain/1/device/vdispl/0/1/ctrl-ring-ref = "2833"
> > > > > + * /local/domain/1/device/vdispl/0/1/ctrl-channel = "17"
> > > > > + * /local/domain/1/device/vdispl/0/1/event-ring-ref = "388"
> > > > > + * /local/domain/1/device/vdispl/0/1/event-channel = "18"
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Backend XenBus Nodes
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + *----------------------------- Protocol version
> > > > > ------------------------------
> > > > > + *
> > > > > + * versions
> > > > > + * Values: <string>
> > > > > + *
> > > > > + * List of XENDISPL_LIST_SEPARATOR separated protocol versions
> > > > > supported
> > > > > + * by the backend. For example "1,2,3".
> > > > > + *
> > > > > + * features
> > > > > + * Values: <list of strings>
> > > > > + *
> > > > > + * XENDISPL_LIST_SEPARATOR separated list of features
> > > > > + * XENDISPL_FEATURE_BE_??? that the backend supports.
> > > > > + *
> > > > > + * be_alloc
> > > > > + * Backend can be a buffer provider/allocator during
> > > > > + * XENDISPL_OP_DBUF_CREATE operation (see below for
> > > > > negotiation).
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Frontend XenBus Nodes
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + *-------------------------------- Addressing
> > > > > ---------------------------------
> > > > > + *
> > > > > + * dom-id
> > > > > + * Values: <uint16_t>
> > > > > + *
> > > > > + * Domain identifier.
> > > > > + *
> > > > > + * dev-id
> > > > > + * Values: <uint16_t>
> > > > > + *
> > > > > + * Device identifier.
> > > > > + *
> > > > > + * conn-idx
> > > > > + * Values: <uint8_t>
> > > > > + *
> > > > > + * Zero based contigous index of the connector.
> > > > > + * /local/domain/<dom-id>/device/vdispl/<dev-id>/<conn-idx>/...
> > > > > + *
> > > > > + *----------------------------- Protocol version
> > > > > ------------------------------
> > > > > + *
> > > > > + * version
> > > > > + * Values: <string>
> > > > > + *
> > > > > + * Protocol version, chosen among the ones supported by the
> > > > > backend.
> > > > > + *
> > > > > + *----------------------------- Connector settings
> > > > > ----------------------------
> > > > > + *
> > > > > + * resolution
> > > > > + * Values: <width, uint32_t>x<height, uint32_t>
> > > > > + *
> > > > > + * Width and height of the connector in pixels separated by
> > > > > + * XENDISPL_RESOLUTION_SEPARATOR.
> > > > Is there an default depth?
> > > no
> > > > 32?
> > > no defaults
> > Should we have a default?
> >
> > And perhaps also say that all the operations are bounded by these?
> not sure we need default here: this is *up to backend*
> what to do with *depths* and *resolutions* which are different
> from what HW display supports, e.g. those buffers
> can be just a part of the final composition.
> For example, backend utilizes a SW compositor which
> makes screen compositing with 1920x1080x32 (lets say, for
> example, this is the most suitable one for the GPU used
> and also this is used for a remote desktop - use-cases may
> differ)
> But, the real HW display is only 1200x800x24. So, once the
> composition is done, it will be downscaled/resampled by any
> means to what HW display wants.
>
> Hope, this better explains why buffers may differ from
> what "resolution" says and depth used: buffer is not
> exactly what you see finally, so it can be bigger, smaller
> or equal - all valid
OK, you may to include this explanation in the document.
> > > > Or should that also be negotiated?
> > > frontend requests the desired value of it, backend provides it
> > > or rejects the request, so front will try other value.
> > > So, yes, this is kind of negotiation here (EINVAL based :)
> > > I will also put a note that "resolution" only defines visible area,
> > > thus display and frame buffer sizes can be different from this value
> > OK, which I suspect means that it can only be smaller, not larger.
> See above, no influence on buffer size
> > > > > + *
> > > > > + *------------------ Connector Request Transport Parameters
> > > > > -------------------
> > > > > + *
> > > > > + * ctrl-channel
> > > > Please call it 'ctrl-event-channel'
> > > I will
> > > > > + * Values: <uint32_t>
> > > > > + *
> > > > > + * The identifier of the Xen connector's control event channel
> > > > > + * used to signal activity in the ring buffer.
> > > > > + *
> > > > > + * ctrl-ring-ref
> > > > > + * Values: <uint32_t>
> > > > > + *
> > > > > + * 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: <uint32_t>
> > > > > + *
> > > > > + * The identifier of the Xen connector's event channel
> > > > > + * used to signal activity in the ring buffer.
> > > > > + *
> > > > > + * event-ring-ref
> > > > > + * Values: <uint32_t>
> > > > > + *
> > > > > + * The Xen grant reference granting permission for the backend
> > > > > to map
> > > > > + * a sole page in a single page sized connector's event ring
> > > > > buffer.
> > > > So you are making two rings and two event channels - and it looks like
> > > > the
> > > > operations are going via the control ring.. but what goes through the
> > > > the other ring buffer?
> > > Events from backend to frontend, currently only XENDISPL_EVT_PG_FLIP
> > Aaaaah! In that case I would change it from 'ctrl-' to
> > 'req-alloc' to match with.
> well, this is the control/command path, for that reason I
> still see its name as "ctrl/cmd-".
> What is more, I can agree to "req", but why "alloc"?
> This path is not only used for allocations, but configuration as well.
> In the future, I would expect other commands/control be passed
> over it.
> So, there is a command/control path ("ctrl-") and event path ("event-")
> which I believe do reflect their usage.
Isn't this tied to 'XENDISPL_FEATURE_BE_ALLOC' feature? If so I would
call it with that name in mind.
> > > I will define 2 sections:
> > > *------------------ Connector Request Transport Parameters
> > > -------------------
> > > *
> > > * ctrl-event-channel
> > > * ctrl-ring-ref
> > > *
> > > *------------------- Connector Event Transport Parameters
> > > --------------------
> > > *
> > > * event-channel
> > > * event-ring-ref
> > >
> > > > Or is the other ring buffer the one that is created via
> > > > 'gref_directory' ?
> > > no
> > > At the bottom:
> > > * In order to deliver asynchronous events from back to front a shared
> > > page
> > > is
> > > * allocated by front and its gref propagated to back via XenStore
> > > entries
> > > * (event-XXX).
> > AAnd you may want to say this is guarded by REQ_ALLOC feature right?
> Not sure I understood you. Event path is totally independent
> from any feature, e.g. REQ_ALLOC.
> It just provides means to send async events
> from back to front, "page flip done" in my case.
<scratche his head> Why do you need a seperate ring to send
responses back? Why not use the same ring on which requests
were sent
> > > > If so, wouldn't you need mulitple event channels? And also the size of
> > > > those
> > > > rings is much much bigger. But it is not really a ring. It looks to hold
> > > > some opaque data?
> > > >
> > > > -ECONFUSED.
> > > > > + */
> > > > > +
> > > > > +/*
> > > > > +
> > > > > ******************************************************************************
> > > > > + * STATE DIAGRAMS
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + * Tool stack creates front and back state nodes with initial state
> > > > > + * XenbusStateInitialising.
> > > > > + * Tool stack creates and sets up frontend display configuration
> > > > > + * nodes per domain.
> > > > > + *
> > > > > + *-------------------------------- Normal flow
> > > > > --------------------------------
> > > > > + *
> > > > > + * 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 per configured
> > > > > + * connector.
> > > > > + * 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 display connectors
> > > > > + * as per configuration.
> > > > > + * |
> > > > > + * |
> > > > > + * V
> > > > > + * XenbusStateConnected
> > > > > + *
> > > > > + * XenbusStateUnknown
> > > > > + * XenbusStateClosed
> > > > > + * XenbusStateClosing
> > > > > + * o Remove virtual display device
> > > > > + * o Remove event channels
> > > > > + * |
> > > > > + * |
> > > > > + * V
> > > > > + * XenbusStateClosed
> > > > > + *
> > > > > + *------------------------------- Recovery flow
> > > > > -------------------------------
> > > > > + *
> > > > > + * In case of frontend unrecoverable errors backend handles that as
> > > > > + * if frontend goes into the XenbusStateClosed state.
> > > > > + *
> > > > > + * In case of backend unrecoverable errors frontend tries removing
> > > > > + * the emulated device. If this is possible at the moment of error,
> > > > What emulated device?
> > > s/emulated/virtualized/g for all?
> > /me nods.
> done
> > > > > + * then frontend goes into the XenbusStateInitialising state and is
> > > > > ready for
> > > > > + * new connection with backend. If the emulated device is still in
> > > > > use and
> > > > emulated device?
> > > >
> > > > > + * cannot be removed, then frontend goes into the
> > > > > XenbusStateReconfiguring state
> > > > .. and what is done in XenbusStateReconfiguring state?
> > > >
> > > > I am assuming 'resolution' is examined?
> > > >
> > > > > + * until either the emulated device removed or backend initiates a
> > > > > new
> > > > > + * connection. On the emulated device removal frontend goes into the
> > > > emulated device?
> > > The whole idea here is to let frontend recover from backend failure.
> > > What happens when backend dies: frontend cannot send requests
> > > to the backend and thus cannot emulate (virtualize) display
> > > device anymore. There is no simple solution like disconnecting rings
> > > and waiting for the backend to re-connect, as display device, just like
> > > sound devices, does hold some state: configuration in use, number of
> > > display
> > > buffers, framebuffers, application state, framework "watchdogs" (like
> > > vertical blank time-outs and recovery at framework level) etc.
> > > So, in most cases, it would require frontend to implement complex recovery
> > > logic. Instead, what I suggest, that we go into some state, which tells
> > > that
> > > we are not yet ready to go into Initialized state and still need some
> > > time:
> > > at this time frontend will try to make sure no new clients from user-space
> > > are accepted, it allows the current client to exit gracefully by returning
> > > error codes etc. Once all the clients are gone frontend can reinitialize
> > > the virtualized device, e.g. DRM/KMS driver, and get into Initialized
> > > state
> > > again.
> > That is fine.
> > > Thus, I was thinking of XenbusStateReconfiguringstate as appropriate in
> > > this
> > > case
> > Right, but somebody has to move to this state. Who would do it?
> when backend dies its state changes to "closed".
> At this moment front tries to remove virtualized device
> and if it is possible/done, then it goes into "initialized"
> state. If not - "reconfiguring".
> So, you would ask how does the front go from "reconfiguring"
> into "initialized" state? This is OS/front specific, but:
> 1. the underlying framework, e.g. DRM/KMS, ALSA, provide
> a callback(s) to signal that the last client to the
> virtualized device has gone and the driver can be removed
> (equivalent to module's usage counter 0)
> 2. one can schedule a delayed work (timer/tasklet/workqueue)
> to periodically check if this is the right time to re-try
> the removal and remove
>
> In both cases, after the virtualized device has been removed we move
> into "initialized" state again and are ready for new connections
> with backend (if it arose from the dead :)
> > Would the
> > frontend have some form of timer to make sure that the backend is still
> > alive? And if it had died then move to Reconfiguring?
> There are at least 2 ways to understand if back is dead:
> 1. XenBus state change (back is closed)
.. If the backend does a nice shutdown..
> 2. time-outs on requests from front to back
>
Right.
>
> Bottom line on the "recovery flow": what I describe could be put in
> the description as an example to clarify more this.
> > > > > + * XenbusStateInitialising state.
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * REQUEST CODES
> > > > > +
> > > > > ******************************************************************************
> > > > > + */
> > > > > +#define XENDISPL_OP_DBUF_CREATE 0
> > > > > +#define XENDISPL_OP_DBUF_DESTROY 1
> > > > > +#define XENDISPL_OP_FB_ATTACH 2
> > > > > +#define XENDISPL_OP_FB_DETACH 3
> > > > > +#define XENDISPL_OP_SET_CONFIG 4
> > > > > +#define XENDISPL_OP_PG_FLIP 5
> > > > > +
> > > > > +/*
> > > > > +
> > > > > ******************************************************************************
> > > > > + * EVENT CODES
> > > > > +
> > > > > ******************************************************************************
> > > > > + */
> > > > > +#define XENDISPL_EVT_PG_FLIP 0
> > > > > +
> > > > > +/*
> > > > > +
> > > > > ******************************************************************************
> > > > > + * XENSTORE FIELD AND PATH NAME STRINGS, HELPERS
> > > > > +
> > > > > ******************************************************************************
> > > > > + */
> > > > > +#define XENDISPL_DRIVER_NAME "vdispl"
> > > > > +
> > > > > +#define XENDISPL_LIST_SEPARATOR ","
> > > > > +#define XENDISPL_RESOLUTION_SEPARATOR "x"
> > > > > +/* Field names */
> > > > > +#define XENDISPL_FIELD_BE_VERSIONS "versions"
> > > > > +#define XENDISPL_FIELD_FE_VERSION "version"
> > > > > +#define XENDISPL_FIELD_FEATURES "features"
> > > > > +#define XENDISPL_FIELD_CTRL_RING_REF "ctrl-ring-ref"
> > > > > +#define XENDISPL_FIELD_CTRL_CHANNEL "ctrl-channel"
> > > > > +#define XENDISPL_FIELD_EVT_RING_REF "event-ring-ref"
> > > > > +#define XENDISPL_FIELD_EVT_CHANNEL "event-channel"
> > > > > +#define XENDISPL_FIELD_RESOLUTION "resolution"
> > > > > +
> > > > > +#define XENDISPL_FEATURE_BE_ALLOC "be_alloc"
> > > > > +
> > > > > +/*
> > > > > +
> > > > > ******************************************************************************
> > > > > + * STATUS RETURN CODES
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + * Status return code is zero on success and -XEN_EXX on failure.
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Assumptions
> > > > > +
> > > > > ******************************************************************************
> > > > > + * o usage of grant reference 0 as invalid grant reference:
> > > > > + * grant reference 0 is valid, but never exposed to a PV driver,
> > > > > + * because of the fact it is already in use/reserved by the PV
> > > > > console.
> > > > > + * o all references in this document to page sizes must be treated
> > > > > + * as pages of size XEN_PAGE_SIZE unless otherwise noted.
> > > > > + *
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Description of the protocol between frontend and backend
> > > > > driver
> > > > > +
> > > > > ******************************************************************************
> > > > > + *
> > > > > + * The two halves of a Para-virtual display driver communicate with
> > > > > + * each other using shared pages and event channels.
> > > > > + * Shared page contains a ring with request/response packets.
> > > > > + *
> > > > > + * All reserved fields in the structures below must be 0.
> > > > > + * Display buffers's cookie of value 0 treated as invalid.
> > > > > + * Framebuffer's cookie of value 0 treated as invalid.
> > > > > + *
> > > > > + *---------------------------------- Requests
> > > > > ---------------------------------
> > > > > + *
> > > > > + * All requests/responses, which are not connector specific, must be
> > > > > sent over
> > > > > + * control ring of the connector with index 0.
> > > > > + *
> > > > > + * For all request/response/event packets that use cookies:
> > > > > + * dbuf_cookie - uint64_t, unique to guest domain value used by
> > > > > the backend
> > > > > + * to map remote display buffer to its local one
> > > > > + * fb_cookie - uint64_t, unique to guest domain value used by the
> > > > > backend
> > > > > + * to map remote framebuffer to its local one
> > > > > + *
> > > > > + * All request packets have the same length (64 octets)
> > > > > + * All request packets have common header:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | operation | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * id - uint16_t, private guest value, echoed in response
> > > > > + * operation - uint8_t, operation code, XENDISPL_OP_???
> > > > > + *
> > > > > + * Request dbuf creation - request creation of a display buffer.
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id |_OP_DBUF_CREATE | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | dbuf_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | dbuf_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | width
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | height
> > > > > | 24
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | bpp
> > > > > | 28
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | buffer_sz
> > > > > | 32
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | flags
> > > > > | 36
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | gref_directory
> > > > > | 40
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 44
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * Must be sent over control ring of the connector with index 0.
> > > > > + *
> > > > > + * width - uint32_t, width in pixels
> > > > > + * height - uint32_t, height in pixels
> > > > Why? Isn't that in the 'resolution' XenBus entry?
> > > It can be bigger that the resolution, for example
> > > if scaling will take place.
> > How would you reflect the scaling part of this? Is that something
> > that is assumed if the XenBus resolution value < widthxheight values?
> As in the above example with back utilizing an SW compositor,
> scaling will be done by the backend, front nows nothing
> about the destiny of the buffers it wants to show
> So, no need to worry about widthXheightXdepth of the buffers
> > > > If not, why even have the 'resolution' field?
> > > The confusion comes from the fact that the resolution is visible area:
> > > will state that clearly.
> > > > And also - what if the width or height are bigger than 'resolution'?
> > > > Should -EINVAL be returned?
> > > >
> > > no problem here: we can have buffers bigger than visible area
> > > > > + * bpp - uint32_t, bits per pixel
> > > > > + * buffer_sz - uint32_t, buffer size to be allocated, octets
> > > > > + * flags - uint32_t, flags of the operation
> > > > > + * o XENDISPL_DBUF_FLG_REQ_ALLOC - if set, then backend is
> > > > > requested
> > > > > + * to allocate the buffer with the parameters provided in this
> > > > > request.
> > > > > + * Page directory is handled as follows:
> > > > > + * Frontend on request:
> > > > > + * o allocates pages for the directory
> > > > > + * o grants permissions for the pages of the directory
> > > > > + * o sets gref_dir_next_page fields
> > > > > + * Backend on response:
> > > > > + * o grants permissions for the pages of the buffer allocated
> > > > <scratches his head>
> > > > > + * o fills in page directory with grant references
> > > > The backend provides the pages instead of the frontend? But you said
> > > > 'allocated pages for the directory' in the frontned part - and since you
> > > > say 'pages' that means more than one? Or did you mean 'the
> > > > gref_directory'
> > > > page and the 'gref_dir_next_page' pages? You may want to be explicit.
> > > >
> > > sure, I will state it clearly that only directory pages
> > > are allocated: gref_directory + gref_dir_next_page(s)
> > Thx
> > > > And what if the 'flags' is zero?
> > > it would mean that even that the backend can allocate buffers
> > > for us we don't want it to do so, we will allocate on our own:
> > > flag is not set
> > > > Or 0x314? Perhaps also mention
> > > > the default one (0 I presume) which would make the backend map
> > > > the frontend pages provided in 'gref_directory'?
> > > yes, I'll put a note on defaults
> > Awesome!
> > > > > + * gref_directory - grant_ref_t, a reference to the first shared page
> > > > > + * describing shared buffer references. At least one page exists.
> > > > > If shared
> > > > > + * buffer size (buffer_sz) exceeds what can be addressed by this
> > > > > single page,
> > > > ^- extra space
> > > done
> > > > > + * then reference to the next page must be supplied (see
> > > > > gref_dir_next_page
> > > > > + * below)
> > > > > + */
> > > > What are some of the return errors ? Would it make sense to mention
> > > > some?
> > > not sure, this way will dictate implementation details,
> > > but we are defining protocol
> > True to an extent.
> And that extent makes me believe we shouldn't dictate
> error codes here. POSIX error codes are (sometimes :)
> more than descriptive )
> > > > What to for example if there are multiple _OP_DBUF_CREATE
> > > > with the same dbuf_cookie but with different width/height?
> > > it will be an error, I will clearly state this:
> > > * An attempt to create multiple display buffers with the same
> > > dbuf_cookie
> > > is
> > > * an error. dbuf_cookie can be re-used after destroying the
> > > corresponding
> > > * display buffer.
> > May also want to say what error value it should be. -EAGAIN? No probably
> > something
> > different.
> As above, I would prefer to not put anything
OK.
> > > > > +
> > > > > +#define XENDISPL_DBUF_FLG_REQ_ALLOC 0x0001
> > > > > +
> > > > > +struct xendispl_dbuf_create_req {
> > > > > + uint64_t dbuf_cookie;
> > > > > + uint32_t width;
> > > > > + uint32_t height;
> > > > > + uint32_t bpp;
> > > > > + uint32_t buffer_sz;
> > > > > + uint32_t flags;
> > > > > + grant_ref_t gref_directory;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Shared page for XENDISPL_OP_DBUF_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
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | gref[0]
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | gref[i]
> > > > > | i*4+8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | gref[N - 1]
> > > > > | N*4+8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * gref_dir_next_page - grant_ref_t, reference to the next page
> > > > > describing
> > > > > + * page directory. Must be 0 if there are no more pages in the
> > > > > list.
> > > > > + * gref[i] - grant_ref_t, reference to a shared page of the buffer
> > > > > + * allocated at XENDISPL_OP_DBUF_CREATE
> > > > > + *
> > > > > + * Number of grant_ref_t entries in the whole page directory is not
> > > > > + * passed, but instead can be calculated as:
> > > > > + * num_grefs_total = (XENDISPL_OP_DBUF_CREATE.buffer_sz +
> > > > > XEN_PAGE_SIZE - 1) /
> > > > > + * XEN_PAGE_SIZE
> > > > > + */
> > > > > +
> > > > > +struct xendispl_page_directory {
> > > > > + grant_ref_t gref_dir_next_page;
> > > > > + grant_ref_t gref[1]; /* Variable length */
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Request dbuf destruction - destroy a previously allocated display
> > > > > buffer:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id |_OP_DBUF_DESTROY| reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | dbuf_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | dbuf_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * Must be sent over control ring of the connector with index 0.
> > > > > + */
> > > > > +
> > > > > +struct xendispl_dbuf_destroy_req {
> > > > > + uint64_t dbuf_cookie;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Request framebuffer attachment - request attachment of a
> > > > > framebuffer to
> > > > > + * previously created display buffer.
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | _OP_FB_ATTACH | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | dbuf_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | dbuf_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie low 32-bit
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie high 32-bit
> > > > > | 24
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | width
> > > > > | 28
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | height
> > > > > | 32
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | pixel_format
> > > > > | 36
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 40
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * Must be sent over control ring of the connector with index 0.
> > > > > + *
> > > > > + * width - uint32_t, width in pixels
> > > > > + * height - uint32_t, height in pixels
> > > > Why? The _OP_DBUF_CREATE already specified the height/width. Why have
> > > > this?
> > > >
> > > > Or are you thinking to have an smaller (or bigger?) resolution
> > > > framebuffer
> > > > than the display? I suppose you can do that which would imply that the
> > > > display
> > > > would switch modes? Perhaps mention this?
> > > >
> > > > Also what if these values are bigger (or lesser) than 'resolution' ?
> > > >
> > > >
> > > > > + * pixel_format - uint32_t, pixel format of the framebuffer, FOURCC
> > > > > code
> > > > You did not mention the fb_cookie?
> > > I have a common section:
> > > * For all request/response/event packets that use cookies:
> > > * dbuf_cookie - uint64_t, unique to guest domain value used by the
> > > backend
> > > * to map remote display buffer to its local one
> > > * fb_cookie - uint64_t, unique to guest domain value used by the
> > > backend
> > > * to map remote framebuffer to its local one
> > > But it is in the "Requests" section - I'll pull it out to be common for
> > > all
> > Aaaah!
> > > > > + */
> > > > > +
> > > > > +struct xendispl_fb_attach_req {
> > > > > + uint64_t dbuf_cookie;
> > > > > + uint64_t fb_cookie;
> > > > > + uint32_t width;
> > > > > + uint32_t height;
> > > > > + uint32_t pixel_format;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Request framebuffer detach - detach a previously
> > > > > + * attached framebuffer from the display buffer in request:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | _OP_FB_DETACH | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * Must be sent over control ring of the connector with index 0.
> > > > > + */
> > > > > +
> > > > > +struct xendispl_fb_detach_req {
> > > > > + uint64_t fb_cookie;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Request configuration set/reset - request to set or reset
> > > > > + * the configuration/mode of the display:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | _OP_SET_CONFIG | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | x
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | y
> > > > > | 24
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | width
> > > > > | 28
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | height
> > > > > | 32
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | bpp
> > > > > | 40
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 44
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * Pass all zeros to reset, otherwise command is treated as
> > > > > + * configuration set.
> > > > All zeros? Including fb_cookie?
> > > Yes, fb_cookie is 0 when we disable the display, as at this point there
> > > could be no fb at all (all released by that time)
> > > > > + * If this is a set configuration request then framebuffer's cookie
> > > > > tells
> > > > Not sure if the first part of the sentence is needed. This is after all
> > > > the section that talks about setting configurations.
> > > ok, will remove it
> > > > > + * the display which framebuffer/dbuf must be displayed while
> > > > > enabling display
> > > > > + * (applying configuration).
> > > > Can you disable an display without changing this?
> > > You pass ALL zeros, including fb_cookie to disable/reset.
> > > > > + *
> > > > > + * x - uint32_t, starting position in pixels by X axis
> > > > > + * y - uint32_t, starting position in pixels by Y axis
> > > > Which is bound by 'resolution' ? Or by the display command?
> > > these are limited by the "resolution" - will state that:
> > > * x, y, width and height are bound by the connector resolution and must
> > > not
> > > * exceed it.
> > Thanks!
> BTW, this is the only place ATM where we stick to resolution
> from XenStore... And it will clearly be stated
/me nods
> > > > > + * width - uint32_t, width in pixels
> > > > > + * height - uint32_t, height in pixels
> > > > > + * bpp - uint32_t, bits per pixel
> > > > Shouldn't there be an stride as well?
> > > no, stride is kind of implementation detail which shouldn't
> > > be in the protocol
> > > > > + */
> > > > > +
> > > > > +struct xendispl_set_config_req {
> > > > > + uint64_t fb_cookie;
> > > > > + uint32_t x;
> > > > > + uint32_t y;
> > > > > + uint32_t width;
> > > > > + uint32_t height;
> > > > > + uint32_t bpp;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Request page flip - request to flip a page identified by the
> > > > > framebuffer
> > > > > + * cookie:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | _OP_PG_FLIP | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + */
> > > > > +
> > > > > +struct xendispl_page_flip_req {
> > > > > + uint64_t fb_cookie;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + *---------------------------------- Responses
> > > > > --------------------------------
> > > > > + *
> > > > > + * All response packets have the same length (64 octets)
> > > > > + *
> > > > > + * All response packets have common header:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | status
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * id - uint16_t, private guest value, echoed from request
> > > > > + * status - int32_t, response status, zero on success and -XEN_EXX
> > > > > on failure
> > > > > + *
> > > > > + *----------------------------------- Events
> > > > > ----------------------------------
> > > > > + *
> > > > > + * Events are sent via a shared page allocated by the front and
> > > > > propagated by
> > > > > + * event-channel/event-ring-ref XenStore entries
> > > > > + * All event packets have the same length (64 octets)
> > > > > + * All event packets have common header:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | type | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > + * id - uint16_t, event id, may be used by front
> > > > > + * type - uint8_t, type of the event
> > > > > + *
> > > > > + *
> > > > > + * Page flip complete event - event from back to front on page flip
> > > > > completed:
> > > > > + * 0 1 2 3
> > > > > octet
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | id | _EVT_PG_FLIP | reserved
> > > > > | 4
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 8
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie low 32-bit
> > > > > | 12
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | fb_cookie high 32-bit
> > > > > | 16
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 20
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + *
> > > > > |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + * | reserved
> > > > > | 64
> > > > > + *
> > > > > +----------------+----------------+----------------+----------------+
> > > > > + */
> > > > > +
> > > > > +struct xendispl_pg_flip_evt {
> > > > > + uint64_t fb_cookie;
> > > > > +};
> > > > > +
> > > > > +struct xendispl_req {
> > > > > + uint16_t id;
> > > > > + uint8_t operation;
> > > > > + uint8_t reserved[5];
> > > > > + union {
> > > > > + struct xendispl_dbuf_create_req dbuf_create;
> > > > > + struct xendispl_dbuf_destroy_req dbuf_destroy;
> > > > > + struct xendispl_fb_attach_req fb_attach;
> > > > > + struct xendispl_fb_detach_req fb_detach;
> > > > > + struct xendispl_set_config_req set_config;
> > > > > + struct xendispl_page_flip_req pg_flip;
> > > > > + uint8_t reserved[56];
> > > > > + } op;
> > > > > +};
> > > > > +
> > > > > +struct xendispl_resp {
> > > > > + uint16_t id;
> > > > > + uint8_t operation;
> > > > > + uint8_t reserved;
> > > > > + int32_t status;
> > > > > + uint8_t reserved1[56];
> > > > > +};
> > > > > +
> > > > > +struct xendispl_evt {
> > > > > + uint16_t id;
> > > > > + uint8_t type;
> > > > > + uint8_t reserved[5];
> > > > > + union {
> > > > > + struct xendispl_pg_flip_evt pg_flip;
> > > > > + uint8_t reserved[56];
> > > > > + } op;
> > > > > +};
> > > > > +
> > > > > +DEFINE_RING_TYPES(xen_displif, struct xendispl_req, struct
> > > > > xendispl_resp);
> > > > > +
> > > > > +/*
> > > > > +
> > > > > ******************************************************************************
> > > > > + * Back to front events delivery
> > > > > +
> > > > > ******************************************************************************
> > > > > + * In order to deliver asynchronous events from back to front a
> > > > > shared page is
> > > > > + * allocated by front and its grefs propagated to back via XenStore
> > > > > entries
> > > > > + * (event-XXX).
> > > > ?
> > > >
> > > > You mean event-channel ? Or control-event-channel?
> > > event-channel which is used for async events from back to front,
> > > control is always used to pass req/resp.
> > > > > + * This page has a common header used by both front and back to
> > > > > synchronize
> > > > > + * access and control event's ring buffer, while back being a
> > > > > producer of the
> > > > > + * events and front being a consumer. The rest of the page after the
> > > > > header
> > > > > + * is used for event packets.
> > > > > + *
> > > > > + * Upon reception of an event(s) front may confirm its reception
> > > > > + * for either each event, group of events or none.
> > > > > + */
> > > > > +
> > > > > +struct xendispl_event_page {
> > > > > + uint32_t in_cons;
> > > > > + uint32_t in_prod;
> > > > > + uint8_t reserved[56];
> > > > > +};
> > > > > +
> > > > > +#define XENDISPL_EVENT_PAGE_SIZE 4096
> > > > > +#define XENDISPL_IN_RING_OFFS (sizeof(struct xendispl_event_page))
> > > > > +#define XENDISPL_IN_RING_SIZE (XENDISPL_EVENT_PAGE_SIZE -
> > > > > XENDISPL_IN_RING_OFFS)
> > > > > +#define XENDISPL_IN_RING_LEN (XENDISPL_IN_RING_SIZE / sizeof(struct
> > > > > xendispl_evt))
> > > > > +#define XENDISPL_IN_RING(page) \
> > > > > + ((struct xendispl_evt *)((char *)(page) +
> > > > > XENDISPL_IN_RING_OFFS))
> > > > > +#define XENDISPL_IN_RING_REF(page, idx) \
> > > > > + (XENDISPL_IN_RING((page))[(idx) % XENDISPL_IN_RING_LEN])
> > > > Why not use the ring.h macros?
> > > well, this is the way the same structs are defined for fbif/kbif
> > > and I am not sure what can be re-used here from ring.h as it is
> > > designed to work with req/resp, not events.
> > > > Especially as the top of this talks about
> > > > "RING_FINAL_CHECK_FOR_REQUESTS"?
> > > I can remove this
> > > > > +
> > > > > +#endif /* __XEN_PUBLIC_IO_DISPLIF_H__ */
> > > > Could we also have this header in:
> > > sure, I will add the same to sndif as well
> > > > /*
> > > > * Local variables:
> > > > * mode: C
> > > > * c-file-style: "BSD"
> > > > * c-basic-offset: 4
> > > > * tab-width: 4
> > > > * indent-tabs-mode: nil
> > > > * End:
> > > > */
> > > >
> > > > please?
> > > > > --
> > > > > 2.7.4
> > > > >
> > > [1] http://marc.info/?l=xen-devel&m=142167153324052&w=4
> > >
> Thank you,
> Oleksandr
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |