drm backend: set "Broadcast RGB" = "Full" for all connections #2310

akvadrako · 2020-06-30T17:54:09Z

I have a new monitor which has three similar modes:

3840x2160 @ 59.939999 Hz
3840x2160 @ 60.000000 Hz
3840x2160 @ 59.997002 Hz

The last one works fine, but the first two have issues displaying colors, because the "Broadcast RGB" property on the connector defaults to "Auto" which means "Limited 16:235", squashing the dynamic range.

Since there is really no way for a sway user to know that 3840x2160@60Hz displays colors poorly, while 3840x2160@59.997Hz will be fine, I was using the wrong mode for weeks.

This patch causes wlroots to always set "Broadcast RGB" = "Full", which I think most people want.

emersion · 2020-06-30T21:26:07Z

It doesn't seem like switching to "Full" unconditionally is a good idea, as TVs tend to use "Limited" instead.
"Broadcast RGB" is an Intel-specific property.

akvadrako · 2020-06-30T22:44:27Z

1. It doesn't seem like switching to "Full" unconditionally is a good idea, as TVs tend to use "Limited" instead.

If it won't be the default, then it would be nice if sway could inform the user which modes use the limited mode; looking at the driver the logic seems to be in drm_default_rgb_quant_range(); it seems like userspace can only guess unless it copied all that logic. The aspect ratio flags seem to be a clue.

Do you have any other ideas for how it might be done? Perhaps setting full just for DP.

2. "Broadcast RGB" is an Intel-specific property.

Yes, I wonder how other drivers handle it. In regards to the patch, if the property doesn't exist it won't cause an issue.

cyanreg · 2020-06-30T22:51:34Z

Its still a good idea even on TVs to always use Full. Most TVs will autodetect, since its trivial to do, and adjust accordingly. Some let you override it, but not as many as you think always assume its always going to be limited. Furthermore, HDR is a thing now, and a lot of HDR content these days, including all of youtube, is in full range. Hence its not unreasonable to assume HDR TVs would also expect full-range. Losing 10% of your dynamic range (possibly more depending on dithering) with HDR is much more fatal than with bt709 (IEC61966-2.1) gamma.
Also, a DisplayPort connection is guaranteed to use full range, just like DVI. And embedded, transparent DP <=> HDMI converters exist, which complicates assumptions further.
Autodetection will result in more fragile configuration, even if you use EDID parsing. In fact, most TVs have broken/unreliable EDID, and I'd be willing to bet the number is higher than TVs that assume limited range RGB always.. If you need a reference on how much will break, take a look at interlaced modes: wlroots has disabled them for years and no one has complained.

akvadrako · 2020-06-30T23:02:41Z

I see there was a proposed DRM_MODE_FLAG_CEA_861_CE_MODE flag, I wonder if it has any chance of being implemented.

emersion · 2020-07-01T09:35:14Z

23:35 <vsyrjala> yes. "ce" modes use limited range, "it" modes use full range
23:43 <vsyrjala> this is one of those things we can't get it right 100% of the time due to broken displays
23:45 <vsyrjala> and sadly we don't know whether broken or non-broken display are more common

cyanreg · 2020-07-09T22:02:11Z

Ping? I've been running this for a while without problems.

emersion · 2020-07-09T22:03:33Z

I'd still like to make this a standard property before starting to use it.

akvadrako · 2020-07-09T22:14:51Z

I’m sure you are better informed but from here it looks like that patch to lkml that’s adds that standard property from a few months ago has stalled.

emersion · 2020-07-10T10:38:06Z

Yes, those patches need some work. For reference:

Introduces a new standard prop: https://lists.freedesktop.org/archives/dri-devel/2020-April/262153.html
Adds a TODO: https://lists.freedesktop.org/archives/dri-devel/2020-June/268149.html

cyanreg · 2020-10-20T09:03:05Z

Ping again, been running this since it was posted, had no problems, already had to help out one person who was wondering why colors look washed out.

emersion · 2020-10-20T14:30:43Z

