[Qemu-devel] [PATCH 0/2] virtio-gpu: hardware specification

[Qemu-devel] [PATCH 0/2] virtio-gpu: hardware specification

[Gerd Hoffmann]

  Hi folks,

Lets kick off the virtio-gpu review process, starting with the virtio
protocol.

This is a tiny patch series for qemu.  Patch #1 carries the header file
describing the virtual hardware: config space, command structs being
sent over the rings, defines etc.  Patch #2 adds a text file describing
virtio-gpu to docs/specs/.  It covers 2D support only for now.

For anybody who wants to dig a bit deeper:  Here are the branches for
qemu and the linux kernel:

  https://www.kraxel.org/cgit/qemu/log/?h=rebase/vga-wip
  https://www.kraxel.org/cgit/linux/log/?h=virtio-gpu-rebase

The qemu patches are in a reasonable state.  The linux kernel patches
are not cleaned up yet, you probably want to look at the final tree
only.

Work has been done by Dave Airlie <airlied@redhat.com> and me.  The
authorship info in git got lost in the qemu patch cleanup process
though.  Suggestions how to handle that best?  Simply add both mine
and Dave's signed-off-by to all virtio-gpu qemu patches?  Is that fine
with you Dave?  Anyone has better ideas?

please review,
  Gerd Hoffmann

Gerd Hoffmann (2):
  virtio-gpu/2d: add hardware spec include file
  virtio-gpu/2d: add docs/specs/virtio-gpu.txt

 docs/specs/virtio-gpu.txt      | 165 +++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtgpu_hw.h | 158 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 323 insertions(+)
 create mode 100644 docs/specs/virtio-gpu.txt
 create mode 100644 include/hw/virtio/virtgpu_hw.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Gerd Hoffmann]

This patch adds the header file with structs and defines for
the virtio based gpu device.  Covers 2d operations only.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/virtio/virtgpu_hw.h | 158 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 include/hw/virtio/virtgpu_hw.h

