Do we really need to run this each time?

Jon: what could we do instead? How to handle people changing hardware, or updated drivers, etc. I'm totally willing to handle this another way :-)

I guess, as a workaround, we can change the timeout... It's still annoying, though, since it makes login much slower. Maybe we could have gdm run the test and pass the result to gnome-session via an environment variable?

we could just rely on the result from previous runs, and have a way in control-center to force a re-run, maybe.

Created attachment 181040 [details] [review]
Increase session runnable helper timeout

Half a second is *way* to short.

Comment on attachment 181040 [details] [review]
Increase session runnable helper timeout

30 seconds is a no-go. Keep in mind that this completely blocks the login, so a long delay is more broken than getting to the fallback mode.

I'm potentially willing to go up to 2s, though.

After a second or so, you probably want to start showing some 'still working...' animation, else users will start hitting the powerbutton...

Surely the system capabilities don't change between boot and session login, right?  Seems a bit odd to do this every login.  No one changes graphics hardware while the system is running and if they do then we should know that.

Probing hardware capabilities is something that would be better done at boot I think.  Eventually GDM will need to know this anyway.

numbers: we have 9 out of 71 testers at the Nouveau test day reporting this issue - https://fedoraproject.org/wiki/Test_Day:2011-02-22_Nouveau . So far with Radeon (happening today), 5 out of 30 testers report this issue.

Downstream bug - https://bugzilla.redhat.com/show_bug.cgi?id=679326 .

