nixos/hardware.display: init module #279789

nazarewk · 2024-01-09T10:15:23Z

Description of changes

While preparing a bug report for root cause of #279739 I came up with a possible way to fix it and confirmed it is indeed working.

https://github.com/NixOS/nixpkgs/blob/580d12f7dc941cad010f3295eda1ed0cb513611c/pkgs/build-support/kernel/modules-closure.sh seems to ignore any files that are not kernel modules

As a part of the discussion feedback I have created a NixOS module out of the whole thing for handling edid files and additional kernel-level display configuration.

Things done

Using it for a few weeks in my configurations https://github.com/nazarewk-iac/nix-configs

Add a 👍 reaction to pull requests you find important.

this allows us not to pass `compressFirmware = false;` inside EDID derivations as this mechanic very tricky to discover. related to NixOS#279789 related to NixOS#279739

this allows us not to pass `compressFirmware = false;` inside EDID derivations as this mechanic very tricky to discover. related to NixOS#279789

soupglasses

Works correctly with nice logging. Only nitpick i have is to document this specific handling of lib/firmware/edid somewhere. As it might not be clear to an end user that edids are required to be put into the edid folder to function correctly.

nazarewk · 2024-01-09T14:46:41Z

Works correctly with nice logging. Only nitpick i have is to document this specific handling of lib/firmware/edid somewhere. As it might not be clear to an end user that edids are required to be put into the edid folder to function correctly.

Yeah, I also had trouble with discovering where to put edids and how to link them together on the system, but I have no idea where to document it.

soupglasses · 2024-01-09T15:33:17Z

I am not sure either. But i could see a subsection like Using Custom EDID files under https://nixos.org/manual/nixos/stable/#sec-kernel-config could be possible. As its very close to where boot.initrd.kernelModules is taught.

K900 · 2024-01-09T18:06:15Z

At this point, why not a NixOS option that does all the right plumbing?

SuperSandro2000

I have no clue about edit but looking pretty good to my naive eyes and we can probably merge this with those small cleanups.

SuperSandro2000 · 2024-04-18T09:23:00Z

pkgs/by-name/li/linuxhw-edid-fetcher/package.nix

+#   let
+#     edids = (linuxhw-edid-fetcher.override {
+#       displays = {
+#         PG278Q_2014 = [ "PG278Q" "2014" ];
+#       };
+#     });
+#   in
+#     "${edids}/lib/firmware/edid/PG278Q_2014.bin";


Suggested change

# let

# edids = (linuxhw-edid-fetcher.override {

# displays = {

# PG278Q_2014 = [ "PG278Q" "2014" ];

# };

# });

# in

# "${edids}/lib/firmware/edid/PG278Q_2014.bin";

# let

# edids = linuxhw-edid-fetcher.override {

# displays.PG278Q_2014 = [ "PG278Q" "2014" ];

# };

# in

# "${edids}/lib/firmware/edid/PG278Q_2014.bin";

we can minimize that example a bit further, to make it easier to understand

actually I added (unnecessary) 2560x1440 to make it obvious PG278Q_2014.bin is derived from the object key, not a search path.

SuperSandro2000 · 2024-04-18T09:23:35Z

pkgs/by-name/li/linuxhw-edid-fetcher/package.nix

+    ''}
+  '';
+
+  passthru.updateScript = nix-update-script { extraArgs = [ "--version=branch=master" ]; };


Suggested change

passthru.updateScript = nix-update-script { extraArgs = [ "--version=branch=master" ]; };

passthru.updateScript = nix-update-script { extraArgs = [ "--version=branch=master" ]; };

SuperSandro2000 · 2024-04-18T09:24:24Z

pkgs/by-name/li/linuxhw-edid-fetcher/linuxhw-edid-fetch.sh

