Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config/output: Search for output config fallbacks #8095

Merged
merged 2 commits into from May 2, 2024

Conversation

kennylevinsen
Copy link
Member

The original sway output config implementation enabled one output at a time, testing modes, render formats and VRR support as it went along. While this sort of fallback is easy to do, it has the downside of not considering the effect of neighbor outputs on the configuration viability.

With backend-wide commits, we can now better consider the effect of neighbor outputs, but to handle the fact that we commit all outputs at once we need to perform a more elaborate search of viable configurations.

Implement a recursive configuration search for when the primary configuration failed to apply.

@kennylevinsen
Copy link
Member Author

kennylevinsen commented Mar 28, 2024

I have tested with:

  1. Adaptive sync on 3 outputs where none support it because of GPU restrictions
  2. Forcibly failed preferred mode, downgrading to the second mode for all outputs
  3. Failed explicit modifiers

On my poor intel, going through all those combinations takes roughly 230 ms.

What I have not tested:

  1. More outputs than there are CRTCs, requiring the output to disable
  2. Render format that is not supported

@kennylevinsen
Copy link
Member Author

Re-arranged the search to do the VRR test last, which significantly speeds up matching when attempting to enable VRR when it doesn't work, without impacting tests where VRR is not an issue. search_valid_config has also been made a bit prettier.

@kennylevinsen kennylevinsen force-pushed the search_configs branch 2 times, most recently from 461b899 to c276b36 Compare March 29, 2024 17:10
@matt1606
Copy link

matt1606 commented Apr 2, 2024

Hi,

While I'm using a not standard and not supported configuration (--unsupported-gpu flag) with a patched wlroots (https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/1823#note_2146862) because of one display being connected via DisplayLink docking station, yet this set of commits broke the setup and the DisplayLink connected display is/remains off (disabled).

I've tried to track the issue and so far found that the slot for the affected display, after being acquired in struct wlr_buffer *wlr_swapchain_acquire(struct wlr_swapchain *swapchain, int *age) from wlroots render/swapchain.c gets released very quickly (few ms), so that the slot can't be skipped again by testing if(slot->acquired) and the if(slot->buffer != NULL) is still met, so that the subsequent acquire operation is being triggered. That results in seeing:

[ERROR] [wlr] [backend/drm/atomic.c:335] Failed to acquire FB for plane 31
[DEBUG] [wlr] [types/wlr_output_swapchain_manager.c:181] Test commit for 3 outputs failed

in the logs, line 3011:
sway-debug.3.log

Reverting back to commit 125c743 brings back the display, compare starting with line 2928:
sway-debug.4.log

Output of swaymsg -t get_ouptuts before and after PR:
displaylink_output_remains_disabled_swaymsg-get_outputs.txt

Also note that the DisplayLink connected display chooses a non-default mode:

[ 2083.067757] evdi: [I] (card1) Notifying mode changed: 1920x1080@60; bpp 32; pixel format AR24 little-endian (0x34325241)

Not sure, (not tested/not confirmed) but this doesn't have to be DisplayLink specific behavior and may affect any display working with non-default mode.

Regards
Mateusz

@kennylevinsen
Copy link
Member Author

Since you are commenting on this unmerged PR which implements a more aggressive search for possible configurations, did you try it to see if it improved the situation?

I've tried to track the issue and so far found that the slot for the affected display, after being acquired in struct wlr_buffer *wlr_swapchain_acquire(struct wlr_swapchain *swapchain, int *age) from wlroots render/swapchain.c gets released very quickly (few ms), so that the slot can't be skipped again by testing if(slot->acquired) and the if(slot->buffer != NULL) is still met, so that the subsequent acquire operation is being triggered.

During output enable testing, many buffers will be acquired and released quickly to find a working configuration before anything makes it to screen. We cannot test configurations without buffers with the current kernel uAPI.

The swapchain manager tries to be smart, reusing the previously used swapchain from enabled outputs if they are compatible with the configuration being tested, and reusing the previously tested swapchain if implicit/explicit modifiers made no difference.


General note: DisplayLink should be avoided like the plague, moreso than nvidia which has actually sobered up a bit in recent times. We can't really help debug much - especially when running random patches on top of wlroots as well - but we don't mind supporting it if there is a simple, non-hack fix.

