It can also break if you have the 'vgem' virtual device, which will often land in card0 before any real hardware has been probed. That could at least be trivially eliminated by checking if the device is KMS at all, and skipping it if not.

some discussion in bug 753531 comment 54 about this.

(In reply to Ray Strode [halfline] from comment #2)
> some discussion in bug 753531 comment 54 about this.

Just yesterday before going home I wrote a patch for this bug. I'll check the discussion and possibly fix the patch and send it here. Thanks

Created attachment 310397 [details] [review]
Use udev to find the master drm device

Review of attachment 310397 [details] [review]:

Hey this is great, thanks for working on this.
Ultimately, we'll want to use egl apis directly for device enumeration, but those apis aren't in yet, so I think this makes sense interim.

Your commit message should say "Find the device that has the boot_vga flag set"

::: src/backends/native/meta-launcher.c
@@ +265,3 @@
 
+/* get path to devnode with boot_vga flag set
+ * or fall back to /dev/dri/card0 if no such device

why are you we falling back?

@@ +269,3 @@
+ * or length of the written string (in snprintf way) */
+static gint
+get_primary_gpu_path(gchar *path, size_t maxlen)

parameters should be on separate lines.
I think it would be more idiomatic of the mutter codebase to return allocated memory instead of writing into a static buffer.
(also it's only run once, so the cost of the allocation is negligible)

@@ +272,3 @@
+{
+  const gchar *subsystems[] = {"drm", NULL};
+  GList *devs, *tmp;

i'd call devs "device_matches" and "tmp" "node" but that's just my style so whatever.

@@ +273,3 @@
+  const gchar *subsystems[] = {"drm", NULL};
+  GList *devs, *tmp;
+  GUdevDevice *pci;

this declaration should go in the loop. i'd call it pci_device

@@ +280,3 @@
+
+  g_udev_enumerator_add_match_name (enumerator, "card*");
+  g_udev_enumerator_add_match_tag (enumerator, "seat");

so you make sure the card has a seat tag here, which is good, but you don't make sure ID_SEAT matches the current seat below. (though ID_SEAT=seat0 is optional so the code should handle ID_SEAT missing for seat0)

@@ +284,3 @@
+  devs = g_udev_enumerator_execute (enumerator);
+  if (!devs)
+    goto out;

so you don't fallback actually you goto out. i think that's right.

@@ +288,3 @@
+  tmp = devs;
+  while (tmp)
+    {

maybe make this a for loop so you don't have tmp = g_list_next (tmp) repeated 3 times. doing so will futureproof the code against inadvertent infinite loops.

also mutter tends to do tmp = tmp->next not use g_list_next

another thing, for clarity assigning tmp->data to temporary variable might make sense (i'd personally call it card_device).

@@ +289,3 @@
+  while (tmp)
+    {
+      if (!g_udev_device_get_device_file (tmp->data))

this should probably have comment like "filter out connector devices like card0-SVIDEO-1" or something.

doing g_udev_device_get_device_type (device) == G_UDEV_DEVICE_TYPE_CHAR would be more specific and then you wouldn't be getting the name and not doing anything with it.

@@ +302,3 @@
+        }
+
+      /* get value of boot_vga attribute or 0 if the device has no boot_vga */

this comment should say "check if the device was used for output at boot time"

@@ +315,3 @@
+  /* if no device satisfy, fall back to /dev/dri/card0 */
+  if (!tmp)
+    retval = snprintf (path, maxlen, "/dev/dri/card0");

i don't think we should do this.

@@ +345,3 @@
+    }
+  else if (retval >= (int) sizeof path)
+    g_error ("buffer too short for the path");

yea that g_error is ugly, just just g_strdup() on the result.

> @@ +280,3 @@
> +
> +  g_udev_enumerator_add_match_name (enumerator, "card*");
> +  g_udev_enumerator_add_match_tag (enumerator, "seat");
> 
> so you make sure the card has a seat tag here, which is good, but you don't
> make sure ID_SEAT matches the current seat below. (though ID_SEAT=seat0 is
> optional so the code should handle ID_SEAT missing for seat0)

How can I get the current seat id from Login1Seat (or any other way)? meta-wayland-seat.c hardcodes seat0 to wl_seat_send_name(), but I suppose we don't want that here.

> maybe make this a for loop so you don't have tmp = g_list_next (tmp)
> repeated 3 times. doing so will futureproof the code against inadvertent
> infinite loops

Yeah, that sounds right

well, you can query it from the Login1Seat proxy using the "id" property, but I wouldn't do that. The easiest way is probably the XDG_SEAT environment variable. The slightly more robust way is to query the session with:

sd_pid_get_session(0, &session);

and then get the seat associated with the session with 

sd_session_get_seat (session, &seat);

Doing it that way is nicer than using the bus, since it reads the information straight from the logind database instead of having logind read it from the database and then having ipc in between. 

Though, thinking about it the bus proxy may already have the "id" property cached as long as it was started without G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES so maybe it would be fine to use it after all.

(In reply to Ray Strode [halfline] from comment #7)
> well, you can query it from the Login1Seat proxy using the "id" property,
> but I wouldn't do that. The easiest way is probably the XDG_SEAT environment
> variable. The slightly more robust way is to query the session with:
> 
> sd_pid_get_session(0, &session);
> 
> and then get the seat associated with the session with 
> 
> sd_session_get_seat (session, &seat);
> 
> Doing it that way is nicer than using the bus, since it reads the
> information straight from the logind database instead of having logind read
> it from the database and then having ipc in between. 
> 
> Though, thinking about it the bus proxy may already have the "id" property
> cached as long as it was started without
> G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES so maybe it would be fine to use
> it after all.

Trying to get the seat id from cache did not work, so I used the sd_* functions. When I was trying to get the cached information, I needed to refactor meta_launcher_new to get the seat proxy before calling get_kms_fd. Although I don't need it with sd_.* functions, I changed the patch a little bit and attaching it too.

Created attachment 310609 [details] [review]
fix handling errors in meta_launcher_new

Created attachment 310610 [details] [review]
Find device that has boot_vga flag set

Review of attachment 310609 [details] [review]:

Thanks, this is a good clean up.

::: src/backends/native/meta-backend-native.c
@@ -329,3 @@
   MetaBackendNativePrivate *priv = meta_backend_native_get_instance_private (native);
 
-  /* We're a display server, so start talking to weston-launch. */

This should probably be a separate commit, since it doesn't have anything to do with error handling.

@@ +331,3 @@
   priv->launcher = meta_launcher_new ();
+  if (!priv->launcher)
+    g_error ("Failed initializing the backend");

This doesn't give any indication on why the failure happened.  We did get indication from within the function, though, as g_warnings.

I think either, meta_launcher_new should take a GError * and this code should print why it's failing, or the g_warnings in meta_launcher_new() should get turned into g_errors' and this check should get dropped.

::: src/backends/native/meta-launcher.c
@@ +299,3 @@
   int kms_fd;
 
   session_proxy = get_session_proxy (NULL);

get_session_proxy should propagate its error.

@@ +307,3 @@
       g_warning ("Could not take control: %s", error->message);
       g_error_free (error);
+      goto out_session;

mutter doesn't normally have multiple out labels in one function.
A more typical approach would be to initialize each variable to NULL and do

g_clear_object (&session_proxy);
g_clear_object (&seat_proxy);

after a lone out: label. of course that's just a subjective style issue, so whatever.

@@ +310,3 @@
     }
 
+  seat_proxy = get_seat_proxy (NULL);

get_seat_proxy should propagate its error

Review of attachment 310610 [details] [review]:

looking good. just a couple minor things

::: src/backends/native/meta-launcher.c
@@ +57,2 @@
 static Login1Session *
+get_session_proxy (char *session_id, GCancellable *cancellable)

parameters should be on separate lines, and session_id should be const char *.

Though in this case, I don't think you need to pass session_id in. just change the
object path below to "/org/freedesktop/login1/session/self" then session_id won't be needed
at all (which will also prevent the need to escape the object path below)

@@ +286,3 @@
+      const gchar *device_seat;
+
+      /* filter out devices that are not character device */

maybe add an example, e.g., ", like card0-DVI-I-1"

@@ +307,3 @@
+      g_object_unref (pci_device);
+
+      if (boot_vga == 1)

this should probably be if (boot_vga == 1 || g_strcmp0 (seat_name, "seat0") != 0) (or something like that). If hardware is explicitly assigned to a non-seat0 seat then we'll want to use it, even though it wasn't used at boot time.

@@ +309,3 @@
+      if (boot_vga == 1)
+        {
+          /* found the boot_vga device */

and if that if statement is changed then this comment needs to be updated

@@ +326,3 @@
 static gboolean
 get_kms_fd (Login1Session *session_proxy,
+            gchar *seat_id,

this should be const gchar *

@@ +372,3 @@
   int kms_fd;
 
+  if (sd_pid_get_session (getpid (), &session_id) < 0)

you can use 0 here instead of getpid (), but getpid() is fine.  I guess it's more self documenting.

Review of attachment 310610 [details] [review]:

::: src/backends/native/meta-launcher.c
@@ +368,3 @@
   Login1Session *session_proxy;
   Login1Seat *seat_proxy;
+  char *session_id, *seat_id;

oh one other thing is these need to be freed with free() when you're done with them.

Created attachment 310779 [details] [review]
Refactor handling errors in meta-launcher

Created attachment 310780 [details] [review]
Find the right drm device

Created attachment 310781 [details] [review]
native: remove obsolete comment

Review of attachment 310779 [details] [review]:

hey thanks, i know the scope of your changes expanded a bit and I appreciate the effort.
Feel free to commit this. I have a few suggestions below you can take or leave first. up to you.

::: src/backends/native/meta-launcher.c
@@ +56,3 @@
+static void
+report_login1_error(const char *prefix,
+                    GError *error)

probably should be called report_error_and_die or something like that.  I mean there's nothing login1 specific about it other than you're only presently using it for logind related stuff.

@@ +63,3 @@
+    g_error ("%s: %s", prefix, error->message);
+  else
+    g_error (prefix);

So the GError doesn't get freed.  That's fine since g_error is fatal anyway, but I'd add a comment anyway, like:

/* At this point we should be dead, so we don't need to free the GError */

Coverity may also complain, but coverity is too hard to predict anyway, so let's cross that bridge when we come to it.

@@ -67,1 @@
 

So I know I told you to make this change, but it doesn't actually match the description in your commit message. I'd either split it out as a separate commit up front, or, at least, highlight it in the commit body.

Review of attachment 310780 [details] [review]:

Looks good. Feel free to commit after fixing upt he style issues.

::: src/backends/native/meta-launcher.c
@@ +277,3 @@
 
+static gchar *
+get_primary_gpu_path(const gchar *seat_name)

missing space here

@@ +293,3 @@
+    goto out;
+
+  for(tmp = devices; tmp != NULL; tmp = tmp->next)

missing space here

@@ +307,3 @@
+      if (!device_seat)
+        /* when ID_SEAT is not set, it means seat0 */
+        device_seat = "seat0";

should have braces here since you have braces in the else

@@ +316,3 @@
+        }
+
+      /* check if the device belongs to our seat */

should probably say "skip devices that don't belong to our seat". Your way isn't wrong, but strcmp semantics are confusing enough that someone quickly skimming the code might interpret your comment to mean to continue if the device belongs.

@@ +357,3 @@
+  path = get_primary_gpu_path(seat_id);
+  if (!path)
+    g_error ("could not get drm master device");

it doesn't actually get master until later, so calling it the "master device" is a little confusing. you could call it the "drm card device" or "drm modesetting device" or "drm kms device" maybe. also maybe "find" instead of "get" ?

@@ +380,3 @@
+  /* on error the seat_id will remain NULL */
+  sd_session_get_seat (session_id, &seat_id);
+  free(session_id);

missing space

@@ +400,3 @@
+  seat_id = get_seat_id ();
+  if (!seat_id)
+    g_error ("Failed getting seat id");

would be nice if we could report the error code here, but I guess not critical.

Review of attachment 310781 [details] [review]:

++

> So I know I told you to make this change, but it doesn't actually match the
> description in your commit message. I'd either split it out as a separate
> commit up front, or, at least, highlight it in the commit body.

Will split it to separate patch.

Regarding commiting the patches - I do not have permissions to do that, so it'd be great if you (or anybody) could commit it for me

Created attachment 310808 [details] [review]
Simplify getting session proxy

Created attachment 310809 [details] [review]
Refactor handling errors in meta-launcher

Created attachment 310810 [details] [review]
Find the right drm device

Comment on attachment 310781 [details] [review]
native: remove obsolete comment

Attachment 310781 [details] pushed as c13ddaf - native: remove obsolete comment

Comment on attachment 310808 [details] [review]
Simplify getting session proxy

Attachment 310808 [details] pushed as 8e22bf5

Attachment 310809 [details] pushed as 1845bfe
Attachment 310810 [details] pushed as 79f755b

(some git-bz screw up here, but should be set)

The commit 

commit 8e22bf5bc96a7d9ff1aba8ea8217a4c3ca06b4ce
Author: Marek Chalupa <mchqwerty@gmail.com>
Date:   Mon Sep 7 13:38:49 2015 +0200

    launcher: simplify getting session dbus proxy
    
    Use path "/org/freedesktop/login1/session/self" instead of
    getting session id and building the path manually
    
    https://bugzilla.gnome.org/show_bug.cgi?id=753434

breaks VT switching for me (running jhbuild on top of Fedora 22). Reverting it makes it work again.

Yeah. Revert it. It's an idea that doesn't work, since signals are sent to the specific session ID.

Ok. Pushed the revert to master, with the reason you mentioned in the commit message. Thanks.

thanks for tracking the bug down.  

technically we're in code freeze I think, but this is a serious enough issue that I doubt release-team would be against the revert. 

Still, can you shoot a mail to the release-team list to let them know?

Ah, right. I'll send a mail.

Sorry, I would have caught it, but I'm a little bit out of the loop for GNOME / mutter stuff lately.

*** Bug 756051 has been marked as a duplicate of this bug. ***