diff --git a/include/hw/virtio/virtgpu_hw.h b/include/hw/virtio/virtgpu_hw.h
new file mode 100644
index 0000000..461f452
--- /dev/null
+++ b/include/hw/virtio/virtgpu_hw.h
@@ -0,0 +1,158 @@
+#ifndef VIRTGPU_HW_H
+#define VIRTGPU_HW_H
+
+enum virtgpu_ctrl_type {
+        VIRTGPU_UNDEFINED = 0,
+
+        /* 2d commands */
+        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
+        VIRTGPU_CMD_RESOURCE_CREATE_2D,
+        VIRTGPU_CMD_RESOURCE_UNREF,
+        VIRTGPU_CMD_SET_SCANOUT,
+        VIRTGPU_CMD_RESOURCE_FLUSH,
+        VIRTGPU_CMD_TRANSFER_TO_HOST_2D,
+        VIRTGPU_CMD_RESOURCE_ATTACH_BACKING,
+        VIRTGPU_CMD_RESOURCE_INVAL_BACKING,
+
+        /* cursor commands */
+        VIRTGPU_CMD_UPDATE_CURSOR = 0x0300,
+        VIRTGPU_CMD_MOVE_CURSOR,
+
+        /* success responses */
+        VIRTGPU_RESP_OK_NODATA = 0x1100,
+        VIRTGPU_RESP_OK_DISPLAY_INFO,
+
+        /* error responses */
+        VIRTGPU_RESP_ERR_UNSPEC = 0x1200,
+};
+
+#define VIRTGPU_FLAG_FENCE (1 << 0)
+
+struct virtgpu_ctrl_hdr {
+        uint32_t type;
+        uint32_t flags;
+        uint64_t fence_id;
+        uint32_t ctx_id;
+        uint32_t padding;
+};
+
+/* data passed in the cursor vq */
+
+struct virtgpu_cursor_pos {
+        uint32_t scanout_id;
+        uint32_t x, y;
+};
+
+/* VIRTGPU_CMD_UPDATE_CURSOR, VIRTGPU_CMD_MOVE_CURSOR */
+struct virtgpu_update_cursor {
+        struct virtgpu_ctrl_hdr hdr;
+        struct virtgpu_cursor_pos pos;  /* update & move */
+        uint32_t resource_id;           /* update only */
+        uint32_t hot_x, hot_y;          /* update only */
+};
+
+/* data passed in the control vq, 2d related */
+
+/* VIRTGPU_CMD_RESOURCE_UNREF */
+struct virtgpu_resource_unref {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t resource_id;
+};
+
+/* VIRTGPU_CMD_RESOURCE_CREATE_2D: create a simple 2d resource with a format */
+struct virtgpu_resource_create_2d {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t resource_id;
+        uint32_t format;
+        uint32_t width;
+        uint32_t height;
+};
+
+/* VIRTGPU_CMD_SET_SCANOUT */
+struct virtgpu_set_scanout {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t scanout_id;
+        uint32_t resource_id;
+        uint32_t width;
+        uint32_t height;
+        uint32_t x;
+        uint32_t y;
+};
+
+/* VIRTGPU_CMD_RESOURCE_FLUSH */
+struct virtgpu_resource_flush {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t resource_id;
+        uint32_t width;
+        uint32_t height;
+        uint32_t x;
+        uint32_t y;
+};
+
+/* VIRTGPU_CMD_TRANSFER_TO_HOST_2D: simple transfer to_host */
+struct virtgpu_transfer_to_host_2d {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t resource_id;
+        uint32_t offset;
+        uint32_t width;
+        uint32_t height;
+        uint32_t x;
+        uint32_t y;
+};
+
+struct virtgpu_mem_entry {
+        uint64_t addr;
+        uint32_t length;
+        uint32_t pad;
+};
+
+/* VIRTGPU_CMD_RESOURCE_ATTACH_BACKING */
+struct virtgpu_resource_attach_backing {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t resource_id;
+        uint32_t nr_entries;
+};
+
+/* VIRTGPU_CMD_RESOURCE_INVAL_BACKING */
+struct virtgpu_resource_inval_backing {
+        struct virtgpu_ctrl_hdr hdr;
+        uint32_t resource_id;
+};
+
+/* VIRTGPU_RESP_OK_DISPLAY_INFO */
+#define VIRTGPU_MAX_SCANOUTS 16
+struct virtgpu_resp_display_info {
+        struct virtgpu_ctrl_hdr hdr;
+        struct virtgpu_display_one {
+                uint32_t enabled;
+                uint32_t width;
+                uint32_t height;
+                uint32_t x;
+                uint32_t y;
+                uint32_t flags;
+        } pmodes[VIRTGPU_MAX_SCANOUTS];
+};
+
+#define VIRTGPU_EVENT_DISPLAY (1 << 0)
+
+struct virtgpu_config {
+        uint32_t events_read;
+        uint32_t events_clear;
+        uint32_t num_scanouts;
+        uint32_t reserved;
+};
+
+/* simple formats for fbcon/X use */
+enum virtgpu_formats {
+        VIRGL_FORMAT_B8G8R8A8_UNORM          = 1,
+        VIRGL_FORMAT_B8G8R8X8_UNORM          = 2,
+        VIRGL_FORMAT_A8R8G8B8_UNORM          = 3,
+        VIRGL_FORMAT_X8R8G8B8_UNORM          = 4,
+
+        VIRGL_FORMAT_B5G5R5A1_UNORM          = 5,
+        VIRGL_FORMAT_B5G6R5_UNORM            = 7,
+
+        VIRGL_FORMAT_R8_UNORM                = 64,
+};
+
+#endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Peter Maydell]

