Use drm_fb_helper's emulation of Linux fbdev to integrate with FreeBSD's vt(4)
#243
Conversation
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
7b69765 to
a4719f3
Compare
|
As mentioned in #236, there seem to be some issues with locking. The linuxkpi code wants to take a sleep lock and perform allocations in several places, which does not work well with the vtdev lock and window spinlocks. The first issue that comes up when you try running the new system with witness enabled is that After bluntly patching that, the next issue is that After patching that, the final issue is once again in vt_flush, this time with calling bitblt. Backtraces for the above: Here's the patches I came up with. I have no idea what invariants exactly these locks are supposed to guard, so I cannot comment on their correctness :) kernel: diff --git a/sys/dev/vt/vt_core.c b/sys/dev/vt/vt_core.c
index 267dd7bf2489..80002b9c21f9 100644
--- a/sys/dev/vt/vt_core.c
+++ b/sys/dev/vt/vt_core.c
@@ -1406,17 +1406,23 @@ vt_flush(struct vt_device *vd)
vd->vd_flags &= ~VDF_INVALID;
a = teken_get_curattr(&vw->vw_terminal->tm_emulator);
+ /*
+ * The below calls may need to run a sleep lock in order to
+ * dispatch linuxkpi work
+ */
+ vtbuf_unlock(&vw->vw_buf);
vt_set_border(vd, &vw->vw_draw_area, a->ta_bgcolor);
vt_termrect(vd, vf, &tarea);
if (vd->vd_driver->vd_invalidate_text)
vd->vd_driver->vd_invalidate_text(vd, &tarea);
if (vt_draw_logo_cpus)
vtterm_draw_cpu_logos(vd);
+ vtbuf_lock(&vw->vw_buf);
}
if (tarea.tr_begin.tp_col < tarea.tr_end.tp_col) {
- vd->vd_driver->vd_bitblt_text(vd, vw, &tarea);
vtbuf_unlock(&vw->vw_buf);
+ vd->vd_driver->vd_bitblt_text(vd, vw, &tarea);
return (1);
}drm-kmod: diff --git a/drivers/gpu/drm/vt_drmfb.c b/drivers/gpu/drm/vt_drmfb.c
index 116121e14f..62ef106d0c 100644
--- a/drivers/gpu/drm/vt_drmfb.c
+++ b/drivers/gpu/drm/vt_drmfb.c
@@ -257,6 +257,7 @@ vt_drmfb_postswitch(struct vt_device *vd)
{
struct fb_info *fbio;
struct linux_fb_info *info;
+ int locked = mtx_owned(&vd->vd_lock);
fbio = vd->vd_softc;
info = to_linux_fb_info(fbio);
@@ -266,8 +267,14 @@ vt_drmfb_postswitch(struct vt_device *vd)
}
if (!kdb_active && !KERNEL_PANICKED()) {
+ if (locked) {
+ VT_UNLOCK(vd);
+ }
linux_set_current(curthread);
info->fbops->fb_set_par(info);
+ if (locked) {
+ VT_LOCK(vd);
+ }
} else {
#ifdef DDB
db_trace_self_depth(10);
|
|
I'm not sure that releasing the window buffer can work in
What I'm experimenting with on my side is to change the lock type of the lock protecting the |
|
Could you post the patch which does that so I can test it? I found some fairly intense graphical corruption switching to hikari while driving my patches so I suspect you may be right. |
|
@rhelmot: The computer crashes hard with my wip patch unfortunately. I think the corruption you see in Hikari is unrelated. The vt(4) integration layer only drives the display of the console, the text terminals before starting an X.Org or Wayland session. Once under X.Org/Wayland, only the DRM drivers are used exclusively and there should be no corruption. Note that with the 5.17 drivers on my Intel 12th gen GPU, Wayland (using Sway) is unusable for me (only garbage on the screen). It works with 5.15-lts however, so this corruption must be fixed at some point in 5.18+. I need to take a picture of the screen and share it to compare with what you see. |
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
a4719f3 to
8d3c57e
Compare
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
2 times, most recently
from
21ae6a8 to
82ffa79
Compare
|
In case this helps: I have similar console problems in Void Linux with kernel 6.1 and the Nvidia proprietary driver. Switching to a new virtual console with ctrl-alt-fnum works, and scrolls normally until you get to the bottom of the screen. It then appears to freeze, but switching back and forth between consoles causes it to refresh. |
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
82ffa79 to
7908237
Compare
|
Hello! I've been away in July. I'm back home and slowly resume work on this vt(4) integration and the Linux 5.17 DRM drivers port. After polishing things a bit today, including commit history and commit messages, I'm starting to use the 5.17 DRM drivers on a daily basis. This includes the new vt(4) integration. We'll see how stable it is or not. Note that there is one thing left to work on: the support for masks in the bitblt operation. To display the mouse pointer in the console, FreeBSD uses a mask it passes to bitblt. Linux doesn't have such a thing. I think it supports an alpha channel but I'm not sure. Anyway, currently the mask is commented out in the new vt(4) integration which means the mouse pointer is never displayed for now. |
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
7908237 to
0cc6615
Compare
|
I have added mouse pointer mask support this morning. The new vt(4) integration is complete and is now ready for wider testing. I admit I only tested it as part of the Linu 5.17 port so far. Note that this new integration depends on changes committed to the Once I'm more confident with the quality, I will submit reviews to reviews.freebsd.org for the patchs to freebsd-src and update the pull request description with links to the reviews. |
|
@JustAnotherHumanBeing reported the following problem with the vt(4) integration in #236:
I just pushed some fixes to both freebsd-src and the update-to-v5.17 branch (which are meant for this pull request, just not had time to rebase yet). However, I didn't look at the issue reported above. I will look at it next. |
|
I found the problem with missing CPU logos. The nested loops in I'm still not satisfied with the overall patch though. Still doing some improvements before I push it again to GitHub. |
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
0cc6615 to
9a72b9b
Compare
|
Ok, new commits pushed. Compared to last week, I changed the overall approach. Initially, I changed vt(4) to support This worked well. Unfortunately, it meant that the screen was actually refreshed once after the entire buffer was updated. With the old integration, the screen was refreshed after each displayed character. Because of that, showing a lot of content like The new approach is based on a flag set by the driver. If the driver sets this flag, the This time, the driver can refresh the screen after each character and still run outside of the vt_buf-locked section. When I compare the speed at which cat(1) can write that big text file to the console, the new integration is a couple percent faster. We are talking 1.35 seconds with the old integration vs. 1.30 seconds with the new one for a file with 500,000 lines on my Intel 12th gen laptop. The problem reported by @JustAnotherHumanBeing is also fixed. I'm going to mark this pull request as ready to review again. |
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
2 times, most recently
from
a7b55ec to
499e1a0
Compare
|
I believe the instabilities I saw with the I'm not marking this pull request as ready for review yet. With the change of |
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
499e1a0 to
28337ab
Compare
|
The iwlwifi driver's suspend & resume support is not yet implemented, so my change to I pushed another change that make it possible to kldunload(8) the |
|
I'm using the 5.17 update on a daily basis, which includes this new vt(4) integration. I believe it's ready for wider testing and review! I didn't submit reviews for the patches to freebsd-src yet. Before I do that, I would love some feedback on the overall approach :-) |
|
Could this rewrite fix some issues I have with the nvidia driver and waking up from suspend to RAM (screen comes back up blank or all garbled, but if I start a new X server, that X server works (while previously started X servers do not)). |
|
I don't think it will fix anything related to the NVIDIA proprietary driver. I'm not sure it relies on DRM in the first place. |
|
I really would like to test on my Framework Gen 13 Laptop. Is there any guide how to do this? |
You could look at #236, but I doubt Intel 13th gen was already supported in Linux 5.17. |
|
I'm marking this pull request as a draft again. There are regressions around the interaction with kdb/ddb. I just discovered that In the process, I stumble upon a problem I saw in the past (but didn't have a solution for): |
|
The |
[Why] We may need that memory early enough that the async initialization may not be ready yet. [How] We stop using a SYSINIT() function and perform the initialization from `i915_init()` instead.
…ster_framebuffer() [Why] To correctly register the VRAM memory fictitious region, we relied on the fact that the region address and size was defined in `fb_info->apertures->ranges[0]`. The values were set by the driver when they initialized their framebuffer. Really, this was a by-product of the framebuffer initialization and the region must be registered regardless of that. Likewise for the accompagnying call to `vt_freeze_main_vd()`. Starting with Linux 5.17, the `amdgpu` doesn't fill the `fb_info->apertures->ranges[0]` values anymore anyway. It starts to use the generic framebuffer code which doesn't use them. Therefore, we need to have a FreeBSD-specific setup of that memory region outside of the framebuffer initialization/vt(4) integration code. [How] A new pair of functions is introduced in `drm_os_freebsd.c` to register and unregister the memory region, as well as calling `vt_(un)freeze_main_vd()` accordingly. The drivers need to call explicitly the new functions. We can't rely on a side effect of the framebuffer initialization code. As a bonus, the calls to vt_(un)freeze_main_vd()` are now symetrical everywhere. Before this change, drivers called `vt_unfreeze_main_vd()` at different places.
... except the code related specifically to sysrq and framebuffer
deferred I/O.
[Why]
In Linux 5.17, the amdgpu driver relies entirely on `drm_fb_helper` to
handle its framebuffer. Indeed, the framebuffer-specific code is
removed.
Also in Linux 5.17, something changed in the i915 driver. I don't know
exactly what, but the memory region passed to vt(4) and `vt_fb` isn't
the one displayed on the screen. Therefore, all writes made by `vt_fb`
leads to no changes on the screen and the computer looks frozen (that's
not the case). However, when I vt-switch to another tty, the screen is
refreshed to show the content of the tty active *before* the switch.
Switching tty is the only time `vt_fb` calls into DRM. There is enough
evidence that the i915 driver now assumes that the framebuffer is
accessed like `drm_fb_helper` does it, i.e. like a Linux fbdev device.
Unfortunately, our vt(4) integration is way way simpler than than Linux'
fbdev.
The goal of this commit and following ones is to change that vt(4)
integration layer to behave like fbdev.
[How]
The first step is uncomment everything in `drm_fb_helper` to make sure
that the API itself is ok.
`drm_fb_helper` depends on lower-level functions, especially
`fb_sys_{read,write}` and `fb_cfb_{read,write}` which are responsible
for all reads and writes to the allocated memory region for the
framebuffer. In this commit, they are simple stubs. They will be
implemented in a future commit.
As mentionned at the top, two areas are left commentted out:
* sysrq, a Linux specific mechanism to interact with the kernel at a
very low level. There is something close in FreeBSD, but we don't need
to worry about it in `drm_fb_helper.
* framebuffer deferred I/O, a framebuffer feature to flush the memory
region asynchronously. We can't really implement that on FreeBSD and I
believe we don't need to.
…r_framebuffer()` [Why] Before, it was scattered in several functions in `drm_fb_helper.c`, enclosed in `#ifdef __FreeBSD__`. It made it difficult to follow how the structure was initialized. Now, everything is centralized in a FreeBSD-specific source file. `register_framebuffer()` would redo the job for several struct fields anyway. [How] Everything is derived from Linux' `struct fb_info`, like before. Only a couple fields are initialized from `struct drm_fb_helper`. This means the lower-level `register_framebuffer()` functions knows about the upper DRM layer.
…elper` [Why] The goal is to make the framebuffer behavior closer to what happens on Linux. One major difference is that on FreeBSD, we used to pass the screen base address to `vt_fb` during initialization, then `vt_fb` would write directly to that memory region. This worked fine for years. Unfortunately, this broke somehow with the backport of DRM from Linux 5.17. At that point, the screen was not refreshed anymore, even though `vt_fb` wrote pixels to the correct memory region. The only time the screen was refreshed, it was during a vt-switch. The screen then displayed the content of the tty we just switched away, but still, something happened on the screen. And that's the only operation we called into `drm_fb_helper` from `vt_fb`. On Linux, `fbcon` calls into `fbdev`, emulated by `drm_fb_helper` for each access (read & write) to the framebuffer. It never reads or writes to the memory region on its own. [How] Therefore we need a new integration layer to make sure vt(4) calls into `drm_fb_helper` like Linux' fbcon does. We do this by adding a new vt(4) backend called `vt_drmfb`, which replaces `vt_fb`. `vt_drmfb` bridges between: * DRM's `drm_fb_helper` emulation of Linux' fbdev below, and * vt(4) console handling above In other words, it maps fbdev callbacks to vt(4)'s backend expected callbacks. `vt_drmfb` takes care of converting the vt(4) callbacks arguments to whatever is expected by the underlying fbdev emulation. Then, when it's time to write to the actual framebuffer memory, the implementation of `cfb_*` functions are derived from the `vt_fb` "setpixel" code. `vt_drmfb` is maintained inside drm-kmod and is compiled inside drm(4). It is not under `sys/dev/vt/hw`. The reason is that `vt_drmfb` needs to know the `drm_fb_helper` API and must evolve with it. `vt_drmfb` has another difference: it doesn't use a taskqueue(9) to call into `drm_fb_helper as `vt_fb` did. It performs the calls synchronously. `drm_fb_helper` already uses a Linux workqueue to make updates asynchronous with the underlying driver. This simplifies the integration a lot because the whole `fb_mode_task`-related machinery is removed from `drm_os_freebsd.c` and `linux_fb.c`. Because we don't need that taskqueue(9) anymore, this patch also gets rid of the `struct vt_kms_softc` indirection. Now, the `struct drm_fb_helper` pointed is stored directly into FreeBSD's `struct fb_info->fb_priv`.
…f one is already installed [Why] Some DRM drivers consider a failure from `registry_framebuffer()` as a fatal one. Also in the future, we may want to support multiple framebuffers at the same time, like Linux.
... compared to the attach order.
... instead of FreeBSD's `struct fb_info`. [Why] With the introduction of `vt_drmfb`, we try to use `struct linux_fb_info` for every parameters and all callbacks. This is to make sure we use the same values as Linux, just in case we forget to update the corresponding FreeBSD's `struct fb_info`.
[Why] At least in i915, it is NULL in my tests because the code behind it was removed a long time ago. We didn't execute this function before the introduction of vt_drmfb, so it didn't cause any problems.
[Why] With vt_drmfb, I believe they are not needed anymore.
dumbbell
force-pushed
the
fully-use-drm_fb_helper-on-freebsd
branch
from
28337ab to
63c6d03
Compare
|
I think the new vt(4) integration is good enough for the time being w.r.t. panic handling. At least it's not worse than the current one in my testing. We can try to improve it iteratively, but it shouldn't block 5.17 anymore IMHO. I'm marking this pull request as ready to review again. |
This change seems to break some drivers such as the mlx5*(4) drivers. As kib@ says: > According to the 'official' Linux kernel documentation, the GFP_KERNEL > flag implies sleepable context. It was introduced while working on the new vt(4)/DRM integration [1]. During this work, doing sleepable allocations broke vt(4) and the DRM drivers. However, I made further improvements and some locking-related fixed to the new integration without revisiting the need for it. After more testing, the improvements to the integration mentionned above seems to make the change to `GFP_KERNEL` unneeded now. I can thus revert it to restore expectations of other drivers. This reverts commit 14dcd40. [1] freebsd/drm-kmod#243 Reviewed by: kib Approved by: kib Differential Revision: https://reviews.freebsd.org/D42962
This change seems to break some drivers such as the mlx5*(4) drivers. As kib@ says: > According to the 'official' Linux kernel documentation, the GFP_KERNEL > flag implies sleepable context. It was introduced while working on the new vt(4)/DRM integration [1]. During this work, doing sleepable allocations broke vt(4) and the DRM drivers. However, I made further improvements and some locking-related fixed to the new integration without revisiting the need for it. After more testing, the improvements to the integration mentionned above seems to make the change to `GFP_KERNEL` unneeded now. I can thus revert it to restore expectations of other drivers. This reverts commit 14dcd40. [1] freebsd/drm-kmod#243 Reviewed by: kib Approved by: kib Differential Revision: https://reviews.freebsd.org/D42962
This patch series replaces our current vt(4) integration layer with a new one relying entirely on
drm_fb_helper.c.Why
In Linux 5.17, the amdgpu driver relies entirely on
drm_fb_helperto handle its framebuffer. Indeed, the framebuffer-specific code is removed.Also in Linux 5.17, something changed in the i915 driver. I don't know exactly what, but the memory region passed to vt(4) and
vt_fbisn't the one displayed on the screen. Therefore, all writes made byvt_fbleads to no changes on the screen and the computer looks frozen (that's not the case). However, when I vt-switch to another tty, the screen is refreshed to show the content of the tty active before the switch. Switching tty is the only timevt_fbcalls into DRM. There is enough evidence that the i915 driver now assumes that the framebuffer is accessed likedrm_fb_helperdoes it, i.e. like a Linux fbdev device.The symptoms are the same when we enable PSR in the i915 driver (
enable_psr=1). It's currently turned off by default on FreeBSD, unlike Linux.Unfortunately, our vt(4) integration is way more basic than than Linux' fbdev and doesn't reproduce the behavior of an fbdev device.
How
The branch contains two major parts:
drm_fb_helper.cand related code in DRM drivers. This makes sure that the code compiles, even though it is not used at that point.vt_drmfb, compiled indrm.ko. This new backend is responsible for bridging the vt(4) callbacks (upper layer) and the fbdev callbacks (lower layer).This work relies on patches to freebsd-src. They are available in the
linuxkpi-updates-for-drmbranch in my freebsd-src fork. Patches were submitted for reviews to Phabricator:https://reviews.freebsd.org/D42054https://reviews.freebsd.org/D42056https://reviews.freebsd.org/D42057https://reviews.freebsd.org/D42750https://reviews.freebsd.org/D42751https://reviews.freebsd.org/D42752TODO
vtbuf_lock()invt_flush()andWQ_EXEC_LOCK()inschedule_work()in linuxkpi (sleepable lock acquired inside a non-sleepable lock section).bitblt_imagecallback mask argument; there is no equivalent on the fbdev side.This work is required by the Linux 5.17 backport (#236).
Fixes #17.