@matt1606
Copy link

matt1606 commented Apr 2, 2024

Tested PR#8095 on top of:
- sway dcb142b, debian .build if you'd wish to inspect it:
sway_1.9+git20240402-1_amd64.build.txt
- wlroots 3fc66d4525916b9301236a000a6ed03311ed25a7 + the mentioned DisplayLink patch + some extra debug messages:
wlroots_extra_debug.patch.txt

Display remains disabled, log:
sway-debug.15.log

General note: DisplayLink should be avoided like the plague (...)

Really, you don't need to tell me that ))

Regards
Mateusz

@kennylevinsen
Copy link
Member Author

The "Failed to acquire FB for plane XYZ" is printed by set_plane_props if either primary_fb or cursor_fb was NULL. The swapchain manager wouldn't issue a test if it got a NULL buffer, so maybe the issue happened later when the buffer was attempted imported.

Check if drm_connector_state_update_primary_fb returned from one of the failure paths that do not log, or otherwise trace why primary_fb is NULL...

@matt1606
Copy link

matt1606 commented Apr 3, 2024

I'll try to check following your hints (it takes some time, since the codebase is new for me, same as the whole graphics topic as well this is the production environment for my daily work).

The "old", pre e2f3eba implementation didn't handle the display during the first buffer allocation attempt too, yet no "Failed to acquire FB for plane XYZ" messages were observed (because it wasn't tried to be acquired during that phase?). If I'm not mistaken it performed a fallback between both the pixel format and the modifier:

00:00:00.284 [DEBUG] [sway/config/output.c:516] Committing output DVI-I-1
00:00:00.284 [DEBUG] [wlr] [types/output/render.c:127] Attaching empty buffer to output for modeset
00:00:00.284 [DEBUG] [wlr] [backend/drm/drm.c:1276] Reallocating CRTCs
00:00:00.284 [DEBUG] [wlr] [backend/drm/drm.c:1339]   Connector DVI-I-1 (connected, needs CRTC): no CRTC → CRTC 35
00:00:00.284 [DEBUG] [wlr] [types/output/swapchain.c:27] Choosing primary buffer format XR24 (0x34325258) for output 'DVI-I-1'
00:00:00.284 [DEBUG] [wlr] [types/output/swapchain.c:96] Testing swapchain for output 'DVI-I-1'
00:00:00.284 [DEBUG] [wlr] [render/swapchain.c:106] Allocating new swapchain buffer
00:00:00.285 [DEBUG] [wlr] [render/allocator/gbm.c:144] Allocated 1920x1080 GBM buffer with format XR24 (0x34325258), modifier Y_TILED_GEN12_RC_CCS_CC (0x0100000000000008)
00:00:00.285 [DEBUG] [wlr] [render/gles2/renderer.c:158] Created GL FBO for buffer 1920x1080
00:00:00.285 [DEBUG] [wlr] [render/gles2/renderer.c:158] Created GL FBO for buffer 1920x1080
00:00:00.285 [DEBUG] [wlr] [render/swapchain.c:106] Allocating new swapchain buffer
00:00:00.285 [DEBUG] [wlr] [render/allocator/gbm.c:144] Allocated 1920x1080 GBM buffer with format AR24 (0x34325241), modifier INVALID (0x00FFFFFFFFFFFFFF)
00:00:00.287 [DEBUG] [wlr] [render/gles2/renderer.c:158] Created GL FBO for buffer 1920x1080
00:00:00.287 [INFO] [wlr] [backend/drm/drm.c:862] connector DVI-I-1: Modesetting with 1920x1080 @ 60.000 Hz
00:00:00.288 [DEBUG] [sway/config/output.c:556] Set DVI-I-1 position to 2560, 360
00:00:00.288 [DEBUG] [sway/tree/arrange.c:262] Usable area for ws: 1920x1080@0,0
00:00:00.288 [DEBUG] [sway/tree/arrange.c:296] Arranging workspace '1' at 4480.000000, 360.000000
00:00:00.288 [DEBUG] [sway/tree/arrange.c:262] Usable area for ws: 2560x1440@0,0
00:00:00.288 [DEBUG] [sway/tree/arrange.c:296] Arranging workspace '2' at 0.000000, 0.000000
00:00:00.288 [DEBUG] [sway/desktop/transaction.c:786] Transaction 0x5555b6a1a6f0 committing with 2 instructions
00:00:00.288 [DEBUG] [sway/desktop/transaction.c:682] Applying transaction 0x5555b6a1a6f0
00:00:00.289 [DEBUG] [wlr] [render/swapchain.c:106] Allocating new swapchain buffer
00:00:00.289 [DEBUG] [wlr] [render/allocator/gbm.c:144] Allocated 64x64 GBM buffer with format AR24 (0x34325241), modifier Y_TILED_GEN12_RC_CCS_CC (0x0100000000000008)
00:00:00.289 [DEBUG] [wlr] [render/gles2/renderer.c:158] Created GL FBO for buffer 64x64
00:00:00.289 [DEBUG] [wlr] [render/gles2/renderer.c:158] Created GL FBO for buffer 64x64
00:00:00.289 [DEBUG] [wlr] [render/swapchain.c:106] Allocating new swapchain buffer
00:00:00.289 [DEBUG] [wlr] [render/allocator/gbm.c:144] Allocated 64x64 GBM buffer with format AR24 (0x34325241), modifier LINEAR (0x0000000000000000)
00:00:00.289 [DEBUG] [wlr] [render/gles2/renderer.c:158] Created GL FBO for buffer 64x64
00:00:00.289 [DEBUG] [sway/tree/workspace.c:293] Workspace: Generating new workspace name for output DVI-I-1

