1. Patchew
  2. Seabios
  3. View series

[SeaBIOS] [PATCH] ati-vga: Fix bios tables for radeon KMS driver

Chad Jablonski posted 1 patch 3 days, 5 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20260722024658.1214775-1-chad@jablonski.xyz
vgasrc/ati-tables.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[SeaBIOS] [PATCH] ati-vga: Fix bios tables for radeon KMS driver
Posted by Chad Jablonski 3 days, 5 hours ago 
This fixes two issues when running ati-vga with model=rv100 under QEMU
using a KMS driver.

First, the radeon KMS driver does an additional check when looking up COMBIOS
table entries. It checks a header size field to validate that the entry
falls in the header region. Prior to this patch that size field is zero
which causes the driver to fallback to another method (CLOCK_CNTL
registers) also resulting in bad values and a division by zero.

0x60 matches what I've observed in the bios of a Radeon QY (RV100)
(PCI 1002:5159). But I don't think the value is actually important
other than needing to be large enough to encompass the table entries
used here.

Second, the connector info section needs a non-zero value in the DDC
type field. Zero causes the radeon driver to fail to initialize the DDC
bus and then no connectors are found. DDC_DVI (0x2) is used here so that
the GPIO_DVI_DDC register is used for the radeon. From what I have seen
r128 drivers ignore this field entirely.

Tested on Debian Squeeze (Linux 2.6.32) and Debian Trixie (Linux 6.12.94).

Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---

To fully test this you'll need the ati-vga CP engine series under review
on qemu-devel: https://lore.kernel.org/qemu-devel/20260719161536.774061-1-chad@jablonski.xyz/

That series is focused on UMS only but this unlocks KMS!
---
 vgasrc/ati-tables.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/vgasrc/ati-tables.S b/vgasrc/ati-tables.S
index cdbde2fa..d7a8e22d 100644
--- a/vgasrc/ati-tables.S
+++ b/vgasrc/ati-tables.S
@@ -12,6 +12,8 @@
 	// main info
 	.org 0x50
 _ati_main:
+	.org 0x50 + 0x06
+	.byte 0x60		// COMBIOS header size
 	.org 0x50 + 0x30
 	.word _ati_pll
 	.org 0x50 + 0x50
@@ -37,7 +39,7 @@ _ati_pll:
 _ati_connector:
 	.byte 0x10		// one chip
 	.byte 0x01		// one connector
-	.word 0x3000		// type DVI-I
+	.word 0x3200		// type DVI-I and DVI DDC
 	.word 0			// end of list
 
 	.org 0x200
-- 
2.54.0

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] ati-vga: Fix bios tables for radeon KMS driver
Posted by Kevin O'Connor via SeaBIOS 2 days, 14 hours ago 
On Tue, Jul 21, 2026 at 10:46:58PM -0400, Chad Jablonski wrote:
> This fixes two issues when running ati-vga with model=rv100 under QEMU
> using a KMS driver.
> 
> First, the radeon KMS driver does an additional check when looking up COMBIOS
> table entries. It checks a header size field to validate that the entry
> falls in the header region. Prior to this patch that size field is zero
> which causes the driver to fallback to another method (CLOCK_CNTL
> registers) also resulting in bad values and a division by zero.
> 
> 0x60 matches what I've observed in the bios of a Radeon QY (RV100)
> (PCI 1002:5159). But I don't think the value is actually important
> other than needing to be large enough to encompass the table entries
> used here.
> 
> Second, the connector info section needs a non-zero value in the DDC
> type field. Zero causes the radeon driver to fail to initialize the DDC
> bus and then no connectors are found. DDC_DVI (0x2) is used here so that
> the GPIO_DVI_DDC register is used for the radeon. From what I have seen
> r128 drivers ignore this field entirely.
> 
> Tested on Debian Squeeze (Linux 2.6.32) and Debian Trixie (Linux 6.12.94).
> 
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>

Thanks.  It seems the ati-tables.S code was introduced by Gerd, so
hopefully he'll be able to review.

That said, is there any reason why this code needs to be in assembler?
It should be simpler to maintain the code if we can use C code
instead.  Perhaps something like the below (untested).  I'm just
guessing on table sizes, alignments, and field names so the example
could likely be improved.

-Kevin


commit eb5ccb59b40dceedec8b381df4a6b6b7cb4193f5
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Wed Jul 22 13:12:58 2026 -0400

    atiext: Implement ATI tables in atiext.c
    
    Move the definitions in ati-tables.S to atiext.c .
    
    Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

