Re: [PATCH v2 1/2] virtio: New virtio_gpu driver

Hi Jiaxun,

On Sat, 20 Jul 2024 at 07:58, Jiaxun Yang jiaxun.yang@flygoat.com wrote:

在2024年7月19日七月 下午11:05，Simon Glass写道：

...

Hi Jiaxun,

On Wed, 17 Jul 2024 at 15:34, Jiaxun Yang jiaxun.yang@flygoat.com wrote:

...

在2024年5月24日五月 下午9:02，Jiaxun Yang写道：

...

This driver is implemened based on latest VirtIO spec. It follows operation prodcure as defined in the spec.

It implemented multihead (mirroring) support as well.

Signed-off-by: Jiaxun Yang jiaxun.yang@flygoat.com

Ping

...

---

v2: - Add big endian code path - Reword typical resolution for Kconfig symbol

---

drivers/virtio/Kconfig | 29 +++ drivers/virtio/Makefile | 1 + drivers/virtio/virtio-uclass.c | 1 + drivers/virtio/virtio_gpu.c | 302 +++++++++++++++++++++++++++++ drivers/virtio/virtio_gpu.h | 428 +++++++++++++++++++++++++++++++++++++++++ include/virtio.h | 4 +- 6 files changed, 764 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass sjg@chromium.org

Nits below

...

...

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig index 1de68867d52e..a4838278fabc 100644 --- a/drivers/virtio/Kconfig +++ b/drivers/virtio/Kconfig @@ -76,4 +76,33 @@ config VIRTIO_RNG help This is the virtual random number generator driver. It can be used with QEMU based targets.

• config VIRTIO_GPU

• bool "virtio GPU driver"
  

• depends on VIRTIO && VIDEO
  

• default y
  

• help
  

•   This is the virtual GPU display for virtio. It can be used with QEMU
  

•   based targets.
  

QEMU-based

Can you add a bit more info about its capabilities? Should we be using this instead of Bochs?

I think it’s up to users preference? For U-Boot there is no visible benefits, for Linux you can enjoy some accel features. I’m not really sure if it’s relevant for U-Boot Kconfig.

...

...

...

+if VIRTIO_GPU +config VIRTIO_GPU_SIZE_X

• int "Width of display (X resolution)"
  

• default 1280
  

• help
  

•   Sets the width of the display.
  

•   These two options control the size of the display set up by QEMU.
  

•   Typical size is 1280 x 1024 for compatibility.
  

+config VIRTIO_GPU_SIZE_Y

• int "High of display (Y resolution)"
  

Height

...

...

• default 1024
  

• help
  

•   Sets the height of the display.
  

•   These two options control the size of the display set up by QEMU.
  

•   Typical size is 1280 x 1024 for compatibility.
  

+endif endmenu diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile index 4c63a6c69043..c830fb6e6049 100644 --- a/drivers/virtio/Makefile +++ b/drivers/virtio/Makefile @@ -11,3 +11,4 @@ obj-$(CONFIG_VIRTIO_SANDBOX) += virtio_sandbox.o obj-$(CONFIG_VIRTIO_NET) += virtio_net.o obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o obj-$(CONFIG_VIRTIO_RNG) += virtio_rng.o +obj-$(CONFIG_VIRTIO_GPU) += virtio_gpu.o diff --git a/drivers/virtio/virtio-uclass.c b/drivers/virtio/virtio-uclass.c index 1dbc1a56aa21..1f3cdbf689c4 100644 --- a/drivers/virtio/virtio-uclass.c +++ b/drivers/virtio/virtio-uclass.c @@ -30,6 +30,7 @@ static const char *const virtio_drv_name[VIRTIO_ID_MAX_NUM] = { [VIRTIO_ID_NET] = VIRTIO_NET_DRV_NAME, [VIRTIO_ID_BLOCK] = VIRTIO_BLK_DRV_NAME, [VIRTIO_ID_RNG] = VIRTIO_RNG_DRV_NAME,

• [VIRTIO_ID_GPU]         = VIRTIO_GPU_DRV_NAME,
  

};

