Re: [RFC PATCH 4/7] drm/vino: add the Vino (RawRl mode-2) framebuffer codec

[sashiko-bot@kernel.org]

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 <mike@fireburn.co.uk>

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/20260617151249.2937-1-mike@fireburn.co.uk?part=4