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

[Gerd Hoffmann <kraxel@redhat.com>]

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