(In reply to comment #7)
> Surely the system capabilities don't change between boot and session login,
> right?

There can be more than one DISPLAY (in the X11 sense) on a box, more than one display (in the hardware sense), and the capabilities can vary widely.  For instance, there are "VGA attached via USB2.0" products (such as the eVGA brand UV Plus products, and others) which can stream HDTV but are very limited in some other ways.

But there should be nothing to preclude GDM from determining this session property well before "login:".

(In reply to comment #5)
> (From update of attachment 181040 [details] [review])
> 30 seconds is a no-go.

Under what failure conditions will it take the full 30 seconds though?  If we don't know of any, then it seems far better to be reliable here than to pick a different arbitrary and also way too short timeout.

Regardless of how long it may take gnome-session-is-accelerated to produce an answer, gdm can do things to avoid the longest delays.  Such as: pre-fetch gnome-session-is-accelerated and its executables and libraries (just read() them as files and discard the buffers), and do this in parallel with invoking gdm, before gdm actually invokes gnome-session-is-accelerated as a sub-task and waits for the result.  In the case of a LiveCD or LiveDVD with actual rotating optical media, the pre-fetch is notably effective, particularly because it overlaps the reading of gnome-session-is-accelerated with the time it takes the human to activate the "login:" mechanism (click a button or type <Enter>, or wait for a multi-second timeout to expire.)

I will work on a GDM patch.  But in the meantime, it makes no sense to leave the timeout in gnome-session as half of a second.

Colin: feel free to change it to 2s or 3s. From what I've read, this should be enough.

(In reply to comment #11)
> Such as: pre-fetch
> gnome-session-is-accelerated and its executables and libraries (just read()
> them as files and discard the buffers)
Isn't this what readahead is already doing?  It seems like either readahead needs to be fixed, or the assumption that this is where the bottleneck is, is wrong (in which case mesa needs to be profiled to figure out why it's so slow to load

(In reply to comment #13)
> Colin: feel free to change it to 2s or 3s. From what I've read, this should be
> enough.

Done; even 3 seconds still feels potentially too short to me if say you're using a multiseat system and have concurrent logins, or the equivalent of "yum update" happens to be running, or NFS root scenarios, etc.

Created attachment 182136 [details] [review]
Add gnome-session-check-accelerated which sets X property

It would make sense to run gnome-session-is-accelerated in
GDM, in parallel with the user is typing their password.  A
good way to pass that knowledge through to gnome-session
so it knows it can avoid repeating the same check is via
an X property.

So this new tool "gnome-session-check-accelerated" runs
"gnome-session-is-accelerated" and stores that status as
an X property.  gnome-session will then itself run it
again, but it should pick up the property and reuse that
value.

I agree with the principle, but why do you need to make this a separate tool ?
I guess this to handle cases where the gl-using process is crashing or otherwise going off into the weeds ?

(In reply to comment #17)
> I agree with the principle, but why do you need to make this a separate tool ?
> I guess this to handle cases where the gl-using process is crashing or
> otherwise going off into the weeds ?

Right, gnome-session-is-accelerated isn't in the gnome-session process for the reasons outlined in its source.

gnome-session-*check*-accelerated is a separate tool so it can be run by both gnome-session and gdm conveniently.

I'm assuming that using GTK+/GDK in the helper tool has no major impact on the execution time, but I've no idea about that. If it has an impact, it looks rather easy to do pure X.

One thing we might want to handle: if gnome-session (or gdm) kills gnome-session-check-accelerated because it takes too long, then we should make g-s-check-accelerated kill g-s-is-accelerated too.

Btw, exit_1_message() isn't used anywhere.

I also find the two names a bit confusing since it's unclear which one does what. Maybe we should rename g-s-is-accelerated to something else, like g-s-check-accelerated-gl.

Why two tools at all? Why not just have gnome-session-is-accelerated set the property in its exit path and exit before really starting if it's already set?

if we're worried about mesa hanging, we could fork a child or whatever.  it's just really bizarre to have two tiny programs both with essentially the same name

[Removing GNOME3.0 target as decided in release-team meeting on March 03, 2011.
This report has an "important" categorisation for GNOME3.0 but is not considered a hard blocker. For querying use the corresponding whiteboard entry added.]

(In reply to comment #20)
> Why two tools at all? Why not just have gnome-session-is-accelerated set the
> property in its exit path and exit before really starting if it's already set?

I already answered that above.  I'll attach a new patch which renames is-accelerated to check-accelerated-helper to make their relationship clearer from just the names (but I already wrote comments in the source).

Well, you wrote

> gnome-session-*check*-accelerated is a separate tool so it can be run by both
> gnome-session and gdm conveniently.

which is an argument for having a separate tool, but we already have a separate tool...  So, I followed up with, why two tools?

If the concern is the first tool is going to hang (is that the concern?), then why do you spawn it synchronously from the second tool?

(In reply to comment #24)
> Well, you wrote
> 
> > gnome-session-*check*-accelerated is a separate tool so it can be run by both
> > gnome-session and gdm conveniently.
> 
> which is an argument for having a separate tool, but we already have a separate
> tool...  So, I followed up with, why two tools?
> 
> If the concern is the first tool is going to hang (is that the concern?), then
> why do you spawn it synchronously from the second tool?

https://bugzilla.gnome.org/show_bug.cgi?id=641992#c18

"gnome-session-*check*-accelerated is a separate tool so it can be run by both
gnome-session and gdm conveniently."

Colin: I believe we should still handle the "process gets terminated" case I talked about in comment 19.

(To address Ray's concerns, we could also have all the code in the same process, and do a fork/exec before doing the gl code -- not sure it's worth it, though)

Created attachment 183183 [details] [review]
avoid double checking

Here is a patch that implements a strategy to avoid duplicating the gl checks when gnome-session spawns the runnablehelper before the one spawned by gdm is done:

Before doing the gl check, set the property to '2' to indicate 'test in progress'.
When we see a '2' upon initially inspecting the property, we lay low and wait for a propertynotify (for up to 5 seconds). When it arrives, we reread the property and return the newly cached value.

Ray has added an autostart file for gnome-session-check-accelerated to gdm now.

Created attachment 183245 [details] [review]
avoid double checking

Slightly improved patch.

1) it actually compiles

2) only wait once for a property change; if we don't get one, just go ahead and to the check ourselves

Review of attachment 183245 [details] [review]:

Please use git formatted commits; the commit message is extremely useful for reviewers, since it helps describe your intention.

::: tools/gnome-session-check-accelerated.c
@@ +82,3 @@
+        XSelectInput (GDK_DISPLAY_XDISPLAY (display), rootwin, PropertyChangeMask);
+        gdk_window_add_filter (root, filter, NULL);
+        return GDK_FILTER_CONTINUE;

Can you call it "on_property_notify_timeout"?

@@ +95,3 @@
         int estatus;
+#undef LIBEXECDIR
+#define LIBEXECDIR "."

Uhh...what?

@@ +133,3 @@
+
+                        }
+                                if (wait_for_property_notify ())

} else {

Created attachment 183306 [details] [review]
avoid double checking

Created attachment 183308 [details] [review]
avoid double checking

Comment on attachment 183308 [details] [review]
avoid double checking

Hrm, splinter doesn't work for this patch. So old-style review.

It looks good. I have a few suggestions for small changes below. Please commit!

>+static GdkFilterReturn
>+property_notify_filter (GdkXEvent *xevent,
>+                        GdkEvent  *event,
>+                        gpointer   data)
>+{
>+        XPropertyEvent *ev = xevent;
>+
>+        if (ev->type == PropertyNotify && ev->atom == is_accelerated_atom) {
>+                property_changed = TRUE;
>+                gtk_main_quit ();
>+        }

Should we also check that the property is changed to 0 or 1, or are we fine assuming that a change is always something valid anyway?

>+        XSelectInput (GDK_DISPLAY_XDISPLAY (display), rootwin, PropertyChangeMask);
>+        gdk_window_add_filter (root, property_notify_filter, NULL);

I'd probably pass property_changed as user_data here and avoid a global variable. But I'll leave this up to you.

>+        g_timeout_add (5000, on_property_notify_timeout, NULL);

Can we put a define at the top for 5000? That's the kind of value I prefer to easily find at the top of a file.

>+                        if (*is_accelerated_ptr == 2) {

Maybe also use a define for 2. Will make it look less random when reading the code.

>+                                /* Test in progress, wait */
>+                                if (wait_for_property_notify ())
>+                                        goto read;
>+                                /* else fall through and do the check ourselves */
>+
>+                        }
>+                        else {

Nitpick: merge on one line.

Pushed with those corrections, thanks