Page MenuHomeFreeBSD

KMS DRM port
ClosedPublic

Authored by jmd on Mar 16 2017, 3:32 AM.

Details

Summary

The KMS DRM bits of the FreeBSDDesktop github.

Test Plan

successful dogfooding on drmless branch of FreeBSDDesktop w/ amdgpu; successful running on AMD Carrizo against recent HEAD.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bapt added a subscriber: bapt.Mar 16 2017, 3:42 AM

The drm module itself should probably be added there as well imho

jmd updated this revision to Diff 26300.Mar 16 2017, 3:46 AM

Rename i915kms,ko to drm3_i915_kms.ko to avoid the name collision with base.

bapt added a comment.Mar 16 2017, 3:48 AM

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

mat requested changes to this revision.Mar 16 2017, 1:07 PM
mat added inline comments.
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.

This revision now requires changes to proceed.Mar 16 2017, 1:07 PM
jmd updated this revision to Diff 26340.Mar 16 2017, 11:35 PM
jmd edited edge metadata.

Added the generic drm module, addressed comments by mat@, renamed i915kms.

Thanks for all the comments! :-)

bapt added a comment.Mar 17 2017, 6:18 AM

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"
jmd updated this revision to Diff 26621.Mar 24 2017, 1:54 AM

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).

swills added a subscriber: swills.Mar 29 2017, 5:54 PM
jmd marked 4 inline comments as done.Mar 30 2017, 3:49 AM

Comments marked done.

jmd updated this revision to Diff 27074.Apr 5 2017, 2:37 AM
jmd edited the test plan for this revision. (Show Details)
jmd added reviewers: swills, x11.

Add radeon KMS module

rezny added a subscriber: rezny.Apr 5 2017, 3:46 AM

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.
As an aside, kms-firmware doesn't seem like the best name, it implies this is firmware for kms when it's firmware for various GPUs. Suggestion: gpu-firmware

jmd added a comment.Apr 5 2017, 4:36 AM

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.

rezny added a comment.Apr 5 2017, 5:39 AM
In D10021#212683, @jmd wrote:

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.

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.

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.

jmd updated this revision to Diff 27125.Apr 6 2017, 1:20 AM

Change port name according to @rezny and @dumbbell 's suggestions. Use drm-next-kmod, eventual stable releases to be called drm-X.YZ-kmod.

Depend on the gpu-firmware-kmod port.

swills accepted this revision as: swills.Apr 10 2017, 1:22 PM

Approved

This revision now requires changes to proceed.Apr 10 2017, 1:22 PM
emaste added a subscriber: emaste.Apr 24 2017, 2:00 AM
mat accepted this revision.May 2 2017, 12:59 PM
This revision is now accepted and ready to land.May 2 2017, 12:59 PM
kwm accepted this revision as: x11.May 19 2017, 7:16 AM
kwm added a subscriber: kwm.

LGTM

bapt added a comment.May 21 2017, 8:24 PM

I see people accepting, but for me it does not build at all on latest freebsd current, is that only me?

jmd added a comment.May 21 2017, 11:04 PM

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!

jmd updated this revision to Diff 32329.Aug 23 2017, 3:13 AM
jmd edited edge metadata.
jmd added subscribers: hselasky, markj.

Thanks to @markj and @hselasky the lkpi changes were upstreamed. This version of drm-next now compiles against HEAD. Version check is upgraded to the ID as of today.

This revision now requires review to proceed.Aug 23 2017, 3:13 AM
hselasky accepted this revision.Aug 23 2017, 7:19 AM

Can you add a port option to set DEBUG_FLAGS="-g" ??

This revision is now accepted and ready to land.Aug 23 2017, 7:19 AM
jbeich added a subscriber: jbeich.Aug 23 2017, 10:21 AM

Can you add a port option to set DEBUG_FLAGS="-g" ??

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?

markj added inline comments.Aug 23 2017, 3:58 PM
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.

markj added inline comments.Aug 23 2017, 4:41 PM
graphics/drm-next-kmod/Makefile
11 ↗(On Diff #32329)

Sorry, MIT is indeed right. Again, though, not all of the code is dual-licensed.

jmd updated this revision to Diff 32357.Aug 24 2017, 4:28 AM
jmd edited edge metadata.
jmd edited the summary of this revision. (Show Details)
jmd edited the test plan for this revision. (Show Details)

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).

This revision now requires review to proceed.Aug 24 2017, 4:28 AM
jmd marked 3 inline comments as done.Aug 24 2017, 4:30 AM

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.

mat added inline comments.Aug 24 2017, 7:03 AM
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.

jmd marked an inline comment as done.Aug 24 2017, 3:01 PM
jmd added inline comments.
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?

jmd updated this revision to Diff 32394.Aug 25 2017, 6:29 PM

Update diff to, after conversation w/ swills, also explicitly mark IGNORE on non-FreeBSD.

hselasky accepted this revision.Aug 28 2017, 8:53 AM
This revision is now accepted and ready to land.Aug 28 2017, 8:53 AM
jmd updated this revision to Diff 32471.Aug 29 2017, 2:53 AM
jmd edited edge metadata.

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.

This revision now requires review to proceed.Aug 29 2017, 2:53 AM
hselasky accepted this revision.Aug 29 2017, 8:23 AM
This revision is now accepted and ready to land.Aug 29 2017, 8:23 AM
kwm added inline comments.Aug 29 2017, 2:57 PM
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.

jmd updated this revision to Diff 32484.Aug 29 2017, 3:19 PM
jmd edited edge metadata.

Address kwm's comments (thanks!) concerning pkg-message.

This revision now requires review to proceed.Aug 29 2017, 3:19 PM
jmd marked 2 inline comments as done.Aug 29 2017, 3:19 PM
jmd marked 4 inline comments as done.
This revision is now accepted and ready to land.Aug 29 2017, 3:22 PM
swills accepted this revision.Aug 29 2017, 4:08 PM

Approved

This revision was automatically updated to reflect the committed changes.