The big merge landed, so only the actual KMS backend is missing. Attaching it here.

Created attachment 253359 [details] [review]
MonitorManager: add a KMS backend

Using the new Cogl API to actually modeset (because we can't
use the DRM API directly without controlling buffer swap), we
can finally have a KMS monitor backend, which means full display
configuration when running on bare metal.

This doesn't apply anymore - can we get it rebased (or better still - merged) ?

Created attachment 254645 [details] [review]
MonitorManager: add a KMS backend

Using the new Cogl API to actually modeset (because we can't
use the DRM API directly without controlling buffer swap), we
can finally have a KMS monitor backend, which means full display
configuration when running on bare metal.

thanks, with this patch, the shell fills my monitor !

Review of attachment 254645 [details] [review]:

Good in general structure (I'm not happy about how we're dealing with failure, but that probably can't be fixed at the moment) - some memory leaks, some questions about details.

::: src/core/monitor-kms.c
@@ +56,3 @@
+  uint32_t edid_blob_id;
+  uint32_t has_dpms_prop : 1;
+  uint32_t has_edid_blob : 1;

DRM object ID's are never zero, so you can use that as a flag value rather than a separate boolean flag.

(Reference: in drm_mode_object_get() in the kernel sources, it gets the ID as:

        ret = idr_get_new_above(&dev->mode_config.crtc_idr, obj, 1, &new_id);

Where 1 is the starting ID.)

@@ +118,3 @@
+
+  for (i = 0; i < output_kms->n_encoders; i++)
+    drmModeFreeEncoder (output_kms->encoders[i]);

You leak the actual encoders block

@@ +133,3 @@
+                gconstpointer two)
+{
+  return memcmp (one, two, sizeof (drmModeModeInfo)) == 0;

This doesn't actually work - the mode info structures generally come originally from  drm_crtc_convert_to_umode() in the kernel sources which does:

   strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
   out->name[DRM_DISPLAY_MODE_LEN-1] = 0;

So the bytes in the name field passed the terminating NULL are undefined. If we have modes named '1920x1280i' and the name field '800x600' the name field for the second might end up as '800x600\0i\0\0\0' or something like that, and there's no guarantee that the junk will be the same when we get the mode for a connector and when we get the current mode for an output.

(The approach is also generally not robust against padding bytes, though I think the current structure doesn't have any padding bytes assuming normal field alignments.)

There's no shortcut, I think other than just comparing field-by-field. (Which then breaks if drmModeInfo grows more fields.)

@@ +146,3 @@
+  hash ^= mode->vdisplay ^ mode->vsync_start ^ mode->vsync_end;
+  hash ^= mode->vrefresh;
+  hash ^= mode->flags ^ mode->type;

Either include the name here or put a comment like:

 /* name is built from other field values so doesn't need to be hashed separately */

(which I think is the case for DRM currently - it's always [width]x[height] or [width]x[height]i)

@@ +170,3 @@
+          output_kms->has_dpms_prop = TRUE;
+          drmModeFreeProperty(prop);
+          break;

The break here is surprising - does an output only have one of DPMS or EDID?

@@ +203,3 @@
+    }
+
+  if (edid_blob->length > 0 && edid_blob->length % 128 == 0)

I think this check (or a >= 128) should be where we call the edid parser - checking one random aspect of validity here doesn't make sense to me.

@@ +209,3 @@
+    drmModeFreePropertyBlob (edid_blob);
+
+  return NULL;

This is odd that you have one line in the 'else' and one after it

@@ +233,3 @@
+
+  manager_kms->n_connectors = resources->count_connectors;
+  manager_kms->connectors = g_new (drmModeConnector *, manager_kms->n_connectors);

Don't you have to clean up the previous ->connectors (etc)?

@@ +354,3 @@
+            meta_output->subpixel_order = COGL_SUBPIXEL_ORDER_NONE;
+          else
+            meta_output->subpixel_order = connector->subpixel;

Just handle all 6 values in a switch and don't count on 4 of them coincidentally having the same numeric value.

@@ +362,3 @@
+	      for (k = 0; k < manager->n_modes; k++)
+		{
+		  if (drm_mode_equal (&connector->modes[j], manager->modes[k].driver_private))

Add a helper function for this.

@@ +375,3 @@
+          output_kms->encoders = g_new0 (drmModeEncoderPtr, output_kms->n_encoders);
+
+          crtc_mask = 0x7F;

0x7F doesn't really say "all bits set" ~(unsigned int)0 perhaps

I spent quite a bit of trying to figure out if and'ing the crtc mask was correct rather than or'ing it - and it's just not clear - there are no examples I could find in any of the drivers of an output with multiple encoders where the different encoders target different crtcs, and the internals of the kernel DRM modules don't seem to be set up to handle the case where the selection of encoder depends on what CRTC is passed into the modeset operation. The encoder selection seems to be more based on things like whether an analog or digital monitor is plugged into a DVI port.

If you have any insight into this a comment would be good - otherwise I'd say something that's just basic and explanatory like:

 /* We only list CRTC's for a connector which are supported by all encoders */

@@ +462,3 @@
+
+  manager->n_outputs = n_actual_outputs;
+  manager->outputs = g_renew (MetaOutput, manager->outputs, manager->n_outputs);

it seems unnecessary to save 4 or 8 bytes for each connector I have on my system without a monitor plugged into it ;-)

@@ +471,3 @@
+
+  /* XXX: intel hardware doesn't usually have clones, but we only have intel
+     cards, so this code was never tested! */

Who is "we" here?

In order to get this review out in a timely fashion, I'm skipping the review of the code below rather than trying to understand the concepts here.

I'd suggest asking one of the graphics people on our team to take a look at some point - maybe glisse since Radeon seems to be a complex example - and remove the XXX once that happens.

@@ +646,3 @@
+        {
+          MetaMonitorMode *mode;
+          uint32_t *outputs;

Shadows a function parameter. (Probably clearer to call this connectors, n_connectors anyways, since we're putting DRM connector IDs into it.)

@@ +660,3 @@
+          for (j = 0; j < n_outputs; j++)
+            {
+              MetaOutput *output = ((MetaOutput**)crtc_info->outputs->pdata)[j];

It's C, rely on the implicit cast from void *.

 MetaOutput *output = g_ptr_array_index (crtc_info->outputs, i);

@@ +694,3 @@
+    }
+
+  /* Disable CRTCs not mentioned in the list */

This comment actively confused me - this loop does some stuff for all monitors.

@@ +702,3 @@
+      crtc->logical_monitor = NULL;
+
+      if (crtc->is_dirty)

Having the is_dirty flag embedded in the structure but only used temporarily for the space of loops is bad practice - it's a little bit of efficiency for a lot of confusion. Don't need to fix it in this patch since it's done like this across all backends, but wanted to mention that I spent quite a bit of time trying to figure out what a "dirty crtc" was.

@@ +738,3 @@
+    {
+      meta_warning ("Applying display configuration failed: %s\n", error->message);
+      g_error_free (error);

I don't understand the idea that it fails without any indication to the caller. The failure (without atomic modeset) leaves the system in some completely undefined state, so the caller needs to either:

 * Read the current state again
 * Reset back to some old state

Is the idea that there is always some sort of "Does this look OK" dialog? Don't we want to cancel that dialog immediately if there is a failure?

Also, by skipping the rest of this function, we've really left the data we keep in an entirely inconsistent state - not only is it inconsistent with the state of the display, it's inconsistent with itself.

(I understand that the actual modesetting is async and it doesn't fail until later - but I still don't understand the concept of how this is used.)

@@ +748,3 @@
+
+      output->is_primary = output_info->is_primary;
+      output->is_presentation = output_info->is_presentation;

Is it a problem that read_current overwrites this?

@@ +833,3 @@
+    drmModeFreeEncoder (manager_kms->encoders[i]);
+  for (i = 0; i < manager_kms->n_connectors; i++)
+    drmModeFreeConnector (manager_kms->connectors[i]);

You leak the actual arrays

Review of attachment 254645 [details] [review]:

Good in general structure (I'm not happy about how we're dealing with failure, but that probably can't be fixed at the moment) - some memory leaks, some questions about details.

::: src/core/monitor-kms.c
@@ +56,3 @@
+  uint32_t edid_blob_id;
+  uint32_t has_dpms_prop : 1;
+  uint32_t has_edid_blob : 1;

DRM object ID's are never zero, so you can use that as a flag value rather than a separate boolean flag.

(Reference: in drm_mode_object_get() in the kernel sources, it gets the ID as:

        ret = idr_get_new_above(&dev->mode_config.crtc_idr, obj, 1, &new_id);

Where 1 is the starting ID.)

@@ +118,3 @@
+
+  for (i = 0; i < output_kms->n_encoders; i++)
+    drmModeFreeEncoder (output_kms->encoders[i]);

You leak the actual encoders block

@@ +133,3 @@
+                gconstpointer two)
+{
+  return memcmp (one, two, sizeof (drmModeModeInfo)) == 0;

This doesn't actually work - the mode info structures generally come originally from  drm_crtc_convert_to_umode() in the kernel sources which does:

   strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
   out->name[DRM_DISPLAY_MODE_LEN-1] = 0;

So the bytes in the name field passed the terminating NULL are undefined. If we have modes named '1920x1280i' and the name field '800x600' the name field for the second might end up as '800x600\0i\0\0\0' or something like that, and there's no guarantee that the junk will be the same when we get the mode for a connector and when we get the current mode for an output.

(The approach is also generally not robust against padding bytes, though I think the current structure doesn't have any padding bytes assuming normal field alignments.)

There's no shortcut, I think other than just comparing field-by-field. (Which then breaks if drmModeInfo grows more fields.)

@@ +146,3 @@
+  hash ^= mode->vdisplay ^ mode->vsync_start ^ mode->vsync_end;
+  hash ^= mode->vrefresh;
+  hash ^= mode->flags ^ mode->type;

Either include the name here or put a comment like:

 /* name is built from other field values so doesn't need to be hashed separately */

(which I think is the case for DRM currently - it's always [width]x[height] or [width]x[height]i)

@@ +170,3 @@
+          output_kms->has_dpms_prop = TRUE;
+          drmModeFreeProperty(prop);
+          break;

The break here is surprising - does an output only have one of DPMS or EDID?

@@ +203,3 @@
+    }
+
+  if (edid_blob->length > 0 && edid_blob->length % 128 == 0)

I think this check (or a >= 128) should be where we call the edid parser - checking one random aspect of validity here doesn't make sense to me.

@@ +209,3 @@
+    drmModeFreePropertyBlob (edid_blob);
+
+  return NULL;

This is odd that you have one line in the 'else' and one after it

@@ +233,3 @@
+
+  manager_kms->n_connectors = resources->count_connectors;
+  manager_kms->connectors = g_new (drmModeConnector *, manager_kms->n_connectors);

Don't you have to clean up the previous ->connectors (etc)?

@@ +354,3 @@
+            meta_output->subpixel_order = COGL_SUBPIXEL_ORDER_NONE;
+          else
+            meta_output->subpixel_order = connector->subpixel;

Just handle all 6 values in a switch and don't count on 4 of them coincidentally having the same numeric value.

@@ +362,3 @@
+	      for (k = 0; k < manager->n_modes; k++)
+		{
+		  if (drm_mode_equal (&connector->modes[j], manager->modes[k].driver_private))

Add a helper function for this.

@@ +375,3 @@
+          output_kms->encoders = g_new0 (drmModeEncoderPtr, output_kms->n_encoders);
+
+          crtc_mask = 0x7F;

0x7F doesn't really say "all bits set" ~(unsigned int)0 perhaps

I spent quite a bit of trying to figure out if and'ing the crtc mask was correct rather than or'ing it - and it's just not clear - there are no examples I could find in any of the drivers of an output with multiple encoders where the different encoders target different crtcs, and the internals of the kernel DRM modules don't seem to be set up to handle the case where the selection of encoder depends on what CRTC is passed into the modeset operation. The encoder selection seems to be more based on things like whether an analog or digital monitor is plugged into a DVI port.

If you have any insight into this a comment would be good - otherwise I'd say something that's just basic and explanatory like:

 /* We only list CRTC's for a connector which are supported by all encoders */

@@ +462,3 @@
+
+  manager->n_outputs = n_actual_outputs;
+  manager->outputs = g_renew (MetaOutput, manager->outputs, manager->n_outputs);

it seems unnecessary to save 4 or 8 bytes for each connector I have on my system without a monitor plugged into it ;-)

@@ +471,3 @@
+
+  /* XXX: intel hardware doesn't usually have clones, but we only have intel
+     cards, so this code was never tested! */

Who is "we" here?

In order to get this review out in a timely fashion, I'm skipping the review of the code below rather than trying to understand the concepts here.

I'd suggest asking one of the graphics people on our team to take a look at some point - maybe glisse since Radeon seems to be a complex example - and remove the XXX once that happens.

@@ +646,3 @@
+        {
+          MetaMonitorMode *mode;
+          uint32_t *outputs;

Shadows a function parameter. (Probably clearer to call this connectors, n_connectors anyways, since we're putting DRM connector IDs into it.)

@@ +660,3 @@
+          for (j = 0; j < n_outputs; j++)
+            {
+              MetaOutput *output = ((MetaOutput**)crtc_info->outputs->pdata)[j];

It's C, rely on the implicit cast from void *.

 MetaOutput *output = g_ptr_array_index (crtc_info->outputs, i);

@@ +694,3 @@
+    }
+
+  /* Disable CRTCs not mentioned in the list */

This comment actively confused me - this loop does some stuff for all monitors.

@@ +702,3 @@
+      crtc->logical_monitor = NULL;
+
+      if (crtc->is_dirty)

Having the is_dirty flag embedded in the structure but only used temporarily for the space of loops is bad practice - it's a little bit of efficiency for a lot of confusion. Don't need to fix it in this patch since it's done like this across all backends, but wanted to mention that I spent quite a bit of time trying to figure out what a "dirty crtc" was.

@@ +738,3 @@
+    {
+      meta_warning ("Applying display configuration failed: %s\n", error->message);
+      g_error_free (error);

I don't understand the idea that it fails without any indication to the caller. The failure (without atomic modeset) leaves the system in some completely undefined state, so the caller needs to either:

 * Read the current state again
 * Reset back to some old state

Is the idea that there is always some sort of "Does this look OK" dialog? Don't we want to cancel that dialog immediately if there is a failure?

Also, by skipping the rest of this function, we've really left the data we keep in an entirely inconsistent state - not only is it inconsistent with the state of the display, it's inconsistent with itself.

(I understand that the actual modesetting is async and it doesn't fail until later - but I still don't understand the concept of how this is used.)

@@ +748,3 @@
+
+      output->is_primary = output_info->is_primary;
+      output->is_presentation = output_info->is_presentation;

Is it a problem that read_current overwrites this?

@@ +833,3 @@
+    drmModeFreeEncoder (manager_kms->encoders[i]);
+  for (i = 0; i < manager_kms->n_connectors; i++)
+    drmModeFreeConnector (manager_kms->connectors[i]);

You leak the actual arrays

(In reply to comment #7)
> Review of attachment 254645 [details] [review]:
> 
> @@ +375,3 @@
> +          output_kms->encoders = g_new0 (drmModeEncoderPtr,
> output_kms->n_encoders);
> +
> +          crtc_mask = 0x7F;
> 
> 0x7F doesn't really say "all bits set" ~(unsigned int)0 perhaps
> 
> I spent quite a bit of trying to figure out if and'ing the crtc mask was
> correct rather than or'ing it - and it's just not clear - there are no examples
> I could find in any of the drivers of an output with multiple encoders where
> the different encoders target different crtcs, and the internals of the kernel
> DRM modules don't seem to be set up to handle the case where the selection of
> encoder depends on what CRTC is passed into the modeset operation. The encoder
> selection seems to be more based on things like whether an analog or digital
> monitor is plugged into a DVI port.
> 
> If you have any insight into this a comment would be good - otherwise I'd say
> something that's just basic and explanatory like:
> 
>  /* We only list CRTC's for a connector which are supported by all encoders */

I don't really know what encoders are supposed to be, so I just implemented what xf86-video-modesetting does.

Btw, on a further look at the kernel code, it appears that the encoder is a function of the monitor attached to the connector, and pretty much changes only on hotplug (the crtc helper calls best_encoder(), which in almost all drivers returns a cached encoder associated with the connector at probe time), so we could just use connector->encoder_id, and get the CRTC mask from there, except I'm not sure it is set if the connector is disabled. In any case, and-ing the mask seems the safest thing to do.

> @@ +462,3 @@
> +
> +  manager->n_outputs = n_actual_outputs;
> +  manager->outputs = g_renew (MetaOutput, manager->outputs,
> manager->n_outputs);
> 
> it seems unnecessary to save 4 or 8 bytes for each connector I have on my
> system without a monitor plugged into it ;-)

One MetaOutput is 144 bytes on x86_64, and you can easily have 8-10 connectors.

> @@ +738,3 @@
> +    {
> +      meta_warning ("Applying display configuration failed: %s\n",
> error->message);
> +      g_error_free (error);
> 
> I don't understand the idea that it fails without any indication to the caller.
> The failure (without atomic modeset) leaves the system in some completely
> undefined state, so the caller needs to either:
>
>  * Read the current state again
>  * Reset back to some old state
> 
> Is the idea that there is always some sort of "Does this look OK" dialog? Don't
> we want to cancel that dialog immediately if there is a failure?
> 
> Also, by skipping the rest of this function, we've really left the data we keep
> in an entirely inconsistent state - not only is it inconsistent with the state
> of the display, it's inconsistent with itself.
> 
> (I understand that the actual modesetting is async and it doesn't fail until
> later - but I still don't understand the concept of how this is used.)

Well, the thing is, to fail at this point is pretty much a programmer error, because we're calling into the kernel yet, so the only checks cogl does are checks that mutter does at a higher layer too.

> @@ +748,3 @@
> +
> +      output->is_primary = output_info->is_primary;
> +      output->is_presentation = output_info->is_presentation;
> 
> Is it a problem that read_current overwrites this?

Yes. But we don't do hotplug yet, so read_current() is called only once.

> @@ +833,3 @@
> +    drmModeFreeEncoder (manager_kms->encoders[i]);
> +  for (i = 0; i < manager_kms->n_connectors; i++)
> +    drmModeFreeConnector (manager_kms->connectors[i]);
> 
> You leak the actual arrays

No, they're freed immediately after.

Created attachment 254842 [details] [review]
MonitorManager: add a KMS backend

Using the new Cogl API to actually modeset (because we can't
use the DRM API directly without controlling buffer swap), we
can finally have a KMS monitor backend, which means full display
configuration when running on bare metal.

Review of attachment 254842 [details] [review]:

I still think it's conceptually broken that read_current unsets any stored is_primary/is_presentation. In general, we can assume that the DRM ids of connectors will stay consistent across hotplug, reprobing, etc, so we can simply store the connector IDs of the primary, presentation outputs to repeat the association.

In terms of modesetting failure - the fact that we don't handle cogl returning immediately with a failure code well (as you say, cogl does most of the work later anyways), then with the fact that we don't have a framework for handling modeset failure at all - I don't think we can necessarily convey all restrictions for all cards in such a way that we'll never get a configuration that the kernel can't happen. But let's leave it like this for the moment - anything else would require callbacks from Cogl, and probably isn't worth doing until we have a concrete error case we're trying to handle more slickly.

Marking needs-work for the primary/presentation thing since I think that should be a simple thing.

::: src/core/monitor-kms.c
@@ +633,3 @@
+      output_kms = meta_output->driver_private;
+
+      if (output_kms->dpms_prop_id > 0)

I dislike using > 0 to mean != 0

Created attachment 254924 [details] [review]
MonitorManager: add a KMS backend

Using the new Cogl API to actually modeset (because we can't
use the DRM API directly without controlling buffer swap), we
can finally have a KMS monitor backend, which means full display
configuration when running on bare metal.

Created attachment 254925 [details] [review]
MonitorKms: add hotplug support

Use GUdev to listen to the kernel events which are emitted when
monitors are hotplugged and hotunplugged.

Now that read_current() doesn't lose information, hotplugging
becomes trivial.
UNTESTED, I need to go the office and try there.

Review of attachment 254924 [details] [review]:

Looks good

Comment on attachment 254924 [details] [review]
MonitorManager: add a KMS backend

Attachment 254924 [details] pushed as 65db8ef - MonitorManager: add a KMS backend

Review of attachment 254925 [details] [review]:

This looks plausible to me - definitely test before landing.

(I don't entirely like the fact that monitor-xrandr and monitor-kms directly call into their vfuncs and duplicate the code for saving and freeing the outputs/modes/crtcs from the parent class. Not a blocker for landing this.)

After a minor fix, this crashes in Cogl, because gbm_surface_lock_from_buffer() returns NULL. Bug 708141.

(In reply to comment #15)
> Review of attachment 254925 [details] [review]:
> 
> This looks plausible to me - definitely test before landing.


FWIW:

+ Trace 232490

Is what I get here.

Review of attachment 254925 [details] [review]:

Segfaults in testing.

Review of attachment 254925 [details] [review]:

::: src/core/monitor-kms.c
@@ +952,3 @@
   CoglDisplay *cogl_display;
   CoglRenderer *cogl_renderer;
+  static const char *subsystems = { "drm/drm_minor", NULL };

The type here should be "static const gchar * const subsystems[]"

this is not relevant anymore