[xrandr] Only use the current information when setting modes

[Chris Wilson]

On Sun, Sep 13, 2015 at 08:01:46PM +0200, Mark Kettenis wrote:
> > Date: Sun, 13 Sep 2015 12:14:57 +0100
> > From: Chris Wilson <chris at chris-wilson.co.uk>
> > 
> > On Sun, Sep 13, 2015 at 01:08:11PM +0200, Mark Kettenis wrote:
> > > > From: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Date: Sun, 13 Sep 2015 11:40:37 +0100
> > > > 
> > > > Before we change the state (e.g. adding a mode or applying one to an
> > > > output), we query the screen resources for the right identifiers. This
> > > > should only use the current information rather than force a reprobe on
> > > > all hardware - not only can that reprobe be very slow (e.g. EDID
> > > > timeouts on the order of seconds), but it may perturb the setup that the
> > > > user is trying to configure.
> > > 
> > > How do you guarantee that that cached information isn't stale?
> > 
> > There is no issue in that, since the user parameters are against the
> > output from a previous run. If the configuration is stale then so are
> > the xrandr parameters.
> 
> I bet tons of people have a script that runs xrandr to configure a
> projector connected to the VGA port on their laptop.  Yes there is no
> guarantee that the parameters I give to xrandr match the current
> configuration.  But if I connect the same projector (or even a
> different one) to the same VGA port I expect this script to work and
> make the display of my laptop appear on the projector.  I expect this
> to work even when I connected the VGA cable after I started X.

If you relied on xrandr without checking for the existence of the output
first, you are describing a race condition. (Any use of xrandr is a race
condition!) To maintain the behaviour of providing that automagic probe,
you can extend get_outputs() to reprobe if the user requests a conflicting
operation, like

@@ -1800,10 +1800,26 @@ apply (void)
  * Use current output state to complete the output list
  */
 static void
-get_outputs (void)
+get_outputs (int use_current)
 {
     int                o;
     output_t    *q;
+    int disconnected;
+
+    do
+    {
+        if (res)
+        {
+            XRRFreeScreenResources(res);
+            res = NULL;
+        }
+
+        get_screen (use_current);
+
+        disconnected = 0;
+
+        for (q = all_outputs; q; q = q->next)
+            q->found = False;
 
         for (o = 0; o < res->noutput; o++)
         {
@@ -1872,7 +1888,16 @@ get_outputs (void)
             }
 
             set_output_info (output, res->outputs[o], output_info);
+            disconnected +=
+                output_info->connection == RR_Disconnected &&
+                output_info->crtc;
         }
+
+        for (q = all_outputs; q; q = q->next)
+            disconnected += !q->found;
+
+    } while (disconnected && ++use_current == 0);
+
     for (q = all_outputs; q; q = q->next)
     {
         if (!q->found)

> > > Seems you already can get the behaviour you want by specifying
> > > --current.  Whereas there is no convenient way to force a probe, other
> > > than an explicit query?
> > 
> > And there should not be. On KMS platforms regeneration of the RR
> > information is driven by hotplug events (either generated by the hw
> > itself or by a kernel worker emulating the notifications otherwise). An
> > explicit probe by the user should be the means of last resort not the
> > default.
> 
> Well, that only works on KMS with udev.  And only if the specific
> driver you're using has udev support.
> 
> To me it seems that you're trying to solve this at the wrong level.
> If a driver (kernel or Xorg) has reliable hotplug support, shouldn't
> it be able to decide whether it needs to reread the EDID information
> of some monitor or not?

Eh? We already have multiple layers of caching. Howver users making
explicit reprobe requests have to override the caching just in case
the cache/hardware is broken. The issue is that xrandr does the
explicit by default (and similarly that very few pieces of software use
XRRGetScreenResourcesCurrent but there are a lot of silly (i.e. where
applications are only querying monitor layout with no desire to set
configurations) uses of XRRGetScreenResources).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre