Re: [RFC PATCH v2 08/10] drm/vino: wire CRTC gamma, plane rotation and DDC/CI monitor controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] The patch introduces a `gamma` field protected by a `kernel::sync::Mutex` and locks it from within DRM atomic commit callbacks (`atomic_enable`, `atomic_disable`, `atomic_update`), which execute in atomic context where sleeping is forbidden.
- [High] The DDC/CI payload length is silently truncated if it exceeds 255 bytes, leading to malformed CP packets.
- [Medium] Dynamic `GAMMA_LUT` property updates are silently ignored while the display is active.
- [Medium] The rotation property is missing on the cursor plane and the rotation is completely ignored when reading cursor pixels.
- [Medium] The DDC/CI I2C adapter's `master_xfer` incorrectly skips unsupported messages and transmission failures instead of returning an error, breaking sequential processing guarantees.
- [Low] The import `i2c` does not follow the kernel vertical import style.

Pre-existing issues:
- [Critical] Missing plane scaling rejection allows userspace to trigger out-of-bounds kernel memory reads during scanout.
- [High] The `ddc_forward` function allocates memory using `GFP_KERNEL` when called from the `atomic_disable` DRM callback, violating atomic context rules.
--

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

drm/vino: wire CRTC gamma, plane rotation and DDC/CI monitor controls

This commit consumes the new gamma, rotation, and i2c bindings for the vino 
driver. It enables a 256-entry GAMMA_LUT, adds a rotation property on the 
primary/cursor plane, and registers a virtual I2C adapter for DDC/CI controls.

> diff --git a/drivers/gpu/drm/vino/cp.rs b/drivers/gpu/drm/vino/cp.rs
> --- a/drivers/gpu/drm/vino/cp.rs
> +++ b/drivers/gpu/drm/vino/cp.rs

[ ... ]

> @@ -151,14 +151,24 @@ pub(super) fn set_mode(counter: u16, t: &Timing) -> Result<KVec<u8>> {
>  /// the CP wall), so re-check against a capture once CP engages. The DDC/CI bytes themselves
>  /// ([`ddc_ci_set_vcp`]) are standard and verified.
>  pub(super) fn ddc_set_vcp(counter: u16, vcp: u8, value: u16) -> Result<KVec<u8>> {
> -    let payload = ddc_ci_set_vcp(vcp, value);
> -    let mut b = KVec::with_capacity(32, GFP_KERNEL)?;
> +    ddc_forward(counter, DDCCI_I2C_ADDR, &ddc_ci_set_vcp(vcp, value))
> +}
> +
> +/// The DDC/CI I2C slave address on the monitor bus.
> +pub(super) const DDCCI_I2C_ADDR: u8 = 0x37;
> +
> +/// CP message tunnelling a raw DDC/CI transaction (`payload`, the bytes a host writes after the
> +/// I2C write-address) to the downstream monitor's I2C slave `addr`. Same `id=0x15 sub=0x22`
> +/// framing as [`ddc_set_vcp`], with the slave + payload length at off20 and the payload at off22.
> +/// This is what the vino I2C adapter forwards for a userspace DDC/CI write (e.g. `ddcutil`).
> +pub(super) fn ddc_forward(counter: u16, addr: u8, payload: &[u8]) -> Result<KVec<u8>> {
> +    let mut b = KVec::with_capacity(32 + payload.len(), GFP_KERNEL)?;

[Severity: High]
This is a pre-existing issue, but can this GFP_KERNEL allocation cause a
scheduling while atomic panic?

The CRTC atomic_disable callback runs in atomic context where sleeping is
forbidden. It calls set_vcp(), which delegates to cp::ddc_set_vcp() and 
subsequently cp::ddc_forward().

Allocating with GFP_KERNEL here can sleep. Could this allocation use
GFP_ATOMIC instead?

>      header(&mut b, 0x15, 0x22, counter)?;
>      pad_to(&mut b, 20)?;
> -    // off20: monitor DDC/CI I2C slave (0x37) + DDC/CI payload length.
> -    b.extend_from_slice(&[0x37, payload.len() as u8], GFP_KERNEL)?;
> -    // off22: the DDC/CI Set-VCP bytes (same off22 convention as the EDID payload).
> -    b.extend_from_slice(&payload, GFP_KERNEL)?;
> +    // off20: monitor DDC/CI I2C slave + DDC/CI payload length.
> +    b.extend_from_slice(&[addr, payload.len() as u8], GFP_KERNEL)?;

[Severity: High]
Can a payload larger than 255 bytes cause malformed CP packets here?

The payload.len() is cast to u8 without bounds checking, meaning the protocol
packet header receives a truncated length, but the full unbounded payload is 
appended below.

Could this cause the downstream dock's CP parser to read misaligned framing,
potentially hanging the hardware or corrupting the stream?

> +    // off22: the DDC/CI bytes (same off22 convention as the EDID payload).
> +    b.extend_from_slice(payload, GFP_KERNEL)?;
>      pad_to(&mut b, 32)?;
>      Ok(b)
>  }

> diff --git a/drivers/gpu/drm/vino/drm_sink.rs b/drivers/gpu/drm/vino/drm_sink.rs
> --- a/drivers/gpu/drm/vino/drm_sink.rs
> +++ b/drivers/gpu/drm/vino/drm_sink.rs

[ ... ]

> @@ -41,6 +42,7 @@
>          plane::{self, PlaneAtomicCommit, RawPlaneState as _},
>          KmsDriver, ModeConfigGuard, ModeConfigInfo, ModeObject as _, NewKmsDevice, Probing,
>      },
> +    i2c,
> +    error::code::EINVAL,

[Severity: Low]
Does this import follow the kernel vertical import style for Rust?

The i2c import is placed on the same line as other items instead of being
vertically aligned.

[ ... ]

> @@ -125,6 +127,12 @@ pub(super) struct VinoDrmData {
>      /// can reach its cached-EDID slot without needing DRM's mode-object list. Written once,
>      /// during single-threaded probe, before the device is registered; read-only thereafter.
>      connector: core::sync::atomic::AtomicPtr<VinoConnector>,
> +    /// The CRTC's gamma ramp cached from the atomic hook as three 256-entry 8-bit LUTs
> +    /// (`[r; 256] ++ [g; 256] ++ [b; 256]`), or `None` for identity. Cached here (not read from
> +    /// the CRTC state) because scanout runs in the plane path; it is `Copy`, so the scanout
> +    /// snapshots it under the lock and applies it without holding the lock in the pixel loop.
> +    #[pin]
> +    gamma: Mutex<Option<[u8; 768]>>,
>  }
>  
>  impl VinoDrmData {

[Severity: High]
Does the use of a sleeping Mutex here violate atomic context rules?

The update_gamma() and gamma_snapshot() functions lock this Mutex, but they
are invoked synchronously during the CRTC atomic_enable, atomic_disable,
and plane atomic_update callbacks.

According to DRM subsystem guidelines, these callbacks run in atomic context
during non-blocking commits where sleeping is strictly forbidden. Should this
be a spinlock instead to avoid scheduling while atomic panics?

[ ... ]

> @@ -302,6 +333,16 @@ fn probe(dev: &NewKmsDevice<'_, Self, Probing>) -> Result {
>              None,
>              false,
>          )?;
> +        // Advertise the rotations vino's re-encode handles 1:1 (dimension-preserving): the
> +        // scanout applies them per source pixel via `rot_src`. 90/270 (which swap width/height)
> +        // are intentionally omitted.
> +        primary.create_rotation_property(
> +            bindings::DRM_MODE_ROTATE_0,
> +            bindings::DRM_MODE_ROTATE_0
> +                | bindings::DRM_MODE_ROTATE_180
> +                | bindings::DRM_MODE_REFLECT_X
> +                | bindings::DRM_MODE_REFLECT_Y,
> +        )?;
>          let cursor = plane::UnregisteredPlane::<VinoPlane>::new(
>              dev,
>              1,
>              &CURSOR_FORMATS,
>              None,
>              plane::Type::Cursor,
>              None,
>              PlaneArgs { head: 0, is_cursor: true },
>          )?;

[Severity: Medium]
Is the cursor plane missing the rotation property?

The commit message mentions wiring rotation for the primary/cursor plane, but
create_rotation_property() is not called for the cursor plane during probe.

Additionally, read_cursor_bgra() maps and reads the cursor framebuffer
without applying any rotation transformation.

[ ... ]

> @@ -372,6 +415,8 @@ fn new(_device: &drm::Device<Self::Driver, drm::Uninit>, _args: &()) -> impl Pin
>      fn atomic_enable(commit: CrtcAtomicCommit<'_, Self>) {
>          let data: &VinoDrmData = commit.crtc().drm_dev();
>          let new = commit.take_new_state();
> +        // Cache the gamma ramp for the scanout to apply.
> +        data.update_gamma(new.gamma_lut());

[Severity: Medium]
Are dynamic GAMMA_LUT property updates silently ignored while the display
is active?

The driver caches the GAMMA_LUT value only in the atomic_enable and
atomic_disable CRTC hooks.

If a compositor dynamically updates the gamma LUT, it will trigger
atomic_flush or atomic_update, but skip the enable hook when the CRTC is
already active, potentially missing color calibration changes until a full
modeset occurs.

[ ... ]

> @@ -489,6 +535,8 @@ fn atomic_update(commit: PlaneAtomicCommit<'_, Self>) {
>          // primary plane 1:1 with the virtual output), so this drives the dynamic scanout
>          // resolution.
>          let (w, h) = (new.crtc_w() as usize, new.crtc_h() as usize);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the lack of plane scaling
rejection allow userspace to trigger out-of-bounds kernel memory reads?

VinoPlane lacks an atomic_check function to reject scaling (e.g., using
drm_atomic_helper_check_plane_state with DRM_PLANE_NO_SCALING).

In atomic_update(), the scanout loop uses the destination CRTC geometry
(new.crtc_w(), new.crtc_h()) as the bounds instead of the source framebuffer
dimensions.

If unprivileged userspace issues an atomic commit with crtc_h greater than
fb.height(), the scanout loop will read past the end of the mapped GEM buffer.

[ ... ]

> @@ -748,13 +816,62 @@ fn get_modes<'a>(
>      }
>  }
>  
> +// ---- DDC/CI I2C adapter -----------------------------------------------------
> +
> +/// vino's DDC/CI virtual I2C bus: a userspace monitor-control tool (`ddcutil`, the desktop
> +/// brightness slider via the I2C DDC path) writes a DDC/CI transaction to the monitor's I2C slave
> +/// on this adapter, and vino tunnels it to the downstream monitor over the dock's CP channel
> +/// (`cp::ddc_forward`, `id=0x15 sub=0x22`) -- the same monitor-I2C bridge vino's DPMS-power VCP
> +/// uses. Writes only for now (Get-VCP reads need the CP reply path); a no-op until CP engages.
> +pub(super) struct VinoI2c;
> +
> +impl i2c::BusController for VinoI2c {
> +    type Context = ARef<VinoDrmDevice>;
> +
> +    fn master_xfer(dev: &ARef<VinoDrmDevice>, msgs: &mut [i2c::Msg]) -> Result<usize> {
> +        let data: &VinoDrmData = dev;
> +        let mut transferred = 0usize;
> +        for msg in msgs.iter() {
> +            if msg.addr() != super::cp::DDCCI_I2C_ADDR as u16 {
> +                continue;
> +            }
> +            if msg.is_read() {
> +                // DDC/CI reads (Get-VCP) require decoding the dock's CP reply -- not wired yet.
> +                continue;
> +            }

[Severity: Medium]
Should master_xfer() return an error when encountering unsupported messages
rather than skipping them?

If master_xfer() receives an array of messages where an early message is
unsupported (e.g., a read) and a later message succeeds, it uses continue to
skip the error and returns a positive transferred count.

This violates the I2C subsystem API contract and causes userspace tools like
ddcutil to assume operations succeeded when they actually failed.

> +            if data
> +                .send_cp(0x15, 0, |ctr| {
> +                    super::cp::ddc_forward(ctr, super::cp::DDCCI_I2C_ADDR, msg.buf())
> +                })
> +                .is_ok()
> +            {
> +                transferred += 1;
> +            }
> +        }
> +        Ok(transferred)
> +    }

[ ... ]

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