Re: [RFC PATCH v2 04/10] drm/vino: add the Vino framebuffer codec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Sleeping functions called from atomic context in the hot scanout path.
- [High] Large contiguous memory allocations via `KVec` cause page allocator WARNs and fail under load.
- [Medium] `Bits::esc` silently emits malformed unary prefix codes for magnitude categories >= 10, corrupting the bitstream.
- [Medium] Contradictory mathematical rounding for negative values between `quantize_dc` and `quantize_dc_round`.
- [Medium] Documentation for `colour_frame_ep08` contradicts its implementation and the hardware wire format.
--

commit e12de38777f2b0ee034bdb7b4eb4f91d2f8055e5
Author: Mike Lothian <mike@fireburn.co.uk>

drm/vino: add the Vino framebuffer codec

This commit adds the Vino framebuffer codec to the DRM subsystem, including
an RLE fallback encoder and a Walsh-Hadamard transform codec. It also
implements the EP08 wire framing for the display dock.

> diff --git a/drivers/gpu/drm/vino/video.rs b/drivers/gpu/drm/vino/video.rs
> new file mode 100644
> index 0000000000000..c520af19a4fbb
> --- /dev/null
> +++ b/drivers/gpu/drm/vino/video.rs
> @@ -0,0 +1,1229 @@
[ ... ]
> +    pub(crate) fn encode_into(&mut self, cur: &[u16], s: &mut KVec<u8>) -> Result {
> +        s.extend_from_slice(&FRAME_INIT, GFP_KERNEL)?;

[Severity: High]
Is it safe to use GFP_KERNEL allocations here?

Plane atomic update callbacks are atomic context where sleeping functions are
strictly forbidden. However, this encoder path is called during scanout
(VinoPlane::atomic_update() -> scanout_one() -> encode_and_send() ->
encode_into()), leading to repeated allocations with GFP_KERNEL.

Additionally, the outer encode_and_send() and encode_and_send_wht()
functions in drm_sink.rs allocate the outgoing frame buffer using
GFP_KERNEL and perform a blocking USB bulk transfer with GFP_KERNEL.

Does this regression trigger kernel warnings, system instability or deadlocks
when called from atomic context?

> +        for y in 0..self.height {
> +            let row = y * self.width;
> +            let mut x = 0;
> +            while x < self.width {
[ ... ]
> +                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])?;

[Severity: High]
Could this KVec allocation grow too large and cause page allocator failures?

While the shadow buffer correctly uses a vmalloc-backed VVec to avoid kmalloc
limits, the encoded output buffer appended to via encode_run_into() can also
scale linearly with resolution and frame differences. If an uncompressible
frame is encountered (e.g. random noise at 4K resolution), the generated
stream could exceed the kmalloc contiguous memory limit before reaching the
outer 65 KB EOVERFLOW check.

Will this regression cause the page allocator to trigger WARNs and return
-ENOMEM under load, allowing userspace compositors to flood the kernel log
and stall display updates?

> +                    p += n;
> +                }
> +            }
> +        }
[ ... ]
> +    fn quantize_dc(plane: usize, v: i32) -> i32 {
> +        let step = if plane == 0 { 16 } else { 64 };
> +        let q = (((v.unsigned_abs() + 8) * (65536 / step)) >> 16) as i32;

[Severity: Medium]
Does this quantization apply the correct bias for chroma steps?

This function implements magnitude rounding with a hardcoded bias of 8. For
chroma values where step = 64, a negative value like v = -232 will result
in -3 instead of -4.

A newer implementation, quantize_dc_round(), implements Euclidean rounding
using the correct step bias:

    fn quantize_dc_round(plane: usize, v: i32) -> i32 {
        let step = if plane == 0 { 16 } else { 64 };
        (v + step / 2).div_euclid(step)
    }

However, the older quantize_dc() is still used for encoding solid strip DC
planes:

    pub(crate) fn solid_strip(x: u16, y: u16, ydc: i32, cbdc: i32, crdc: i32) -> Result<KVec<u8>> {
        let esc = esc_len(quantize_dc(2, crdc), SOLID_DC_CMAX)

Could this contradictory mathematical rounding act as a regression that causes
decoding errors for negative chroma DCs?

> +        if v < 0 {
> +            -q
> +        } else {
> +            q
> +        }
> +    }
[ ... ]
> +        fn esc(&mut self, v: i32, cmax: u32) -> Result {
> +            if v == 0 {
> +                return self.bit(0);
> +            }
> +            let c = mag_category(v);
> +            let off = v.unsigned_abs() - (1 << (c - 1));
> +            for _ in 0..c {
> +                self.bit(1)?;
> +            }

[Severity: Medium]
Should the number of unary 1 bits emitted be clamped to cmax?

The code limits the unary prefix to cmax by omitting the 0-terminator if
c >= cmax, but the loop unconditionally emits c ones. If a high frequency edge
produces a magnitude category of 10 or higher (while cmax is 9 for AC
coefficients), this will emit 10 or more ones.

A hardware decoder expecting a maximum prefix length of 9 will stop after
reading 9 ones and interpret the 10th one as the magnitude offset, corrupting
the bitstream for the rest of the strip. Can this logic be adjusted to prevent
emitting malformed unary prefix codes?

> +            if c < cmax {
> +                self.bit(0)?;
> +            }
[ ... ]
> +    /// Encode a full width x height 8-bit-RGB frame into the Vino WHT colour EP08 frame(s) --
> +    /// the colour counterpart of the luma encode_frame, and the assembler the live scanout path
> +    /// drives once the CP wall falls. px(x, y) yields the source pixel's (R, G, B); the caller
> +    /// applies any rotation / gamma / format conversion (so this stays a pure codec). The surface is
> +    /// tiled into 64x16 strips in raster order, each built from colour_block + colour_strip.
> +    /// The forward length-hint tail is then chained across the WHOLE frame -- strip k's 2-byte
> +    /// tail is patched to strip k+1's L - 2 (the last strip keeps its own), even across EP08-frame
> +    /// boundaries. Finally the strip stream is split at strip boundaries into <= u16::MAX - 12-byte
> +    /// EP08 frames, each prefixed with a write_ep08_header carrying an incrementing seq from
> +    /// seq0. Returns the ready-to-send frames and the next seq.
> +    ///
> +    /// width/height must be multiples of 64 and 16 (EINVAL otherwise) -- the codec's strip
> +    /// geometry; the live scanout path falls back to RLE for non-aligned modes (see
> +    /// docs/VIDEO-TODO.md). Byte-exact for the recovered colour grammar (chroma sync/DC/AC); the
> +    /// anti-fabrication boundary is the synthetic steepest-chroma edge cases (VIDEO-TODO.md 8/9).
> +    pub(crate) fn colour_frame_ep08(

[Severity: Medium]
Is the documentation for colour_frame_ep08() out of date with its current
implementation?

The docstring claims it chains a forward length-hint tail across the frame and
splits the stream into EP08 frames prefixed with write_ep08_header().

However, the function delegates to frame_records(), which uses a different TLV
record format. The comments in frame_records() explicitly state that the
legacy write_ep08_header() behavior was wrong and caused hardware faults:

    // There is no per-transfer header (the old write_ep08_header sub=0x30
    // over concatenated strips was wrong and made the dock fault).

Could the API documentation be updated to correctly describe the new TLV
record format instead of the obsolete layout?

> +        width: usize,
> +        height: usize,
> +        seq0: u32,
> +        mut px: impl FnMut(usize, usize) -> (u8, u8, u8),
> +    ) -> Result<(KVec<KVec<u8>>, u32)> {

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