This email thread also suggests that we should stick to full range unconditionally (especially when we'll implement color management): https://lists.freedesktop.org/archives/dri-devel/2020-October/283396.html

This PR is still blocked on registering the property as a standard one.

emersion · 2020-10-21T08:53:32Z

More info on turning "Broadcast RGB" into a generic prop: https://dri.freedesktop.org/docs/drm/gpu/todo.html#consolidate-custom-driver-modeset-properties

Matombo · 2021-06-22T16:42:22Z

This email thread also suggests that we should stick to full range unconditionally (especially when we'll implement color management): https://lists.freedesktop.org/archives/dri-devel/2020-October/283396.html

This PR is still blocked on registering the property as a standard one.

Is this the only blockage for this patch?

Because in this patchset https://lkml.org/lkml/2021/6/18/294, I standardized "Broadcast RGB" and implemented it for Intel and AMD. And if you say this MR is ready to be merged once the patch got accepted it would help with: https://www.kernel.org/doc/html/v5.12/gpu/drm-uapi.html#open-source-userspace-requirements

emersion · 2021-06-22T16:48:49Z

Yes, this is the only blocker. I trust @cyanreg that this is the correct thing to do. This patch also needs to be rebased.

cyanreg · 2021-06-22T20:59:57Z

I'm still using this locally and rebasing if needed. Repo is https://github.com/cyanreg/wlroots if you need it for now.

akvadrako · 2021-06-22T22:14:12Z

Yes, this is the only blocker. I trust @cyanreg that this is the correct thing to do. This patch also needs to be rebased.

rebased

emersion

A few minor comments

backend/drm/atomic.c

include/backend/drm/properties.h

akvadrako · 2021-07-10T19:08:31Z

A few minor comments

Thanks, I've applied the feedback and squashed.

cyanreg · 2021-08-20T10:08:57Z

Not all HDR on youtube is limited-range, some is full range.
Not all Blu-Rays are limited-range, since some Dolby Vision HDR is full range.
Who cares about DVDs, it's 2021. They're badly-mastered interlaced garbage anyway.
Not all streams are limited-range, twitch happily accepts full-range, and in fact, AMD hardware encoders by default encode in full-range. I estimate about 20-30% of twitch streams are full-range.

And finally,
All computer monitors are full-range. All. Not some. All. If they accept RGB, and are a monitor meant for computers, they operate on full-range. All TVs made in the past 15 years with a digital input accept full-range. All of them also autodetect its presence and adjust their settings accordingly. In fact, outputting limited-range is more likely to create confusion on our side, since nothing to do with computers accepts or even acknowledges limited-range RGB. No codec, including AV1 nor VP9, can signal limited-range RGB either, only full-range.

Emantor · 2021-08-20T11:34:04Z

backend/drm/atomic.c

@@ -220,6 +225,10 @@ static bool atomic_crtc_commit(struct wlr_drm_connector *conn,

 	struct atomic atom;
 	atomic_begin(&atom);
+	if (conn->props.broadcast_rgb) {


Is this required on every commit? I'd assume that it is enough to set this on modeset.

We always set all of our state on each commit. Simplifies the code and I don't think it's a real performance concern.

Makes sense.

This is a glue file to allow integration with builds.sr.ht.

emersion · 2021-11-01T16:00:40Z

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/2310

Subsurface position is considered to be a part of the parent surface's state, therefore it should be modified when the parent is committed.

This commit fixes the way the damage that doesn't come directly from the client is handled.

Two new events are added: name and description. The name is immutable. The description can be updated on-the-fly.

The multi backend was returned instead of the primary DRM backend.

This helper is responsible for listening for new DRM devices and create new child DRM backends as necessary.

Co-authored-by: Simon Ser <contact@emersion.fr>

This allows compositors to avoid sending multiple frame done events to a surface that is rendered on multiple outputs at once. This may also be used in the same way for presentation feedback.

This doesn't work if scene outputs are not used as the primary output of scene surfaces will always be NULL. Therefore, take a wlr_scene_output instead of separate wlr_scene and wlr_output arguments and rename the function to wlr_scene_output_send_frame_done(). The actual behavior of the function is unchanged.

Otherwise it will send enter events to clients that already have keyboard/pointer focus. Notably Qt applications warns about this.

Allows the compositor to submit tokens to the pool of currently active tokens. This can be useful when the launcher doesn't use or support xdg-activation-v1 by itself - e.g. when it is X11 based or use gtk_shell1.

Since [1], we can get the wl_display directly from the wl_global. [1]: https://gitlab.freedesktop.org/wayland/wayland/-/commit/2b22160fb690a76247aa9bd0be3069ff43e8239f

This avoids open-coding our own logic. The resulting code is more readable. References: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/146

This has been added in [1] and allows us to close buffer handles without manually calling drmIoctl. [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/192

This helper automates sending presentation feedback to clients based on the primary output of scene surfaces.

This fixes configure loop in Sway when clients re-send same properties on every configure event. Original issue: https://todo.sr.ht/~mil/sxmo-tickets/413

This patch makes it so we bind to zwp_linux_dmabuf_v1 version 4 and we use it to grab the main device. v4 sends supported formats via a table so we need to handle this as well. v4 allows wlroots to remove the requirement for Mesa's internal wl_drm interface.

These currently use uint32_t while they are an int32_t in the protocol.

According to [1] this should be done at each release with breaking ABI changes. [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/wikis/Core-contributor-guide#releasing-a-new-version Fixes: 7360810 ("build: bump to version 0.16.0")

Compositors should've all been updated to use the new header by now.

All of these projects use meson.override_dependency() so we can stop referencing their internal variable name to grab the depndencies we need.

The libinput backend is now optional. However, this means that a user building wlroots without the correct libinput dependencies will end up with a compositor which doesn't respond to input events. wlr_backend_autocreate is supposed to return a sensible setup, so in this case let's just error out and explain what happened. Users can suppress the check by setting WLR_LIBINPUT_NO_DEVICES=1 (already used to suppress the zero input device case inside the libinput backend). Compositors which really want to create a bare DRM backend can easily create it manually instead of using wlr_backend_autocreate.

This function was already removed in e5b5592 but it was forgotten to remove it from the header.

Some clients (e.g. mpv, Firefox) request a new wl_surface.frame callback without damaging their surface. When this happens, schedule a new output frame. Closes: https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3350

akvadrako mentioned this pull request Jun 30, 2020

Support for setting broadcast RGB to full swaywm/sway#3173

Open

akvadrako force-pushed the broadcast-rgb-property branch from 8011439 to f69eb84 Compare June 22, 2021 22:12

emersion suggested changes Jun 23, 2021

View reviewed changes

backend/drm/atomic.c Outdated Show resolved Hide resolved

backend/drm/atomic.c Outdated Show resolved Hide resolved

include/backend/drm/properties.h Outdated Show resolved Hide resolved

akvadrako force-pushed the broadcast-rgb-property branch from f69eb84 to 41331de Compare July 10, 2021 19:07

set "Broadcast RGB" = "Full" for all connections

0096bf8

akvadrako force-pushed the broadcast-rgb-property branch from 41331de to 0096bf8 Compare July 10, 2021 19:09

Emantor reviewed Aug 20, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

ci: add .gitlab-ci.yml

d1b7567

This is a glue file to allow integration with builds.sr.ht.

s/GitHub/GitLab/

8bc1086

Kirill Primak and others added 30 commits December 9, 2021 18:26

subsurface: apply position change at the right moment

df7d280

Subsurface position is considered to be a part of the parent surface's state, therefore it should be modified when the parent is committed.

surface: fix non-buffer damage handling

7964bda

This commit fixes the way the damage that doesn't come directly from the client is handled.

subsurface: don't add to parent list immediately

0fcc842

output: add support for protocol interface version 4

e3fefda

Two new events are added: name and description. The name is immutable. The description can be updated on-the-fly.

backend: fix return value of attempt_drm_backend

f6d3efb

The multi backend was returned instead of the primary DRM backend.

backend/drm: add wlr_drm_backend_monitor

d8ca494

This helper is responsible for listening for new DRM devices and create new child DRM backends as necessary.

scene: send surface enter/leave output events

0215dff

Co-authored-by: Simon Ser <contact@emersion.fr>

scene: add primary output to wlr_scene_surface

fb1f613

This allows compositors to avoid sending multiple frame done events to a surface that is rendered on multiple outputs at once. This may also be used in the same way for presentation feedback.

scene: add wlr_scene_send_frame_done()

fecde72

tinywl: use wlr_scene_send_frame_done()

ad01cdf

seat: Only resend keyboard/pointer enter to focused clients

3191492

Otherwise it will send enter events to clients that already have keyboard/pointer focus. Notably Qt applications warns about this.

xdg-activation: Allow to submit tokens

4c59f7d

Allows the compositor to submit tokens to the pool of currently active tokens. This can be useful when the launcher doesn't use or support xdg-activation-v1 by itself - e.g. when it is X11 based or use gtk_shell1.

util/global: remove wl_display arg from wlr_global_destroy_safe

4377b55

Since [1], we can get the wl_display directly from the wl_global. [1]: https://gitlab.freedesktop.org/wayland/wayland/-/commit/2b22160fb690a76247aa9bd0be3069ff43e8239f

backend/drm: use drmModeFormatModifierBlobIterNext

a15c327

This avoids open-coding our own logic. The resulting code is more readable. References: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/146

backend/drm: use drmCloseBufferHandle

bedfec9

This has been added in [1] and allows us to close buffer handles without manually calling drmIoctl. [1]: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/192

build: add subproject fallback for libdrm

c0b120a

scene: add wlr_scene_set_presentation()

07ccc6e

This helper automates sending presentation feedback to clients based on the primary output of scene surfaces.

layer-shell: don't set committed flag if the property didn't change

8e566f7

This fixes configure loop in Sway when clients re-send same properties on every configure event. Original issue: https://todo.sr.ht/~mil/sxmo-tickets/413

build: bump to version 0.16.0

7360810

layer-shell: fix type of margins

92d137c

These currently use uint32_t while they are an int32_t in the protocol.

build: bump soversion

562b24b

According to [1] this should be done at each release with breaking ABI changes. [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/wikis/Core-contributor-guide#releasing-a-new-version Fixes: 7360810 ("build: bump to version 0.16.0")

Remove wlr_box.h redirection

93e050c

Compositors should've all been updated to use the new header by now.

build: simplify Meson subproject fallbacks

b5a019d

All of these projects use meson.override_dependency() so we can stop referencing their internal variable name to grab the depndencies we need.

wlr_texture: remove wlr_texture_from_wl_drm() from header

823476e

This function was already removed in e5b5592 but it was forgotten to remove it from the header.

scene: schedule an output frame on wl_surface.frame

812951f

Some clients (e.g. mpv, Firefox) request a new wl_surface.frame callback without damaging their surface. When this happens, schedule a new output frame. Closes: https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3350

Merge branch 'master' into broadcast-rgb-property

d8f9cb7

Merge remote-tracking branch 'up/master' into broadcast-rgb-property

f71f0b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drm backend: set "Broadcast RGB" = "Full" for all connections #2310

drm backend: set "Broadcast RGB" = "Full" for all connections #2310

akvadrako commented Jun 30, 2020

emersion commented Jun 30, 2020

akvadrako commented Jun 30, 2020

cyanreg commented Jun 30, 2020

akvadrako commented Jun 30, 2020

emersion commented Jul 1, 2020

cyanreg commented Jul 9, 2020

emersion commented Jul 9, 2020

akvadrako commented Jul 9, 2020

emersion commented Jul 10, 2020

cyanreg commented Oct 20, 2020

emersion commented Oct 20, 2020

emersion commented Oct 21, 2020

Matombo commented Jun 22, 2021

emersion commented Jun 22, 2021

cyanreg commented Jun 22, 2021

akvadrako commented Jun 22, 2021

emersion left a comment

akvadrako commented Jul 10, 2021

cyanreg commented Aug 20, 2021

Emantor Aug 20, 2021

emersion Aug 20, 2021

Emantor Aug 20, 2021

This comment has been minimized.

This comment has been minimized.

emersion commented Nov 1, 2021

drm backend: set "Broadcast RGB" = "Full" for all connections #2310

Are you sure you want to change the base?

drm backend: set "Broadcast RGB" = "Full" for all connections #2310

Conversation

akvadrako commented Jun 30, 2020

emersion commented Jun 30, 2020

akvadrako commented Jun 30, 2020

cyanreg commented Jun 30, 2020

akvadrako commented Jun 30, 2020

emersion commented Jul 1, 2020

cyanreg commented Jul 9, 2020

emersion commented Jul 9, 2020

akvadrako commented Jul 9, 2020

emersion commented Jul 10, 2020

cyanreg commented Oct 20, 2020

emersion commented Oct 20, 2020

emersion commented Oct 21, 2020

Matombo commented Jun 22, 2021

emersion commented Jun 22, 2021

cyanreg commented Jun 22, 2021

akvadrako commented Jun 22, 2021

emersion left a comment

Choose a reason for hiding this comment

akvadrako commented Jul 10, 2021

cyanreg commented Aug 20, 2021

Emantor Aug 20, 2021

Choose a reason for hiding this comment

emersion Aug 20, 2021

Choose a reason for hiding this comment

Emantor Aug 20, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

emersion commented Nov 1, 2021