On 11 September 2014 16:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
> This patch adds the header file with structs and defines for
> the virtio based gpu device.  Covers 2d operations only.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/virtio/virtgpu_hw.h | 158 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 include/hw/virtio/virtgpu_hw.h
>
> diff --git a/include/hw/virtio/virtgpu_hw.h b/include/hw/virtio/virtgpu_hw.h
> new file mode 100644
> index 0000000..461f452
> --- /dev/null
> +++ b/include/hw/virtio/virtgpu_hw.h
> @@ -0,0 +1,158 @@
> +#ifndef VIRTGPU_HW_H
> +#define VIRTGPU_HW_H
> +
> +enum virtgpu_ctrl_type {
> +        VIRTGPU_UNDEFINED = 0,
>

This is clearly all well out of line with our
coding style guide, which isn't a terribly good start...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Gerd Hoffmann]

  Hi,

> > +enum virtgpu_ctrl_type {
> > +        VIRTGPU_UNDEFINED = 0,
> >
> 
> This is clearly all well out of line with our
> coding style guide, which isn't a terribly good start...

Oh yea, code style is fun.  This file is needed in both qemu & linux
kernel, which have different code styles.  I pipe it though a sed script
already when syncing them up, weeding out the tabs.  I'll go tweak my
script to fix the indent too.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On 09/11/2014 09:09 AM, Gerd Hoffmann wrote:
> This patch adds the header file with structs and defines for
> the virtio based gpu device.  Covers 2d operations only.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/virtio/virtgpu_hw.h | 158 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 include/hw/virtio/virtgpu_hw.h
> 
> diff --git a/include/hw/virtio/virtgpu_hw.h b/include/hw/virtio/virtgpu_hw.h
> new file mode 100644
> index 0000000..461f452
> --- /dev/null
> +++ b/include/hw/virtio/virtgpu_hw.h
> @@ -0,0 +1,158 @@
> +#ifndef VIRTGPU_HW_H
> +#define VIRTGPU_HW_H

Non-trivial file, deserves a copyright and license notice.

> +
> +enum virtgpu_ctrl_type {
> +        VIRTGPU_UNDEFINED = 0,
> +
> +        /* 2d commands */
> +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,

Please consider also adding:

#define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO

and friends.  It makes it MUCH nicer for application software to probe
for later extensions if every member of the enum is also associated with
a preprocessor macro.


> +
> +struct virtgpu_ctrl_hdr {
> +        uint32_t type;
> +        uint32_t flags;
> +        uint64_t fence_id;
> +        uint32_t ctx_id;
> +        uint32_t padding;
> +};
> +

Is the padding to ensure that this is aligned regardless of 32-bit or
64-bit hosts?  Is it worth adding a compile-time assertion about the
size of the struct to ensure the compiler doesn't add any additional
padding?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Gerd Hoffmann]

  Hi,

> > @@ -0,0 +1,158 @@
> > +#ifndef VIRTGPU_HW_H
> > +#define VIRTGPU_HW_H
> 
> Non-trivial file, deserves a copyright and license notice.

Added.

> > +
> > +enum virtgpu_ctrl_type {
> > +        VIRTGPU_UNDEFINED = 0,
> > +
> > +        /* 2d commands */
> > +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> 
> Please consider also adding:
> 
> #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
> 
> and friends.  It makes it MUCH nicer for application software to probe
> for later extensions if every member of the enum is also associated with
> a preprocessor macro.

I don't think this will ever be shipped as library header for external
users ...

> > +struct virtgpu_ctrl_hdr {
> > +        uint32_t type;
> > +        uint32_t flags;
> > +        uint64_t fence_id;
> > +        uint32_t ctx_id;
> > +        uint32_t padding;
> > +};
> > +
> 
> Is the padding to ensure that this is aligned regardless of 32-bit or
> 64-bit hosts?

Yes.

> Is it worth adding a compile-time assertion about the
> size of the struct to ensure the compiler doesn't add any additional
> padding?

Makes sense.  What is the usual trick to do that?

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]

On 09/12/2014 04:44 AM, Gerd Hoffmann wrote:

>>> +enum virtgpu_ctrl_type {
>>> +        VIRTGPU_UNDEFINED = 0,
>>> +
>>> +        /* 2d commands */
>>> +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
>>
>> Please consider also adding:
>>
>> #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
>>
>> and friends.  It makes it MUCH nicer for application software to probe
>> for later extensions if every member of the enum is also associated with
>> a preprocessor macro.
> 
> I don't think this will ever be shipped as library header for external
> users ...

Fair enough. I wasn't sure, so it didn't hurt to ask.

> 
>>> +struct virtgpu_ctrl_hdr {
>>> +        uint32_t type;
>>> +        uint32_t flags;
>>> +        uint64_t fence_id;
>>> +        uint32_t ctx_id;
>>> +        uint32_t padding;
>>> +};
>>> +
>>
>> Is the padding to ensure that this is aligned regardless of 32-bit or
>> 64-bit hosts?
> 
> Yes.
> 
>> Is it worth adding a compile-time assertion about the
>> size of the struct to ensure the compiler doesn't add any additional
>> padding?
> 
> Makes sense.  What is the usual trick to do that?

Among others,

struct dummy {
  int size_check : (sizeof(virtgpu_ctrl_hdr) == 24 ? 1 : -1);
};

Since bitfields cannot have a negative size, the compiler will
forcefully fail compilation if the struct size ever changes.  Similar
tricks include setting up array bounds that would be negative on
failure, or (inside a function body) declaring a switch that has
colliding case statements on failure.  But depending on the set of
compiler warning options in effect, declaring an otherwise unused struct
may be too noisy.  So if you need more ideas and a good read, check out
the comments in how gnulib does it (the link mentions GPLv3+, but that
file is also shipped as LGPLv2+ so it is compatible with qemu):

git.savannah.gnu.org/cgit/gnulib.git/tree/lib/verify.h

with an end result that a user just writes:

verify(sizeof(virtgpu_ctrl_hdr) == 24);

and calls it a day.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

On 09/12/2014 06:48 AM, Eric Blake wrote:

> So if you need more ideas and a good read, check out
> the comments in how gnulib does it (the link mentions GPLv3+, but that
> file is also shipped as LGPLv2+ so it is compatible with qemu):
> 
> git.savannah.gnu.org/cgit/gnulib.git/tree/lib/verify.h

Just to be clear,

http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/verify.h

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Peter Maydell]

On 12 September 2014 13:48, Eric Blake <eblake@redhat.com> wrote:
> On 09/12/2014 04:44 AM, Gerd Hoffmann wrote:
>
>>>> +enum virtgpu_ctrl_type {
>>>> +        VIRTGPU_UNDEFINED = 0,
>>>> +
>>>> +        /* 2d commands */
>>>> +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
>>>
>>> Please consider also adding:
>>>
>>> #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
>>>
>>> and friends.  It makes it MUCH nicer for application software to probe
>>> for later extensions if every member of the enum is also associated with
>>> a preprocessor macro.
>>
>> I don't think this will ever be shipped as library header for external
>> users ...
>
> Fair enough. I wasn't sure, so it didn't hurt to ask.
>
>>
>>>> +struct virtgpu_ctrl_hdr {
>>>> +        uint32_t type;
>>>> +        uint32_t flags;
>>>> +        uint64_t fence_id;
>>>> +        uint32_t ctx_id;
>>>> +        uint32_t padding;
>>>> +};
>>>> +
>>>
>>> Is the padding to ensure that this is aligned regardless of 32-bit or
>>> 64-bit hosts?
>>
>> Yes.
>>
>>> Is it worth adding a compile-time assertion about the
>>> size of the struct to ensure the compiler doesn't add any additional
>>> padding?
>>
>> Makes sense.  What is the usual trick to do that?
>
> Among others,
>
> struct dummy {
>   int size_check : (sizeof(virtgpu_ctrl_hdr) == 24 ? 1 : -1);
> };

QEMU already has a BUILD_BUG_ON() macro, so you can just say

QEMU_BUILD_BUG_ON(sizeof(virtgpu_ctrl_hdr) != 24);

-- PMM

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Michael S. Tsirkin]

On Fri, Sep 12, 2014 at 12:44:56PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > @@ -0,0 +1,158 @@
> > > +#ifndef VIRTGPU_HW_H
> > > +#define VIRTGPU_HW_H
> > 
> > Non-trivial file, deserves a copyright and license notice.
> 
> Added.

Pls remember to make it consistent with other virtio headers,
which are 3-clause BSD, prefixed with this reminder:

 This header is BSD licensed so anyone can use the definitions to
 implement compatible drivers/servers.



> > > +
> > > +enum virtgpu_ctrl_type {
> > > +        VIRTGPU_UNDEFINED = 0,
> > > +
> > > +        /* 2d commands */
> > > +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> > 
> > Please consider also adding:

VIRTIO_GPU_ everywhere to make it consistent with other
virtio headers?


> > 
> > #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
> > 
> > and friends.  It makes it MUCH nicer for application software to probe
> > for later extensions if every member of the enum is also associated with
> > a preprocessor macro.
> 
> I don't think this will ever be shipped as library header for external
> users ...
> 
> > > +struct virtgpu_ctrl_hdr {
> > > +        uint32_t type;
> > > +        uint32_t flags;
> > > +        uint64_t fence_id;
> > > +        uint32_t ctx_id;
> > > +        uint32_t padding;
> > > +};
> > > +
> > 
> > Is the padding to ensure that this is aligned regardless of 32-bit or
> > 64-bit hosts?
> 
> Yes.
> 
> > Is it worth adding a compile-time assertion about the
> > size of the struct to ensure the compiler doesn't add any additional
> > padding?
> 
> Makes sense.  What is the usual trick to do that?
> 
> thanks,
>   Gerd

BUILD_BUG_ON in linux, QEMU_BUILD_BUG_ON in QEMU.
You have to stick it in a C file though, so it
won't be visible in this patch.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Peter Maydell]

On 14 September 2014 06:46, Michael S. Tsirkin <mst@redhat.com> wrote:
> BUILD_BUG_ON in linux, QEMU_BUILD_BUG_ON in QEMU.
> You have to stick it in a C file though, so it
> won't be visible in this patch.

Why do you think that? We have several header files which
use QEMU_BUILD_BUG_ON and I don't see any reason why
it would have to be invoked from a .c file.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Michael S. Tsirkin]

On Sun, Sep 14, 2014 at 07:04:11AM -0700, Peter Maydell wrote:
> On 14 September 2014 06:46, Michael S. Tsirkin <mst@redhat.com> wrote:
> > BUILD_BUG_ON in linux, QEMU_BUILD_BUG_ON in QEMU.
> > You have to stick it in a C file though, so it
> > won't be visible in this patch.
> 
> Why do you think that? We have several header files which
> use QEMU_BUILD_BUG_ON and I don't see any reason why
> it would have to be invoked from a .c file.
> 
> -- PMM

Because Gerd wants to share this with linux uapi, and
you can't use BUILD_BUG_ON in uapi headers on linux.

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Peter Maydell]

On 14 September 2014 07:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 14, 2014 at 07:04:11AM -0700, Peter Maydell wrote:
>> On 14 September 2014 06:46, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > BUILD_BUG_ON in linux, QEMU_BUILD_BUG_ON in QEMU.
>> > You have to stick it in a C file though, so it
>> > won't be visible in this patch.
>>
>> Why do you think that? We have several header files which
>> use QEMU_BUILD_BUG_ON and I don't see any reason why
>> it would have to be invoked from a .c file.

> Because Gerd wants to share this with linux uapi, and
> you can't use BUILD_BUG_ON in uapi headers on linux.

Who owns the "master" copy of the header and commits
to making sure it builds on other things than Linux+gcc
in that case?

-- PMM

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Michael S. Tsirkin]

