RE: [PATCH 1/2] virtio-gpu: splitting one extended mode guest fb into n-scanouts

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

Hi DW,

> When guest is running Linux/X11 with extended multiple displays mode enabled,
> the guest shares one scanout resource each time containing whole surface
> rather than sharing individual display output separately. This extended frame
> is properly splited and rendered on the corresponding scanout surfaces but
> not in case of blob-resource (zero copy).
> 
> This code change lets the qemu split this one large surface data into multiple
> in case of blob-resource as well so that each sub frame then can be blitted
> properly to each scanout.
> 
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  hw/display/virtio-gpu-udmabuf.c | 19 +++++++++++--------
>  hw/display/virtio-gpu.c         |  5 +++--
>  include/hw/virtio/virtio-gpu.h  |  5 +++--
>  include/ui/console.h            |  4 ++++
>  stubs/virtio-gpu-udmabuf.c      |  3 ++-
>  5 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> index 3c01a415e7..a64194c6de 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -171,7 +171,8 @@ static VGPUDMABuf
>  *virtio_gpu_create_dmabuf(VirtIOGPU *g,
>                            uint32_t scanout_id,
>                            struct virtio_gpu_simple_resource *res,
> -                          struct virtio_gpu_framebuffer *fb)
> +                          struct virtio_gpu_framebuffer *fb,
> +                          struct virtio_gpu_rect *r)
>  {
>      VGPUDMABuf *dmabuf;
> 
> @@ -183,6 +184,10 @@ static VGPUDMABuf
>      dmabuf->buf.width = fb->width;
>      dmabuf->buf.height = fb->height;
>      dmabuf->buf.stride = fb->stride;
> +    dmabuf->buf.x = r->x;
> +    dmabuf->buf.y = r->y;
> +    dmabuf->buf.scanout_width;
> +    dmabuf->buf.scanout_height;
[Kasireddy, Vivek] Would you not be able to use buf.width and buf.height?
What is the difference between these and scanout_width/height?

>      dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
>      dmabuf->buf.fd = res->dmabuf_fd;
> 
> @@ -195,24 +200,22 @@ static VGPUDMABuf
>  int virtio_gpu_update_dmabuf(VirtIOGPU *g,
>                               uint32_t scanout_id,
>                               struct virtio_gpu_simple_resource *res,
> -                             struct virtio_gpu_framebuffer *fb)
> +                             struct virtio_gpu_framebuffer *fb,
> +                             struct virtio_gpu_rect *r)
>  {
>      struct virtio_gpu_scanout *scanout = &g->parent_obj.scanout[scanout_id];
>      VGPUDMABuf *new_primary, *old_primary = NULL;
> 
> -    new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb);
> +    new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
>      if (!new_primary) {
>          return -EINVAL;
>      }
> 
>      if (g->dmabuf.primary) {
> -        old_primary = g->dmabuf.primary;
> +        old_primary = g->dmabuf.primary[scanout_id];
>      }
> 
> -    g->dmabuf.primary = new_primary;
> -    qemu_console_resize(scanout->con,
> -                        new_primary->buf.width,
> -                        new_primary->buf.height);
> +    g->dmabuf.primary[scanout_id] = new_primary;
>      dpy_gl_scanout_dmabuf(scanout->con, &new_primary->buf);
> 
>      if (old_primary) {
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index e183f4ecda..11a87dad79 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -523,9 +523,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>                  console_has_gl(scanout->con)) {
>                  dpy_gl_update(scanout->con, 0, 0, scanout->width,
[Kasireddy, Vivek] Unrelated to your change but shouldn't 0,0 be replaced with 
scanout->x and scanout->y?

>                                scanout->height);
> -                return;
>              }
>          }
> +        return;
>      }
> 
>      if (!res->blob &&
> @@ -598,6 +598,7 @@ static void virtio_gpu_update_scanout(VirtIOGPU *g,
>      scanout->y = r->y;
>      scanout->width = r->width;
>      scanout->height = r->height;
> +    qemu_console_resize(scanout->con, scanout->width, scanout->height);
[Kasireddy, Vivek] IIRC, there was no qemu_console_resize for the non-blob resources case.
Moving this call to update_scanout means that it will also be called for non-blob resources
Path; not sure if this is OK but you might want restrict this call to only blob.

>  }
> 
>  static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
> @@ -633,7 +634,7 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
> 
>      if (res->blob) {
>          if (console_has_gl(scanout->con)) {
> -            if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb)) {
> +            if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
[Kasireddy, Vivek] Instead of passing the rectangle "r", you might want to consider using
the scanout object which is specific to each scanout and can be easily retried by:
scanout = &g->parent_obj.scanout[scanout_id];

Thanks,
Vivek
>                  virtio_gpu_update_scanout(g, scanout_id, res, r);
>                  return;
>              }
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index bcf54d970f..6372f4bbb5 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -187,7 +187,7 @@ struct VirtIOGPU {
> 
>      struct {
>          QTAILQ_HEAD(, VGPUDMABuf) bufs;
> -        VGPUDMABuf *primary;
> +        VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
>      } dmabuf;
>  };
> 
> @@ -273,7 +273,8 @@ void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource
> *res);
>  int virtio_gpu_update_dmabuf(VirtIOGPU *g,
>                               uint32_t scanout_id,
>                               struct virtio_gpu_simple_resource *res,
> -                             struct virtio_gpu_framebuffer *fb);
> +                             struct virtio_gpu_framebuffer *fb,
> +                             struct virtio_gpu_rect *r);
> 
>  /* virtio-gpu-3d.c */
>  void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> diff --git a/include/ui/console.h b/include/ui/console.h
> index b30b63976a..87316aef83 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -167,6 +167,10 @@ typedef struct QemuDmaBuf {
>      uint32_t  fourcc;
>      uint64_t  modifier;
>      uint32_t  texture;
> +    uint32_t  x;
> +    uint32_t  y;
> +    uint32_t  scanout_width;
> +    uint32_t  scanout_height;
>      bool      y0_top;
>  } QemuDmaBuf;
> 
> diff --git a/stubs/virtio-gpu-udmabuf.c b/stubs/virtio-gpu-udmabuf.c
> index 81f661441a..f692e13510 100644
> --- a/stubs/virtio-gpu-udmabuf.c
> +++ b/stubs/virtio-gpu-udmabuf.c
> @@ -20,7 +20,8 @@ void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource
> *res)
>  int virtio_gpu_update_dmabuf(VirtIOGPU *g,
>                               uint32_t scanout_id,
>                               struct virtio_gpu_simple_resource *res,
> -                             struct virtio_gpu_framebuffer *fb)
> +                             struct virtio_gpu_framebuffer *fb,
> +                             struct virtio_gpu_rect *r)
>  {
>      /* nothing (stub) */
>      return 0;
> --
> 2.17.1
>