[{"id":3685298,"web_url":"http://patchwork.ozlabs.org/comment/3685298/","msgid":"<98ad9ff8-7227-fc11-a221-22fc73f75f47@eik.bme.hu>","list_archive_url":null,"date":"2026-05-02T14:01:58","subject":"Re: [PATCH v2] ati-vga: fix ati_set_dirty address calculation","submitter":{"id":16148,"url":"http://patchwork.ozlabs.org/api/people/16148/","name":"BALATON Zoltan","email":"balaton@eik.bme.hu"},"content":"On Wed, 29 Apr 2026, Chad Jablonski wrote:\n> This fixes three bugs with the ati_set_dirty address calculation.\n\nGood catch, thanks for finding this.\n\n> First, vbe_start_addr is a word offset. All other values in the\n> calculation are byte offsets. It must be converted to bytes.\n>\n> Second, when setting the dirty region with memory_region_set_dirty\n> the vbe_start_addr is used to calculate the start of the dirty region.\n> This is a problem because the vbe_start_addr is the offset at which scan out\n> begins. This puts it in the visible screen coordinate system. The dirty\n> region however is in the virtual screen coordinate system. This can cause both\n> overmarking and missed updates. This is removed from the calculation.\n>\n> Third, when the start address of a blit is outside of the bounds check\n> the entire blit is missed and not set to dirty. This happens even if the\n> blit does partially overlap with the visible screen. The fix here is to\n> find the intersection of the visible screen and the blit and mark only\n> that region as dirty.\n>\n> This does not attempt to apply clipping to the blit. So there will be\n> overmarking in some cases.\n>\n> Signed-off-by: Chad Jablonski \n> ---\n> hw/display/ati_2d.c | 26 +++++++++++++++++++-------\n> 1 file changed, 19 insertions(+), 7 deletions(-)\n>\n> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c\n> index 504d1c5708..3192b864fd 100644\n> --- a/hw/display/ati_2d.c\n> +++ b/hw/display/ati_2d.c\n> @@ -69,19 +69,31 @@ typedef struct {\n> static void ati_set_dirty(VGACommonState *vga, const ATI2DCtx *ctx)\n> {\n> DisplaySurface *ds = qemu_console_surface(vga->con);\n> + unsigned int bypp = ctx->bpp / 8;\n> + unsigned long dirty_size = ((ctx->dst.height - 1) * ctx->dst_stride) +\n> + (ctx->dst.width * bypp);\n\ndirty_size is only used once below for dirty_end so maybe it does not need \na separate variable just do the calculation on one line similar to \nvis_end and inline this in dirty_end.\n\n> + uint8_t *dirty_start = ctx->dst_bits + (ctx->dst.y * ctx->dst_stride) +\n> + (ctx->dst.x * bypp);\n> + uint8_t *dirty_end = dirty_start + dirty_size;\n> + /*\n> + * The blit may be outside of the visible screen (e.g. virtual desktops.)\n> + * Dirty only the intersection of the visible screen and the blit.\n> + */\n> + uint8_t *vis_start = vga->vram_ptr + (vga->vbe_start_addr * 4);\n\nThere are excessive parenthesis in above formulas, these are not needed \naround multiplications. I see the intent but it's not usually done in \nQEMU so these could be dropped.\n\nInstead of adding vga->vram_ptr here and then substracting it in \nmemory_region_set_dirty below again maybe keep these offsets and substract \nvga->vram_ptr from ctx->dst_bits in dirty_start once instead.\n\n> + uint8_t *vis_end = vis_start + vga->vbe_regs[VBE_DISPI_INDEX_YRES] *\n> + vga->vbe_line_offset;\n> + uint8_t *start = MAX(vis_start, dirty_start);\n> + uint8_t *end = MIN(vis_end, dirty_end);\n>\n> (void)ds;\n> DPRINTF(\"%p %u ds: %p %d %d rop: %x\\n\", vga->vram_ptr, vga->vbe_start_addr,\n> surface_data(ds), surface_stride(ds), surface_bits_per_pixel(ds),\n> ctx->rop3 >> 16);\n> - if (ctx->dst_bits >= vga->vram_ptr + vga->vbe_start_addr &&\n> - ctx->dst_bits < vga->vram_ptr + vga->vbe_start_addr +\n> - vga->vbe_regs[VBE_DISPI_INDEX_YRES] * vga->vbe_line_offset) {\n> - memory_region_set_dirty(&vga->vram,\n> - vga->vbe_start_addr + ctx->dst_offset +\n> - ctx->dst.y * ctx->dst_stride,\n> - ctx->dst.height * ctx->dst_stride);\n> +\n> + if (start >= end) {\n> + return;\n> }\n> + memory_region_set_dirty(&vga->vram, start - vga->vram_ptr, end - start);\n\nI'm never sure about sizes and off by one errors so can't tell if that's \ncorrect but I've tested it and it still fixes my reproducer so this seems \nto work. Please also cc qemu-stable for the next version of the patch.\n\nRegards,\nBALATON Zoltan\n\n> }\n>\n> static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":"legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=nongnu.org\n (client-ip=209.51.188.17; helo=lists1p.gnu.org;\n envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n receiver=patchwork.ozlabs.org)","Received":["from lists1p.gnu.org (lists1p.gnu.org [209.51.188.17])\n\t(using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4g78kp74Cgz1yGq\n\tfor ; Sun, 03 May 2026 00:03:14 +1000 (AEST)","from localhost ([::1] helo=lists1p.gnu.org)\n\tby lists1p.gnu.org with esmtp (Exim 4.90_1)\n\t(envelope-from )\n\tid 1wJAv7-0004t3-OS; Sat, 02 May 2026 10:02:15 -0400","from eggs.gnu.org ([2001:470:142:3::10])\n by lists1p.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)\n (Exim 4.90_1) (envelope-from )\n id 1wJAv1-0004rS-61\n for qemu-devel@nongnu.org; Sat, 02 May 2026 10:02:07 -0400","from zero.eik.bme.hu ([152.66.115.2])\n by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)\n (Exim 4.90_1) (envelope-from )\n id 1wJAuy-0007rQ-K4\n for qemu-devel@nongnu.org; Sat, 02 May 2026 10:02:06 -0400","from localhost (localhost [127.0.0.1])\n by zero.eik.bme.hu (Postfix) with ESMTP id 01125596967;\n Sat, 02 May 2026 16:02:01 +0200 (CEST)","from zero.eik.bme.hu ([127.0.0.1])\n by localhost (zero.eik.bme.hu [127.0.0.1]) (amavis, port 10028) with ESMTP\n id JLWz0MBExucb; Sat, 2 May 2026 16:01:58 +0200 (CEST)","by zero.eik.bme.hu (Postfix, from userid 432)\n id C9FDE59695A; Sat, 02 May 2026 16:01:58 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n by zero.eik.bme.hu (Postfix) with ESMTP id C826C596958;\n Sat, 02 May 2026 16:01:58 +0200 (CEST)"],"X-Virus-Scanned":"amavis at eik.bme.hu","Date":"Sat, 2 May 2026 16:01:58 +0200 (CEST)","From":"BALATON Zoltan ","To":"Chad Jablonski ","cc":"qemu-devel@nongnu.org, Gerd Hoffmann ,\n\t=?iso-8859-15?q?Marc-Andr=E9_Lureau?= ","Subject":"Re: [PATCH v2] ati-vga: fix ati_set_dirty address calculation","In-Reply-To":"<20260429175434.474429-1-chad@jablonski.xyz>","Message-ID":"<98ad9ff8-7227-fc11-a221-22fc73f75f47@eik.bme.hu>","References":"<20260429175434.474429-1-chad@jablonski.xyz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII; format=flowed","Received-SPF":"pass client-ip=152.66.115.2; envelope-from=balaton@eik.bme.hu;\n helo=zero.eik.bme.hu","X-Spam_score_int":"-18","X-Spam_score":"-1.9","X-Spam_bar":"-","X-Spam_report":"(-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001,\n SPF_PASS=-0.001 autolearn=ham autolearn_force=no","X-Spam_action":"no action","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"qemu development ","List-Unsubscribe":",\n ","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org"}}]