From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id A6796986797 for ; Sat, 25 Jun 2022 13:45:16 +0000 (UTC) From: David Hoppenbrouwers References: Message-ID: Date: Sat, 25 Jun 2022 15:45:46 +0200 MIME-Version: 1.0 In-Reply-To: Subject: [virtio-dev] virtio-gpu: Clarification regarding VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: virtio-dev@lists.oasis-open.org List-ID: 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