Page MenuHomeFreeBSD

linuxkpi: upstream drm-kmod conflicting changes
Needs ReviewPublic

Authored by bz on Sep 29 2020, 10:20 PM.

Details

Summary

The upcoming in-kernel implementations for linuxkpi based on work on
iwlwifi (and other wireless drivers) conflicts in a few places with
the drm-kmod grpahics work outside the base system.

In order to transition smoothly and bring implementation parts, such
as a central implementation for linux firmware loading into the tree,
extract the conflicting bits for separate upfront review so that
ports and packages have a chance to deal with this before other things
come.
Some of the files are deliberately "empty" or some implementations
are still labeled with XXX but should provide equal support as the
out-of-tree work.

Changes to the LinuxKPI infrastructure and to drm-kmod will follow
indepedently.

MFC after:		3 months
X-MFS after:		still unclear as it would break kms-drm.
Obtained from:	bz_iwlwifi
Sponsored by:		The FreeBSD Foundation

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 33924
Build 31124: arc lint + arc unit

Event Timeline

bz requested review of this revision.Sep 29 2020, 10:20 PM
bz created this revision.
sys/compat/linuxkpi/common/include/asm/unaligned.h
53

This return is not needed.

63

Same.

82

What is the purpose of this comment ?

sys/compat/linuxkpi/common/include/linux/cpu.h
38 ↗(On Diff #77660)

s/if/with/ ?

sys/compat/linuxkpi/common/include/linux/kobject.h
159

Again return. Would be nice to give a hint about intended functionality.

sys/compat/linuxkpi/common/src/linux_firmware.c
57

Extra () around condition.

149

^^^

This change looks good. Just address @kib's comments.

sys/compat/linuxkpi/common/include/linux/cpu.h
41 ↗(On Diff #77660)

No point of adding this empty file, drm-kmod only have an dummy include so it will be used if base or gplv2/ doesn't have it.

sys/compat/linuxkpi/common/include/linux/device.h
572

Not sure we want to add those functions right now.
We need to propertly track all those malloc in the device struct in some way by either adding the devres stuff if it's also used directly by some drivers or doing our own stuff.

sys/compat/linuxkpi/common/include/linux/firmware.h
42

I think this comment is obsolete now that you only define the struct once.

sys/compat/linuxkpi/common/include/linux/pm.h
37

I don't think that we build anything with CONFIG_PM_SLEEP added now so no need to add this for now.

bz marked 8 inline comments as done.Oct 1 2020, 1:36 PM

I'll wait to see what @manu or others in case of kobject_uevent_env() say and then upload the final diff. Interim version coming in a minute.

sys/compat/linuxkpi/common/include/asm/unaligned.h
82

Sorry 20 year old habit.

sys/compat/linuxkpi/common/include/linux/cpu.h
41 ↗(On Diff #77660)

I believe I had issues at one point; otherwise I would not have added it but I am fine to leave it out if you say it's not needed and then bring it back later with the content.

sys/compat/linuxkpi/common/include/linux/device.h
572

Yes, but currently this is no better or worse than the DRM implementation from what I gathered.
The reason of this set of changes is not to change the status-quo but to resolve the linuxkpi conflicts so that further additions to the linuxkpi in base can be done without worrying (too much) about DRM conflicts again and again.
In that light, would it be okay to leave them like this?

sys/compat/linuxkpi/common/include/linux/kobject.h
159

Again return. Would be nice to give a hint about intended functionality.

Removed the "return".
Not sure about what we'd do with it; hence left the XXX; I know iwlwifi sends an INACCESSIBLE event when it detects that the card (pice endpoint) gone and it attempts a removal cleanup. Not sure if we do anything related to udev/sysfs at the moment or need a shortcut or simply ignore it; hence left the /* XXX */ to clearly distinguish from a purposefully left empty function. I'll be happy to put a suggested text there if any of you know more?

sys/compat/linuxkpi/common/include/linux/pm.h
37

I believe one of the drivers in my tree did need them as they had not properly #ifdefed everything.
Otherwise I'd not have done them.
I can leave them out for the pure DRM conflict resolving but then they'd come back soon anyway.
Which one would you prefer?

bz marked 2 inline comments as done.

Address comments from kib mostly.

sys/compat/linuxkpi/common/include/linux/device.h
562

Why can't you just make a list in the _dev, and extend the allocation with a STAILQ entry?
Then there is at least some way to clean up.

I added a linuxkpi group in Phabricator that folks with an interest may join

sys/compat/linuxkpi/common/include/linux/device.h
572

There is a lot in the drm-kmod code that needs to be in tree, the main reason that they aren't there for now is that nobody took the time to clean them and properly done them.
I don't think that this is a good things to add stuff in linuxkpi in base if they aren't correct.

sys/compat/linuxkpi/common/include/linux/pm.h
37

The only user in drm-kmod is backlight which will go away soon so you can leave it.

sys/compat/linuxkpi/common/include/linux/device.h
562

Because it is more than just tracking mallocs imho; I have drivers not cleaning up PCI things either and neither this nor DRM currently track it. It's an entire separate thing to implement all this and test it. If I keep doing this I'll still be talking about conflicting things with DRM in 6 months. No one says we shouldn't do this. I am trying to keep this one simple; it's been hard enough to compile it.

bz marked 2 inline comments as done.Oct 1 2020, 2:47 PM
bz added inline comments.
sys/compat/linuxkpi/common/include/linux/device.h
572

The code from the gplv2 directory is mostly not applicable to be moved into head I was told. Neither did I know about it when I initially worked on my code (which was good). I had shared my code previously so people could pick and choose and knew what was coming. The only reason I went near the DRM linuxkpi implementation is to avoid any breakage when starting to move things into HEAD putting #ifdefs around duplicate code.

The only way to move forward for me is (a) to resolve conflicts with DRM, ideally in one go and bump (rather than 10) or (b) break DRM. I've been really trying hard the last weeks not to have to do (b). I can alternatively give you another big dump of my linuxkpi directory as I did in the past and let you guys sort it out.. it's just going to get bigger and won't be able to merged until the parts of this is solved.

Can't we simply do the resource tracking for all the "m" functions in head a week or two after these macros went in? I'll be more than happy if I can finally unload and re-load my driver without a reboot in between as well. But I also need to track PCI resources and other things as mentioned above, not just mallocs. And I don't want to test this then against DRM while I am still resolving conflicts. But you'll get these things for free to test independently of conflicting code and can focus on the DRM parts.

sys/compat/linuxkpi/common/include/linux/pm.h
37

Thank you!

sys/compat/linuxkpi/common/include/linux/device.h
562

I understand, but I still think we should go the extra mile and implement these functions properly. At least add a global STAILQ_HEAD() where all these allocations are listed. When we unload these are flushed.

It is not much effort. (size) + sizeof(void *).

Do you want me to fix it?

bz marked an inline comment as done.Oct 2 2020, 7:15 PM
bz added inline comments.
sys/compat/linuxkpi/common/include/linux/device.h
562

No, as I said, the extra nile is not the extra yard.

If this is done it needs to be done properly for other resources as well. I started to look into that and put a separate review up for it to make you feel more comfortable that it'll be done.

Otherwise I'll just had to deal with changing everything again in a week or two.

But thanks for the offer.

sys/compat/linuxkpi/common/include/linux/kobject.h
44

Is this comment useful ?

159

The first half of the last paragraph makes quite good note to put there, IMO.

sys/compat/linuxkpi/common/include/linux/pci.h
1073

Why this comment ? Why is 0 significant for drivers ?

sys/compat/linuxkpi/common/include/linux/device.h
572

Not sure we want to add those functions right now.
We need to propertly track all those malloc in the device struct in some way by either adding the devres stuff if it's also used directly by some drivers or doing our own stuff.

I had no idea what devres was. Sounds like "we are too lazy to properly write drivers" or "we are smart and let some common code do all the hard work for us even if it means we might hold unneeded resources for ages". Would be interesting to "profile" hit rates on these resources one day (sorry there's a researcher here in me ;-).

It seems drm does use devres; at least I saw mentions of it.
My current set of wifi drivers don't seem to.
I'll go and see what I can extract from the non-GPL drm/driver code in terms of API and start using that.
My initial idea was to hang it off the kobj/dev somewhere and then deal with it there.
Might be a better way than what I thought to do and certainly better than just adding a tailq for mallocs.

bdragon added inline comments.
sys/compat/linuxkpi/common/include/linux/pci.h
1073

IIRC, On POWER8 / POWER9, each pci slot on the board has its own root complex, which makes it a separate domain, so, no, you should not assert that it is 0.

Intel machines tend to stick everything on one root complex.