qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/1] ati-vga: Fix framebuffer mapping by using hardware-correct aperture sizes
@ 2025-10-17 14:50 Chad Jablonski
  2025-10-17 14:50 ` [PATCH v3 1/1] " Chad Jablonski
  0 siblings, 1 reply; 4+ messages in thread
From: Chad Jablonski @ 2025-10-17 14:50 UTC (permalink / raw)
  To: qemu-devel; +Cc: balaton, kraxel, marcandre.lureau, Chad Jablonski

v3:
- Additional style and comment changes per BALATON
- Moved #include "qemu/units.h" to ati_int.h
- Added data to commit message for additional 32MB RV100 card
- Fixed CONFIG_APER_SIZE for R100 to return half of BAR0 (as confirmed
  by testing of the 32MB Radeon card)

v2:
- Fixed CONFIG_APER_SIZE to return half of BAR0 for Rage 128
- Left CONFIG_APER_SIZE unchanged for R100 (half of vram_size)
- Improved commit message and minor style changes

Chad Jablonski (1):
  ati-vga: Fix framebuffer mapping by using hardware-correct aperture
    sizes

 hw/display/ati.c     | 17 +++++++++++++++--
 hw/display/ati_int.h |  5 +++++
 2 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.51.0



^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v3 1/1] ati-vga: Fix framebuffer mapping by using hardware-correct aperture sizes
  2025-10-17 14:50 [PATCH v3 0/1] ati-vga: Fix framebuffer mapping by using hardware-correct aperture sizes Chad Jablonski
@ 2025-10-17 14:50 ` Chad Jablonski
  2025-10-17 15:09   ` BALATON Zoltan
  0 siblings, 1 reply; 4+ messages in thread
From: Chad Jablonski @ 2025-10-17 14:50 UTC (permalink / raw)
  To: qemu-devel; +Cc: balaton, kraxel, marcandre.lureau, Chad Jablonski

Rage 128 cards always request 64MB for their linear (framebuffer)
aperture and R100 cards always request 128MB. This is regardless
of the amount of physical VRAM on the board. The following are results
from real hardware tests:

Card                              VRAM    PCI BAR0   CONFIG_MEMSIZE  CONFIG_APER_SIZE  AGP_APER_OFFSET
-----------------------           ----    --------   --------------  ----------------  ---------------
Rage 128 Pro Ultra TF             32MB     64MB       0x02000000      0x02000000        0x02000000
Rage 128 RF/SG AGP                16MB     64MB       0x01000000      0x02000000        0x02000000
Radeon R100 QD [Radeon 7200]      64MB    128MB       0x04000000      0x04000000        N/A
Radeon RV100 QY [Radeon 7000/VE]  32MB    128MB       0x02000000      0x04000000        N/A

Previously the linear aperture (BAR0) would match the VRAM size.
This discrepancy caused issues with the X.org and XFree86 r128 drivers.
These drivers apply a mask of 0xfc000000 (2^26 = 64MB) to the linear
aperture address. If that address is not on a 64MB boundary the
framebuffer points to an incorrect memory location.

Testing shows that the Radeon R100 also has a BAR0 larger than VRAM
(128MB in this case) and the X.org radeon driver also masks to 64MB.

For Rage 128, CONFIG_APER_SIZE also differs from the previous value and
the behavior stated in the documentation. The Rage 128 register guide
states that it should contain the size of the VRAM + AGP memory. The cards
tested above show that this isn't the case. These tests also included
enabling/disabling AGP with 8MB of memory. It didn't change the
contents of CONFIG_APER_SIZE.

For both Rage 128 and R100 the CONFIG_APER_SIZE is half of the PCI BAR0 size.

Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati.c     | 17 +++++++++++++++--
 hw/display/ati_int.h |  5 +++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index f7c0006a87..54a067c243 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -361,7 +361,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
                                       PCI_BASE_ADDRESS_0, size) & 0xfffffff0;
         break;
     case CONFIG_APER_SIZE:
-        val = s->vga.vram_size / 2;
+        val = (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
+               ATI_RAGE128_LINEAR_APER_SIZE : ATI_R100_LINEAR_APER_SIZE) / 2;
         break;
     case CONFIG_REG_1_BASE:
         val = pci_default_read_config(&s->dev,
@@ -952,6 +953,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
 {
     ATIVGAState *s = ATI_VGA(dev);
     VGACommonState *vga = &s->vga;
+    uint64_t aper_size;
 
 #ifndef CONFIG_PIXMAN
     if (s->use_pixman != 0) {
@@ -1011,7 +1013,18 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     /* io space is alias to beginning of mmregs */
     memory_region_init_alias(&s->io, OBJECT(s), "ati.io", &s->mm, 0, 0x100);
 
-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &vga->vram);
+    /*
+     * The framebuffer is at the beginning of the linear aperture. For
+     * Rage128 the upper half of the aperture is reserved for an AGP
+     * window (which we do not emulate.)
+     */
+    aper_size = s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
+                ATI_RAGE128_LINEAR_APER_SIZE : ATI_R100_LINEAR_APER_SIZE;
+    memory_region_init(&s->linear_aper, OBJECT(dev), "ati-linear-aperture0",
+                       aper_size);
+    memory_region_add_subregion(&s->linear_aper, 0, &vga->vram);
+
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->linear_aper);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
     pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mm);
 
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index f5a47b82b0..708cc1dd3a 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -10,6 +10,7 @@
 #define ATI_INT_H
 
 #include "qemu/timer.h"
+#include "qemu/units.h"
 #include "hw/pci/pci_device.h"
 #include "hw/i2c/bitbang_i2c.h"
 #include "vga_int.h"
@@ -29,6 +30,9 @@
 /* Radeon RV100 (VE) */
 #define PCI_DEVICE_ID_ATI_RADEON_QY 0x5159
 
+#define ATI_RAGE128_LINEAR_APER_SIZE (64 * MiB)
+#define ATI_R100_LINEAR_APER_SIZE (128 * MiB)
+
 #define TYPE_ATI_VGA "ati-vga"
 OBJECT_DECLARE_SIMPLE_TYPE(ATIVGAState, ATI_VGA)
 
@@ -97,6 +101,7 @@ struct ATIVGAState {
     QEMUCursor *cursor;
     QEMUTimer vblank_timer;
     bitbang_i2c_interface bbi2c;
+    MemoryRegion linear_aper;
     MemoryRegion io;
     MemoryRegion mm;
     ATIVGARegs regs;
-- 
2.51.0



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 1/1] ati-vga: Fix framebuffer mapping by using hardware-correct aperture sizes
  2025-10-17 14:50 ` [PATCH v3 1/1] " Chad Jablonski
