From 81f42c1a368bd09dd9183cec33068d829a7fe93c Mon Sep 17 00:00:00 2001 From: Pujun Lun Date: Mon, 21 Nov 2022 23:35:08 -0800 Subject: [PATCH] crosvm: update CLI syntax for specifying DPI for displays. The current syntax is: --gpu-display horizontal-dpi=100,vertical-dpi=200 We would like to change that to: --gpu-display dpi=[100,200] We will keep the backward compatibility for now, but the caller should not use both of syntax at the same time. FixedGpuDisplayParameters is introduced for backward compatibility support, and it will be removed once the old syntax is deprecated. BUG=b:260101753 TEST=presubmit Change-Id: I66a31c448ecc1eba60f6d1cbcbc753315c6c1df6 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4049340 Reviewed-by: Alexandre Courbot Reviewed-by: Jason Macnak Commit-Queue: Pujun Lun --- devices/src/virtio/gpu/edid.rs | 8 +- src/crosvm/cmdline.rs | 36 ++++++++- src/crosvm/gpu_config.rs | 143 ++++++++++++++++++++++++++++++--- vm_control/src/gpu.rs | 29 ++++--- 4 files changed, 186 insertions(+), 30 deletions(-) diff --git a/devices/src/virtio/gpu/edid.rs b/devices/src/virtio/gpu/edid.rs index ddd525176a..0e0cc7da59 100644 --- a/devices/src/virtio/gpu/edid.rs +++ b/devices/src/virtio/gpu/edid.rs @@ -124,13 +124,13 @@ impl DisplayInfo { pub fn new(params: &GpuDisplayParameters) -> Self { let (width, height) = params.get_virtual_display_size(); - let width_millimeters = if params.horizontal_dpi != 0 { - ((width as f32 / params.horizontal_dpi as f32) * MILLIMETERS_PER_INCH) as u16 + let width_millimeters = if params.horizontal_dpi() != 0 { + ((width as f32 / params.horizontal_dpi() as f32) * MILLIMETERS_PER_INCH) as u16 } else { 0 }; - let height_millimeters = if params.vertical_dpi != 0 { - ((height as f32 / params.vertical_dpi as f32) * MILLIMETERS_PER_INCH) as u16 + let height_millimeters = if params.vertical_dpi() != 0 { + ((height as f32 / params.vertical_dpi() as f32) * MILLIMETERS_PER_INCH) as u16 } else { 0 }; diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index c371b0e86b..32554ba472 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -41,6 +41,8 @@ use devices::virtio::device_constants::video::VideoDeviceConfig; use devices::virtio::snd::parameters::Parameters as SndParameters; use devices::virtio::vhost::user::device; #[cfg(feature = "gpu")] +use devices::virtio::GpuDisplayParameters; +#[cfg(feature = "gpu")] use devices::virtio::GpuParameters; #[cfg(unix)] use devices::virtio::NetParameters; @@ -57,9 +59,9 @@ use resources::AddressRange; use serde::Deserialize; #[cfg(feature = "gpu")] use serde_keyvalue::FromKeyValues; -#[cfg(feature = "gpu")] -use vm_control::gpu::DisplayParameters as GpuDisplayParameters; +#[cfg(feature = "gpu")] +use super::gpu_config::fixup_gpu_display_options; #[cfg(feature = "gpu")] use super::gpu_config::fixup_gpu_options; #[cfg(all(feature = "gpu", feature = "virgl_renderer_next"))] @@ -639,6 +641,24 @@ impl TryFrom for FixedGpuParameters { } } +/// Container for `GpuDisplayParameters` that have been fixed after parsing using serde. +/// +/// This deserializes as a regular `GpuDisplayParameters` and applies validation. +/// TODO(b/260101753): Remove this once the old syntax for specifying DPI is deprecated. +#[cfg(feature = "gpu")] +#[derive(Debug, Deserialize, FromKeyValues)] +#[serde(try_from = "GpuDisplayParameters")] +pub struct FixedGpuDisplayParameters(pub GpuDisplayParameters); + +#[cfg(feature = "gpu")] +impl TryFrom for FixedGpuDisplayParameters { + type Error = String; + + fn try_from(gpu_display_params: GpuDisplayParameters) -> Result { + fixup_gpu_display_options(gpu_display_params) + } +} + /// Deserialize `config_file` into a `RunCommand`. #[cfg(feature = "config-file")] fn load_config_file>(config_file: P) -> Result, String> { @@ -1102,7 +1122,15 @@ pub struct RunCommand { /// initially hidden (default: false). /// refresh-rate=INT - Force a specific vsync generation /// rate in hertz on the guest (default: 60) - pub gpu_display: Vec, + /// dpi=[INT,INT] - The horizontal and vertical DPI of the + /// display (default: [320,320]) + /// horizontal-dpi=INT - The horizontal DPI of the display + /// (default: 320) + /// Deprecated - use `dpi` instead. + /// vertical-dpi=INT - The vertical DPI of the display + /// (default: 320) + /// Deprecated - use `dpi` instead. + pub gpu_display: Vec, #[cfg(all(unix, feature = "gpu", feature = "virgl_renderer_next"))] #[argh(option)] @@ -2461,7 +2489,7 @@ impl TryFrom for super::config::Config { cfg.gpu_parameters .get_or_insert_with(Default::default) .display_params - .extend(cmd.gpu_display); + .extend(cmd.gpu_display.into_iter().map(|p| p.0)); } #[cfg(windows)] diff --git a/src/crosvm/gpu_config.rs b/src/crosvm/gpu_config.rs index d0bebc9448..e4b51d317d 100644 --- a/src/crosvm/gpu_config.rs +++ b/src/crosvm/gpu_config.rs @@ -7,7 +7,9 @@ use devices::virtio::GpuDisplayParameters; #[cfg(feature = "gfxstream")] use devices::virtio::GpuMode; use devices::virtio::GpuParameters; +use vm_control::gpu::DEFAULT_DPI; +use crate::crosvm::cmdline::FixedGpuDisplayParameters; use crate::crosvm::cmdline::FixedGpuParameters; use crate::crosvm::config::Config; @@ -16,7 +18,9 @@ fn default_use_vulkan() -> bool { !cfg!(window) } -pub fn fixup_gpu_options(mut gpu_params: GpuParameters) -> Result { +pub(crate) fn fixup_gpu_options( + mut gpu_params: GpuParameters, +) -> Result { match ( gpu_params.__width_compat.take(), gpu_params.__height_compat.take(), @@ -59,6 +63,36 @@ pub fn fixup_gpu_options(mut gpu_params: GpuParameters) -> Result Result { + let (horizontal_dpi_compat, vertical_dpi_compat) = ( + display_params.__horizontal_dpi_compat.take(), + display_params.__vertical_dpi_compat.take(), + ); + // Make sure `display_params.dpi` is always populated. + display_params.dpi = Some(match display_params.dpi { + Some(dpi) => { + if horizontal_dpi_compat.is_some() || vertical_dpi_compat.is_some() { + return Err( + "if 'dpi' is supplied, 'horizontal-dpi' and 'vertical-dpi' must not be supplied" + .to_string(), + ); + } + dpi + } + None => ( + horizontal_dpi_compat.unwrap_or(DEFAULT_DPI), + vertical_dpi_compat.unwrap_or(DEFAULT_DPI), + ), + }); + + Ok(FixedGpuDisplayParameters(display_params)) +} + pub(crate) fn validate_gpu_config(cfg: &mut Config) -> Result<(), String> { if let Some(gpu_parameters) = cfg.gpu_parameters.as_mut() { if !gpu_parameters.pci_bar_size.is_power_of_two() { @@ -518,25 +552,110 @@ mod tests { #[test] fn parse_gpu_display_options_dpi() { - const HORIZONTAL_DPI: u32 = 320; + const HORIZONTAL_DPI: u32 = 160; const VERTICAL_DPI: u32 = 25; - let display_params = parse_gpu_display_options( - format!( - "horizontal-dpi={},vertical-dpi={}", - HORIZONTAL_DPI, VERTICAL_DPI - ) - .as_str(), + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu-display", + format!("dpi=[{},{}]", HORIZONTAL_DPI, VERTICAL_DPI).as_str(), + "/dev/null", + ], ) + .unwrap() + .try_into() .unwrap(); - assert_eq!(display_params.horizontal_dpi, HORIZONTAL_DPI); - assert_eq!(display_params.vertical_dpi, VERTICAL_DPI); + + let gpu_params = config.gpu_parameters.unwrap(); + + assert_eq!(gpu_params.display_params.len(), 1); + assert_eq!( + gpu_params.display_params[0].horizontal_dpi(), + HORIZONTAL_DPI + ); + assert_eq!(gpu_params.display_params[0].vertical_dpi(), VERTICAL_DPI); + } + + #[test] + fn parse_gpu_display_options_default_dpi() { + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &["--gpu-display", "mode=windowed[800,600]", "/dev/null"], + ) + .unwrap() + .try_into() + .unwrap(); + + let gpu_params = config.gpu_parameters.unwrap(); + + assert_eq!(gpu_params.display_params.len(), 1); + assert_eq!(gpu_params.display_params[0].horizontal_dpi(), DEFAULT_DPI); + assert_eq!(gpu_params.display_params[0].vertical_dpi(), DEFAULT_DPI); + } + + #[test] + fn parse_gpu_display_options_dpi_compat() { + const HORIZONTAL_DPI: u32 = 160; + const VERTICAL_DPI: u32 = 25; + + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu-display", + format!( + "horizontal-dpi={},vertical-dpi={}", + HORIZONTAL_DPI, VERTICAL_DPI + ) + .as_str(), + "/dev/null", + ], + ) + .unwrap() + .try_into() + .unwrap(); + + let gpu_params = config.gpu_parameters.unwrap(); + + assert_eq!(gpu_params.display_params.len(), 1); + assert_eq!( + gpu_params.display_params[0].horizontal_dpi(), + HORIZONTAL_DPI + ); + assert_eq!(gpu_params.display_params[0].vertical_dpi(), VERTICAL_DPI); } #[test] fn parse_gpu_display_options_dpi_duplicated() { - assert!(parse_gpu_display_options("horizontal-dpi=160,horizontal-dpi=320").is_err()); - assert!(parse_gpu_display_options("vertical-dpi=25,vertical-dpi=50").is_err()); + assert!(crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu-display", + "horizontal-dpi=160,horizontal-dpi=320", + "/dev/null", + ], + ) + .is_err()); + + assert!(crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu-display", + "vertical-dpi=25,vertical-dpi=50", + "/dev/null", + ], + ) + .is_err()); + + assert!(crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu-display", + "dpi=[160,320],horizontal-dpi=160,vertical-dpi=25", + "/dev/null", + ], + ) + .is_err()); } #[test] diff --git a/vm_control/src/gpu.rs b/vm_control/src/gpu.rs index ccd905865e..f0b2f1bc2d 100644 --- a/vm_control/src/gpu.rs +++ b/vm_control/src/gpu.rs @@ -24,10 +24,6 @@ fn default_refresh_rate() -> u32 { DEFAULT_REFRESH_RATE } -fn default_dpi() -> u32 { - DEFAULT_DPI -} - /// Trait that the platform-specific type `DisplayMode` needs to implement. pub(crate) trait DisplayModeTrait { /// Returns the initial host window size. @@ -55,10 +51,14 @@ pub struct DisplayParameters { pub hidden: bool, #[serde(default = "default_refresh_rate")] pub refresh_rate: u32, - #[serde(default = "default_dpi")] - pub horizontal_dpi: u32, - #[serde(default = "default_dpi")] - pub vertical_dpi: u32, + // TODO(b/260101753): `dpi` has to be an `Option` for supporting CLI backward compatibility. + // That should be changed once compat fields below are deprecated. + pub dpi: Option<(u32, u32)>, + // `horizontal-dpi` and `vertical-dpi` are supported for CLI backward compatibility. + #[serde(rename = "horizontal-dpi")] + pub __horizontal_dpi_compat: Option, + #[serde(rename = "vertical-dpi")] + pub __vertical_dpi_compat: Option, } impl DisplayParameters { @@ -73,8 +73,9 @@ impl DisplayParameters { mode, hidden, refresh_rate, - horizontal_dpi, - vertical_dpi, + dpi: Some((horizontal_dpi, vertical_dpi)), + __horizontal_dpi_compat: None, + __vertical_dpi_compat: None, } } @@ -89,6 +90,14 @@ impl DisplayParameters { pub fn get_virtual_display_size(&self) -> (u32, u32) { self.mode.get_virtual_display_size() } + + pub fn horizontal_dpi(&self) -> u32 { + self.dpi.expect("'dpi' is None").0 + } + + pub fn vertical_dpi(&self) -> u32 { + self.dpi.expect("'dpi' is None").1 + } } impl Default for DisplayParameters {