From b849ebd0e17da45e826595a83f59cc0267b3427f Mon Sep 17 00:00:00 2001 From: Pujun Lun Date: Thu, 6 Oct 2022 23:35:27 -0700 Subject: [PATCH] crosvm: GPU and display arg parsing for Windows. This replaces the handwritten arg parser with the serde_keyvalue based parser. Eventually we will unify the arg parsing with Unix. BUG=b:233676779 TEST=cargo b --features all-msvc64,gpu --no-default-features TEST=cargo t -p vm_control --features all-msvc64,gpu --no-default-features TEST=cargo t -p crosvm sys::windows::config:: --features all-msvc64,gpu --no-default-features Change-Id: I36a563be9767c7e5cbd3ab44f6a9ba23cd64cdb6 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3939033 Commit-Queue: Pujun Lun Reviewed-by: Alexandre Courbot --- Cargo.lock | 1 + devices/Cargo.toml | 2 +- devices/src/virtio/gpu/mod.rs | 9 +- devices/src/virtio/gpu/parameters.rs | 6 +- devices/src/virtio/gpu/virtio_gpu.rs | 2 + devices/src/virtio/mod.rs | 10 +- gpu_display/src/gpu_display_win/mod.rs | 16 + src/crosvm/cmdline.rs | 32 +- src/crosvm/sys/unix/config.rs | 7 +- src/crosvm/sys/windows/cmdline.rs | 6 +- src/crosvm/sys/windows/config.rs | 713 ++++++++----------------- vm_control/Cargo.toml | 3 + vm_control/src/gpu.rs | 17 +- vm_control/src/sys.rs | 2 +- vm_control/src/sys/unix/gpu.rs | 6 +- vm_control/src/sys/windows/gpu.rs | 69 ++- 16 files changed, 359 insertions(+), 542 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 31d29d53ac..f9301b2638 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1990,6 +1990,7 @@ dependencies = [ "sync", "thiserror", "vm_memory", + "winapi", ] [[package]] diff --git a/devices/Cargo.toml b/devices/Cargo.toml index 75726b1bbd..fcf48f4c67 100644 --- a/devices/Cargo.toml +++ b/devices/Cargo.toml @@ -46,6 +46,7 @@ disk = { path = "../disk" } downcast-rs = { version = "1.2.0", optional = true } enumn = "0.1.0" ffmpeg = { path = "../media/ffmpeg", optional = true } +gpu_display = { path = "../gpu_display", optional = true } rutabaga_gfx = { path = "../rutabaga_gfx" } hypervisor = { path = "../hypervisor" } kvm_sys = { path = "../kvm_sys" } @@ -82,7 +83,6 @@ vp8 = { path = "../media/vp8", optional = true } [target.'cfg(unix)'.dependencies] fuse = {path = "../fuse" } -gpu_display = { path = "../gpu_display", optional = true } libcras = { version = "*", optional = true } minijail = "*" p9 = "*" diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs index 5204a5fee2..af8fe7628d 100644 --- a/devices/src/virtio/gpu/mod.rs +++ b/devices/src/virtio/gpu/mod.rs @@ -83,6 +83,7 @@ pub enum GpuMode { Mode2D, #[serde(rename = "virglrenderer", alias = "3d", alias = "3D")] ModeVirglRenderer, + #[cfg(feature = "gfxstream")] #[serde(rename = "gfxstream")] ModeGfxstream, } @@ -1108,6 +1109,7 @@ impl Gpu { let component = match gpu_parameters.mode { GpuMode::Mode2D => RutabagaComponentType::Rutabaga2D, GpuMode::ModeVirglRenderer => RutabagaComponentType::VirglRenderer, + #[cfg(feature = "gfxstream")] GpuMode::ModeGfxstream => RutabagaComponentType::Gfxstream, }; @@ -1126,12 +1128,15 @@ impl Gpu { .set_use_glx(gpu_parameters.renderer_use_glx) .set_use_surfaceless(gpu_parameters.renderer_use_surfaceless) .set_use_vulkan(gpu_parameters.use_vulkan.unwrap_or_default()) - .set_use_guest_angle(gpu_parameters.gfxstream_use_guest_angle.unwrap_or_default()) - .set_support_gles31(gpu_parameters.gfxstream_support_gles31) .set_wsi(gpu_parameters.wsi.as_ref()) .set_use_external_blob(external_blob) .set_use_render_server(use_render_server); + #[cfg(feature = "gfxstream")] + let rutabaga_builder = rutabaga_builder + .set_use_guest_angle(gpu_parameters.gfxstream_use_guest_angle.unwrap_or_default()) + .set_support_gles31(gpu_parameters.gfxstream_support_gles31); + Gpu { exit_evt_wrtube, #[cfg(unix)] diff --git a/devices/src/virtio/gpu/parameters.rs b/devices/src/virtio/gpu/parameters.rs index c5a5e0f046..227cfeeb49 100644 --- a/devices/src/virtio/gpu/parameters.rs +++ b/devices/src/virtio/gpu/parameters.rs @@ -57,11 +57,13 @@ pub struct GpuParameters { pub renderer_use_glx: bool, #[serde(rename = "surfaceless")] pub renderer_use_surfaceless: bool, + #[cfg(feature = "gfxstream")] #[serde(rename = "angle")] pub gfxstream_use_guest_angle: Option, #[serde(rename = "vulkan")] pub use_vulkan: Option, - #[serde(skip)] + #[cfg(feature = "gfxstream")] + #[serde(rename = "gles31")] pub gfxstream_support_gles31: bool, pub wsi: Option, pub udmabuf: bool, @@ -82,6 +84,7 @@ impl Default for GpuParameters { renderer_use_gles: true, renderer_use_glx: false, renderer_use_surfaceless: true, + #[cfg(feature = "gfxstream")] gfxstream_use_guest_angle: None, use_vulkan: None, mode: if cfg!(feature = "virgl_renderer") { @@ -89,6 +92,7 @@ impl Default for GpuParameters { } else { GpuMode::Mode2D }, + #[cfg(feature = "gfxstream")] gfxstream_support_gles31: true, wsi: None, cache_path: None, diff --git a/devices/src/virtio/gpu/virtio_gpu.rs b/devices/src/virtio/gpu/virtio_gpu.rs index f29e9f3a60..8a9054870f 100644 --- a/devices/src/virtio/gpu/virtio_gpu.rs +++ b/devices/src/virtio/gpu/virtio_gpu.rs @@ -22,6 +22,8 @@ use rutabaga_gfx::ResourceCreate3D; use rutabaga_gfx::ResourceCreateBlob; use rutabaga_gfx::Rutabaga; use rutabaga_gfx::RutabagaBuilder; +#[cfg(windows)] +use rutabaga_gfx::RutabagaError; use rutabaga_gfx::RutabagaFence; use rutabaga_gfx::RutabagaFenceHandler; use rutabaga_gfx::RutabagaHandle; diff --git a/devices/src/virtio/mod.rs b/devices/src/virtio/mod.rs index f6c7ddcb19..f52895dc1b 100644 --- a/devices/src/virtio/mod.rs +++ b/devices/src/virtio/mod.rs @@ -27,6 +27,8 @@ mod virtio_pci_device; pub mod block; pub mod console; +#[cfg(feature = "gpu")] +pub mod gpu; pub mod resource_bridge; #[cfg(feature = "audio")] pub mod snd; @@ -38,6 +40,8 @@ pub use self::block::*; pub use self::console::*; pub use self::descriptor_utils::Error as DescriptorError; pub use self::descriptor_utils::*; +#[cfg(feature = "gpu")] +pub use self::gpu::*; pub use self::input::*; pub use self::interrupt::*; pub use self::iommu::*; @@ -54,15 +58,11 @@ cfg_if::cfg_if! { if #[cfg(unix)] { mod p9; mod pmem; - pub mod wl; + pub mod wl; pub mod fs; - #[cfg(feature = "gpu")] - pub mod gpu; pub mod net; - #[cfg(feature = "gpu")] - pub use self::gpu::*; pub use self::iommu::sys::unix::vfio_wrapper; pub use self::net::*; pub use self::p9::*; diff --git a/gpu_display/src/gpu_display_win/mod.rs b/gpu_display/src/gpu_display_win/mod.rs index d3b05019af..bd38ca71b4 100644 --- a/gpu_display/src/gpu_display_win/mod.rs +++ b/gpu_display/src/gpu_display_win/mod.rs @@ -36,6 +36,8 @@ use metrics::Metrics; pub use surface::NoopSurface as Surface; #[cfg(feature = "kiwi")] use sync::Mutex; +use vm_control::gpu::DisplayMode; +use vm_control::gpu::DisplayParameters; use window_message_processor::DisplaySendToWndProc; pub use window_procedure_thread::WindowProcedureThread; @@ -62,6 +64,20 @@ pub struct DisplayProperties { pub gpu_main_display_tube: Option>>, } +impl From<&DisplayParameters> for DisplayProperties { + fn from(params: &DisplayParameters) -> Self { + let is_fullscreen = matches!(params.mode, DisplayMode::BorderlessFullScreen(_)); + let (window_width, window_height) = params.get_window_size(); + + Self { + start_hidden: params.hidden, + is_fullscreen, + window_width, + window_height, + } + } +} + pub struct DisplayWin { wndproc_thread: WindowProcedureThread, display_closed_event: Event, diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index 22635559ed..23fa1aa5b6 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -7,8 +7,6 @@ cfg_if::cfg_if! { use std::net; use base::RawDescriptor; - #[cfg(feature = "gpu")] - use vm_control::gpu::DisplayParameters as GpuDisplayParameters; use devices::virtio::vhost::user::device::parse_wayland_sock; use super::sys::config::{ @@ -45,6 +43,8 @@ use devices::SerialParameters; use devices::StubPciParameters; use hypervisor::ProtectionType; use resources::AddressRange; +#[cfg(feature = "gpu")] +use vm_control::gpu::DisplayParameters as GpuDisplayParameters; #[cfg(feature = "gpu")] use super::sys::config::parse_gpu_options; @@ -708,7 +708,6 @@ pub struct RunCommand { /// initially hidden (default: false). /// refresh-rate=INT - Force a specific vsync generation /// rate in hertz on the guest (default: 60) - #[cfg(unix)] pub gpu_display: Vec, #[cfg(feature = "gpu")] #[argh(option, long = "gpu", from_str_fn(parse_gpu_options))] @@ -1720,6 +1719,23 @@ impl TryFrom for super::config::Config { #[cfg(feature = "gpu")] { cfg.gpu_parameters = cmd.gpu_params; + if !cmd.gpu_display.is_empty() { + cfg.gpu_parameters + .get_or_insert_with(Default::default) + .display_params + .extend(cmd.gpu_display); + } + + #[cfg(windows)] + if let Some(gpu_parameters) = &cfg.gpu_parameters { + let num_displays = gpu_parameters.display_params.len(); + if num_displays > 1 { + return Err(format!( + "Only one display is supported (supplied {})", + num_displays + )); + } + } } #[cfg(unix)] @@ -1770,16 +1786,6 @@ impl TryFrom for super::config::Config { .pivot_root = p; } - #[cfg(feature = "gpu")] - { - if !cmd.gpu_display.is_empty() { - cfg.gpu_parameters - .get_or_insert_with(Default::default) - .display_params - .extend(cmd.gpu_display); - } - } - cfg.net_vq_pairs = cmd.net_vq_pairs; } diff --git a/src/crosvm/sys/unix/config.rs b/src/crosvm/sys/unix/config.rs index b95843df75..68628c9b4d 100644 --- a/src/crosvm/sys/unix/config.rs +++ b/src/crosvm/sys/unix/config.rs @@ -446,8 +446,11 @@ mod tests { let gpu_params: GpuParameters = from_key_values("backend=virglrenderer").unwrap(); assert_eq!(gpu_params.mode, GpuMode::ModeVirglRenderer); - let gpu_params: GpuParameters = from_key_values("backend=gfxstream").unwrap(); - assert_eq!(gpu_params.mode, GpuMode::ModeGfxstream); + #[cfg(feature = "gfxstream")] + { + let gpu_params: GpuParameters = from_key_values("backend=gfxstream").unwrap(); + assert_eq!(gpu_params.mode, GpuMode::ModeGfxstream); + } } #[cfg(feature = "gpu")] diff --git a/src/crosvm/sys/windows/cmdline.rs b/src/crosvm/sys/windows/cmdline.rs index 8bc326f117..f58cc3ad7b 100644 --- a/src/crosvm/sys/windows/cmdline.rs +++ b/src/crosvm/sys/windows/cmdline.rs @@ -143,8 +143,12 @@ mod tests { "568", ]); } - if cfg!(feature = "gpu") { + if cfg!(all(feature = "gpu", feature = "gfxstream")) { args.extend(["--gpu", "angle=true,backend=gfxstream,egl=true,gles=false,glx=false,refresh_rate=60,surfaceless=false,vulkan=true,wsi=vk,display_mode=borderless_full_screen,hidden"]); + args.extend([ + "--gpu-display", + "mode=borderless_full_screen,hidden,refresh-rate=60", + ]); } if cfg!(feature = "audio") { args.extend(["--ac97", "backend=win_audio"]); diff --git a/src/crosvm/sys/windows/config.rs b/src/crosvm/sys/windows/config.rs index 09e5b7a98f..3201e44e15 100644 --- a/src/crosvm/sys/windows/config.rs +++ b/src/crosvm/sys/windows/config.rs @@ -4,32 +4,22 @@ use std::str::FromStr; -#[cfg(feature = "gpu")] -use base::info; #[cfg(all(feature = "prod-build", feature = "kiwi"))] use devices::serial_device::SerialType; #[cfg(feature = "gpu")] use devices::virtio::GpuDisplayMode; #[cfg(feature = "gpu")] use devices::virtio::GpuDisplayParameters; -#[cfg(feature = "gpu")] +#[cfg(all(feature = "gpu", feature = "gfxstream"))] use devices::virtio::GpuMode; #[cfg(feature = "gpu")] use devices::virtio::GpuParameters; -#[cfg(feature = "gpu")] -use devices::virtio::DEFAULT_REFRESH_RATE; #[cfg(feature = "audio")] use devices::Ac97Parameters; use devices::SerialParameters; -#[cfg(feature = "gpu")] -use rutabaga_gfx::calculate_context_mask; -#[cfg(feature = "gpu")] -use rutabaga_gfx::RutabagaWsi; use serde::Deserialize; use serde::Serialize; -#[cfg(feature = "gpu")] -use crate::crosvm::argument; use crate::crosvm::config::Config; #[cfg(feature = "audio")] @@ -41,16 +31,6 @@ pub fn parse_ac97_options( Err(format!("unknown ac97 parameter {} {}", key, value)) } -#[cfg(feature = "gpu")] -pub fn is_gpu_backend_deprecated(backend: &str) -> bool { - match backend { - "2d" | "2D" | "3d" | "3D" | "virglrenderer" => { - cfg!(feature = "gfxstream") - } - _ => false, - } -} - #[cfg(feature = "gfxstream")] pub fn use_vulkan() -> bool { false @@ -80,392 +60,43 @@ pub fn validate_config(_cfg: &mut Config) -> std::result::Result<(), String> { #[cfg(feature = "gpu")] pub fn parse_gpu_options(s: &str) -> Result { - parse_gpu_options_inner(s).map_err(|e| e.to_string()) -} + use crate::crosvm::config::from_key_values; + let mut gpu_params: GpuParameters = from_key_values(s)?; -#[cfg(feature = "gpu")] -fn parse_gpu_options_inner(s: &str) -> argument::Result { - let mut gpu_params: GpuParameters = Default::default(); - #[cfg(feature = "gfxstream")] - let mut vulkan_specified = false; - #[cfg(feature = "gfxstream")] - let mut gles31_specified = false; - #[cfg(feature = "gfxstream")] - let mut angle_specified = false; - - let mut width: Option = None; - let mut height: Option = None; - let mut dpi: Option = None; - let mut display_mode: Option = None; - #[cfg(feature = "gfxstream")] - let mut refresh_rate: Option = None; - let opts = s - .split(',') - .map(|frag| frag.split('=')) - .map(|mut kv| (kv.next().unwrap_or(""), kv.next().unwrap_or(""))); - let mut hidden: Option = None; - - for (k, v) in opts { - match k { - "backend" => match v { - "2d" | "2D" => { - if crate::crosvm::sys::config::is_gpu_backend_deprecated(v) { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from( - "this backend type is deprecated, please use gfxstream.", - ), - }); - } else { - gpu_params.mode = GpuMode::Mode2D; - } - } - "3d" | "3D" | "virglrenderer" => { - if crate::crosvm::sys::config::is_gpu_backend_deprecated(v) { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from( - "this backend type is deprecated, please use gfxstream.", - ), - }); - } else { - gpu_params.mode = GpuMode::ModeVirglRenderer; - } - } - #[cfg(feature = "gfxstream")] - "gfxstream" => { - gpu_params.mode = GpuMode::ModeGfxstream; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from( - #[cfg(feature = "gfxstream")] - "gpu parameter 'backend' should be one of (2d|virglrenderer|gfxstream)", - #[cfg(not(feature = "gfxstream"))] - "gpu parameter 'backend' should be one of (2d|3d)", - ), - }); - } - }, - "egl" => match v { - "true" | "" => { - gpu_params.renderer_use_egl = true; - } - "false" => { - gpu_params.renderer_use_egl = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'egl' should be a boolean"), - }); - } - }, - "gles" => match v { - "true" | "" => { - gpu_params.renderer_use_gles = true; - } - "false" => { - gpu_params.renderer_use_gles = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'gles' should be a boolean"), - }); - } - }, - "glx" => match v { - "true" | "" => { - gpu_params.renderer_use_glx = true; - } - "false" => { - gpu_params.renderer_use_glx = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'glx' should be a boolean"), - }); - } - }, - "surfaceless" => match v { - "true" | "" => { - gpu_params.renderer_use_surfaceless = true; - } - "false" => { - gpu_params.renderer_use_surfaceless = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'surfaceless' should be a boolean"), - }); - } - }, - #[cfg(feature = "gfxstream")] - "angle" => { - angle_specified = true; - match v { - "true" | "" => { - gpu_params.gfxstream_use_guest_angle = true; - } - "false" => { - gpu_params.gfxstream_use_guest_angle = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'angle' should be a boolean"), - }); - } - } - } - "vulkan" => { - #[cfg(feature = "gfxstream")] - { - vulkan_specified = true; - } - match v { - "true" | "" => { - gpu_params.use_vulkan = true; - } - "false" => { - gpu_params.use_vulkan = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'vulkan' should be a boolean"), - }); - } - } - } - #[cfg(feature = "gfxstream")] - "gles3.1" => { - gles31_specified = true; - match v { - "true" | "" => { - gpu_params.gfxstream_support_gles31 = true; - } - "false" => { - gpu_params.gfxstream_support_gles31 = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'gles3.1' should be a boolean"), - }); - } - } - } - "wsi" => match v { - "vk" => { - gpu_params.wsi = Some(RutabagaWsi::Vulkan); - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'wsi' should be vk"), - }); - } - }, - "width" => { - if let Some(width) = width { - return Err(argument::Error::TooManyArguments(format!( - "width was already specified: {}", - width - ))); - } - width = Some( - v.parse::() - .map_err(|_| argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'width' must be a valid integer"), - })?, - ); - } - "height" => { - if let Some(height) = height { - return Err(argument::Error::TooManyArguments(format!( - "height was already specified: {}", - height - ))); - } - height = Some( - v.parse::() - .map_err(|_| argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from( - "gpu parameter 'height' must be a valid integer", - ), - })?, - ); - } - "dpi" => { - if let Some(dpi) = dpi { - return Err(argument::Error::TooManyArguments(format!( - "dpi was already specified: {}", - dpi - ))); - } - dpi = Some( - v.parse::() - .map_err(|_| argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'dpi' must be a valid integer"), - })?, - ); - } - #[cfg(feature = "gfxstream")] - "refresh_rate" => { - if let Some(refresh_rate) = refresh_rate { - return Err(argument::Error::TooManyArguments(format!( - "refresh_rate was already specified: {}", - refresh_rate - ))); - } - refresh_rate = - Some( - v.parse::() - .map_err(|_| argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from( - "gpu parameter 'refresh_rate' must be a valid integer", - ), - })?, - ); - } - "display_mode" => { - if let Some(display_mode) = display_mode { - return Err(argument::Error::TooManyArguments(format!( - "display_mode was already specified: {}", - display_mode - ))); - } - display_mode = Some(String::from(v)); - } - "hidden" => match v { - "true" | "" => { - hidden = Some(true); - } - "false" => { - hidden = Some(false); - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'hidden' should be a boolean"), - }); - } - }, - "cache-path" => gpu_params.cache_path = Some(v.to_string()), - "cache-size" => gpu_params.cache_size = Some(v.to_string()), - "udmabuf" => match v { - "true" | "" => { - gpu_params.udmabuf = true; - } - "false" => { - gpu_params.udmabuf = false; - } - _ => { - return Err(argument::Error::InvalidValue { - value: v.to_string(), - expected: String::from("gpu parameter 'udmabuf' should be a boolean"), - }); - } - }, - "context-types" => { - let context_types: Vec = v.split(':').map(|s| s.to_string()).collect(); - gpu_params.context_mask = calculate_context_mask(context_types); - } - "" => {} - _ => { - return Err(argument::Error::UnknownArgument(format!( - "gpu parameter {}", - k - ))); - } + match ( + gpu_params.__width_compat.take(), + gpu_params.__height_compat.take(), + ) { + (Some(width), Some(height)) => { + let display_mode = GpuDisplayMode::Windowed(width, height); + gpu_params + .display_params + .push(GpuDisplayParameters::default_with_mode(display_mode)); } - } - - let mut display_param = match display_mode.as_deref() { - Some("windowed") => GpuDisplayParameters::default_windowed(), - Some("borderless_full_screen") => GpuDisplayParameters::default_borderless_full_screen(), - None => Default::default(), - Some(display_mode) => { - return Err(argument::Error::InvalidValue { - value: display_mode.to_string(), - expected: String::from( - "gpu parameter 'display_mode' must be either 'borderless_full_screen' \ - or 'windowed'", - ), - }) - } - }; - - if let Some(hidden) = hidden { - display_param.hidden = hidden; - } - - #[cfg(feature = "gfxstream")] - { - if let Some(refresh_rate) = refresh_rate { - gpu_params.refresh_rate = refresh_rate; - } - } - - match display_param.display_mode { - GpuDisplayMode::Windowed { - width: ref mut width_in_params, - height: ref mut height_in_params, - dpi: ref mut dpi_in_params, - } => { - if let Some(width) = width { - *width_in_params = width; - } - if let Some(height) = height { - *height_in_params = height; - } - if let Some(dpi) = dpi { - *dpi_in_params = dpi; - } - } - GpuDisplayMode::BorderlessFullScreen(_) => { - if width.is_some() || height.is_some() || dpi.is_some() { - return Err(argument::Error::UnknownArgument( - "width, height, or dpi is only supported for windowed display mode".to_string(), - )); - } + (None, None) => {} + _ => { + return Err("must include both 'width' and 'height' if either is supplied".to_string()) } } #[cfg(feature = "gfxstream")] - { - if !vulkan_specified && gpu_params.mode == GpuMode::ModeGfxstream { - gpu_params.use_vulkan = crate::crosvm::sys::config::use_vulkan(); - } - if angle_specified || gles31_specified { - match gpu_params.mode { - GpuMode::ModeGfxstream => {} - _ => { - return Err(argument::Error::UnknownArgument( - "gpu parameters angle and gles3.1 are only supported for gfxstream backend" - .to_string(), - )); - } - } + if matches!(gpu_params.mode, GpuMode::ModeGfxstream) { + if gpu_params.use_vulkan.is_none() { + gpu_params.use_vulkan = Some(use_vulkan()); } + } else { + return Err(format!( + "backend type {:?} is deprecated, please use gfxstream", + gpu_params.mode + )); } - gpu_params.display_params = vec![display_param]; Ok(gpu_params) } #[cfg(feature = "gpu")] pub(crate) fn validate_gpu_config(cfg: &mut Config) -> Result<(), String> { - if let Some(gpu_parameters) = cfg.gpu_parameters.as_ref() { + if let Some(gpu_parameters) = cfg.gpu_parameters.as_mut() { if gpu_parameters.display_params.is_empty() { gpu_parameters.display_params.push(Default::default()); } @@ -477,10 +108,6 @@ pub(crate) fn validate_gpu_config(cfg: &mut Config) -> Result<(), String> { for virtio_single_touch in cfg.virtio_single_touch.iter_mut() { virtio_single_touch.set_default_size(width, height); } - - let dpi = gpu_parameters.display_params[0].get_dpi(); - info!("using dpi {} on the Android guest", dpi); - cfg.params.push(format!("androidboot.lcd_density={}", dpi)); } Ok(()) } @@ -541,14 +168,25 @@ impl FromStr for HypervisorKind { #[cfg(test)] mod tests { + #[cfg(feature = "gpu")] + use argh::FromArgs; #[cfg(feature = "gpu")] use devices::virtio::gpu::GpuDisplayMode; + #[cfg(all(feature = "gpu", feature = "gfxstream"))] + use rutabaga_gfx::RutabagaWsi; #[cfg(any(feature = "audio", feature = "gpu"))] use super::*; #[cfg(feature = "gpu")] + use crate::crosvm::config::from_key_values; + #[cfg(feature = "gpu")] use crate::crosvm::sys::config::parse_gpu_options; + #[cfg(feature = "gpu")] + fn parse_gpu_display_options(s: &str) -> Result { + from_key_values::(s) + } + #[cfg(all(feature = "gpu", feature = "gfxstream"))] #[test] fn parse_gpu_options_gfxstream_with_wsi_specified() { @@ -578,49 +216,24 @@ mod tests { #[cfg(all(feature = "gpu"))] #[test] fn parse_gpu_options_default_vulkan_support() { - #[cfg(unix)] - assert!( - !parse_gpu_options("backend=virglrenderer") - .unwrap() - .use_vulkan - ); #[cfg(feature = "gfxstream")] - assert!(!parse_gpu_options("backend=gfxstream").unwrap().use_vulkan); - #[cfg(all(feature = "gfxstream", unix))] - assert!(parse_gpu_options("backend=gfxstream").unwrap().use_vulkan); + assert_eq!( + parse_gpu_options("backend=gfxstream").unwrap().use_vulkan, + Some(false) + ); } #[cfg(all(feature = "gpu"))] #[test] fn parse_gpu_options_with_vulkan_specified() { - assert!(parse_gpu_options("vulkan=true").unwrap().use_vulkan); - #[cfg(unix)] - assert!( - parse_gpu_options("backend=virglrenderer,vulkan=true") - .unwrap() - .use_vulkan + assert_eq!( + parse_gpu_options("vulkan=true").unwrap().use_vulkan, + Some(true) ); - #[cfg(unix)] - assert!( - parse_gpu_options("vulkan=true,backend=virglrenderer") - .unwrap() - .use_vulkan + assert_eq!( + parse_gpu_options("vulkan=false").unwrap().use_vulkan, + Some(false) ); - assert!(!parse_gpu_options("vulkan=false").unwrap().use_vulkan); - #[cfg(unix)] - assert!( - !parse_gpu_options("backend=virglrenderer,vulkan=false") - .unwrap() - .use_vulkan - ); - #[cfg(unix)] - assert!( - !parse_gpu_options("vulkan=false,backend=virglrenderer") - .unwrap() - .use_vulkan - ); - #[cfg(unix)] - assert!(parse_gpu_options("backend=virglrenderer,vulkan=invalid_value").is_err()); assert!(parse_gpu_options("vulkan=invalid_value,backend=virglrenderer").is_err()); } @@ -628,130 +241,248 @@ mod tests { #[test] fn parse_gpu_options_gfxstream_with_gles31_specified() { assert!( - parse_gpu_options("backend=gfxstream,gles3.1=true") + parse_gpu_options("backend=gfxstream,gles31=true") .unwrap() .gfxstream_support_gles31 ); assert!( - parse_gpu_options("gles3.1=true,backend=gfxstream") + parse_gpu_options("gles31=true,backend=gfxstream") .unwrap() .gfxstream_support_gles31 ); assert!( - !parse_gpu_options("backend=gfxstream,gles3.1=false") + !parse_gpu_options("backend=gfxstream,gles31=false") .unwrap() .gfxstream_support_gles31 ); assert!( - !parse_gpu_options("gles3.1=false,backend=gfxstream") + !parse_gpu_options("gles31=false,backend=gfxstream") .unwrap() .gfxstream_support_gles31 ); - assert!(parse_gpu_options("backend=gfxstream,gles3.1=invalid_value").is_err()); - assert!(parse_gpu_options("gles3.1=invalid_value,backend=gfxstream").is_err()); + assert!(parse_gpu_options("backend=gfxstream,gles31=invalid_value").is_err()); + assert!(parse_gpu_options("gles31=invalid_value,backend=gfxstream").is_err()); } #[cfg(all(feature = "gpu", feature = "gfxstream"))] #[test] fn parse_gpu_options_not_gfxstream_with_gles31_specified() { - assert!(parse_gpu_options("backend=virglrenderer,gles3.1=true").is_err()); - assert!(parse_gpu_options("gles3.1=true,backend=virglrenderer").is_err()); + assert!(parse_gpu_options("backend=virglrenderer,gles31=true").is_err()); + assert!(parse_gpu_options("gles31=true,backend=virglrenderer").is_err()); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_gpu_display_mode() { - let display_params = parse_gpu_options("display_mode=windowed") + fn parse_gpu_options_no_display_specified() { + let display_params = parse_gpu_options("").unwrap().display_params; + assert!(display_params.is_empty()); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_display_size_valid() { + const WIDTH: u32 = 1720; + const HEIGHT: u32 = 1800; + + let display_params = parse_gpu_options(format!("width={},height=720", WIDTH).as_str()) .unwrap() .display_params; + assert_eq!(display_params.len(), 1); + assert!( + matches!(display_params[0].mode, GpuDisplayMode::Windowed(width, _) if width == WIDTH) + ); + + let display_params = parse_gpu_options(format!("width=1280,height={}", HEIGHT).as_str()) + .unwrap() + .display_params; + assert_eq!(display_params.len(), 1); + assert!( + matches!(display_params[0].mode, GpuDisplayMode::Windowed(_, height) if height == HEIGHT) + ); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_display_size_incomplete() { + assert!(parse_gpu_options("width=1280").is_err()); + assert!(parse_gpu_options("height=720").is_err()); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_display_size_duplicated() { + assert!(parse_gpu_options("width=1280,width=1280,height=720").is_err()); + assert!(parse_gpu_options("width=1280,height=720,height=720").is_err()); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_display_options_mode() { + let display_params = parse_gpu_display_options("mode=windowed[1280,720]").unwrap(); assert!(matches!( - display_params[0].display_mode, - GpuDisplayMode::Windowed { .. } + display_params.mode, + GpuDisplayMode::Windowed(_, _) )); - let display_params = parse_gpu_options("display_mode=borderless_full_screen") - .unwrap() - .display_params; + let display_params = parse_gpu_display_options("mode=borderless_full_screen").unwrap(); assert!(matches!( - display_params[0].display_mode, + display_params.mode, GpuDisplayMode::BorderlessFullScreen(_) )); - assert!(parse_gpu_options("display_mode=invalid_mode").is_err()); + assert!(parse_gpu_display_options("mode=invalid_mode").is_err()); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_gpu_display_mode_duplicated() { - assert!(parse_gpu_options("display_mode=windowed,display_mode=windowed").is_err()); + fn parse_gpu_display_options_mode_duplicated() { + assert!( + parse_gpu_display_options("mode=windowed[1280,720],mode=windowed[1280,720]").is_err() + ); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_borderless_full_screen_shouldnt_be_specified_with_size() { - assert!(parse_gpu_options("display_mode=borderless_full_screen,width=1280").is_err()); - assert!(parse_gpu_options("display_mode=borderless_full_screen,height=720").is_err()); + fn parse_gpu_display_options_borderless_full_screen_should_not_be_specified_with_size() { + assert!(parse_gpu_display_options("mode=borderless_full_screen[1280,720]").is_err()); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_windowed_with_size() { + fn parse_gpu_display_options_windowed_with_size() { const WIDTH: u32 = 1720; const HEIGHT: u32 = 1800; - const DPI: u32 = 1808; let display_params = - parse_gpu_options(format!("display_mode=windowed,width={}", WIDTH).as_str()) - .unwrap() - .display_params; - assert!( - matches!(display_params[0].display_mode, GpuDisplayMode::Windowed { width, .. } if width == WIDTH) - ); + parse_gpu_display_options(format!("mode=windowed[{},720]", WIDTH).as_str()).unwrap(); + assert!(matches!( + display_params.mode, + GpuDisplayMode::Windowed(width, _) if width == WIDTH + )); let display_params = - parse_gpu_options(format!("display_mode=windowed,height={}", HEIGHT).as_str()) - .unwrap() - .display_params; - assert!( - matches!(display_params[0].display_mode, GpuDisplayMode::Windowed { height, .. } if height == HEIGHT) - ); + parse_gpu_display_options(format!("mode=windowed[1280,{}]", HEIGHT).as_str()).unwrap(); + assert!(matches!( + display_params.mode, + GpuDisplayMode::Windowed(_, height) if height == HEIGHT + )); - let display_params = - parse_gpu_options(format!("display_mode=windowed,dpi={}", DPI).as_str()) - .unwrap() - .display_params; - assert!( - matches!(display_params[0].display_mode, GpuDisplayMode::Windowed { dpi, .. } if dpi == DPI) - ); + assert!(parse_gpu_display_options("mode=windowed[]").is_err()); + assert!(parse_gpu_display_options("mode=windowed[1280]").is_err()); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_hidden() { - let display_params = parse_gpu_options("hidden=true").unwrap().display_params; - assert!(display_params[0].hidden); + fn parse_gpu_display_options_hidden() { + let display_params = parse_gpu_display_options("hidden").unwrap(); + assert!(display_params.hidden); - let display_params = parse_gpu_options("hidden=false").unwrap().display_params; - assert!(matches!(display_params[0].hidden, false)); + let display_params = parse_gpu_display_options("hidden=true").unwrap(); + assert!(display_params.hidden); + + let display_params = parse_gpu_display_options("hidden=false").unwrap(); + assert!(!display_params.hidden); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_refresh_rate() { - const REFRESH_RATE: u32 = 120; - let display_params = parse_gpu_options(format!("refresh_rate={}", REFRESH_RATE).as_str()) - .unwrap() - .display_params; + fn parse_gpu_display_options_hidden_duplicated() { + assert!(parse_gpu_display_options("hidden,hidden").is_err()); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_display_options_refresh_rate() { + const REFRESH_RATE: u32 = 30; + + let display_params = + parse_gpu_display_options(format!("refresh-rate={}", REFRESH_RATE).as_str()).unwrap(); assert_eq!(display_params.refresh_rate, REFRESH_RATE); - - assert!(parse_gpu_options(format!("refresh_rate=invalid_value").as_str()).is_err()); } #[cfg(feature = "gpu")] #[test] - fn parse_gpu_options_size_duplicated() { - assert!(parse_gpu_options("width=1280,width=1280").is_err()); - assert!(parse_gpu_options("height=1280,height=1280").is_err()); - assert!(parse_gpu_options("dpi=1280,dpi=1280").is_err()); + fn parse_gpu_display_options_refresh_rate_duplicated() { + assert!(parse_gpu_display_options("refresh-rate=30,refresh-rate=60").is_err()); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_and_gpu_display_options_valid() { + const WIDTH: u32 = 1720; + const HEIGHT: u32 = 1800; + const EXPECTED_DISPLAY_MODE: GpuDisplayMode = GpuDisplayMode::Windowed(WIDTH, HEIGHT); + const BACKEND: &str = if cfg!(feature = "gfxstream") { + "gfxstream" + } else { + "2d" + }; + + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu", + format!("backend={}", BACKEND).as_str(), + "--gpu-display", + format!("windowed[{},{}]", WIDTH, HEIGHT).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].mode, EXPECTED_DISPLAY_MODE); + + // `width` and `height` in GPU options are supported for CLI backward compatibility. + let config: Config = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu", + format!("backend={},width={},height={}", BACKEND, WIDTH, HEIGHT).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].mode, EXPECTED_DISPLAY_MODE); + } + + #[cfg(feature = "gpu")] + #[test] + fn parse_gpu_options_and_gpu_display_options_multi_display_unsupported() { + let command = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu-display", + "mode=borderless_full_screen", + "--gpu-display", + "mode=borderless_full_screen", + "/dev/null", + ], + ) + .unwrap(); + assert!(Config::try_from(command).is_err()); + + let command = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu", + "width=1280,height=720", + "--gpu-display", + "mode=borderless_full_screen", + "/dev/null", + ], + ) + .unwrap(); + assert!(Config::try_from(command).is_err()); } } diff --git a/vm_control/Cargo.toml b/vm_control/Cargo.toml index d0d422f71f..c35b769112 100644 --- a/vm_control/Cargo.toml +++ b/vm_control/Cargo.toml @@ -28,3 +28,6 @@ serde_keyvalue = { path = "../serde_keyvalue", features = ["argh_derive"] } sync = { path = "../common/sync" } thiserror = "*" vm_memory = { path = "../vm_memory" } + +[target.'cfg(windows)'.dependencies] +winapi = "*" diff --git a/vm_control/src/gpu.rs b/vm_control/src/gpu.rs index 128bc0cf84..845b8f2a5c 100644 --- a/vm_control/src/gpu.rs +++ b/vm_control/src/gpu.rs @@ -5,8 +5,6 @@ use std::collections::BTreeMap as Map; use std::fmt; use std::fmt::Display; -#[cfg(windows)] -use std::marker::PhantomData; use std::path::Path; use serde::Deserialize; @@ -32,7 +30,14 @@ fn default_dpi() -> u32 { } /// Trait that the platform-specific type `DisplayMode` needs to implement. -pub trait DisplayModeTrait { +pub(crate) trait DisplayModeTrait { + /// Returns the initial host window size. + fn get_window_size(&self) -> (u32, u32); + + /// Returns the virtual display size used for creating the display device. + /// + /// This may be different from the initial host window size since different display backends may + /// have different alignment requirements on it. fn get_virtual_display_size(&self) -> (u32, u32); } @@ -42,7 +47,7 @@ impl Default for DisplayMode { } } -#[derive(Clone, Debug, PartialEq, Deserialize, FromKeyValues, Serialize)] +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, FromKeyValues)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] pub struct DisplayParameters { #[serde(default)] @@ -78,6 +83,10 @@ impl DisplayParameters { Self::new(mode, false, DEFAULT_REFRESH_RATE, DEFAULT_DPI, DEFAULT_DPI) } + pub fn get_window_size(&self) -> (u32, u32) { + self.mode.get_window_size() + } + pub fn get_virtual_display_size(&self) -> (u32, u32) { self.mode.get_virtual_display_size() } diff --git a/vm_control/src/sys.rs b/vm_control/src/sys.rs index 5cf50e86db..c164a35539 100644 --- a/vm_control/src/sys.rs +++ b/vm_control/src/sys.rs @@ -13,7 +13,7 @@ cfg_if::cfg_if! { pub mod windows; pub use windows as platform; #[cfg(feature = "gpu")] - pub type DisplayMode = platform::gpu::WinDisplayMode; + pub type DisplayMode = platform::gpu::WinDisplayMode; } else { compile_error!("Unsupported platform"); } diff --git a/vm_control/src/sys/unix/gpu.rs b/vm_control/src/sys/unix/gpu.rs index f6a71917fe..26a135798b 100644 --- a/vm_control/src/sys/unix/gpu.rs +++ b/vm_control/src/sys/unix/gpu.rs @@ -14,9 +14,13 @@ pub enum UnixDisplayMode { } impl DisplayModeTrait for UnixDisplayMode { - fn get_virtual_display_size(&self) -> (u32, u32) { + fn get_window_size(&self) -> (u32, u32) { match self { Self::Windowed(width, height) => (*width, *height), } } + + fn get_virtual_display_size(&self) -> (u32, u32) { + self.get_window_size() + } } diff --git a/vm_control/src/sys/windows/gpu.rs b/vm_control/src/sys/windows/gpu.rs index 227fbbc3fc..1b4377447e 100644 --- a/vm_control/src/sys/windows/gpu.rs +++ b/vm_control/src/sys/windows/gpu.rs @@ -16,43 +16,36 @@ use crate::gpu::DisplayModeTrait; const DISPLAY_WIDTH_SOFT_MAX: u32 = 1920; const DISPLAY_HEIGHT_SOFT_MAX: u32 = 1080; -#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum WinDisplayMode { Windowed(u32, u32), - BorderlessFullScreen(PhantomData), + BorderlessFullScreen(#[serde(skip)] PhantomData), } -impl DisplayModeTrait for WinDisplayMode { - fn get_virtual_display_size(&self) -> (u32, u32) { - let (width, height) = match self { +impl DisplayModeTrait for WinDisplayMode { + fn get_window_size(&self) -> (u32, u32) { + match self { Self::Windowed(width, height) => (*width, *height), - Self::BorderlessFullScreen(_) => { - let (width, height) = DisplayDataProvider::get_host_display_size(); - adjust_virtual_display_size(width, height) - } - }; + Self::BorderlessFullScreen(_) => T::get_host_display_size(), + } + } + + fn get_virtual_display_size(&self) -> (u32, u32) { + let (width, height) = self.get_window_size(); + let (width, height) = adjust_virtual_display_size(width, height); info!("Guest display size: {}x{}", width, height); (width, height) } } -impl From> for WinDisplayModeArg { - fn from(mode: WinDisplayMode) -> WinDisplayModeArg { - match mode { - WinDisplayMode::Windowed { .. } => WinDisplayModeArg::Windowed, - WinDisplayMode::BorderlessFullScreen(_) => WinDisplayModeArg::BorderlessFullScreen, - } - } -} - /// Trait for returning host display data such as resolution. Tests may overwrite this to specify /// display data rather than rely on properties of the actual display device. trait ProvideDisplayData { fn get_host_display_size() -> (u32, u32); } -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct DisplayDataProvider; impl ProvideDisplayData for DisplayDataProvider { @@ -78,3 +71,39 @@ fn adjust_virtual_display_size(width: u32, height: u32) -> (u32, u32) { let width = width - (width % 8); (width, height) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn borderless_full_screen_virtual_window_width_should_be_multiple_of_8() { + struct MockDisplayDataProvider; + + impl ProvideDisplayData for MockDisplayDataProvider { + fn get_host_display_size() -> (u32, u32) { + (1366, 768) + } + } + + let mode = WinDisplayMode::::BorderlessFullScreen(PhantomData); + let (width, _) = mode.get_virtual_display_size(); + assert_eq!(width % 8, 0); + } + + #[test] + fn borderless_full_screen_virtual_window_size_should_be_smaller_than_soft_max() { + struct MockDisplayDataProvider; + + impl ProvideDisplayData for MockDisplayDataProvider { + fn get_host_display_size() -> (u32, u32) { + (DISPLAY_WIDTH_SOFT_MAX + 1, DISPLAY_HEIGHT_SOFT_MAX + 1) + } + } + + let mode = WinDisplayMode::::BorderlessFullScreen(PhantomData); + let (width, height) = mode.get_virtual_display_size(); + assert!(width <= DISPLAY_WIDTH_SOFT_MAX); + assert!(height <= DISPLAY_HEIGHT_SOFT_MAX); + } +}