Cursor-plane-position-only updates are broken with VRR

[YaLTeR]

#1578 made it so cursor-plane-position-only updates are skipped when VRR is enabled. Unfortunately, this means that they are indeed skipped when VRR is enabled. So if I enable VRR on an output where no surface is updating and just move the cursor, it won't redraw and get stuck until something else happens to update.

Effectively this breaks not-on-demand VRR, or on-demand VRR where surface decided to freeze for a bit, because the cursor movement won't update the screen by itself.

smithay/src/backend/drm/compositor/mod.rs

Lines 2668 to 2684 in c3f3ac8

	// if the update only contains a cursor position update, skip it for vrr
	if self.vrr_enabled()
	&& allow_partial_update
	&& next_frame_state.planes.iter().all(|(plane, state)| {
	state.skip
	|| (self.planes.cursor.iter().any(|p| *plane == p.handle)
	&& state.buffer().map(|b| &b.fb)
	== previous_state.plane_buffer(*plane).map(|b| &b.fb))
	})
	{
	for plane in self.planes.cursor.iter() {
	let Some(state) = next_frame_state.plane_state_mut(plane.handle) else {
	continue;
	};
	state.skip = true;
	}
	}

cc @Drakulix @cmeissl

[cmeissl]

Yeah, I feared something like this is going to happen. Imo we need to leave the decision to the compositor, so that it can keep some minimum rate. The most straightforward way would be to instead of skipping to expose a similar logic in the result returned from render_frame.

[YaLTeR]

FWIW in niri and on my (probably good) monitors so far I've been reasonably happy with always redrawing on cursor updates. If it's a game then it probably hides the cursor anyway so there's no VRR breakage because there's no cursor plane to update in the first place.

There's some separate issue with cursor updates, even constrained to other outputs, breaking VRR when direct scanout isn't working, but that issue is still present even with this skipping. Might be a driver bug idk.

[YaLTeR]

Should that if block be removed for now until there's a better solution?

[Drakulix]

Should that if block be removed for now until there's a better solution?

What would you say if we just add a bool to render_frame whether to consider cursor-updates? That way a compositor could manage this manually to e.g. drive a minimum refresh rate, if only cursor-updates are happening.

[YaLTeR]

Would that work for cosmic-comp? I think if you could drive a minimum refresh rate for cursor-only updates (or drive a full refresh rate if the compositor knows there are no visible surfaces on the output at the moment), then that should be good.

In niri I'd like to always render on cursor updates for now until I have a chance to think some more about the logic. Sounds like it should be doable by always passing true to the proposed "consider cursor-only updates?" bool.

[YaLTeR]

Or maybe this logic straight up needs to live in the compositor? Since the idea to avoid triggering VRR with cursor-position-only updates applies just as well when there's no cursor plane and the cursor is composited.

[Drakulix]

Would that work for cosmic-comp? I think if you could drive a minimum refresh rate for cursor-only updates (or drive a full refresh rate if the compositor knows there are no visible surfaces on the output at the moment), then that should be good.

I haven't tried it yet, but I essentially envision making our estimated vblank timer track the minimum refresh rate in case of VRR instead (which we hopefully can pull from libdisplay-info and otherwise fallback to something like max-refresh/2). Then when we get a content update earlier, we can submit immediately disallowing cursor-only-updates and either get an empty frame (which means we continue waiting for the estimated lower vblank) or get an update and go into the Queued state. If the timer expires, we allow cursor-only updates and submit.

This would still allow any reasonable fast application to drive the monitors refresh rate.

[Drakulix]

Potential implementation (on top of DrmOutput): 54379b2