From 38182f458e6e5f2749595f31c7e454a7f8624d7a Mon Sep 17 00:00:00 2001 From: Pujun Lun Date: Wed, 26 Oct 2022 12:14:01 -0700 Subject: [PATCH] crosvm: merge --gpu-display into --gpu in CLI. Now that serde_keyvalue is capable of parsing vectors and sub-structs, we are making the following pattern the official way of providing display parameters: --gpu backend=3d,displays=[[mode=windowed[800,600]],\ [mode=borderless_full_screen]] The reasons for this change are: (1) We will likely use this syntax for other CLI options as well. (2) We only support one single virtual GPU as of today, but technically we could support multiple. When we add that support, there is no ambiguity as to which display is connected to which GPU with this syntax. For backwards compatibility, we will preserve the following ways to provide display parameters: (1) --gpu width=800,height=600 (which implies windowed mode) (2) --gpu-display mode=windowed,width=800,height=600 \ --gpu-display mode=borderless_full_screen BUG=b:254284360 TEST=presubmit Change-Id: I7f0078b7cf7554c1e93882afce47a4a2a3170a78 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3984545 Commit-Queue: Pujun Lun Reviewed-by: Alexandre Courbot --- devices/src/virtio/gpu/parameters.rs | 2 +- src/crosvm/cmdline.rs | 27 +++--- src/crosvm/gpu_config.rs | 127 +++++++++++++++++++++++---- 3 files changed, 127 insertions(+), 29 deletions(-) diff --git a/devices/src/virtio/gpu/parameters.rs b/devices/src/virtio/gpu/parameters.rs index 855437072b..187bbc5eca 100644 --- a/devices/src/virtio/gpu/parameters.rs +++ b/devices/src/virtio/gpu/parameters.rs @@ -42,7 +42,7 @@ mod serde_context_mask { pub struct GpuParameters { #[serde(rename = "backend")] pub mode: GpuMode, - #[serde(skip)] + #[serde(rename = "displays")] pub display_params: Vec, // `width` and `height` are supported for CLI backwards compatibility. #[serde(rename = "width")] diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index 32554ba472..4f2dde042b 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -489,7 +489,7 @@ pub enum GpuSubCommand { pub struct GpuAddDisplaysCommand { #[argh(option)] /// displays - pub gpu_display: Vec, + pub gpu_display: Vec, #[argh(positional, arg_name = "VM_SOCKET")] /// VM Socket path @@ -1079,12 +1079,17 @@ pub struct RunCommand { /// Possible key values: /// backend=(2d|virglrenderer|gfxstream) - Which backend to /// use for virtio-gpu (determining rendering protocol) + /// displays=[[GpuDisplayParameters]] - The list of virtual + /// displays to create. See the possible key values for + /// GpuDisplayParameters in the section below. /// context-types=LIST - The list of supported context /// types, separated by ':' (default: no contexts enabled) /// width=INT - The width of the virtual display connected /// to the virtio-gpu. + /// Deprecated - use `displays` instead. /// height=INT - The height of the virtual display /// connected to the virtio-gpu. + /// Deprecated - use `displays` instead. /// egl[=true|=false] - If the backend should use a EGL /// context for rendering. /// glx[=true|=false] - If the backend should use a GLX @@ -1103,15 +1108,8 @@ pub struct RunCommand { /// cache-size=SIZE - The maximum size of the shader cache. /// pci-bar-size=SIZE - The size for the PCI BAR in bytes /// (default 8gb). - pub gpu: Vec, - - #[cfg(feature = "gpu")] - #[argh(option)] - #[serde(skip)] // TODO(b/255223604) - #[merge(strategy = append)] - /// (EXPERIMENTAL) Comma separated key=value pairs for setting - /// up a display on the virtio-gpu device - /// Possible key values: + /// + /// Possible key values for GpuDisplayParameters: /// mode=(borderless_full_screen|windowed[width,height]) - /// Whether to show the window on the host in full /// screen or windowed mode. If not specified, windowed @@ -1130,6 +1128,15 @@ pub struct RunCommand { /// vertical-dpi=INT - The vertical DPI of the display /// (default: 320) /// Deprecated - use `dpi` instead. + pub gpu: Vec, + + #[cfg(feature = "gpu")] + #[argh(option)] + #[serde(skip)] // TODO(b/255223604). Deprecated - use `gpu` instead. + #[merge(strategy = append)] + /// (EXPERIMENTAL) Comma separated key=value pairs for setting + /// up a display on the virtio-gpu device. See comments for `gpu` + /// for possible key values of GpuDisplayParameters. pub gpu_display: Vec, #[cfg(all(unix, feature = "gpu", feature = "virgl_renderer_next"))] diff --git a/src/crosvm/gpu_config.rs b/src/crosvm/gpu_config.rs index e4b51d317d..2efe1b37fa 100644 --- a/src/crosvm/gpu_config.rs +++ b/src/crosvm/gpu_config.rs @@ -659,19 +659,71 @@ mod tests { } #[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); + fn parse_gpu_options_single_display() { + { + let gpu_params = parse_gpu_options("displays=[[mode=windowed[800,600]]]").unwrap(); + assert_eq!(gpu_params.display_params.len(), 1); + assert_eq!( + gpu_params.display_params[0].mode, + GpuDisplayMode::Windowed(800, 600) + ); + } + + #[cfg(windows)] + { + let gpu_params = parse_gpu_options("displays=[[mode=borderless_full_screen]]").unwrap(); + assert_eq!(gpu_params.display_params.len(), 1); + assert!(matches!( + gpu_params.display_params[0].mode, + GpuDisplayMode::BorderlessFullScreen(_) + )); + } + } + + #[test] + fn parse_gpu_options_multi_display() { + { + let gpu_params = + parse_gpu_options("displays=[[mode=windowed[500,600]],[mode=windowed[700,800]]]") + .unwrap(); + assert_eq!(gpu_params.display_params.len(), 2); + assert_eq!( + gpu_params.display_params[0].mode, + GpuDisplayMode::Windowed(500, 600) + ); + assert_eq!( + gpu_params.display_params[1].mode, + GpuDisplayMode::Windowed(700, 800) + ); + } + + #[cfg(windows)] + { + let gpu_params = parse_gpu_options( + "displays=[[mode=windowed[800,600]],[mode=borderless_full_screen]]", + ) + .unwrap(); + assert_eq!(gpu_params.display_params.len(), 2); + assert_eq!( + gpu_params.display_params[0].mode, + GpuDisplayMode::Windowed(800, 600) + ); + assert!(matches!( + gpu_params.display_params[1].mode, + GpuDisplayMode::BorderlessFullScreen(_) + )); + } + } + + #[test] + fn parse_gpu_options_single_display_compat() { const BACKEND: &str = get_backend_name(); let config: Config = crate::crosvm::cmdline::RunCommand::from_args( &[], &[ "--gpu", - format!("backend={}", BACKEND).as_str(), - "--gpu-display", - format!("windowed[{},{}]", WIDTH, HEIGHT).as_str(), + format!("backend={},width=500,height=600", BACKEND,).as_str(), "/dev/null", ], ) @@ -682,14 +734,18 @@ mod tests { 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); + assert_eq!( + gpu_params.display_params[0].mode, + GpuDisplayMode::Windowed(500, 600) + ); - // `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(), + format!("backend={}", BACKEND,).as_str(), + "--gpu-display", + "mode=windowed[700,800]", "/dev/null", ], ) @@ -700,7 +756,10 @@ mod tests { 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); + assert_eq!( + gpu_params.display_params[0].mode, + GpuDisplayMode::Windowed(700, 800) + ); } #[cfg(unix)] @@ -711,9 +770,13 @@ mod tests { &[], &[ "--gpu", - format!("backend={},width=500,height=600", get_backend_name()).as_str(), + format!( + "backend={},width=500,height=600,displays=[[mode=windowed[700,800]]]", + get_backend_name() + ) + .as_str(), "--gpu-display", - "mode=windowed[700,800]", + "mode=windowed[900,1000]", "/dev/null", ], ) @@ -723,14 +786,18 @@ mod tests { let gpu_params = config.gpu_parameters.unwrap(); - assert_eq!(gpu_params.display_params.len(), 2); + assert_eq!(gpu_params.display_params.len(), 3); assert_eq!( - gpu_params.display_params[0].get_virtual_display_size(), - (500, 600), + gpu_params.display_params[0].mode, + GpuDisplayMode::Windowed(700, 800) ); assert_eq!( - gpu_params.display_params[1].get_virtual_display_size(), - (700, 800), + gpu_params.display_params[1].mode, + GpuDisplayMode::Windowed(500, 600) + ); + assert_eq!( + gpu_params.display_params[2].mode, + GpuDisplayMode::Windowed(900, 1000) ); } } @@ -763,6 +830,30 @@ mod tests { ) .unwrap(); assert!(Config::try_from(command).is_err()); + + let command = crate::crosvm::cmdline::RunCommand::from_args( + &[], + &[ + "--gpu", + "displays=[[mode=windowed[1280,720]]]", + "--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", + "displays=[[mode=windowed[500,600]],[mode=windowed[700,800]]]", + "/dev/null", + ], + ) + .unwrap(); + assert!(Config::try_from(command).is_err()); } #[test]