Re: [Qemu-devel] [RfC PATCH 06/15] virtio-gpu/2d: add virtio gpu core code

[Max Reitz]

On 2015-02-27 at 06:10, Gerd Hoffmann wrote:
> 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<address@hidden>
> > > Signed-off-by: Gerd Hoffmann<address@hidden>
> > > ---
> > >    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.

OK.

[snip]

> > > +    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.

Exactly, but it looks like to me that it doesn't (at least not always).

Max