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

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

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

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 edited edge metadata.

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

Thanks for all the comments! :-)

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"

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

jmd marked 4 inline comments as done.Mar 30 2017, 3:49 AM

Comments marked done.

jmd edited the test plan for this revision. (Show Details)
jmd added reviewers: swills, x11.

Add radeon KMS module

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

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.

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.

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.

This revision now requires changes to proceed.Apr 10 2017, 1:22 PM
This revision is now accepted and ready to land.May 2 2017, 12:59 PM
kwm added a subscriber: kwm.

LGTM

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

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

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

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?

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.

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.

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?

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

This revision is now accepted and ready to land.Aug 28 2017, 8: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
This revision is now accepted and ready to land.Aug 29 2017, 8:23 AM
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 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
This revision was automatically updated to reflect the committed changes.