Created attachment 126677 [details] [review]
Patch to fix the bug

This patch will detect the version of RandR and use XRRGetScreenResourcesCurrent when available.

Hrm, if the API is only available in 1.3, then your patch won't compile with xrandr 1.2. Or am I missing something?

It will compile. If RandR 1.2 is used then the old behaviour (XRRGetScreenResources) is preserved.

The RandR version check takes place at runtime.

Created attachment 126752 [details] [review]
Patch to fix the bug at compilation time

As Vincent pointed out on IRC, compilation will fail if RandR 1.3 is not available since the new function doesn't exist in 1.2.

This new patch is even simpler and checks the availability of RandR 1.3 at compilation time

Where is RANDR_13_INTERFACE defined? I don't see it in headers from http://cgit.freedesktop.org/xorg/proto/randrproto/tree/

Created attachment 126771 [details] [review]
Patch to fix the bug at compilation time based on randr.h

This patch relies on randr.h i.e. on the RandR procol rather than on the
xserver itself. This will prevent us from adding the xserver development
libraries as build dependencies (thus preserving the previous behaviour).

Thanks, committed!

You still need to check the version of the extension at runtime, otherwise you will fail if communicating with an older server.

Created attachment 126905 [details] [review]
ignore output property changes

This patch written by Peter Clifton makes sure that output property changes (e.g. in the backlight) don't trigger XRRGetScreenSizeRange, thus reducing the amount of times this expensive function is called.

But does using XRRGetScreenResourcesCurrent() actually work?

Note that the Display capplet in gnome-control-center has a "detect displays" button which calls gnome_rr_screen_refresh():

  gnome_rr_screen_refresh()
    screen_update()
      screen_info_new()
        fill_out_screen_info()
          XRRGetScreenResourcesCurrent()

The capplet expects that gnome_rr_screen_refresh() will yield "the latest refreshed status", not "the last known status", so that the capplet will be able to display accurate information about the monitors that are plugged in.

