Use drm-extras for monitor info

And also remove matching outputs by serial. TODO: add matching by the new serial string

.github/workflows/ci.pinnacle.yml

@@ -59,10 +59,10 @@ jobs:
         uses: extractions/setup-just@v1
       - name: Test
         if: ${{ runner.debug != '1' }}
-        run: just install test --no-default-features -- --test-threads=1
+        run: just install test
       - name: Test (debug)
         if: ${{ runner.debug == '1' }}
-        run: RUST_LOG=debug RUST_BACKTRACE=1 just install test --no-default-features -- --nocapture --test-threads=1
+        run: RUST_LOG=debug RUST_BACKTRACE=1 just install test -- --nocapture
   check-format:
     runs-on: ubuntu-24.04
     name: Check formatting

Cargo.lock

@@ -2302,7 +2302,7 @@ dependencies = [
  "bitflags 2.6.0",
  "libc",
  "libdisplay-info-derive",
- "libdisplay-info-sys 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "libdisplay-info-sys",
  "thiserror 1.0.69",
 ]
 
@@ -2323,11 +2323,6 @@ version = "0.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "ea8cec1fa7872b621f40c756bc1304b1a975461282e250b0e76737b037c0c236"
 
-[[package]]
-name = "libdisplay-info-sys"
-version = "0.1.0"
-source = "git+https://github.com/Smithay/libdisplay-info-rs?rev=a482d0d#a482d0d4b71762c13d40fa394efe04473916f31c"
-
 [[package]]
 name = "libloading"
 version = "0.7.4"
@@ -3197,7 +3192,7 @@ dependencies = [
  "drm-sys 0.8.0",
  "gag",
  "indexmap 2.7.0",
- "libdisplay-info-sys 0.1.0 (git+https://github.com/Smithay/libdisplay-info-rs?rev=a482d0d)",
+ "libdisplay-info",
  "pinnacle",
  "pinnacle-api",
  "pinnacle-api-defs",

Cargo.toml

@@ -135,8 +135,8 @@ chrono = "0.4.39"
 bytemuck = "1.20.0"
 pinnacle-api = { path = "./api/rust", default-features = false }
 gag = "1.0.0"
-drm-sys = "0.8.0" # TODO: remove and use libdisplay-info
-libdisplay-info-sys = { git = "https://github.com/Smithay/libdisplay-info-rs", rev = "a482d0d" }
+drm-sys = "0.8.0"
+libdisplay-info = "0.1.0"
 indexmap = { workspace = true }
 snowcap = { path = "./snowcap", optional = true }
 snowcap-api = { path = "./snowcap/api/rust", optional = true }

api/lua/pinnacle/grpc/defs.lua

@@ -401,6 +401,7 @@ local pinnacle_signal_v0alpha1_StreamControl = {
 ---@field keyboard_focus_stack_window_ids integer[]?
 ---@field enabled boolean?
 ---@field powered boolean?
+---@field serial_str string?
 
 ---@class pinnacle.render.v0alpha1.SetUpscaleFilterRequest
 ---@field filter pinnacle.render.v0alpha1.Filter?

api/lua/pinnacle/output.lua

@@ -162,13 +162,8 @@ end
 ---
 ---@return boolean
 local function output_id_matches(id_str, op)
-    if id_str:match("^serial:") then
-        local serial = tonumber(id_str:sub(8))
-        return serial and serial == op:serial() or false
-    else
     return id_str == op.name
 end
-end
 
 ---@class OutputSetup
 ---@field filter (fun(output: OutputHandle): boolean)? -- A filter for wildcard matches that should return true if this setup should apply to the passed in output.
@@ -195,11 +190,6 @@ end
 ---
 ---Otherwise, keys will attempt to match the exact name of an output.
 ---
----Use `"serial:<number>"` to match outputs by their EDID serial. For example, `"serial:143256"`.
----Note that not all displays have EDID serials. Also, serials are not guaranteed to be unique.
----If you're unlucky enough to have two displays with the same serial, you'll have to use their names
----or filter with wildcards instead.
----
 ---##### Setups
 ---
 ---If an output is matched, the corresponding `OutputSetup` entry will be applied to it.
@@ -233,8 +223,6 @@ end
 ---    ["eDP-1"] = {
 ---        tags = { "6", "7" },
 ---    },
----    -- Match an output by its EDID serial number
----    ["serial:235987"] = { ... }
 ---})
 ---```
 ---
@@ -362,9 +350,6 @@ end
 ---
 ---Keys for `locs` should be output identifiers. These are strings of
 ---the name of the output, for example "eDP-1" or "HDMI-A-1".
----Additionally, if you want to match the EDID serial of an output,
----prepend the serial with "serial:", for example "serial:174652".
----You can find this by doing `get-edid | edid-decode`.
 ---
 ---#### Fallback relative-tos
 ---
@@ -400,16 +385,6 @@ end
 ---
 --- -- Only relayout on output connect and resize
 ---Output.setup_locs({ "connect", "resize" }, { ... })
----
---- -- Use EDID serials for identification.
---- -- You can run
---- -- require("pinnacle").run(function(Pinnacle)
---- --     print(Pinnacle.output.get_focused():serial())
---- -- end)
---- -- in a Lua repl to find the EDID serial of the focused output.
----Output.setup_locs("all" {
----    ["serial:139487"] = { ... },
----})
 ---```
 ---
 ---@param update_locs_on (UpdateLocsOn)[] | "all"