Based on dmesg output for the "perfect" evdi driver, the correct mode for this display is: 1920x1080@60; bpp 32; pixel format AR24 little-endian (0x34325241)

[   23.786292] evdi: [I] Attaching to usb:2-3.2
[   29.176339] evdi: [I] (card1) Opened by Task 5517 (DesktopManagerE) of process 5309 (DisplayLinkMana)
[   29.177104] evdi: [I] (card1) Added i2c adapter bus number 17
[   29.177109] evdi: [I] (card1) Connected with Task 5517 (DesktopManagerE) of process 5309 (DisplayLinkMana)
[   29.177112] evdi: [I] (card1) Connector state: connected
[   33.258090] evdi: [I] (card1) Opened by Task 2996 (systemd-logind) of process 2996 (systemd-logind)
[   33.497244] evdi: [I] (card1) Opened by Task 6892 (sway) of process 6892 (sway)
[   33.528196] evdi: [I] (card1) Opened by Task 6892 (sway) of process 6892 (sway)
[   33.528202] evdi: [I] (card1) Closed by Task 6892 (sway) of process 6892 (sway)
[   34.999253] evdi: [I] (card1) Connector state: connected
[   34.999264] evdi: [I] (card1) Edid property set
[   35.003042] evdi: [I] (card1) Notifying display power state: on
[   35.003067] evdi: [I] (card1) Notifying mode changed: 1920x1080@60; bpp 32; pixel format AR24 little-endian (0x34325241)
[   35.003070] evdi: [I] (card1) Notifying display power state: on
[   35.019021] evdi: [W] evdi_painter_connect:886 (card1) Double connect - replacing 0000000019e584f3 with 0000000019e584f3
[   35.019027] evdi: [I] (card1) Connected with Task 5517 (DesktopManagerE) of process 5309 (DisplayLinkMana)
[   35.019030] evdi: [I] (card1) Connector state: connected
[   35.371136] evdi: [I] (card1) Notifying mode changed: 1920x1080@60; bpp 32; pixel format AR24 little-endian (0x34325241)

In this PR you start the fallback with changing the resolution/refresh rate first, what if performing the pixel format and/or modifier would be performed prior to that?

@kennylevinsen
Copy link
Member Author

Each try of the swapchain manager tries both explicit and implicit modifiers, the latter being the fallback. We don't try every modifier due to buffer allocation still being a little costly, and that would mean way more buffers allocated, but that's nothing new to sway.

The idea with this PR is that every possible combination is tested using cheaper test commits, stopping at the first found that works. The old logic only tested these things for each output in isolation, which could lead to an Nth output falling because previous outputs were configured in a resource-intensive manner when even though though this could be fixed. The new logic instead always looks at all outputs, searching through all possible combinations.

