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 <[email protected]> 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/[email protected]?part=8