@@ -999,7 +974,7 @@ end
 ---@field tags TagHandle[]
 ---@field scale number?
 ---@field transform Transform?
----@field serial integer?
+---@field serial string?
 ---@field keyboard_focus_stack WindowHandle[]
 ---@field enabled boolean?
 ---@field powered boolean?
@@ -1043,7 +1018,7 @@ function OutputHandle:props()
         tags = tag_handles,
         scale = response.scale,
         transform = transform_name_to_code[response.transform] --[[@as Transform?]],
-        serial = response.serial,
+        serial = response.serial_str,
         keyboard_focus_stack = keyboard_focus_stack_handles,
         enabled = response.enabled,
         powered = response.powered,
@@ -1197,11 +1172,11 @@ function OutputHandle:transform()
     return self:props().transform
 end
 
----Get this output's EDID serial number.
+---Get this output's EDID serial.
 ---
 ---Shorthand for `handle:props().serial`.
 ---
----@return integer?
+---@return string?
 function OutputHandle:serial()
     return self:props().serial
 end

api/protocol/pinnacle/output/v0alpha1/output.proto

@@ -118,6 +118,7 @@ message GetPropertiesResponse {
   repeated uint32 keyboard_focus_stack_window_ids = 17;
   optional bool enabled = 18;
   optional bool powered = 19;
+  optional string serial_str = 20;
 }
 
 service OutputService {

api/rust/src/output.rs

@@ -9,7 +9,7 @@
 //! This module provides [`Output`], which allows you to get [`OutputHandle`]s for different
 //! connected monitors and set them up.
 
-use std::{num::NonZeroU32, str::FromStr};
+use std::str::FromStr;
 
 use futures::FutureExt;
 use pinnacle_api_defs::pinnacle::output::{
@@ -555,13 +555,6 @@ pub enum OutputLoc {
 pub enum OutputId {
     /// Identify using the output's name.
     Name(String),
-    /// Identify using the output's EDID serial number.
-    ///
-    /// Note: some displays (like laptop screens) don't have a serial number, in which case this won't match it.
-    /// Additionally the Rust API assumes monitor serial numbers are unique.
-    /// If you're unlucky enough to have two monitors with the same serial number,
-    /// use [`OutputId::Name`] instead.
-    Serial(NonZeroU32),
 }
 
 impl OutputId {
@@ -577,7 +570,6 @@ impl OutputId {
     pub fn matches(&self, output: &OutputHandle) -> bool {
         match self {
             OutputId::Name(name) => *name == output.name(),
-            OutputId::Serial(serial) => Some(serial.get()) == output.serial(),
         }
     }
 }
@@ -1028,7 +1020,7 @@ impl OutputHandle {
                 .collect(),
             scale: response.scale,
             transform: response.transform.and_then(|tf| tf.try_into().ok()),
-            serial: response.serial,
+            serial: response.serial_str,
             keyboard_focus_stack: response
                 .keyboard_focus_stack_window_ids
                 .into_iter()
@@ -1227,15 +1219,15 @@ impl OutputHandle {
         self.props_async().await.transform
     }
 
-    /// Get this output's EDID serial number.
+    /// Get this output's EDID serial.
     ///
     /// Shorthand for `self.props().serial`
-    pub fn serial(&self) -> Option<u32> {
+    pub fn serial(&self) -> Option<String> {
         self.props().serial
     }
 
     /// The async version of [`OutputHandle::serial`].
-    pub async fn serial_async(&self) -> Option<u32> {
+    pub async fn serial_async(&self) -> Option<String> {
         self.props_async().await.serial
     }
 
@@ -1355,8 +1347,8 @@ pub struct OutputProperties {
     pub scale: Option<f32>,
     /// This output's transform.
     pub transform: Option<Transform>,
-    /// This output's EDID serial number.
-    pub serial: Option<u32>,
+    /// This output's EDID serial.
+    pub serial: Option<String>,
     /// This output's window keyboard focus stack.
     pub keyboard_focus_stack: Vec<WindowHandle>,
     /// Whether this output is enabled.

justfile

@@ -101,7 +101,7 @@ run *args: gen-lua-pb-defs
 
 # Run `cargo test`
 test *args: gen-lua-pb-defs
-    cargo test {{args}}
+    cargo test --no-default-features {{args}}
 
 compile-wlcs:
     #!/usr/bin/env bash

src/api.rs

@@ -1477,9 +1477,11 @@ impl output_service_server::OutputService for OutputService {
                 }) as i32
             });
 
-            let serial = output.as_ref().and_then(|output| {
-                output.with_state(|state| state.serial.map(|serial| serial.get()))
-            });
+            let serial = Some(0);
+
+            let serial_str = output
+                .as_ref()
+                .map(|output| output.with_state(|state| state.serial.clone()));
 
             let keyboard_focus_stack_window_ids = output
                 .as_ref()
@@ -1527,6 +1529,7 @@ impl output_service_server::OutputService for OutputService {
                 keyboard_focus_stack_window_ids,
                 enabled,
                 powered,
+                serial_str,
             }
         })
         .await

src/backend/udev.rs

@@ -91,8 +91,6 @@ use crate::{
     state::{FrameCallbackSequence, Pinnacle, State, WithState},
 };
 
-use self::drm::util::EdidInfo;
-
 use super::{BackendData, UninitBackend};
 
 const SUPPORTED_FORMATS: &[Fourcc] = &[
@@ -915,14 +913,20 @@ impl Udev {
             connector.interface_id()
         );
 
-        let (make, model, serial) = EdidInfo::try_from_connector(&device.drm, connector.handle())
-            .map(|info| (info.manufacturer, info.model, info.serial))
-            .unwrap_or_else(|err| {
-                warn!("Failed to parse EDID info: {err}");
-                ("Unknown".into(), "Unknown".into(), None)
-            });
+        let display_info =
+            smithay_drm_extras::display_info::for_connector(&device.drm, connector.handle());
 
-        let (phys_w, phys_h) = connector.size().unwrap_or((0, 0));
+        let (make, model, serial) = display_info
+            .map(|info| {
+                (
+                    info.make().unwrap_or("Unknown".into()),
+                    info.model().unwrap_or("Unknown".into()),
+                    info.serial().unwrap_or("Unknown".into()),
+                )
+            })
+            .unwrap_or_else(|| ("Unknown".into(), "Unknown".into(), "Unknown".into()));
+
+        let (phys_w, phys_h) = connector.size().unwrap_or_default();
 
         if pinnacle.outputs.keys().any(|op| {
             op.user_data()

src/backend/udev/drm.rs

@@ -3,7 +3,6 @@ use smithay::{backend::drm::DrmDevice, reexports::drm::control::crtc};
 use tracing::warn;
 use util::get_drm_property;
 
-pub mod edid_manus;
 pub mod util;
 
 const DRM_CRTC_ACTIVE: &str = "ACTIVE";

src/backend/udev/drm/util.rs

@@ -1,129 +1,19 @@
-use std::{ffi::CString, io::Write, mem::MaybeUninit, num::NonZeroU32, time::Duration};
+use std::{ffi::CString, io::Write, time::Duration};
 
 use anyhow::Context;
 use drm_sys::{
     drm_mode_modeinfo, DRM_MODE_FLAG_NHSYNC, DRM_MODE_FLAG_NVSYNC, DRM_MODE_FLAG_PHSYNC,
     DRM_MODE_FLAG_PVSYNC, DRM_MODE_TYPE_USERDEF,
 };
-use libdisplay_info_sys::cvt::{
-    di_cvt_compute, di_cvt_options, di_cvt_reduced_blanking_version_DI_CVT_REDUCED_BLANKING_NONE,
-    di_cvt_timing,
-};
+use libdisplay_info::cvt::{self, ReducedBlankingVersion};
 use pinnacle_api_defs::pinnacle::output::v0alpha1::SetModelineRequest;
 use smithay::reexports::drm::{
     self,
-    control::{connector, property, Device, ModeFlags, ResourceHandle},
+    control::{property, Device, ModeFlags, ResourceHandle},
 };
 
-use super::edid_manus::get_manufacturer;
-
 // A bunch of this stuff is from cosmic-comp
 
-#[derive(Debug, Clone)]
-pub struct EdidInfo {
-    pub model: String,
-    pub manufacturer: String,
-    pub serial: Option<NonZeroU32>,
-}
-
-impl EdidInfo {
-    pub fn try_from_connector(
-        device: &impl Device,
-        connector: connector::Handle,
-    ) -> anyhow::Result<Self> {
-        let edid_prop = get_drm_property(device, connector, "EDID")?;
-        let edid_info = device.get_property(edid_prop)?;
-
-        let mut info = Err(anyhow::anyhow!("No info"));
-
-        let props = device.get_properties(connector)?;
-        let (ids, vals) = props.as_props_and_values();
-        for (&id, &val) in ids.iter().zip(vals.iter()) {
-            if id == edid_prop {
-                if let property::Value::Blob(edid_blob) = edid_info.value_type().convert_value(val)
-                {
-                    let blob = device.get_property_blob(edid_blob)?;
-                    info = parse_edid(&blob);
-                }
-                break;
-            }
-        }
-
-        info
-    }
-}
-
-/// Minimally parse the model and manufacturer from the given EDID data buffer.
-///
-/// `edid-rs` does not properly parse manufacturer ids (it has the order of the id bytes reversed
-/// and doesn't add 64 to map the byte to a character), and it additionally
-/// fails to parse detailed timing descriptors with an hactive that's divisible by 256
-/// (see https://github.com/tuomas56/edid-rs/pull/1).
-///
-/// Because of this, we're just rolling our own minimal parser instead.
-fn parse_edid(buffer: &[u8]) -> anyhow::Result<EdidInfo> {
-    // Manufacterer id is bytes 8-9, big endian
-    let manu_id = u16::from_be_bytes(buffer[8..=9].try_into()?);
-
-    // Characters are bits 14-10, 9-5, and 4-0.
-    // They also map 0b00001..=0b11010 to A..=Z, so add 64 to get the character.
-    let char1 = ((manu_id & 0b0111110000000000) >> 10) as u8 + 64;
-    let char2 = ((manu_id & 0b0000001111100000) >> 5) as u8 + 64;
-    let char3 = (manu_id & 0b0000000000011111) as u8 + 64;
-
-    let manufacturer = get_manufacturer([char1 as char, char2 as char, char3 as char]);
-
-    // INFO: This probably *isn't* completely unique between all monitors
-    let serial = u32::from_le_bytes(buffer[12..=15].try_into()?);
-
-    // Monitor names are inside of these display/monitor descriptors at bytes 72..=125.
-    // Each descriptor is 18 bytes long.
-    let descriptor1 = &buffer[72..=89];
-    let descriptor2 = &buffer[90..=107];
-    let descriptor3 = &buffer[108..=125];
-
-    let descriptors = [descriptor1, descriptor2, descriptor3];
-
-    let model = descriptors
-        .into_iter()
-        .find_map(|desc| {
-            // The descriptor is a monitor descriptor if its first 2 bytes are 0.
-            let is_monitor_descriptor = desc[0..=1] == [0, 0];
-            // The descriptor describes a monitor name if it has the tag 0xfc at byte 3.
-            let is_monitor_name = desc[3] == 0xfc;
-
-            if is_monitor_descriptor && is_monitor_name {
-                // Name is up to 13 bytes at bytes 5..=17 within the descriptor.
-                let monitor_name = desc[5..=17]
-                    .iter()
-                    // Names are terminated with a newline if shorter than 13 bytes.
-                    .take_while(|&&byte| byte != b'\n')
-                    .map(|&byte| byte as char)
-                    .collect::<String>();
-
-                // NOTE: The EDID spec mandates that bytes after the newline are padded with
-                // |     spaces (0x20), but we're just gonna ignore that haha
-
-                Some(monitor_name)
-            } else {
-                None
-            }
-        })
-        .or_else(|| {
-            // Get the product code instead.
-            // It's at bytes 10..=11, little-endian.
-            let product_code = u16::from_le_bytes(buffer[10..=11].try_into().ok()?);
-            Some(format!("{product_code:x}"))
-        })
-        .unwrap_or("Unknown".to_string());
-
-    Ok(EdidInfo {
-        model,
-        manufacturer,
-        serial: NonZeroU32::new(serial),
-    })
-}
-
 pub(super) fn get_drm_property(
     device: &impl Device,
     handle: impl ResourceHandle,
@@ -221,8 +111,8 @@ pub fn create_drm_mode(width: i32, height: i32, refresh_mhz: Option<u32>) -> drm
 
 // From https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/95ac3e99242b4e7f59f00dd073ede405ff8e9e26/backend/drm/util.c#L247
 fn generate_cvt_mode(hdisplay: i32, vdisplay: i32, vrefresh: Option<f64>) -> drm_mode_modeinfo {
-    let options: di_cvt_options = di_cvt_options {
-        red_blank_ver: di_cvt_reduced_blanking_version_DI_CVT_REDUCED_BLANKING_NONE,
+    let options = cvt::Options {
+        red_blank_ver: ReducedBlankingVersion::None,
         h_pixels: hdisplay,
         v_lines: vdisplay,
         ip_freq_rqd: vrefresh.unwrap_or(60.0),
@@ -234,12 +124,7 @@ fn generate_cvt_mode(hdisplay: i32, vdisplay: i32, vrefresh: Option<f64>) -> drm
         margins_rqd: false,
     };
 
-    let mut timing = MaybeUninit::<di_cvt_timing>::zeroed();
-    // SAFETY: is an ffi function
-    unsafe { di_cvt_compute(timing.as_mut_ptr(), &options as *const _) };
-
-    // SAFETY: Initialized in the function above
-    let timing = unsafe { timing.assume_init() };
+    let timing = cvt::Timing::compute(options);
 
     let hsync_start = (hdisplay + timing.h_front_porch as i32) as u16;
     let vsync_start = (timing.v_lines_rnd + timing.v_front_porch) as u16;

src/output.rs

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 
-use std::{cell::RefCell, num::NonZeroU32};
+use std::cell::RefCell;
 
 use indexmap::IndexSet;
 use pinnacle_api_defs::pinnacle::signal::v0alpha1::{
@@ -64,7 +64,8 @@ pub struct OutputState {
 
     pub focus_stack: WindowKeyboardFocusStack,
     pub screencopy: Option<Screencopy>,
-    pub serial: Option<NonZeroU32>,
+    // This monitor's edid serial. "Unknown" if it doesn't have one.
+    pub serial: String,
     pub modes: Vec<Mode>,
     pub lock_surface: Option<LockSurface>,
     pub blanking_state: BlankingState,

src/protocol/output_management.rs

@@ -397,10 +397,7 @@ where
     if head.version() >= zwlr_output_head_v1::EVT_MAKE_SINCE {
         head.make(physical_props.make);
         head.model(physical_props.model);
-
-        if let Some(serial_number) = output.with_state(|state| state.serial) {
-            head.serial_number(serial_number.to_string());
-        }
+        head.serial_number(output.with_state(|state| state.serial.clone()));
     }
 
     if head.version() >= zwlr_output_head_v1::EVT_ADAPTIVE_SYNC_SINCE {

tests/common/mod.rs

@@ -1,4 +1,4 @@
-use std::{panic::UnwindSafe, path::PathBuf, time::Duration};
+use std::{panic::UnwindSafe, path::PathBuf, sync::Mutex, time::Duration};
 
 use anyhow::anyhow;
 use pinnacle::{state::State, tag::TagId};
@@ -27,6 +27,8 @@ pub fn sleep_millis(millis: u64) {
     std::thread::sleep(Duration::from_millis(millis));
 }
 
+static MUTEX: Mutex<()> = Mutex::new(());
+
 pub fn test_api<F>(test: F) -> anyhow::Result<()>
 where
     F: FnOnce(Sender<Box<dyn FnOnce(&mut State) + Send>>) -> anyhow::Result<()>
@@ -34,6 +36,14 @@ where
         + UnwindSafe
         + 'static,
 {
+    let _guard = match MUTEX.lock() {
+        Ok(guard) => guard,
+        Err(err) => {
+            MUTEX.clear_poison();
+            err.into_inner()
+        }
+    };
+
     let mut event_loop = EventLoop::<State>::try_new()?;
     let mut state = State::new(
         pinnacle::cli::Backend::Dummy,