Hi Jiaxun,
On Sat, 20 Jul 2024 at 07:58, Jiaxun Yang <jiaxun.y...@flygoat.com> wrote:
>
>
>
> 在2024年7月19日七月 下午11:05，Simon Glass写道：
> > Hi Jiaxun,
> >
> > On Wed, 17 Jul 2024 at 15:34, Jiaxun Yang <jiaxun.y...@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.y...@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 <s...@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.y...@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
