DisplayConfig: Specify mode ID string instead of mode spec

To be able to add more modes types that happen to have the same
resolution and refresh rate, change the API to specify modes using an
ID string. The ID string is temporary, and only works for associating a
mode for the monitor instance that it was part of.

https://bugzilla.gnome.org/show_bug.cgi?id=765011

src/backends/meta-monitor-manager.c

@@ -1492,7 +1492,7 @@ meta_monitor_manager_legacy_handle_apply_configuration  (MetaDBusDisplayConfig *
 #define META_DISPLAY_CONFIG_MODE_FLAGS_PREFERRED (1 << 0)
 #define META_DISPLAY_CONFIG_MODE_FLAGS_CURRENT (1 << 1)
 
-#define MODE_FORMAT "(iiddadu)"
+#define MODE_FORMAT "(siiddadu)"
 #define MODES_FORMAT "a" MODE_FORMAT
 #define MONITOR_SPEC_FORMAT "(ssss)"
 #define MONITOR_FORMAT "(" MONITOR_SPEC_FORMAT MODES_FORMAT "a{sv})"
@@ -1549,13 +1549,19 @@ meta_monitor_manager_handle_get_current_state (MetaDBusDisplayConfig *skeleton,
         {
           MetaMonitorMode *monitor_mode = k->data;
           GVariantBuilder supported_scales_builder;
-          MetaMonitorModeSpec *monitor_mode_spec;
+          const char *mode_id;
+          int mode_width, mode_height;
+          float refresh_rate;
           float preferred_scale;
           float *supported_scales;
           int n_supported_scales;
           uint32_t flags = 0;
 
-          monitor_mode_spec = meta_monitor_mode_get_spec (monitor_mode);
+          mode_id = meta_monitor_mode_get_id (monitor_mode);
+          meta_monitor_mode_get_resolution (monitor_mode,
+                                            &mode_width, &mode_height);
+          refresh_rate = meta_monitor_mode_get_refresh_rate (monitor_mode);
+
           preferred_scale =
             meta_monitor_manager_calculate_monitor_mode_scale (manager,
                                                                monitor,
@@ -1580,9 +1586,10 @@ meta_monitor_manager_handle_get_current_state (MetaDBusDisplayConfig *skeleton,
             flags |= META_DISPLAY_CONFIG_MODE_FLAGS_PREFERRED;
 
           g_variant_builder_add (&modes_builder, MODE_FORMAT,
-                                 monitor_mode_spec->width,
+                                 mode_id,
-                                 monitor_mode_spec->height,
+                                 mode_width,
-                                 monitor_mode_spec->refresh_rate,
+                                 mode_height,
+                                 refresh_rate,
                                  (double) preferred_scale,
                                  &supported_scales_builder,
                                  flags);
@@ -1797,13 +1804,16 @@ meta_monitor_manager_is_config_applicable (MetaMonitorManager *manager,
   return TRUE;
 }
 
-static MetaMonitorSpec *
+static MetaMonitor *
-find_monitor_spec (MetaMonitorManager *manager,
+find_monitor_from_connector (MetaMonitorManager *manager,
                              char               *connector)
 {
   GList *monitors;
   GList *l;
 
+  if (!connector)
+    return NULL;
+
   monitors = meta_monitor_manager_get_monitors (manager);
   for (l = monitors; l; l = l->next)
     {
@@ -1811,14 +1821,13 @@ find_monitor_spec (MetaMonitorManager *manager,
       MetaMonitorSpec *monitor_spec = meta_monitor_get_spec (monitor);
 
       if (g_str_equal (connector, monitor_spec->connector))
-        return meta_monitor_spec_clone (monitor_spec);
+        return monitor;
     }
 
   return NULL;
 }
 
-#define MONITOR_MODE_SPEC_FORMAT "(iid)"
+#define MONITOR_CONFIG_FORMAT "(ssa{sv})"
-#define MONITOR_CONFIG_FORMAT "(s" MONITOR_MODE_SPEC_FORMAT "a{sv})"
 #define MONITOR_CONFIGS_FORMAT "a" MONITOR_CONFIG_FORMAT
 
 #define LOGICAL_MONITOR_CONFIG_FORMAT "(iidub" MONITOR_CONFIGS_FORMAT ")"
@@ -1830,49 +1839,43 @@ create_monitor_config_from_variant (MetaMonitorManager *manager,
 {
 
   MetaMonitorConfig *monitor_config = NULL;
-  char *connector;
+  g_autofree char *connector = NULL;
+  g_autofree char *mode_id = NULL;
+  MetaMonitorMode *monitor_mode;
+  MetaMonitor *monitor;
   MetaMonitorSpec *monitor_spec;
   MetaMonitorModeSpec *monitor_mode_spec;
-  GVariant *properties_variant = NULL;
+  g_autoptr (GVariant) properties_variant = NULL;
   gboolean enable_underscanning = FALSE;
-  int32_t mode_width;
-  int32_t mode_height;
-  double mode_refresh_rate;
 
-  monitor_spec = g_new0 (MetaMonitorSpec, 1);
+  g_variant_get (monitor_config_variant, "(ss@a{sv})",
-  monitor_mode_spec = g_new0 (MetaMonitorModeSpec, 1);
-
-  g_variant_get (monitor_config_variant, "(s" MONITOR_MODE_SPEC_FORMAT "@a{sv})",
                  &connector,
-                 &mode_width,
+                 &mode_id,
-                 &mode_height,
-                 &mode_refresh_rate,
                  &properties_variant);
 
-  *monitor_mode_spec = (MetaMonitorModeSpec) {
+  monitor = find_monitor_from_connector (manager, connector);
-    .width = mode_width,
+  if (!monitor)
-    .height = mode_height,
-    .refresh_rate = mode_refresh_rate
-  };
-
-  monitor_spec = find_monitor_spec (manager, connector);
-  if (!monitor_spec)
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
                    "Invalid connector '%s' specified", connector);
-      g_free (monitor_mode_spec);
       return NULL;
     }
 
-  if (!meta_verify_monitor_mode_spec (monitor_mode_spec, error))
+  monitor_mode = meta_monitor_get_mode_from_id (monitor, mode_id);
+  if (!monitor_mode)
     {
-      g_free (monitor_mode_spec);
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
-      meta_monitor_spec_free (monitor_spec);
+                   "Invalid mode '%s' specified", mode_id);
       return NULL;
     }
 
   g_variant_lookup (properties_variant, "underscanning", "b", &enable_underscanning);
 
+  monitor_spec = meta_monitor_spec_clone (meta_monitor_get_spec (monitor));
+
+  monitor_mode_spec = g_new0 (MetaMonitorModeSpec, 1);
+  *monitor_mode_spec = *meta_monitor_mode_get_spec (monitor_mode);
+
   monitor_config = g_new0 (MetaMonitorConfig, 1);
   *monitor_config = (MetaMonitorConfig) {
     .monitor_spec = monitor_spec,

src/backends/meta-monitor.c

@@ -33,6 +33,7 @@
 
 typedef struct _MetaMonitorMode
 {
+  char *id;
   MetaMonitorModeSpec spec;
   MetaMonitorCrtcMode *crtc_modes;
 } MetaMonitorMode;
@@ -50,6 +51,7 @@ typedef struct _MetaMonitorPrivate
 
   GList *outputs;
   GList *modes;
+  GHashTable *mode_ids;
 
   MetaMonitorMode *preferred_mode;
   MetaMonitorMode *current_mode;
@@ -333,6 +335,7 @@ meta_monitor_finalize (GObject *object)
   MetaMonitor *monitor = META_MONITOR (object);
   MetaMonitorPrivate *priv = meta_monitor_get_instance_private (monitor);
 
+  g_hash_table_destroy (priv->mode_ids);
   g_list_free_full (priv->modes, (GDestroyNotify) meta_monitor_mode_free);
   g_clear_pointer (&priv->outputs, g_list_free);
   meta_monitor_spec_free (priv->spec);
@@ -341,6 +344,9 @@ meta_monitor_finalize (GObject *object)
 static void
 meta_monitor_init (MetaMonitor *monitor)
 {
+  MetaMonitorPrivate *priv = meta_monitor_get_instance_private (monitor);
+
+  priv->mode_ids = g_hash_table_new (g_str_hash, g_str_equal);
 }
 
 static void
@@ -351,6 +357,36 @@ meta_monitor_class_init (MetaMonitorClass *klass)
   object_class->finalize = meta_monitor_finalize;
 }
 
+static char *
+generate_mode_id (MetaMonitorModeSpec *monitor_mode_spec)
+{
+  char refresh_rate_str[G_ASCII_DTOSTR_BUF_SIZE];
+
+  g_ascii_dtostr (refresh_rate_str, G_ASCII_DTOSTR_BUF_SIZE,
+                  monitor_mode_spec->refresh_rate);
+
+  return g_strdup_printf ("%dx%d@%s",
+                          monitor_mode_spec->width,
+                          monitor_mode_spec->height,
+                          refresh_rate_str);
+}
+
+static gboolean
+meta_monitor_add_mode (MetaMonitor     *monitor,
+                       MetaMonitorMode *monitor_mode)
+{
+  MetaMonitorPrivate *priv = meta_monitor_get_instance_private (monitor);
+
+  if (g_hash_table_lookup (priv->mode_ids,
+                           meta_monitor_mode_get_id (monitor_mode)))
+    return FALSE;
+
+  priv->modes = g_list_append (priv->modes, monitor_mode);
+  g_hash_table_insert (priv->mode_ids, monitor_mode->id, monitor_mode);
+
+  return TRUE;
+}
+
 static void
 meta_monitor_normal_generate_modes (MetaMonitorNormal *monitor_normal)
 {
@@ -372,6 +408,7 @@ meta_monitor_normal_generate_modes (MetaMonitorNormal *monitor_normal)
         .height = crtc_mode->height,
         .refresh_rate = crtc_mode->refresh_rate
       },
+      mode->id = generate_mode_id (&mode->spec);
       mode->crtc_modes = g_new (MetaMonitorCrtcMode, 1);
       mode->crtc_modes[0] = (MetaMonitorCrtcMode) {
         .output = output,
@@ -383,7 +420,8 @@ meta_monitor_normal_generate_modes (MetaMonitorNormal *monitor_normal)
       if (output->crtc && crtc_mode == output->crtc->current_mode)
         monitor_priv->current_mode = mode;
 
-      monitor_priv->modes = g_list_append (monitor_priv->modes, mode);
+      if (!meta_monitor_add_mode (monitor, mode))
+        meta_monitor_mode_free (mode);
     }
 }
 
@@ -671,15 +709,22 @@ create_tiled_monitor_mode (MetaMonitorTiled *monitor_tiled,
   MetaMonitorPrivate *monitor_priv =
     meta_monitor_get_instance_private (monitor);
   MetaMonitorModeTiled *mode;
+  int width, height;
   GList *l;
   unsigned int i;
   gboolean is_preferred = TRUE;
 
   mode = g_new0 (MetaMonitorModeTiled, 1);
   mode->is_tiled = TRUE;
-  meta_monitor_tiled_calculate_tiled_size (monitor,
+  meta_monitor_tiled_calculate_tiled_size (monitor, &width, &height);
-                                           &mode->parent.spec.width,
+
-                                           &mode->parent.spec.height);
+  mode->parent.spec = (MetaMonitorModeSpec) {
+    .width = width,
+    .height = height,
+    .refresh_rate = refresh_rate,
+  };
+  mode->parent.id = generate_mode_id (&mode->parent.spec);
+
   mode->parent.crtc_modes = g_new0 (MetaMonitorCrtcMode,
                                     g_list_length (monitor_priv->outputs));
   for (l = monitor_priv->outputs, i = 0; l; l = l->next, i++)
@@ -701,8 +746,6 @@ create_tiled_monitor_mode (MetaMonitorTiled *monitor_tiled,
         .crtc_mode = tiled_crtc_mode
       };
 
-      mode->parent.spec.refresh_rate = refresh_rate;
-
       is_preferred = is_preferred && tiled_crtc_mode == output->preferred_mode;
     }
 
@@ -720,6 +763,8 @@ generate_tiled_monitor_modes (MetaMonitorTiled *monitor_tiled)
   MetaOutput *main_output;
   GList *tiled_modes = NULL;
   unsigned int i;
+  MetaMonitorMode *best_mode = NULL;
+  GList *l;
 
   main_output = meta_monitor_get_main_output (META_MONITOR (monitor_tiled));
 
@@ -746,24 +791,28 @@ generate_tiled_monitor_modes (MetaMonitorTiled *monitor_tiled)
         monitor_priv->preferred_mode = mode;
     }
 
-  if (!monitor_priv->preferred_mode)
+  while ((l = tiled_modes))
-    {
-      MetaMonitorMode *best_mode = NULL;
-      GList *l;
-
-      for (l = tiled_modes; l; l = l->next)
     {
       MetaMonitorMode *mode = l->data;
 
+      tiled_modes = g_list_remove_link (tiled_modes, l);
+
+      if (!meta_monitor_add_mode (monitor, mode))
+        {
+          meta_monitor_mode_free (mode);
+          continue;
+        }
+
+      if (!monitor_priv->preferred_mode)
+        {
           if (!best_mode ||
               mode->spec.refresh_rate > best_mode->spec.refresh_rate)
             best_mode = mode;
         }
-
-      monitor_priv->preferred_mode = best_mode;
     }
 
-  monitor_priv->modes = g_list_concat (monitor_priv->modes, tiled_modes);
+  if (best_mode)
+    monitor_priv->preferred_mode = best_mode;
 }
 
 static MetaMonitorMode *
@@ -789,6 +838,7 @@ create_untiled_monitor_mode (MetaMonitorTiled *monitor_tiled,
     .height = crtc_mode->height,
     .refresh_rate = crtc_mode->refresh_rate
   };
+  mode->parent.id = generate_mode_id (&mode->parent.spec);
   mode->parent.crtc_modes = g_new0 (MetaMonitorCrtcMode,
                              g_list_length (monitor_priv->outputs));
 
@@ -888,7 +938,11 @@ generate_untiled_monitor_modes (MetaMonitorTiled *monitor_tiled)
       if (!mode)
         continue;
 
-      monitor_priv->modes = g_list_append (monitor_priv->modes, mode);
+      if (!meta_monitor_add_mode (monitor, mode))
+        {
+          meta_monitor_mode_free (mode);
+          continue;
+        }
 
       if (is_monitor_mode_assigned (monitor, mode))
         {
@@ -1130,6 +1184,7 @@ meta_monitor_tiled_class_init (MetaMonitorTiledClass *klass)
 static void
 meta_monitor_mode_free (MetaMonitorMode *monitor_mode)
 {
+  g_free (monitor_mode->id);
   g_free (monitor_mode->crtc_modes);
   g_free (monitor_mode);
 }
@@ -1153,6 +1208,15 @@ meta_monitor_get_logical_monitor (MetaMonitor *monitor)
     return NULL;
 }
 
+MetaMonitorMode *
+meta_monitor_get_mode_from_id (MetaMonitor *monitor,
+                               const char  *monitor_mode_id)
+{
+  MetaMonitorPrivate *priv = meta_monitor_get_instance_private (monitor);
+
+  return g_hash_table_lookup (priv->mode_ids, monitor_mode_id);
+}
+
 static gboolean
 meta_monitor_mode_spec_equals (MetaMonitorModeSpec *monitor_mode_spec,
                                MetaMonitorModeSpec *other_monitor_mode_spec)
@@ -1458,6 +1522,12 @@ meta_monitor_mode_get_spec (MetaMonitorMode *monitor_mode)
   return &monitor_mode->spec;
 }
 
+const char *
+meta_monitor_mode_get_id (MetaMonitorMode *monitor_mode)
+{
+  return monitor_mode->id;
+}
+
 void
 meta_monitor_mode_get_resolution (MetaMonitorMode *monitor_mode,
                                   int             *width,

src/backends/meta-monitor.h

@@ -145,6 +145,9 @@ gboolean meta_monitor_get_suggested_position (MetaMonitor *monitor,
 
 MetaLogicalMonitor * meta_monitor_get_logical_monitor (MetaMonitor *monitor);
 
+MetaMonitorMode * meta_monitor_get_mode_from_id (MetaMonitor *monitor,
+                                                 const char  *monitor_mode_id);
+
 MetaMonitorMode * meta_monitor_get_mode_from_spec (MetaMonitor         *monitor,
                                                    MetaMonitorModeSpec *monitor_mode_spec);
 
@@ -174,6 +177,8 @@ float * meta_monitor_calculate_supported_scales (MetaMonitor                *mon
                                                  MetaMonitorScalesConstraint constraints,
                                                  int                        *n_supported_scales);
 
+const char * meta_monitor_mode_get_id (MetaMonitorMode *monitor_mode);
+
 MetaMonitorModeSpec * meta_monitor_mode_get_spec (MetaMonitorMode *monitor_mode);
 
 void meta_monitor_mode_get_resolution (MetaMonitorMode *monitor_mode,

src/org.gnome.Mutter.DisplayConfig.xml

@@ -315,7 +315,8 @@
 	* s vendor: vendor name
 	* s product: product name
 	* s serial: product serial
-	* a(iiddadu) modes: available modes
+	* a(siiddad) modes: available modes
+	    * s id: mode ID
 	    * i width: width in physical pixels
 	    * i height: height in physical pixels
 	    * d refresh rate: refresh rate
@@ -404,7 +405,7 @@
     -->
     <method name="GetCurrentState">
       <arg name="serial" direction="out" type="u" />
-      <arg name="monitors" direction="out" type="a((ssss)a(iiddadu)a{sv})" />
+      <arg name="monitors" direction="out" type="a((ssss)a(siiddadu)a{sv})" />
       <arg name="logical_monitors" direction="out" type="a(iiduba(ssss)a{sv})" />
       <arg name="properties" direction="out" type="a{sv}" />
     </method>
@@ -431,10 +432,9 @@
 	* d: scale
 	* u: transform (see GetCurrentState)
 	* b primary: true if this is the primary logical monitor
-	* a(siida{sv}): a list of monitors, each consisting of:
+	* a(ssa{sv}): a list of monitors, each consisting of:
 	    * s: connector
-	    * (iid): monitor mode spec (resolution width, resolution height,
+	    * s: monitor mode ID
-		     refresh rate)
 	    * a{sv}: monitor properties, including:
 	        - "enable_underscanning" (b): enable monitor underscanning;
 					      may only be set when underscanning
@@ -450,7 +450,7 @@
     <method name="ApplyMonitorsConfig">
       <arg name="serial" direction="in" type="u" />
       <arg name="method" direction="in" type="u" />
-      <arg name="logical_monitors" direction="in" type="a(iiduba(s(iid)a{sv}))" />
+      <arg name="logical_monitors" direction="in" type="a(iiduba(ssa{sv}))" />
       <arg name="properties" direction="in" type="a{sv}" />
     </method>
   </interface>