Re: [virtio-dev] [PATCH 2/2] virtio-gpu/2d: add docs/specs/virtio-gpu.txt

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Sep 11, 2014 at 05:09:33PM +0200, Gerd Hoffmann wrote:
> diff --git a/docs/specs/virtio-gpu.txt b/docs/specs/virtio-gpu.txt
> new file mode 100644
> index 0000000..9455383
> --- /dev/null
> +++ b/docs/specs/virtio-gpu.txt
> @@ -0,0 +1,165 @@
> +virtio-gpu specification
> +========================

This document refers to the implementation for structs and does not
fully document the semantics of the virtqueue commands.

Mixing the implementation and specification is risky since
implementation changes cannot be checked against the specification.  In
order to make this document self-contained you need to define the struct
layouts.

Error conditions and corner cases are not documented for the virtqueue
commands.  I've asked about a few of them below, but there more are
required to make this specification complete enough so someone else
could write a compatible implementation.

> +drive virtio-gpu in 2D mode
> +---------------------------
> +
> +The virtio-gpu is based around the concept of resources private to the
> +host, the guest must DMA transfer into these resources. This is a
> +design requirement in order to interface with future 3D rendering. In
> +the unaccelerated there is no support for DMA transfers from

"the unaccelerated case"?

> +VIRTGPU_CMD_RESOURCE_CREATE_2D:
> +  Command: struct virtgpu_resource_create_2d
> +
> +  Create a 2D resource on the host.
> +
> +  This creates a 2D resource on the host with the specified width,
> +  height and format. Only a small subset of formats are support. The
> +  resource ids are generated by the guest.

Can the host refuse due to lack of resources?

> +VIRTGPU_CMD_SET_SCANOUT:
> +  Command: struct virtgpu_set_scanout
> +
> +  Set the scanout parameters for a single output.
> +
> +  This sets the scanout parameters for a single scanout. The
> +  resource_id is the resource to be scanned out from, along with a
> +  rectangle specified by x, y, width and height.

What if x, y, width, and height are out-of-range for the given resource?

What if width and height exceed the scanout width and height?

Is it possible to unset the scanout for a resource?  Can a resource be
set on multiple scanouts?

Does VIRTGPU_CMD_SET_SCANOUT need to be called between every
VIRTGPU_CMD_RESOURCE_FLUSH or is does the assignment persist?

> +VIRTGPU_CMD_RESOURCE_ATTACH_BACKING:
> +  Command: struct virtgpu_resource_attach_backing
> +
> +  Assign backing pages to a resource.
> +
> +  This assign an array of guest pages (struct virtgpu_mem_entry) as

"assigns"

> +  the backing store for a resource. These pages are then used for the
> +  transfer operations for that resource from that point on.
> +
> +VIRTGPU_CMD_RESOURCE_INVAL_BACKING:
> +  Command: struct virtgpu_resource_inval_backing

Why is it called INVAL_BACKING instead of DETACH_BACKING?  "Detach" is
logical since there is also an "attach" command.

Attachment: pgpkHBEdgpxCd.pgp
Description: PGP signature