backend/drm: Fix up tearing for atomic modesetting #1996

kode54 · 2024-07-18T00:14:41Z

Remove the NO_ATOMIC check, and add a conditional fallback for errors, which usually result from properties being applied during a flip, such as hardware cursor image changes. Falling back is limited so that persistent errors do not keep immediate flips running forever, but instead disable them until an output is reset. Verified working at least on AMD Radeon 6700 XT.

src/output.c

Consolatis · 2024-07-18T02:20:14Z

Thanks for the PR.

LGTM, not tested.
Not merging right away to allow for further feedback.

ahesford

I don't think we should expose a new configuration option for this. It's awkward, confusing, and will be obsolete whenever we (or wlroots, or whoever is the authority here) considers tearing allowance with atomic modesetting non-experimental. At best, we should note in the manual page (and perhaps in a comment in rc.xml.all) that enabling this option without setting WLR_DRM_NO_ATOMIC is experimental and not yet recommended for "production". That has the added benefit of simplifying the code, because we can drop the checks entirely instead of gating them behind another option. (Alternatively, we can log a warning if WLR_DRM_NO_ATOMIC is not set when tearing is enabled, and then remove the warning at some point in the future.)

ahesford · 2024-07-18T02:44:35Z

src/output.c

+/* consecutive commit count before disabling until output reset */
+#define TEARING_FAIL_LIMIT 120
+


This is an arbitrary magic number. I think it would be better tied to the output refresh rate (e.g., throttle or disable the tearing preference after some fixed number of seconds) or perhaps exposed as a configuration option.

ahesford · 2024-07-18T02:45:28Z

include/labwc.h

@@ -389,6 +389,8 @@ struct output {

 	bool leased;
 	bool gamma_lut_changed;
+
+	uint32_t tearing_fail_count;


I believe our normal style would dictate something like nr_tearing_failures rather than tearing_fail_count.

Using uint32_t seems like unnecessary specificity here, but I guess we don't use unsigned int anywhere else in that header. I wonder if the other uint32_t declarations are mandated by things like the wlroots API, which wouldn't apply to our internal counter.

ahesford · 2024-07-18T02:46:46Z

src/output.c

-		lab_wlr_scene_output_commit(output->scene_output,
-			&output->pending);
+			get_tearing_preference(output) &&
+			output->tearing_fail_count < TEARING_FAIL_LIMIT;


This check should be applied inside get_tearing_preference.

ahesford · 2024-07-18T03:01:47Z

src/output.c

+		if (!lab_wlr_scene_output_commit(output->scene_output,
+				&output->pending) && output->pending.tearing_page_flip) {
+			if (++output->tearing_fail_count >= TEARING_FAIL_LIMIT) {
+				wlr_log(WLR_INFO, "Tearing failed for %u consecutive"
+					" frames, disabling", (unsigned int)TEARING_FAIL_LIMIT);
+			}
+			output->pending.tearing_page_flip = false;
+			lab_wlr_scene_output_commit(output->scene_output,
+				&output->pending);
+		} else if (output->pending.tearing_page_flip) {
+			output->tearing_fail_count = 0;
+		}


I think restructing this makes the logic a bit easier to read through (the setting of scene_output could be moved further up and used in the sanity check at the top of the function):

Suggested change

if (!lab_wlr_scene_output_commit(output->scene_output,

&output->pending) && output->pending.tearing_page_flip) {

if (++output->tearing_fail_count >= TEARING_FAIL_LIMIT) {

wlr_log(WLR_INFO, "Tearing failed for %u consecutive"

" frames, disabling", (unsigned int)TEARING_FAIL_LIMIT);

}

output->pending.tearing_page_flip = false;

lab_wlr_scene_output_commit(output->scene_output,

&output->pending);

} else if (output->pending.tearing_page_flip) {

output->tearing_fail_count = 0;

}

struct wlr_scene_output *scene_output = output->scene_output;

struct wlr_output_state *pending = &output->pending;

bool committed =

lab_wlr_scene_output_commit(scene_output, pending);

if (!pending->tearing_page_flip) {

return;

}

if (committed) {

output->tearing_fail_count = 0;

} else {

if (++output->tearing_fail_count >= TEARING_FAIL_LIMIT) {

wlr_log(WLR_INFO, "setting tearing allowance failed "

"for %u consecutive frames, disabling",

(unsigned int)TEARING_FAIL_LIMIT);

}

pending->tearing_page_flip = false;

lab_wlr_scene_output_commit(scene_output, pending);

}

ahesford

I think this is looking good. With a nitpick and some trivial documentation cleanup, I think this will be ready to go.

ahesford · 2024-07-20T03:02:54Z

docs/labwc-config.5.scd

 	Allow tearing to reduce input lag. Default is no.
-	This option requires setting the environment variable
-	WLR_DRM_NO_ATOMIC=1.
+	Use of this variable without setting the environment variable
+	WLR_DRM_NO_ATOMIC=1 is considered experimental at this time, and
+	may have minor issues if used in combination with hardware cursors,
+	though none were observed yet as a noticeable problem in testing.
 	*yes* allow tearing if requested by the active window.


It seems the existing description was not properly edited when we added it. Please drop the final sentence, and maybe we can refine the description for clarity:

Allow tearing, if requested by the active window, to reduce input lag. Default is no. Note: enabling this option with atomic mode setting is experimental. If you experience undesirable side effects when tearing is allowed, consider setting the environment variable WLR_DRM_NO_ATOMIC=1 when launching labwc.

ahesford · 2024-07-20T03:03:41Z

src/output.c

+{
+	/* Two seconds worth of frames, guessing 60Hz if refresh is zero */
+	unsigned int refresh = output->wlr_output->refresh;
+	return refresh ? refresh / 500 : 120;


In the interest of Postel's law:

Suggested change

return refresh ? refresh / 500 : 120;

return refresh > 0 ? refresh / 500 : 120;

Additionally, track errors and abandon the tearing allowance when it cannot be set for two-seconds' worth of consecutive frames.

ahesford · 2024-07-20T12:11:42Z

Thanks!

Consolatis reviewed Jul 18, 2024

View reviewed changes

src/output.c Outdated Show resolved Hide resolved

Consolatis reviewed Jul 18, 2024

View reviewed changes

src/output.c Outdated Show resolved Hide resolved

kode54 force-pushed the atomic-modesetting-tearing branch 4 times, most recently from bc0bbec to 71d8f2e Compare July 18, 2024 02:08

ahesford requested changes Jul 18, 2024

View reviewed changes

kode54 force-pushed the atomic-modesetting-tearing branch 6 times, most recently from 3a55005 to fcc6ff6 Compare July 18, 2024 08:56

ahesford requested changes Jul 20, 2024

View reviewed changes

kode54 force-pushed the atomic-modesetting-tearing branch 3 times, most recently from 74a4c1d to dda1d43 Compare July 20, 2024 09:31

output: allow tearing with atomic mode setting

c8fd81e

Additionally, track errors and abandon the tearing allowance when it cannot be set for two-seconds' worth of consecutive frames.

ahesford force-pushed the atomic-modesetting-tearing branch from dda1d43 to c8fd81e Compare July 20, 2024 12:02

ahesford approved these changes Jul 20, 2024

View reviewed changes

ahesford merged commit d033a2f into labwc:master Jul 20, 2024
4 of 5 checks passed

Consolatis mentioned this pull request Jul 24, 2024

feat: add tearing fullscreen options #1941

Merged

RicArch97 mentioned this pull request Oct 9, 2024

Tearing preference is fully denied as result of tearing page-flip commit failures #2229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backend/drm: Fix up tearing for atomic modesetting #1996

backend/drm: Fix up tearing for atomic modesetting #1996

		/* consecutive commit count before disabling until output reset */
		#define TEARING_FAIL_LIMIT 120

	return refresh ? refresh / 500 : 120;
	return refresh > 0 ? refresh / 500 : 120;

backend/drm: Fix up tearing for atomic modesetting #1996

backend/drm: Fix up tearing for atomic modesetting #1996

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment