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

2014-09-15 Thread 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


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2014-09-15 Thread 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2014-09-14 Thread 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization