Page MenuHomeFreeBSD

DRM: Add DRM core files and DRMKPI
Needs ReviewPublic

Authored by manu on Jan 8 2020, 12:41 PM.

Details

Reviewers
imp
mmel
hselasky
johalun
Group Reviewers
ARM
arm64
Core Team
Summary

Import DRM core from Linux 5.4(-ish).
For ARM and ARM64 driver we need DRM in base for a differents reasons :

  • For embedded product we might want to have driver in the kernel
  • Linux drivers are GPL licenced and thus can only be used as kernel modules
  • The frameworks for FDT based resource is very different between Linux and FreeBSD, making a linuxkpi abstraction really difficult
  • Drivers need to be synced with DTS, which we import quickly after a Linux release is out.

Directories are structured as :

  • core/ : The drm core files and include, this matches was is present in Linux at drivers/gpu/drm/ and include/drm
    • freebsd/ : FreeBSD specific files, this includes some helpers that cannot be OS agnostic (like gem_cma/fb_cma/gem_frambuffer), some code for framebuffer<->VT relation (and VT switching) and the sysctl/sysfs needed code for drm core.
    • drmkpi/ : A somewhat stripped down copy of linuxkpi The reasons for having a copy are :
    • The full linuxkpi is usefull for importing drivers from Linux, but for FreeBSD drivers it adds some useless stuff like a linux_dev for each device.
    • We don't need nor want sysfs, netdevice code or other stuff not needed for DRM.
      • It should be self-contained for the DRM drivers and subsystem (which is partialy done).
      • bridges/ : This is where drm_bridges device will live, for now it only contain a generic interface for them.

All drm related sysctls are under dev.drm (like dev.drm.debug)
VT switching works even when we panic (tested with sysctl debug.kdb.panic=1)
The only missing pieces for a full sync with Linux 5.4 are the recent changes
in the dma-buf and drm_objsync (like the timeline feature).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

manu created this revision.Jan 8 2020, 12:41 PM
hselasky added a comment.EditedJan 8 2020, 12:55 PM

drmkpi/ : A somewhat stripped down copy of linuxkpi The reasons for having a copy are :
The full linuxkpi is usefull for importing drivers from Linux, but for FreeBSD drivers it adds some useless stuff like a linux_dev for each device.

I really don't like this.

How many are the differences?

The currently layout of sys/compat/linuxkpi , was exactly made so that you could overlay different versions of LinuxKPI, having one base LinuxKPI. We should try to build on that. Your approach likely also breaks the kernel linker, which currently doesn't care if you have multiple symbols with same name. What happens if you load drmkpi and linuxkpi at the same time?

Please integrate drmkpi within the sys/compat/linuxkpi folder, like this:

1) Create sys/compat/linuxkpi/basedrm/xxxx  include and src
2) use #include_next <> where appropriate.

--HPS

manu added a comment.Jan 8 2020, 1:00 PM

drmkpi/ : A somewhat stripped down copy of linuxkpi The reasons for having a copy are :
The full linuxkpi is usefull for importing drivers from Linux, but for FreeBSD drivers it adds some useless stuff like a linux_dev for each device.

I really don't like this.
How many are the differences?
The currently layout of sys/compat/linuxkpi , was exactly made so that you could overlay different versions of LinuxKPI, having one base LinuxKPI. We should try to build on that. Your approach likely also breaks the kernel linker, which currently doesn't care if you have multiple symbols with same name. What happens if you load drmkpi and linuxkpi at the same time?
Please integrate drmkpi within the sys/compat/linuxkpi folder, like this:

1) Create sys/compat/linuxkpi/basedrm/xxxx  include and src
2) use #include_next <> where approprate.

--HPS

That would mean creating a .h for every file but I guess this is cleaner, I'll have a look at this.

In D23085#505935, @manu wrote:

drmkpi/ : A somewhat stripped down copy of linuxkpi The reasons for having a copy are :
The full linuxkpi is usefull for importing drivers from Linux, but for FreeBSD drivers it adds some useless stuff like a linux_dev for each device.

I really don't like this.
How many are the differences?
The currently layout of sys/compat/linuxkpi , was exactly made so that you could overlay different versions of LinuxKPI, having one base LinuxKPI. We should try to build on that. Your approach likely also breaks the kernel linker, which currently doesn't care if you have multiple symbols with same name. What happens if you load drmkpi and linuxkpi at the same time?
Please integrate drmkpi within the sys/compat/linuxkpi folder, like this:

1) Create sys/compat/linuxkpi/basedrm/xxxx  include and src
2) use #include_next <> where approprate.

--HPS

That would mean creating a .h for every file but I guess this is cleaner, I'll have a look at this.

If the files are identical, then you don't need any wrapper, using include "basedrm" first, then "common" afterwards.

It is great you got things working!

Extend the base LinuxKPI source functions with the flags you need, like don't create linux_dev and have one wrapper in common and one in basedrm.

For example:

common/include/xxx.h

#if defined(LINUXKPI_BASEDRM)
Do it this way
#else
Do it that way
#endif

basedrm/include/xxx put the DRM header files here which you need.

Static inline functions is your friend.

Maybe Johannes has some opinions too.

Is it possible to add this driver as part of the existing KMS DRM driver?

--HPS

Also keep in mind that we can split out network related functions from the LinuxKPI, in an own linuxkpi_net .

Like:

linuxkpi_base.ko
linuxkpi_net.ko
linuxkpi_drm.ko
linuxkpi_gplv2.ko

and so on.

This would be the right approach forward.

manu added a comment.Jan 8 2020, 1:18 PM
In D23085#505935, @manu wrote:

drmkpi/ : A somewhat stripped down copy of linuxkpi The reasons for having a copy are :
The full linuxkpi is usefull for importing drivers from Linux, but for FreeBSD drivers it adds some useless stuff like a linux_dev for each device.

I really don't like this.
How many are the differences?
The currently layout of sys/compat/linuxkpi , was exactly made so that you could overlay different versions of LinuxKPI, having one base LinuxKPI. We should try to build on that. Your approach likely also breaks the kernel linker, which currently doesn't care if you have multiple symbols with same name. What happens if you load drmkpi and linuxkpi at the same time?
Please integrate drmkpi within the sys/compat/linuxkpi folder, like this:

1) Create sys/compat/linuxkpi/basedrm/xxxx  include and src
2) use #include_next <> where approprate.

--HPS

That would mean creating a .h for every file but I guess this is cleaner, I'll have a look at this.

If the files are identical, then you don't need any wrapper, using include "basedrm" first, then "common" afterwards.

I've just did some basic test and I think I can update the patch using this technique in a few days.

It is great you got things working!

It has been working for almost a year for me and more for @mmel :)

Extend the base LinuxKPI source functions with the flags you need, like don't create linux_dev and have one wrapper in common and one in basedrm.
For example:

common/include/xxx.h
#if defined(LINUXKPI_BASEDRM)
Do it this way
#else
Do it that way
#endif
basedrm/include/xxx put the DRM header files here which you need.

Static inline functions is your friend.
Maybe Johannes has some opinions too.
Is it possible to add this driver as part of the existing KMS DRM driver?

What do you mean ?

--HPS

manu added a comment.Jan 8 2020, 1:20 PM

Also keep in mind that we can split out network related functions from the LinuxKPI, in an own linuxkpi_net .
Like:
linuxkpi_base.ko
linuxkpi_net.ko
linuxkpi_drm.ko
linuxkpi_gplv2.ko
and so on.
This would be the right approach forward.

That might be a good solution yes.
For now this doesn't work as a module, the main reason is that I could never make my drm driver works as a module for a few reasons :

  • This is a multi driver thing and when in the same kernel module I couldn't find the right incatation for driver ordering
  • When in multi-part module I have some weird issue with interfaces between them (like I cannot call the interface function of another module).
Is it possible to add this driver as part of the existing KMS DRM driver?

What do you mean ?

I mean as a part of https://github.com/FreeBSDDesktop/kms-drm .

--HPS

