[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/9] virtio-gpu: add 3d mode and virgl rendering
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 5/9] virtio-gpu: add 3d mode and virgl rendering support. |
Date: |
Mon, 14 Sep 2015 20:14:54 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
On 09.09.2015 13:20, Gerd Hoffmann wrote:
> Add virglrenderer library detection. Add 3d mode to virtio-gpu,
> wire up virglrenderer library. When in 3d mode render using the
> new context management and texture scanout callbacks.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
> configure | 32 +++
> hw/display/Makefile.objs | 6 +-
> hw/display/virtio-gpu-3d.c | 598
> +++++++++++++++++++++++++++++++++++++++++
> hw/display/virtio-gpu.c | 130 ++++++++-
> include/hw/virtio/virtio-gpu.h | 22 +-
> trace-events | 8 +
> 6 files changed, 785 insertions(+), 11 deletions(-)
> create mode 100644 hw/display/virtio-gpu-3d.c
As always when it comes to virtio-gpu, I'm just a very, very naive
reviewer, thus I only have "local" things to complain about.
[snip]
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> new file mode 100644
> index 0000000..ac62fb4
> --- /dev/null
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -0,0 +1,598 @@
[snip]
> +static void virgl_resource_detach_backing(VirtIOGPU *g,
> + struct virtio_gpu_ctrl_command
> *cmd)
> +{
> + struct virtio_gpu_resource_detach_backing detach_rb;
> + struct iovec *res_iovs = NULL;
> + int num_iovs = 0;
> +
> + VIRTIO_GPU_FILL_CMD(detach_rb);
> + trace_virtio_gpu_cmd_res_back_detach(detach_rb.resource_id);
> +
> + virgl_renderer_resource_detach_iov(detach_rb.resource_id,
> + &res_iovs,
> + &num_iovs);
> + if (res_iovs == NULL || num_iovs == 0) {
> + return;
> + }
> + virtio_gpu_cleanup_mapping_iov(res_iovs, num_iovs);
Is res_iovs leaked here?
> +}
[snip]
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index a67d927..4dbdd12 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
[snip]
> @@ -45,6 +62,27 @@ static void update_cursor_data_simple(VirtIOGPU *g,
> pixels * sizeof(uint32_t));
> }
>
> +#ifdef CONFIG_VIRGL
> +
> +static void update_cursor_data_virgl(VirtIOGPU *g,
> + struct virtio_gpu_scanout *s,
> + uint32_t resource_id)
> +{
> + uint32_t width, height;
> + uint32_t pixels, *data;
> +
> + data = virgl_renderer_get_cursor_data(resource_id, &width, &height);
> + if (!data) {
> + return;
> + }
> +
> + pixels = s->current_cursor->width * s->current_cursor->height;
> + memcpy(s->current_cursor->data, data, pixels * sizeof(uint32_t));
> + free(data);
width and height are unused; should they be compared against
s->current_cursor->{width,height} to spot discrepancies?
> +}
> +
> +#endif
> +
> static void update_cursor(VirtIOGPU *g, struct virtio_gpu_update_cursor
> *cursor)
> {
> struct virtio_gpu_scanout *s;
[snip]
> @@ -92,9 +131,23 @@ static void virtio_gpu_set_config(VirtIODevice *vdev,
> const uint8_t *config)
> static uint64_t virtio_gpu_get_features(VirtIODevice *vdev, uint64_t
> features,
> Error **errp)
> {
> + VirtIOGPU *g = VIRTIO_GPU(vdev);
> +
> + if (virtio_gpu_virgl_enabled(g->conf)) {
> + features |= (1 << VIRTIO_GPU_FEATURE_VIRGL);
> + }
> return features;
> }
>
> +static void virtio_gpu_set_features(VirtIODevice *vdev, uint64_t features)
> +{
> + static const uint32_t virgl = (1 << VIRTIO_GPU_FEATURE_VIRGL);
> + VirtIOGPU *g = VIRTIO_GPU(vdev);
> +
> + g->use_virgl_renderer = ((features & virgl) == virgl);
Could a non-well-behaving guest just set this feature bit even if it was
not reported by virtio_gpu_get_features() because it has been disabled?
This would then result in g->use_virgl_renderer being set to true, which
would have two implications:
(1) In case CONFIG_VIRGL is not defined, no implication right now. But
maybe in the future there may be cases where the flag is queried
even with !defined(CONFIG_VIRGL).
(2) virgl may be disabled even with CONFIG_VIRGL being defined; namely
with HOST_WORDS_BIGENDIAN. That would probably be a problem, because
we'd then call the virgl functions even though we meant not to.
Max
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 2/9] sdl2: quick & dirty flicker workaround, (continued)
- [Qemu-devel] [PATCH 3/9] ui/console: add opengl context and scanout support interfaces., Gerd Hoffmann, 2015/09/09
- [Qemu-devel] [PATCH 9/9] gtk/opengl: add opengl context and scanout support (GtkGLArea), Gerd Hoffmann, 2015/09/09
- [Qemu-devel] [PATCH 8/9] gtk/opengl: add opengl context and scanout support (egl), Gerd Hoffmann, 2015/09/09
- [Qemu-devel] [PATCH 5/9] virtio-gpu: add 3d mode and virgl rendering support., Gerd Hoffmann, 2015/09/09
- Re: [Qemu-devel] [PATCH 0/9] add virgl rendering support., Marc-André Lureau, 2015/09/11