+    test -d "$1" && test -f "$1/AnalogDisplay.md" && test -f "$1/DigitalDisplay.md"
+}
+
+main() {


We could just drop the main function

I prefer to have an explicit entry point in form of main() because:

it's hard to follow scripts interweaving running code with function definitions

it's a lot harder to locate where the code starts executing

I just apply this "policy" to anything larger than ~10 lines of bash, as soon as you reach it you need helper functions etc.

SuperSandro2000 · 2024-04-18T09:24:43Z

pkgs/by-name/li/linuxhw-edid-fetcher/linuxhw-edid-fetch.sh

+        repo="${TMPDIR:-/tmp}/edid"
+        log "Not running inside 'https://github.com/linuxhw/EDID', downloading content to ${repo}"
+        if ! check_repo "$repo"; then
+            curl -L https://github.com/linuxhw/EDID/tarball/master | tar -zx -C "${repo}" --strip-components=1


Suggested change

curl -L https://github.com/linuxhw/EDID/tarball/master | tar -zx -C "${repo}" --strip-components=1

curl -L https://github.com/linuxhw/EDID/tarball/HEAD | tar -zx -C "${repo}" --strip-components=1

just in case they ever change the default branch

nice, wasn't aware of this

SuperSandro2000 · 2024-04-18T09:25:23Z

pkgs/by-name/li/linuxhw-edid-fetcher/linuxhw-edid-fetch.sh

+        exit 1
+    fi
+
+    : "${repo:="$PWD"}"


Shouldn't we rather use a tmp file for this?

it's just for detecting whether current directory is already checked out repository, if it's not it downloads stuff to /tmp/edid

SuperSandro2000 · 2024-04-18T09:26:19Z

nixos/modules/services/hardware/display.nix

+        };
+      }
+    )
+    (lib.mkIf (cfg.edid.packages != null) {


We should probably hide this behind en enable flag. Some modules, eg in nixos-hardware might fill this and having to use mkForce to clear it, is not that nice.

nazarewk · 2024-04-18T09:46:42Z

@SuperSandro2000 thanks for taking interest in it, I'm currently working on a separate script for doing the runtime configuration and would LOVE to merge after I got it to work.

Yesterday I updated nixos-unstable in my configs + swapped Radeon RX550 to Pro W6600 and the current script stopped working completely because kernel/amdgpu started doing additional wild stuff like trying to load it multiple times and turning off the display after failing to load EDID even though it was force enabled:

[   25.456331] snd_hda_intel 0000:0c:00.1: bound 0000:0c:00.0 (ops amdgpu_dm_audio_component_bind_ops [amdgpu])
[   25.770685] [drm] forcing DP-4 connector on
[   25.881959] amdgpu 0000:0c:00.0: [drm] *ERROR* [CONNECTOR:122:DP-4] Requesting EDID firmware "edid/PG278Q_120.bin" failed (err=-22)
[   25.890385] [drm:detect_link_and_local_sink [amdgpu]] *ERROR* No EDID read.
[   25.939326] [drm] kiq ring mec 2 pipe 1 q 0
[   25.944541] [drm] VCN decode and encode initialized successfully(under DPG Mode).
--
[   25.971560] amdgpu 0000:0c:00.0: amdgpu: ring jpeg_dec uses VM inv eng 5 on hub 8
[   25.973019] [drm] Initialized amdgpu 3.54.0 20150101 for 0000:0c:00.0 on minor 1
[   25.980115] amdgpu 0000:0c:00.0: [drm] *ERROR* [CONNECTOR:122:DP-4] Requesting EDID firmware "edid/PG278Q_120.bin" failed (err=-22)
[   25.980123] [drm:amdgpu_dm_connector_mode_valid [amdgpu]] *ERROR* No EDID firmware found on connector: DP-4 ,forcing to OFF!
[   25.983103] fbcon: amdgpudrmfb (fb0) is primary device
[   25.983269] [drm] DSC precompute is not needed.
--
[   37.626741] hid-generic 0003:1050:0407.000E: input,hidraw8: USB HID v1.10 Keyboard [Yubico YubiKey OTP+FIDO+CCID] on usb-0000:07:00.1-4.3/input0
[ 1485.826017] logitech-hidpp-device 0003:046D:406D.000A: HID++ 4.5 device connected.
[ 2093.540565] amdgpu 0000:0c:00.0: [drm] *ERROR* [CONNECTOR:122:DP-4] Requesting EDID firmware "edid/PG278Q_120.bin" failed (err=-22)
[ 2093.548974] [drm:detect_link_and_local_sink [amdgpu]] *ERROR* No EDID read.
[ 2093.831415] amdgpu 0000:0c:00.0: [drm] *ERROR* [CONNECTOR:122:DP-4] Requesting EDID firmware "edid/PG278Q_120.bin" failed (err=-22)
[ 2093.831421] [drm:amdgpu_dm_connector_mode_valid [amdgpu]] *ERROR* No EDID firmware found on connector: DP-4 ,forcing to OFF!
[ 2093.844356] nvme 0000:01:00.0: VPD access failed.  This is likely a firmware bug on this device.  Contact the card vendor for a firmware update
[ 2094.288519] [drm:detect_link_and_local_sink [amdgpu]] *ERROR* No EDID read.
[ 2205.443882] usb 5-3: USB disconnect, device number 3
[ 2205.443890] usb 5-3.2: USB disconnect, device number 5

Needed to add a few additional guards against it and thought it'll be easier to develop it in my own repo then upstream it.

nazarewk · 2024-04-18T14:46:51Z

ok, I've created and tested in my configs edido script which applies (selected) video= and complete drm.edid_firmware= kernel parameters during runtime. It loads /proc/cmdline automatically unless you pass it arguments directly.

meutraa · 2024-04-19T08:55:08Z

I have issues with the latest version of the display-edid-apply script (using applyAtRuntime, as the kernel parameters is still broken).

With applyWithKernelParameters false, the only log line displayed is:

log "loading kernel parameters from /proc/cmdline"

In this case the edid is not applied.

With applyWithKernelParameters true it ends with:

log "EDID is already up to date with '%s'" "${edidPath}"

I checked the hash of the current-firmware edid.bin and it did indeed match that of the edid_override. So CHANGED remains 0, and the trigger hotplug doesn't occur. With a manual trigger hotplug, the edid is functioning. I'm really not sure what is going on here. Perhaps something odd with the kernel where it loads it, but doesn't really.

nazarewk · 2024-04-19T09:25:01Z

Yeah, the kernel has major issues loading EDID files over last few weeks, just getting worse with each update.

Good catch with having no boot.kernelParams unless applyWithKernelParameters is set.

billksun · 2024-05-02T16:13:25Z

Never thought I would need this, but yes, I need this!

modules-closure.sh seems to consider everything under /lib/firmware to be a kernel module, this change adds a special handling of `edid` directory, which does not contain kernel modules fixes NixOS#279739

…hru.config`

jys1670 · 2024-05-12T10:52:17Z

Seems that errors like
amdgpu 0000:e7:00.0: [drm] *ERROR* [CONNECTOR:111:DP-5] Requesting EDID firmware "edid/edid.bin" failed (err=-22)
are caused by 1ed5df1 (and unrelated to kernel version bumps)
With its revert, applyWithKernelParameters = true; works fine on my machine.

K900 · 2024-05-12T10:57:56Z

Complete shot in the dark, but if you use the unmodified kernel and add initcall_blacklist=simpledrm_platform_driver_init, does it work?

jys1670 · 2024-05-12T11:10:47Z

Complete shot in the dark, but if you use the unmodified kernel and add initcall_blacklist=simpledrm_platform_driver_init, does it work?

Nope, I can't even boot with this one, sadly.

FlyingWombat · 2024-05-13T00:23:03Z

nixos/modules/services/hardware/display.nix

+          pathsToLink = [ "/lib/firmware/edid" ];
+          ignoreCollisions = true;
+        }) // {
+          compressFirmware = false;


We shouldn't actually need compressFirmware = false;, since the kernel is built with compressed firmware support by default. While this looks trivial either way (compression vs nixos config complexity), I still think it should be removed if it isn't needed. If it is needed, then I think that means there are other problems with how nixos is configuring the kernel.

it is needed, I think code paths in kernel/drivers do not decompress EDID files or at least not properly. Don't remember the details, but i tried to investigate quite extensively (without much knowledge of C/C++ itself)

But my archlinux partition is using a zstd compressed edid firmware file without issue. Using Archlinux vanilla kernel v6.8.9.arch1-2. So it isn't a problem with the kernel, but with Nixos kernel configuration.

$ cat /proc/cmdline BOOT_IMAGE=/boot/vmlinuz-linux root=UUID=eb3729f4-547c-42a7-86b4-6020f9a31762 rw loglevel=3 quiet nowatchdog intel_iommu=on iommu=pt zswap.enabled=0 ibt=off amdgpu.ppfeaturemask=0xffffffff drm.edid_firmware=edid/g70nc_edid.bin $ ls /usr/lib/firmware/edid/ g70nc_edid.bin.zst

I compressed it manually with zstd -19 g70nc_edid.bin

FlyingWombat · 2024-05-13T00:37:04Z

Seems that errors like amdgpu 0000:e7:00.0: [drm] *ERROR* [CONNECTOR:111:DP-5] Requesting EDID firmware "edid/edid.bin" failed (err=-22) are caused by 1ed5df1 (and unrelated to kernel version bumps) With its revert, applyWithKernelParameters = true; works fine on my machine.

That's odd that commit is causing the error, since the kernel configs in that commit match what is set in my Archlinux partition (dual boot), and it loads edid_firmware (compressed) without issue. It must be failing in combination with some other options nixos is setting.

FlyingWombat · 2024-05-13T19:46:44Z

I noticed Archlinux has this patch in their kernel, to "skip simpledrm if nvidia-drm.modeset=1 is set". It could be relevant to the issues that @jys1670 mentioned were introduced with 1ed5df1 -- if the user has an Nvidia gpu.

Though, I have an AMD gpu and still get the same err=-22 when loading edid firmware.

github-actions bot added the 6.topic: kernel label Jan 9, 2024

nazarewk force-pushed the hardware-firmware-edid-fix branch from 580d12f to 11e9e9b Compare January 9, 2024 10:26

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 9, 2024

nazarewk mentioned this pull request Jan 9, 2024

makeModulesClosure: include /lib/firmware/edid nazarewk/nixpkgs#1

Closed

13 tasks

nazarewk marked this pull request as draft January 9, 2024 12:10

nazarewk force-pushed the hardware-firmware-edid-fix branch from 11e9e9b to 44b70eb Compare January 9, 2024 13:00

nazarewk changed the title ~~modules-closure: include /lib/firmware/edid~~ makeModulesClosure: include /lib/firmware/edid Jan 9, 2024

nazarewk marked this pull request as ready for review January 9, 2024 13:01

nazarewk mentioned this pull request Jan 9, 2024

compressFirmwareXz: never compress lib/firmware/edid #279828

Closed

13 tasks

soupglasses approved these changes Jan 9, 2024

View reviewed changes

delroth added the 12.approvals: 1 label Jan 9, 2024

RaitoBezarius requested review from alyssais and K900 January 9, 2024 14:59

github-actions bot added 6.topic: nixos 8.has: documentation labels Jan 9, 2024

delroth removed the 12.approvals: 1 label Jan 9, 2024

nazarewk force-pushed the hardware-firmware-edid-fix branch from 192b3c7 to 7bc1197 Compare April 17, 2024 12:06

SuperSandro2000 reviewed Apr 18, 2024

View reviewed changes

nazarewk force-pushed the hardware-firmware-edid-fix branch 2 times, most recently from 62dc18b to 0fe1957 Compare April 18, 2024 12:40

nazarewk requested a review from SuperSandro2000 April 18, 2024 12:52

nazarewk force-pushed the hardware-firmware-edid-fix branch 3 times, most recently from cb0b041 to 0467e26 Compare April 18, 2024 14:35

nazarewk force-pushed the hardware-firmware-edid-fix branch from 0467e26 to ae53a50 Compare April 22, 2024 07:23

wegank added the 2.status: merge conflict label May 3, 2024

nazarewk added 6 commits May 7, 2024 09:30

makeModulesClosure: include /lib/firmware/edid

42862ce

modules-closure.sh seems to consider everything under /lib/firmware to be a kernel module, this change adds a special handling of `edid` directory, which does not contain kernel modules fixes NixOS#279739

linuxhw-edid-fetcher: init at unstable-2023-05-08

1e29ebe

nixos/hardware.display: init

2a902be

edido: init

0eab3ef

nixos/hardware.display: apply EDID before multi-user.target

fe417ee

nixos/hardware.display: expose `hardware.display.edid.modelines.passt…

e70276e

…hru.config`

nazarewk force-pushed the hardware-firmware-edid-fix branch from ae53a50 to e70276e Compare May 7, 2024 07:31

ofborg bot removed the 2.status: merge conflict label May 7, 2024

FlyingWombat reviewed May 13, 2024

View reviewed changes

wegank added the 2.status: merge conflict label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/hardware.display: init module #279789

nixos/hardware.display: init module #279789

nazarewk commented Jan 9, 2024 •

edited

soupglasses left a comment

nazarewk commented Jan 9, 2024

soupglasses commented Jan 9, 2024

K900 commented Jan 9, 2024

SuperSandro2000 left a comment

SuperSandro2000 Apr 18, 2024

nazarewk Apr 18, 2024

SuperSandro2000 Apr 18, 2024

SuperSandro2000 Apr 18, 2024

nazarewk Apr 18, 2024 •

edited

SuperSandro2000 Apr 18, 2024

nazarewk Apr 18, 2024

SuperSandro2000 Apr 18, 2024

nazarewk Apr 18, 2024

SuperSandro2000 Apr 18, 2024

nazarewk commented Apr 18, 2024 •

edited

nazarewk commented Apr 18, 2024

meutraa commented Apr 19, 2024 •

edited

nazarewk commented Apr 19, 2024

billksun commented May 2, 2024

jys1670 commented May 12, 2024

K900 commented May 12, 2024

jys1670 commented May 12, 2024

FlyingWombat May 13, 2024 •

edited

nazarewk May 13, 2024

FlyingWombat May 13, 2024 •

edited

FlyingWombat commented May 13, 2024

FlyingWombat commented May 13, 2024

	passthru.updateScript = nix-update-script { extraArgs = [ "--version=branch=master" ]; };
	passthru.updateScript = nix-update-script { extraArgs = [ "--version=branch=master" ]; };

	curl -L https://github.com/linuxhw/EDID/tarball/master \| tar -zx -C "${repo}" --strip-components=1
	curl -L https://github.com/linuxhw/EDID/tarball/HEAD \| tar -zx -C "${repo}" --strip-components=1

nixos/hardware.display: init module #279789

Are you sure you want to change the base?

nixos/hardware.display: init module #279789

Conversation

nazarewk commented Jan 9, 2024 • edited

Description of changes

Things done

soupglasses left a comment

Choose a reason for hiding this comment

nazarewk commented Jan 9, 2024

soupglasses commented Jan 9, 2024

K900 commented Jan 9, 2024

SuperSandro2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazarewk Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazarewk commented Apr 18, 2024 • edited

nazarewk commented Apr 18, 2024

meutraa commented Apr 19, 2024 • edited

nazarewk commented Apr 19, 2024

billksun commented May 2, 2024

jys1670 commented May 12, 2024

K900 commented May 12, 2024

jys1670 commented May 12, 2024

FlyingWombat May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlyingWombat May 13, 2024 • edited

Choose a reason for hiding this comment

FlyingWombat commented May 13, 2024

FlyingWombat commented May 13, 2024

nazarewk commented Jan 9, 2024 •

edited

nazarewk Apr 18, 2024 •

edited

nazarewk commented Apr 18, 2024 •

edited

meutraa commented Apr 19, 2024 •

edited

FlyingWombat May 13, 2024 •

edited

FlyingWombat May 13, 2024 •

edited