Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The frame-damage clips overflow fallback logic permanently drops previously accumulated damage clips instead of properly merging them. - [High] The CRTC gamma LUT state is incorrectly shared globally rather than being per-head, causing independent display heads to overwrite or clear each other's gamma ramps. Pre-existing issues: - [High] Kernel out-of-bounds read occurs when userspace requests plane scaling. - [High] The plane `atomic_update` and CRTC callbacks run in atomic context but acquire sleepable Mutexes and perform GFP_KERNEL allocations. - [Medium] The cursor plane update logic uses the raw virtual address of the framebuffer as a cache key, risking an ABA problem where a re-uploaded cursor is silently ignored. -- commit 02346681a0e083ee04b818868a94582e0d21e04e Author: Mike Lothian <[email protected]> drm/vino: two heads, 90/270 rotation, damage clips and connector probe Consume the new connector detect()/mode_valid() and plane damage-clip bindings, and finish the KMS feature set the driver previously stubbed: > diff --git a/drivers/gpu/drm/vino/drm_sink.rs > b/drivers/gpu/drm/vino/drm_sink.rs > index 898cfae8fd4fb..ee04a5af9f7d5 100644 > --- a/drivers/gpu/drm/vino/drm_sink.rs > +++ b/drivers/gpu/drm/vino/drm_sink.rs > @@ -435,10 +474,12 @@ fn atomic_enable(commit: CrtcAtomicCommit<'_, Self>) { > /// against a shadow the dock may have dropped, and blanks the monitor > via DDC/CI -- a > /// no-op until CP engages. > fn atomic_disable(commit: CrtcAtomicCommit<'_, Self>) { > - let data: &VinoDrmData = commit.crtc().drm_dev(); > + let crtc = commit.crtc(); > + let head = crtc.head; > + let data: &VinoDrmData = crtc.drm_dev(); > data.update_gamma(None); [Severity: High] Since the driver now supports multiple independent display heads, is it safe to share a single global gamma state? `data.update_gamma` modifies the shared `gamma` in `VinoDrmData`. This could inadvertently overwrite or clear the gamma ramps of other connected displays when one head is updated or disabled. Should `gamma` be made per-head, similar to the `connectors` array? Also, this is a pre-existing issue, but doesn't `data.update_gamma` lock a sleepable Mutex? CRTC callbacks like `atomic_disable` and `atomic_enable` can run in atomic context during non-blocking atomic commits where sleeping is forbidden. [ ... ] > @@ -498,14 +554,14 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) { > return; > } > let plane = commit.plane(); > + let head = plane.head; > let data: &VinoDrmData = plane.drm_dev(); > - let new = commit.take_new_state(); > > // Cursor plane: forward the cursor bitmap/position to the dock over > CP (id=0x1b create, > // 0x401c image, 0x1a move -- see `cp::cursor_*`). A no-op until CP > engages, like scanout. > if plane.is_cursor { > use core::sync::atomic::Ordering::Relaxed; > - const HEAD: u8 = 0; > + let new = commit.take_new_state(); > match new.framebuffer::<VinoDrmDriver>() { > Some(fb) => { > let w = fb.width() as u16; > @@ -514,15 +570,17 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) { > // position (a bare cursor move must not re-send the > whole image). > let key = fb as *const _ as usize; > if plane.cursor_last.swap(key, Relaxed) != key { [Severity: Medium] This is a pre-existing issue, but does using the raw virtual address of the framebuffer as a cache key risk an ABA problem? If userspace frees a cursor framebuffer and immediately allocates a new one, the memory allocator may place it at the same virtual address. This could cause the new cursor image to be silently ignored because the cache key hasn't changed. [ ... ] > @@ -537,6 +597,37 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) { > let (w, h) = (new.crtc_w() as usize, new.crtc_h() as usize); [Severity: High] This is a pre-existing issue, but can this lead to an out-of-bounds read if userspace requests plane scaling? The destination dimensions `crtc_w` and `crtc_h` are used to iterate and calculate source memory offsets. If a compositor provides a small framebuffer and requests scaling, the offsets could far exceed the bounds of the source framebuffer. > // Plane rotation/reflection (identity unless the compositor set the > rotation property). > let rotation = new.rotation(); > + // Collect the client's individual frame-damage clips (the > rectangles that > + // `damage_merged()` would collapse into one bounding box), each > clamped to the output, so > + // only the genuinely changed rectangles are re-converted from the > source rather than their > + // whole enclosing box. Only for identity rotation (the clips are in > un-rotated source > + // space; mapping them through 90/270 is not worth it for the > throttled fallback path), and > + // never on the WHT keyframe path -- see `encode_and_send`. A fixed > stack array keeps the > + // atomic-commit path allocation-free; on overflow the clips > collapse into one bounding box. > + // An empty list means "convert the whole output" (used for the > rotated/reflected case). > + let mut clips = [(0usize, 0usize, 0usize, 0usize); MAX_DAMAGE_CLIPS]; > + let mut nclips = 0usize; > + if rotation & bindings::DRM_MODE_ROTATE_MASK == > bindings::DRM_MODE_ROTATE_0 > + && rotation & (bindings::DRM_MODE_REFLECT_X | > bindings::DRM_MODE_REFLECT_Y) == 0 > + { > + new.for_each_damage_clip(old, |r| { > + let c = ( > + (r.x1.max(0) as usize).min(w), > + (r.y1.max(0) as usize).min(h), > + (r.x2.max(0) as usize).min(w), > + (r.y2.max(0) as usize).min(h), > + ); > + if nclips < MAX_DAMAGE_CLIPS { > + clips[nclips] = c; > + nclips += 1; > + } else { > + // Overflow: collapse everything so far (and `c`) into > clips[0]'s bounding box. > + let b = clips[0]; > + clips[0] = (b.0.min(c.0), b.1.min(c.1), b.2.max(c.2), > b.3.max(c.3)); > + nclips = 1; [Severity: High] Does this overflow fallback logic correctly merge the clips? When `nclips` reaches `MAX_DAMAGE_CLIPS`, it updates `clips[0]` to be the bounding box of `clips[0]` and `c`. However, this completely discards the previously accumulated clips from `clips[1]` through `clips[15]`, dropping them permanently rather than expanding the bounding box to include them. > + } > + }); > + } > > use core::sync::atomic::Ordering::Relaxed; [ ... ] > @@ -693,6 +803,8 @@ fn encode_and_send( > return encode_and_send_wht(data, plane, vaddr, pitch, rotation, w, > h); > } > let gamma = data.gamma_snapshot(); > + // Source dimensions (swapped from the output for 90/270 rotation). > + let (sw, sh) = src_dims(rotation, w, h); > let frame = { > let mut st = plane.scanout.lock(); [Severity: High] This isn't a bug introduced by this patch, but doesn't `atomic_update` run in atomic context during non-blocking atomic commits? Locking a sleepable Mutex like `plane.scanout.lock()` here can cause kernel warnings and system instability. [ ... ] > @@ -746,7 +868,7 @@ fn encode_and_send( > // DRM device, so it is bound for the duration of this push. > let dev = unsafe { data.intf.as_bound() }; > prime_video_eps(dev); > - dev.bulk_send(VIDEO_EP, &frame, super::timeout(), GFP_KERNEL)?; > + dev.bulk_send(VIDEO_EPS[plane.head as usize], &frame, super::timeout(), > GFP_KERNEL)?; [Severity: High] Similarly, this is a pre-existing issue, but does `dev.bulk_send` perform allocations using the `GFP_KERNEL` flag? Using `GFP_KERNEL` in atomic context is unsafe and can sleep. > Ok(()) > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