That might be a good solution yes.
For now this doesn't work as a module, the main reason is that I could never make my drm driver works as a module for a few reasons :

Doesn't matter. Just make corresponding kernel options, like the existing COMPAT_LINUXKPI .

options COMPAT_LINUXKPI_BASE
options COMPAT_LINUXKPI_NET
options COMPAT_LINUXKPI_DRM

...

and so on.

manu added a comment.Jan 8 2020, 1:39 PM
Is it possible to add this driver as part of the existing KMS DRM driver?

What do you mean ?

I mean as a part of https://github.com/FreeBSDDesktop/kms-drm .
--HPS

No, why would I do that ?

manu added a comment.Jan 8 2020, 1:39 PM

That might be a good solution yes.
For now this doesn't work as a module, the main reason is that I could never make my drm driver works as a module for a few reasons :

Doesn't matter. Just make corresponding kernel options, like the existing COMPAT_LINUXKPI .
options COMPAT_LINUXKPI_BASE
options COMPAT_LINUXKPI_NET
options COMPAT_LINUXKPI_DRM
...
and so on.

Yup.

In D23085#505951, @manu wrote:

I mean as a part of https://github.com/FreeBSDDesktop/kms-drm .
--HPS

No, why would I do that ?

It might be easier to maintain in the future. I'm just asking. I don't know all the details.

--HPS

mmel added a comment.Jan 8 2020, 2:00 PM

common/include/xxx.h
#if defined(LINUXKPI_BASEDRM)
Do it this way
#else
Do it that way
#endif
basedrm/include/xxx put the DRM header files here which you need.

Static inline functions is your friend.
Maybe Johannes has some opinions too.
Is it possible to add this driver as part of the existing KMS DRM driver?

