WIP: Recognize XPS 15 7590 OLED variant

[Herbstein]

This is a WIP pull request that does not currently work as desired.

The current behaviour is that running the tool while a non-max brightness is set it turns down the brightness as expected. However, attempting to further increase or decrease the brightness makes the brightness go to max brightness.

When trying to increase and decrease the brightness flashes of lower brightness can be seen. Could this be related to the two TODOs at the bottom about Mutter changing the gamma values?

Lastly, I added two new values to be looked at when determining the output. This was needed as the version is simply an empty file on the XPS 15 7590.

[jackpot51]

Are there systems that would match this but not have an OLED display?

[Herbstein]

I am very unsure of this. Dell has several configurations of this laptop. Some with the OLED screen and some without. There is, to my knowledge, not a single place to identify which SKU the laptops with OLED screens have - other than running the commands and checking manually.

I've not done too much work on this since opening the PR - exams and all that - but I also not convinced I am knowledgeable enough to identify the strange behavior of the program on this specific machine.

[jackpot51]

Could you provide the EDID information? That may be better to identify the display as OLED

[Herbstein]

I wrote that I couldn't get what looked like valid EDID, but I managed to get parse-edid to work. The following is the output of program.

Section "Monitor"
	Identifier "���"
	ModelName "���"
	VendorName "SDC"
	# Monitor Manufactured week 10 of 2019
	# EDID version 1.4
	# Digital Display
	DisplaySize 340 190
	Gamma 2.20
	Option "DPMS" "false"

	#Extension block found. Parsing...
extb[4]: 0xe3 (0xe0)

It also comments that no video blocks are present, which supposedly is highly unusual.

[jackpot51]

I would have expected and Identifier or ModelName set which would allow differentiating the OLED panel.

[Herbstein]

I use the command parse-edid < /sys/class/drm/card0-eDP-1/edid. Weirdly enough the Identifier and ModelName fields change values everytime I run the command. This is very perplexing to me, and makes me think I did something wrong. What it is I have no idea about.

[jackpot51]

Please try the following:

sudo get-edid | parse-edid

[Herbstein]

Running that command using the iGPU-only mode (which is what I prefer to run) results in no EDID information being found. Switching to the hybrid mode results in the same result as the one posted earlier - no video blocks and the Identifier and ModelName fields change every time the command is run.

[Herbstein]

I talked to a friend about this, and we identified edid-decode as an alternative parser. It worked a lot better than parse-edid and gave me a more consistent result.

EDID version: 1.4
Manufacturer: SDC Model a029 Serial Number 0
Made in week 10 of 2019
Digital display
10 bits per primary color channel
DisplayPort interface
Maximum image size: 34 cm x 19 cm
Gamma: 2.20
Supported color formats: RGB 4:4:4
First detailed timing includes the native pixel format and preferred refresh rate
Display x,y Chromaticity:
  Red:   0.6845, 0.3125
  Green: 0.2431, 0.7070
  Blue:  0.1386, 0.0546
  White: 0.3125, 0.3291
Established timings supported:
Standard timings supported:
Detailed mode: Clock 573.410 MHz, 344 mm x 194 mm
               3840 3888 3920 4400 hborder 0
               2160 2164 2168 2172 vborder 0
               +hsync -vsync 
               VertFreq: 60 Hz, HorFreq: 130320 Hz
Detailed mode: Clock 573.410 MHz, 344 mm x 194 mm
               3840 3888 3920 4400 hborder 0
               2160 2164 2168 2172 vborder 0
               +hsync -vsync 
               VertFreq: 60 Hz, HorFreq: 130320 Hz
ASCII string: 0HHFM
Manufacturer-specified data, tag 0
Has 1 extension blocks
Checksum: 0xc0 (valid)

CTA extension block
Extension version: 3
11 bytes of CTA data
  Extended tag: Colorimetry data block
    BT2020RGB
  Extended tag: HDR static metadata data block
    Electro optical transfer functions:
      Traditional gamma - SDR luminance range
      SMPTE ST2084
    Supported static metadata descriptors:
      Static metadata type 1
    Desired content max luminance: 115 (603.666 cd/m^2)
    Desired content max frame-average luminance: 109 (530.095 cd/m^2)
    Desired content min luminance: 208 (4.016 cd/m^2)