On Sun, Sep 14, 2014 at 07:32:21AM -0700, Peter Maydell wrote:
> On 14 September 2014 07:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 14, 2014 at 07:04:11AM -0700, Peter Maydell wrote:
> >> On 14 September 2014 06:46, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > BUILD_BUG_ON in linux, QEMU_BUILD_BUG_ON in QEMU.
> >> > You have to stick it in a C file though, so it
> >> > won't be visible in this patch.
> >>
> >> Why do you think that? We have several header files which
> >> use QEMU_BUILD_BUG_ON and I don't see any reason why
> >> it would have to be invoked from a .c file.
> 
> > Because Gerd wants to share this with linux uapi, and
> > you can't use BUILD_BUG_ON in uapi headers on linux.
> 
> Who owns the "master" copy of the header and commits
> to making sure it builds on other things than Linux+gcc
> in that case?
> 
> -- PMM

For most of virtio neither linux nor QEMU are the master.
syncing them has been done manually in the past.
As we are copying other headers from Linux anyway,
I think it would be better for everyone if we make
the Linux headers the master for QEMU going forward.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Peter Maydell]

On 14 September 2014 08:09, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 14, 2014 at 07:32:21AM -0700, Peter Maydell wrote:
>> Who owns the "master" copy of the header and commits
>> to making sure it builds on other things than Linux+gcc
>> in that case?