(Remember that we have a "Detect Displays" command precisely because we don't get hotplug notifications in many situations, so the user must be able to trigger a "detect everything now" command.)

We may need to make a distinction between "get me the last known status", suitable for gnome_rr_screen_new(), and "get me the refreshed status", for gnome_rr_screen_refresh().

CCing Søren to see if he has something to say about the intended API with RANDR 1.3 in the picture.

Hmm, I take your point.

The distinction between retrieving explcitly probed costly information, and performing costly probes from within a change notification callback should probably be made in the code.

Federico: it works well here. I suspect that XRRGetScreenSizeRange() (called in fill_out_screen_info) does (or causes) hardware probing (since it's a rather expensive call).

The way GnomeRR works, it is never useful to call XRRGetScreenResourcesCurrent(). Either a reprobe is necessary, or the existing information in GnomeRRScreen is still valid.

RandR is designed so that you are supposed to call XRRGetScreenResources() whenever you get a change notification. It may make sense to move that call to an idle handler since you often get more than one at the same time.

I really don't think that is true any more.

The change notifications should actually carry the information you need, and if not, the Xserver notifying you _of_ the change, already knows the required config, hence XRRGetScreenResourcesCurrent() _is_ appropriate.

This change breaks the Detect Monitors button

Yes, now that the bug in libxrandr has been fixed I think this patch can be reverted. I agree with Federico though that it would be wise to make a distinction between getting the last known status and reprobing hardware.

Created attachment 130951 [details] [review]
Output properties changes call the least expensive function.

The attached function simply distinguishes between changes in output properties and other RR events. When the former take place, the least expensive function is called.

If you agree with my changes I can work on it so as to check the version of RandR both at runtime and at compilation time.

Let me know.

NOTE: the following bug report seems to be a duplicate of this one:
http://bugzilla.gnome.org/show_bug.cgi?id=574931

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

(In reply to comment #18)

> The attached function simply distinguishes between changes in output properties
> and other RR events. When the former take place, the least expensive function
> is called.

Nice!  Can you please make some stylistic changes:

* Call the extra argument "needs_reprobe" instead of "is_cheap", to make it self-explanatory, and of course reverse the test in fill_out_screen_info().

* The convention is that a GError argument to a function is always passed at the end.  Put the needs_reprobe argument before the GError.

* Can you pass the "-p" option to diff so it will put function names in each hunk?

* In screen_on_event(), can't we handle *all* events with needs_reprobe=FALSE?

* We may need to do something to avoid XRRGetScreenSizeRange().  Can we call it only when the updated timestamp changes, or something like that?

> If you agree with my changes I can work on it so as to check the version of
> RandR both at runtime and at compilation time.

Yes, we need that.

>
> * In screen_on_event(), can't we handle *all* events with
> needs_reprobe=FALSE?
 
I would rather not, otherwise if something more important than output properties changes and we don't notice it, thus not calling RRGetScreenInfo again (etc.), weird things can happen.
 
>
> * We may need to do something to avoid XRRGetScreenSizeRange().  Can we
> call it only when the updated timestamp changes, or something like that?
>
 
Yes, that call is pretty expensive too. I think we can skip it if "needs_reprobe == FALSE".
 
As I said above, it's still good to keep it if something (other than output properties) changes.
 
I'll attach a new patch soon.

Created attachment 131041 [details] [review]
Final patch

I hope the attached patch matches what Federico suggested. If it doesn't, just let me know and I'll rewrite it.

Actually, now that I think about it, it's not such a great idea to check the version of RandR for each event.

I'll rewrite the patch.

(In reply to comment #21)
> >
> > * In screen_on_event(), can't we handle *all* events with
> > needs_reprobe=FALSE?
> 
> I would rather not, otherwise if something more important than output
> properties changes and we don't notice it, thus not calling RRGetScreenInfo
> again (etc.), weird things can happen.

I mean, when the X server sends a RANDR event, does it also have knowledge of the new state of the outputs?  I would assume so, otherwise it wouldn't have sent the event ;)  Can you check the X sources to see if this is the case?  If the server already knows the new state, then we can call XRRGetScreenResourcesCurent() instead of the expensive call when we get an event.

I have tried to distinguish between the different events supported by RandR and I've noticed that the RRScreenChangeNotify event (i.e. the one that should be triggered when you plug in or unplug outputs) is triggered only when you explicitly probe hardware.

I talked to ajax and keithp and they told me that "they're hooking up interrupt-driven hotplug in KMS, but it isn't working yet" and that kind of event is "entirely dependent on the driver to wire up interrupts for output changes.  which none of them do yet."

When all this is ready we could safely use XRRGetScreenResourcesCurrent() (or XRRGetScreenResources) when we receive a RRScreenChangeNotify event. For all the rest (i.e. for devices that you don't get real hotplug for like some vga or tv ports) users will have to click on the "Detect monitors" button so as to probe hardware manually.

In conclusion all we do at the moment is simply get RROutputPropertyNotify (mainly because of changes in the "BACKLIGHT" property) when for example we unplug or plug in the AC cable of a laptop, which is really not something for which we should call XRRGetScreenResources().

In my opinion, if we really want to make this future-proof (and fix this bug too) we should do the following:
1) intercept RRScreenChangeNotify events and, only in this case, call XRRGetScreenSizeRange() and either XRRGetScreenResources() or XRRGetScreenResourcesCurrent().
2) in case of other events it would be more than enough to call XRRGetScreenResourcesCurrent()

Created attachment 131247 [details] [review]
Final version of the patch

The attached patch should finally fix this bug and make gnome-desktop future-proof (waiting for KMS and hotplug events).

Please review it and let me know if there's something that you would like to see changed.

Created attachment 132356 [details] [review]
/gnome-desktop-bgo568160-randr-avoid-reprobing.diff

This is the patch I committed; it is essentially Alberto's patch with some stylistic changes.

2009-04-08  Federico Mena Quintero  <federico@novell.com>

	http://bugzilla.gnome.org/show_bug.cgi?id=568160 - Use
	XRRGetScreenResourcesCurrent() if available (i.e. we were compiled
	with RANDR 1.3 and it is actually available at runtime), when we
	only need to fetch the current RANDR status.  Use
	XRRGetScreenResources() as usual when we do need to re-probe.
	Patch by Alberto Milone <albertomilone@alice.it>, with minor
	modifications:

	* gnome-rr.c (fill_out_screen_info): Take a needs_reprobe
	argument.  We'll use XRRGetScreenResourcesCurrent() or
	XRRGetScreenResources() as appropriate.
	(screen_on_event): Don't re-probe RANDR for all events; just for
	RRScreenChangeNotify.  This avoids expensive probing for (e.g.)
	when only output properties change (RRNotify).