I don't agree with this. By my experience, static inline functions and #defines overriding standard freebsd functions creates very unlovable environment.
Our goal for drmkpi is not to emulate linux (mainly not task, files,etc...) behavior but create a library for translation of basic linux primitives to freebsd ones (semaphores, mutexes) where its posible, nothing more.
I plan to trying next step by moving all function to C files strictly prefixed by drmpki_<foo>. The rename header (by using #defines or inline wrapper) will be included only to original linux files, and it will not be include any of freebsd headers. This allow us to separate linux and freebsd world.

Also, please, don't take this that linuxkpi critique, I fully understand why you chosed actual implementation style. We simply have slightly different conditions a so we must take drmkpi as adaptation, not emulation, layer.

manu added a comment.Jan 8 2020, 3:28 PM

I've pushed some change here : https://github.com/evadot/freebsd/commits/drm_arm_v5.4-rebase-linuxkpi

This moves drmkpi files into sys/compat/linuxkpi
This uses the headers from common/include/ when we can.
This uses the linux_*.c files when we can.
I've only done that so @hselasky can see more the diff between linuxkpi and drmkpi.

OK, I'll see if I can spend some time tomorrow to look at the code. Thank you!

bcr added a subscriber: bcr.Jan 8 2020, 4:20 PM

Do you need any specific actions from the core team or was core only added as reviewer because Phabricator tried to be smart (too much mention of the word "core" in a different context)?
Let us know if we need to discuss/decide something.

manu added a comment.Jan 8 2020, 4:31 PM
In D23085#505975, @bcr wrote:

Do you need any specific actions from the core team or was core only added as reviewer because Phabricator tried to be smart (too much mention of the word "core" in a different context)?
Let us know if we need to discuss/decide something.

core was added because some files are dual licenced GPL/MIT and there is an herald rule that adds core for anything that matches GPL.
We will first fix the technical details and if core is needed we'll ping you :)

bcr added a comment.Jan 8 2020, 4:48 PM
In D23085#505979, @manu wrote:
In D23085#505975, @bcr wrote:

Do you need any specific actions from the core team or was core only added as reviewer because Phabricator tried to be smart (too much mention of the word "core" in a different context)?
Let us know if we need to discuss/decide something.

core was added because some files are dual licenced GPL/MIT and there is an herald rule that adds core for anything that matches GPL.
We will first fix the technical details and if core is needed we'll ping you :)

Ah yes, I found the herald rules. Cool, we'll be in touch.

Is the ports drm graphics drivers intended to be switched over to this framework, or is it only for ARM kernel drivers?

manu added a comment.Jan 9 2020, 11:05 AM

Is the ports drm graphics drivers intended to be switched over to this framework, or is it only for ARM kernel drivers?

I don't plan to do anything in the port drm driver

Can you use the port-drm driver at the same time as base DRM? Some people use PCI graphics adapters with ARM.

manu added a comment.Jan 9 2020, 11:31 AM

Can you use the port-drm driver at the same time as base DRM? Some people use PCI graphics adapters with ARM.

If they are at the same version that could work but I don't plan to work on that.
There is one board that FreeBSD kinda support with a PCIe controller that support graphics card and it's the macchiatobin which cost $500.
While my allwinner driver supports any H3/H5/A64 boards (there is something like ~40 boards).
@mmel nvidia driver will add support to any nvidia board that we already supports.
And I'll start a rockchip driver soon.
And by "some people" you mean one guy.

We simply have slightly different conditions a so we must take drmkpi as adaptation, not emulation, layer.

Why not keep the emulation for everything that's already in linuxkpi/drm-kmod, while adding adaptation for the FDT stuff and other missing parts?!

Adding a completely different copy of drm_*.c from what is used on desktop feels like a mess and duplicate work.

There is one board that FreeBSD kinda support with a PCIe controller that support graphics card and it's the macchiatobin which cost $500.

Well, $339 plus shipping but with a DDR4 stick.
The HoneyComb LX2K might work too (they did the ECAM thing!), and the expensive servers (eMAG, ThunderX/2) should work as well btw.

@mmel nvidia driver will add support to any nvidia board that we already supports.

Only tegra display or nouveau too?
Nouveau is for both tegra and desktop… this is why it really sucks that embedded and desktop drivers are in separate worlds >_<

And I'll start a rockchip driver soon.

Have you seen NetBSD's rockchip driver btw?

manu added a comment.Jan 10 2020, 5:12 PM

We simply have slightly different conditions a so we must take drmkpi as adaptation, not emulation, layer.

Why not keep the emulation for everything that's already in linuxkpi/drm-kmod, while adding adaptation for the FDT stuff and other missing parts?!

To what goal ? We don't want to use the linux driver.

Adding a completely different copy of drm_*.c from what is used on desktop feels like a mess and duplicate work.

There is one board that FreeBSD kinda support with a PCIe controller that support graphics card and it's the macchiatobin which cost $500.

Well, $339 plus shipping but with a DDR4 stick.
The HoneyComb LX2K might work too (they did the ECAM thing!), and the expensive servers (eMAG, ThunderX/2) should work as well btw.

And for thoses boards it's better to use the drm-kmod port with radeon/amdgpu.
That does raise the question : do we want multiple GENERIC-like config, one for "server grade" one for SBC.

@mmel nvidia driver will add support to any nvidia board that we already supports.

Only tegra display or nouveau too?
Nouveau is for both tegra and desktop… this is why it really sucks that embedded and desktop drivers are in separate worlds >_<

And I'll start a rockchip driver soon.

Have you seen NetBSD's rockchip driver btw?

manu added a comment.Jan 16 2020, 9:30 PM

OK, I'll see if I can spend some time tomorrow to look at the code. Thank you!

Did you had time to look at this ?

Cheers,

Sorry, I need more time to review this.

Probably I asked already, but can the DRMKPI be used at the same time another driver is using the LinuxKPI?

manu added a comment.Jan 17 2020, 9:37 AM

Probably I asked already, but can the DRMKPI be used at the same time another driver is using the LinuxKPI?

Not currently but this needs to be addressed before commit.

The DRMKPI needs to be tighter integrated with the current LinuxKPI, before this part can be upstreamed in my opinion.

My main worry is the DRMKPI will not be maintained when bugs are found in the LinuxKPI and/or kernel and/or Linux upstream.

Can you update this patch with the current changes integrating DRMKPI with LinuxKPI?

I believe some work was already done in this area.