@ 2025-10-17 15:09   ` BALATON Zoltan
  2025-10-17 15:50     ` Chad Jablonski
  0 siblings, 1 reply; 4+ messages in thread
From: BALATON Zoltan @ 2025-10-17 15:09 UTC (permalink / raw)
  To: Chad Jablonski; +Cc: qemu-devel, kraxel, marcandre.lureau

On Fri, 17 Oct 2025, Chad Jablonski wrote:
> Rage 128 cards always request 64MB for their linear (framebuffer)
> aperture and R100 cards always request 128MB. This is regardless
> of the amount of physical VRAM on the board. The following are results
> from real hardware tests:
>
> Card                              VRAM    PCI BAR0   CONFIG_MEMSIZE  CONFIG_APER_SIZE  AGP_APER_OFFSET
> -----------------------           ----    --------   --------------  ----------------  ---------------
> Rage 128 Pro Ultra TF             32MB     64MB       0x02000000      0x02000000        0x02000000
> Rage 128 RF/SG AGP                16MB     64MB       0x01000000      0x02000000        0x02000000
> Radeon R100 QD [Radeon 7200]      64MB    128MB       0x04000000      0x04000000        N/A
> Radeon RV100 QY [Radeon 7000/VE]  32MB    128MB       0x02000000      0x04000000        N/A
>
> Previously the linear aperture (BAR0) would match the VRAM size.
> This discrepancy caused issues with the X.org and XFree86 r128 drivers.
> These drivers apply a mask of 0xfc000000 (2^26 = 64MB) to the linear
> aperture address. If that address is not on a 64MB boundary the
> framebuffer points to an incorrect memory location.
>
> Testing shows that the Radeon R100 also has a BAR0 larger than VRAM
> (128MB in this case) and the X.org radeon driver also masks to 64MB.
>
> For Rage 128, CONFIG_APER_SIZE also differs from the previous value and
> the behavior stated in the documentation. The Rage 128 register guide
> states that it should contain the size of the VRAM + AGP memory. The cards
> tested above show that this isn't the case. These tests also included
> enabling/disabling AGP with 8MB of memory. It didn't change the
> contents of CONFIG_APER_SIZE.
>
> For both Rage 128 and R100 the CONFIG_APER_SIZE is half of the PCI BAR0 size.
>
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> ---
> hw/display/ati.c     | 17 +++++++++++++++--
> hw/display/ati_int.h |  5 +++++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index f7c0006a87..54a067c243 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -361,7 +361,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>                                       PCI_BASE_ADDRESS_0, size) & 0xfffffff0;
>         break;
>     case CONFIG_APER_SIZE:
> -        val = s->vga.vram_size / 2;
> +        val = (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
> +               ATI_RAGE128_LINEAR_APER_SIZE : ATI_R100_LINEAR_APER_SIZE) / 2;

