All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work
@ 2026-07-30 18:26 Daniel Palmer
  2026-07-30 18:26 ` [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS Daniel Palmer
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Daniel Palmer @ 2026-07-30 18:26 UTC (permalink / raw)
  To: deller; +Cc: linux-fbdev, dri-devel, linux-kernel, Daniel Palmer

Currently using voodoo cards that weren't booted by the video
BIOS aren't usable.

This means if you happen to have a proper - non-x86 - computer,
another card is the primary or you have a new BIOS that cannot
run the video BIOS tdfxfb would probe but you be staring at
"no signal detected".

This series implements manually booting cards using the
values from the video BIOS' own table.

Debugging was partially assisted by Claude Fable 5. I could not
work out why this would not work for months so I told it to
create a voodoo 3 emulation for QEMU to compare the BIOS against
my code. That made it obvious my init sequence matched the BIOS
but the VGA core wasn't running.

This has been tested on a real voodoo 3 in an x86-64 board.
I have some patches to add an interface for doing 3D rendering
that are not included in this series and 3D rendering is also
working so as far as I can tell everything is good.

Daniel Palmer (5):
  fbdev: tdfxfb: Add helper to read config table from BIOS
  fbdev: tdfxfb: Attempt to detect if the card wasn't booted
  fbdev: tdfxfb: Manually boot unbooted cards
  fbdev: tdfxfb: Wake the VGA core before programming the CRTC
  fbdev: tdfxfb: Program the initial video mode

 drivers/video/fbdev/tdfxfb.c | 151 +++++++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

-- 
2.53.0


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

* [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS
  2026-07-30 18:26 [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work Daniel Palmer
@ 2026-07-30 18:26 ` Daniel Palmer
  2026-07-30 18:34   ` sashiko-bot
  2026-07-30 18:26 ` [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted Daniel Palmer
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Palmer @ 2026-07-30 18:26 UTC (permalink / raw)
  To: deller; +Cc: linux-fbdev, dri-devel, linux-kernel, Daniel Palmer

In the case that the video BIOS didn't run because the card isn't the
primary card, the BIOS doesn't support running old skool video BIOS
(modern BIOS without CSM), or the machine isn't x86 it needs to be
booted manually. To do this the config table in the BIOS is needed.

Add a helper to get the config table in preparation for manually
booting cards.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/video/fbdev/tdfxfb.c | 74 ++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index cc6a074f3165..fa554b09a0a6 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -71,6 +71,7 @@
 #include <linux/string.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 #include <linux/fb.h>
 #include <linux/init.h>
 #include <linux/pci.h>
@@ -336,6 +337,79 @@ static u32 do_calc_pll(int freq, int *freq_out)
 	return (n << 8) | (m << 2) | k;
 }
 
