Add support for HDR10 #8617
Add support for HDR10 #8617
Conversation
|
For me, this patch with HDR enabled adds significant delay to opening programs; even @dnkl's foot seems to take about half a second whereas without HDR the delay is imperceptible. In truth, my RX 6900 XT isn't state-of-the-art anymore, but it's certainly not a slouch. With Screen recording (the colors aren't actually this pale; wf-recorder just seems to lack HDR compatibility currently): recording.mp4 |
|
The delay on opening a program seems to have been fixed. Feels much better. |
|
Yes, it was unrelated to this PR: #8775 |
|
Getting error when compiling with clang. ninja outputDefaulting |
|
Ah yes, clang doesn't realize that the uninitialized variable is unreachable. Added a default value! |
emersion
force-pushed
the
hdr10
branch
2 times, most recently
from
5445fee to
ec343a0
Compare
| } else { | ||
| wlr_output_state_set_render_format(pending, DRM_FORMAT_XRGB8888); | ||
| } | ||
| enum render_bit_depth render_bit_depth = get_config_render_bit_depth(oc); |
There was a problem hiding this comment.
This defaults to 10-bit even if set_hdr would bail out on requesting HDR. Maybe we should flip things, setting HDR and changing the default based on whether HDR was set or not.
There was a problem hiding this comment.
Hm, that sounds complicated to implement because get_config_render_bit_depth() is called from elsewhere too, e.g. search_render_format(). Also I think it's better to keep things simple to make the render bit depth default only depend on the config alone rather than the renderer/output capabilities.
There was a problem hiding this comment.
It does feel a little bit surprising that output * hdr on (where someone just wants to use HDR whenever available) would lead to all non-HDR displays having 10-bit render depth, especially as you cannot easily revert it (output * render_bit_depth 8 would also impact HDR-capable displays), and as 10-bit on Vulkan would activate the blending buffer which currently triples VRAM use and degrades performance a bit.
On the other hand I doubt anyone needs output * hdr on, at least right now where HDR is very much hit and miss. We can probably just ignore this for now.
get_config_render_bit_depth() is called from elsewhere too, e.g. search_render_format()
Both uses would be able to take wlr_output_state as argument if one wanted to check if hdr is enabled to select default render bit depth. search_render_format is called after queue_output_config has filled out the state, and HDR is not currently part of the output config search so it stays set until we give up.
|
|
||
| bool hdr = oc && oc->hdr == 1; | ||
| if (hdr && oc->color_transform != NULL) { | ||
| sway_log(SWAY_ERROR, "Cannot HDR on output %s: output has an ICC profile set", wlr_output->name); |
There was a problem hiding this comment.
I don't think we should log from here, queue_output_config is called all the the time and should just produce the current wlr_output_state. Same applies to set_hdr - if someone does output * hdr on, getting "PQ transfer function not supported by output" errors one or more times for every output not supporting HDR, every time any output changes is a bit noisy...
Maybe we could log something about output capabilities (HDR and VRR for example) when we create the output.
There was a problem hiding this comment.
We already log for other errors, e.g. mode not found, invalid modeline etc.
I think it's important to log something when the user tries to enable HDR but we can't. Logging on startup isn't enough IMHO.
The current scheme only logs when the output configuration changes, which isn't too spammy and easy enough for users to fix if they want to (by only enabling HDR if the output supports it).
There was a problem hiding this comment.
only logs when the output configuration changes
Multiple times on every change. I really don't like log noise. :/
I have slowly been chipping away at it (e.g., a630272), removing most bogus logging and collecting meaningful stuff in places like dump_output_state instead of just whenever we construct a wlr_output_state. The only remaining logs in queue_output_config is set_mode stating fallback to preferred mode and set_modeline failing if called on a non-drm output, which I didn't have an immediate replacement for...
The HDR case is logically similar to the adaptive sync case where the setting is best effort and can be disabled by display, link or connector restrictions, in which case not enabling is not considered an error. I do think in that case that we need to clarify what the output capabilities are in the log, as we right now only communicate what we end up doing (with debug log level).
There was a problem hiding this comment.
I noticed the adaptive sync thing and was about to write a patch to add a log there. I find it pretty confusing that a command like output DP-1 adaptive_sync on would silently do nothing.
There was a problem hiding this comment.
This was the kind of INFO-level logging I was thinking of: #8784
I feel like that's clearer than error logging. We could consider also logging the final output state as INFO though, right now that's only debug.
There was a problem hiding this comment.
I don't think that kind of logging is very helpful to users TBH.
There was a problem hiding this comment.
I think it's far more helpful than sporadic error messages. Errors is something people try to fix as if something is wrong that has to be corrected, and there's nothing to tell if something isn't meant to work.
It's much easier to know if something is supported and to troubleshoot if the first step is: Does it say "yes"? Both for users, those providing support and those writing various FAQs/wikis for self-help.
But I won't hold up the feature for that further.
There was a problem hiding this comment.
If the config tries to enable HDR10 and HDR10 cannot be enabled, I think it's only fair to print an error.
Anyways, it doesn't seem like we're going to come to an agreement here.
|
I get The model in question is "G272QPF E2" by MSI. I tried manually disabling this check, but that resulted in some pretty awful colors. |
That's because your renderer doesn't support color transforms as the error stated. Make sure you're using the vulkan renderer ( |
|
I recompiled wlroots and used the vulkan renderer, now sway starts without error, but the colors are still broken. Most of this github page is just white, there's no contrast. |
|
Something broke in a recent change (around when |
Indeed: |
|
Yep, that fixed it. Thanks. |
This worked for me. I am not sure how to actually test the HDR support but both sway and my monitor say it's enabled, and there's no obvious bugs. |
|
I'm getting:
Despite it being OK on Windows. Am I missing a setting? The laptop is an ASUS Zenbook 14 OLED (UX3405). Currently on 35748a5 for Sway & 31b78a4f3 for wlroots Relevant logs (hopefully): Click to expand!
Relevant env vars: Sway is started with |
|
@paul-ri Can you share your EDID? You can find it in |
|
Output of Click to expand!
|
|
Ah, thanks! It seems like all of the interesting stuff is inside a DisplayID 2 extension block. See this issue for DisplayID 2 support in libdisplay-info: https://gitlab.freedesktop.org/emersion/libdisplay-info/-/issues/18 |
|
Cheers! That'll help other folks in the same situation as me. Thanks for the amazing efforts! |
7 participants
No description provided.