So can't we just use here memory_region_size(&s->linear_aper) / 2 like for 
CONFIG_REG_APER_SIZE? With that

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Thank you for testing and finding out all this.

Regards,
BALATON Zoltan

>         break;
>     case CONFIG_REG_1_BASE:
>         val = pci_default_read_config(&s->dev,
> @@ -952,6 +953,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
> {
>     ATIVGAState *s = ATI_VGA(dev);
>     VGACommonState *vga = &s->vga;
> +    uint64_t aper_size;
>
> #ifndef CONFIG_PIXMAN
>     if (s->use_pixman != 0) {
> @@ -1011,7 +1013,18 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>     /* io space is alias to beginning of mmregs */
>     memory_region_init_alias(&s->io, OBJECT(s), "ati.io", &s->mm, 0, 0x100);
>
> -    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &vga->vram);
> +    /*
> +     * The framebuffer is at the beginning of the linear aperture. For
> +     * Rage128 the upper half of the aperture is reserved for an AGP
> +     * window (which we do not emulate.)
> +     */
> +    aper_size = s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
> +                ATI_RAGE128_LINEAR_APER_SIZE : ATI_R100_LINEAR_APER_SIZE;
> +    memory_region_init(&s->linear_aper, OBJECT(dev), "ati-linear-aperture0",
> +                       aper_size);
> +    memory_region_add_subregion(&s->linear_aper, 0, &vga->vram);
> +
> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->linear_aper);
>     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
>     pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mm);
>
> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> index f5a47b82b0..708cc1dd3a 100644
> --- a/hw/display/ati_int.h
> +++ b/hw/display/ati_int.h
> @@ -10,6 +10,7 @@
> #define ATI_INT_H
>
> #include "qemu/timer.h"
> +#include "qemu/units.h"
> #include "hw/pci/pci_device.h"
> #include "hw/i2c/bitbang_i2c.h"
> #include "vga_int.h"
> @@ -29,6 +30,9 @@
> /* Radeon RV100 (VE) */
> #define PCI_DEVICE_ID_ATI_RADEON_QY 0x5159
>
> +#define ATI_RAGE128_LINEAR_APER_SIZE (64 * MiB)
> +#define ATI_R100_LINEAR_APER_SIZE (128 * MiB)
> +
> #define TYPE_ATI_VGA "ati-vga"
> OBJECT_DECLARE_SIMPLE_TYPE(ATIVGAState, ATI_VGA)
>
> @@ -97,6 +101,7 @@ struct ATIVGAState {
>     QEMUCursor *cursor;
>     QEMUTimer vblank_timer;
>     bitbang_i2c_interface bbi2c;
> +    MemoryRegion linear_aper;
>     MemoryRegion io;
>     MemoryRegion mm;
>     ATIVGARegs regs;
>


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 1/1] ati-vga: Fix framebuffer mapping by using hardware-correct aperture sizes
  2025-10-17 15:09   ` BALATON Zoltan
@ 2025-10-17 15:50     ` Chad Jablonski
  0 siblings, 0 replies; 4+ messages in thread
From: Chad Jablonski @ 2025-10-17 15:50 UTC (permalink / raw)
  To: BALATON Zoltan, Chad Jablonski; +Cc: qemu-devel, kraxel, marcandre.lureau

>
> So can't we just use here memory_region_size(&s->linear_aper) / 2 like for 
> CONFIG_REG_APER_SIZE? With that
>

Yep! That makes a lot more sense. Sending v4.

> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Thank you for testing and finding out all this.
>

Thank you for taking the time to review!

- Chad


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-10-17 15:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-17 14:50 [PATCH v3 0/1] ati-vga: Fix framebuffer mapping by using hardware-correct aperture sizes Chad Jablonski
2025-10-17 14:50 ` [PATCH v3 1/1] " Chad Jablonski
2025-10-17 15:09   ` BALATON Zoltan
2025-10-17 15:50     ` Chad Jablonski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).