Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The software vblank hrtimer free-runs indefinitely at ~60Hz even when the display is disabled (DPMS off), causing continuous power drain. --
commit a169dcaae3d3a68e1d3207b3eca11363159c01a8 Author: Mike Lothian <[email protected]> drm/vino: hrtimer-driven software vblank The atomic helpers were completing every page-flip immediately via drm_atomic_helper_fake_vblank(), because the CRTC had no vblank support. This patch implements VblankSupport for VinoCrtc backed by a per-CRTC hrtimer that fires once per frame to pace page flips. > diff --git a/drivers/gpu/drm/vino/drm_sink.rs > b/drivers/gpu/drm/vino/drm_sink.rs > index ee04a5af9f7d5..ce04c8b65461c 100644 > --- a/drivers/gpu/drm/vino/drm_sink.rs > +++ b/drivers/gpu/drm/vino/drm_sink.rs [ ... ] > @@ -417,11 +428,69 @@ fn probe(dev: &NewKmsDevice<'_, Self, Probing>) -> > Result { [ ... ] > +impl HrTimerCallback for VblankTimer { > + type Pointer<'a> = Arc<Self>; > + > + fn run(this: ArcBorrow<'_, Self>, mut ctx: HrTimerCallbackContext<'_, > Self>) -> HrTimerRestart { > + let crtc = this.crtc.load(Ordering::Relaxed); > + if !crtc.is_null() && this.enabled.load(Ordering::Relaxed) { > + // SAFETY: `crtc` is the `drm_crtc` stored in `enable_vblank` > while the device is live; > + // the timer is cancelled (its handle dropped) before the crtc > is freed at teardown. > + unsafe { bindings::drm_crtc_handle_vblank(crtc) }; > + } > + let interval = > this.interval_ns.load(Ordering::Relaxed).max(1_000_000); > + ctx.forward_now(Delta::from_nanos(interval)); > + HrTimerRestart::Restart [Severity: Medium] Will this timer continue to free-run indefinitely at ~60Hz even when the display is disabled? When the display is turned off, disable_vblank() merely clears the enabled flag, but VblankTimer::run() unconditionally returns HrTimerRestart::Restart. Since the timer is only started once in enable_vblank() and is only cancelled when the CRTC object is dropped, it appears this creates continuous background wakeups on an idle system, which can prevent the CPU from entering deep sleep states. Could the timer self-terminate when the enabled flag is false, and be restarted by enable_vblank() to avoid draining power during DPMS off? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