+/*
+ * Convert a pllctrl register value back to a frequency in kHz.
+ * Formula from 3dfx documentation.
+ */
+static u32 tdfx_pll_to_khz(u32 pll)
+{
+	return (14318 * (((pll >> 8) & 0xff) + 2) /
+		(((pll >> 2) & 0x3f) + 2)) >> (pll & 3);
+}
+
+/* Layout of the "OEM config" table in voodoo 3 BIOS */
+struct tdfx_bios_cfg {
+	__le32 pciinit0;	/* 0x00 */
+	__le32 miscinit0;	/* 0x04 */
+	__le32 miscinit1;	/* 0x08 */
+	__le32 draminit0;	/* 0x0c */
+	__le32 draminit1;	/* 0x10 */
+	__le32 agpinit0;	/* 0x14 */
+	__le32 pllctrl1;	/* 0x18 - memory PLL */
+	__le32 pllctrl2;	/* 0x1c - graphics PLL */
+	__le32 sgrammode;	/* 0x20 - SGRAM/SDRAM mode register data */
+} __packed;
+
+#define TDFX_ROM_CFG_PTR	0x50
+
+static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
+				struct tdfx_bios_cfg *cfg)
+{
+	u16 romcfg, oemcfg;
+	void __iomem *rom;
+	size_t romsize;
+	u8 *image;
+	u32 khz;
+
+	rom = pci_map_rom(pdev, &romsize);
+	if (!rom || !romsize)
+		return false;
+
+	image = vmalloc(romsize);
+	if (!image) {
+		pci_unmap_rom(pdev, rom);
+		return false;
+	}
+	memcpy_fromio(image, rom, romsize);
+	pci_unmap_rom(pdev, rom);
+
+	/* ROM[0x50] -> ROM config table -> OEM config table */
+	if (TDFX_ROM_CFG_PTR + 2 > romsize)
+		goto out;
+	romcfg = image[TDFX_ROM_CFG_PTR] | image[TDFX_ROM_CFG_PTR + 1] << 8;
+	if (romcfg == 0xffff || romcfg + 2 > romsize)
+		goto out;
+	oemcfg = image[romcfg] | image[romcfg + 1] << 8;
+	if (oemcfg == 0xffff || oemcfg + sizeof(*cfg) > romsize)
+		goto out;
+	memcpy(cfg, image + oemcfg, sizeof(*cfg));
+	vfree(image);
+
+	/*
+	 * Make sure we didn't read garbage from the BIOS and will
+	 * end up setting a frequency that explodes someone's expensive
+	 * card.
+	 */
+	khz = tdfx_pll_to_khz(le32_to_cpu(cfg->pllctrl1));
+	if (khz < 40000 || khz > 250000 || !le32_to_cpu(cfg->draminit0))
+		return false;
+	return true;
+
+out:
+	vfree(image);
+	return false;
+}
+
 static void do_write_regs(struct fb_info *info, struct banshee_reg *reg)
 {
 	struct tdfx_par *par = info->par;
-- 
2.53.0


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

* [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted
  2026-07-30 18:26 [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work Daniel Palmer
  2026-07-30 18:26 ` [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS Daniel Palmer
@ 2026-07-30 18:26 ` Daniel Palmer
  2026-07-30 18:41   ` sashiko-bot
  2026-07-30 18:26 ` [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards Daniel Palmer
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Palmer @ 2026-07-30 18:26 UTC (permalink / raw)
  To: deller; +Cc: linux-fbdev, dri-devel, linux-kernel, Daniel Palmer

Until now a card had to have been booted by its video BIOS
otherwise the driver would probe, create the fb etc but there
would be no output on the display.

There doesn't seem to be a documented way work out if the BIOS
ran or not. Checking if the values in registers match what is
in the config table in the BIOS seems to be the only option.

On my 16MB Voodoo 3 3000 the registers show 4MB of memory when
unbooted. Checking 4MB != 16MB is enough decide if to reject
the card or not for my machine at least.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/video/fbdev/tdfxfb.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index fa554b09a0a6..1d8bffe1e9fd 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -410,6 +410,25 @@ static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
 	return false;
 }
 
+/*
+ * Try to work out if the card was booted or not, just checks
+ * if the register reported memory amount matches what the BIOS
+ * reports for now.
+ */
+static int tdfxfb_hw_init(struct fb_info *info, struct pci_dev *pdev)
+{
+	struct tdfx_par *par = info->par;
+	struct tdfx_bios_cfg cfg;
+
+	if (tdfxfb_get_bios_cfg(pdev, &cfg) &&
+	    tdfx_inl(par, DRAMINIT0) == le32_to_cpu(cfg.draminit0))
+		return 0;
+
+	dev_err(&pdev->dev,
+		"Card hasn't booted and is unusable\n");
+	return -ENODEV;
+}
+
 static void do_write_regs(struct fb_info *info, struct banshee_reg *reg)
 {
 	struct tdfx_par *par = info->par;
@@ -1505,6 +1524,9 @@ static int tdfxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto out_err_regbase;
 	}
 
+	if (tdfxfb_hw_init(info, pdev))
+		goto out_err_regbase;
+
 	info->fix.smem_start = pci_resource_start(pdev, 1);
 	info->fix.smem_len = do_lfb_size(default_par, pdev->device);
 	if (!info->fix.smem_len) {
-- 
2.53.0


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

* [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards
  2026-07-30 18:26 [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work Daniel Palmer
  2026-07-30 18:26 ` [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS Daniel Palmer
  2026-07-30 18:26 ` [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted Daniel Palmer
@ 2026-07-30 18:26 ` Daniel Palmer
  2026-07-30 18:41   ` sashiko-bot
  2026-07-30 18:26 ` [PATCH 4/5] fbdev: tdfxfb: Wake the VGA core before programming the CRTC Daniel Palmer
  2026-07-30 18:26 ` [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode Daniel Palmer
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Palmer @ 2026-07-30 18:26 UTC (permalink / raw)
  To: deller; +Cc: linux-fbdev, dri-devel, linux-kernel, Daniel Palmer

If the card is detected as being unbooted it isn't too difficult
to use the config table in its BIOS to fire it up so do it.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/video/fbdev/tdfxfb.c | 56 ++++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index 1d8bffe1e9fd..4c1d847c89c9 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -67,6 +67,7 @@
 #include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/string.h>
 #include <linux/mm.h>
@@ -414,19 +415,62 @@ static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
  * Try to work out if the card was booted or not, just checks
  * if the register reported memory amount matches what the BIOS
  * reports for now.
+ *
+ * If we have a BIOS config table attempt to manually boot the
+ * card if needed.
  */
 static int tdfxfb_hw_init(struct fb_info *info, struct pci_dev *pdev)
 {
-	struct tdfx_par *par = info->par;
+	u32 mempll, gfxpll, draminit0, draminit1, miscinit1, dram_mode;
 	struct tdfx_bios_cfg cfg;
+	bool have_cfg = tdfxfb_get_bios_cfg(pdev, &cfg);
+	struct tdfx_par *par = info->par;
 
-	if (tdfxfb_get_bios_cfg(pdev, &cfg) &&
-	    tdfx_inl(par, DRAMINIT0) == le32_to_cpu(cfg.draminit0))
+	if (have_cfg && tdfx_inl(par, DRAMINIT0) == le32_to_cpu(cfg.draminit0))
 		return 0;
 
-	dev_err(&pdev->dev,
-		"Card hasn't booted and is unusable\n");
-	return -ENODEV;
+	if (have_cfg) {
+		dev_info(&pdev->dev,
+			 "Manually booting card using config table\n");
+		mempll = le32_to_cpu(cfg.pllctrl1);
+		gfxpll = le32_to_cpu(cfg.pllctrl2);
+		draminit0 = le32_to_cpu(cfg.draminit0);
+		draminit1 = le32_to_cpu(cfg.draminit1);
+		miscinit1 = le32_to_cpu(cfg.miscinit1);
+		dram_mode = le32_to_cpu(cfg.sgrammode);
+		tdfx_outl(par, PCIINIT0, le32_to_cpu(cfg.pciinit0));
+		tdfx_outl(par, AGPINIT, le32_to_cpu(cfg.agpinit0));
+	} else {
+		dev_err(&pdev->dev,
+			"Card hasn't booted and is unusable\n");
+		return -ENODEV;
+	}
+
+	/* memory clock, and the graphics clock if the card wants one */
+	tdfx_outl(par, PLLCTRL1, mempll);
+	if (gfxpll)
+		tdfx_outl(par, PLLCTRL2, gfxpll);
+	/* PLL lock */
+	udelay(100);
+
+	tdfx_outl(par, MISCINIT1, miscinit1);
+	tdfx_outl(par, DRAMINIT0, draminit0);
+	tdfx_outl(par, DRAMINIT1, draminit1);
+
+	/* Make sure the DRAM config is applied before continuing */
+	wmb();
+
+	/* SDRAM/SGRAM wake up: load the mode register */
+	tdfx_outl(par, DRAMDATA, dram_mode);
+	tdfx_outl(par, DRAMCOMMAND, 0x10d);
+
+	tdfx_outl(par, LFBMEMORYCONFIG, 0x00001fff);
+	tdfx_outl(par, MISCINIT0, 0);
+
+	/* Make sure the remaining config is applied */
+	wmb();
+
+	return 0;
 }
 
 static void do_write_regs(struct fb_info *info, struct banshee_reg *reg)
-- 
2.53.0


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

* [PATCH 4/5] fbdev: tdfxfb: Wake the VGA core before programming the CRTC
  2026-07-30 18:26 [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work Daniel Palmer
                   ` (2 preceding siblings ...)
  2026-07-30 18:26 ` [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards Daniel Palmer
@ 2026-07-30 18:26 ` Daniel Palmer
  2026-07-30 18:26 ` [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode Daniel Palmer
  4 siblings, 0 replies; 10+ messages in thread
From: Daniel Palmer @ 2026-07-30 18:26 UTC (permalink / raw)
  To: deller; +Cc: linux-fbdev, dri-devel, linux-kernel, Daniel Palmer

If the card was unbooted the VGA core needs to be woken up before
poking at it.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/video/fbdev/tdfxfb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index 4c1d847c89c9..9d55efd2c934 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -482,6 +482,10 @@ static void do_write_regs(struct fb_info *info, struct banshee_reg *reg)
 
 	tdfx_outl(par, MISCINIT1, tdfx_inl(par, MISCINIT1) | 0x01);
 
+	/* Wake the VGA core if it hasn't already been woken up */
+	tdfx_outl(par, VGAINIT0, reg->vgainit0);
+	vga_outb(par, 0x3c3, 0x01);
+
 	crt_outb(par, 0x11, crt_inb(par, 0x11) & 0x7f); /* CRT unprotect */
 
 	banshee_make_room(par, 3);
-- 
2.53.0


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

* [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode
  2026-07-30 18:26 [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work Daniel Palmer
                   ` (3 preceding siblings ...)
  2026-07-30 18:26 ` [PATCH 4/5] fbdev: tdfxfb: Wake the VGA core before programming the CRTC Daniel Palmer
@ 2026-07-30 18:26 ` Daniel Palmer
  2026-07-30 18:33   ` sashiko-bot
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Palmer @ 2026-07-30 18:26 UTC (permalink / raw)
  To: deller; +Cc: linux-fbdev, dri-devel, linux-kernel, Daniel Palmer

If the card does not get bound to by fbcon set_par() never happens
and the initial video mode is not setup and the display detects
no signal.

Program the video mode and also clear the framebuffer memory so
random garbage isn't displayed.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/video/fbdev/tdfxfb.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index 9d55efd2c934..d03cb47b34a5 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -1692,6 +1692,13 @@ static int tdfxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	 * Our driver data
 	 */
 	pci_set_drvdata(pdev, info);
+
+	/* Program a video mode so the display detects a signal */
+	tdfxfb_set_par(info);
+
+	/* Don't scare the user with random garbage on their display */
+	memset_io(info->screen_base, 0, info->fix.smem_len);
+
 	return 0;
 
 out_err_iobase:
-- 
2.53.0


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

* Re: [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode
  2026-07-30 18:26 ` [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode Daniel Palmer
@ 2026-07-30 18:33   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-07-30 18:33 UTC (permalink / raw)
  To: Daniel Palmer; +Cc: dri-devel

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware programming and framebuffer clearing occur after the device is registered, causing data races with user-space and destroying fbcon console output.
--

--- Patch [5]: [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode ---
commit 90ecec6554588f0e84d39be7f57c41d5df7e26d5
Author: Daniel Palmer <daniel@0x0f.com>

fbdev: tdfxfb: Program the initial video mode

If the card does not get bound to by fbcon, the initial video mode is
not setup and the display detects no signal. This commit programs the
video mode and clears the framebuffer memory so random garbage isn't
displayed.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
> @@ -1692,6 +1692,13 @@ static int tdfxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	 * Our driver data
>  	 */
>  	pci_set_drvdata(pdev, info);
> +
> +	/* Program a video mode so the display detects a signal */
> +	tdfxfb_set_par(info);
> +
> +	/* Don't scare the user with random garbage on their display */
> +	memset_io(info->screen_base, 0, info->fix.smem_len);

[Severity: High]
Does calling tdfxfb_set_par() and memset_io() after register_framebuffer()
cause issues with fbcon initialization?

When register_framebuffer() is called just prior to this, it synchronously
notifies the console subsystem. If fbcon binds to the new framebuffer, it
configures the display and draws the console text and boot logo.

By unconditionally clearing the VRAM and setting the parameters after
register_framebuffer() returns, doesn't this wipe out the console output
that fbcon just initialized?

Additionally, since register_framebuffer() exposes the device to user-space
via /dev/fb0, could this create a data race against concurrent user-space
ioctls accessing hardware registers or VRAM without holding info->lock?

> +
>  	return 0;
>  
>  out_err_iobase:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730182640.2808572-1-daniel@0x0f.com?part=5

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

* Re: [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS
  2026-07-30 18:26 ` [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS Daniel Palmer
@ 2026-07-30 18:34   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-07-30 18:34 UTC (permalink / raw)
  To: Daniel Palmer; +Cc: dri-devel

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces `tdfxfb_get_bios_cfg` and `tdfx_pll_to_khz` as `static` functions but does not use them anywhere in the file. This triggers `-Wunused-function` compiler warnings, which break builds with `CONFIG_WERROR` enabled.
--

--- Patch [1]: [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS ---
commit d67a333d81585d56234b84350edd145b08f26be1
Author: Daniel Palmer <daniel@0x0f.com>

fbdev: tdfxfb: Add helper to read config table from BIOS

This commit adds a helper to get the config table from the video BIOS in
preparation for manually booting cards.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
[ ... ]
> +/*
> + * Convert a pllctrl register value back to a frequency in kHz.
> + * Formula from 3dfx documentation.
> + */
> +static u32 tdfx_pll_to_khz(u32 pll)
> +{
> +	return (14318 * (((pll >> 8) & 0xff) + 2) /
> +		(((pll >> 2) & 0x3f) + 2)) >> (pll & 3);
> +}
> +
> +/* Layout of the "OEM config" table in voodoo 3 BIOS */
> +struct tdfx_bios_cfg {
[ ... ]
> +};
> +
> +#define TDFX_ROM_CFG_PTR	0x50
> +
> +static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
> +				struct tdfx_bios_cfg *cfg)
> +{

[Severity: Low]
Does adding this static function and its helper tdfx_pll_to_khz without any
callers trigger a -Wunused-function compiler warning that breaks builds with
CONFIG_WERROR enabled? 

Could these functions be marked with __maybe_unused, or introduced in the
same commit as their callers to ensure the tree compiles cleanly at every
commit?

> +	u16 romcfg, oemcfg;
> +	void __iomem *rom;
> +	size_t romsize;
> +	u8 *image;
> +	u32 khz;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730182640.2808572-1-daniel@0x0f.com?part=1

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

* Re: [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted
  2026-07-30 18:26 ` [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted Daniel Palmer
@ 2026-07-30 18:41   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-07-30 18:41 UTC (permalink / raw)
  To: Daniel Palmer; +Cc: dri-devel

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver unconditionally rejects perfectly working cards if the video BIOS ROM is inaccessible or in an unsupported format.
--

--- Patch [2]: [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted ---
commit e450df3d917986ae9c59e1033b55fdddafa9cf19
Author: Daniel Palmer <daniel@0x0f.com>
Subject: fbdev: tdfxfb: Attempt to detect if the card wasn't booted

This commit adds hardware initialization checks to tdfxfb_probe() by
attempting to read the configuration from the video BIOS. If the register
values do not match the BIOS config, it rejects the card to avoid providing
a blank display.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
[ ... ]
> +/*
> + * Try to work out if the card was booted or not, just checks
> + * if the register reported memory amount matches what the BIOS
> + * reports for now.
> + */
> +static int tdfxfb_hw_init(struct fb_info *info, struct pci_dev *pdev)
> +{
> +	struct tdfx_par *par = info->par;
> +	struct tdfx_bios_cfg cfg;
> +
> +	if (tdfxfb_get_bios_cfg(pdev, &cfg) &&
> +	    tdfx_inl(par, DRAMINIT0) == le32_to_cpu(cfg.draminit0))
> +		return 0;
> +

[Severity: High]
Will this unconditionally reject working cards if the video BIOS ROM is
inaccessible or in an unsupported format?

Looking at tdfxfb_get_bios_cfg(), it can naturally return false if
pci_map_rom() fails. This is common on architectures without PCI ROM
support, or if the ROM BAR is disabled. It will also return false if the
BIOS ROM does not contain the specific 3dfx OEM configuration table.

Because of the logical && short-circuit, if tdfxfb_get_bios_cfg() fails to
read the config, the tdfx_inl() check is skipped and the function falls
through to return -ENODEV.

Could this cause a regression for hardware that was previously supported but
lacks a readable BIOS ROM?

> +	dev_err(&pdev->dev,
> +		"Card hasn't booted and is unusable\n");
> +	return -ENODEV;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730182640.2808572-1-daniel@0x0f.com?part=2

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

* Re: [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards
  2026-07-30 18:26 ` [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards Daniel Palmer
@ 2026-07-30 18:41   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-07-30 18:41 UTC (permalink / raw)
  To: Daniel Palmer; +Cc: dri-devel

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI MMIO writes are not flushed before udelay() or hardware dependent operations, and wmb() is misused to flush PCI writes.
- [High] The miscinit0 field from the BIOS configuration table is ignored and incorrectly hardcoded to 0.
--

--- Patch [3]: [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards ---
commit 2a277f3197dc755d9c642a91cfd45c9bd1ee77f8
Author: Daniel Palmer <daniel@0x0f.com>

fbdev: tdfxfb: Manually boot unbooted cards
    
If the card is detected as being unbooted it isn't too difficult
to use the config table in its BIOS to fire it up so do it.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> index 1d8bffe1e9fdf..4c1d847c89c9d 100644
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
[ ... ]
> @@ -414,19 +415,62 @@ static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
>   * Try to work out if the card was booted or not, just checks
>   * if the register reported memory amount matches what the BIOS
>   * reports for now.
> + *
> + * If we have a BIOS config table attempt to manually boot the
> + * card if needed.
>   */
>  static int tdfxfb_hw_init(struct fb_info *info, struct pci_dev *pdev)
>  {
[ ... ]
> +	/* memory clock, and the graphics clock if the card wants one */
> +	tdfx_outl(par, PLLCTRL1, mempll);
> +	if (gfxpll)
> +		tdfx_outl(par, PLLCTRL2, gfxpll);
> +	/* PLL lock */
> +	udelay(100);

[Severity: High]
Does this delay reliably wait for the PLL lock?

tdfx_outl uses writel which performs a posted write on PCI buses, so the delay
might start before the PCI bridge actually delivers the write to the device.

Would adding a dummy read to a device register (e.g. tdfx_inl) before the
udelay ensure the posted writes are flushed?

> +
> +	tdfx_outl(par, MISCINIT1, miscinit1);
> +	tdfx_outl(par, DRAMINIT0, draminit0);
> +	tdfx_outl(par, DRAMINIT1, draminit1);
> +
> +	/* Make sure the DRAM config is applied before continuing */
> +	wmb();

[Severity: High]
Does wmb actually flush the PCI posted writes?

A CPU memory barrier typically only flushes CPU store buffers, not PCI
posted write buffers, so the DRAM configuration might still be pending
when the subsequent wake-up commands are sent.

> +
> +	/* SDRAM/SGRAM wake up: load the mode register */
> +	tdfx_outl(par, DRAMDATA, dram_mode);
> +	tdfx_outl(par, DRAMCOMMAND, 0x10d);
> +
> +	tdfx_outl(par, LFBMEMORYCONFIG, 0x00001fff);
> +	tdfx_outl(par, MISCINIT0, 0);

[Severity: High]
Is this hardcoded 0 intentional?

The rest of the fields (like miscinit1) are populated from the BIOS
configuration table. It appears le32_to_cpu(cfg.miscinit0) is completely
ignored, which could overwrite essential hardware configurations managed
by MISCINIT0 with 0.

> +
> +	/* Make sure the remaining config is applied */
> +	wmb();
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730182640.2808572-1-daniel@0x0f.com?part=3

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

end of thread, other threads:[~2026-07-30 18:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-30 18:26 [PATCH 0/5] fbdev: tdfxfb: Make "unbooted" cards work Daniel Palmer
2026-07-30 18:26 ` [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS Daniel Palmer
2026-07-30 18:34   ` sashiko-bot
2026-07-30 18:26 ` [PATCH 2/5] fbdev: tdfxfb: Attempt to detect if the card wasn't booted Daniel Palmer
2026-07-30 18:41   ` sashiko-bot
2026-07-30 18:26 ` [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards Daniel Palmer
2026-07-30 18:41   ` sashiko-bot
2026-07-30 18:26 ` [PATCH 4/5] fbdev: tdfxfb: Wake the VGA core before programming the CRTC Daniel Palmer
2026-07-30 18:26 ` [PATCH 5/5] fbdev: tdfxfb: Program the initial video mode Daniel Palmer
2026-07-30 18:33   ` sashiko-bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.