Re: [PATCH v3 6/13] drm/vino: add the video codec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The Navarro parameter map generation exceeds the maximum record stride cap (4080 bytes) for high-resolution displays like 4K.
- [High] The production entropy VLC encoder `Bits::finish` pads the final byte with zeroes instead of the wire-format-required one-bits.
--

commit 74ff1e1c314d743efc9972c4a877920b6216a374
Author: Mike Lothian <mike@fireburn.co.uk>
Date:   Wed Aug 26 16:37:31 2026 +0000

    drm/vino: add the video codec

This commit adds a proprietary multilevel Haar codec to encode video frames 
for DisplayLink docks. It includes the encoder, transform, strip grammar, 
record framing, and a software color pipeline.

> diff --git a/drivers/gpu/drm/vino/video/haar/records.rs b/drivers/gpu/drm/vino/video/haar/records.rs
> new file mode 100644
> index 0000000000000..422ae7d83ae27
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/video/haar/records.rs

[ ... ]

> +    let mut band = 0usize;
> +    let mut record = 0usize;
> +    while band < bands {
> +        // The first record takes up to PARAM_TLVS_PER_RECORD sub-records, the second the rest.
> +        let take_tlvs = if record == 0 {
> +            PARAM_TLVS_PER_RECORD
> +        } else {
> +            bands.div_ceil(PARAM_BANDS_PER_TLV)
> +        };

[Severity: High]
Will this logic exceed the 4080-byte stride cap on high-resolution displays?

When configuring a supported high-resolution mode like 4K (3840x2160), the 
display is split into 270 bands (bands = 270). The first parameter map record 
consumes 15 TLVs (120 bands). For the second record, navarro_strip_params() 
evaluates take_tlvs as 270.div_ceil(8) = 34. 

The loop then writes the remaining 19 TLVs into the second record. Since 19 
TLVs * 262 bytes equals 4978 bytes, this exceeds the STRIDE_CAP of 4080 
bytes, which can cause the dock's USB endpoint to halt and the display to 
fail.

[ ... ]

> diff --git a/drivers/gpu/drm/vino/video/haar/transform.rs b/drivers/gpu/drm/vino/video/haar/transform.rs
> new file mode 100644
> index 0000000000000..b009061ce13bc
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/video/haar/transform.rs

[ ... ]

> +    /// Flush the accumulator and yield the packed bytes, zero-padding the final byte.
> +    pub(crate) fn finish(mut self) -> Result<KVec<u8>> {
> +        let nbytes = self.nacc.div_ceil(8) as usize;
> +        for k in 0..nbytes {
> +            self.out.push((self.acc >> (8 * k)) as u8, GFP_KERNEL)?;
> +        }
> +        Ok(self.out)
> +    }

[Severity: High]
Does padding the final byte with zeros instead of ones violate the expected 
wire format and risk image corruption?

In Bits::finish(), the unused high bits of self.acc contain zeros, meaning 
the partial byte at the end of an unaligned strip is padded with zeros. 
Because zero is a valid symbol in the entropy code, the dock may decode 
unintended extra coefficients and desynchronize. 

If the byte is padded with 1-bits instead, it forms a truncated all-ones 
escape sequence that the decoder safely ignores.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826163913.7052-1-mike@fireburn.co.uk?part=6