swaywm / sway

i3-compatible Wayland compositor
https://swaywm.org
MIT License
14.72k stars 1.11k forks source link

Support for setting broadcast RGB to full #3173

Open pauledd opened 6 years ago

pauledd commented 6 years ago

Hi Is there a corresponding command in sway to this one in xrandr xrandr --output HDMI-1 --set "Broadcast RGB" "Full" and xrandr --output HDMI-1 --set "Broadcast RGB" "Limited 16:235"

I can change the colorspace while sway is running by starting a regualar X-Server and issue the commands above but but it would be neat if that could be done without starting an X-server.

ascent12 commented 6 years ago

This is one of the various DRM properties that the kernel provides us. That particular one is Intel specific, so we wouldn't add a specific API for it. However, I do want to add a way to set arbitrary DRM properties like you're doing with xrandr at some point. Support is required inside wlroots first, though.

emersion commented 6 years ago

I can change the colorspace while sway is running by starting a regualar X-Server and issue the commands above but but it would be neat if that could be done without starting an X-server.

It wouldn't be too hard to build a standalone tool that changes DRM properties. Would probably require CAP_SYS_ADMIN though.

ascent12 commented 6 years ago

I'm not actually sure if you can change properties if you're not the DRM master, and an external tool trying to force it off us isn't going to work, even with privileges, Even then, I don't really want external tools trying to change the state, causing the wlroots internal state to be incorrect and probably be racy as hell.

emersion commented 6 years ago

True.

Kristaba commented 5 years ago

This is an old issue, but I would be rather interested to know if there is any progress/workaround to set the color space from sway now?

However, I do want to add a way to set arbitrary DRM properties like you're doing with xrandr at some point. Support is required inside wlroots first, though.

When I see the list of Intel KMS parameters, I imagine it is not the only very specific parameter that someone would have to customize when using sway or other wlroots-based compositor, so a generic approach seems a good idea.

I am not familiar with wlroots and sway development, does someone have some pointers on the parts missing to have such feature and the main difficulties?

ascent12 commented 5 years ago

@Kristaba That link you have is out of date. The current documentation is https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#kms-properties

Due to the extremely dynamic nature of the properties, it's really hard to come up with a sane external API. The main thing is that the correct enum and bitmask values aren't guaranteed to be stable or even not have any holes. I imagine that the API would have to be largely string-based for it to even be slightly sane.

Our current property code currently isn't aware of unknown properties. It'd have to be changed to remember everything including property values. An associative array of some kind (hashmap, trie, etc.) may be warranted.

The current commit handling code needs to extended to manage extra properties, including rolling them back when a commit fails. This is really hard for legacy DRM, so I'd be fine if this was only implemented for atomic DRM.

We need to have a blacklist of the properties that backend is already handling, so that people can't use such an API to really mess with our internal state.

The relevant code is in here: https://github.com/swaywm/wlroots/tree/master/backend/drm DRM certainly isn't the easiest API to get into, and the code is even undergoing some restructuring, but the property handling code could be done largely separately from the rest of it. If you still want to try and tackle this, feel free to ask any extra questions on #sway-devel on freenode.

emersion commented 5 years ago

Not sure it's a good idea to expose properties directly to the user tbh

ascent12 commented 5 years ago

Maybe not, but there are some driver-specific ones which we wouldn't really want to add explicit support for, but might be something that people still want to set. They probably mostly involve colors. Although, if we get ever get "proper" color management support, maybe there won't be an need for a lot of these extra properties.

Kristaba commented 5 years ago

A small update here, as I did a (very ugly) proof of concept in wlroots and sway to manage the color range property. I tried as a standalone tool beforehand (as proposed by @ascent12) but setting this property appear to be ignored in this case. It is probably related to the concept of DRM master, but I may have miss something.

To make things clear, not being able to set this property is really annoying for Intel card hardware. For some weird reasons, they implemented "overcorrectly" the protocol for deciding the colorspace to use. Hence, many devices such as display port adapters and some screens lead to a "Limited Color Space" to be used, which give washed colors on the display... So, while this setting appear to be very specific with automatic selection being OK for 99% of users, I and many other Intel card owners really use it to obtain a descent color display.

Now, the question is what to do with it on wlroots/sway. To be honest, it really looks like an Intel implementation bug, but for what I understood they will not "fix" it as they consider it is the protocol specification. A few patches sometimes give the hope that it will be managed better (https://patchwork.kernel.org/patch/10871251/ for instance) but the problem is still here for years. This dumb setting is probably the last missing piece for having sway perfectly functional for me (compared to my previous xorg setup).

After doing my ugly implementation, I understand better the issues pointed out by @ascent12 and @emersion. However I really believe sway (and therefore wlroots) should allow getting/setting arbitrary DRM properties, probably with the mentioned blacklist to keep consistent internal states in wlroots. This is annoying as it requires indeed some kind of loosely specified string-based API, while the rest of wlroots seems to be designed cleanly and with care. But at some point, vendor-specific settings probably require that.

I am not sure to find more time in the upcoming month, but will try to implement a first acceptable version in order to discuss the pros/cons of the different possible solutions. As most of the implementation should occur in wlroots, I imagine it would be great to open a dedicated issue on it for this discussion. If someone has any new consideration on this topic, do not hesitate to point it to me!

emersion commented 5 years ago

I tried as a standalone tool beforehand (as proposed by @ascent12) but setting this property appear to be ignored in this case.

A standalone tool should work, just run it as root, maybe before starting sway.

I'm still very much -1 to allowing users to change arbitrary DRM properties.

Kristaba commented 5 years ago

A standalone tool should work, just run it as root, maybe before starting sway.

Indeed, before sway it seems to work, but I can't manage some edge-cases this way. I use my laptop in different configurations (home/office). In one of them, an adapter is acting as a DP DST device, hence creating dynamically a new pair of outputs (DP-4 and DP-5 in my case, but they definitely are not accessible before to connect this dock). So I would have to kill all my workspace, run the magic tool and re-start sway after that, it is really not convenient... :/

I'm still very much -1 to allowing users to change arbitrary DRM properties.

Just to be sure to understand well, what are your main concerns on this topic? Is it something related to security/stability? Or more to keep a clean API?

emersion commented 5 years ago

It's more about not allowing users to shoot themselves in the foot. Additionally it's more of a workaround, not a real fix. If you need this, I'd suggest patching sway/wlroots locally.

GermainZ commented 4 years ago

For reference to those looking for a solution and ending up here, a hacky workaround is described here. It's basically building and running this command before starting Sway:

proptest -M i915 -D /dev/dri/card0 <output connector ID> connector <"Broadcast RGB" property ID> <value: 1 for full on my setup>
akvadrako commented 4 years ago

FYI a patch for wlroots is available which sets this to full. https://github.com/swaywm/wlroots/pull/2310