Linux Virtualization - Sep 2014 - [virtio-dev] [PATCH 2/2] virtio-gpu/2d: add docs/specs/virtio-gpu.txt

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20140912/fdeb9b85/attachment.sig>

On Fr, 2014-09-12 at 10:10 +0100, Stefan Hajnoczi wrote:
> 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.
Ok.  The short-term goal for this text is to help reviewing the code by
documenting how the device is supposed to work.  Being good enough for
an independent implementation is the next level.  I'll keep it on the
radar though.
> > +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?
Yes.  virtgpu_ctrl_hdr.type in the response will be set to
VIRTGPU_RESP_ERR_* then.  Current implementation does that only on
malloc() failure, there is no accounting (yet) to limit the amout of
memory the guest is allowed to allocate.

/me notes to write a section on error handling.
> > +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?
You'll get an error.
> What if width and height exceed the scanout width and height?
Using only a subrectangle of the resource for the scanout is legal.
> Is it possible to unset the scanout for a resource?
Yes, use resource_id 0.
> Can a resource be
> set on multiple scanouts?
Yes.
> Does VIRTGPU_CMD_SET_SCANOUT need to be called between every
> VIRTGPU_CMD_RESOURCE_FLUSH or is does the assignment persist?
Assignment is persistent.
> > +  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.
No particular reason I think.  Dave?

cheers,
  Gerd

>> Can the host refuse due to lack of resources?
>
> Yes.  virtgpu_ctrl_hdr.type in the response will be set to
> VIRTGPU_RESP_ERR_* then.  Current implementation does that only on
> malloc() failure, there is no accounting (yet) to limit the amout of
> memory the guest is allowed to allocate.
We do probably need to work out some sort of accounting system, it can
probably reliably only be a total value of resources, since we've no
idea if the host driver will store them in VRAM or main memory. Quite
how to fail gracefully is a question, probably need to report to the
guest what context did the allocation and see if we can destroy it.
>> > +
>> > +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.
>
> No particular reason I think.  Dave?
>
Not reason I can remember, I think I was thinking of having separate
inval and detach at one point, but it didn't really make any sense, so
renaming to detach is fine with me.

Dave.