From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 70A6EEED616 for ; Fri, 15 Sep 2023 16:05:04 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qhBIz-0000et-7d; Fri, 15 Sep 2023 12:04:29 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qhBIy-0000eY-9n for qemu-devel@nongnu.org; Fri, 15 Sep 2023 12:04:28 -0400 Received: from mail-pf1-x429.google.com ([2607:f8b0:4864:20::429]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qhBIv-0004uz-JV for qemu-devel@nongnu.org; Fri, 15 Sep 2023 12:04:28 -0400 Received: by mail-pf1-x429.google.com with SMTP id d2e1a72fcca58-68fb6fd2836so2102889b3a.0 for ; Fri, 15 Sep 2023 09:04:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=daynix-com.20230601.gappssmtp.com; s=20230601; t=1694793864; x=1695398664; darn=nongnu.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=7xjJ3MhLzFB/78TdP6TmU291XQqb5tjT3DKBrgO8H54=; b=1Z9v5+tQbb3c3Na+Rqm8NGmeVC02hicYSbouLFwZ7Mz9P3Cr3wvznSd30QnIZT0YLb ZzZs0JdDn+wr/GGM521mmvTGDFkXYCIQJqBuML2E+rRxSMhrzA9kPmn0jiJbIzNmeZm7 nK9TVOWPgrEARgXlSJ7sUu4vxra+bzu+DJc76MrUhFAenFn08QuQk/cnZxnFBjNG5Xem WYng0MxFx94fomEl/FCMbAyVeg18/6dwDwbDoPf5P/Yeui2HQ0VDU4KCQqg8XG/gDV3B WeqItW1QX5D4sg6blp8RIcbuRuEZddC7zaldMOW3uwjWRPbFVzZj18+IlTHzOryBzwvo lwIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694793864; x=1695398664; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7xjJ3MhLzFB/78TdP6TmU291XQqb5tjT3DKBrgO8H54=; b=YAXV9GQdgctGfSH4YWE03p6TJGiCp62yHB/kiggcd/PYq8mQgWYJhOM5CMDb83UHTC dadIGxfdU02/j5R81gmATQcZngMlf4vlS5+rXFJEsBW+PITuhRK4MGEpOjRqm8XlpXT6 YH+BeacNZt3v6kpwdr1h+R+cdyY5q/Xa7+I6z2oTVvWao+WjqEAjSuuPAs2mLBRzton3 IOXAZYiCkDiHt6WEtvAPmcDvS2dAjNxotm2IhfP+usGoQrk5kaXTDbaGhqwN7g0ULGJF QzzCCn19ZEgASJU2l2WDthN4vRum8+yDq8THLl+TUArt1s45Np0iDSx79kEbHjESRLWr VJsQ== X-Gm-Message-State: AOJu0YxZHebMRWCZK/nuCfHqPUKA8+jclpdYL4qFPswfxLyCRRbmzPde hE6LiFOVssExcuBuxOGscjINIQ== X-Google-Smtp-Source: AGHT+IHLH1is+UJko0Vbh8oF95stkGXSzkqKgn7kiaS3g+QftQn3FWhjkA2JmUSB343OR7NtN9GNrA== X-Received: by 2002:a05:6a00:1884:b0:68f:c078:b0b6 with SMTP id x4-20020a056a00188400b0068fc078b0b6mr2439986pfh.10.1694793863749; Fri, 15 Sep 2023 09:04:23 -0700 (PDT) Received: from ?IPV6:2400:4050:a840:1e00:78d2:b862:10a7:d486? ([2400:4050:a840:1e00:78d2:b862:10a7:d486]) by smtp.gmail.com with ESMTPSA id a26-20020aa780da000000b0068ff267f094sm3150033pfn.158.2023.09.15.09.04.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 15 Sep 2023 09:04:23 -0700 (PDT) Message-ID: Date: Sat, 16 Sep 2023 01:04:17 +0900 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands Content-Language: en-US To: Huang Rui , Gerd Hoffmann , "Michael S . Tsirkin" , Stefano Stabellini , Anthony PERARD , Antonio Caggiano , "Dr . David Alan Gilbert" , Robert Beckett , Dmitry Osipenko , =?UTF-8?Q?Alex_Benn=C3=A9e?= , qemu-devel@nongnu.org Cc: xen-devel@lists.xenproject.org, Gurchetan Singh , Albert Esteve , ernunes@redhat.com, =?UTF-8?Q?Philippe_Mathieu-Daud=C3=A9?= , Alyssa Ross , =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= , Alex Deucher , =?UTF-8?Q?Christian_K=C3=B6nig?= , Xenia Ragiadakou , Pierre-Eric Pelloux-Prayer , Honglei Huang , Julia Zhang , Chen Jiqian , Antonio Caggiano References: <20230915111130.24064-1-ray.huang@amd.com> <20230915111130.24064-10-ray.huang@amd.com> From: Akihiko Odaki In-Reply-To: <20230915111130.24064-10-ray.huang@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Received-SPF: none client-ip=2607:f8b0:4864:20::429; envelope-from=akihiko.odaki@daynix.com; helo=mail-pf1-x429.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On 2023/09/15 20:11, Huang Rui wrote: > From: Antonio Caggiano > > Support BLOB resources creation, mapping and unmapping by calling the > new stable virglrenderer 0.10 interface. Only enabled when available and > via the blob config. E.g. -device virtio-vga-gl,blob=true > > Signed-off-by: Antonio Caggiano > Signed-off-by: Dmitry Osipenko > Signed-off-by: Xenia Ragiadakou > Signed-off-by: Huang Rui > --- > > V4 -> V5: > - Use memory_region_init_ram_ptr() instead of > memory_region_init_ram_device_ptr() (Akihiko) > > hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > hw/display/virtio-gpu.c | 4 +- > include/hw/virtio/virtio-gpu.h | 5 + > meson.build | 4 + > 4 files changed, 225 insertions(+), 1 deletion(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 312953ec16..563a6f2f58 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -17,6 +17,7 @@ > #include "trace.h" > #include "hw/virtio/virtio.h" > #include "hw/virtio/virtio-gpu.h" > +#include "hw/virtio/virtio-gpu-bswap.h" > > #include "ui/egl-helpers.h" > > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > virgl_renderer_resource_create(&args, NULL, 0); > } > > +static void virgl_resource_destroy(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res) > +{ > + if (!res) > + return; > + > + QTAILQ_REMOVE(&g->reslist, res, next); > + > + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > + g_free(res->addrs); > + > + g_free(res); > +} > + > static void virgl_cmd_resource_unref(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > + struct virtio_gpu_simple_resource *res; > struct virtio_gpu_resource_unref unref; > struct iovec *res_iovs = NULL; > int num_iovs = 0; > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > VIRTIO_GPU_FILL_CMD(unref); > trace_virtio_gpu_cmd_res_unref(unref.resource_id); > > + res = virtio_gpu_find_resource(g, unref.resource_id); > + > virgl_renderer_resource_detach_iov(unref.resource_id, > &res_iovs, > &num_iovs); > if (res_iovs != NULL && num_iovs != 0) { > virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > + if (res) { > + res->iov = NULL; > + res->iov_cnt = 0; > + } > } > + > virgl_renderer_resource_unref(unref.resource_id); > + > + virgl_resource_destroy(g, res); > } > > static void virgl_cmd_context_create(VirtIOGPU *g, > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, > g_free(resp); > } > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_create_blob cblob; > + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; > + int ret; > + > + VIRTIO_GPU_FILL_CMD(cblob); > + virtio_gpu_create_blob_bswap(&cblob); > + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); > + > + if (cblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, cblob.resource_id); > + if (res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", > + __func__, cblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = g_new0(struct virtio_gpu_simple_resource, 1); > + if (!res) { > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; > + } > + > + res->resource_id = cblob.resource_id; > + res->blob_size = cblob.size; > + > + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { > + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), > + cmd, &res->addrs, &res->iov, > + &res->iov_cnt); > + if (!ret) { > + g_free(res); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + return; > + } > + } > + > + QTAILQ_INSERT_HEAD(&g->reslist, res, next); > + > + virgl_args.res_handle = cblob.resource_id; > + virgl_args.ctx_id = cblob.hdr.ctx_id; > + virgl_args.blob_mem = cblob.blob_mem; > + virgl_args.blob_id = cblob.blob_id; > + virgl_args.blob_flags = cblob.blob_flags; > + virgl_args.size = cblob.size; > + virgl_args.iovecs = res->iov; > + virgl_args.num_iovs = res->iov_cnt; > + > + ret = virgl_renderer_resource_create_blob(&virgl_args); > + if (ret) { > + virgl_resource_destroy(g, res); > + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + } > +} > + > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_map_blob mblob; > + int ret; > + void *data; > + uint64_t size; > + struct virtio_gpu_resp_map_info resp; > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + VIRTIO_GPU_FILL_CMD(mblob); > + virtio_gpu_map_blob_bswap(&mblob); > + > + if (mblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, mblob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + if (res->region) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); > + if (ret) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res->region = g_new0(MemoryRegion, 1); > + if (!res->region) { > + virgl_renderer_resource_unmap(res->resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; > + } > + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); > + OBJECT(res->region)->free = g_free; > + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); > + memory_region_set_enabled(res->region, true); > + > + memset(&resp, 0, sizeof(resp)); > + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; > + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); > + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); > +} > + > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, > + struct virtio_gpu_simple_resource > + *res) > +{ > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", > + __func__, res->resource_id); > + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; This returns enum virtio_gpu_ctrl_type while virgl_renderer_resource_unmap() returns -errno and the returned error code is ignored. I think you can just inline this function into virgl_cmd_resource_unmap_blob(). It will be more concise and consistent with virgl_cmd_resource_map_blob(). > + } > + > + memory_region_set_enabled(res->region, false); > + memory_region_del_subregion(&b->hostmem, res->region); > + object_unparent(OBJECT(res->region)); > + res->region = NULL; > + > + return virgl_renderer_resource_unmap(res->resource_id); > +} > + > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_unmap_blob ublob; > + VIRTIO_GPU_FILL_CMD(ublob); > + virtio_gpu_unmap_blob_bswap(&ublob); > + > + if (ublob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, ublob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, ublob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + virtio_gpu_virgl_resource_unmap(g, res); > +} > + > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > + > void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > case VIRTIO_GPU_CMD_GET_EDID: > virtio_gpu_get_edid(g, cmd); > break; > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: > + virgl_cmd_resource_create_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: > + virgl_cmd_resource_map_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > + virgl_cmd_resource_unmap_blob(g, cmd); > + break; > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > default: > cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > break; > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 5b7a7eab4f..cc4c1f81bb 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > return; > } > > +#ifndef HAVE_VIRGL_RESOURCE_BLOB > if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { > - error_setg(errp, "blobs and virgl are not compatible (yet)"); > + error_setg(errp, "Linked virglrenderer does not support blob resources"); > return; > } > +#endif > } > > if (!virtio_gpu_base_device_realize(qdev, > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > index 55973e112f..b9adc28071 100644 > --- a/include/hw/virtio/virtio-gpu.h > +++ b/include/hw/virtio/virtio-gpu.h > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { > int dmabuf_fd; > uint8_t *remapped; > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + /* only blob resource needs this region to be mapped as guest mmio */ > + MemoryRegion *region; > +#endif > + Since this is specific to virgl, let's create a struct that embeds virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu common code.