Skip to content

Conversation

@cmeissl
Copy link
Contributor

@cmeissl cmeissl commented Mar 16, 2025

It does not implement support for custom mode lines like discussed in #333, but setting a custom mode calculated from cvt timtings. I will extend this with support for custom mode lines in a separate PR.

@cmeissl cmeissl force-pushed the feature/custom_modes branch from 58f7286 to d07d3e1 Compare March 16, 2025 21:41
Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Thanks, the config syntax and the core logic seems ok. For the custom modeline extension I suppose we could have an extra modeline "1 2 3 4 ..." line that overrides the mode line if present?

The main leftover thing is advertising the custom mode to wlr output management.

Also, similarly, we need to advertise the custom mode (if any) in niri_ipc::Output and its current_mode. In this case I believe we could add an is_custom: bool to niri_ipc::Mode then just put the custom mode alongside the normal modes. And print it accordingly from niri msg outputs.

Comment on lines +3676 to +3681
scale 2
transform "flipped-90"
position x=10 y=20
mode custom=true "1920x1080@144"
variable-refresh-rate on-demand=true
background-color "rgba(25, 25, 102, 1.0)"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
scale 2
transform "flipped-90"
position x=10 y=20
mode custom=true "1920x1080@144"
variable-refresh-rate on-demand=true
background-color "rgba(25, 25, 102, 1.0)"
mode custom=true "1920x1080@144"

output_state.redraw_state = RedrawState::WaitingForEstimatedVBlank(token);
}

pub fn calculate_mode_cvt(width: u16, height: u16, refresh: f64) -> control::Mode {
Copy link
Owner

Choose a reason for hiding this comment

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

Would be good to have a few snapshot tests for this function, like here for example:

niri/src/utils/scale.rs

Lines 111 to 115 in 9fb02b9

#[test]
fn test_round_scale() {
assert_snapshot!(closest_representable_scale(1.3), @"1.3");
assert_snapshot!(closest_representable_scale(1.31), @"1.3083333333333333");
assert_snapshot!(closest_representable_scale(1.32), @"1.3166666666666667");

There's an assert_debug_snapshot!() for using the Debug impl.

Comment on lines +76 to +82
<sup>Since: unreleased</sup> A custom mode can be activated by setting `custom=true`, in which case the refresh rate is mandatory.

```kdl
/// Set a custom mode not offered by the connected display
output "HDMI-A-1" {
mode custom=true "2560x1440@143.912"
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
<sup>Since: unreleased</sup> A custom mode can be activated by setting `custom=true`, in which case the refresh rate is mandatory.
```kdl
/// Set a custom mode not offered by the connected display
output "HDMI-A-1" {
mode custom=true "2560x1440@143.912"
}
<sup>Since: next release</sup> You can configure a custom mode (not offered by the monitor) by setting `custom=true`.
In this case, the refresh rate is mandatory.
> [!CAUTION]
> Custom modes may damage your monitor, especially if it's a CRT.
> Follow the maximum supported limits in your monitor's instructions.
```kdl
// Use a custom mode for this display.
output "HDMI-A-1" {
mode custom=true "2560x1440@143.912"
}

};
let mut name: [core::ffi::c_char; 32] = [0; 32];
let min_length = name.len().min(mode_name_bytes.len());
name[0..min_length].copy_from_slice(&mode_name_bytes[0..min_length]);
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't need to be null-terminated right?

Copy link
Owner

Choose a reason for hiding this comment

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

Does the custom mode need to be advertised to the client as part of the output modes? I think that's the only way to set it as current_mode.

It's not entirely clear what wlroots does in this case, but I think it does send the custom mode as one of the fake modes:

https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/22db307e4cd63f64228aaf3c763764470f112982/types/wlr_output_management_v1.c#L877

In our case here, we need to be careful with the mode index when the custom mode is set/unset, though I believe that should already be covered by the old.modes.len() == conf.modes.len() in the diffing code.

@rwx7
Copy link

rwx7 commented Jun 10, 2025

My monitor's native resolution isn't recognized properly with an Intel Arc A310 on Arch and Fedora. Monitor works with Windows and Macbook.

I merged this PR into my local branch. I ran cvt -r 2560 1440 60 to generate a modeline that works in both Hyprland and Sway. I configured niri to use the calculated refresh rate and verified with niri validate.

output "HDMI-A-3" {
   mode custom=true "2560x1440@59.95"
   // mode custom=true "2560x1440@60"
}

Result is a blank screen for both mode entries.

@wrldspawn
Copy link

Tested this PR in a VM (trying to use ultrawide resolutions but VMware doesn't support them by default)

Result is a blank screen for both mode entries.

From log output: Smithay is erroring when creating DRM compositor with "No supported plane buffer format found"

@YaLTeR
Copy link
Owner

YaLTeR commented Oct 29, 2025

Implemented in #2479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants