swaywm / sway

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

FreeSync stops working in frame capped games when moving mouse #6832

Open Elendil211 opened 2 years ago

Elendil211 commented 2 years ago

sway version 1.7

Config is default with exception of output $monitor mode 1920x1080@164.995Hz adaptive_sync on

Behavior:

.

From what I understand, this behavior would be desired if a hardware cursor was displayed. However, it also occurs when there is no hardware cursor.

This is a problem, because limiting the frame rate in order to make use of FreeSync is desirable to get low input lag. The current behavior makes it impossible to use it.

exp80 commented 2 years ago

Probably not helpful, but I found this Reddit post from a year ago which describes the same issue. Has anybody looked into this yet? I am sorry if this comes out as rude and I understand this is an open source project and the contributors are all volunteers, but I have a hard time understanding why this issue is seemingly ignored. As I understand this completely breaks the core functionality of FreeSync in a lot of cases. Isn't this pretty important?

emersion commented 2 years ago

Ref https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/44#note_1284481

exp80 commented 2 years ago

If I understand what you linked correctly, displaying hardware cursors with VRR is still being worked on and the behaviour is not ideal, as it currently always prioritises the cursor. This is an issue, but that should only happen when the hardware cursor is visible right, or am I misunderstanding something?

From this issue:

From what I understand, this behavior would be desired if a hardware cursor was displayed. However, it also occurs when there is no hardware cursor.

Doesn't that mean that this is a seperate issue or a bug with sway? I also found this comment, which seems to be in line with what I think.

emersion commented 2 years ago

Oh, I missed the fact that no cursor was involved here.

exp80 commented 2 years ago

Sorry for the impatience but are there any plans to fix this? Even without a more proper solution with cursors and VRR, I think that this would be a great improvement, at least for my use case.

emersion commented 2 years ago

I don't have plans to investigate this, patches welcome.

YellowOnion commented 2 years ago

I noticed my monitor maxes out on video playback in Firefox (60fps video), and video games, doesn't seem related to mouse movement though, Even if I move cursor to a different monitor, still maxed at 180fps.

SeekerOfAsh commented 1 year ago

I'm also facing this issue. As described, the refresh rate of the monitor jumps to the upper limits (which in my case would be 160hz) when ever the mouse is moved. Regardless if the cursor is visible, or if the game in question is in full screen / windowed mode. For normal desktop use, this behaviour makes sense, especially in laptops, since it saves power.

That said, when a full screen application is drawn, it should be prioritized over the mouse refresh rate. I think the gnome VRR developers encountered a similar issue. Perhaps a flag could be set, to prioritize full screen apps over the hardware curser.

Sorry if I sound impatient, but is there any plans of fixing this issue in the near future?

aqxa1 commented 1 year ago

