When using a virtual desktop any amount of scrolling would cause
ati_set_dirty to set the wrong region of memory as dirty and screen
updates are missed. This is because the previous implementation
used vga->vbe_start_addr in the dirty address calculation. It
works when it's zero but as soon as the virtual desktop is involved
it's often non-zero.
The first issue is that vga->vbe_start_addr is a word offset and was
being added to byte offsets. But regardless, the vbe_start_address affects
the scan out and not the framebuffer. We shouldn't need to apply an
offset for it at all when thinking about dirty tracking.
The second is that a bounds check was also being performed to ensure
that the dirty memory was within the visible screen. With the virtual
desktop this isn't actually what we want. Updates can and will happen
outside of the visible screen area. Those should be marked dirty.
Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---
hw/display/ati_2d.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 504d1c5708..dc33b014eb 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -71,17 +71,11 @@ static void ati_set_dirty(VGACommonState *vga, const ATI2DCtx *ctx)
DisplaySurface *ds = qemu_console_surface(vga->con);
(void)ds;
- DPRINTF("%p %u ds: %p %d %d rop: %x\n", vga->vram_ptr, vga->vbe_start_addr,
- surface_data(ds), surface_stride(ds), surface_bits_per_pixel(ds),
- ctx->rop3 >> 16);
- if (ctx->dst_bits >= vga->vram_ptr + vga->vbe_start_addr &&
- ctx->dst_bits < vga->vram_ptr + vga->vbe_start_addr +
- vga->vbe_regs[VBE_DISPI_INDEX_YRES] * vga->vbe_line_offset) {
- memory_region_set_dirty(&vga->vram,
- vga->vbe_start_addr + ctx->dst_offset +
- ctx->dst.y * ctx->dst_stride,
- ctx->dst.height * ctx->dst_stride);
- }
+ DPRINTF("%p ds: %p %d %d rop: %x\n", vga->vram_ptr, surface_data(ds),
+ surface_stride(ds), surface_bits_per_pixel(ds), ctx->rop3 >> 16);
+ memory_region_set_dirty(&vga->vram,
+ ctx->dst_offset + ctx->dst.y * ctx->dst_stride,
+ ctx->dst.height * ctx->dst_stride);
}
static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
--
2.53.0On Fri, 17 Apr 2026, Chad Jablonski wrote:
> When using a virtual desktop any amount of scrolling would cause
> ati_set_dirty to set the wrong region of memory as dirty and screen
> updates are missed. This is because the previous implementation
I guess this is to fix chars not appearing in xterm when a virtual desktop
is panned that you noticed here?
https://lists.nongnu.org/archive/html/qemu-devel/2026-04/msg00785.html
> used vga->vbe_start_addr in the dirty address calculation. It
> works when it's zero but as soon as the virtual desktop is involved
> it's often non-zero.
>
> The first issue is that vga->vbe_start_addr is a word offset and was
> being added to byte offsets. But regardless, the vbe_start_address affects
> the scan out and not the framebuffer. We shouldn't need to apply an
> offset for it at all when thinking about dirty tracking.
Obviously I don't quite understood how dirty tracking should work.
> The second is that a bounds check was also being performed to ensure
> that the dirty memory was within the visible screen. With the virtual
> desktop this isn't actually what we want. Updates can and will happen
> outside of the visible screen area. Those should be marked dirty.
But I still think only the visible area should need marking dirty because it
is used to update display so marking non visible parts should have no
effect. I'm not sure if it has any performance impact but just seems
unnecessary so that's why this tried to limit it to visible area. I've
added to cc some people who could maybe enlighten us on this.
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> ---
> hw/display/ati_2d.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 504d1c5708..dc33b014eb 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -71,17 +71,11 @@ static void ati_set_dirty(VGACommonState *vga, const ATI2DCtx *ctx)
> DisplaySurface *ds = qemu_console_surface(vga->con);
>
> (void)ds;
> - DPRINTF("%p %u ds: %p %d %d rop: %x\n", vga->vram_ptr, vga->vbe_start_addr,
> - surface_data(ds), surface_stride(ds), surface_bits_per_pixel(ds),
> - ctx->rop3 >> 16);
> - if (ctx->dst_bits >= vga->vram_ptr + vga->vbe_start_addr &&
> - ctx->dst_bits < vga->vram_ptr + vga->vbe_start_addr +
> - vga->vbe_regs[VBE_DISPI_INDEX_YRES] * vga->vbe_line_offset) {
> - memory_region_set_dirty(&vga->vram,
> - vga->vbe_start_addr + ctx->dst_offset +
> - ctx->dst.y * ctx->dst_stride,
> - ctx->dst.height * ctx->dst_stride);
> - }
> + DPRINTF("%p ds: %p %d %d rop: %x\n", vga->vram_ptr, surface_data(ds),
> + surface_stride(ds), surface_bits_per_pixel(ds), ctx->rop3 >> 16);
> + memory_region_set_dirty(&vga->vram,
> + ctx->dst_offset + ctx->dst.y * ctx->dst_stride,
> + ctx->dst.height * ctx->dst_stride);
> }
If this is how it should be done and limiting dirty tracking to visible
part does not save any work I wonder if we still need this funcion and
couldn't just use memory_region_set_dirty directly? I'd like to avoid
slowing updates down with unnecessarily marking off screen areas but I'm
not sure that concern is valid.
Regards,
BALATON Zoltan
> > I guess this is to fix chars not appearing in xterm when a virtual desktop > is panned that you noticed here? > https://lists.nongnu.org/archive/html/qemu-devel/2026-04/msg00785.html > Yep! I should have mentioned this. >> The second is that a bounds check was also being performed to ensure >> that the dirty memory was within the visible screen. With the virtual >> desktop this isn't actually what we want. Updates can and will happen >> outside of the visible screen area. Those should be marked dirty. > > But I still think only the visible area should need marking dirty because it > is used to update display so marking non visible parts should have no > effect. I'm not sure if it has any performance impact but just seems > unnecessary so that's why this tried to limit it to visible area. I've > added to cc some people who could maybe enlighten us on this. > After digging into this deeper I think you're right that the non-visible dirty marking isn't necessary. Looking at vga_draw_graphic in hw/display/vga.c it looks like anytime the screen pans, a full screen update is triggered. But from what I can tell the full screen update doesn't clear the dirty map. Only partial updates do that. So setting non-visible regions dirty could cause the non-visible regions to be updated later when it isn't necessary. Meaning the user could pan over the region, do an unrelated blit and the stale dirty regions would also be updated. I'm not sure what sort of perfomance implications this has though. I think if we want to avoid this the way to do it may be to just take the intersection of the currently visible screen area and the destination area and mark just that as dirty. Even fixing the unit issue the old bounds check would miss blits that began outside of the visible screen and continued onto the visible screen. Another thought, we could clear dirty bits on full screen updates. But that's a change with a much larger blast radius and I would be nervous to make it.
On Sat, 18 Apr 2026, Chad Jablonski wrote: >> I guess this is to fix chars not appearing in xterm when a virtual desktop >> is panned that you noticed here? >> https://lists.nongnu.org/archive/html/qemu-devel/2026-04/msg00785.html >> > > Yep! I should have mentioned this. > >>> The second is that a bounds check was also being performed to ensure >>> that the dirty memory was within the visible screen. With the virtual >>> desktop this isn't actually what we want. Updates can and will happen >>> outside of the visible screen area. Those should be marked dirty. >> >> But I still think only the visible area should need marking dirty because it >> is used to update display so marking non visible parts should have no >> effect. I'm not sure if it has any performance impact but just seems >> unnecessary so that's why this tried to limit it to visible area. I've >> added to cc some people who could maybe enlighten us on this. >> > > After digging into this deeper I think you're right that the > non-visible dirty marking isn't necessary. Looking at vga_draw_graphic I've found a case which this patch seems to fix and does not involve panning (unfortunately I can't give a reproducer as it is with AmigaOS that isn't free) but I don't understand why and what exactly happens. Maybe updates are missed when guest updates display by directly writing to VRAM? But such writes should cause region to become dirty due to vga_dirty_log_start() called at the end of vga_common_init() which we call from ati_vga_realize(). Without this patch after a mode change display is only updated around the mouse where it's explicitely marked dirty (with guest_hwcursor=on as guest_hwcursor=off is bugged) but with this patch updates correctly but I don't know if it's just a side effect or actual fix for something. Maybe it's just the calculation fix and not the off screen dirty marking part, I haven't try to test it with restricting to visible screen (maybe I wait for v2 of the patch). Regards, BALATON Zoltan
On Sat, 18 Apr 2026, Chad Jablonski wrote: >> I guess this is to fix chars not appearing in xterm when a virtual desktop >> is panned that you noticed here? >> https://lists.nongnu.org/archive/html/qemu-devel/2026-04/msg00785.html >> > > Yep! I should have mentioned this. > >>> The second is that a bounds check was also being performed to ensure >>> that the dirty memory was within the visible screen. With the virtual >>> desktop this isn't actually what we want. Updates can and will happen >>> outside of the visible screen area. Those should be marked dirty. >> >> But I still think only the visible area should need marking dirty because it >> is used to update display so marking non visible parts should have no >> effect. I'm not sure if it has any performance impact but just seems >> unnecessary so that's why this tried to limit it to visible area. I've >> added to cc some people who could maybe enlighten us on this. >> > > After digging into this deeper I think you're right that the > non-visible dirty marking isn't necessary. Looking at vga_draw_graphic > in hw/display/vga.c it looks like anytime the screen pans, a full screen > update is triggered. But from what I can tell the full screen update doesn't > clear the dirty map. Only partial updates do that. So setting non-visible > regions dirty could cause the non-visible regions to be updated later > when it isn't necessary. Meaning the user could pan over the region, do > an unrelated blit and the stale dirty regions would also be updated. I'm > not sure what sort of perfomance implications this has though. > > I think if we want to avoid this the way to do it may be to just take > the intersection of the currently visible screen area and the > destination area and mark just that as dirty. That could work but maybe we also have problems with calculating intersects. The ATI docs say SC values can be negative (Range -8192 to 8191) but we don't seem to handle that correctly. I don't know if any drivers use negative values in scissor clipping, I've only noticed it when checking with different values so maybe that's not be a problem in common cases. > Even fixing the unit issue > the old bounds check would miss blits that began outside of the visible > screen and continued onto the visible screen. Maybe this is the real reason but I still don't see how a relatively small blit of one character in a fully visible xterm not even near the left of screen could be missed this way if the screen is panned so I'm still suspecting clipping might have something to do with this. Not marking out of screen updates also does not explain that so it's probably just wrong calculation of visible area. Or you meant that's just another way the current calculation could miss updates. Yes that's possible. > Another thought, we could clear dirty bits on full screen updates. But > that's a change with a much larger blast radius and I would be nervous to > make it. Can we invalidate dirty rect in ati_vga_set_offset to avoid changing vga? Regards, BALATON Zoltan
© 2016 - 2026 Red Hat, Inc.