From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbYHP-0001tM-Bq for qemu-devel@nongnu.org; Mon, 14 Sep 2015 14:15:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbYHM-0007Zk-LK for qemu-devel@nongnu.org; Mon, 14 Sep 2015 14:15:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52479) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbYHM-0007ZL-DN for qemu-devel@nongnu.org; Mon, 14 Sep 2015 14:15:00 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 1605E3B3C7 for ; Mon, 14 Sep 2015 18:14:58 +0000 (UTC) References: <1441797654-15350-1-git-send-email-kraxel@redhat.com> <1441797654-15350-6-git-send-email-kraxel@redhat.com> From: Max Reitz Message-ID: <55F70E9E.3020603@redhat.com> Date: Mon, 14 Sep 2015 20:14:54 +0200 MIME-Version: 1.0 In-Reply-To: <1441797654-15350-6-git-send-email-kraxel@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7LlWC0k1Id1aHR3WfRqsalMatF4t3fANk" Subject: Re: [Qemu-devel] [PATCH 5/9] virtio-gpu: add 3d mode and virgl rendering support. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --7LlWC0k1Id1aHR3WfRqsalMatF4t3fANk Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable 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. >=20 > Signed-off-by: Gerd Hoffmann > --- > 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_comma= nd *cmd) > +{ > + struct virtio_gpu_resource_detach_backing detach_rb; > + struct iovec *res_iovs =3D NULL; > + int num_iovs =3D 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 =3D=3D NULL || num_iovs =3D=3D 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)); > } > =20 > +#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 =3D virgl_renderer_get_cursor_data(resource_id, &width, &heig= ht); > + if (!data) { > + return; > + } > + > + pixels =3D 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_curso= r *cursor) > { > struct virtio_gpu_scanout *s; [snip] > @@ -92,9 +131,23 @@ static void virtio_gpu_set_config(VirtIODevice *vde= v, const uint8_t *config) > static uint64_t virtio_gpu_get_features(VirtIODevice *vdev, uint64_t f= eatures, > Error **errp) > { > + VirtIOGPU *g =3D VIRTIO_GPU(vdev); > + > + if (virtio_gpu_virgl_enabled(g->conf)) { > + features |=3D (1 << VIRTIO_GPU_FEATURE_VIRGL); > + } > return features; > } > =20 > +static void virtio_gpu_set_features(VirtIODevice *vdev, uint64_t featu= res) > +{ > + static const uint32_t virgl =3D (1 << VIRTIO_GPU_FEATURE_VIRGL); > + VirtIOGPU *g =3D VIRTIO_GPU(vdev); > + > + g->use_virgl_renderer =3D ((features & virgl) =3D=3D 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 --7LlWC0k1Id1aHR3WfRqsalMatF4t3fANk Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJV9w6eAAoJEDuxQgLoOKyt+R4IAI2bJQrevTiufLDQ2utxva7I 3nRFTgjqBOuL0N+CLdsN5l9MGkHeX1VGwJXUgQjY7TzyHsmbomhlce8iisFH9L3I vdKRCE0UEBYTLOi+4HpUCA0PFVCWjGcLoSrFy0xjqwrS7shqbQyw99BY2GTCpzGS L0d76/iIHmN/G96lNatD23M5O5prwdGWyrHrZ6gkiTOktDLsBv45phj8cS6F7q4U 22AxhCAvMQATKTcVUQRbTb6Yp6CPshyMd/9PJXOxuTkxhwHZo0yNNH6RpAo+UWJ3 TDFeVtsIMlutMilmqDbxVYrhl7kJgBGPZ+79j2k2UhjJXFa3m1P2JoIXUf4G/8A= =jGYt -----END PGP SIGNATURE----- --7LlWC0k1Id1aHR3WfRqsalMatF4t3fANk--