Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The cursor plane's atomic update callback performs operations that can sleep, including framebuffer vmap and GFP_KERNEL allocations, violating atomic context rules. - [Medium] The tracking state `cursor_last` is unconditionally updated before the new cursor image is successfully mapped and transmitted, breaking subsequent updates if an error occurs. - [Medium] The driver does not send commands to the hardware dock to hide the cursor when it is disabled or moved entirely off-screen. - [Medium] Manual color channel extraction from a natively read 32-bit word produces reversed colors on big-endian architectures. --
commit c3d3f61b0980905330252aecb30ece0564fb99bc Author: Mike Lothian <[email protected]> drm/vino: wire the hardware cursor plane This commit re-wires the hardware cursor plane for the vino driver by creating a Type::Cursor plane. The cursor plane's atomic_update forwards the cursor bitmap and position to the dock over CP. > diff --git a/drivers/gpu/drm/vino/drm_sink.rs > b/drivers/gpu/drm/vino/drm_sink.rs > index 450c7c84a0677..fde52336fcd14 100644 > --- a/drivers/gpu/drm/vino/drm_sink.rs > +++ b/drivers/gpu/drm/vino/drm_sink.rs [ ... ] > @@ -429,6 +454,36 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) { > let plane = commit.plane(); > 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; > + match new.framebuffer::<VinoDrmDriver>() { > + Some(fb) => { > + let w = fb.width() as u16; > + let h = fb.height() as u16; > + // Re-upload the bitmap only when the framebuffer > changes; always update the > + // 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] If read_cursor_bgra() fails, doesn't this unconditional update of cursor_last mean we'll permanently skip uploading this framebuffer on subsequent updates, falsely assuming it was already uploaded? > + let _ = data.send_cp(0x1b, 0, |ctr| > super::cp::cursor_create(ctr, HEAD, w, h)); > + if let Ok(bgra) = read_cursor_bgra(fb, w as usize, h > as usize) { [Severity: High] Does read_cursor_bgra() sleep in atomic context? Plane atomic_update callbacks run in atomic context, but read_cursor_bgra() performs sleepable operations like vmap() and GFP_KERNEL allocations. Will this trigger lockdep warnings ("scheduling while atomic") or kernel panics? > + let _ = data > + .send_cp(0x401c, 0, |ctr| > super::cp::cursor_image(ctr, HEAD, w, h, &bgra)); > + } > + } > + let x = new.crtc_x().max(0) as u16; > + let y = new.crtc_y().max(0) as u16; [Severity: Medium] Are we leaving a ghost cursor visible when it should be hidden? By clamping negative coordinates to 0, a cursor moved off-screen to the left or top will stick to the screen edge instead of moving out of view. > + let _ = data.send_cp(0x1a, 0, |ctr| > super::cp::cursor_move(ctr, HEAD, x, y)); > + } > + // Cursor disabled: forget the last bitmap so it re-uploads > if it comes back. > + None => plane.cursor_last.store(0, Relaxed), [Severity: Medium] Similarly, when the cursor is logically disabled (framebuffer is None), this clears the software state but doesn't send any hardware commands to the dock to actually hide the cursor. Will the cursor remain visible? > + } > + return; > + } > + > let Some(fb) = new.framebuffer::<VinoDrmDriver>() else { return }; [ ... ] > @@ -480,6 +535,31 @@ fn prime_video_eps(dev: > &super::usb::Interface<kernel::device::Bound>) { > } > } > > +/// Map the cursor framebuffer and convert it to the `w*h*4` BGRA bitmap the > dock expects > +/// (`cp::cursor_image`). The source is XRGB/ARGB8888; each pixel is written > out as B, G, R, A. > +fn read_cursor_bgra( > + fb: &kms::framebuffer::Framebuffer<VinoDrmDriver>, > + w: usize, > + h: usize, > +) -> Result<KVec<u8>> { > + let vmap = fb.vmap()?; > + let src = vmap.as_ptr(); > + let pitch = fb.pitch(0) as usize; > + let mut out = KVec::with_capacity(w * h * 4, GFP_KERNEL)?; > + for dy in 0..h { > + for dx in 0..w { > + // SAFETY: `dy*pitch + dx*4 + 3` is within the mapped cursor > framebuffer (`pitch*h` > + // bytes); `dx < w <= pitch/4`, `dy < h`. > + let px = unsafe { (src.add(dy * pitch + dx * 4) as *const > u32).read_unaligned() }; > + out.push((px & 0xff) as u8, GFP_KERNEL)?; // B > + out.push(((px >> 8) & 0xff) as u8, GFP_KERNEL)?; // G > + out.push(((px >> 16) & 0xff) as u8, GFP_KERNEL)?; // R > + out.push(((px >> 24) & 0xff) as u8, GFP_KERNEL)?; // A [Severity: Medium] Will this color extraction logic produce distorted or reversed colors on big-endian architectures? The little-endian DRM_FORMAT_ARGB8888 buffer is read as a native u32. On a big-endian system, extracting the bytes with these shifts will grab the wrong bytes for each color channel. > + } > + } > + Ok(out) > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
