Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The `crtc_atomic_disable` callback executes in an atomic context but triggers sleeping operations, including `GFP_KERNEL` allocations. - [High] The `connector_atomic_set_property` directly mutates global hardware state during the atomic check/assembly phase, violating DRM atomic semantics. - [Medium] Missing contrast property replay during DPMS-on monitor wake. --
commit f57a262e5ecdc9d7a9ab4c348f2232b364488d0e Author: Mike Lothian <[email protected]> Add monitor controls that ride the same control plane This patch adds DDC/CI monitor controls over a custom control plane for brightness, contrast, and DPMS. It adds connector properties for brightness and contrast, and hooks into the CRTC disable path for power off. > diff --git a/drivers/gpu/drm/vino/drm_sink.rs > b/drivers/gpu/drm/vino/drm_sink.rs > index afbf883fba36..bcc871958a8a 100644 > --- a/drivers/gpu/drm/vino/drm_sink.rs > +++ b/drivers/gpu/drm/vino/drm_sink.rs [ ... ] > @@ -637,6 +660,11 @@ fn install_edid(connector: *mut bindings::drm_connector, > blob: &[u8]) -> i32 { > if let Err(e) = data.send_cp(head.index, 0x48, 16, |ctr| > super::cp::set_mode(ctr, &timing)) { > pr_warn!("vino: head{} runtime mode-set send failed ({e:?})\n", > head.index); > } > + // Bring the monitor out of DPMS standby (DDC/CI VCP 0xD6 = on). > Inferred wire (see > + // `cp::ddc_set_vcp`); a no-op until CP engages, and re-applies the > user's brightness. > + let _ = data.set_vcp(head.index, super::cp::VCP_POWER_MODE, > super::cp::POWER_ON); > + let b = head.brightness.load(core::sync::atomic::Ordering::Relaxed); > + let _ = data.set_vcp(head.index, super::cp::VCP_BRIGHTNESS, b as u16); [Severity: Medium] Does this code omit re-applying the contrast property during the DPMS-on monitor wake sequence? The struct documentation for head.contrast states it is replayed on DPMS-on, but only head.brightness is loaded and re-applied to the monitor here. If contrast is not restored, the monitor could lose its custom contrast setting when waking from standby and revert to hardware defaults. > } > > /// CRTC `.atomic_disable`: the display is turning off. [ ... ] > @@ -671,6 +701,10 @@ fn install_edid(connector: *mut bindings::drm_connector, > blob: &[u8]) -> i32 { > } > head.cursor_primed > .store(false, core::sync::atomic::Ordering::SeqCst); > + // DPMS-off: blank the monitor backlight via DDC/CI (VCP 0xD6 = off) > rather than leaving > + // the last frame frozen on the panel. Inferred wire (see > `cp::ddc_set_vcp`); no-op until > + // CP engages. > + let _ = data.set_vcp(head.index, super::cp::VCP_POWER_MODE, > super::cp::POWER_OFF); [Severity: High] Does crtc_atomic_disable() execute in an atomic context where sleeping is forbidden? This calls data.set_vcp(), which delegates to ddc_set_vcp(). As seen in cp.rs, ddc_set_vcp() uses sleeping allocations: cp.rs:ddc_set_vcp() { ... let mut b = KVec::with_capacity(32, GFP_KERNEL)?; ... b.extend_from_slice(&[0x37, payload.len() as u8], GFP_KERNEL)?; ... } Allocating memory with GFP_KERNEL in the atomic CRTC disable callback could lead to a scheduling-while-atomic panic during display modesets. > pr_info!("vino: KMS CRTC disable -- head{} display OFF (scanout > stopped)\n", head.index); > } [ ... ] > +unsafe extern "C" fn connector_atomic_set_property( > + connector: *mut bindings::drm_connector, > + _state: *mut bindings::drm_connector_state, > + property: *mut bindings::drm_property, > + val: u64, > +) -> i32 { [ ... ] > + if slot.swap(v, core::sync::atomic::Ordering::Relaxed) != v { > + let _ = data.set_vcp(head.index, vcp, v as u16); > + } > + 0 > +} [Severity: High] Does this directly mutate global hardware state during the atomic check or assembly phase? In the DRM atomic architecture, the atomic_set_property callback can be invoked during state assembly and checking, such as for test-only commits (DRM_MODE_ATOMIC_TEST_ONLY). Because data.set_vcp() immediately pushes the DDC/CI write to the monitor during this phase, a test-only commit will irreversibly modify the live hardware state, breaking the expectations of userspace compositors relying on atomic test operations having no side effects. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