> For most of virtio neither linux nor QEMU are the master.
> syncing them has been done manually in the past.
> As we are copying other headers from Linux anyway,
> I think it would be better for everyone if we make
> the Linux headers the master for QEMU going forward.

That's problematic because our copies of the linux headers
are only (and can only) be included in our include path
on Linux hosts. I had this problem with the PSCI headers
for ARM (I can't say I really like the solution I came up
with there, so if we can do better that would be cool).

-- PMM

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Michael S. Tsirkin]

On Sun, Sep 14, 2014 at 09:11:45AM -0700, Peter Maydell wrote:
> On 14 September 2014 08:09, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 14, 2014 at 07:32:21AM -0700, Peter Maydell wrote:
> >> Who owns the "master" copy of the header and commits
> >> to making sure it builds on other things than Linux+gcc
> >> in that case?
> 
> > For most of virtio neither linux nor QEMU are the master.
> > syncing them has been done manually in the past.
> > As we are copying other headers from Linux anyway,
> > I think it would be better for everyone if we make
> > the Linux headers the master for QEMU going forward.
> 
> That's problematic because our copies of the linux headers
> are only (and can only) be included in our include path
> on Linux hosts. I had this problem with the PSCI headers
> for ARM (I can't say I really like the solution I came up
> with there, so if we can do better that would be cool).
> 
> -- PMM

Right. My idea was to add stuff that's used on all platforms
to some other directory (besides linux-headers).
I didn't try to implement this yet so I don't know whether
there are any issues with this.
Should be better than duplicating it manually.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Gerd Hoffmann]

  Hi,

> >>
> >> Why do you think that? We have several header files which
> >> use QEMU_BUILD_BUG_ON and I don't see any reason why
> >> it would have to be invoked from a .c file.
> 
> > Because Gerd wants to share this with linux uapi, and
> > you can't use BUILD_BUG_ON in uapi headers on linux.
> 
> Who owns the "master" copy of the header and commits
> to making sure it builds on other things than Linux+gcc
> in that case?

