On 6/23/24 08:44, Akihiko Odaki wrote: > On 2024/06/23 6:55, Dmitry Osipenko wrote: >> From: Robert Beckett <bob.beck...@collabora.com> >> >> Support displaying blob resources by handling SET_SCANOUT_BLOB >> command. >> >> Signed-by: Antonio Caggiano <antonio.caggi...@collabora.com> >> Signed-off-by: Robert Beckett <bob.beck...@collabora.com> >> Signed-off-by: Huang Rui <ray.hu...@amd.com> >> Reviewed-by: Antonio Caggiano <quic_acagg...@quicinc.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipe...@collabora.com> >> --- >> hw/display/virtio-gpu-virgl.c | 109 +++++++++++++++++++++++++++++++++ >> hw/display/virtio-gpu.c | 12 ++-- >> include/hw/virtio/virtio-gpu.h | 7 +++ >> 3 files changed, 122 insertions(+), 6 deletions(-) >> >> diff --git a/hw/display/virtio-gpu-virgl.c >> b/hw/display/virtio-gpu-virgl.c >> index 3ffea478e723..60befab7efc2 100644 >> --- a/hw/display/virtio-gpu-virgl.c >> +++ b/hw/display/virtio-gpu-virgl.c >> @@ -17,6 +17,8 @@ >> #include "trace.h" >> #include "hw/virtio/virtio.h" >> #include "hw/virtio/virtio-gpu.h" >> +#include "hw/virtio/virtio-gpu-bswap.h" >> +#include "hw/virtio/virtio-gpu-pixman.h" >> #include "ui/egl-helpers.h" >> @@ -78,6 +80,7 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU >> *g, >> res->base.height = c2d.height; >> res->base.format = c2d.format; >> res->base.resource_id = c2d.resource_id; >> + res->base.dmabuf_fd = -1; >> QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next); >> args.handle = c2d.resource_id; >> @@ -125,6 +128,7 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU >> *g, >> res->base.height = c3d.height; >> res->base.format = c3d.format; >> res->base.resource_id = c3d.resource_id; >> + res->base.dmabuf_fd = -1; >> QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next); >> args.handle = c3d.resource_id; >> @@ -509,6 +513,106 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, >> g_free(resp); >> } >> +#if VIRGL_VERSION_MAJOR >= 1 >> +static void virgl_cmd_set_scanout_blob(VirtIOGPU *g, >> + struct virtio_gpu_ctrl_command >> *cmd) >> +{ >> + struct virtio_gpu_framebuffer fb = { 0 }; >> + struct virgl_renderer_resource_info info; >> + struct virtio_gpu_virgl_resource *res; >> + struct virtio_gpu_set_scanout_blob ss; >> + uint64_t fbend; >> + >> + VIRTIO_GPU_FILL_CMD(ss); >> + virtio_gpu_scanout_blob_bswap(&ss); >> + trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id, >> + ss.r.width, ss.r.height, >> ss.r.x, >> + ss.r.y); >> + >> + if (ss.scanout_id >= g->parent_obj.conf.max_outputs) { >> + qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id >> specified %d", >> + __func__, ss.scanout_id); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID; >> + return; >> + } >> + >> + if (ss.resource_id == 0) { >> + virtio_gpu_disable_scanout(g, ss.scanout_id); >> + return; >> + } >> + >> + if (ss.width < 16 || >> + ss.height < 16 || >> + ss.r.x + ss.r.width > ss.width || >> + ss.r.y + ss.r.height > ss.height) { >> + qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds >> for" >> + " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n", >> + __func__, ss.scanout_id, ss.resource_id, >> + ss.r.x, ss.r.y, ss.r.width, ss.r.height, >> + ss.width, ss.height); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; >> + return; >> + } >> + >> + res = virtio_gpu_virgl_find_resource(g, ss.resource_id); >> + if (!res) { >> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist >> %d\n", >> + __func__, ss.resource_id); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >> + return; >> + } >> + if (virgl_renderer_resource_get_info(ss.resource_id, &info)) { >> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not have >> info %d\n", >> + __func__, ss.resource_id); >> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >> + return; >> + } >> + if (res->base.dmabuf_fd < 0) { >> + res->base.dmabuf_fd = info.fd; > > Just in case you missed my previous email: >> res->base.dmabuf_fd is conditionally assigned but >> virgl_renderer_resource_get_info() is called unconditionally, which is >> inconsistent. > >> The relevant code is better to be moved into >> virgl_cmd_resource_create_blob() for consistenty with >> virtio_gpu_resource_create_blob().
Right, I squashed this change into patch #12 instead of #10 by mistake. Will fix it shortly, thanks for the review! -- Best regards, Dmitry