This seems to only affect desktop cursors for me now (November 12 wlroots git and sway git). Camera movement almost always works, and some games with cursors work (I guess they aren't using the desktop cursor but implementing it themselves):

Working

Not working

That being said, it does seem to affect software cursors as well (e.g. with WLR_NO_HARDWARE_CURSORS=1) which feels like a bug to me. As an aside, there is a bit of issue with multimonitor configurations, with some games jumping to my max refresh periodically that doesn't occur in single monitor config. Probably a driver bug though.

WillPower3309 commented 1 year ago

Using a game as an example, would it be possible to fix this by setting the refresh rate to the closest multiple of the frame rate of the game to the max refresh rate when the mouse is moved?

Example: 144hz monitor, game running 60fps. We could set the refresh rate to 120hz (closest multiple of 60 to 144) when the mouse is moved to allow for smooth mouse movement while also allowing the refresh rate to be a multiple of the game's fps (thereby in theory still ensuring no tearing).

rhoot commented 1 year ago

I was trying to make sway not trigger new frames when moving the mouse a couple of months ago. Turns out it's not too difficult to achieve if your needs aren't very high. That said, I imagine most people have more needs than I do, but I haven't managed to find the time to make it more generally usable. So instead I'm posting the patches here in case someone else wants to finish it and clean up the API a bit (and probably make it an option).

Specifically: With the patches applied, mouse input will never trigger a redraw if sway has a view fullscreened, and adaptive sync is enabled. If adaptive sync is not enabled, or nothing is fullscreened, mouse input triggers redraws as usual. Where that behavior falls a bit short of being "generally usable" is that it probably makes sense to limit it to some minimum refresh rate. As is, if you were to fullscreen a fully static window, you would never see your mouse cursor move. For my use that just happens to not be a problem since I only enable adaptive sync when I launch a game, and disable it again after (using scripts).

I've been using these patches for the past two months without issues. I also posted an earlier version of them in an AMD driver bug report before, and people replied to say it's working for them too. (Though please don't comment about the patches in that report, it just adds noise.)

Two things worth noting:

Github won't let me attach the patch files for some reason, so I posted them to a gist instead: https://gist.github.com/rhoot/dcead8b195054387c067919fef499c5c

aqxa1 commented 1 year ago

I tried to test the upgraded patches but it looks like fullscreen adaptive sync has regressed yet again for me (with or without your patches). I'll try and find out which part of the stack has decided to break.

EDIT: Just seems to be direct scan-out issues (still). I didn't notice since I mainly use my second GPU which doesn't trigger the issue (since direct scan-out is not implemented for now on secondary GPUs I believe).

Anyway, the patches are working as intended.

GrabbenD commented 1 year ago

@aqxa1 Hyprland supports direct scanout. The Sway patch would need to be tweaked for Hyprland's codebase though if you'd like to test it

aqxa1 commented 1 year ago

@GrabbenD Multi GPU direct scan out? I'd be happy to test it if it's adapted, although I probably don't need it as much given both of my GPUs are full dedicated GPUs with plenty of bandwidth (which I understand means I'm probably not being significantly bottlenecked).

GrabbenD commented 1 year ago

I don't know the specifics unfortunately @aqxa1

rhoot commented 1 year ago

In case anyone is actually using my patches, I've updated the ones in the gist. Changes from before:

GrabbenD commented 11 months ago

@rhoot Downside of these patches is that mouse stutters a lot in main menu or loading screens (e.g. Halo Infinite)

rhoot commented 11 months ago

Yeah, I mentioned in an earlier comment that solving that would probably be required in order to make it more generally usable:

Where that behavior falls a bit short of being "generally usable" is that it probably makes sense to limit it to some minimum refresh rate. As is, if you were to fullscreen a fully static window, you would never see your mouse cursor move. For my use that just happens to not be a problem since I only enable adaptive sync when I launch a game, and disable it again after (using scripts).

WillPower3309 commented 11 months ago

Yeah, I mentioned in an earlier comment that solving that would probably be required in order to make it more generally usable:

Where that behavior falls a bit short of being "generally usable" is that it probably makes sense to limit it to some minimum refresh rate. As is, if you were to fullscreen a fully static window, you would never see your mouse cursor move. For my use that just happens to not be a problem since I only enable adaptive sync when I launch a game, and disable it again after (using scripts).

Why not limit it to the highest multiple of your current frame rate with respect to the display's refresh rate? For example, we could have the cursor update 50 times a second if the game is running at 25 fps on a 60hz monitor

YellowOnion commented 11 months ago

@WillPower3309 FreeSync supports a range of refresh rates, your idea is generally okay, but only matters when the game drops below the minimum refresh rate supported by the monitor, and we can pick any combination of frame lengths that sum to the actual render time, for example my monitor supports 48-180hz.

@rhoot Sway already has max_render_time variable. I suspect this patch is going about limiting the redraw rate the wrong way as it effectively breaks the point of the timer.

rhoot commented 11 months ago

Don't get me wrong: I never said there's not a solution. I merely said I haven't added one in my patches. The fact that it's not "generally usable" is why it's not a PR. I'm still waiting for the AMD bug on this to be fixed before I may have the motivation to try to do it properly if no one does it earlier.

Why not limit it to the highest multiple of your current frame rate with respect to the display's refresh rate? For example, we could have the cursor update 50 times a second if the game is running at 25 fps on a 60hz monitor

Game frame time (more important than FPS for presenting) fluctuates. Even if you're going at a rock solid 25 FPS, it will fluctuate by some 10-100 µs in one direction or the other. Add frame spikes and varying GPU loads between frames and you have a range of frame times that the game renders in as opposed to just one.

In order to apply your suggestion, sway would have to predict how long it will be from the previous frame before the game renders the next. If it mispredicts and the next frame is ready way sooner than sway had expected, it could cause the game frame to be delayed as sway had just triggered a refresh to update the cursor position, and refreshes can't happen immediately after they have already happened. And the result would be that the game stutters for a frame, since the game had already assumed the time between the previous frame and this new frame.

Personally I would much rather have a dumb but reliable solution than one that tries to be smart. It will only have an effect when the game runs at a really low frame rate anyway, which (hopefully) is not typical. Basically just using the minimum rate supported by the monitor, or some user configurable value with some sensible default if that's too low (e.g my minimum is 1 Hz). KWin's PR on this appears to be leaning in the same direction, with using the maximum of 40 or the monitor's lowest VRR rate.

Edit: I guess I should at least acknowledge games that intentionally limit rendering to something low, like 30 FPS. But those games typically don't use a mouse since it would be a horrible experience, so cursor updates don't apply to those games.

Sway already has max_render_time variable. I suspect this patch is going about limiting the redraw rate the wrong way as it effectively breaks the point of the timer.

Did you look at the code in the patch? It doesn't affect the timer at all. All it does is:

  1. Make wlroots not request a new frame to be rendered when the cursor moves.
  2. When software cursors are enabled, ignore the damage they cause when determining if it's time to render, instead relying solely on client damage. Cursor damage gets reapplied just before rendering instead.
  3. Toggle it on/off depending on if adaptive sync is enabled, and if something is fullscreened.

It doesn't limit the redraw rate at all.

YellowOnion commented 11 months ago
  1. Make wlroots not request a new frame to be rendered when the cursor moves.

But it should redraw the frame at max_render_time irrespective if the cursor moved or not. as I said it breaks the timer as the function output_repaint_timer_handler now is a nop when the cursor is the only thing moving, as @GrabbenD mentioned, it stutters because it's not redrawing / rendering at the minimum framerate anymore.

@rhoot the function you probably want to modify is wlr_output_cursor_move(), it calls drm_connector_move_cursor and sets the damage for the software cursor.

rhoot commented 11 months ago

But it should redraw the frame at max_render_time irrespective if the cursor moved or not. [...] it stutters because it's not redrawing / rendering at the minimum framerate anymore.

It sounds like you think max_render_time does something different than what it does? It doesn't configure the time to wait between renders. It configures how late before a monitor refresh to defer rendering when a frame is needed. If you set it to 1 ms sway will try to render the next frame 1 ms before the monitor refreshes, if there's anything to render.

And sure you could argue that the cursor moving is "something to render". But if you let the cursor trigger that frame, it will lead to stutter. That's literally what this issue you're commenting on is about: Making the cursor not trigger frames. If the cursor moves and it causes sway to render/present its frame, if the game then finishes rendering its frame shortly after that, that game frame can't be shown until the next smallest monitor refresh interval.

This is what one would want, where every time a game finishes rendering, sway can immediately render it and the monitor can immediately present it. Silky smooth gameplay.

Ideal sway frames

Now say I move the cursor. And I let sway trigger a frame for that. Since this example monitor supports up to 100 Hz, with max_render_time set to 1, sway would try to render it after 9 ms from the previous frame so it can have the frame ready to go at 10 ms. Since that's the earliest the monitor can refresh. But if it does, you get stutter.

Non-ideal cursor move

Note the second 17 ms frame now being presented for 20 ms, and the third 17 ms frame being presented for 13 ms (numbers are rounded). The goal is to instead have it do this, while playing a game. Back to silky smooth gameplay.

Ideal cursor move

So not having the cursor trigger a render is the entire point of my patch, as well as the point of this whole discussion. Obviously once the frame rate drops below something acceptable for interactivity it needs to start triggering frames for cursors again and you have to deal with the stutter. But for that, the time to wait with rendering would be better to base on the lowest refresh rate supported rather than the highest. On this example monitor it would then start letting the cursor trigger frames at 40 Hz instead. Or better yet, let the user configure the limit if they want.

the function you probably want to modify is wlr_output_cursor_move(), it calls drm_connector_move_cursor and sets the damage for the software cursor.

The damage is mainly irrelevant. For one, only software cursors cause damage and most people very likely aren't using software cursors. And secondly, as I already described above, the entire point of this issue/patch is to make cursor moves not trigger frames. The damage caused by software cursors just have to be reapplied before actually rendering.

YellowOnion commented 11 months ago

@rhoot You're right about max_render_time I got it confused with the refresh timer.

The problem is you're not disabling the cursor trigger, you're attempting to turn it off after the redraw has been triggered. move_cursor triggers repaint_handler to be called, the code is ugly and broken when the game driving the display freezes.

My monitor has a range of 48-180hz, a correct implementation should repaint the display, with the cursor moving, at 48hz minimum, just like how a non-FreeSync monitor repaint every frame at 60hz.

The code should look like this: https://github.com/YellowOnion/wlroots/commit/deferred-cursor-move https://github.com/YellowOnion/sway/commit/deferred-cursor-move

rhoot commented 11 months ago

The problem is you're not disabling the cursor trigger, you're attempting to turn it off after the redraw has been triggered.

No? The wlroots patch specifically disables triggering redraws. Nothing else is triggering redraws from cursor moves. Unless you mean the no_frames_for_cursor_moves bool updating a frame late? That was of no consequence to me since all future frames should be affected. I can live with one frame of delay between it activating and deactivating. That was just the easiest place to hook up logic for fullscreen vs non-fullscreen.

the code is ugly and broken

I thought I was pretty up front about it being ugly and broken, but I guess I could have been more explicit about it? It does work (I've been using it for over half a year). I just did the bare minimum work possible to get it working for myself for my own use case, not caring if it was hacky: That just makes it easier to rebase when sway/wlroots updates, since it has smaller surface area. And personally I don't care about the cursor freezing since it doesn't affect me.

Like I said, there's a reason I didn't make a PR with it and encouraged others to make it more usable. You are more than welcome to do it properly, which it looks like you already started. :+1:

My monitor has a range of 48-180hz, a correct implementation should repaint the display, with the cursor moving, at 48hz minimum

Which again, is exactly what I said. I just never got to that point, since I've been waiting for the AMD driver bug to be fixed.

YellowOnion commented 11 months ago

@rhoot I was just saying that because the implementation started out wrong, you should have approached it from a different angle (as per my original comment saying you shouldn't turn it on and off in timer_handler).

I'll leave it be now that you inspired me to write my own patch that does it the correct way™

\@EVERYONE: VRR fix

This patch is for v1.9, so feel free to test it.

This supports any focused window! and allows you to set a max latency like so:

$ swaymsg '[title="Factorio"] max_cursor_latency 8333'

The value is in microseconds, this will force Factorio (a 60fps locked game) to run at 120fps when moving the cursor.

Right now there no way to read the EDID info for lowest support Hz so I've just hard coded 30fps as the limit for now.

https://github.com/YellowOnion/sway/commit/deferred-cursor-move

I do not see the AMD driver bug issue, but maybe I'm blind or just lucky with my card of choice. But I do see an issue with Scan out driving monitor at max fps, so might need to run with -Dnoscanout

I've opened up a MR on wlroots, https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4340

rhoot commented 11 months ago

I updated your commits to work on latest master of sway/wlroots: https://gist.github.com/rhoot/5d47b0aabe96f6d44659d2f40329860f

Seems to be working pretty well, once configured for a view. I tried running gamescope -r 60 vkcube with this and while my monitor did jump up to 65 Hz once or twice, that could also have been due to other factors. It only seemed to happen shortly after fullscreening it and then go back down to 60 and stay there. I'll try running this for a while. :+1:

That said, I think it'd be nicer to make the config be specified in terms of updates per second instead of microseconds per frame. So something like [title="Factorio"] min_cursor_fps 120. I think max_cursor_latency makes sense in the API, but in the user facing config thinking in terms of FPS is just way simpler.

YellowOnion commented 11 months ago

Looks like you rebased the latest version cool :-), neither wayland, wlroots, or sway have access to the edid VVR info (like minimum, or horizontal khz), which could help with better timing, I noticed some of the spikes myself (worst being 48hz app, driving monitor at 95hz lol), I did try to improve the accuracy after noticing a bug/oversight, but then replaced it with some other code that might make it worse off.

But yeah the current code is designed to deliver cursor updates in a timely manner, not a smooth manner.

For the sway user control, I was just trying to get some minimal implementation working, tbh I don't like integer fps differences and how they lose precision the lower the fps is, lots of monitors are not actually 60fps (it depends on horizontal frequency, resolution, and the vblank time), especially with VRR on (I still need to workout the math for my monitor to confirm this), and all feature film is not 24fps (they're officially defined as 24000/1001), TV also has a similar denominator, using time/latency has better precision for low fps scenarios, so I'm thinking about how to get better accuracy before just doing a the naive _fps parameter.

I have a few ideas that could be combined.

It's also worth mention that I'm pretty sure Sway and wlroots don't make any attempts to smooth updates in general either, from the code I read, which might explain why there's reports of flicking in sway, if you have a windowed app open, and swaybar or a chat client updates for example, both force the display to update. This patch only changes the behavior of cursor redraws so it's a little outside the scope of cursor management.

max_render_time also has a heavy impact on timing, it's currently using the max hz of display, and a low-res wayland timer, I was surprised that I could pull an extra 20% FPS setting it to 6ms, instead of "off", which has me curious about how the entire graphics pipe interacts here.

GrabbenD commented 10 months ago

@YellowOnion What the formula to calculate 8333 for a 120Hz display?

YellowOnion commented 10 months ago

@YellowOnion What the formula to calculate 8333 for a 120Hz display?

It's not dependant on your display, you can pick any number you want if you want the cursor to update at a specific rate:

1000000/Hz

$ echo $((1000000 / 120))
8333
GrabbenD commented 10 months ago

Thank you @YellowOnion

Is there actually any way to use max_cursor_latency + adaptive_sync only for fullscreen windows?

YellowOnion commented 10 months ago

@GrabbenD I think you need to write a script to do that. swaymsg -m -t SUBSCRIBE '["window"]

aqxa1 commented 10 months ago

So I got around to testing this. Presumably you need to set the cursor rate to the minimum VRR refresh for it to work throughout the VRR range (I.e. mouse should never move at max refresh)? Because if I tried setting a multiple (say 120hz/8333ms) with a 144hz max refresh, it would still sometimes jump to 144hz, even if I locked the app to 60fps. Setting a multiple of the minimum didn't work either (54fps -> 118fps/8474ms).

However, setting to the minimum works amazingly well and since the minimum is almost 60fps anyway, that's fine for my purposes.

EDIT: And the rebased patches against current git don't seem to work properly for me. It compiles, and the settings all seem to apply properly going by the logs, but cursor jumps to max throughout the range.

GrabbenD commented 3 months ago

New patches are available for addressing cursor plane updates being dropped with VRR

https://gitlab.freedesktop.org/drm/amd/-/issues/2186#note_2368021

Hope all goes well considering I've had exceptionally good experience with FreeSync in Sway 1.9 + Wlroots 0.17


Another area of improvement worth investigating is cursor integration with Explicit Sync. I've seen mentions of this technology having the potential to improve performance of cursor panes with VRR

YellowOnion commented 3 months ago

Hey so I pushed patches for sway 1.9 / wlroots 0.17

Not really sure what to do about the fluctuating fps...I think the issue is other apps on screen or general sway rendering stuff.

Even wayland only has a 1ms resolution on display updates...which seems to be extremely lacking for precise redraw timing.

Maybe tweak max_render_time

YellowOnion commented 3 months ago

Actually I found two timing bugs, surprised it was drawing anything tbh...numbers look far more consistent, and should handle scanout mode better too.

Faugus commented 2 months ago

New patches are available for addressing cursor plane updates being dropped with VRR

https://gitlab.freedesktop.org/drm/amd/-/issues/2186#note_2368021

Hope all goes well considering I've had exceptionally good experience with FreeSync in Sway 1.9 + Wlroots 0.17

Another area of improvement worth investigating is cursor integration with Explicit Sync. I've seen mentions of this technology having the potential to improve performance of cursor panes with VRR

I tried these patches and they don't fix the problem.

GrabbenD commented 2 months ago

@Faugus Please share your feedback under the linked comment if you want AMDGPU developers to see it

https://gitlab.freedesktop.org/drm/amd/-/issues/2186#note_2368021

Faugus commented 2 months ago

@GrabbenD I mean they fix the stuttery cursor as shown in the https://gitlab.freedesktop.org/drm/amd/-/issues/2186#note_2368021

But they don't fix the Freesync breaking when moving the mouse on wlroots.