Re: [RFC PATCH v2 10/10] drm/vino: hrtimer-driven software vblank

[sashiko-bot@kernel.org]

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 <mike@fireburn.co.uk>

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/20260703030217.2886-1-mike@fireburn.co.uk?part=10