The KMS DRM bits of the FreeBSDDesktop github.
Details
Details
- Reviewers
mat swills • hselasky kwm - Group Reviewers
x11 - Commits
- rP448928: New port: graphics/drm-next-kmod.
successful dogfooding on drmless branch of FreeBSDDesktop w/ amdgpu; successful running on AMD Carrizo against recent HEAD.
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I don't think it is worth renaming as there won't be collision, one would be in /boot/modules the other in boot/kernel
| graphics/kms-drm/Makefile | ||
|---|---|---|
| 5 ↗ | (On Diff #26300) | For ports where upstream does not define version numbers, and coming from git, it is customary to prepend 'g' to the date based version, something like this: PORTVERSION= g20170228 So that, if, one day, upstream starts using a version, say, 1.0, it will just work. $ pkg version -t g20170228 1.0 < $ pkg version -t 20170228 1.0 > |
| 6 ↗ | (On Diff #26300) | This does not do anything, remove it. |
| 21 ↗ | (On Diff #26300) | This is the default and should not be set. |
| graphics/kms-drm/pkg-descr | ||
| 1–2 ↗ | (On Diff #26300) | Those two lines should be removed. |
Comment Actions
Added the generic drm module, addressed comments by mat@, renamed i915kms.
Thanks for all the comments! :-)
Comment Actions
Now that you added drm the Makefile should be modified so that each drm headers could be found inside the work dir rather that trying to figure out them in the SRC directory
Btw SRCTOP is not defined in the ports tree meaning you got error like this:
<built-in>:1:10: fatal error: '/sys/dev/drm/drm_os_config.h' file not found #include "/sys/dev/drm/drm_os_config.h"
Comment Actions
Add some headers to the github referenced. Fix licensing, there are some GPLv2 licensed files.
Note: this will still not compile on HEAD due to missing linuxkpi functionality or it being not compatible (e.g. rbtree).
Comment Actions
Consider adding -kmod suffix as is common (although not universal) for ports that install kernel modules. Also, since there are kms drm drivers in the kernel, it may be nice to further distinguish these in name, e.g. drm-next-kmod or drm-dri3-kmod.
| graphics/kms-drm/Makefile | ||
|---|---|---|
| 14 ↗ | (On Diff #27074) | Rather than than depend on a random file you can use the package name, i.e. RUN_DEPENDS= kms-firmware>0:graphics/kms-firmware, which also lets you specify a version requirement. |
Comment Actions
given how the K in kms is kernel, kmod suffix seems redundant, no? Concerning versioning: assuming the remaining lkpi bits make it to CURRENT soon, we will start looking into drm 4.10 (johalun already offered). That will probably require again extensions to lkpi. So once this back and forth settles, kms-drm is more stable (there are still lots of known issues), and CURRENT is closer to become 12-STABLE, I will branch off whatever is in kms-drm as "kms-drm-X.YZ" and we will use this for the latest KMS DRM to only work with recent CURRENT/lkpi (unless lkpi gets backported). I guess this port could be named kms-drm-next or kms-drm-devel but kms-drm-devel-kmod seems like a mouthful.
| graphics/kms-drm/Makefile | ||
|---|---|---|
| 14 ↗ | (On Diff #27074) | Good point concerning the generic import. I think kms-firmware is good. The firmware is not useful beyond KMS and will be loaded by KMS. |
Comment Actions
No, it's not redundant, or I wouldn't have suggested it. Kernel Mode Setting doesn't tell me that this will install kernel modules as -kmod does. There is also libkms.so installed by libdrm which has the same K in the name but is not a kernel module. The suffix has practical benefit, for example after installkernel I can just run portmaster *-kmod if ports are named consistently. Perhaps drm-${VERSION}-kmod and drm-devel-kmod would make sense? Not such a mouthful without an extraneous prefix.
I think kms-firmware is good. The firmware is not useful beyond KMS and will be loaded by KMS.
The point seems to have been missed. Heck, the statement "loaded by KMS" doesn't even make sense. KMS is merely a feature introduced in DRI2 and implemented in the DRM drivers for recent GPUs. This is firmware that will be used by a few DRM drivers which happen to implement KMS but not exclusively. It is more logical to name the firmware according to the hardware it's for rather than a particular feature provided by the drivers that will be used. It may be best to go further and separate the firmware into multiple ports, i.e. amd-gpu-firmware-kmod and intel-gpu-firmware-kmod, as I expect they are covered by different licenses, will be updated at different times, etc. I had not said it earlier but the firmware port(s) should also have the -kmpd suffix for consistency, consider for example net/{bwi,bwn,malo}-firmware-kmod.
Comment Actions
Hi!
I agree with @rezny on the usefulness of "kms" in the name. That feature was a huge turn in the graphics drivers, but it remains just one aspect of them nonetheless. Having the words "drm" and "kmod" is important in the name, plus possibly something about the branch if we want to have several ports at the same time in the Ports tree. So drm-kmod or drm-4.9-kmod look like good candidates to me.
Comment Actions
I see people accepting, but for me it does not build at all on latest freebsd current, is that only me?
Comment Actions
No, that's to be expected. As from the summary: "Still requires linuxkpi changes not yet upstreamed, OSVERSION check will be upgraded then."
Hope this helps!
Comment Actions
DEBUG_FLAGS is handled by WITH_DEBUG logic in Mk/bsd.port.mk which is supported by every port that respects CFLAGS. To explicitly advertise it adding OPTIONS_DEFINE=DEBUG should be enough. Unfortunately, it would still leave out stuff like INVARIANTS (e.g., for KASSERT, mtx_assert) or WITH_CTF (DTrace) unless one builds via PORTS_MODULES per build(7).
| graphics/drm-next-kmod/Makefile | ||
|---|---|---|
| 26 ↗ | (On Diff #32329) | Maybe use <bsd.port.options.mk> instead. Nothing later depends on USE_* or USES defining special variables. |
| graphics/drm-next-kmod/pkg-descr | ||
| 1 ↗ | (On Diff #32329) | Isn't this line redundant with COMMENT in Makefile? |
| graphics/drm-next-kmod/Makefile | ||
|---|---|---|
| 11 ↗ | (On Diff #32329) | Why MIT? What does it mean to have two licenses specified here? Most of the code is dual BSD/GPLv2-licensed, but some of it is just GPLv2. |
| graphics/drm-next-kmod/Makefile | ||
|---|---|---|
| 11 ↗ | (On Diff #32329) | Sorry, MIT is indeed right. Again, though, not all of the code is dual-licensed. |
Comment Actions
Addresses comments made. Adds linuxkpi_gplv2.ko module. Shuffles some of the Makefile around that newer portlint complains about (note: it still complains about IGNORE being too late and ARCH_REASON, however, this is as high as I can put them).
Comment Actions
Comments addressed.
| graphics/drm-next-kmod/Makefile | ||
|---|---|---|
| 11 ↗ | (On Diff #32329) | note that it uses multi (i.e., code base has multiple licenses), which I think is fitting. Thanks for the heads-up that there are also BSD2CLAUSE pieces. Added that license. |
| graphics/drm-next-kmod/Makefile | ||
|---|---|---|
| 23–25 ↗ | (On Diff #32357) | That should probably be: IGNORE_FreeBSD_10= not supported on 11.x or older, no kernel support IGNORE_FreeBSD_11= not supported on 11.x or older, no kernel support That way you do not need to include bsd.port.options.mk. |
| graphics/drm-next-kmod/Makefile | ||
|---|---|---|
| 23–25 ↗ | (On Diff #32357) | The issue is that not all revisions of HEAD do even compile this port. It depends on changes to the linuxkpi that were only recently upstreamed. Hence the version check. What's your recommendation? |
Comment Actions
Update diff to, after conversation w/ swills, also explicitly mark IGNORE on non-FreeBSD.
Comment Actions
Incorporate changes requested by kwm: uidfix for building, reorder the Makefile again, and add a pkg-message with usage information and an example for loading the modules properly.
| graphics/drm-next-kmod/pkg-message | ||
|---|---|---|
| 2 ↗ | (On Diff #32471) | is amdgpu replacing radeonsi completly or does it compliment it? If it does compilment it it should be listed with which GPU's families they support? Just GPU mention the border GPU's, like radeonsi supports until xxxx and amdgpu supports newer versions. And name i915, i915kms as that is how the modules is named. |
| 5 ↗ | (On Diff #32471) | I think it might be beter to use either amdgpu or i915kms module with the full path in the example. This might otherwise suggest that you could just put "i915kms" in there. And end up loading the base version. |