In my personal repo the master copy is in the linux tree and qemu gets
synced.  I've placed the QEMU_BUILD_BUG_ON calls in the (qemu-only)
virtio-gpu.c now.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Gerd Hoffmann]

> > > > +
> > > > +enum virtgpu_ctrl_type {
> > > > +        VIRTGPU_UNDEFINED = 0,
> > > > +
> > > > +        /* 2d commands */
> > > > +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> > > 
> > > Please consider also adding:
> 
> VIRTIO_GPU_ everywhere to make it consistent with other
> virtio headers?

The names are already pretty long as-is, especially considering the
80cols rule in our codestyle.  I'd prefer to keep them the way they are
now.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Michael S. Tsirkin]

On Mon, Sep 15, 2014 at 12:40:07PM +0200, Gerd Hoffmann wrote:
> > > > > +
> > > > > +enum virtgpu_ctrl_type {
> > > > > +        VIRTGPU_UNDEFINED = 0,
> > > > > +
> > > > > +        /* 2d commands */
> > > > > +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> > > > 
> > > > Please consider also adding:
> > 
> > VIRTIO_GPU_ everywhere to make it consistent with other
> > virtio headers?
> 
> The names are already pretty long as-is, especially considering the
> 80cols rule in our codestyle.  I'd prefer to keep them the way they are
> now.
> 
> cheers,
>   Gerd
> 

VIRTIO_GPU_ is not longer than VIRTIO_NET_ is it?

VIRTGRU seems too generic.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Peter Maydell]

On 11 September 2014 16:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
> This patch adds the header file with structs and defines for
> the virtio based gpu device.  Covers 2d operations only.

Please don't cc subscriber only mailing lists
(virtio-dev@lists.oasis-open.org) on posts to qemu-devel;
it just means everybody responding gets mailer bounce
junkmail.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Gerd Hoffmann]

On Do, 2014-09-11 at 16:20 +0100, Peter Maydell wrote:
> On 11 September 2014 16:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > This patch adds the header file with structs and defines for
> > the virtio based gpu device.  Covers 2d operations only.
> 
> Please don't cc subscriber only mailing lists
> (virtio-dev@lists.oasis-open.org) on posts to qemu-devel;
> it just means everybody responding gets mailer bounce
> junkmail.

I don't feel like splitting the discussion though.  Suggestions how to
deal with that?  I think I've seen most active virtio-dev folks on the
virtualization list too, is it ok to just drop virtio-dev for the next
round and hope that folks see it via virtualization list?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Christopher Covington]

On 09/11/2014 11:43 AM, Gerd Hoffmann wrote:
> On Do, 2014-09-11 at 16:20 +0100, Peter Maydell wrote:
>> On 11 September 2014 16:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>> This patch adds the header file with structs and defines for
>>> the virtio based gpu device.  Covers 2d operations only.
>>
>> Please don't cc subscriber only mailing lists
>> (virtio-dev@lists.oasis-open.org) on posts to qemu-devel;
>> it just means everybody responding gets mailer bounce
>> junkmail.
> 
> I don't feel like splitting the discussion though.  Suggestions how to
> deal with that?  I think I've seen most active virtio-dev folks on the
> virtualization list too, is it ok to just drop virtio-dev for the next
> round and hope that folks see it via virtualization list?

Perhaps you and other subscribers to the offending list can BCC it and
announce that it's BCC'd body.

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

Re: [Qemu-devel] [virtio-dev] Re: [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Paolo Bonzini]

> On Do, 2014-09-11 at 16:20 +0100, Peter Maydell wrote:
> > On 11 September 2014 16:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > This patch adds the header file with structs and defines for
> > > the virtio based gpu device.  Covers 2d operations only.
> > 
> > Please don't cc subscriber only mailing lists
> > (virtio-dev@lists.oasis-open.org) on posts to qemu-devel;
> > it just means everybody responding gets mailer bounce
> > junkmail.
> 
> I don't feel like splitting the discussion though.  Suggestions how to
> deal with that?  I think I've seen most active virtio-dev folks on the
> virtualization list too, is it ok to just drop virtio-dev for the next
> round and hope that folks see it via virtualization list?

There's also virtio-comment.

