[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] virtio-gpu: Clarification regarding VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D
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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]