Thanks, good catch on the revision. Bumped it now.

I don't think Fixes is a good fit for that but I did include the commit hash of
the 570 bump to document the previous change.

You want a Fixes tag in this commit? Or are you saying in the future commit that removes this? I don't think putting a Fixes tag in this commit makes sense, this commit doesn't contain a fix it only documents a workaround.

The suspend/resume breakage was found after this commit, it wasn't something known before committing or else it would not have landed.

@grahamperrin so this "failed to allocate memory" issue happens when you are using displays connected through the HP display dock? Does it ever happen when you are just using the monitors directly connected to the NVIDIA card? Is this a laptop? Those docks have always given me all kinds of problems, the thinkpad dock I have likes to turn one particular monitor of mine off and on randomly.

Still LGTM, thanks

Looks good, thanks

Aside from the one note about the fbdev comment this looks good to me, thanks.

+mentor and also manu to double check everything looks compatible with the new drm-66-kmod.

So the stock MODULE_PNP_INFOs do handle structures such as that, I guess the problem is we don't use them for ACPI things. from the manpage:

static struct my_pciids {
               uint32_t devid;
               const char *desc;
       } my_ids[] = {
               { 0x12345678, "Foo bar" },
               { 0x9abcdef0, "Baz fizz" },
       };

Curious your opinions on this. Maybe I'm misreading the pnp code but it looks like we have been handing it the wrong format and it thinks it's a list of strings when it isn't.

No worries, thanks for submitting a fix.

I'm not in the wireless group but I suppose I could be. I also lurk in the kernel/desktop/gaming channels in the freebsd discord if you use that, feel free to ping or email me.

Sure, what IMPROVING file are you referring to? I'd be happy to test things, I'm trying to get a modern laptop setup on this lenovo legion laptop I have so it would be good to use rtw89 for that.

Huh I think I had seen the long idle disconnect thing earlier today but didn't realize that was what it was.

linuxkpi_ieee80211 has a function it calls to zero the next/prev fields called TAILQ_ELEM_INIT, so that it could do this check.

Thanks updated, does this look more clear?

What's preventing rtw89 from getting enabled in the build? With this fix I could comfortably test youtube, iperf3, ssh. Speed was ~20mbps but stable. I think it could make sense to open this up to more people.

Pushed here: https://github.com/amshafer/freebsd/commit/214a96bb38bf5534b59eb149367369a233fa6826

Pushed it here: https://github.com/amshafer/freebsd/tree/this_module

Added parentheses

Yes, thanks. I don't have a committer bit.

Thanks, updated to just remove the module_get() bits for now.

Updated with jhb's proposal. Also implemented module_get and module_put by casting to linker_file_t and bumping the refcount. Retested with PRIME offloading to confirm this still fixes the panic. Unlike the previous (similar) version this handles the statically compiled module case.

Any update on getting this merged?

I believe that would work and I agree it would be much simpler. @kib is there something the (b) variant provides that (a) would be missing if we went with @jhb's proposal? It seems like they provide the same functionality?

I guess the last question is what linuxkpi macro should actually create the global? I had previously used LKPI_DRIVER_MODULE, but not everywhere uses that. I guess we could make our own DEFINE_THIS_MODULE() macro and then call it from LKPI_DRIVER_MODULE, and add calls elsewhere as needed?

It seems the way it works is that THIS_MODULE is a NULL pointer during "builtin" (statically building modules into the kernel), and points to the struct module of the dynamically loaded module otherwise. I think we could do something similar where we get the current linuxkpi behavior (THIS_MODULE is null) if we aren't building a dynamically loaded module?

how would any of proposals handle LKPI modules statically compiled into the kernel?

Instead, it should be only resolved locally, causing undefined symbol error if attempt is made to reference it from kld without the module symbol (section ?).

So what is the expected use of THIS_MODULE in Linux? Determine that current module is not that module, or something else?

Updated with KLD_DPF debug statement.

Updated to be generic. I changed the name to __this_linker_file to better represent
what the variable points at. On linux __this_module points at a struct module, so
for us we point at a linker_file_t.

Updated with style, thanks.

There isn't a PR, this has been a longstanding issue with nvidia-drm that I'm
finally getting around to solving.

Thanks for all the suggestions! I've implemented it by doing the special
casing in linker_file_lookup_symbol_internal as suggested. It looks like
this does work, I can see different owner fields populated in nvidia-drm
and drm. Also confirmed multiple usages of THIS_MODULE in drm.ko point to
the same thing.

It sounds like linux has insmod set up the __this_module variable when a module is loaded. If you're asking about doing something equivalent then I don't have a good estimate for how hard that would be. I don't know the details though.

That's a promising idea. I wonder if instead of having an unusual inline function we could instead make it part of the build setup. Something like setting KBUILD_MODNAME_HASH to whatever sha1 -s "$KBUILD_MODNAME" returns as part of the Makefile? That seems like it might be a little cleaner and easier to debug. Only downside is we have to update all of the makefiles that use KBUILD_MODNAME, I can't think of a way to automate it off the top of my head.

Bump __FreeBSD_version for drm-kmod

Since there were no comments I've gone ahead with the strategy I
outlined in my last comment: Have drm-kmod create a global variable
during LKPI_DRIVER_MODULE and point THIS_MODULE at it.

Maybe an interesting solution would be to do something a bit more DRM specific using LKPI_DRIVER_MODULE. Afaict only the DRM drivers will run into the issue that motivated this change. There's no reason we couldn't do something like this in base or just move the bits I mentioned into base.

Yes sorry, bumped PORTREVISION

This has been tested by @grahamperrin

Moved BUILD_DEPENDS to before as suggested.

Updated to handle 13.2 by patching conftest.sh

Updated to add back in RUN_DEPENDS

In D44305#1010541, @arrowd wrote:

On FreeBSD 13 this results in
../common/inc/nv-mm.h:110:31: error: too many arguments to function call, expected 5, have 8

That's for struct class though, right? I'm talking about struct module, and struct file_operations. Here's an example usage from nvidia-drm: https://github.com/amshafer/nvidia-driver/blob/535.98/nvidia/src/nvidia-drm/nvidia-drm-gem.c#L156. Iirc there's a couple similar ones in other drivers in base.

I don't think so? The 550 build has been broken because of this so no actual version has been "released" for nvidia-drm-510-kmod with 550, so it doesn't seem like bumping the revision would do anything? I can if needed though

I'm open to other suggestions about how we can do this. We definitely want THIS_MODULE to be non-zero, but having it be an opaque pointer that isn't a struct module isn't that great. Barring fully implementing module in linuxkpi I think something like this could make sense to allow emulated code to check if devices/dmabufs/etc come from the same module?

I think you need to update to a more recent CURRENT, that was added at the end of december I think.

Odd I hadn't seen that warning. I removed the PORTVERSIONS and also reset PORTREVISION.

Update to add a pkg-message warning about the new modesetting sysctl

Rebase on latest main