Paolo

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

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/virtio-gpu.txt | 165 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 165 insertions(+)
 create mode 100644 docs/specs/virtio-gpu.txt

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
+========================
+
+virtio-gpu is a virtio based graphics adapter.  It can operate in 2D
+mode and in 3D (virgl) mode.  3D mode will offload rendering ops to
+the host gpu and therefore requires a gpu with 3D support on the host
+machine.
+
+The initial version will have 2D mode only.  It provides support for
+ARGB Hardware cursors and multiple scanouts (aka heads).
+
+
+features
+--------
+
+There are no feature bits (yet).
+There will be one in the future for 3D mode support.
+
+
+config space
+------------
+
+struct virtgpu_config {
+        uint32_t events_read;
+        uint32_t events_clear;
+        uint32_t num_scanouts;
+        uint32_t reserved;
+};
+
+The two members events_read and events_clear are used to signal events
+to the driver.  Currently one event is defined for a display
+change. When a config space interrupt is received the driver should
+read the events_read field.  The events processed should be written to
+the events_clear field.  The device will clear the bits in events_read
+then, mimicing write-to-clear behavior.
+
+num_scanouts specifies the number of scanouts supported by the device.
+
+The fourth field is reserved (3D mode will need this in the future).
+
+
+virt queues
+-----------
+
+The virtio-gpu exposes 2 virtio queues to the guest:
+
+ (1) control vq - guest->host queue for sending commands and getting
+     responses when required.
+ (2) cursor vq - guest->host queue for sending cursor position and
+     resource updates
+
+Both queues have the same format.  Each request and each response have
+a fixed header (struct virtgpu_ctrl_hdr), followed by command specific
+data fieds.  The separate cursor queue is the "fast track" for
+cursor-related commands, so they go though without being possibly
+delayed by other commands in the control queue.
+
+
+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
+resources, just to them.
+
+Resources are initially simple 2D resources, consisting of a width,
+height and format along with an identifier. The guest must then attach
+backing store to the resources in order for DMA transfers to
+work. This is like a GART in a real GPU.
+
+A typical guest user would create a 2D resource using
+VIRTGPU_CMD_RESOURCE_CREATE_2D, attach backing store using
+VIRTGPU_CMD_RESOURCE_ATTACH_BACKING, then attach the resource to a
+scanout using VIRTGPU_CMD_SET_SCANOUT, then use
+VIRTGPU_CMD_TRANSFER_SEND_2D to send updates to the resource, and
+finally VIRTGPU_CMD_RESOURCE_FLUSH to flush the scanout buffers to
+screen.
+
+
+control queue commands (2D)
+---------------------------
+
+VIRTGPU_CMD_GET_DISPLAY_INFO:
+  Command: none (just struct virtgpu_ctrl_hdr).
+  Returns: struct virtgpu_resp_display_info.
+
+  Retrieve the current output configuration.
+
+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.
+
+VIRTGPU_CMD_RESOURCE_UNREF:
+  Command: struct virtgpu_resource_unref
+
+  Destroy a resource.
+
+  This informs the host that a resource is no longer required by the
+  guest.
+
+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.
+
+VIRTGPU_CMD_RESOURCE_FLUSH:
+  Command: struct virtgpu_resource_flush
+
+  Flush a scanout resource.
+
+  This flushes a resource to screen, it takes a rectangle and a
+  resource id, and flushes any scanouts the resource is being used on.
+
+VIRTGPU_CMD_TRANSFER_TO_HOST_2D:
+  Command: struct virtgpu_transfer_to_host_2d
+
+  Transfer from guest memory to host resource.
+
+  This takes a resource id along with an destination offset into the
+  resource, and a box to transfer from the host backing for the
+  resource.
+
+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
+  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
+
+  Detach backing pages from a resource.
+
+  This detaches any backing pages from a resource, to be used in case of
+  guest swapping or object destruction.
+
+
+cursor queue commands
+---------------------
+
+VIRTGPU_CMD_UPDATE_CURSOR
+  Command: struct virtgpu_update_cursor
+
+  Update cursor from the specified resource id.  The driver must
+  transfer the cursor into the resource beforehand (using control
+  queue commands)
+
+VIRTGPU_CMD_MOVE_CURSOR
+  Command: struct virtgpu_update_cursor
+
+  Move cursor.  Only virtgpu_update_cursor.pos field is used.
-- 
1.8.3.1

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On 09/11/2014 09:09 AM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/specs/virtio-gpu.txt | 165 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 docs/specs/virtio-gpu.txt
> 
> 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

I know you are just following existing bad practice in this directory,
but it would be nice to declare copyright and license on this file.


> +
> +The two members events_read and events_clear are used to signal events
> +to the driver.  Currently one event is defined for a display
> +change. When a config space interrupt is received the driver should
> +read the events_read field.  The events processed should be written to
> +the events_clear field.  The device will clear the bits in events_read
> +then, mimicing write-to-clear behavior.

s/mimicing/mimicking/ (stupid English rule that any verb ending in 'ic'
becomes 'ick' when adding 'ed' or 'ing')

