[PATCH 2/2] os/connection: Remove NewOutputPending
Use any_output_pending() instead.
Signed-off-by: Jeremy Huddleston Sequoia
CC: Keith Packard
---
os/WaitFor.c| 2 +-
os/connection.c | 2 --
os/io.c | 5 +
os/osdep.h | 2 --
4 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/os/WaitFor.c b/os/WaitFor.c
index 024df35..cde2c8c 100644
--- a/os/WaitFor.c
+++ b/os/WaitFor.c
@@ -213,7 +213,7 @@ WaitForSomething(Bool are_ready)
timeout = check_timers();
BlockHandler(&timeout);
-if (NewOutputPending)
+if (any_output_pending())
FlushAllOutput();
/* keep this check close to select() call to minimize race */
if (dispatchException)
diff --git a/os/connection.c b/os/connection.c
index 4630438..dd0a25a 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -123,7 +123,6 @@ SOFTWARE.
struct ospoll *server_poll;
int MaxClients = 0;
-Bool NewOutputPending; /* not yet attempted to write some new output
*/
Bool NoListenAll; /* Don't establish any listening sockets */
static Bool RunFromSmartParent; /* send SIGUSR1 to parent process */
@@ -714,7 +713,6 @@ ClientReady(int fd, int xevents, void *data)
mark_client_ready(client);
if (xevents & X_NOTIFY_WRITE) {
ospoll_mute(server_poll, fd, X_NOTIFY_WRITE);
-NewOutputPending = TRUE;
}
}
diff --git a/os/io.c b/os/io.c
index ff0ec3d..0ebff08 100644
--- a/os/io.c
+++ b/os/io.c
@@ -596,7 +596,7 @@ FlushAllOutput(void)
{
OsCommPtr oc;
register ClientPtr client, tmp;
-Bool newoutput = NewOutputPending;
+Bool newoutput = any_output_pending();
if (!newoutput)
return;
@@ -607,7 +607,6 @@ FlushAllOutput(void)
* simply wait for the select to tell us when he's ready to receive.
*/
CriticalOutputPending = FALSE;
-NewOutputPending = FALSE;
xorg_list_for_each_entry_safe(client, tmp, &output_pending_clients,
output_pending) {
if (client->clientGone)
@@ -761,13 +760,11 @@ WriteToClient(ClientPtr who, int count, const void *__buf)
output_pending_clear(who);
if (!any_output_pending()) {
CriticalOutputPending = FALSE;
-NewOutputPending = FALSE;
}
return FlushClient(who, oc, buf, count);
}
-NewOutputPending = TRUE;
output_pending_mark(who);
memmove((char *) oco->buf + oco->count, buf, count);
oco->count += count;
diff --git a/os/osdep.h b/os/osdep.h
index 90a247f..9c8bd48 100644
--- a/os/osdep.h
+++ b/os/osdep.h
@@ -165,8 +165,6 @@ extern void SetConnectionTranslation(int conn, int client);
extern void ClearConnectionTranslation(void);
#endif
-extern Bool NewOutputPending;
-
extern WorkQueuePtr workQueue;
/* in WaitFor.c */
--
2.10.0 (Apple Git-99)
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/2] os/connection: Call ClientReady() based on a level trigger rather than an edge trigger
On encountering an EAGAIN, FlushClient() re-adds the client to the pending list and returns, but FlushClient() doesn't get called again. We would expect it to be called via FlushAllOutput() via WaitForSomething(), but that only happens when NewOutputPending is set. FlushAllOutput() also early-exits if NewOutputPending is not set. The only place that is setting NewOutputPending is ClientReady(). The problem there is that ClientReady() is called based on an edge trigger and not a level trigger. Signed-off-by: Jeremy Huddleston Sequoia CC: Keith Packard --- os/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/os/connection.c b/os/connection.c index 0d42184..4630438 100644 --- a/os/connection.c +++ b/os/connection.c @@ -749,7 +749,7 @@ AllocNewConnection(XtransConnInfo trans_conn, int fd, CARD32 conn_time) SetConnectionTranslation(fd, client->index); #endif ospoll_add(server_poll, fd, - ospoll_trigger_edge, + ospoll_trigger_level, ClientReady, client); set_poll_client(client); -- 2.10.0 (Apple Git-99) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: XLoadQueryFont() not returning with recent xserver master
I tracked it down to _XSERVTransWritev returning -1 with errno=EAGAIN within FlushClient(). I was able to workaround the issue by immediately retrying (usually once, sometimes twice). The problem is with the existing EAGAIN error handling. FlushClient() re-adds the client to the pending list and returns, but FlushClient() doesn't get called again. We would expect it to be called via FlushAllOutput() via WaitForSomething(), but that only happens when NewOutputPending is set. FlushAllOutput() also early-exits if NewOutputPending is not set. The only place that is setting NewOutputPending is ClientReady(). The problem there is that ClientReady() is called based on an edge trigger and not a level trigger. I'll be sending a patch for that in a moment, but I have a followup question from all of this... should we even have NewOutputPending any more? Should we just check any_output_pending()? The following two patches for consideration address these points. Thanks, Jeremy > On Sep 16, 2016, at 23:16, Jeremy Huddleston Sequoia > wrote: > > I bisected the issue to: > > f993091e7db81b0420e23c485378cba112278839 is the first bad commit > commit f993091e7db81b0420e23c485378cba112278839 > Author: Keith Packard > Date: Thu May 26 10:40:44 2016 -0700 > >os: Switch server to poll(2) [v3] > >Eliminates all of the fd_set mangling in the server main thread > >v2: Listen for POLLOUT while writes are blocked. > >v3: Only mark client not ready on EAGAIN return from read > >Signed-off-by: Keith Packard >Reviewed-by: Adam Jackson > > --- > > I'll see if I can dig into it a bit later, but my time is a bit thin. If > anyone else has a hunch as to what might be going on to narrow my search, I'd > appreciate it. If not, I'll probably start by taking a closer look at this > particular change to understand what's going wrong. > > Thanks, > Jeremy > >> On Sep 11, 2016, at 16:54, Jeremy Huddleston Sequoia >> wrote: >> >> Upon a bit more digging, it looks like the clients are listed in >> output_pending_clients, but it's not getting delivered. >> >> This situation reveals another issue with a use-after-free during >> CloseDownClient() in which a client will get re-added to the >> output_pending_clients during FlushClient() as part of CloseDownConnection(). >> >> See https://bugs.freedesktop.org/show_bug.cgi?id=97770 for more details on >> that. >> >>> On Sep 11, 2016, at 12:24, Jeremy Huddleston Sequoia >>> wrote: >>> >>> Using current master plus the various patches I submitted to the list last >>> night, I'm able to launch the server fairly reliably under ASan again, but >>> some clients are getting wedged. Most notably, xterm gets stuck waiting >>> for a reply from XLoadQueryFont(). Looking at the state of the server >>> threads, it looks fine. It's processing other requests just fine from >>> other clients. >>> >>> Has anyone noticed anything odd like this or have some hunch as to where I >>> might start looking other than to do yet another bisect? =/ >>> >>> Thread 0x344e96 DispatchQueue 1 1000 samples (1-1000) >>> priority 31 (base 31) >>> 1000 start + 52 (xterm + 6312) [0x10d3bf8a8] >>> 1000 main + 3752 (xterm + 146228) [0x10d3e1b34] >>>1000 spawnXTerm + 977 (xterm + 148776) [0x10d3e2528] >>> 1000 VTInit + 22 (xterm + 68248) [0x10d3cea98] >>>1000 XtRealizeWidget + 135 (libXt.6.dylib + 84572) [0x10d4ffa5c] >>> 1000 RealizeWidget + 871 (libXt.6.dylib + 85805) [0x10d4fff2d] >>>1000 RealizeWidget + 365 (libXt.6.dylib + 85299) [0x10d4ffd33] >>> 1000 VTRealize + 324 (xterm + 82404) [0x10d3d21e4] >>>1000 SetVTFont + 335 (xterm + 121728) [0x10d3dbb80] >>> 1000 xtermLoadFont + 514 (xterm + 114081) [0x10d3d9da1] >>>1000 xtermOpenFont + 86 (xterm + 112342) [0x10d3d96d6] >>> 1000 XLoadQueryFont + 311 (libX11.6.dylib + 29419) >>> [0x10d54f2eb] >>>1000 _XQueryFont + 163 (libX11.6.dylib + 32127) >>> [0x10d54fd7f] >>> 1000 _XReply + 279 (libX11.6.dylib + 143484) >>> [0x10d56b07c] >>>1000 xcb_wait_for_reply + 103 (libxcb.1.dylib + >>> 8247) [0x10d711037] >>> 1000 wait_for_reply + 251 (libxcb.1.dylib + >>> 8521) [0x10d711149] >>>1000 _xcb_conn_wait + 466 (libxcb.1.dylib + >>> 4166) [0x10d710046] >>> 1000 _xcb_in_read + 1051 (libxcb.1.dylib >>> + 12750) [0x10d7121ce] >>>1000 __select + 10 >>> (libsystem_kernel.dylib + 106318) [0x7fffc5e2df4e] >>> *1000 _sleep_continue + 0 >>> (kernel.development + 7459248) [0xff800091d1b0] >>> >>> >> >> ___ >> xorg-devel@lists.x.org: X.Org development >> Archiv
[PATCH xserver 2/2] xf86RandR12: Fix XF86VidModeSetGamma triggering a BadImplementation error
Commit b4e46c0444bb ("xfree86: Hook up colormaps and RandR 1.2 gamma code")
dropped the providing of a pScrn->ChangeGamma callback from the xf86RandR12
code. Leaving pScrn->ChangeGamma NULL in most cases.
This triggers the BadImplementation error in xf86ChangeGamma() :
if (pScrn->ChangeGamma)
return (*pScrn->ChangeGamma) (pScrn, gamma);
return BadImplementation;
Which causes X-apps using XF86VidModeSetGamma to crash with a
X protocol error.
This commit fixes this by re-introducing the xf86RandR12ChangeGamma
helper removed by the commit and adjusting it to work with the new
combined palette / gamma code.
Fixes: b4e46c0444bb ("xfree86: Hook up colormaps and RandR 1.2 gamma code")
Signed-off-by: Hans de Goede
---
hw/xfree86/modes/xf86RandR12.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index 36767b3..7b3b97b 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -1916,6 +1916,34 @@ xf86RandR12LoadPalette(ScrnInfoPtr pScrn, int numColors,
int *indices,
}
}
+/*
+ * Compatibility with XF86VidMode's gamma changer. This necessarily clobbers
+ * any per-crtc setup. You asked for it...
+ */
+
+static int
+xf86RandR12ChangeGamma(ScrnInfoPtr pScrn, Gamma gamma)
+{
+RRCrtcPtr randr_crtc = xf86CompatRRCrtc(pScrn);
+int size;
+
+if (!randr_crtc)
+return Success;
+
+size = max(0, randr_crtc->gammaSize);
+if (!size)
+return Success;
+
+init_one_component(randr_crtc->gammaRed, size, gamma.red);
+init_one_component(randr_crtc->gammaGreen, size, gamma.green);
+init_one_component(randr_crtc->gammaBlue, size, gamma.blue);
+xf86RandR12CrtcSetGamma(xf86ScrnToScreen(pScrn), randr_crtc);
+
+pScrn->gamma = gamma;
+
+return Success;
+}
+
static Bool
xf86RandR12EnterVT(ScrnInfoPtr pScrn)
{
@@ -2115,6 +2143,7 @@ xf86RandR12Init12(ScreenPtr pScreen)
rp->rrProviderDestroy = xf86RandR14ProviderDestroy;
pScrn->PointerMoved = xf86RandR12PointerMoved;
+pScrn->ChangeGamma = xf86RandR12ChangeGamma;
randrp->orig_EnterVT = pScrn->EnterVT;
pScrn->EnterVT = xf86RandR12EnterVT;
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/2] xf86RandR12: Move calculating of shift inside init_one_component
This is a preparation patch to allow easier usage of init_one_component
outside of xf86RandR12CrtcInitGamma.
Signed-off-by: Hans de Goede
---
hw/xfree86/modes/xf86RandR12.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index b136123..36767b3 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -1325,9 +1325,12 @@ xf86RandR12CrtcSetGamma(ScreenPtr pScreen, RRCrtcPtr
randr_crtc)
}
static void
-init_one_component(CARD16 *comp, unsigned size, unsigned shift, float gamma)
+init_one_component(CARD16 *comp, unsigned size, float gamma)
{
int i;
+unsigned shift;
+
+for (shift = 0; (size << shift) < (1 << 16); shift++);
if (gamma == 1.0) {
for (i = 0; i < size; i++)
@@ -1344,7 +1347,7 @@ static Bool
xf86RandR12CrtcInitGamma(xf86CrtcPtr crtc, float gamma_red, float gamma_green,
float gamma_blue)
{
-unsigned size = crtc->randr_crtc->gammaSize, shift;
+unsigned size = crtc->randr_crtc->gammaSize;
CARD16 *red, *green, *blue;
if (!crtc->funcs->gamma_set &&
@@ -1358,11 +1361,9 @@ xf86RandR12CrtcInitGamma(xf86CrtcPtr crtc, float
gamma_red, float gamma_green,
green = red + size;
blue = green + size;
-for (shift = 0; (size << shift) < (1 << 16); shift++);
-
-init_one_component(red, size, shift, gamma_red);
-init_one_component(green, size, shift, gamma_green);
-init_one_component(blue, size, shift, gamma_blue);
+init_one_component(red, size, gamma_red);
+init_one_component(green, size, gamma_green);
+init_one_component(blue, size, gamma_blue);
RRCrtcGammaSet(crtc->randr_crtc, red, green, blue);
free(red);
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
CARD32 vs uint32_t
libXfont2's _xfont2_client_funcs struct has a get_time_in_millis entry that is declared as 'uint32_t (*get_time_in_millis)(void);' xserver tries to use GetTimeInMillis for this, but it is declared as 'CARD32 GetTimeInMillis(void)'. The issue is that CARD32 is actually 'unsigned long' on i386, so the compiler complains about the possible mismatch (even though int and long are the same in such a configuration). dixfonts.c:2028:27: warning: incompatible pointer types initializing 'uint32_t (*)(void)' (aka 'unsigned int (*)(void)') with an expression of type 'CARD32 (void)' (aka 'unsigned long (void)') [-Wincompatible-pointer-types,Semantic Issue] .get_time_in_millis = GetTimeInMillis, ^~~ 1 warning generated. I'd really like to see us use stdint.h in Xmd.h. Are there any arguments against doing such a change? smime.p7s Description: S/MIME cryptographic signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [ANNOUNCE] xorg-server 1.18.99.2
Jeremy Huddleston Sequoia writes: > [ Unknown signature status ] > >> On Sep 16, 2016, at 13:57, Keith Packard wrote: >> >> >> I think we're ready for RC1 at this point, but wanted to give people a >> chance to scream about "just one more API change" until tomorrow. Let me >> know if there's something I'm missing; if I don't hear anything, I'll be >> tagging RC1 in the morning. > > IMO, we're not quite RC quality on master, so tagging it that way > feels a bit premature. And I didn't, I just set it to 99.2, which is a post-1.18, but pre-RC1 tag in our scheme (I think). > ASan trips over a bunch of issues still. I'd like to land the patches > that I sent out over the weekend that address memory corruption issues > if the server is started without any screens attached. That issue was > made obvious due to the input thread changes which altered our > initialization order to reveal the issue. > We should also address the use-after-free in CloseDownClient that I > discussed in https://bugs.freedesktop.org/show_bug.cgi?id=97770 and > figure out what's causing replies to get stuck unsent in the first > place (cf 9/11: Re: XLoadQueryFont() not returning with recent xserver > master). RC1 means we've got the feature set we want, but there may still be bugs. Do you have any features you'd like to land for 1.19 which haven't made it yet? Do you think there are bugs sufficient to jeopardize the 1.19 schedule? -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: CARD32 vs uint32_t
Jeremy Huddleston Sequoia writes: > I'd really like to see us use stdint.h in Xmd.h. Are there any > arguments against doing such a change? We can't for client side because Xlib, but I'd love to switch the X server to stdint. I think that would be pretty easy? -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [ANNOUNCE] xorg-server 1.18.99.2
Sent from my iPhone... > On Sep 17, 2016, at 21:43, Keith Packard wrote: > > Jeremy Huddleston Sequoia writes: > >> [ Unknown signature status ] >> >>> On Sep 16, 2016, at 13:57, Keith Packard wrote: >>> >>> >>> I think we're ready for RC1 at this point, but wanted to give people a >>> chance to scream about "just one more API change" until tomorrow. Let me >>> know if there's something I'm missing; if I don't hear anything, I'll be >>> tagging RC1 in the morning. >> >> IMO, we're not quite RC quality on master, so tagging it that way >> feels a bit premature. > > And I didn't, I just set it to 99.2, which is a post-1.18, but pre-RC1 > tag in our scheme (I think). > >> ASan trips over a bunch of issues still. I'd like to land the patches >> that I sent out over the weekend that address memory corruption issues >> if the server is started without any screens attached. That issue was >> made obvious due to the input thread changes which altered our >> initialization order to reveal the issue. > >> We should also address the use-after-free in CloseDownClient that I >> discussed in https://bugs.freedesktop.org/show_bug.cgi?id=97770 and >> figure out what's causing replies to get stuck unsent in the first >> place (cf 9/11: Re: XLoadQueryFont() not returning with recent xserver >> master). > > RC1 means we've got the feature set we want, but there may still be > bugs. > > Do you have any features you'd like to land for 1.19 which haven't made > it yet? Do you think there are bugs sufficient to jeopardize the 1.19 > schedule? No features on my end. I've basically just been keeping XQuartz in maintenance mode, so no qualms with the API or Feature Freeze. I'd just like to try to get some of these bugs that I noticed squashed before the rc1 tag if possible. > > -- > -keith ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
