Re: agp_nvidia port for review

[Andrius V <vezhlys%gmail.com@localhost>]

On Mon, Jun 8, 2026 at 1:50 AM Taylor R Campbell <riastradh%netbsd.org@localhost> wrote:
>
> > 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.
>

Yeah, I tried sorting lexicographically, but the build was indeed failing.
I'll try to follow your advice and clean it up. However, it may be
needed for all AGP drivers.

> > +     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);
>

Sure, will do. I should probably check the style more diligently.

> > +     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) {
>

I think I felt that it was a very "bulky" statement for a switch key.

> > +             /* 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.
>

I'm not sure what the history is behind not wiring up the detach
functions in the first place. #if 0 is used in all agp_<vendor>
drivers I've checked.
I don't think it's part of agp_methods. If we want to enable it, we
should likely do so as a separate project covering all AGP drivers.
I'd like to keep it out of scope for this particular PR. Nevertheless,
I'll check what I can do to test it more thoroughly and, if needed,
update the code.
As of now, I have only compile-tested it before disabling it.

> > +     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?

I believe so. It's not only x86-specific, as far as I know, it's
primarily specific to AMD K7 (Socket 462/A — Athlon, Duron, and
Sempron CPUs), as implied by the constant definition AMD_K7_NUM_IORR.

Thank you for comments!

Regards,
Andrius V