Proper handling of extension events/errors?
I seem to have stumbled upon a rather horrible flaw in how extension events and errors are handled in the Xorg ecosystem, and this is breaking Xlib. :/ The wire protocol for extensions sends just two values, one base for events and one base for errors. The number of events or errors are _not_ sent over the wire, but is implicit information that the server and client must share. This means that the number of events/errors is an essential part of the protocol definition, and must be treated with care. Unfortunately it seems like people just update the number of events/errors as needed... Right now we're seeing misbehaviours with TigerVNC on Ubuntu. The reason being that Ubuntu has extended the number of Xfixes events from 3 to 2. So the server believes that events 90-91 are Xfixes, and 92-93 are RandR. But when the client gets event 92, it believes that is for Xfixes and uses the wrong wire-to-event callback. This does not seem to be isolated to libXfixes unfortunately. Looking at libXrandr, I see the same thing. AddExtension() is called with a fixed number of events, even though older RandR had just the one event. So it seems like the extension libraries need some fixing. libX11 could probably also check that an event/error slot is actually empty before filling it. And be a bit more cautious in the future when adding new events/errors, as it easy to create incompatibilities. :) For now I guess I'll have to add some padding to the server side AddExtension() between each extension. Hopefully I won't run out of ids... Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc 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: [Tigervnc-devel] [PATCH:tigervnc-trunk] Make fb header transformations work with Xorg 1.12.1
On Sat, 14 Apr 2012 14:10:20 -0700 Alan Coopersmith alan.coopersm...@oracle.com wrote: The reformatting of all the Xserver sources in the 1.12.1 release changed the headers so now some of them have multiple instances of the C++ and and xor keywords on the same line, so the hack to sanitize them needs to use the sed g flag to replace all instances, not just the first. Committed, thanks. Btw, would you like commit rights? I'd say you have enough street cred for that not to be an issue. :) Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc 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: Spurious bad X_ChangeProperty and MappingNotify:s
After a whole lot of digging, I've managed to trace this problem to its source. Basically it's XKB that's sending events even though the extension is disabled, resulting in more or less a random type value (which on this systems happens to be interpreted as XError). On 1.5 and 1.6, the code needs to check for noXkbExtension (see patch below). 1.7 and beyond is a bit more complex given the big rewrite going on in Xi. I can't say I see many checks for no XkbExtension at all in the new code. Is anyone actually testing with XKB compiled in but disabled? :) Anyhoo, here's a patch for 1.5 and 1.6: --- Xi/exevents.c.orig 2009-04-21 17:01:24.0 +0200 +++ Xi/exevents.c 2009-04-21 17:02:11.0 +0200 @@ -977,7 +977,8 @@ SendDeviceMappingNotify(ClientPtr client } #ifdef XKB -if (request == MappingKeyboard || request == MappingModifier) +if (!noXkbExtension (request == MappingKeyboard || +request == MappingModifier)) XkbApplyMappingChange(dev, request, firstKeyCode, count, client); #endif Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com signature.asc Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Some key repeats do not work without XKB
Another delightful bug when XKB is disabled. If you feed the core only
repeated KeyPress events, it's supposed to generate KeyRelease events
automatically. Unfortunately that code is broken and generates an
incorrect list of events.
Problems exists in 1.5 and 1.6.
Patch:
--- dix/getevents.c.orig2009-04-24 16:16:44.0 +0200
+++ dix/getevents.c 2009-04-24 16:19:20.0 +0200
@@ -406,6 +406,7 @@ GetKeyboardValuatorEvents(xEvent *events
int key_code, int first_valuator,
int num_valuators, int *valuators) {
int numEvents = 0;
+int numReleaseEvents;
CARD32 ms = 0;
KeySym *map = pDev-key-curKeySyms.map;
KeySym sym;
@@ -470,11 +471,13 @@ GetKeyboardValuatorEvents(xEvent *events
if (noXkbExtension)
#endif
{
-numEvents += GetKeyboardValuatorEvents(events, pDev,
- KeyRelease, key_code,
- first_valuator,
num_valuators,
- valuators);
-events += numEvents;
+numReleaseEvents = GetKeyboardValuatorEvents(events, pDev,
+ KeyRelease, key_code,
+ first_valuator,
+ num_valuators,
+ valuators);
+numEvents += numReleaseEvents;
+events += numReleaseEvents;
}
}
--
Pierre OssmanOpenSource-based Thin Client Technology
System Developer Telephone: +46-13-21 46 00
Cendio ABWeb: http://www.cendio.com
signature.asc
Description: PGP signature
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: Some key repeats do not work without XKB
On Tue, 28 Apr 2009 10:04:39 +1000 Peter Hutterer peter.hutte...@who-t.net wrote: ACK. Please open a bugreport for this, attach this patch and CC me on it. I'll nominate it for the matching server branches. While you're at it, please move the numReleaseEvents declaration into the noXkbExtension block too. Bug 21455. Want me to do the same for this: http://lists.x.org/archives/xorg-devel/2009-April/000744.html Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com signature.asc Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
lack of GetModifierMapping/SetModifierMapping symmetry
I'm having a bit of a problem with Xvnc here and I need to understand the core design a bit more. The basic problem is that some client does GetModifierMapping followed by a SetModifierMapping. This has the effect of completely nuking the modifier mapping tables. The reason this happens is how Xorg handles the mapping between the multiple keyboards internally and the single keyboard exposed via X11. A call to GetModifierMapping gives you the mappings for the currently active keyboard, but SetModifierMapping modifies the mappings for all keyboards (strictly speaking, all core keyboards). Now since Xvnc shares most of its code with libvnc.so (the addon to a running Xorg server), it adds a second keyboard and does not try to make it the primary one. Since Xorg wants a primary keyboard, it creates a dummy one which is basically blank. So the client mentioned above will read the empty mappings from this dummy keyboard and write them to the VNC keyboard. I'm not entirely sure how to fix this. I'd like to understand why GetModifierMapping/SetModifierMapping are implemented the way they are first though. Does anyone have any insight into that? Peter, you've been neck deep in Xorg's input layer for some time now. Do you have any ideas? Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com signature.asc Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: lack of GetModifierMapping/SetModifierMapping symmetry
On Thu, 30 Apr 2009 12:25:50 +1000 Peter Hutterer peter.hutte...@who-t.net wrote: On Tue, Apr 28, 2009 at 04:07:37PM +0200, Pierre Ossman wrote: The reason this happens is how Xorg handles the mapping between the multiple keyboards internally and the single keyboard exposed via X11. A call to GetModifierMapping gives you the mappings for the currently active keyboard, but SetModifierMapping modifies the mappings for all keyboards (strictly speaking, all core keyboards). Assuming that SetModifierMapping works on all keyboards, GetModifierMapping from any keyboard is identical to any other one. This theory applies to a couple of calls that allow to query or apply magic on an input device. In which case I'd consider the mechanism for adding new keyboards broken. If the idea is that the modifier mapping should be global, then adding a new keyboard with a different mapping should either a) change the global mapping, or b) overwrite the mapping on the new keyboard with the global one. The current behaviour is just random and magical. I'm not entirely sure how to fix this. I'd like to understand why GetModifierMapping/SetModifierMapping are implemented the way they are first though. Does anyone have any insight into that? For years the server looked the same so clients relied on a certain behaviour. Now, with multiple keyboards that may all be independent we have to balance between doing the right thing and not breaking clients. This is not a behaviour that has been with us for years. It was added for xserver 1.3. So apparently modifying just the active keyboard was not a major problem until then. Daniel did not describe why the change was needed in his commit message unfortunately. MPX gets rid of this in parts because the requests only work on a single master device at a time. If you only have a single MD however, the behaviour shouldn't be any different. The fix for you in libvnc/Xvnc would simply be to initialise a default mapping for the magic keyboard, wouldn't it? I do, but it gets overwritten by the mapping of another keyboard. I've done a quick hack that does a SwitchCoreKeyboard() to the VNC keyboard once it gets the on call to its keyboard proc. That seems to work well enough for the Xvnc case. I'm not sure what side effects that method will have when it comes to using libvnc.so with a real X server though, so I haven't dared commit that workaround permanently yet. Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com signature.asc Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: lack of GetModifierMapping/SetModifierMapping symmetry
On Fri, 1 May 2009 15:40:50 +1000 Peter Hutterer peter.hutte...@who-t.net wrote: On Thu, Apr 30, 2009 at 09:14:43AM +0200, Pierre Ossman wrote: In which case I'd consider the mechanism for adding new keyboards broken. If the idea is that the modifier mapping should be global, then adding a new keyboard with a different mapping should either a) change the global mapping, or b) overwrite the mapping on the new keyboard with the global one. The modifier mapping isn't global. it applies to the core keyboard which is defined by the core protocol. All core calls only apply to this keyboard. In the past this was't a problem since we only had one keyboard anyway. Since the mapping is applied to every (core) keyboard, I'd consider it global in any practical sense, even though there are multiple places where it is stored. This is not a behaviour that has been with us for years. It was added for xserver 1.3. So apparently modifying just the active keyboard was not a major problem until then. The active keyboard never changed in the past. that's essentially the big difference. How was multiple keyboards handled internally before that? Perhaps there are some things there that can make sense even today for the VNC case. Is the VNC keyboard an extension device or does it replace inputInfo.keyboard? It's an extension device. It is a core keyboard, but it only activates via SwitchCoreKeyboard(). Overwriting inputInfo.keyboard does not seem to be the correct thing to do when VNC is just a module attaching to an existing X server. Rgds -- Pierre OssmanOpenSource-based Thin Client Technology System Developer Telephone: +46-13-21 46 00 Cendio ABWeb: http://www.cendio.com signature.asc Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
[RFC] [PATCH] Extend block and wakeup handling to cover writes as well
For TigerVNC we need to ability to gracefully wait for a client socket
to become writeable again. Right now we have to either patch the Xorg
code, or resort to polling. I'd very much prefer if we could hook into
the main select loop properly. :)
Please see the attached patch and see if this seems like a reasonable
way to solve this. I thought about changing the BlockHandlerProc
definition, but it is exposed in application headers so it didn't seem
safe to touch.
PS. Please cc me on any replies, and allow for some delay in responses
on account of holidays. :)
Rgds
--
Pierre Ossman Software Development
Cendio AB https://cendio.com
Teknikringen 8 https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
From 97775d5d757d6ec383e3a94967c579d5e62bd97d Mon Sep 17 00:00:00 2001
From: Pierre Ossman oss...@cendio.se
Date: Fri, 19 Dec 2014 13:03:51 +0100
Subject: [PATCH] Extend block and wakeup handling to cover writes as well
Although not as common as waiting on data to read, some modules also
need to be able to gracefully wait for the ability to write data to
some socket they own. This allows them to do that without a lot of
ugly trickery.
---
Xext/sleepuntil.c | 5 +--
Xext/sync.c | 12 ---
config/udev.c | 6 ++--
dix/dixfonts.c | 6 ++--
dix/dixutils.c | 58 +
hw/vfb/InitOutput.c | 2 +-
hw/xfree86/common/xf86Init.c| 2 +-
hw/xfree86/dri/dri.c| 3 +-
hw/xfree86/drivers/modesetting/vblank.c | 4 +--
hw/xnest/Init.c | 3 +-
include/dix.h | 18 ++
miext/shadow/shadow.c | 4 +--
os/WaitFor.c| 25 +-
os/xdmcp.c | 4 +--
14 files changed, 96 insertions(+), 56 deletions(-)
diff --git a/Xext/sleepuntil.c b/Xext/sleepuntil.c
index 993c028..bff7951 100644
--- a/Xext/sleepuntil.c
+++ b/Xext/sleepuntil.c
@@ -97,7 +97,7 @@ ClientSleepUntil(ClientPtr client,
if (!BlockHandlerRegistered) {
if (!RegisterBlockAndWakeupHandlers(SertafiedBlockHandler,
SertafiedWakeupHandler,
-(void *) 0)) {
+NULL, NULL, NULL)) {
free(pRequest);
return FALSE;
}
@@ -203,7 +203,8 @@ SertafiedWakeupHandler(void *data, int i, void *LastSelectMask)
}
if (!pPending) {
RemoveBlockAndWakeupHandlers(SertafiedBlockHandler,
- SertafiedWakeupHandler, (void *) 0);
+ SertafiedWakeupHandler,
+ NULL, NULL, NULL);
BlockHandlerRegistered = FALSE;
}
}
diff --git a/Xext/sync.c b/Xext/sync.c
index ba08cd1..0c70972 100644
--- a/Xext/sync.c
+++ b/Xext/sync.c
@@ -2622,11 +2622,13 @@ ServertimeBracketValues(void *pCounter, CARD64 * pbracket_less,
{
if (!pnext_time pbracket_greater) {
RegisterBlockAndWakeupHandlers(ServertimeBlockHandler,
- ServertimeWakeupHandler, NULL);
+ ServertimeWakeupHandler,
+ NULL, NULL, NULL);
}
else if (pnext_time !pbracket_greater) {
RemoveBlockAndWakeupHandlers(ServertimeBlockHandler,
- ServertimeWakeupHandler, NULL);
+ ServertimeWakeupHandler,
+ NULL, NULL, NULL);
}
pnext_time = pbracket_greater;
}
@@ -2811,14 +2813,16 @@ IdleTimeBracketValues(void *pCounter, CARD64 * pbracket_less,
if (registered !pbracket_less !pbracket_greater) {
RemoveBlockAndWakeupHandlers(IdleTimeBlockHandler,
- IdleTimeWakeupHandler, pCounter);
+ IdleTimeWakeupHandler,
+ NULL, NULL, pCounter);
}
else if (!registered (pbracket_less || pbracket_greater)) {
/* Reset flag must be zero so we don't force a idle timer reset on
the first wakeup */
LastEventTimeToggleResetAll(FALSE);
RegisterBlockAndWakeupHandlers(IdleTimeBlockHandler,
- IdleTimeWakeupHandler, pCounter);
+ IdleTimeWakeupHandler,
+ NULL, NULL, pCounter);
}
priv-value_greater = pbracket_greater;
diff --git a/config/udev.c b/config/udev.c
Re: [RFC] [PATCH] Extend block and wakeup handling to cover writes as well
On Fri, 23 Jan 2015 07:35:04 -0800, Keith Packard wrote: Pierre Ossman oss...@cendio.se writes: On Thu, 25 Dec 2014 13:55:13 -0800, Keith Packard wrote: Pierre Ossman oss...@cendio.se writes: Please see the attached patch and see if this seems like a reasonable way to solve this. I thought about changing the BlockHandlerProc definition, but it is exposed in application headers so it didn't seem safe to touch. I think we should just change the BlockHandlerProc definition to include the write data as well. Either change requires updating every place these functions are used anyways. How about these patches then? Yup, that looks good. Reviewed-by: Keith Packard kei...@keithp.com Great. Can it get queued up for the next releases in that case? Rgds -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? pgpQg0Hw3bS6O.pgp Description: OpenPGP 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: [RFC] [PATCH] Extend block and wakeup handling to cover writes as well
On Thu, 25 Dec 2014 13:55:13 -0800,
Keith Packard wrote:
Pierre Ossman oss...@cendio.se writes:
Please see the attached patch and see if this seems like a reasonable
way to solve this. I thought about changing the BlockHandlerProc
definition, but it is exposed in application headers so it didn't seem
safe to touch.
I think we should just change the BlockHandlerProc definition to include
the write data as well. Either change requires updating every place
these functions are used anyways.
How about these patches then?
Rgds
--
Pierre Ossman Software Development
Cendio AB https://cendio.com
Teknikringen 8 https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
From 86180ca65b5149f4aedc816ee611be0ef442997f Mon Sep 17 00:00:00 2001
From: Pierre Ossman oss...@cendio.se
Date: Fri, 23 Jan 2015 11:52:22 +0100
Subject: [PATCH] Allow block handlers to also track write availability
Signed-off-by: Pierre Ossman oss...@cendio.se
---
Xdefs.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Xdefs.h b/Xdefs.h
index e25a208..b283352 100644
--- a/Xdefs.h
+++ b/Xdefs.h
@@ -103,6 +103,7 @@ typedef struct timeval **OSTimePtr;
typedef void (* BlockHandlerProcPtr)(void * /* blockData */,
OSTimePtr /* pTimeout */,
- void * /* pReadmask */);
+ void * /* pReadmask */,
+ void * /* pWritemask */);
#endif
--
1.9.3
From 1e19405c27d4e4cf697fdcf736942f7a91bd1139 Mon Sep 17 00:00:00 2001
From: Pierre Ossman oss...@cendio.se
Date: Fri, 19 Dec 2014 13:03:51 +0100
Subject: [PATCH] Extend block and wakeup handling to cover writes as well
Although not as common as waiting on data to read, some modules also
need to be able to gracefully wait for the ability to write data to
some socket they own. This allows them to do that without a lot of
ugly trickery.
Signed-off-by: Pierre Ossman oss...@cendio.se
---
Xext/sleepuntil.c | 12
Xext/sync.c | 12
composite/compalloc.c | 5 +++--
config/udev.c | 5 +++--
dix/dixfonts.c | 4 ++--
dix/dixutils.c | 20
exa/exa.c | 8
hw/vfb/InitOutput.c | 6 --
hw/xfree86/common/xf86Events.c | 2 +-
hw/xfree86/common/xf86Priv.h| 2 +-
hw/xfree86/common/xf86VGAarbiter.c | 8
hw/xfree86/common/xf86VGAarbiterPriv.h | 5 +++--
hw/xfree86/dri/dri.c| 18 --
hw/xfree86/dri/dri.h| 11 +++
hw/xfree86/drivers/modesetting/driver.c | 5 +++--
hw/xfree86/drivers/modesetting/vblank.c | 4 ++--
hw/xfree86/modes/xf86Rotate.c | 4 ++--
hw/xnest/Handlers.c | 6 --
hw/xnest/Handlers.h | 5 +++--
include/dix.h | 9 ++---
include/dixfont.h | 3 ++-
include/scrnintstr.h| 6 --
mi/misprite.c | 7 ---
miext/shadow/shadow.c | 4 ++--
os/WaitFor.c| 25 -
os/xdmcp.c | 10 ++
render/animcur.c| 4 ++--
27 files changed, 128 insertions(+), 82 deletions(-)
diff --git a/Xext/sleepuntil.c b/Xext/sleepuntil.c
index 993c028..93e5cc8 100644
--- a/Xext/sleepuntil.c
+++ b/Xext/sleepuntil.c
@@ -65,11 +65,13 @@ static int SertafiedDelete(void * /* value */ ,
);
static void SertafiedBlockHandler(void */* data */ ,
OSTimePtr /* wt */ ,
- void */* LastSelectMask */
+ void */* LastReadMask */ ,
+ void */* LastWriteMask */
);
static void SertafiedWakeupHandler(void * /* data */ ,
int /* i */ ,
- void * /* LastSelectMask */
+ void */* LastReadMask */ ,
+ void */* LastWriteMask */
);
int
@@ -154,7 +156,8 @@ SertafiedDelete(void *value, XID id)
}
static void
-SertafiedBlockHandler(void *data, OSTimePtr wt, void *LastSelectMask)
+SertafiedBlockHandler(void *data, OSTimePtr wt,
+ void *LastReadMask, void *LastWriteMask)
{
SertafiedPtr pReq, pNext;
unsigned long delay;
@@ -186,7 +189,8 @@ SertafiedBlockHandler(void *data, OSTimePtr wt, void *LastSelectMask)
}
static void
-SertafiedWakeupHandler(void *data, int i, void *LastSelectMask
Re: [RFC] [PATCH] Extend block and wakeup handling to cover writes as well
On Mon, 26 Jan 2015 10:25:48 -0800 Keith Packard <kei...@keithp.com> wrote: > Pierre Ossman <oss...@cendio.se> writes: > > > Great. Can it get queued up for the next releases in that case? > > It's in my list, yes. > Ping. Still in the list? :) Rgds -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: Performance drop when updating > 500k pixels
On 16/01/17 21:36, Adam Jackson wrote: On Fri, 2017-01-13 at 15:07 +0100, Pierre Ossman wrote: I'll answer myself here... This seems to be a CPU cache issue. Below this limit I see: 4,469,985 cache-misses:u#0.336 % of all cache refs 35,279,259,258 instructions:u#1.70 insn per cycle (100.00%) Above the limit I get: 194,571,782 cache-misses:u# 30.322 % of all cache refs 18,084,891,734 instructions:u#0.73 insn per cycle So no wonder things take a turn for the worse. I'll have to think a bit on how to make this more efficient. Ideas are always welcome. Seems like a job for non-temporal stores? Will that help though? I suspect the performance hit is when reading back the buffer, not writing it. The test is rather simplistic and writes linearly to memory, so write-combining should take car of the store portion. Perhaps some clever way of making the X server upload the data to the graphics card in tandem with the application generating it? Or perhaps I should switch to OpenGL and do it all client side for things like this? Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc
On 04/10/16 17:45, Hans de Goede wrote: If been working for 7 days on a row now to get 1.19 in shape for Fedora 25, so I'm afraid that the v2 of this patch I'm working on is going to be a take it or leave it offer from my pov. You are of course more then welcome to improve upon the patch yourself. Alright. I'll have a look at doing the finishing touches. Thanks for the patch. :) While on the subject of write-ready notification I noticed what I believe is a bug in tigervnc-1.7.0/common/rdr/FdOutStream.cxx: FdOutStream::flush(). When the stream is nonblocking and writeWithTimeout() returns 0, then flush() will simply retry again (and again and again), if I read the code correctly), so it will *busy* wait until all data is written. I'll have a look. It may simply be that flush() is defined as always blocking. I'll check who calls it. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc
On 03/10/16 17:59, Hans de Goede wrote: Hello tigervnc devs, As part of updating Fedora to xserver 1.19 I've written a tiger vnc patch to make tigervnc work with xserver 1.19. Nice, Thanks. :) Since xserver 1.19 switches from select to poll the changes are non trivial and require some #ifdef-s. The new code is a lot cleaner then the old code though, which is good. And with server 1.19 the os/WaitFor.c changes are no longer necessary. Very nice. However I'm not a fan of the large amount of duplicate code we're dragging around now. I'd prefer to switch everything over to the new model, and put something in vncBlockHandler.c that emulates the new API on top of the old system. Attached is a tigervnc-1.7.0 patch, as well as a patch to apply to the xserver sources to patch in the tigervnc ext and hw/vnc dir into the buidlsys. +#include "os.h" NAK on this I'm afraid. Using Xorg headers in C++ has been endless grief. Hence the wrappers in both directions. I'd say put this in vncBlockHandler.c and tie it up via vncExtInit.cc. + SetNotifyFd(sock->getFd(), HandleSocketFd, X_NOTIFY_READ, this); I don't see any code setting X_NOTIFY_WRITE? --- xserver/include/os.h~ 2016-10-03 09:07:29.0 +0200 +++ xserver/include/os.h2016-10-03 14:13:00.013654506 +0200 @@ -621,7 +621,7 @@ extern _X_EXPORT void LogClose(enum ExitCode error); extern _X_EXPORT Bool -LogSetParameter(LogParameter param, int value); +LogSetParameter(enum _LogParameter param, int value); extern _X_EXPORT void LogVWrite(int verb, const char *f, va_list args) _X_ATTRIBUTE_PRINTF(2, 0); Is this a fix that's meant to go upstream at some point? Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: [PATCH] Handle animated cursor on shared sprite
This was causing crashes in TigerVNC with Chrome/Chromium. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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] Handle animated cursor on shared sprite
Sprites (and hence cursors) can be shared between multiple devices.
However the animation code was not prepared for this and could wind
up in a case where it would continue to animate a free:d cursor.
---
include/inputstr.h | 14 -
render/animcur.c | 85 +-
2 files changed, 51 insertions(+), 48 deletions(-)
diff --git a/include/inputstr.h b/include/inputstr.h
index 568f5f9..a485f5e 100644
--- a/include/inputstr.h
+++ b/include/inputstr.h
@@ -246,6 +246,12 @@ typedef struct _SpriteRec {
ScreenPtr pEnqueueScreen;
ScreenPtr pDequeueScreen;
+/* keep states for animated cursor */
+struct {
+ScreenPtr pScreen;
+int elt;
+CARD32 time;
+} anim;
} SpriteRec;
typedef struct _KeyClassRec {
@@ -509,14 +515,6 @@ typedef struct _SpriteInfoRec {
DeviceIntPtr paired;/* The paired device. Keyboard if
spriteOwner is TRUE, otherwise the
pointer that owns the sprite. */
-
-/* keep states for animated cursor */
-struct {
-CursorPtr pCursor;
-ScreenPtr pScreen;
-int elt;
-CARD32 time;
-} anim;
} SpriteInfoRec, *SpriteInfoPtr;
/* device types */
diff --git a/render/animcur.c b/render/animcur.c
index 52e6b8b..fc87e0d 100644
--- a/render/animcur.c
+++ b/render/animcur.c
@@ -142,35 +142,42 @@ AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void
*arg)
CARD32 soonest = ~0; /* earliest time to wakeup again */
for (dev = inputInfo.devices; dev; dev = dev->next) {
-if (IsPointerDevice(dev) && pScreen == dev->spriteInfo->anim.pScreen) {
-if (!activeDevice)
-activeDevice = TRUE;
-
-if ((INT32) (now - dev->spriteInfo->anim.time) >= 0) {
-AnimCurPtr ac = GetAnimCur(dev->spriteInfo->anim.pCursor);
-int elt = (dev->spriteInfo->anim.elt + 1) % ac->nelt;
-DisplayCursorProcPtr DisplayCursor;
-
-/*
- * Not a simple Unwrap/Wrap as this
- * isn't called along the DisplayCursor
- * wrapper chain.
- */
-DisplayCursor = pScreen->DisplayCursor;
-pScreen->DisplayCursor = as->DisplayCursor;
-(void) (*pScreen->DisplayCursor) (dev,
- pScreen,
- ac->elts[elt].pCursor);
-as->DisplayCursor = pScreen->DisplayCursor;
-pScreen->DisplayCursor = DisplayCursor;
-
-dev->spriteInfo->anim.elt = elt;
-dev->spriteInfo->anim.time = now + ac->elts[elt].delay;
-}
-
-if (soonest > dev->spriteInfo->anim.time)
-soonest = dev->spriteInfo->anim.time;
+if (!IsPointerDevice(dev))
+continue;
+if (!dev->spriteInfo->spriteOwner)
+continue;
+if (!IsAnimCur(dev->spriteInfo->sprite->current))
+continue;
+if (pScreen != dev->spriteInfo->sprite->anim.pScreen)
+continue;
+
+if (!activeDevice)
+activeDevice = TRUE;
+
+if ((INT32) (now - dev->spriteInfo->sprite->anim.time) >= 0) {
+AnimCurPtr ac = GetAnimCur(dev->spriteInfo->sprite->current);
+int elt = (dev->spriteInfo->sprite->anim.elt + 1) % ac->nelt;
+DisplayCursorProcPtr DisplayCursor;
+
+/*
+ * Not a simple Unwrap/Wrap as this
+ * isn't called along the DisplayCursor
+ * wrapper chain.
+ */
+DisplayCursor = pScreen->DisplayCursor;
+pScreen->DisplayCursor = as->DisplayCursor;
+(void) (*pScreen->DisplayCursor) (dev,
+ pScreen,
+ ac->elts[elt].pCursor);
+as->DisplayCursor = pScreen->DisplayCursor;
+pScreen->DisplayCursor = DisplayCursor;
+
+dev->spriteInfo->sprite->anim.elt = elt;
+dev->spriteInfo->sprite->anim.time = now + ac->elts[elt].delay;
}
+
+if (soonest > dev->spriteInfo->sprite->anim.time)
+soonest = dev->spriteInfo->sprite->anim.time;
}
if (activeDevice)
@@ -192,17 +199,17 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr
pScreen, CursorPtr pCursor)
Unwrap(as, pScreen, DisplayCursor);
if (IsAnimCur(pCursor)) {
-if (pCursor != pDev->spriteInfo->anim.pCursor) {
+if (pDev->spriteInfo->spriteOwner &&
+(pCursor != pDev->spriteInfo->sprite->current)) {
AnimCurPtr ac = GetAnimCur(pCursor);
ret = (*pScreen->DisplayCursor)
(pDev, pScreen, ac->elts[0].pCursor);
if (ret) {
-pDev->spriteInfo->anim.elt = 0;
-
Re: [PATCH] Handle animated cursor on shared sprite
On 12/10/16 18:05, Emil Velikov wrote: Note that this changes the ABI so it might not be good for the point releases. Unless one wants to make things extra 'fun' for binary only drivers ;-) Might be a safe ABI change though as it only manipulates elements at the end of the structures, and only things drivers shouldn't access anyway. So it's only an issue if they depend on the size of those structs. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: [PATCH] Handle animated cursor on shared sprite
+if (!activeDevice) +activeDevice = TRUE; why the check here ? just set. Historical reasons I guess. This is not code I changed, just re-indented. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: Performance drop when updating > 500k pixels
On 12/01/17 13:53, Pierre Ossman wrote: > > For small updates I see performance growing logarithmically, measured in > pixels/second. At around 500k pixels it starts leveling off at ~1.5 > Gpixels/second. After that the performance starts dropping linearly as > the update grows. I'll answer myself here... This seems to be a CPU cache issue. Below this limit I see: 4,469,985 cache-misses:u#0.336 % of all cache refs 35,279,259,258 instructions:u#1.70 insn per cycle (100.00%) Above the limit I get: 194,571,782 cache-misses:u# 30.322 % of all cache refs 18,084,891,734 instructions:u#0.73 insn per cycle So no wonder things take a turn for the worse. I'll have to think a bit on how to make this more efficient. Ideas are always welcome. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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
Performance drop when updating > 500k pixels
I'm hitting some odd performance glitch here that I hope someone could provide some insight in to. I've been experimenting with getting pixels on the screen as efficiently as possible in TigerVNC. As part of that I made a test program that tries to draw things as quickly as possible (some XSync()s thrown in to avoid queue build-up though). This looks sane on some systems, but on my workstation I'm getting some rather odd results: For small updates I see performance growing logarithmically, measured in pixels/second. At around 500k pixels it starts leveling off at ~1.5 Gpixels/second. After that the performance starts dropping linearly as the update grows. The method uses is XShmPutPixmap(). Splitting up the transfer in to multiple calls doesn't seem to help. This is a Fedora 25 with an Intel E3-1200 card. Wayland vs X11 doesn't seem to matter much. Ideas? Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc
On 05/01/17 17:44, ipilc...@redhat.com wrote: On Wednesday, October 5, 2016 at 4:13:48 AM UTC-5, Pierre Ossman wrote: Alright. I'll have a look at doing the finishing touches. Thanks for the patch. :) FYI, there seems to be an issue with either the patch or v1.19 itself. https://bugzilla.redhat.com/show_bug.cgi?id=1408724 Urgh. I have no 1.19 system yet, so hopefully someone else can have a look. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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: [tigervnc-devel] PATCH: Add xorg-xserver 1.19 support to tigervnc
On 09/01/17 20:15, Hans de Goede wrote: Yes I agree that would be better, Pierre, can you take care of merging Alan's improved version ? All done and available on master now. Thanks for fixing this. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? ___ 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
DRI3 damage tracking?
Hi everyone, I'm playing around with adding DRI3 support to Xvnc, so that OpenGL and Vulkan can be accelerated for headless VNC sessions. Everything seems to be working, but there is one inefficiency that I'm trying to resolve, and that's minimising the shuffling of data between the GPU and CPU. The X server can follow what it modifies, so knowing what to push back to the GPU is fairly straightforward. But I cannot find any details about the other direction. I.e. what the DRI3 clients modify via direct rendering. Presently, I have to assume that everything that the X server wants to read is modified, and always read that back from the GPU. So, is there some information the X server can see about what has been modified? Or perhaps at least if the buffer has been touched at all since the last synchronisation? Ideally, the copying could be completely avoided by accessing the same memory. But apparently¹, gbm_bo_map() isn't guaranteed to actually map memory. ¹ https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/945#note_1483370 Regards, -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
Re: DRI3 damage tracking?
On 27/02/2024 13:03, Enrico Weigelt, metux IT consult wrote: I'm confused: what exactly are you reading back ? To my understanding, the client renders into some buffer and at some point tells the Xserver to copy that buffer (or region of it) into the window. I'm not deep into DRI/GLX, but I'd assume it doens't always need to compose the whole buffer / window area. That's not the whole picture, unfortunately. To start with, what the client wants to draw (CopyArea/Present) is not the same thing as what it has actually modified. It might not have modified anything and only wants to copy from an off-screen Pixmap to a window because of an Expose event. But the more important part is what happens when the server would like to write to the buffer. E.g., "draw a line from 5,5 to 200,200". The server currently has no idea if the client has rendered anything to the buffer. So it first has to read back the 5,5 to 200,200 rectangle. Then draw the line. Then write back the same rectangle. Only then can we be sure the client has seen our new line, and that we didn't use a stale copy of the Pixmap data and reverted any other changes the client did by itself. Regards -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
Re: DRI3 damage tracking?
On 27/02/2024 17:23, Enrico Weigelt, metux IT consult wrote: But the more important part is what happens when the server would like to write to the buffer. Now I'm even more confused: why should the server want to write in the same buffer ? If I'm not completely mistaken, the client renders to some buffer (via GL/DRI) and then tells the server to compose that buffer into the window (not sure whether it also works w/ other drawables like pixmaps). The client is not limited to using that buffer as a source. It can also tell the X server to use that buffer as a destination. Regards, -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköpinghttps://facebook.com/ThinLinc Phone: +46-13-214600 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
