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 <acourbot@chromium.org>
Reviewed-by: Jason Macnak <natsu@google.com>
Commit-Queue: Pujun Lun <lunpujun@google.com>

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
         };

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<GpuParameters> 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<GpuDisplayParameters> for FixedGpuDisplayParameters {
+    type Error = String;
+
+    fn try_from(gpu_display_params: GpuDisplayParameters) -> Result<Self, Self::Error> {
+        fixup_gpu_display_options(gpu_display_params)
+    }
+}
+
 /// Deserialize `config_file` into a `RunCommand`.
 #[cfg(feature = "config-file")]
 fn load_config_file<P: AsRef<Path>>(config_file: P) -> Result<Box<RunCommand>, 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<GpuDisplayParameters>,
+    ///     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<FixedGpuDisplayParameters>,
 
     #[cfg(all(unix, feature = "gpu", feature = "virgl_renderer_next"))]
     #[argh(option)]
@@ -2461,7 +2489,7 @@ impl TryFrom<RunCommand> 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)]

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<FixedGpuParameters, String> {
+pub(crate) fn fixup_gpu_options(
+    mut gpu_params: GpuParameters,
+) -> Result<FixedGpuParameters, String> {
     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<FixedGpuParame
     Ok(FixedGpuParameters(gpu_params))
 }
 
+/// Fixes `GpuDisplayParameters` after parsing using serde.
+///
+/// The `dpi` field is guaranteed to be populated after this is called.
+pub(crate) fn fixup_gpu_display_options(
+    mut display_params: GpuDisplayParameters,
+) -> Result<FixedGpuDisplayParameters, String> {
+    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(
+        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();
+
+        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();
-        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_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]

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<u32>,
+    #[serde(rename = "vertical-dpi")]
+    pub __vertical_dpi_compat: Option<u32>,
 }
 
 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 {