Hi,

My name is Pascal Schoenhardt. I am the student doing the XRandR 1.2
SOC project.

I've begun working on a patch regarding this bug, but have a few things I need clarified. Here's what I've done so far:

I've done is created a new function
   static void init_multihead_support (GdkScreen *screen)

This function checks whether RandR 1.2 is present. If so, it fills the
screen_x11->monitors[] array using calls to RandR. If RandR 1.2 is not
present, the old init functions,
   init_randr_support
   init_xinerama_support
are called. This is what was meant by "use randr instead of xinerama where available", correct?

I'm implementing the functions to expose the additional CRTC/Output
properties right now, and that is where I am not entirely clear on
what is to be done:

- gint gdk_screen_get_monitor_[width/height]_mm (GdkScreen *screen,
gint monitor_num):
As I understand it, monitor_num refers to one of the CRTC's attached
to the single, virtual screen. However, this CRTC could have many
possible outputs attached. Therefore, there are at least two possible
things this function could return:

 - The "mm" of the CRTC, determined mathematically by the resolution
of the monitor_num'th crtc, and the DPI it is set to
 - The actual physical dimensions reported by randr of the 1'st
output attached to the monitor_num'th crtc on the screen.

Which one of these did you have in mind when you wrote up the bug?

- const gchar *gdk_screen_get_monitor_name (GdkScreen *screen, gint
monitor_num):
The purpose is clear, but again, I am confused on which device is
being referred to here. monitor_num refers to a crtc. How do we decide
which of the possibly many outputs attached to that CRTC should be
named?

- gint gdk_screen_get_monitor_connected (GdkScreen *screen, gint monitor_num):
I don't really understand what we are after here. Should this maybe be
"get_monitors_connected", and return the number of outputs attached to
a particular CRTC? If so, that would clear up some other questions. If
it was possible to determine the number of outputs attached to a CRTC,
then previous functions could be done as follows:

 - gint gdk_screen_get_monitor_height_mm (GdkScreen *screen, gint
monitor_num, gint output_num)
which would return the heigh in mm of the output_num'th display
device, attached to the monitor_num'th CRTC of screen.

-XID gdk_x11_screen_get_monitor_output (GdkScreen *screen, gint monitor_num)
A small amount of clarification herer would be nice.

Finally , I have a question about ABI's; I'm still pretty new to
glib/gobject. If I add something to the instance structure (ie: struct
_GdkScreenX11), will this break ABI? If ABI is broken, all programs
that use it will need to be recompiled, correct? What if additional
variables are added to the end of the instance structure, and no
existing ones are removed or modified; will this still break ABI?

Cheers,

Pascal Schoenhardt

Hi Pascal, glad you are working on this !

