virtio-gpu: backing store stride unspecified?

[Demindiro]

Hello,

There is a behaviour with the QEMU virtio-gpu device that seems wrong to me: When copying data from the guest's backing store to the host's buffers it does account for the rectangle offset and size when drawing but it uses the stride (width) of the host's resource when calculating offsets in the backing store regardless of the width specified in the rectangle:

Code: Select all

static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
                                           struct virtio_gpu_ctrl_command *cmd)
{
    struct virtio_gpu_simple_resource *res;
    int h;
    uint32_t src_offset, dst_offset, stride;
    int bpp;
    pixman_format_code_t format;
    struct virtio_gpu_transfer_to_host_2d t2d;

    VIRTIO_GPU_FILL_CMD(t2d);
    virtio_gpu_t2d_bswap(&t2d);
    trace_virtio_gpu_cmd_res_xfer_toh_2d(t2d.resource_id);

    res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
                                         __func__, &cmd->error);
    if (!res || res->blob) {
        return;
    }

    if (t2d.r.x > res->width ||
        t2d.r.y > res->height ||
        t2d.r.width > res->width ||
        t2d.r.height > res->height ||
        t2d.r.x + t2d.r.width > res->width ||
        t2d.r.y + t2d.r.height > res->height) {
        qemu_log_mask(LOG_GUEST_ERROR, "%s: transfer bounds outside resource"
                      " bounds for resource %d: %d %d %d %d vs %d %d\n",
                      __func__, t2d.resource_id, t2d.r.x, t2d.r.y,
                      t2d.r.width, t2d.r.height, res->width, res->height);
        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
        return;
    }

    format = pixman_image_get_format(res->image);
    bpp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
    stride = pixman_image_get_stride(res->image);

    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));
    }
}

I have a strong feeling that

Code: Select all

src_offset = t2d.offset + stride * h;

should be

Code: Select all

src_offset = t2d.offset + t2d.r.width * h;

instead.

