Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
Hi, On Wed, Feb 23, 2011 at 03:46:14PM -0800, Keith Packard wrote: On Wed, 23 Feb 2011 10:16:46 -0800, Aaron Plattner aplatt...@nvidia.com wrote: However, I do need to object strongly to such a major protocol change being made after RC2 of a release cycle. We're supposed to be making only critical bug fixes at this point! Planning fail. So, I think I can resolve these issues, and I thank you for your review. I clearly need to be more careful about getting protocol changes and client API reviews made before committing code into the X server. As an alternative plan, I've got a server branch with all of RandR 1.4 removed so that we can properly review the protocol, client library interfaces and driver interfaces. git://people.freedesktop.org/~keithp/xserver.git backout-randr I'd love to hear opinions on which option people would be happiest with. I'd be happiest with RandR 1.4 backed out until 1.11. I just don't think it's had quite enough (non-rushed) review, exposure or testing, and it's not the best of precedents to be setting for future releases. Sorry. Cheers, Daniel signature.asc Description: Digital signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Thu, 24 Feb 2011 13:37:50 +, Daniel Stone dan...@fooishbar.org wrote: I'd be happiest with RandR 1.4 backed out until 1.11. I just don't think it's had quite enough (non-rushed) review, exposure or testing, and it's not the best of precedents to be setting for future releases. Sorry. As release manager, this seems like the right solution. However, it's a larger change than adding the new fix-ups, which doesn't exactly make me less nervous. In either case, I need to see some testing of the server before I feel comfortable releasing it. Can I get people to run the backout-randr branch today so we can get some idea of whether we had accidental dependencies on the randr 1.4 code in drivers or other parts of the server? The branch without RandR 1.4: git://people.freedesktop.org/~keithp/xserver backout-randr If you want to try out the RandR 1.4 bits, you can use: git://people.freedesktop.org/~keithp/xserver randr-fixes To test the RandR 1.4 bits, you'll need: git://anongit.freedesktop.org/xorg/proto/randrproto master git://people.freedesktop.org/~keithp/libXrandr randr-1.4 git://people.freedesktop.org/~keithp/xrandr randr-1.4 Please report success or failure from any testing. -- keith.pack...@intel.com pgpwhlhI5DxEB.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
Hi, Keith Packard kei...@keithp.com (24/02/2011): As release manager, this seems like the right solution. However, it's a larger change than adding the new fix-ups, which doesn't exactly make me less nervous. not that I'm advocating last-minute pushes, I gave it a try anyway. If you want to try out the RandR 1.4 bits, you can use: git://people.freedesktop.org/~keithp/xserver randr-fixes To test the RandR 1.4 bits, you'll need: git://anongit.freedesktop.org/xorg/proto/randrproto master git://people.freedesktop.org/~keithp/libXrandr randr-1.4 git://people.freedesktop.org/~keithp/xrandr randr-1.4 Please report success or failure from any testing. Short story: build ok, runtime nok. Longer story: (I'm assuming you're going to add version checks at the end, which seems to be the standard way when patches are floating around; without those, xrandr 1.4 would fail to build without libxrandr 1.4, for example. That said:) Building was OK. Basic rotations or mode changes on LVDS1 were OK. Dual screen wasn't. I plugged a VGA monitor on my laptop (with an Intel chipset, from the 94x series: 8086:27a2). Default: xrandr --auto and: xrandr --output VGA1 --same-as LVDS1 are OK. That doesn't work right: xrandr --output VGA1 --right-of LVDS1 My pointer now appears on both LVDS1 and VGA1, at the same (relative I'd say?) position: - when the pointer sits in the corner of an output, it sits on the same corner of the other output. - needless to say, when the pointer reaches the right border of the left screen, it also reaches the right border of the right screen, and disappears in a galaxy (not too) far away. For my tests, I tried to avoid running into combinatorial explosion, so I kept 1.4-aware libxrandr the whole time. * server + xrandr = ok → hurrah! * server + xrandr-1.4 = nok → X Error of failed request [see below] * server-1.4 + xrandr = ok → no (evident) regression * server-1.4 + xrandr-1.4 = nok → double pointer effect. The X Error is: | $ xrandr --output VGA1 --right-of LVDS1 | X Error of failed request: BadLength (poly request too large or internal Xlib length error) | Major opcode of failed request: 149 (RANDR) | Minor opcode of failed request: 36 () | Serial number of failed request: 34 | Current serial number in output stream: 34 The 4th case is pretty annoying, and was detailed above. The 2nd case worries me a bit. Shouldn't one be able to use library/binary newer than the server, and expect those to figure out what the server can do and how‽ Feeling doubtful, KiBi. signature.asc Description: Digital signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Fri, 25 Feb 2011 03:01:17 +0100, Cyril Brulebois k...@debian.org wrote: The 4th case is pretty annoying, and was detailed above. The 2nd case worries me a bit. Shouldn't one be able to use library/binary newer than the server, and expect those to figure out what the server can do and how‽ The BadLength error means that the library is busted (yikes!). I'll go ahead and merge the whole sequence of RandR 1.4 reverts to master and push out another RC right now; it's pretty clear which way we need to go. I'll plan on shipping 1.10 tomorrow if things test OK. I've been running the backout-randr code all day today, and it works for me, with minor monitor switching tested. -- keith.pack...@intel.com pgp8uDv61zGO9.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Thu, Feb 24, 2011 at 12:42 PM, Keith Packard kei...@keithp.com wrote: On Thu, 24 Feb 2011 13:37:50 +, Daniel Stone dan...@fooishbar.org wrote: I'd be happiest with RandR 1.4 backed out until 1.11. I just don't think it's had quite enough (non-rushed) review, exposure or testing, and it's not the best of precedents to be setting for future releases. Sorry. As release manager, this seems like the right solution. However, it's a larger change than adding the new fix-ups, which doesn't exactly make me less nervous. In either case, I need to see some testing of the server before I feel comfortable releasing it. Can I get people to run the backout-randr branch today so we can get some idea of whether we had accidental dependencies on the randr 1.4 code in drivers or other parts of the server? We have this commit in the most recent radeon release: http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=3dc28c86eb57cac819e4ed650acfe1f7df6ef565 Which will conflict with the backout. Alex The branch without RandR 1.4: git://people.freedesktop.org/~keithp/xserver backout-randr If you want to try out the RandR 1.4 bits, you can use: git://people.freedesktop.org/~keithp/xserver randr-fixes To test the RandR 1.4 bits, you'll need: git://anongit.freedesktop.org/xorg/proto/randrproto master git://people.freedesktop.org/~keithp/libXrandr randr-1.4 git://people.freedesktop.org/~keithp/xrandr randr-1.4 Please report success or failure from any testing. -- keith.pack...@intel.com ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Fri, 25 Feb 2011 01:23:57 -0500, Alex Deucher alexdeuc...@gmail.com wrote: We have this commit in the most recent radeon release: http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=3dc28c86eb57cac819e4ed650acfe1f7df6ef565 Which will conflict with the backout. You can switch that to #ifdef RANDR_14_INTERFACE I will note that I nearly left this particular change in the server; it was the only change not dependent on the RandR 1.4 protocol. -- keith.pack...@intel.com pgpYa296RFalN.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Wed, Feb 16, 2011 at 10:59:22PM -0800, Keith Packard wrote:
This permits clients to perform incremental configuration changes
instead of requiring a complete new configuration to be written.
Signed-off-by: Keith Packard kei...@keithp.com
---
randr/rrcrtc.c | 332
++--
1 files changed, 202 insertions(+), 130 deletions(-)
diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
index 5fe6900..07e9019 100644
--- a/randr/rrcrtc.c
+++ b/randr/rrcrtc.c
@@ -1483,63 +1483,82 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr
screen,
PixmapPtr pixmap;
intrc, i, j;
Rotation rotation;
+intnumOutputs;
VERIFY_RR_CRTC(x-crtc, crtc, DixSetAttrAccess);
-if (x-mode == None)
-{
- mode = NULL;
- if (x-nOutput 0)
- return BadMatch;
+mode = crtc-mode;
+numOutputs = crtc-numOutputs;
+
+if (x-set RR_SetCrtcOutputs)
+ numOutputs = x-nOutput;
+
+if (x-set RR_SetCrtcMode) {
+ if (x-mode == None)
+ mode = NULL;
+ else
+ {
+ VERIFY_RR_MODE(x-mode, mode, DixSetAttrAccess);
+ if (x-nOutput == 0)
+ return BadMatch;
+ }
randrproto.txt says nothing about SetCrtcMode.
}
-else
+
+if (numOutputs)
{
- VERIFY_RR_MODE(x-mode, mode, DixSetAttrAccess);
- if (x-nOutput == 0)
+ if (mode == NULL)
return BadMatch;
-}
-if (x-nOutput)
-{
- outputs = malloc(x-nOutput * sizeof (RROutputPtr));
+ outputs = malloc(numOutputs * sizeof (RROutputPtr));
if (!outputs)
return BadAlloc;
}
-else
+else {
+ if (mode != NULL)
+ return BadMatch;
randrproto.txt says nothing about SetCrtcOutputs. The interplay encoded
here probably ought to be described in the protocol doc.
outputs = NULL;
-
-if (x-pixmap == None)
- pixmap = NULL;
-else if (x-pixmap == RR_CurrentScanoutPixmap)
- pixmap = crtc-scanoutPixmap;
-else
-{
- rc = dixLookupResourceByType((pointer *) pixmap, x-pixmap,
-RT_PIXMAP, client, DixWriteAccess);
- if (rc != Success) {
- free(outputs);
- return rc;
- }
- /* XXX check to make sure this is a scanout pixmap */
}
-for (i = 0; i x-nOutput; i++)
-{
- rc = dixLookupResourceByType((pointer *)(outputs + i), outputIds[i],
-RROutputType, client, DixSetAttrAccess);
- if (rc != Success)
+if (x-set RR_SetCrtcPixmap) {
+ if (x-pixmap == None)
+ pixmap = NULL;
+ else
{
- free(outputs);
- return rc;
+ rc = dixLookupResourceByType((pointer *) pixmap, x-pixmap,
+RT_PIXMAP, client, DixWriteAccess);
+ if (rc != Success) {
+ free(outputs);
+ return rc;
+ }
+ /* XXX check to make sure this is a scanout pixmap */
}
- /* validate crtc for this output */
- for (j = 0; j outputs[i]-numCrtcs; j++)
- if (outputs[i]-crtcs[j] == crtc)
- break;
- if (j == outputs[i]-numCrtcs)
+} else
+ pixmap = crtc-scanoutPixmap;
+
+if (x-set RR_SetCrtcOutputs) {
+ for (i = 0; i numOutputs; i++)
{
- free(outputs);
- return BadMatch;
+ rc = dixLookupResourceByType((pointer *)(outputs + i),
outputIds[i],
+RROutputType, client,
DixSetAttrAccess);
+ if (rc != Success)
+ {
+ free(outputs);
+ return rc;
+ }
+ /* validate crtc for this output */
+ for (j = 0; j outputs[i]-numCrtcs; j++)
+ if (outputs[i]-crtcs[j] == crtc)
+ break;
+ if (j == outputs[i]-numCrtcs)
+ {
+ free(outputs);
+ return BadMatch;
+ }
}
+} else
+ memcpy(outputs, crtc-outputs, numOutputs * sizeof (RROutputPtr));
+
+for (i = 0; i numOutputs; i++)
+{
/* validate mode for this output */
for (j = 0; j outputs[i]-numModes + outputs[i]-numUserModes; j++)
{
@@ -1555,10 +1574,11 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr
screen,
return BadMatch;
}
}
+
/* validate clones */
-for (i = 0; i x-nOutput; i++)
+for (i = 0; i numOutputs; i++)
{
- for (j = 0; j x-nOutput; j++)
+ for (j = 0; j numOutputs; j++)
{
int k;
if (i == j)
@@ -1579,46 +1599,68 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr
screen,
if
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Wed, Feb 23, 2011 at 10:16:46AM -0800, Aaron Plattner wrote: On Wed, Feb 16, 2011 at 10:59:22PM -0800, Keith Packard wrote: Other than the protocol doc comments and the existing bug noted above, Reviewed-by: Aaron Plattner aplatt...@nvidia.com However, I do need to object strongly to such a major protocol change being made after RC2 of a release cycle. We're supposed to be making only critical bug fixes at this point! Planning fail. -- Aaron I believe this all got commited a while ago. There was even a day or so when the proto and xserver were out of sync, and there was a tinderbox complaint and at least one bug. Only a single SOB was present. Luc Verhaegen. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Wed, 23 Feb 2011 10:16:46 -0800, Aaron Plattner aplatt...@nvidia.com wrote:
randrproto.txt says nothing about SetCrtcMode.
...
randrproto.txt says nothing about SetCrtcOutputs. The interplay encoded
here probably ought to be described in the protocol doc.
Yeah, randrproto.txt is missing some text there...
-screen_config.screen_pixmap_width = stuff-screenPixmapWidth;
-screen_config.screen_pixmap_height = stuff-screenPixmapHeight;
-screen_config.screen_width = stuff-screenWidth;
-screen_config.screen_height = stuff-screenHeight;
-screen_config.mm_width = stuff-widthInMillimeters;
-screen_config.mm_height = stuff-heightInMillimeters;
+if (num_configs 0) {
Existing bug, but it's not clear from randrproto.txt that RRSetCrtcConfigs
doesn't do anything if 'set' doesn't contain SetScreenCrtcs. From the
wording of the protocol doc, I would expect set = RR_SetScreenPixmapSize |
RR_SetScreenSizeInMillimeters | RR_SetScreenSize to change those three
things.
Agreed, this is a bug.
Other than the protocol doc comments and the existing bug noted above,
Reviewed-by: Aaron Plattner aplatt...@nvidia.com
However, I do need to object strongly to such a major protocol change being
made after RC2 of a release cycle. We're supposed to be making only
critical bug fixes at this point! Planning fail.
So, I think I can resolve these issues, and I thank you for your
review. I clearly need to be more careful about getting protocol changes
and client API reviews made before committing code into the X server.
As an alternative plan, I've got a server branch with all of RandR 1.4
removed so that we can properly review the protocol, client library
interfaces and driver interfaces.
git://people.freedesktop.org/~keithp/xserver.git backout-randr
I'd love to hear opinions on which option people would be happiest with.
--
keith.pack...@intel.com
pgpgGjf0kVsV4.pgp
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Wed, 23 Feb 2011 23:12:15 +0100, Luc Verhaegen l...@skynet.be wrote: I believe this all got commited a while ago. There was even a day or so when the proto and xserver were out of sync, and there was a tinderbox complaint and at least one bug. Only a single SOB was present. I just uncovered this application interface bug a bit over a week ago, and made the RandR protocol changes so that I could get the server code ready for 1.10. Very sub-optimal from a process perspective, but I'm not willing to ship the RandR 1.4 bits as they were when merged -- the application interface needs to change. -- keith.pack...@intel.com pgp27nMPA1nUi.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] randr: Add 'set' fields to SetCrtcConfigs request
On Wed, Feb 23, 2011 at 03:49:58PM -0800, Keith Packard wrote: On Wed, 23 Feb 2011 23:12:15 +0100, Luc Verhaegen l...@skynet.be wrote: I believe this all got commited a while ago. There was even a day or so when the proto and xserver were out of sync, and there was a tinderbox complaint and at least one bug. Only a single SOB was present. I just uncovered this application interface bug a bit over a week ago, and made the RandR protocol changes so that I could get the server code ready for 1.10. Very sub-optimal from a process perspective, but I'm not willing to ship the RandR 1.4 bits as they were when merged -- the application interface needs to change. Ah, ok, at no point this was clear, or i missed it. Luc Verhaegen. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
