* Re: [virtio-dev] virtio-gpu: Clarification regarding VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D
2022-06-25 13:45 ` [virtio-dev] virtio-gpu: Clarification regarding VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D David Hoppenbrouwers
@ 2022-06-26 6:43 ` Michael S. Tsirkin
0 siblings, 0 replies; 2+ messages in thread
From: Michael S. Tsirkin @ 2022-06-26 6:43 UTC (permalink / raw)
To: David Hoppenbrouwers; +Cc: virtio-dev, Gerd Hoffmann
On Sat, Jun 25, 2022 at 03:45:46PM +0200, David Hoppenbrouwers wrote:
> Hello,
>
> I believe there is a bug in QEMU's implementation of
> VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D. However it is not clear to me what
> the correct behaviour should be.
>
> The specification says the driver must "allocate a framebuffer from
> guest ram, and attach it as backing storage to the resource just
> created". To me this implies that the size of the backing storage must
> match the size of the resource.
>
> Consequently I intuitively expect that to update a rectangle R in the
> resource the same rectangle must be updated in the backing store, i.e:
>
> +--------------------+ +--------------------+
> | | | |
> | Host | | Backing |
> | Resource | | Storage |
> | +-----+ | | +-----+ |
> | | | | | | | |
> | | R | | | | R | |
> | | | | | | | |
> | +-----+ | | +-----+ |
> +--------------------+ +--------------------+
>
> However, QEMU actually starts drawing from the upper-left corner of the
> backing storage:
>
> +--------------------+ +-----+--------------+
> | | | | |
> | Host | | R | Backing |
> | Resource | | | Storage |
> | +-----+ | +-----+ |
> | | | | | |
> | | R | | | |
> | | | | | |
> | +-----+ | | |
> +--------------------+ +--------------------+
>
> Reading the specification again I realized it does not explicitly
> mandate that the size of the backing storage must match the size of the
> resource. But if that were the case I would expect QEMU to behave like this:
>
> +--------------------+ +--------------------+
> | | | R +-------+
> | Host | +------------+ |
> | Resource | | |
> | +-----+ | | Backing |
> | | | | | Storage |
> | | R | | | |
> | | | | | |
> | +-----+ | | |
> +--------------------+ +--------------------+
>
> The relevant code in QEMU is in virtio_gpu_transfer_to_host_2d:
>
> 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);
>
> I would expect src_offset to either be
>
> src_offset = dst_offset;
>
> if the backing storage's size is supposed to match the resource's size.
> If not, I'd expect src_offset to be
>
> src_offset = t2d.offset + t2d.r.width * h;
>
>
> My question boils down to:
>
> - Is QEMU's implementation correct?
> - If yes, what is the motivation behind it? Especially, what is the
> purpose of the explicit offset?
> - If not, what is the expected behaviour?
>
>
> With kind regards,
> David Hoppenbrouwers
+cc Gerd.
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 2+ messages in thread