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

[Max Reitz <mreitz@redhat.com>]

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<airlied@redhat.com>
>>> Signed-off-by: Gerd Hoffmann<kraxel@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.

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