Re: [Qemu-devel] [PATCH 4/6] virtio-gpu: maintain command queue

[Marc-André Lureau]

Hi

On Wed, Jan 13, 2016 at 1:02 PM, Gerd Hoffmann <address@hidden> wrote:
> We'll go take out the commands we receive out of the virt queue and put
> them into a linked list, to decouple virtio queue handling from actual
> command processing.
>
> Also move cmd processing to new virtio_gpu_handle_ctrl func, so we can
> easily kick it from different places.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  hw/display/virtio-gpu.c        | 60 
> ++++++++++++++++++++++++++----------------
>  include/hw/virtio/virtio-gpu.h |  1 +
>  2 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 7e79a9c..4433c12 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -754,33 +754,21 @@ static void virtio_gpu_handle_cursor_cb(VirtIODevice 
> *vdev, VirtQueue *vq)
>      qemu_bh_schedule(g->cursor_bh);
>  }
>
> -static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> +static void virtio_cpu_process_cmdq(VirtIOGPU *g)

cpu/gpu

>  {
> -    VirtIOGPU *g = VIRTIO_GPU(vdev);
>      struct virtio_gpu_ctrl_command *cmd;
>
> -    if (!virtio_queue_ready(vq)) {
> -        return;
> -    }
> -
> -#ifdef CONFIG_VIRGL
> -    if (!g->renderer_inited && g->use_virgl_renderer) {
> -        virtio_gpu_virgl_init(g);
> -        g->renderer_inited = true;
> -    }
> -#endif
> -
> -    cmd = g_new(struct virtio_gpu_ctrl_command, 1);
> -    while (virtqueue_pop(vq, &cmd->elem)) {
> -        cmd->vq = vq;
> -        cmd->error = 0;
> -        cmd->finished = false;
> -        if (virtio_gpu_stats_enabled(g->conf)) {
> -            g->stats.requests++;
> -        }
> +    while (!QTAILQ_EMPTY(&g->cmdq)) {
> +        cmd = QTAILQ_FIRST(&g->cmdq);
>
> +        /* process command */
>          VIRGL(g, virtio_gpu_virgl_process_cmd, virtio_gpu_simple_process_cmd,
>                g, cmd);
> +        QTAILQ_REMOVE(&g->cmdq, cmd, next);
> +        if (virtio_gpu_stats_enabled(g->conf)) {
> +            g->stats.requests++;
> +        }
> +
>          if (!cmd->finished) {
>              QTAILQ_INSERT_TAIL(&g->fenceq, cmd, next);
>              g->inflight++;
> @@ -790,11 +778,38 @@ static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, 
> VirtQueue *vq)
>                  }
>                  fprintf(stderr, "inflight: %3d (+)\r", g->inflight);
>              }
> -            cmd = g_new(struct virtio_gpu_ctrl_command, 1);
>          }

else g_free(cmd)

>      }
> +}
> +
> +static void virtio_gpu_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOGPU *g = VIRTIO_GPU(vdev);
> +    struct virtio_gpu_ctrl_command *cmd;
> +
> +    if (!virtio_queue_ready(vq)) {
> +        return;
> +    }
> +
> +#ifdef CONFIG_VIRGL
> +    if (!g->renderer_inited && g->use_virgl_renderer) {
> +        virtio_gpu_virgl_init(g);
> +        g->renderer_inited = true;
> +    }
> +#endif
> +
> +    cmd = g_new0(struct virtio_gpu_ctrl_command, 1);

g_new0 is costly (the struct is big) and quite unnecessary here:

> +    while (virtqueue_pop(vq, &cmd->elem)) {
> +        cmd->vq = vq;
> +        cmd->error = 0;
> +        cmd->finished = false;

Add:
+ cmd->waiting = false;

and you can switch to g_new.

> +        QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
> +        cmd = g_new0(struct virtio_gpu_ctrl_command, 1);

same

> +    }
>      g_free(cmd);
>
> +    virtio_cpu_process_cmdq(g);
> +
>  #ifdef CONFIG_VIRGL
>      if (g->use_virgl_renderer) {
>          virtio_gpu_virgl_fence_poll(g);
> @@ -920,6 +935,7 @@ static void virtio_gpu_device_realize(DeviceState *qdev, 
> Error **errp)
>      g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
>      g->cursor_bh = qemu_bh_new(virtio_gpu_cursor_bh, g);
>      QTAILQ_INIT(&g->reslist);
> +    QTAILQ_INIT(&g->cmdq);
>      QTAILQ_INIT(&g->fenceq);
>
>      g->enabled_output_bitmask = 1;
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 9b279d7..f7e7a52 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -94,6 +94,7 @@ typedef struct VirtIOGPU {
>      DeviceState *qdev;
>
>      QTAILQ_HEAD(, virtio_gpu_simple_resource) reslist;
> +    QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq;
>      QTAILQ_HEAD(, virtio_gpu_ctrl_command) fenceq;
>
>      struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUT];
> --
> 1.8.3.1
>



-- 
Marc-André Lureau