However, I'm unable to confirm as the latest specification doesn't seem to mention anything, I cannot find any other emulators implementing a virtio-gpu device and every virtio-gpu driver I find seems to transfer & flush the entire framebuffer (except Linux perhaps, but I'm still trying to interpret its driver).

I'm hoping someone here can confirm whether QEMU's behaviour is correct or not. If not, I'll ping the QEMU devs.

[Octocontrabass]

QEMU must be allocating its host buffers in the same format as the guest buffers. Since this command copies between two buffers of the same dimensions, the stride only needs to be calculated once.

It does look odd, though.

[Demindiro]

QEMU must be allocating its host buffers in the same format as the guest buffers.

The format only refers to the pixel format, not dimensions AFAICT (emphasis mine):

VIRTIO_GPU_CMD_RESOURCE_CREATE_2D ...

Code: Select all

enum virtio_gpu_formats {
    VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM = 1,
    VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM = 2,
...

This creates a 2D resource on the host with the specified width, height and format. ...

Since this command copies between two buffers of the same dimensions

I also thought the backing store had to have the same dimensions but the specification does not actually say this (emphasis mine):

5.7.6.1
Device Operation: Create a framebuffer and configure scanout
• Create a host resource using VIRTIO_GPU_CMD_RESOURCE_CREATE_2D.
• Allocate a framebuffer from guest ram, and attach it as backing storage to the resource just created, us-
ing VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING. Scatter lists are supported, so the frame-
buffer doesn’t need to be contignous in guest physical memory.
• Use VIRTIO_GPU_CMD_SET_SCANOUT to link the framebuffer to a display scanout.

This does give the impression the backing store needs to match the dimensions of the host resource. However, it does not actually explicitly say that.

Also, if the dimensions are indeed expected to match I'd expect to find `src_offset = dst_offset;` instead of `src_offset = t2d.offset + stride * h`, since the latter means I always need to start drawing in the backing store from the top-left corner (0,0) instead of whatever X,Y coordinates the rectangle specifies. You also wouldn't need `t2d.offset` if the former was used.

In short, I do not see any reason to use a separate `src_offset` if the backing store's size is expected to match the host's resource size

(In hindsight, I could calculate the t2d.offset such that I can start drawing in the backing store from X,Y instead of 0,0, though that seems rather pointless to me compared to the device doing just that from the get-go).

VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING
...
This assign an array of guest pages as the backing store for a resource. These pages are then used
for the transfer operations for that resource from that point on.

This does not explicitly mention the backing store needs to match the size of the host's resource either.

VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D
...
This takes a resource id along with an destination offset into the resource, and a box to transfer to the
host backing for the resource.

I interpret the last part as "the size of the backing store matches the area of the box (= rectangle, presumably)", given that an offset also needs to be specified explictly.

VIRTIO_GPU_CMD_SET_SCANOUT ...

This sets the scanout parameters for a single scanout. The resource_id is the resource to be scanned
out from, along with a rectangle.

Scanout rectangles must be completely covered by the underlying resource. ...

While this essentially says that the resource's size must be equal or larger than the size of the scanout it does not mention the backing store at all.

[Octocontrabass]

The format only refers to the pixel format, not dimensions AFAICT (emphasis mine):

Right. Since the host and guest buffers are always in the same format, the format will never be responsible for a difference in stride. That leaves only the dimensions.

I also thought the backing store had to have the same dimensions but the specification does not actually say this (emphasis mine):

It's strongly implied that the host and guest buffers have the same dimensions by the fact that the guest can't specify the dimensions of its buffer when it assigns a backing store to a resource. Maybe it's time to contact the spec authors?

Also, if the dimensions are indeed expected to match I'd expect to find `src_offset = dst_offset;` instead of `src_offset = t2d.offset + stride * h`, since the latter means I always need to start drawing in the backing store from the top-left corner (0,0) instead of whatever X,Y coordinates the rectangle specifies. You also wouldn't need `t2d.offset` if the former was used.

In short, I do not see any reason to use a separate `src_offset` if the backing store's size is expected to match the host's resource size

(In hindsight, I could calculate the t2d.offset such that I can start drawing in the backing store from X,Y instead of 0,0, though that seems rather pointless to me compared to the device doing just that from the get-go).

I'm not sure why you would want to do it this way either. Being able to specify different coordinates for the source and destination rectangles doesn't seem very useful when the source and destination buffers are the same size.

[Demindiro]

I've sent an email to the virtio-dev mailing list. We just have to wait for a response now

[linuxyne]

The rectangle's width is the width of the update region, while stride is the pitch (length of one entire row) of the image. To go from one row to the next in the image, one needs to multiply by stride. The width of the rectangle is considered when QEMU wants to know how long the row to be updated is.

The offset points to the upper left corner of the rectangle in the image buffer. To go to the next row in the buffer, offset + stride is the correct formula. To go to the second row, offset + stride * 2, etc.

The rectangle's width has no bearing on stride, except it should remain witin the limits established by the framebuffer/image dimensions (height and stride).

How did you calculate the offset? You can check virtio_gpu_update_dumb_bo in Linux to see how it calculates the offset.

[Demindiro]

How did you calculate the offset? You can check virtio_gpu_update_dumb_bo in Linux to see how it calculates the offset.

I initially set it to 0, though I later realized I could calculate it such that it starts at whatever X,Y. It seems that Linux does this.

What I don't understand is why an explicit offset is necessary if QEMU can easily do `src_offset = dst_offset`. This would both be simpler and more efficient. The only reason I can think of is that the offset has a different purpose that I don't understand.

[linuxyne]

I initially set it to 0

I did suspect as much.

What I don't understand is why an explicit offset is necessary if QEMU can easily do `src_offset = dst_offset`

The problem doesn't seem to be with QEMU; the virtio-gpu spec requires you to provide two different pieces of information: (1) The upper-left corner of the update region, inside the guest framebuffer, through the offset parameter. (2) The upper-left corner of the update region, in the host buffer, through its pixel coordinates.

This can be seen from how the QEMU utilizes the rectangle's x/y coordinates in determining the offset into destination (host) buffer, and the offset parameter in determining the offset into the src (guest) buffer.

It does seem redudant to provide both pieces of information, though could there be cases (such as screen mirroring) where the src_offset and dst_offset may differ?

Edit: Screen mirroring doesn't seem very convincing; there doesn't seem to be a problem with the offsets remaining the same.

Edit2: Perhaps to facilitate double buffering?

Edit3: It might also have to do with Mesa. The VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D command is also sent when Mesa requests the DRM_IOCTL_VIRTGPU_TRANSFER_TO_HOST, perhaps in order to satisfy blitting/copying requests and such.