int virtio_get_config(struct udevice *vdev, unsigned int offset, diff --git a/drivers/virtio/virtio_gpu.c b/drivers/virtio/virtio_gpu.c new file mode 100644 index 000000000000..0b306bb9d2fa --- /dev/null +++ b/drivers/virtio/virtio_gpu.c @@ -0,0 +1,302 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2024, Jiaxun Yang jiaxun.yang@flygoat.com
• */

+#define pr_fmt(fmt) "virtio_gpu: " fmt

+#include <dm.h> +#include <log.h> +#include <malloc.h> +#include <video.h> +#include <virtio_types.h> +#include <virtio.h> +#include <virtio_ring.h> +#include "virtio_gpu.h" +#include <asm/io.h>

+struct virtio_gpu_priv {

• struct virtqueue *vq;
  

• u32 scanout_res_id;
  

• u64 fence_id;
  

• bool in_sync;
  

+};

+static int virtio_gpu_do_req(struct udevice *dev,

•                      enum virtio_gpu_ctrl_type type,
  

•                      void *in, size_t in_size,
  

•                      void *out, size_t out_size, bool flush)
  

Please add a function comment

...

...

+{

• int ret;
  

• uint len;
  

• struct virtio_gpu_priv *priv = dev_get_priv(dev);
  

• struct virtio_sg in_sg;
  

• struct virtio_sg out_sg;
  

• struct virtio_sg *sgs[] = { &in_sg, &out_sg };
  

• struct virtio_gpu_ctrl_hdr *ctrl_hdr_in = in;
  

• struct virtio_gpu_ctrl_hdr *ctrl_hdr_out = out;
  

• ctrl_hdr_in->type = cpu_to_virtio32(dev, (u32)type);
  

• if (flush) {
  

•         ctrl_hdr_in->flags = cpu_to_virtio32(dev, VIRTIO_GPU_FLAG_FENCE);
  

•         ctrl_hdr_in->fence_id = cpu_to_virtio64(dev, priv->fence_id++);
  

• } else {
  

•         ctrl_hdr_in->flags = 0;
  

•         ctrl_hdr_in->fence_id = 0;
  

• }
  

• ctrl_hdr_in->ctx_id = 0;
  

• ctrl_hdr_in->ring_idx = 0;
  

• in_sg.addr = in;
  

• in_sg.length = in_size;
  

• out_sg.addr = out;
  

• out_sg.length = out_size;
  

• ret = virtqueue_add(priv->vq, sgs, 1, 1);
  

• if (ret) {
  

•         log_debug("virtqueue_add failed %d\n", ret);
  

•         return ret;
  

• }
  

• virtqueue_kick(priv->vq);
  

• debug("wait...");
  

• while (!virtqueue_get_buf(priv->vq, &len))
  

•         ;
  

• debug("done\n");
  

• if (out_size != len) {
  

•         log_debug("Invalid response size %d, expected %d\n",
  

•                   len, (uint)out_size);
  

• }
  

• return virtio32_to_cpu(dev, ctrl_hdr_out->type);
  

+}

+static int virtio_gpu_probe(struct udevice *dev) +{

• struct virtio_gpu_priv *priv = dev_get_priv(dev);
  

• struct video_uc_plat *plat = dev_get_uclass_plat(dev);
  

• struct video_priv *uc_priv = dev_get_uclass_priv(dev);
  

• struct virtio_gpu_ctrl_hdr ctrl_hdr_in;
  

• struct virtio_gpu_ctrl_hdr ctrl_hdr_out;
  

• struct virtio_gpu_resp_display_info *disp_info_out;
  

• struct virtio_gpu_display_one *disp;
  

• struct virtio_gpu_resource_create_2d *res_create_2d_in;
  

• void *res_buf_in;
  

• struct virtio_gpu_resource_attach_backing *res_attach_backing_in;
  

• struct virtio_gpu_mem_entry *mem_entry;
  

• struct virtio_gpu_set_scanout *set_scanout_in;
  

• unsigned int scanout_mask = 0;
  

• int ret, i;
  

• if (!plat->base) {
  

•         log_warning("No framebuffer allocated\n");
  

•         return -EINVAL;
  

• }
  

• ret = virtio_find_vqs(dev, 1, &priv->vq);
  

• if (ret < 0) {
  

•         log_warning("virtio_find_vqs failed\n");
  

•         return ret;
  

• }
  

• disp_info_out = malloc(sizeof(struct virtio_gpu_resp_display_info));
  

• ret = virtio_gpu_do_req(dev, VIRTIO_GPU_CMD_GET_DISPLAY_INFO,
  

&ctrl_hdr_in,

•                         sizeof(struct virtio_gpu_ctrl_hdr), disp_info_out,
  

•                         sizeof(struct virtio_gpu_resp_display_info), false);
  

• if (ret != VIRTIO_GPU_RESP_OK_DISPLAY_INFO) {
  

•         log_warning("CMD_GET_DISPLAY_INFO failed %d\n", ret);
  

•         ret = -EINVAL;
  

•         goto out_free_disp;
  

• }
  

• for (i = 0; i < VIRTIO_GPU_MAX_SCANOUTS; i++) {
  

•         disp = &disp_info_out->pmodes[i];
  

•         if (!disp->enabled)
  

•                 continue;
  

•         log_debug("Found available scanout: %d\n", i);
  

•         scanout_mask |= 1 << i;
  

• }
  

• if (!scanout_mask) {
  

•         log_warning("No active scanout found\n");
  

•         ret = -EINVAL;
  

•         goto out_free_disp;
  

• }
  

• free(disp_info_out);
  

• disp_info_out = NULL;
  

• /* TODO: We can parse EDID for those info */
  

• uc_priv->xsize = CONFIG_VAL(VIRTIO_GPU_SIZE_X);
  

• uc_priv->ysize = CONFIG_VAL(VIRTIO_GPU_SIZE_Y);
  

• uc_priv->bpix = VIDEO_BPP32;
  

• priv->scanout_res_id = 1;
  

• res_create_2d_in = malloc(sizeof(struct
  

virtio_gpu_resource_create_2d));

• res_create_2d_in->resource_id = cpu_to_virtio32(dev,
  

priv->scanout_res_id); +#ifdef __BIG_ENDIAN

• res_create_2d_in->format = cpu_to_virtio32(dev,
  

VIRTIO_GPU_FORMAT_X8B8G8R8_UNORM); +#else

• res_create_2d_in->format = cpu_to_virtio32(dev,
  

VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM); +#endif

• res_create_2d_in->width = cpu_to_virtio32(dev, uc_priv->xsize);
  

• res_create_2d_in->height = cpu_to_virtio32(dev, uc_priv->ysize);
  

• ret = virtio_gpu_do_req(dev, VIRTIO_GPU_CMD_RESOURCE_CREATE_2D,
  

res_create_2d_in,

•                         sizeof(struct virtio_gpu_resource_create_2d), &ctrl_hdr_out,
  

•                         sizeof(struct virtio_gpu_ctrl_hdr), false);
  

• if (ret != VIRTIO_GPU_RESP_OK_NODATA) {
  

•         log_warning("CMD_RESOURCE_CREATE_2D failed %d\n", ret);
  

•         ret = -EINVAL;
  

•         goto out_free_res_create_2d;
  

• }
  

• free(res_create_2d_in);
  

• res_create_2d_in = NULL;
  

• res_buf_in = malloc(sizeof(struct virtio_gpu_resource_attach_backing)
  

•                     sizeof(struct virtio_gpu_mem_entry));
  

• res_attach_backing_in = res_buf_in;
  

• mem_entry = res_buf_in + sizeof(struct
  

virtio_gpu_resource_attach_backing);

• res_attach_backing_in->resource_id = cpu_to_virtio32(dev,
  

priv->scanout_res_id);

• res_attach_backing_in->nr_entries = cpu_to_virtio32(dev, 1);
  

• mem_entry->addr = cpu_to_virtio64(dev, virt_to_phys((void
  

*)plat->base));

• mem_entry->length = cpu_to_virtio32(dev, plat->size);
  

• mem_entry->padding = 0;
  

• ret = virtio_gpu_do_req(dev, VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING,
  

res_buf_in,

•                         sizeof(struct virtio_gpu_resource_attach_backing) +
  

•                         sizeof(struct virtio_gpu_mem_entry), &ctrl_hdr_out,
  

•                         sizeof(struct virtio_gpu_ctrl_hdr), false);
  

• if (ret != VIRTIO_GPU_RESP_OK_NODATA) {
  

•         log_warning("CMD_RESOURCE_ATTACH_BACKING failed %d\n", ret);
  

•         ret = -EINVAL;
  

•         goto out_free_res_buf;
  

• }
  

• free(res_buf_in);
  

• res_buf_in = NULL;
  

• set_scanout_in = malloc(sizeof(struct virtio_gpu_set_scanout));
  

Could these structs be inside priv instead of allocating each one?

Those structs are all used only once at initialization.

We can save some runtime memory by freeing them here :-)

Then you can just use a local var.

...

...

...

• while (scanout_mask) {
  

•         u32 active_scanout = ffs(scanout_mask) - 1;
  

•         set_scanout_in->r.x = 0;
  

•         set_scanout_in->r.y = 0;
  

•         set_scanout_in->r.width = cpu_to_virtio32(dev, uc_priv->xsize);
  

•         set_scanout_in->r.height = cpu_to_virtio32(dev, uc_priv->ysize);
  

•         set_scanout_in->scanout_id = cpu_to_virtio32(dev, active_scanout);
  

•         set_scanout_in->resource_id = cpu_to_virtio32(dev,
  

priv->scanout_res_id);

[..]

...

...

...

+/* data passed in the cursor vq */

+struct virtio_gpu_cursor_pos {

• __le32 scanout_id;
  

• __le32 x;
  

• __le32 y;
  

• __le32 padding;
  

+};

+/* VIRTIO_GPU_CMD_UPDATE_CURSOR, VIRTIO_GPU_CMD_MOVE_CURSOR */ +struct virtio_gpu_update_cursor {

• struct virtio_gpu_ctrl_hdr hdr;
  

• struct virtio_gpu_cursor_pos pos;  /* update & move */
  

• __le32 resource_id;           /* update only */
  

• __le32 hot_x;                 /* update only */
  

• __le32 hot_y;                 /* update only */
  

• __le32 padding;
  

+};

+/* data passed in the control vq, 2d related */

But there are no comments so we don't know what these structs are for or what the fields do. Can you add documentation, or a pointer to somewhere where it exists?

The whole header is copied from Linux kernel so I’m not really sure if we want to edit it.

Maybe I can mention about the specification at start of the driver code?

Yes that would help.

I thought Linux's aversion to comments had softened a little?

Regards, Simon