0 native detailed modes
Checksum: 0xe2 (valid)

[jackpot51]

Heh, that's the same panel we have in the System76 Adder Pro. I think using the panel identifier would be a reliable way to do this.

[Herbstein]

My plan is to add the edid crate as a dependency, look for and parse all the EDID files in the directories in /sys/class/drm/card0, and continue with the first known OLED panel, if any.

Ideally it'd also support both multiple cards and multiple OLED displays but that seems out of scope right now.

After that is implemented I also still need to identify why it's managing to set the brightness correctly on startup, but not when adjusting while the program is actually running.

[jackpot51]

That looks pretty good. I would also test the vendor ID. There is code in gnome-settings-daemon that changes the gamma settings. That is what is overriding changes here.

[Herbstein]

I added the checks for vendor ID and removed the unwraps. I don't have access to another Linux box - or an HDMI cable - so I can't say for sure that this doesn't do something weird on an unknown display. I don't think it will though.

Do you know how to change the behaviour of gnome-settings-daemon to avoid it resetting the gamma of the screen? I haven't found anything from a quick look around.

[jackpot51]

Looks great! I will get this tested sometime this week. We have a patch for gnome-settings-daemon that may interest you here: https://github.com/pop-os/gnome-settings-daemon/blob/master_eoan/debian/patches/oled_brightness.patch

It currently uses the DMI information but there may be a way to clean it up so the detection happens in system76-oled.

[Herbstein]

Parsing out the two required values from the EDID files shouldn't be too hard in C. Even in the Rust code I considered not including the extra parser dependency. For this task the specification is fairly simple and straightforward.

Doing the check manually in C does of course result in a duplication of the known OLED list. If this is intended to be a more general solution as more laptops get OLED screens that might not be ideal. However, I'm not well versed enough in dbus communication to propose a solution for containing it in this program.

[jackpot51]

@Herbstein that sounds good, to do the check in C inside of gnome-settings-daemon

[Herbstein]

I am unsure what the development workflow on the daemon is. Adding, generating, and applying patches is fairly straight forward. Should I apply the OLED patch, write the new checking, and then generate a new patch? And should that patch override the old OLED patch or be a secondary patch to it?

[jackpot51]

Yep, that is correct. Update the OLED patch is the best way to do it.

[sean-public]

Is this still waiting for review? Are there changes required in another daemon? I am struggling with a 7590 as well; gnome-tweaks Soft Power still doesn't work either, so I'm stuck with a script that runs xrandr --output eDP-1 --brightness $1 or similar when I need to adjust the brightness.

[Mbertu]

Is this still waiting for review? Are there changes required in another daemon? I am struggling with a 7590 as well; gnome-tweaks Soft Power still doesn't work either, so I'm stuck with a script that runs xrandr --output eDP-1 --brightness $1 or similar when I need to adjust the brightness.

I'm in the same situation, hope in a fast resolution.

[Herbstein]

The situation is that I suddenly got super busy with my bachelor project and general university stuff. The final part of the patch requires modifying the pop-os gnome-settings-daemon fork. Specifically, the patch for OLED , though a second patch is required to build on pop-os itself.

The change needed for the OLED patch is parsing the file /sys/class/drm/card0-eDP-1/edid, checking it's one of the known OLED panels, and thus replacing the laptop check that is currently used in the daemon. The edid file has a specific layout, that is nicely documented on wikipedia.

If anyone wants to swoop in and handle the C-part I more than welcome it! Otherwise I'll get to it when my workload slows down slightly.

[Herbstein]

I wrote the above, and then actually started working on it. @jackpot51 What is the process for reviewing changes to the daemon before merging them? Do I just create a pull request containing the single patch file, or do you want to have a "normal" source-based pull request that can then be turned into a patch as a final step?

Also, how do I test whether what I've written works? I guess it involves running the custom daemon in place of the normal one, but not sure how.

[jackpot51]

Yes, please create a pull request and I will review it. We can talk about testing and other stuff there if you make one.

[jjlwork]

Let me know if any help is needed with testing, I also have this laptop with the OLED and stumbled on this when looking for solutions. I am happy to do whatever I can.

