diff mbox series

[v7,5/9] gfxstream + rutabaga prep: added need defintions, fields, and options

Message ID 20230817022322.466-6-gurchetansingh@google.com (mailing list archive)
State New, archived
Headers show
Series gfxstream + rutabaga_gfx | expand

Commit Message

Gurchetan Singh Aug. 17, 2023, 2:23 a.m. UTC 
From: Gurchetan Singh <gurchetansingh@chromium.org>

This modifies the common virtio-gpu.h file have the fields and
defintions needed by gfxstream/rutabaga, by VirtioGpuRutabaga.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Tested-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
---
v1: void *rutabaga --> struct rutabaga *rutabaga (Akihiko)
    have a separate rutabaga device instead of using GL device (Bernard)

v2: VirtioGpuRutabaga --> VirtIOGPURutabaga (Akihiko)
    move MemoryRegionInfo into VirtIOGPURutabaga (Akihiko)
    remove 'ctx' field (Akihiko)
    remove 'rutabaga_active'

v6: remove command from commit message, refer to docs instead (Manos)

 include/hw/virtio/virtio-gpu.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Mark Cave-Ayland Aug. 23, 2023, 2:32 p.m. UTC | #1 
On 17/08/2023 03:23, Gurchetan Singh wrote:

> From: Gurchetan Singh <gurchetansingh@chromium.org>
> 
> This modifies the common virtio-gpu.h file have the fields and
> defintions needed by gfxstream/rutabaga, by VirtioGpuRutabaga.
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Tested-by: Alyssa Ross <hi@alyssa.is>
> Tested-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> Reviewed-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
> v1: void *rutabaga --> struct rutabaga *rutabaga (Akihiko)
>      have a separate rutabaga device instead of using GL device (Bernard)
> 
> v2: VirtioGpuRutabaga --> VirtIOGPURutabaga (Akihiko)
>      move MemoryRegionInfo into VirtIOGPURutabaga (Akihiko)
>      remove 'ctx' field (Akihiko)
>      remove 'rutabaga_active'
> 
> v6: remove command from commit message, refer to docs instead (Manos)
> 
>   include/hw/virtio/virtio-gpu.h | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 55973e112f..e2a07e68d9 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -38,6 +38,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPUGL, VIRTIO_GPU_GL)
>   #define TYPE_VHOST_USER_GPU "vhost-user-gpu"
>   OBJECT_DECLARE_SIMPLE_TYPE(VhostUserGPU, VHOST_USER_GPU)
>   
> +#define TYPE_VIRTIO_GPU_RUTABAGA "virtio-gpu-rutabaga-device"
> +OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPURutabaga, VIRTIO_GPU_RUTABAGA)
> +
>   struct virtio_gpu_simple_resource {
>       uint32_t resource_id;
>       uint32_t width;
> @@ -94,6 +97,7 @@ enum virtio_gpu_base_conf_flags {
>       VIRTIO_GPU_FLAG_DMABUF_ENABLED,
>       VIRTIO_GPU_FLAG_BLOB_ENABLED,
>       VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
> +    VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
>   };
>   
>   #define virtio_gpu_virgl_enabled(_cfg) \
> @@ -108,6 +112,8 @@ enum virtio_gpu_base_conf_flags {
>       (_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
>   #define virtio_gpu_context_init_enabled(_cfg) \
>       (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
> +#define virtio_gpu_rutabaga_enabled(_cfg) \
> +    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
>   #define virtio_gpu_hostmem_enabled(_cfg) \
>       (_cfg.hostmem > 0)
>   
> @@ -232,6 +238,28 @@ struct VhostUserGPU {
>       bool backend_blocked;
>   };
>   
> +#define MAX_SLOTS 4096
> +
> +struct MemoryRegionInfo {
> +    int used;
> +    MemoryRegion mr;
> +    uint32_t resource_id;
> +};
> +
> +struct rutabaga;
> +
> +struct VirtIOGPURutabaga {
> +    struct VirtIOGPU parent_obj;

The QOM macro should define a typedef for you, so you can drop the "struct" here.

> +
> +    struct MemoryRegionInfo memory_regions[MAX_SLOTS];
> +    char *capset_names;
> +    char *wayland_socket_path;
> +    char *wsi;
> +    bool headless;
> +    uint32_t num_capsets;
> +    struct rutabaga *rutabaga;
> +};
> +

Shouldn't the VIRTIO_GPU_RUTABAGA QOM declaration and this structure be in a separate 
virtio-gpu-rutabaga header file which also includes the header defining struct 
rutabaga? The fact that you're having to pre-declare struct rutabaga in this header 
when rutabaga support is an optional dependency doesn't seem right.

>   #define VIRTIO_GPU_FILL_CMD(out) do {                                   \
>           size_t s;                                                       \
>           s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0,          \


ATB,

Mark.
Gurchetan Singh Aug. 24, 2023, 11:47 p.m. UTC | #2 
On Wed, Aug 23, 2023 at 7:32 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 17/08/2023 03:23, Gurchetan Singh wrote:
>
> > From: Gurchetan Singh <gurchetansingh@chromium.org>
> >
> > This modifies the common virtio-gpu.h file have the fields and
> > defintions needed by gfxstream/rutabaga, by VirtioGpuRutabaga.
> >
> > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> > Tested-by: Alyssa Ross <hi@alyssa.is>
> > Tested-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> > Reviewed-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> > ---
> > v1: void *rutabaga --> struct rutabaga *rutabaga (Akihiko)
> >      have a separate rutabaga device instead of using GL device (Bernard)
> >
> > v2: VirtioGpuRutabaga --> VirtIOGPURutabaga (Akihiko)
> >      move MemoryRegionInfo into VirtIOGPURutabaga (Akihiko)
> >      remove 'ctx' field (Akihiko)
> >      remove 'rutabaga_active'
> >
> > v6: remove command from commit message, refer to docs instead (Manos)
> >
> >   include/hw/virtio/virtio-gpu.h | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> >
> > diff --git a/include/hw/virtio/virtio-gpu.h
> b/include/hw/virtio/virtio-gpu.h
> > index 55973e112f..e2a07e68d9 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -38,6 +38,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPUGL, VIRTIO_GPU_GL)
> >   #define TYPE_VHOST_USER_GPU "vhost-user-gpu"
> >   OBJECT_DECLARE_SIMPLE_TYPE(VhostUserGPU, VHOST_USER_GPU)
> >
> > +#define TYPE_VIRTIO_GPU_RUTABAGA "virtio-gpu-rutabaga-device"
> > +OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPURutabaga, VIRTIO_GPU_RUTABAGA)
> > +
> >   struct virtio_gpu_simple_resource {
> >       uint32_t resource_id;
> >       uint32_t width;
> > @@ -94,6 +97,7 @@ enum virtio_gpu_base_conf_flags {
> >       VIRTIO_GPU_FLAG_DMABUF_ENABLED,
> >       VIRTIO_GPU_FLAG_BLOB_ENABLED,
> >       VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
> > +    VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
> >   };
> >
> >   #define virtio_gpu_virgl_enabled(_cfg) \
> > @@ -108,6 +112,8 @@ enum virtio_gpu_base_conf_flags {
> >       (_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
> >   #define virtio_gpu_context_init_enabled(_cfg) \
> >       (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
> > +#define virtio_gpu_rutabaga_enabled(_cfg) \
> > +    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
> >   #define virtio_gpu_hostmem_enabled(_cfg) \
> >       (_cfg.hostmem > 0)
> >
> > @@ -232,6 +238,28 @@ struct VhostUserGPU {
> >       bool backend_blocked;
> >   };
> >
> > +#define MAX_SLOTS 4096
> > +
> > +struct MemoryRegionInfo {
> > +    int used;
> > +    MemoryRegion mr;
> > +    uint32_t resource_id;
> > +};
> > +
> > +struct rutabaga;
> > +
> > +struct VirtIOGPURutabaga {
> > +    struct VirtIOGPU parent_obj;
>
> The QOM macro should define a typedef for you, so you can drop the
> "struct" here.
>
> > +
> > +    struct MemoryRegionInfo memory_regions[MAX_SLOTS];
> > +    char *capset_names;
> > +    char *wayland_socket_path;
> > +    char *wsi;
> > +    bool headless;
> > +    uint32_t num_capsets;
> > +    struct rutabaga *rutabaga;
> > +};
> > +
>
> Shouldn't the VIRTIO_GPU_RUTABAGA QOM declaration and this structure be in
> a separate
> virtio-gpu-rutabaga header file which also includes the header defining
> struct
> rutabaga? The fact that you're having to pre-declare struct rutabaga in
> this header
> when rutabaga support is an optional dependency doesn't seem right.
>

It is the prevailing style of the virtio-gpu code.  For example, we do have
"virtio_gpu_virgl_*" functions, vhost-user, and udmabuf stubs in the same
file.  So for now, I didn't add an extra header file in v12.  In the
future, separating out optional dependencies into constituent header files
could be future refactoring/cleanup.


>
> >   #define VIRTIO_GPU_FILL_CMD(out) do {
>  \
> >           size_t s;
>  \
> >           s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0,
>   \
>
>
> ATB,
>
> Mark.
>
>
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 55973e112f..e2a07e68d9 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -38,6 +38,9 @@  OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPUGL, VIRTIO_GPU_GL)
 #define TYPE_VHOST_USER_GPU "vhost-user-gpu"
 OBJECT_DECLARE_SIMPLE_TYPE(VhostUserGPU, VHOST_USER_GPU)
 
+#define TYPE_VIRTIO_GPU_RUTABAGA "virtio-gpu-rutabaga-device"
+OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPURutabaga, VIRTIO_GPU_RUTABAGA)
+
 struct virtio_gpu_simple_resource {
     uint32_t resource_id;
     uint32_t width;
@@ -94,6 +97,7 @@  enum virtio_gpu_base_conf_flags {
     VIRTIO_GPU_FLAG_DMABUF_ENABLED,
     VIRTIO_GPU_FLAG_BLOB_ENABLED,
     VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
+    VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
 };
 
 #define virtio_gpu_virgl_enabled(_cfg) \
@@ -108,6 +112,8 @@  enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
 #define virtio_gpu_context_init_enabled(_cfg) \
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
+#define virtio_gpu_rutabaga_enabled(_cfg) \
+    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
 #define virtio_gpu_hostmem_enabled(_cfg) \
     (_cfg.hostmem > 0)
 
@@ -232,6 +238,28 @@  struct VhostUserGPU {
     bool backend_blocked;
 };
 
+#define MAX_SLOTS 4096
+
+struct MemoryRegionInfo {
+    int used;
+    MemoryRegion mr;
+    uint32_t resource_id;
+};
+
+struct rutabaga;
+
+struct VirtIOGPURutabaga {
+    struct VirtIOGPU parent_obj;
+
+    struct MemoryRegionInfo memory_regions[MAX_SLOTS];
+    char *capset_names;
+    char *wayland_socket_path;
+    char *wsi;
+    bool headless;
+    uint32_t num_capsets;
+    struct rutabaga *rutabaga;
+};
+
 #define VIRTIO_GPU_FILL_CMD(out) do {                                   \
         size_t s;                                                       \
         s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0,          \