diff --git a/vgasrc/ati-tables.S b/vgasrc/ati-tables.S
deleted file mode 100644
index cdbde2f..0000000
--- a/vgasrc/ati-tables.S
+++ /dev/null
@@ -1,43 +0,0 @@
-//
-// Fake ati bios tables.
-//
-// aty128fb and radeonfb try to gather informations from these tables,
-// so add some stuff here to make the drivers happy.  Specifically
-// radeonfb needs the pll information, otherwise it'll crash with a
-// division by zero ...
-//
-	.org 0x48
-	.word _ati_main
-
-	// main info
-	.org 0x50
-_ati_main:
-	.org 0x50 + 0x30
-	.word _ati_pll
-	.org 0x50 + 0x50
-	.word _ati_connector
-
-	// pll info
-	.org 0x100
-_ati_pll:
-	.word	0		// ??? (not used by radeonfb)
-	.word	0
-	.word	0
-	.word	0
-	.word	23000		// sclk
-	.word	23000		// mclk
-	.word	0
-	.word	2700		// ref_clk
-	.word	4		// ref_div
-	.long	12000		// ppll_min
-	.long	35000		// ppll_max
-
-	// connector info
-	.org 0x140
-_ati_connector:
-	.byte 0x10		// one chip
-	.byte 0x01		// one connector
-	.word 0x3000		// type DVI-I
-	.word 0			// end of list
-
-	.org 0x200
diff --git a/vgasrc/atiext.c b/vgasrc/atiext.c
index 69dfd46..f003eea 100644
--- a/vgasrc/atiext.c
+++ b/vgasrc/atiext.c
@@ -338,6 +338,65 @@ static void ati_i2c_edid_rage128(void)
     dprintf(1, "ati: ... %s\n", valid ? "good" : "invalid");
 }
 
+/****************************************************************
+ * Fake ati bios tables
+ ****************************************************************/
+
+// aty128fb and radeonfb try to gather informations from these tables,
+// so add some stuff here to make the drivers happy.  Specifically
+// radeonfb needs the pll information, otherwise it'll crash with a
+// division by zero ...
+struct ati_main_table_s {
+    u8 pad_0[48];
+    u16 pll_table;
+    u8 pad_50[30];
+    u16 connector_table;
+    u8 pad_82[94];
+} PACKED;
+
+struct ati_pll_table_s {
+    u8 pad_0[8];
+    u16 sclk;
+    u16 mclk;
+    u8 pad_12[2];
+    u16 ref_clk;
+    u16 ref_div;
+    u32 ppll_min;
+    u32 ppll_max;
+    u8 pad_26[38];
+} PACKED;
+
+struct ati_connector_table_s {
+    u8 num_chip;
+    u8 num_connector;
+    u16 flags;
+    u16 next;
+} PACKED;
+
+extern u16 _rom_header_ati_table_anchor;
+static struct ati_main_table_s ati_main_table VAR16 __aligned(16);
+static struct ati_pll_table_s ati_pll_table VAR16 __aligned(16) = {
+    .sclk = 23000,
+    .mclk = 23000,
+    .ref_clk = 2700,
+    .ref_div = 4,
+    .ppll_min = 12000,
+    .ppll_max = 35000,
+};
+static struct ati_connector_table_s ati_connector_table VAR16 __aligned(16) = {
+    .num_chip = 0x10,
+    .num_connector = 1,
+    .flags = 0x3000,
+};
+
+static void
+ati_table_setup(void)
+{
+    SET_VGA(ati_main_table.pll_table, (u32)&ati_pll_table);
+    SET_VGA(ati_main_table.connector_table, (u32)&ati_connector_table);
+    SET_VGA(_rom_header_ati_table_anchor, (u32)&ati_main_table);
+}
+
 /****************************************************************
  * init
  ****************************************************************/
@@ -379,6 +438,8 @@ ati_setup(void)
     SET_VGA(VBE_total_memory, totalmem);
     SET_VGA(ati_io_addr, io_addr);
 
+    ati_table_setup();
+
     // Validate modes
     struct generic_svga_mode *m = svga_modes;
     unsigned int mcount = GET_GLOBAL(svga_mcount);
diff --git a/vgasrc/vgaentry.S b/vgasrc/vgaentry.S
index f9624fc..14e557a 100644
--- a/vgasrc/vgaentry.S
+++ b/vgasrc/vgaentry.S
@@ -41,7 +41,11 @@ _rom_header_signature:
         .asciz "IBM"
 
 #if CONFIG_VGA_ATI
-#include "ati-tables.S"
+	.org 0x48
+        .global _rom_header_ati_table_anchor
+_rom_header_ati_table_anchor:
+	.word 0
+        .align 16
 #endif
 
 /****************************************************************

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] ati-vga: Fix bios tables for radeon KMS driver
Posted by Gerd Hoffmann via SeaBIOS 2 days, 3 hours ago 
  Hi,

> Thanks.  It seems the ati-tables.S code was introduced by Gerd, so
> hopefully he'll be able to review.

Changes look sensible to me.

> That said, is there any reason why this code needs to be in assembler?

I don't remember.  Maybe to have the linker handle the table references,
so they are set at compile time ...

> +static void
> +ati_table_setup(void)
> +{
> +    SET_VGA(ati_main_table.pll_table, (u32)&ati_pll_table);
> +    SET_VGA(ati_main_table.connector_table, (u32)&ati_connector_table);
> +    SET_VGA(_rom_header_ati_table_anchor, (u32)&ati_main_table);
> +}

... instead of runtime.

I have no objections to move this to C though.

take care,
  Gerd

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] ati-vga: Fix bios tables for radeon KMS driver
Posted by Chad Jablonski 2 days, 5 hours ago 
>
> That said, is there any reason why this code needs to be in assembler?
> It should be simpler to maintain the code if we can use C code
> instead.  Perhaps something like the below (untested).  I'm just
> guessing on table sizes, alignments, and field names so the example
> could likely be improved.
>

Let me see if I can get this in C. Thanks for taking a look and thank you
for the massive head start.
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.