Not sure if it applies to this type of testing, but I also have access to a docking station(Dell D3100) and multiple monitors, however the monitors are not 4k.

[Mbertu]

If I try to compile this application I get this error:

error: failed to run custom build command for x11 v2.18.2
Caused by:
process didn't exit successfully: /home/michele/Scaricati/system76-oled-xps7590/target/release/build/x11-01952e09a8712b90/build-script-build (exit code: 101)
--- stdout
cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu
cargo:rustc-link-lib=X11

--- stderr
thread 'main' panicked at 'called Result::unwrap() on an Err value: Failure { command: ""pkg-config" "--libs" "--cflags" "xrandr" "xrandr >= 1.5"", output: Output { status: ExitStatus(ExitStatus(256)), stdout: "", stderr: "Package xrandr was not found in the pkg-config search path.\nPerhaps you should add the directory containing xrandr.pc\'\nto the PKG_CONFIG_PATH environment variable\nNo package \'xrandr\' found\nPackage xrandr was not found in the pkg-config search path.\nPerhaps you should add the directory containing xrandr.pc'\nto the PKG_CONFIG_PATH environment variable\nNo package 'xrandr' found\n" } }', /home/michele/.cargo/registry/src/github.com-1ecc6299db9ec823/x11-2.18.2/build.rs:36:5
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

warning: build failed, waiting for other jobs to finish...
error: build failed
make: *** [Makefile:57: target/release/system76-oled] Error 101

An x11 versions dependency issue, what I'm doing wrong?
I'm on Pop!_Os 19.10

[Herbstein]

I'm not at my laptop right now, but I think you want to install libx11-dev. Do not that this doesn't work currently, as it also needs support in gnome-settings-daemon, for which I've also submitted a pull request.

[Herbstein]

I missed your edit. You want to install libxrandr-dev!

[Mbertu]

I missed your edit. You want to install libxrandr-dev!

Thanks. The application doesn't work but I just want try by myself and maybe start to contribute :D
Compile successfully the package is the first step :)

Out of curiosity, can I ask you what is the support in gnome-settings-daemon for?

[Herbstein]

Out of curiosity, can I ask you what is the support in gnome-settings-daemon for?

Of course! gnome-settings-daemon also does some work in modifying and handling brightness values in a Pop_Os! install. When this program intercepts brightness changes gnome-settings-daemon reverts the display to full brightness. That is what, on my machine, results in a flashing screen when trying to turn the brightness way down. If you try turning the brightness all the way down and then start this program your actual brightness should go down to the desired level. This is because (as I understand it) gnome-settings-daemon only reverts back when you're pressing on the shortcut key.

EDIT: Let me expand a little on what the specific support changes needed in both this program and gnome-settings-daemon was/is.

When System76 needed the OLED brightness for their Adder WS laptop they simply used checked the laptop model information to make sure it didn't interfere with normal usage on other laptops. To support other models of laptop I instead (with guidance from Jeremy) started parsing the EDID from the built-in display. The OLED in the XPS7590 turns out to be the same panel, so supporting both models is fairly easy with that approach. Lastly, adding another display now only requires knowing two values from the EDID of the new display. It still needs to be inserted into both programs, however.

[Mbertu]

Thanks for the explanation, I followed the discussion in this PR but I missed that information.
At this point, is possible to modify the gnome-settings-daemon to compile a version that make the brightness switch work on every OLED display? I'll edit and compile it by myself in case. Just for fun and try new things :D

[Mbertu]

Just found the other PR in gnome settings daeomn repo :)

[Herbstein]

I really, really felt like I needed this when I created the PR but I've grown accustomed to the harsh light - to other people it's harsh even in daylight. But I want to see this through because I can see other people are struggling with the issue.

[jorisfritzsche]

@Herbstein Really appreciate the effort you're putting into this! I just built this PR and it's the first fix for the OLED brightness issue that I've found that actually works well. Much appreciated!

[yiapps]

OLED brightness working on XPS 7590 after last update.

[Wouter0100]

@rhb388 on Pop!_OS 20.04?

[yiapps]

@rhb388 on Pop!_OS 20.04?

yes it is working on Pop!_OS 20.04