On Do, 2015-02-26 at 11:08 -0500, Max Reitz wrote:
> On 2015-02-23 at 05:23, Gerd Hoffmann wrote:
> > This patch adds the core code for virtio gpu emulation,
> > covering 2d support.
> >
> > Written by Dave Airlie and Gerd Hoffmann.
> >
> > Signed-off-by: Dave Airlie<airl...@redhat.com>
> > Signed-off-by: Gerd Hoffmann<kra...@redhat.com>
> > ---
> > hw/display/Makefile.objs | 2 +
> > hw/display/virtio-gpu.c | 903
> > +++++++++++++++++++++++++++++++++++++++++
> > include/hw/virtio/virtio-gpu.h | 147 +++++++
> > trace-events | 14 +
> > 4 files changed, 1066 insertions(+)
> > create mode 100644 hw/display/virtio-gpu.c
> > create mode 100644 include/hw/virtio/virtio-gpu.h
>
> Again I mostly only have formal complaints...
>
> But one non-formal question: As far as I understood virtio-gpu's mode of
> operation from this patch, it looks like there is one resource per
> scanout, and that resource is basically the whole screen (which can be
> updated partially).
This is correct (for 2d mode, 3d will be different).
> If that is the case, what do we gain from being able to display a
> resource on multiple scanouts? If we don't associate a scanout to a
> resource with set_scanout, the resource won't ever be displayed on that
> scanout; and if we associate it, the scanout's position and dimension
> will be exactly the same as the resource's, so associating a resource
> with multiple scanouts means that all those scanouts will be duplicates
> of each other, which in turn means we can duplicate heads. But that
> seems more like a frontend feature to me...
It's handled this way to mimic behavior of physical hardware, where you
can configure your scanouts (monitor plugs of gfx hardware) in a simliar
way.
Main advantage of taking this route is that virtual hardware and
physical hardware can be configued the same way, i.e. you can -- for
example -- setup screen mirroring with xrandr.
> > +static void update_cursor_data_simple(VirtIOGPU *g,
> > + struct virtio_gpu_scanout *s,
> > + uint32_t resource_id)
> > +{
> > + struct virtio_gpu_simple_resource *res;
> > + uint32_t pixels;
> > +
> > + res = virtio_gpu_find_resource(g, resource_id);
> > + if (!res) {
> > + return;
> > + }
> > +
> > + pixels = s->current_cursor->width * s->current_cursor->height;
>
> This multiplication might overflow (but I can't imagine that one could
> use this for an exploit).
Added checks.
> > +static struct virtio_gpu_simple_resource *
> > +virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
> > +{
> > + struct virtio_gpu_simple_resource *res;
> > +
> > + QTAILQ_FOREACH(res, &g->reslist, next) {
> > + if (res->resource_id == resource_id) {
> > + return res;
> > + }
> > + }
> > + return NULL;
> > +}
>
> How often do you think this function is called, and how many resources
> do you expect there to be? Would it make sense to make this a hash table?
Not for 2d, it's only scanouts and cursors.
In 3d mode virglrenderer will manage resources, and there will be alot
more (textures, buffers, ....).
> > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > + return;
> > + }
> > +
> > + res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +
> > + res->width = c2d.width;
> > + res->height = c2d.height;
> > + res->format = c2d.format;
> > + res->resource_id = c2d.resource_id;
>
> Considering resource_id == 0 is sometimes (apparently) used as "no
> resource" (e.g. in transfer_to_host_2d), would it make sense to test
> against that here?
Added check.
> > + format = pixman_image_get_format(res->image);
> > + bpp = (PIXMAN_FORMAT_BPP(format) + 7) / 8;
>
> Could have used DIV_ROUND_UP(); also, I don't find it anywhere in the
> virtio-gpu spec (at least not in the one spec I found), so will this
> really work if PIXMAN_FORMAT_BPP(format) is not a multiple of 8? I can
> imagine this being correct for *_BPP() == 15, but maybe not so much for
> *_BPP() == 1; but then again, maybe you don't plan on supporting
> sub-byte pixel formats anyway.
Yes, it's for bpp=15.
> > + if (t2d.offset || t2d.r.x || t2d.r.y ||
> > + t2d.r.width != pixman_image_get_width(res->image)) {
> > + void *img_data = pixman_image_get_data(res->image);
> > + for (h = 0; h < t2d.r.height; h++) {
> > + src_offset = t2d.offset + stride * h;
> > + dst_offset = (t2d.r.y + h) * stride + (t2d.r.x * bpp);
> > +
> > + iov_to_buf(res->iov, res->iov_cnt, src_offset,
> > + (uint8_t *)img_data
> > + + dst_offset, t2d.r.width * bpp);
> > + }
> > + } else {
> > + iov_to_buf(res->iov, res->iov_cnt, 0,
> > + pixman_image_get_data(res->image),
> > + pixman_image_get_stride(res->image)
> > + * pixman_image_get_height(res->image));
>
> Will this work with stride != t2d.r.width * bpp?
Those cases should take the if branch above and loop over all lines to
handle it correctly.
> > + res = virtio_gpu_find_resource(g, ss.resource_id);
>
> If ss.resource_id == 0, the result is unused; this function call could
> therefore be put after the next if block.
Done.
> > + g->enable = 1;
> > + if (ss.resource_id == 0) {
> > + scanout = &g->scanout[ss.scanout_id];
> > + if (g->scanout[ss.scanout_id].resource_id) {
>
> Would be shorter as "if (scanout->resource_id)".
Yep, fixed.
> > +int virtio_gpu_create_mapping_iov(struct
> > virtio_gpu_resource_attach_backing *ab,
> > + struct virtio_gpu_ctrl_command *cmd,
> > + struct iovec **iov)
> > +{
> > + struct virtio_gpu_mem_entry *ents;
> > + size_t esize, s;
> > + int i;
> > +
> > + esize = sizeof(*ents) * ab->nr_entries;
>
> Can overflow (on 32 bit hosts).
[ ... ]
> However, I think it's best to simply limit ab->nr_entries to some sane
> value
Done.
thanks,
Gerd
