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.

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>
---
 vgasrc/atiext.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/vgasrc/atiext.c b/vgasrc/atiext.c
index f003eea8..073408ba 100644
--- a/vgasrc/atiext.c
+++ b/vgasrc/atiext.c
@@ -347,7 +347,9 @@ static void ati_i2c_edid_rage128(void)
 // radeonfb needs the pll information, otherwise it'll crash with a
 // division by zero ...
 struct ati_main_table_s {
-    u8 pad_0[48];
+    u8 pad_0[6];
+    u8 size;
+    u8 pad_7[41];
     u16 pll_table;
     u8 pad_50[30];
     u16 connector_table;
@@ -374,7 +376,9 @@ struct ati_connector_table_s {
 } PACKED;
 
 extern u16 _rom_header_ati_table_anchor;
-static struct ati_main_table_s ati_main_table VAR16 __aligned(16);
+static struct ati_main_table_s ati_main_table VAR16 __aligned(16) = {
+    .size = sizeof(struct ati_main_table_s),
+};
 static struct ati_pll_table_s ati_pll_table VAR16 __aligned(16) = {
     .sclk = 23000,
     .mclk = 23000,
@@ -386,7 +390,7 @@ static struct ati_pll_table_s ati_pll_table VAR16 __aligned(16) = {
 static struct ati_connector_table_s ati_connector_table VAR16 __aligned(16) = {
     .num_chip = 0x10,
     .num_connector = 1,
-    .flags = 0x3000,
+    .flags = 0x3200, // type DVI-I and use DVI DDC
 };
 
 static void
-- 
2.54.0

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

Dear Chad,


Thank you for your patches. Some minor comments.

Am 28.07.26 um 16:54 schrieb Chad Jablonski:
> 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

For me, “First, ….  Second, …” are good indicators to split the commit 
into one commit per issue.

> 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

to fall back

> registers) also resulting in bad values and a division by zero.
> 
> 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

… causes Linux’ radeon dirver …?

> 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).

Do you have a command line how to test this? (I think SeaBIOS does not 
put the cover letter into the git archive, so users just reading the 
commit would also not have the information, what QEMU requirements are 
needed.)

> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> ---
>   vgasrc/atiext.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/vgasrc/atiext.c b/vgasrc/atiext.c
> index f003eea8..073408ba 100644
> --- a/vgasrc/atiext.c
> +++ b/vgasrc/atiext.c
> @@ -347,7 +347,9 @@ static void ati_i2c_edid_rage128(void)
>   // radeonfb needs the pll information, otherwise it'll crash with a
>   // division by zero ...
>   struct ati_main_table_s {
> -    u8 pad_0[48];
> +    u8 pad_0[6];
> +    u8 size;
> +    u8 pad_7[41];
>       u16 pll_table;
>       u8 pad_50[30];
>       u16 connector_table;
> @@ -374,7 +376,9 @@ struct ati_connector_table_s {
>   } PACKED;
>   
>   extern u16 _rom_header_ati_table_anchor;
> -static struct ati_main_table_s ati_main_table VAR16 __aligned(16);
> +static struct ati_main_table_s ati_main_table VAR16 __aligned(16) = {
> +    .size = sizeof(struct ati_main_table_s),
> +};
>   static struct ati_pll_table_s ati_pll_table VAR16 __aligned(16) = {
>       .sclk = 23000,
>       .mclk = 23000,
> @@ -386,7 +390,7 @@ static struct ati_pll_table_s ati_pll_table VAR16 __aligned(16) = {
>   static struct ati_connector_table_s ati_connector_table VAR16 __aligned(16) = {
>       .num_chip = 0x10,
>       .num_connector = 1,
> -    .flags = 0x3000,
> +    .flags = 0x3200, // type DVI-I and use DVI DDC

For the curious, Linux’ radeon checks in 
`radeon_get_legacy_connector_info_from_bios()` [1].

In the comment you could use the Linux names for easier searching?

     CONNECTOR_DVI_I_LEGACY and use DDC_DVI

(Or define the macros also in SeaBIOS.)

>   };
>   
>   static void

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul


[1]: 
https://elixir.bootlin.com/linux/v6.12.96/source/drivers/gpu/drm/radeon/radeon_combios.c#L2314
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

Thanks for taking a look Paul. All good points, I will incorporate them in v3.
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org