Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The EP08 transport header truncates the payload length to 16 bits, corrupting frames larger than 64KB. - [High] Incorrect indexing of the `SECTION_CODE` array causes `Mode::Rle` and `Mode::Adaptive` to emit the wrong frame-end section codes. - [High] The encoded stream uses `KVec<u8>`, which relies on contiguous kmalloc and will fail for large frame updates. - [High] The encoder partially updates its shadow buffer before an error occurs, causing permanent corruption of the differential stream. --
commit f949babe97f31ea182f0de355c24ec2f2844883f Author: Mike Lothian <[email protected]> drm/vino: add the Vino (RawRl mode-2) framebuffer codec Add the video module: the RawRl ("Raw/RLX" mode-2) encoder, clean-room from the AArch64 reference-driver decompile, which emits packed-RGB565 frames the dock decodes without the impractical Vino Walsh-Hadamard entropy codec. > diff --git a/drivers/gpu/drm/vino/video.rs b/drivers/gpu/drm/vino/video.rs > new file mode 100644 > index 0000000000000..bb5ea893575fc > --- /dev/null > +++ b/drivers/gpu/drm/vino/video.rs [ ... ] > + pub(super) fn encode(&mut self, cur: &[u16]) -> Result<KVec<u8>> { > + let mut s = KVec::new(); > + self.encode_into(cur, &mut s)?; > + Ok(s) > + } > + > + /// Like [`encode`](Self::encode) but appends the marker stream to a > caller-owned > + /// `out` instead of allocating a fresh `KVec`. The hot scanout path > + /// ([`encode_and_send`](super::drm_sink::encode_and_send)) uses this to > encode > + /// straight into a buffer that already reserves the EP08 transport > header, so a > + /// frame costs one allocation with no separate framing copy. > + pub(super) fn encode_into(&mut self, cur: &[u16], s: &mut KVec<u8>) -> > Result { [Severity: High] Does KVec allocation risk causing kmalloc failures for large frame updates? The encode function allocates a KVec to hold the encoded frame. Since the encoded output for a full Raw frame or an incompressible Adaptive frame can approach or exceed 4 MiB, this vector might grow beyond the page allocator's maximum contiguous kmalloc order. Could this lead to page allocator warnings and failures during heavy frame updates? > + s.extend_from_slice(&FRAME_INIT, GFP_KERNEL)?; > + for y in 0..self.height { > + let row = y * self.width; > + let mut x = 0; > + while x < self.width { > + while x < self.width && cur[row + x] == self.shadow[row + x] > { > + x += 1; > + } > + if x >= self.width { > + break; > + } > + let run_start = x; > + while x < self.width && cur[row + x] != self.shadow[row + x] > { > + x += 1; > + } > + let run_end = x; > + let mut p = run_start; > + while p < run_end { > + let n = (run_end - p).min(MAX_BLOCK_PIXELS); > + let coord = (((row + p) * 2) & 0xff_ffff) as u32; > + encode_run_into(s, self.mode, coord, &cur[row + p..row + > p + n])?; > + p += n; > + } > + for k in run_start..run_end { > + self.shadow[row + k] = cur[row + k]; > + } [Severity: High] Does this error handling permanently corrupt the differential stream if an allocation fails? The shadow buffer is mutated iteratively inside the processing loop after encode_run_into(), which can return an error via ?. If it fails, the frame processing is aborted and not sent. However, the shadow buffer has already been updated with the pixels from successfully processed runs. Because the dock never receives the aborted frame, its internal state no longer matches the host's self.shadow, which will cause visual artifacts on subsequent frames since the encoder will assume the dock already has those pixels. > + } > + } > + let code = SECTION_CODE[(self.mode as > usize).saturating_sub(1).min(6)]; [Severity: High] Does saturating_sub(1) map the 0-based enum values incorrectly for Mode::Rle and Mode::Adaptive? The Mode enum is 0-based (Raw = 0, Rle = 1, Adaptive = 2). Applying saturating_sub(1) evaluates to 0 for both Raw and Rle, making Mode::Rle emit SECTION_CODE[0] (0x01). Similarly, Mode::Adaptive evaluates to index 1 and emits SECTION_CODE[1] (0x00). This causes the encoder to emit the wrong framing codes for RLE and Adaptive modes, breaking hardware decoding. > + s.extend_from_slice(&SYNC, GFP_KERNEL)?; > + s.extend_from_slice(&[0xaf, 0x20, 0x1f, code], GFP_KERNEL)?; [ ... ] > +pub(super) fn write_ep08_header(hdr: &mut [u8], payload_len: usize, seq: > u32) { > + hdr[0] = 0; > + hdr[1] = 0; > + hdr[2..4].copy_from_slice(&((payload_len + 12) as u16).to_le_bytes()); [Severity: High] Can casting the payload length to u16 truncate it and break the transport header for large frames? A full 1080p frame encoded in Raw or Adaptive mode produces a payload of several megabytes (up to ~4 MiB). Casting this usize payload length to u16 will silently drop the upper bits. Could this be the reason for the USB resets noted in the module documentation, as the dock would receive a malformed header with an incorrect size and lose framing synchronization? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