The only thing off the top of my head that differs - other than more buffers being allocated - is that the new logic does not mix implicit/explicit modifiers. wlr_output_swapchain_manager tries all outputs as either/or, while old sway lighting up one output at a time would try explicit/implicit modifiers for a single output when it is added...

@matt1606
Copy link

matt1606 commented Apr 3, 2024

new logic does not mix implicit/explicit modifiers

Yup, that might be the case, when there is a need for Y_TILED_GEN12_RC_CCS_CC for one output and LINEAR for other output.

I did a more verbose run with printing the slot addresses, also to handle eDP-1 and DVI-I-1 resolution ambiguity that I've seen in the logs (couldn't distinguish between eDP-1 and DVI-I-1 swapchains/buffers). Here's the log (cut after it enters a loop): sway-debug.13_.log
Short analysis:

# test commit for 1 output:
creating new swapchain            -> swapchain_1 for eDP-1
swapchain_1 slot_3 0x5612a50d84e8 -> found free slot 3 -> allocated new swapchain buffer 1920x1080 for eDP-1

# test commit for 2 outputs:
swapchain_1 slot_3 0x5612a50d84e8 -> slot 3 already acquired
swapchain_1 slot_2 0x5612a50d84c0 -> found free slot 2 -> allocated new swapchain buffer 1920x1080 for eDP-1
creating new swapchain            -> swapchain_2 for HDMI-A-1
swapchain_2 slot_3 0x5612a4751228 -> found free slot 3 -> allocated new swapchain buffer 2560x1440 for HDMI-A-1

# test commit for 3 outputs:
swapchain_2 slot_3 0x5612a4751228 -> slot 3 already acquired
swapchain_2 slot_2 0x5612a4751200 -> found free slot 2 -> allocated new swapchain buffer 2560x1440 for HDMI-A-1
swapchain_1 slot_2 0x5612a50d84c0 -> slot 2 already acquired
swapchain_1 slot_3 0x5612a50d84e8 -> slot 3 buffer not NULL -> trying to acquire
creating new swapchain            -> swapchain_3 for DVI-I-1
swapchain_3 slot_3 0x5612a4e4f0e8 -> found free slot 3 -> allocated new swapchain buffer 1920x1080 for DVI-I-1
                                  -> Failed to acquire FB for plane 31

Therefore the FB acquire attempt fails after the very first run of wlr_swapchain_acquire for the newly created swapchain (and thus buffer) for DVI-I-1 output.
It has to follow the call path:
manager_output_prepare returns true and
manager_test continues with calling
wlr_backend_test then
backend->impl->test which for DRM calls
drm_connector_test then
drm_connector_commit_state
drm_commit
drm->iface->commit
atomic_device_commit
atomic_connector_add
set_plane_props -> Failed to acquire FB for plane 31

@kennylevinsen
Copy link
Member Author

This isn't related to this PR, so please open a separate issue for it. Post the output of drm_info over there while at it.

@kennylevinsen
Copy link
Member Author

  1. Fixed some null-pointer derefs when the config is NULL
  2. Fixed handling of backends that do not have any modes
  3. Made the logging considerably easier to read

sway/config/output.c Show resolved Hide resolved
sway/config/output.c Outdated Show resolved Hide resolved
The original sway output config implementation enabled one output at a
time, testing modes, render formats and VRR support as it went along.
While this sort of fallback is easy to do, it has the downside of not
considering the effect of neighbor outputs on the configuration
viability.

With backend-wide commits, we can now better consider the effect of
neighbor outputs, but to handle the fact that we commit all outputs at
once we need to perform a more elaborate search of viable
configurations.

Implement a recursive configuration search for when the primary
configuration failed to apply.
Instead of having each search function print its various test decisions,
print the full state at the end of every search. This makes it much
clearer what state a particular test includes.
Copy link
Contributor

@bl4ckb0ne bl4ckb0ne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kennylevinsen kennylevinsen merged commit 2686afb into swaywm:master May 2, 2024
3 checks passed
@kennylevinsen kennylevinsen deleted the search_configs branch May 2, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants