Re: [RFC PATCH 6/7] drm/vino: add DDC/CI brightness/contrast, DPMS power and DFU info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `crtc_atomic_disable` callback executes in an atomic context but triggers sleeping operations, including `GFP_KERNEL` allocations.
- [High] The `connector_atomic_set_property` directly mutates global hardware state during the atomic check/assembly phase, violating DRM atomic semantics.
- [Medium] Missing contrast property replay during DPMS-on monitor wake.
--

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

Add monitor controls that ride the same control plane

This patch adds DDC/CI monitor controls over a custom control plane for
brightness, contrast, and DPMS. It adds connector properties for
brightness and contrast, and hooks into the CRTC disable path for power
off.

> diff --git a/drivers/gpu/drm/vino/drm_sink.rs b/drivers/gpu/drm/vino/drm_sink.rs
> index afbf883fba36..bcc871958a8a 100644
> --- a/drivers/gpu/drm/vino/drm_sink.rs
> +++ b/drivers/gpu/drm/vino/drm_sink.rs

[ ... ]

> @@ -637,6 +660,11 @@ fn install_edid(connector: *mut bindings::drm_connector, blob: &[u8]) -> i32 {
>      if let Err(e) = data.send_cp(head.index, 0x48, 16, |ctr| super::cp::set_mode(ctr, &timing)) {
>          pr_warn!("vino: head{} runtime mode-set send failed ({e:?})\n", head.index);
>      }
> +    // Bring the monitor out of DPMS standby (DDC/CI VCP 0xD6 = on). Inferred wire (see
> +    // `cp::ddc_set_vcp`); a no-op until CP engages, and re-applies the user's brightness.
> +    let _ = data.set_vcp(head.index, super::cp::VCP_POWER_MODE, super::cp::POWER_ON);
> +    let b = head.brightness.load(core::sync::atomic::Ordering::Relaxed);
> +    let _ = data.set_vcp(head.index, super::cp::VCP_BRIGHTNESS, b as u16);

[Severity: Medium]
Does this code omit re-applying the contrast property during the DPMS-on
monitor wake sequence?

The struct documentation for head.contrast states it is replayed on DPMS-on,
but only head.brightness is loaded and re-applied to the monitor here. If
contrast is not restored, the monitor could lose its custom contrast setting
when waking from standby and revert to hardware defaults.

>  }
>  
>  /// CRTC `.atomic_disable`: the display is turning off.

[ ... ]

> @@ -671,6 +701,10 @@ fn install_edid(connector: *mut bindings::drm_connector, blob: &[u8]) -> i32 {
>      }
>      head.cursor_primed
>          .store(false, core::sync::atomic::Ordering::SeqCst);
> +    // DPMS-off: blank the monitor backlight via DDC/CI (VCP 0xD6 = off) rather than leaving
> +    // the last frame frozen on the panel. Inferred wire (see `cp::ddc_set_vcp`); no-op until
> +    // CP engages.
> +    let _ = data.set_vcp(head.index, super::cp::VCP_POWER_MODE, super::cp::POWER_OFF);

[Severity: High]
Does crtc_atomic_disable() execute in an atomic context where sleeping is
forbidden?

This calls data.set_vcp(), which delegates to ddc_set_vcp(). As seen in
cp.rs, ddc_set_vcp() uses sleeping allocations:

cp.rs:ddc_set_vcp() {
    ...
    let mut b = KVec::with_capacity(32, GFP_KERNEL)?;
    ...
    b.extend_from_slice(&[0x37, payload.len() as u8], GFP_KERNEL)?;
    ...
}

Allocating memory with GFP_KERNEL in the atomic CRTC disable callback could
lead to a scheduling-while-atomic panic during display modesets.

>      pr_info!("vino: KMS CRTC disable -- head{} display OFF (scanout stopped)\n", head.index);
>  }

[ ... ]

> +unsafe extern "C" fn connector_atomic_set_property(
> +    connector: *mut bindings::drm_connector,
> +    _state: *mut bindings::drm_connector_state,
> +    property: *mut bindings::drm_property,
> +    val: u64,
> +) -> i32 {

[ ... ]

> +    if slot.swap(v, core::sync::atomic::Ordering::Relaxed) != v {
> +        let _ = data.set_vcp(head.index, vcp, v as u16);
> +    }
> +    0
> +}

[Severity: High]
Does this directly mutate global hardware state during the atomic check or
assembly phase?

In the DRM atomic architecture, the atomic_set_property callback can be
invoked during state assembly and checking, such as for test-only commits
(DRM_MODE_ATOMIC_TEST_ONLY).

Because data.set_vcp() immediately pushes the DDC/CI write to the monitor
during this phase, a test-only commit will irreversibly modify the live
hardware state, breaking the expectations of userspace compositors relying
on atomic test operations having no side effects.

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