> +Both queues have the same format.  Each request and each response have
> +a fixed header (struct virtgpu_ctrl_hdr), followed by command specific
> +data fieds.  The separate cursor queue is the "fast track" for

s/fieds/fields/

> +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

s/unaccelerated/unaccelerated mode/

> +  This creates a 2D resource on the host with the specified width,
> +  height and format. Only a small subset of formats are support. The

s/support/supported/
and can you delineate that subset?


> +  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.

Is it worth a mention here or generically up front where 0,0 is in
relation to the screen, and in which direction positive numbers move?
I'm assuming 0,0 is top left, and larger x moves right, larger y moves down.

Are there restrictions against rectangles that overlap beyond screen
boundaries?


> +  This assign an array of guest pages (struct virtgpu_mem_entry) as

s/assign/assigns/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Gerd Hoffmann]

  Hi,

> > +++ b/docs/specs/virtio-gpu.txt
> > @@ -0,0 +1,165 @@
> > +virtio-gpu specification
> 
> I know you are just following existing bad practice in this directory,
> but it would be nice to declare copyright and license on this file.

Added copyright for now.
Dunno about license, IIRC GPLv2 isn't good for text, suggestions?

> > +  This creates a 2D resource on the host with the specified width,
> > +  height and format. Only a small subset of formats are support. The
> 
> s/support/supported/
> and can you delineate that subset?

enum virtgpu_formats (text updated).

> > +  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.
> 
> Is it worth a mention here or generically up front where 0,0 is in
> relation to the screen, and in which direction positive numbers move?
> I'm assuming 0,0 is top left, and larger x moves right, larger y moves down.

Correct.  Text updated.

> Are there restrictions against rectangles that overlap beyond screen
> boundaries?

Backing resource must cover the scanout completely.
With multiple heads (and therefore multiple scanouts) it is legal for
the scanouts to overlap or even be identical (=> screen mirroring).

Text updated.

thanks,
  Gerd

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]

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 #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Gerd Hoffmann]

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

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

[Dave Airlie]

>> 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.

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

[Gerd Hoffmann]

On Sa, 2014-09-13 at 07:14 +1000, Dave Airlie wrote:
> >> 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.

Best would be if virgilrenderer.so just fails
virgl_renderer_resource_create() calls.

> 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.

Done.

cheers,
  Gerd

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

[Michael S. Tsirkin]

On Thu, Sep 11, 2014 at 05:09:33PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/specs/virtio-gpu.txt | 165 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 docs/specs/virtio-gpu.txt

Please don't put this hardware spec in QEMU.
Contribute it to OASIS Virtio TC instead.
If you need help with that, please let me know.

-- 
MST

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

[Dave Airlie]

On 14 September 2014 19:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Sep 11, 2014 at 05:09:33PM +0200, Gerd Hoffmann wrote:
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  docs/specs/virtio-gpu.txt | 165 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 165 insertions(+)
>>  create mode 100644 docs/specs/virtio-gpu.txt
>
> Please don't put this hardware spec in QEMU.
> Contribute it to OASIS Virtio TC instead.
> If you need help with that, please let me know.

I think we'd be premature in putting this anywhere that isn't
alongside the code until we've addressed at least any resource
allocation or possible security concerns.

Dave.

Re: [Qemu-devel] [virtio-dev] [PATCH 0/2] virtio-gpu: hardware specification

[Dave Airlie]

On 12 September 2014 01:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi folks,
>
> Lets kick off the virtio-gpu review process, starting with the virtio
> protocol.
>
> This is a tiny patch series for qemu.  Patch #1 carries the header file
> describing the virtual hardware: config space, command structs being
> sent over the rings, defines etc.  Patch #2 adds a text file describing
> virtio-gpu to docs/specs/.  It covers 2D support only for now.
>
> For anybody who wants to dig a bit deeper:  Here are the branches for
> qemu and the linux kernel:
>
>   https://www.kraxel.org/cgit/qemu/log/?h=rebase/vga-wip
>   https://www.kraxel.org/cgit/linux/log/?h=virtio-gpu-rebase
>
> The qemu patches are in a reasonable state.  The linux kernel patches
> are not cleaned up yet, you probably want to look at the final tree
> only.
>
> Work has been done by Dave Airlie <airlied@redhat.com> and me.  The
> authorship info in git got lost in the qemu patch cleanup process
> though.  Suggestions how to handle that best?  Simply add both mine
> and Dave's signed-off-by to all virtio-gpu qemu patches?  Is that fine
> with you Dave?  Anyone has better ideas?

I don't mind just adding a line in the commit msgs with Authors in it,
along with both signoffs.

Dave.