* [virtio-dev] virtio-gpu: Clarification regarding VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D [not found] <bcfa29cc-72bc-a2ef-0f7b-a882cada66b0@demindiro.com> @ 2022-06-25 13:45 ` David Hoppenbrouwers 2022-06-26 6:43 ` Michael S. Tsirkin 0 siblings, 1 reply; 2+ messages in thread From: David Hoppenbrouwers @ 2022-06-25 13:45 UTC (permalink / raw) To: virtio-dev 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 --------------------------------------------------------------------- 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
* 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
end of thread, other threads:[~2022-06-26 6:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <bcfa29cc-72bc-a2ef-0f7b-a882cada66b0@demindiro.com>
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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.