Re: agp_nvidia port for review

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Mon, 8 Jun 2026 00:54:14 +0300
> From: Andrius V <vezhlys%gmail.com@localhost>
> 
> +#include <sys/param.h>
> +#include <sys/systm.h>
> ...
> +#include <sys/agpio.h>
> +
> +#include <dev/pci/pcidevs.h>
> ...
> +#include <dev/pci/agpreg.h>

Sort includes lexicographically within each section, as in
/usr/share/misc/style.

If it doesn't build when sorted, insert the necessary forward
declarations or #includes in the included .h files to make it build.
If it still doesn't build because of a cyclic dependency, time to get
out the chainsaw and fix the header ouroboros.

> +	u_int32_t		initial_aperture; /* aperture size at startup */
> [...]
> +	u_int32_t		wbc_mask;

Since this is effectively new code without an upstream (assuming it's
going to be deleted in FreeBSD soon), use the standard C99 uint32_t
types.

> +	error = nvidia_init_iorr(apbase, AGP_GET_APERTURE(sc));
> +	error = 0;
> +	if (error) {

?

> +		pci_conf_write(sc->as_pc, nsc->mc2_tag, AGP_NVIDIA_2_ATTBASE(i),
> +				 (gatt->ag_physical + (i % nsc->num_dirs) * 64 * 1024) | 1);

Four-space additional indentation on continuation lines, per KNF.
Maybe use a temporary to keep the nesting levels down:

		const uint32_t physical =
		    gatt->ag_physical + (i % nsc->num_dirs) * 64 * 1024;
		pci_conf_write(sc->as_pc, nsc->mc2_tag,
		    AGP_NVIDIA_2_ATTBASE(i),
		    physical | 1);

> +	int val;
> +	
> +	val = (int) pci_conf_read(sc->as_pc, sc->as_tag, AGP_NVIDIA_0_APSIZE) & 0xff;
> +	switch (val) {

Why the intermediate val, and the cast?  Why not just:

	switch (pci_conf_read(sc->as_pc, sc->as_tag, AGP_NVIDIA_0_APSIZE) &
	    0xff) {

> +		/* Wait no more than 3 seconds. */
> +		for (i = 0; i < 3000; i++) {
> +			wbc_reg = pci_conf_read(sc->as_pc, nsc->mc1_tag, AGP_NVIDIA_1_WBC);
> +			if ((nsc->wbc_mask & wbc_reg) == 0)
> +				break;
> +			else
> +				DELAY(1000);

Seems like a very long time to wait in a DELAY loop?  Wonder if this
should be a preempt_point.

> +#if 0
> +static int
> +agp_nvidia_detach(struct agp_softc *sc)

Would be nice to test the detach path and not leave it #if 0, so this
could be a loadable _and unloadable_ module in the future.

> +	for(iorr_addr = 0; iorr_addr < AMD_K7_NUM_IORR; iorr_addr++) {

KNF: space between `for' and `('.

> +		base = rdmsr(IORR_BASE0 + 2 * iorr_addr);
> +		mask = rdmsr(IORR_MASK0 + 2 * iorr_addr);

Is this driver x86-specific?