Re: [PATCH v3 8/13] drm/vino: add the KMS device and the atomic path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Operations that sleep (Mutex locks, GFP_KERNEL allocations, and vmap) and long synchronous memory copies are performed within DRM atomic commit callbacks.
- [High] Page flips are permanently lost if an older frame is still pending, causing the screen to freeze on stale frames.
- [High] atomic_check reads shared global state without global atomic tracking, allowing concurrent commits to overcommit the dock's pixel bandwidth.
--

commit b222a22b8b0ea906ee63e26872096f97f4fb7ece
Author: Mike Lothian <mike@fireburn.co.uk>
drm/vino: add the KMS device and the atomic path

This commit adds the KMS side of the sink, including the DRM driver, GEM
and file types, CRTCs, planes, and connectors. It introduces atomic
callbacks to record desired state and workers to handle scanout, formatting,
and KMS limits checking.

> diff --git a/drivers/gpu/drm/vino/drm_sink.rs b/drivers/gpu/drm/vino/drm_sink.rs
> index 0000000000000..86708db89699a
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/drm_sink.rs
[ ... ]
> +    fn get(
> +        &mut self,
> +        fb: &kms::framebuffer::Framebuffer<VinoDrmDriver>,
> +    ) -> Result<Arc<SourceBinding>> {
[ ... ]
> +        let binding = Arc::new(
> +            SourceBinding {
> +                framebuffer: ARef::from(fb),
> +                mapping: fb.owned_vmap::<VinoObject>()?,
> +            },
> +            GFP_KERNEL,
> +        )?;

[Severity: High]
Does this code sleep in an atomic context?

DRM CRTC and plane atomic callbacks execute in an atomic context during
non-blocking commits, where sleeping is strictly forbidden. This performs
a GFP_KERNEL allocation and maps memory with fb.owned_vmap(), both of
which can sleep.

> diff --git a/drivers/gpu/drm/vino/drm_sink/dispatch.rs b/drivers/gpu/drm/vino/drm_sink/dispatch.rs
> index 0000000000000..bd759ad4bc992
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/drm_sink/dispatch.rs
[ ... ]
> +    pub(super) fn queue_scanout(
> +        &self,
> +        dev: &VinoDrmDevice,
> +        fb: &kms::framebuffer::Framebuffer<VinoDrmDriver>,
> +        mut frame: PendingScanout,
> +    ) {
[ ... ]
> +        let coalesce = {
> +            let pending = self.pending_scanout.lock();
> +            pending[connector].as_ref().is_some_and(|queued| {
> +                queued.w == frame.w
> +                    && queued.h == frame.h
> +                    && queued.rotation == frame.rotation
> +                    && self.keyframe_pending.load(Ordering::Acquire) & (1u32 << connector) == 0
> +            })
> +        };
> +        if coalesce {
> +            vino_debug!("vino: socket {socket} flip coalesced before snapshot\n");
> +            return;
> +        }

[Severity: High]
Can this early return cause the display to permanently freeze on stale frames?

If pending[connector] contains an unclaimed frame (making coalesce true),
this discards the new frame before taking a snapshot. The scanout worker
will only encode and scan out the older pending frame. If the compositor
goes idle and stops submitting frames, the worker will repeatedly process
the old frame and arm the settle_repaint timer with it, losing the most
recent updates.

[ ... ]
> +        let (mut surface, binding, idx) = {
> +            let mut pool = self.shadow[connector].lock();

[Severity: High]
Could this Mutex lock cause a deadlock or panic when called from the
atomic commit path?

This acquires a sleeping Mutex (shadow.lock()), which is unsafe in
atomic context.

[ ... ]
> +        let r = snapshot_to_shadow(
> +            self.geometry(),
> +            &mut surface,
> +            &binding.mapping,
> +            source_w,
> +            source_h,
> +        );

[Severity: High]
Is it safe to perform a synchronous memory copy of up to 14.7 MB here?

Executing a long synchronous memory copy in the non-blocking atomic
commit callback can lead to system instability, as atomic contexts
should be kept as short as possible.

> diff --git a/drivers/gpu/drm/vino/drm_sink/limits.rs b/drivers/gpu/drm/vino/drm_sink/limits.rs
> index 0000000000000..96d2d8dcf8c29
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/drm_sink/limits.rs
[ ... ]
> +    pub(super) fn other_connectors_rate(
> +        &self,
> +        state: &kernel::drm::kms::atomic::AtomicStateMutator<VinoDrmDriver>,
> +        connector: usize,
> +    ) -> u32 {
[ ... ]
> +        let timings = *self.last_timing.lock();
> +        let mut total: u32 = 0;
> +        for (i, t) in timings.iter().enumerate() {
> +            if i == connector {
> +                continue;
> +            }

[Severity: High]
Does reading shared global state here without global DRM atomic tracking
allow concurrent commits to overcommit the dock's pixel bandwidth?

Because this state is protected only by a local spinlock rather than being
part of the global DRM atomic state machine (e.g., via a drm_private_obj),
concurrent commits to different connectors will not serialize against each
other. Each commit will see the other connector's old bandwidth and succeed,
potentially bypassing hardware limits and causing a blank display.

> diff --git a/drivers/gpu/drm/vino/drm_sink/mode_objects.rs b/drivers/gpu/drm/vino/drm_sink/mode_objects.rs
> index 0000000000000..d3ec818ec1daa
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/drm_sink/mode_objects.rs
[ ... ]
> +    fn atomic_check(check: CrtcAtomicCheck<'_, Self>) -> Result {
[ ... ]
> +        let others = data.other_connectors_rate(&state, connector);
> +        let combined = new_rate.saturating_add(others);

[Severity: High]
Could this call read a stale bandwidth value from other connectors?

As noted in limits.rs, this reads shared global state without global
atomic tracking, which might allow concurrent commits to overcommit
the display bandwidth.

[ ... ]
> +    fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) {
[ ... ]
> +        data.queue_scanout(
> +            dev,
> +            fb,
> +            PendingScanout {
> +                connector,
> +                rotation,
> +                clips,
> +                nclips,
> +                w,
> +                h,
> +                shadow_idx: 0,
> +                shadow_generation: 0,
> +            },
> +        );
> +    }

[Severity: High]
Does calling queue_scanout() from this atomic callback invoke operations
that are unsafe in atomic context?

VinoPlane::atomic_update() executes in an atomic context during
non-blocking commits. However, queue_scanout() acquires a sleeping
Mutex, allocates with GFP_KERNEL, maps memory, and performs large
synchronous memory copies.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826163913.7052-1-mike@fireburn.co.uk?part=8