Here is my take on these questions (I'll ask some of our X hackers to double-check that I am not telling you nonsense here):

I think xinerama monitors (ie what the monitor_num in our api used to
refer to) should correspond to Xrandr outputs; crtcs are a concept that 
we don't really have to expose in the gdk api.

   RRGetOutputInfo
	output: OUTPUT
	config-timestamp: TIMESTAMP
	status: RRCONFIGSTATUS
	timestamp: TIMESTAMP
	crtc: CRTC
	
	name: STRING
	connection: CONNECTION
	subpixel-order: SUBPIXELORDER
        widthInMillimeters, heightInMillimeters: CARD32
	crtcs: LISTofCRTC
	clones: LISTofOUTPUT
	modes: LISTofOUTPUT
	num-preferred: CARD16

This is what Xrandr gives you for each output. 

name - is what I wanted to get from gdk_screen_get_monitor_name
connection - is what I expected to use for gdk_screen_get_monitor_connected
width/heightInMillimiters - is what gdk_screen_get_monitor_width/height_mm
                            return

Xrandr identifies outputs via XIDs, not the monitor_nums we have in the
gdk api. gdk_x11_screen_get_monitor_output() would be the way to map
between them, which might be necessary if you want to call Xrandr api
directly.

Adding to _GdkScreenX11 is fine; we have done it in the past. Adding to those
structs is bad if you expect people to derive from the class, which is not the case here.


One additional bit of information that I snapped up from our X hackers:
Xrandr exposes the EDID information for outputs. Some inforamation out of
that might be worth exposing, namely vendor and model strings.

It's probably not worth adding:

  gint gdk_screen_get_monitor_connected (GdkScreen *screen, gint monitor_num);

I'd recommend just not listing monitors that don't have a status of 'connected' to keep things simple.

OK, I see what you mean. So screen_x11->monitors[] should hold a list of all outputs. To fill it, we loop through each CRTC, and add each monitor in that order.

The reason I though that we would expose CRTCs, not outputs, was that in the control panel I am going to write, when users interact with the monitors and adjust their relative positions, they will be interacting with the CRTC's mostly, not the outputs. However, any app that needs that kind of control will use randr directly, so I guess we really don't need to expose CRTCs in gdk.

Another question: how much error checking needs to be done here? ie: someone calls for information with an invalid monitor_num. Should we be checking that in gdk, or is it up to the person using it to check these kinds of bounds. If we check it, what do we return in cases of error?

Exposing EDID information seems like a good idea. I will work that in as well.

I think we can get away without exposing CRTCs because this is a readonly api.
As you said, if you want to modify configuration, you need to use xrandr directly.

Wrt to error checking, public api should have g_return_if_fail-style checks
for programming errors like the invalid input you give as an example.

Created attachment 91191 [details] [review]
An incomplete patch, just to show progress. Compiles, but is UNTESTED!

Well, here is the patch as it currently stands. It is NOT yet complete, but I have kind of pushed it aside a bit and started working on the GUI display configuration tool. As suggested by Matthias, I am posting it to get some feedback - hopefully I'm not heading completely down the wrong path here!

What is done:
- When available, RandR 1.2+ is used instead of xinerama
- The width and height functions are implemented
- The name function is implemented
- the output function is implemented
- the monitors_changed signal has been added, but is not yet emitted anywhere

What isn't done, or places I'm stuck:
- Where should I subscribe to the new RandR signals? Where should I catch them (probably somewhere in gdkevents-x11.c, I would think).
- Lots of comments in the gdkscreen-x11.c init_multihead_support() function marked with TODO: which contain questions about things I wasn't sure about
- Return values in the case of error for most of the functions. Is what I am currently returning OK?

The patch compiles, but it is UNTESTED!

So, there it is. Feedback is eagerly awaited. Thanks!

Pascal

Ok, some initial feedback:

- Don't use HAVE_X. Just define the signal for all platforms
  and maybe document that it is only implemented on X currently. And the
  documentation should preferably not talk about "CRTCs and Outputs", 
  maybe say "screen geometry" or "number, size or position of monitors"

- Likewise for the gdk_screen functions, don't jump through HAVE_X
  and _imp() hoops, just define the functions in gdkscreen-x11.c and
  let other backends define their own version in their backend-specific
  code. 
 
- * Returns the width of the default screen in millimeters.
+ * Returns the width of the default screen in          meters.
 
  That looks suspicious...


- gdk_screen_get_monitor_output is an X-specific function (it returns
  and XID, and thus should live in gdkx.h and be called
  gdk_x11_screen_get_monitor_output
  

+  /* TODO: Currently returning 0 if no RandR or failure. Aks what should be returned. */

  Without XRANDR, you should probably use monitor dimension and screen
  dimensions to come up with a number, something like
  monitor-pixel-width / screen-pixel-width * screen-physical-width 


+  if (screen_x11->randr12)
+  {
+       XRRScreenResources *sr;
+    XRROutputInfo *output;
+       gchar *retval;
+       
+    sr = XRRGetScreenResources ( screen_x11->xdisplay,
+                                 screen_x11->xroot_window );
+    
+    output = XRRGetOutputInfo ( screen_x11->xdisplay,
+                                sr,
+                                (RROutput)screen_x11->act_outputs[monitor_num] );

  Might be worthwhile to factor this out into a 
  gdk_screen_get_output_info (screen, monitor_num)
  helper function ? 


+  if ((ver_maj > 2) || (ver_maj == 1 && ver_min >= 2))

  You probably mean ver_maj >= 2 here 


+       /* TODO: Is this level of error-checking necessary? Malloc only 
+        * fails when the system is out of memory. At that point, the system
+        * will probably crash anyways... */

  Not necessary. Just use g_malloc(). If we are out of memory here, we
  are doomed anyway.


+       for (i = 0; i < sr->noutput; i++)
+       {
+         outputs[i] = XRRGetOutputInfo ( screen_x11->xdisplay,
+                                         sr,
+                                         sr->outputs[i] );

  Some coding style: we use gnu-like indent of 2, and no space between 
  parens and arguments, like so:
 
       for (i = 0; i < sr->noutput; i++)
         {
           outputs[i] = XRRGetOutputInfo (screen_x11->xdisplay,
                                          sr,
                                          sr->outputs[i]);




- Where should I subscribe to the new RandR signals? Where should I catch them
(probably somewhere in gdkevents-x11.c, I would think).


  You should call XRRSelectInput() at the same place where you are 
  calling XSelectInput() right now. The right place to handle the 
  XRandr events is the huge switch in gdkevents-x11.c:gdk_event_translate
  Check out how other extension events are handled there, like
  XKB, or XFixes.


- Lots of comments in the gdkscreen-x11.c init_multihead_support() function
marked with TODO: which contain questions about things I wasn't sure about

  I've answered some of them above, let me grep for TODO now:

+   * TODO: FIND OUT WHAT VERSION THIS WILL BE INCLUDED IN...

  I'd put 2.14 for now. That seems very likely.

+  /* TODO: Currently returning 0 if no RandR or failure. Aks what should be returned. */

  I've answered that above

+  g_return_val_if_reached (NULL);
+  /* TODO: Currently returning NULL if no RandR or failure. Aks what should be returned. */

  Returning NULL is fine here. It should just be "return NULL" though,
  no g_return_val_if_reached(), that is only for programming errors.
  The docs should mention that NULL is a possible return value.

     
General comment regarding init_multihead(): You probably want to factor out
the part that allocates and fills the outputs and monitors arrays, since
you will have to do that again when the geometry changes (ie when you
get an XRANDR event), before emitting the monitors-changed signal.

git clone git://anongit.freedesktop.org/git/xorg/app/edid-decode

has some reference code for decoding edid information, if you want to
look into that. The relevant part for our purposes might just be the
following:

    printf("Manufacturer: %s Model %x Serial Number %u\n",
            manufacturer_name(edid + 0x08),
            (unsigned short)(edid[0x0A] + (edid[0x0B] << 8)),
            (unsigned int)(edid[0x0C] + (edid[0x0D] << 8)
                           + (edid[0x0E] << 16) + (edid[0x0F] << 24)));

Created attachment 97649 [details] [review]
cleaned up version of the patch

Created attachment 99451 [details] [review]
New patch

Attaching new patch as requested by Bastien. 

It's available from here as well

http://www.gnome.org/~ssp/randr/gtk.patch

The patch has meanwhile been committed.