Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mar 5, 2010, at 10:09 PM, Jesse Barnes wrote:
I just fixed these up and pushed them into the tree along with another
fix for handling offscreen drawables better. Tests indicate that OML
swap divisor/remainder stuff is working correctly now.
Thanks for doing that. But stuff got seriously garbled in
I830ScheduleWaitMSC() inside src/i830_dri.c - this won't work
correctly for any glXWaitForMscOML(target_msc, divisor, remainder)
call, except for the special cases with divisor == 0!
This passage...
/*
* If divisor is zero, or current_msc is smaller than
target_msc,
* we just need to make sure target_msc passes before
waking up the
* client.
*/
if (divisor == 0) {
...
should read as...
if (divisor == 0 || current_msc target_msc) {
This passage ...
/*
* If the calculated remainder and the condition isn't
satisified, it
* means we've passed the last point where seq % divisor ==
remainder,
* so we need to wait for the next time that will happen.
*/
if ((current_msc % divisor) != remainder)
vbl.request.sequence += divisor;
... should be replaced by ...
vbl.request.sequence = current_msc - (current_msc % divisor) +
remainder;
/*
* If calculated remainder is larger than requested remainder,
it means
* we've passed the last point where seq % divisor == remainder,
so we need
* to wait for the next time that will happen.
*/
if ((current_msc % divisor) remainder)
vbl.request.sequence += divisor;
Other than that, it's fine.
Oh and in I830DRI2ScheduleSwap() , this statement ...
if (pipe 0)
vbl.request.type |= DRM_VBLANK_SECONDARY;
... accidentally got copied twice into the if () clause, which is not
harmful, but a bit redundant :-)
best,
-mario
*
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany
e-mail: mario.klei...@tuebingen.mpg.de
office: +49 (0)7071/601-1623
fax:+49 (0)7071/601-616
www:http://www.kyb.tuebingen.mpg.de/~kleinerm
*
For a successful technology, reality must take precedence
over public relations, for Nature cannot be fooled.
(Richard Feynman)
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] DRI2: fixup handling of last_swap_target
On Sun, 2010-03-07 at 08:44 +0100, Mario Kleiner wrote: Your deferred pPriv deletion logic looks now good to me. But if deletion is deferred then i think almost all public functions inside dri2.c should return 'BadDrawable' not only for pPriv == NULL but also for pPriv-refCount == 0, e.g., DRI2SwapBuffers, DRI2GetMSC, ... From the viewpoint of client code, a refCount == 0 is the same as a destroyed drawable. For all practical purposes it is already gone. Maybe it would make sense to consolidate the (pPriv == NULL || pPriv-refCount == 0) check into a macro or function, something like DRI2IsDrawableAlive(pPriv); and then call that in most DRI2xxx functions? Sounds like a good idea to me FWIW. Speaking of consolidation: Is it just me, or do the X server - driver interfaces you guys have been discussing seem to require way too much brains in the drivers which should rather be centralized in the server? (I admit I've only been skimming the code and discussion, so my impression may be wrong) -- Earthling Michel Dänzer |http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PULL] Minor bug-fixes discovered by static analysis.
On Sat, 2010-03-06 at 19:19 +0100, ext Matt Turner wrote:
On Fri, Mar 5, 2010 at 7:12 AM, Oliver McFadden
oliver.mcfad...@nokia.com wrote:
are available in the git repository at:
git://gitorious.org/omcfadde/xserver.git analysis
Do you not have an account on FreeDesktop.org?
I do, but all of my Nokia X.org work is hosted on Gitorious. Shouldn't
be a problem though...
Oliver McFadden (5):
exa: exaFinishAccess: Overrun of static array pExaScr-access of
size 6 at position 6 with index variable i
This looks like it's got a typo in it. 'pPixmap),);' - notice the extra
comma.
No, this is correct. Check the define for EXA_FatalErrorDebugWithRet:
#ifdef DEBUG
#define EXA_FatalErrorDebug(x) FatalError x
#define EXA_FatalErrorDebugWithRet(x, ret) FatalError x
#else
#define EXA_FatalErrorDebug(x) ErrorF x
#define EXA_FatalErrorDebugWithRet(x, ret) \
do { \
ErrorF x; \
return ret; \
} while (0)
#endif
So it will evaluate as: { FatalError(...); return ; }
fb: fbFinishScreenInit: leaked_storage: Variable (visuals|depths)
goes out of scope
Reviewed-by: Matt Turner matts...@gmail.com
parser: xf86readConfigFile: unreachable: This code cannot be reached:
free(val.str);
Not sure I understand this one.
Error() was called before free(), thus free() is unreachable code, and
val.str will never be freed and NULL-ed.
common: xf86Configure: alloc_strlen: Allocated memory does not have
space for the terminating NUL of the string
Reviewed-by: Matt Turner matts...@gmail.com
Xext: IdleTimeBlockHandler: unsigned_compare: Comparing unsigned less
than zero is never true. timeout 0UL
I'm not sure this does it exactly. Up at line 2320, we have 'unsigned
long timeout = -1;'.
Yes, you're right this one needs further investigation. However if you
could check the ones that I've explained above and add your Reviewed-by
tags, I can send a new pull request with the patches.
I'll also check into the unsigned_compare patch further.
Xext/sync.c |4 +---
exa/exa.c |4 ++--
fb/fbscreen.c |4
hw/xfree86/common/xf86Configure.c |2 +-
hw/xfree86/parser/read.c |4 ++--
5 files changed, 10 insertions(+), 8 deletions(-)
Matt Turner
Thanks,
-- Oliver.
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] DRI2: fixup handling of last_swap_target
On Sun, 7 Mar 2010 08:44:51 +0100
Mario Kleiner mario.klei...@tuebingen.mpg.de wrote:
On Mar 5, 2010, at 6:50 PM, Jesse Barnes wrote:
Ok pushed fixes for these issues to the repo above. I know you've put
a lot of time into this already, but can you take another look to make
sure I got everything?
No problem.
Note I left the swap interval default at 1
since GLX_MESA_swap_control should let us set it to 0 to get
unthrottled behavior, which I just fixed and tested.
Agreed, that's ok. GLX_MESA_swap_control is a way to do it.
The current state looks almost good, esp. your fixes to
DRI2SwapBuffers for swap_interval == 0 and for target_msc == 0.
But a few more things:
In DRI2SwapBuffers, in the body of ...
/* Old DDX or no swap interval, just blit */
if (!ds-ScheduleSwap || !pPriv-swap_interval) {
...
... it calls DRI2SwapComplete and passes target_msc as the 'msc'
value of swap completion. Would be probably better to pass a zero
value, just as it is done in the Intel DDX when the swap doesn't
happen at all at the requested target_msc, but rather immediately due
to some problems. If ds-ScheduleSwap isn't available then the 'msc'
is basically undefined and unknown, so zero is a better return value.
In the !pPriv-swap_interval case one could call a *ds-getMSC() to
get a more meaningful return value for 'msc'. Don't know if it's
worth the effort, but a zero return would be better than 'target_msc'
imho.
Right, good idea. That way software using the new event won't get
bogus values with old DDXes.
We need to add another patch to Mesa. Mesa translates a glXSwapBuffers
() call into a DRI2SwapBuffers() call with target_msc, divisor and
remainder all zero and DRI2SwapBuffers takes this as hint that
glXSwapBuffers behaviour is requested. However these are also legal
arguments to a glXSwapBuffersMscOML(..., ,0,0,0) call, which has to
behave differently than a glXSwapBuffers call -- In this case,
DRI2SwapBuffers would confuse this with a glXSwapBuffers request and
do the wrong thing.
I'd propose adding this line to the __glXSwapBuffersMscOML() routine
in mesa's /src/glx/x11/glxcmds.c file:
if (target_msc == 0 divisor == 0 remainder == 0) remainder = 1;
- if target_msc and divisor are zero, then the remainder is ignored
inside the DRI implementation, so the actual value of remainder
doesn't matter for swap behaviour, but can be used to disambiguate
glXSwapBuffers vs. glXSwapBuffersMsc... for a glXSwapBuffersMscOML
(0,0,0) call.
Ok, I'll have to check the behaviors again, but this sounds reasonable.
Your deferred pPriv deletion logic looks now good to me. But if
deletion is deferred then i think almost all public functions
inside dri2.c should return 'BadDrawable' not only for pPriv == NULL
but also for pPriv-refCount == 0, e.g., DRI2SwapBuffers,
DRI2GetMSC, ... From the viewpoint of client code, a refCount == 0 is
the same as a destroyed drawable. For all practical purposes it is
already gone. Maybe it would make sense to consolidate the (pPriv ==
NULL || pPriv-refCount == 0) check into a macro or function,
something like DRI2IsDrawableAlive(pPriv); and then call that in most
DRI2xxx functions?
Yeah, that would make things cleaner, I'll do that.
I believe there's also another problem with the pPriv-blockedClient
logic. The implementation only keeps track of blockedClient, but not
for the reason why the client blocked. This can cause trouble in
DRI2WakeClient. Consider this example which i think would cause the
client to hang:
1. Assume current msc is 1000 and there are 2 threads in the client.
// Thread 1: Request swap at target_msc 1010:
2. glXSwapBuffersMscOML(dpy, drawable, 1010, 0, 0);
// Thread 2: Request wait until target_msc 2000:
3. glXWaitForMscOML(dpu, drawable, 2000, 0, 0, );
4. Thread 1: Calls XDestroyWindow(dpy, drawable);
Step 2 will schedule a swap for msc = 1010.
Step 3 will pPriv-blockedClient = client and IgnoreClient(client)
the client and schedule a wakeup at msc = 2000.
The xconnection dpy is now blocked.
Step 4 doesn't execute yet, because the xconnection is blocked.
Now at msc = 1010 - swap completes - DRI2SwapComplete -
DRI2WakeClient - executes the elseif branch:
...
} else if (pPriv-target_sbc == -1) {
if (pPriv-blockedClient)
AttendClient(pPriv-blockedClient);
pPriv-blockedClient = NULL;
...
- this will AttendClient() the client and release pPriv-
blockedClient, i.e., release at msc 1010 although the block was
meant to be released at msc 2000. Thread 2 still waits for a _XReply
before it can return from the glXWaitForMscOML...
Now step 4 can execute due to the unfrozen xconnection dpy.
DRI2DestroyDrawable() executes and DRI2FreeDrawable() gets called
becuase there are no pending flips or blockedClients anymore.
Re: [PATCH] DRI2: fixup handling of last_swap_target
On Sun, 07 Mar 2010 15:38:24 +0100 Michel Dänzer mic...@daenzer.net wrote: On Sun, 2010-03-07 at 08:44 +0100, Mario Kleiner wrote: Your deferred pPriv deletion logic looks now good to me. But if deletion is deferred then i think almost all public functions inside dri2.c should return 'BadDrawable' not only for pPriv == NULL but also for pPriv-refCount == 0, e.g., DRI2SwapBuffers, DRI2GetMSC, ... From the viewpoint of client code, a refCount == 0 is the same as a destroyed drawable. For all practical purposes it is already gone. Maybe it would make sense to consolidate the (pPriv == NULL || pPriv-refCount == 0) check into a macro or function, something like DRI2IsDrawableAlive(pPriv); and then call that in most DRI2xxx functions? Sounds like a good idea to me FWIW. Speaking of consolidation: Is it just me, or do the X server - driver interfaces you guys have been discussing seem to require way too much brains in the drivers which should rather be centralized in the server? (I admit I've only been skimming the code and discussion, so my impression may be wrong) We've gone back and forth on that a bit. My initial version had more logic (all the event handling in particular) in the server. The main problem with that was it created an unintuitive split for doing things like page flipping. But more than that, the vblank count and DRM handling really belongs in the DDX, and splitting that up also caused some weirdness in terms of the interfaces. The interfaces we have now do require the DDX to do a bit of work, but they also allow it to do nice things like page flipping and virtualize the vblank count if they want. That said, I'd have no problem factoring out common functionality as other drivers grow support for these features. At the very least we could probably provide a DRI2 driver module that did some DRM interactions (so as to keep those bits of code out of the core DRI2 module). -- Jesse Barnes, Intel Open Source Technology Center ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Sun, 7 Mar 2010 09:15:46 +0100
Mario Kleiner mario.klei...@tuebingen.mpg.de wrote:
On Mar 5, 2010, at 10:09 PM, Jesse Barnes wrote:
I just fixed these up and pushed them into the tree along with another
fix for handling offscreen drawables better. Tests indicate that OML
swap divisor/remainder stuff is working correctly now.
Thanks for doing that. But stuff got seriously garbled in
I830ScheduleWaitMSC() inside src/i830_dri.c - this won't work
correctly for any glXWaitForMscOML(target_msc, divisor, remainder)
call, except for the special cases with divisor == 0!
This passage...
/*
* If divisor is zero, or current_msc is smaller than
target_msc,
* we just need to make sure target_msc passes before
waking up the
* client.
*/
if (divisor == 0) {
...
should read as...
if (divisor == 0 || current_msc target_msc) {
This passage ...
/*
* If the calculated remainder and the condition isn't
satisified, it
* means we've passed the last point where seq % divisor ==
remainder,
* so we need to wait for the next time that will happen.
*/
if ((current_msc % divisor) != remainder)
vbl.request.sequence += divisor;
... should be replaced by ...
vbl.request.sequence = current_msc - (current_msc % divisor) +
remainder;
/*
* If calculated remainder is larger than requested remainder,
it means
* we've passed the last point where seq % divisor == remainder,
so we need
* to wait for the next time that will happen.
*/
if ((current_msc % divisor) remainder)
vbl.request.sequence += divisor;
Other than that, it's fine.
Oh and in I830DRI2ScheduleSwap() , this statement ...
if (pipe 0)
vbl.request.type |= DRM_VBLANK_SECONDARY;
... accidentally got copied twice into the if () clause, which is not
harmful, but a bit redundant :-)
Arg, I did botch that patch. And of course I only tested the swap
buffers behavior and not OML's WaitMSC so I didn't catch it. I'll
improve the test and push the fix.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] xkeyboard-config: updated geometry description of model pc105
Hi, Scanning through the list at http://en.wikipedia.org/wiki/Keyboard_layout shows that the vast majority of 105 key keyboard layouts on this planet feature a tall, non-rectangular Return key (i.e. spanning two rows). These keyboards are therefore inaccurately represented by their current geometry description in xkeyboard-config. This becomes apparent in applications that rely on said geometry description, such as xkbprint and libgnomekbd. The attached patch solves the problem by changing shape and position of the Return key as well as BKSL, which is moved to the lower left of Return. A rectangular approximate outline of the Return key as demanded by the XKB API documentation is included. Regards, Jakob PS: Please CC me on any replies as I am not subscribed to this list. xkeyboard-config-pc105-geometry.patch Description: Binary data ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Summer of Code ideas
Hey guys, Summer of code is coming on us. I need you to help fill/correct the ideas page at http://wiki.x.org/wiki/SummerOfCodeIdeas I moved some 2009 ideas which are still relevant into 2010. But I don't really know about the state of subsystems I don't follow, for example I know nothing about xcb or input. So I'd be grateful if you could update it and move the relevant ideas to 2010, or add new ideas. Thanks, Stephane ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xkeyboard-config: updated geometry description of model pc105
Twas brillig at 21:43:00 07.03.2010 UTC+01 when jakob.kumme...@googlemail.com did gyre and gimble: JK Scanning through the list at JK http://en.wikipedia.org/wiki/Keyboard_layout shows that the vast JK majority of 105 key keyboard layouts on this planet feature a tall, JK non-rectangular Return key (i.e. spanning two rows). Mostly depends on language of keyboard. It makes sense to create separate keyboard layout. JK The attached patch solves the problem by changing shape and JK position of the Return key as well as BKSL, which is moved to the JK lower left of Return. Some of such keyboards have backslash to the left of Backspace button, which is is square, not rectangular in this situation. -- http://fossarchy.blogspot.com/ pgp9EmEBTcMrg.pgp Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: Summer of Code ideas
On Mon, 8 Mar 2010 08:19:27 am Stephane Marchesin wrote: Hey guys, Summer of code is coming on us. I need you to help fill/correct the ideas page at http://wiki.x.org/wiki/SummerOfCodeIdeas I moved some 2009 ideas which are still relevant into 2010. But I don't really know about the state of subsystems I don't follow, for example I know nothing about xcb or input. So I'd be grateful if you could update it and move the relevant ideas to 2010, or add new ideas. Thanks, Stephane ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel One thing I'd like to see tackled is adding composite support to the xinerama extension. ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Ubuntu with DisplayLink
I found your forum online, and hope that you'll be able to help. I'm a life-long Windows user who just recently took the plunge and switched to Ubuntu 9.1 . All is fine except that I have been unable to get my Atlona HDPIX to work correctly with Displaylink drivers for Linux/Ubuntu 9.1. I installed libdlo-0.1.2 and it does recognize the unit and begins to do a self test. My extended display correctly passes the first 2 tests but then gets stuck on the screenful of DisplayLink logos. At that point the system locks, and I need to literally unplug everything and reboot. My only thought is that during testing the drivers are attempting to use a screen resolution that my 26 LCD doesn't care for. I know I need to make changes to the settings in my XORG.CONF file but not sure what to do. I'm using a 2.7 mhz system with 8GB of RAM. What would you recommend I do? Don DiMuccio Cranston, RI 401-944-5514 ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xkeyboard-config: updated geometry description of model pc105
Twas brillig at 23:09:00 07.03.2010 UTC+01 when jakob.kumme...@googlemail.com did gyre and gimble: JK but since I don't have a collection of international keyboards in JK my basement and don't know any more complete or more accurate JK listing, it's the best source of data I have available. I possess Russian 104-key keyboards with all the variants discussed: with single-row Return key, with tall Return and small Backspace and with tall Return; with tall Return, wide Backspace and backslash next to ' key. So, ehm, it's not really easy to select proper layout even for single language. Given this experience layouts in wikipedia article look random for me (especially 105 vs. 104 when 105th key is just / keys). (UK keyboard look strange too - I had laptop with UK layout and it 105th key was used as , not as |\). JK 26 to 2 is what I consider a large enough majority to change the JK default geometry description. Did you count frequency of each keyboard? I bet US keyboards are much more common than the Albanian or Turkish. -- http://fossarchy.blogspot.com/ pgpK9xQzQZPew.pgp Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xkeyboard-config: updated geometry description of model pc105
Twas brillig at 23:46:34 07.03.2010 UTC+01 when jakob.kumme...@googlemail.com did gyre and gimble: JK I know from personal experience that usually French, German, JK Spanish, Swiss and UK keyboards have 105 keys with a tall Return JK key (as in the Wiki article). Uhm, now I remember that most of keyboards with the 105th key I've seen have backslash near to ' character and tall Return. No objections then. -- http://fossarchy.blogspot.com/ pgputyMS6xssm.pgp Description: PGP signature ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH synaptics] Only return Success on mode switch for mode Relative.
We don't do Absolute mode in synaptics, so let's not return Success when a
client tries to switch the touchpad to absolute mode.
Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
src/synaptics.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/synaptics.c b/src/synaptics.c
index bf4a28b..c28988f 100644
--- a/src/synaptics.c
+++ b/src/synaptics.c
@@ -2372,7 +2372,7 @@ static int
SwitchMode(ClientPtr client, DeviceIntPtr dev, int mode)
{
DBG(3, SwitchMode called\n);
-return Success;
+return (mode == Relative) ? Success : XI_BadMode;
}
static Bool
--
1.6.6.1
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xinput] test-xi2: print event type name as well.
Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
src/test_xi2.c | 31 ++-
1 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/src/test_xi2.c b/src/test_xi2.c
index 53d984f..6fdc4ad 100644
--- a/src/test_xi2.c
+++ b/src/test_xi2.c
@@ -258,6 +258,35 @@ test_sync_grab(Display *display, Window win)
printf(Done\n);
}
+static const char* type_to_name(int evtype)
+{
+const char *name;
+
+switch(evtype) {
+case XI_DeviceChanged:name = DeviceChanged; break;
+case XI_KeyPress: name = KeyPress;break;
+case XI_KeyRelease: name = KeyRelease; break;
+case XI_ButtonPress: name = ButtonPress; break;
+case XI_ButtonRelease:name = ButtonRelease; break;
+case XI_Motion: name = Motion; break;
+case XI_Enter:name = Enter; break;
+case XI_Leave:name = Leave; break;
+case XI_FocusIn: name = FocusIn; break;
+case XI_FocusOut: name = FocusOut;break;
+case XI_HierarchyChanged: name = HierarchyChanged;break;
+case XI_PropertyEvent:name = PropertyEvent; break;
+case XI_RawKeyPress: name = RawKeyPress; break;
+case XI_RawKeyRelease:name = RawKeyRelease; break;
+case XI_RawButtonPress: name = RawButtonPress; break;
+case XI_RawButtonRelease: name = RawButtonRelease;break;
+case XI_RawMotion:name = RawMotion; break;
+default:
+ name = unknown event type; break;
+}
+return name;
+}
+
+
int
test_xi2(Display *display,
int argc,
@@ -341,7 +370,7 @@ test_xi2(Display*display,
cookie-type == GenericEvent
cookie-extension == xi_opcode)
{
-printf(EVENT type %d\n, cookie-evtype);
+printf(EVENT type %d (%s)\n, cookie-evtype,
type_to_name(cookie-evtype));
switch (cookie-evtype)
{
case XI_DeviceChanged:
--
1.6.6.1
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] DRI2: fixup handling of last_swap_target
Great. I'll try to quickly go over it again when you're done.
I think i found another issue or two:
In DRI2SwapBuffers() you have this code for glXSwapBuffersMscOML
handling:
...
} else {
/* glXSwapBuffersMscOML could have a 0 target_msc, honor it */
*swap_target = target_msc;
}
Now while my previous patch was to restrictive and didn't allow to
honor a 0 target_msc, this one is too liberal. It would allow a
client to schedule many swaps for the same target_msc value, e.g.,
for target_msc = 1000:
while (1) glXSwapBuffersMscOML(dpy, drawable, 1000, 0, 0);
- This would cause the DRM at vblank 1000 to deliver many swap-
vblank events during the same vblank, which i assume can cause
trouble with timestamping of the swaps and probably server
responsiveness? In the pageflipped case, you'd schedule one pageflip
successfully and then if i understand correctly, on the 2nd call for
this vblank the pageflip ioctl would fail with something like EAGAIN
or EBUSY, which would cause the ddx to fall back to blits. If i
understand correctly from I830DRI2FrameEventHandler -
I830DRI2CopyRegion, blit requests get submitted to the command fifo,
drmCommandNone(intel-drmSubFD, DRM_I915_GEM_THROTTLE); gets called,
then DRI2SwapComplete gets called with the 'msc' value of delivery of
the vblank event - in this case with 1000.
Not sure if the GEM_THROTTLE'ing somehow takes care of such issues -
or the fact that the gpu command fifo must be full at some time, but
is there something that would prevent many calls to DRI2SwapComplete
with exactly the same 'msc' / vblank value and 'ust' timestamp
despite the fact that most of the swaps won't happen at that msc but
later? Would any of the throttling here - or something like a full
fifo - cause the server to stall or get sluggish? I don't know enough
about the implementation, but allowing to deliver many identical
vblank swap events sounds problematic to me.
Maybe we could change above code to something like:
...
} else {
/* glXSwapBuffersMscOML could have a 0 target_msc, honor it */
if (target_msc = pPriv-last_swap_target pPriv-swap_interval)
target_msc = pPriv-last_swap_target + 1;
*swap_target = target_msc;
}
This would not constrain a given target_msc to honor the exact
swap_interval setting - what my previous -wrong- patch did. For a
target_msc of zero etc., i believe this updated target_msc would
result in the same behaviour as the current code. It wouldn't push
the target_msc into the far future, but only make sure it doesn't
get stuck in the past or at a fixed value in the future, basically
only ensuring the only at most one swap per msc increment
requirement of the OML_sync_control spec.
A similar error case that could be kind'a solved by the above would
be wrong ordering of swaprequests:
Assume we're at msc 500, last_swap_target something smaller than
e.g., 1000:
glutSolidTeapot();
glXSwapBuffersMscOML(dpy, drawable, 1, 0, 0);
glXSwapBuffersMscOML(dpy, drawable, 9000, 0, 0);
glXSwapBuffersMscOML(dpy, drawable, 8000, 0, 0);
glXSwapBuffersMscOML(dpy, drawable, 7000, 0, 0);
If you follow the current code it would cause roughly the following
sequence, due to scheduling vblank events for execution at 7000,
8000, 9000, 1:
First swap with teapot at 7000, not at 1 as mandated by the spec
(which requires execution of requests in order of submission, ie.
fifo order).
Successive swaps probably at 8000, 9000, 1.
With above code it would be the more correct (less wrong?) order
1, 10001, 10002, 10003.
Another potential related problem is that calls to DRI2ScheduleSwap()
while there are already swaps pending do not throttle the client. So
this...
while (1) {
glutSolidTeapot();
glXSwapBuffers();
}
...is fine, because the drawing in glutSolidTeapot() will require new
buffers (DRI2GetBuffers() etc.) and that routine will call
DRI2ThrottleClient() if swaps are pending.
This otoh...
while (1) glXSwapBuffers();
... i think won't throttle - at least i didn't find code that would
do this. So you'd probably request large number of vblank events in
the DRM for swaps at successive vblank's. At some point i assume the
kernel would run out of memory for new vblank event structs or the
list of pending events inside the DRM would be full - drmWaitVblank
() would fail with ENOMEM - goto blitfallback; would trigger some
kind of behaviour and you'd have some weirdness in display - an
intermix of immediate swaps from the blitfallback and zombie swaps
as the vblank events get scheduled.
As weird as above example looks - swapping without drawing inbetween
- it is something that gets e.g., used for a quick dirty
implementation of frame-sequential stereo (with 3d shutter glasses
etc.) on systems that don't support OpenGL quadbuffered stereo
contexts.
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mar 7, 2010, at 6:18 PM, Jesse Barnes wrote: Arg, I did botch that patch. And of course I only tested the swap buffers behavior and not OML's WaitMSC so I didn't catch it. I'll improve the test and push the fix. No problem. Just fyi: I noticed you added a test in I830DRI2ScheduleSwap() that outputs X_Warnings if a swap is scheduled more than 100 vblanks ahead. At least for the users of my toolkit, scheduling a swap more than 100 vblanks ahead wouldn't be something noteworthy but quite a typical case of normal use of the system. At the end of a work day some users would probably find ten thousands of such warnings in some log, assuming those warnings get logged at the normal log level. So this usage case is less weird than one would think :-) -mario ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xkeyboard-config: updated geometry description of model pc105
Thanks for your submission, but it will get farther if you submit to the xkeyboard-config maintainers, which have their own mailing list and patch submission rules: http://www.freedesktop.org/wiki/Software/XKeyboardConfig/Development -- -Alan Coopersmith- alan.coopersm...@sun.com Oracle Solaris Platform Engineering: X Window System ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PULL] Minor bug-fixes discovered by static analysis.
On Fri, Mar 05, 2010 at 02:12:00PM +0200, Oliver McFadden wrote: are available in the git repository at: git://gitorious.org/omcfadde/xserver.git analysis Oliver McFadden (5): exa: exaFinishAccess: Overrun of static array pExaScr-access of size 6 at position 6 with index variable i fb: fbFinishScreenInit: leaked_storage: Variable (visuals|depths) goes out of scope parser: xf86readConfigFile: unreachable: This code cannot be reached: free(val.str); common: xf86Configure: alloc_strlen: Allocated memory does not have space for the terminating NUL of the string Xext: IdleTimeBlockHandler: unsigned_compare: Comparing unsigned less than zero is never true. timeout 0UL Xext/sync.c |4 +--- exa/exa.c |4 ++-- fb/fbscreen.c |4 hw/xfree86/common/xf86Configure.c |2 +- hw/xfree86/parser/read.c |4 ++-- 5 files changed, 10 insertions(+), 8 deletions(-) fwiw, I find it a lot easier to review patches that land on the mailing list than a pull request where I have to go off, pull into my tree, review and then copy/paste segments from the patch to be able to add comments. it also makes it harder to track when something has changed, since a tree doesn't usually contain the Patch v3 part of it. Cheers, Peter ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mon, 8 Mar 2010 01:23:11 +0100 Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: On Mar 7, 2010, at 6:18 PM, Jesse Barnes wrote: Arg, I did botch that patch. And of course I only tested the swap buffers behavior and not OML's WaitMSC so I didn't catch it. I'll improve the test and push the fix. No problem. Just fyi: I noticed you added a test in I830DRI2ScheduleSwap() that outputs X_Warnings if a swap is scheduled more than 100 vblanks ahead. At least for the users of my toolkit, scheduling a swap more than 100 vblanks ahead wouldn't be something noteworthy but quite a typical case of normal use of the system. At the end of a work day some users would probably find ten thousands of such warnings in some log, assuming those warnings get logged at the normal log level. So this usage case is less weird than one would think :-) Ok, I was worried about that. I'll remove the message; I guess there's no easy way to tell between legitimate infrequent swaps and bad MSC requests... -- Jesse Barnes, Intel Open Source Technology Center ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] DRI2: fixup handling of last_swap_target
On Mon, 8 Mar 2010 01:03:53 +0100
Mario Kleiner mario.klei...@tuebingen.mpg.de wrote:
Great. I'll try to quickly go over it again when you're done.
I think i found another issue or two:
In DRI2SwapBuffers() you have this code for glXSwapBuffersMscOML
handling:
...
} else {
/* glXSwapBuffersMscOML could have a 0 target_msc, honor it */
*swap_target = target_msc;
}
Now while my previous patch was to restrictive and didn't allow to
honor a 0 target_msc, this one is too liberal. It would allow a
client to schedule many swaps for the same target_msc value, e.g.,
for target_msc = 1000:
while (1) glXSwapBuffersMscOML(dpy, drawable, 1000, 0, 0);
Hm, I thought I had added some logic before that too, maybe I just have
it locally.
- This would cause the DRM at vblank 1000 to deliver many swap-
vblank events during the same vblank, which i assume can cause
trouble with timestamping of the swaps and probably server
responsiveness? In the pageflipped case, you'd schedule one pageflip
successfully and then if i understand correctly, on the 2nd call for
this vblank the pageflip ioctl would fail with something like EAGAIN
or EBUSY, which would cause the ddx to fall back to blits. If i
understand correctly from I830DRI2FrameEventHandler -
I830DRI2CopyRegion, blit requests get submitted to the command fifo,
drmCommandNone(intel-drmSubFD, DRM_I915_GEM_THROTTLE); gets called,
then DRI2SwapComplete gets called with the 'msc' value of delivery of
the vblank event - in this case with 1000.
Not sure if the GEM_THROTTLE'ing somehow takes care of such issues -
or the fact that the gpu command fifo must be full at some time, but
is there something that would prevent many calls to DRI2SwapComplete
with exactly the same 'msc' / vblank value and 'ust' timestamp
despite the fact that most of the swaps won't happen at that msc but
later? Would any of the throttling here - or something like a full
fifo - cause the server to stall or get sluggish? I don't know enough
about the implementation, but allowing to deliver many identical
vblank swap events sounds problematic to me.
If the completions end up using blits, they'll just queue to the kernel
and may even stall the server until completion if the no-tearing option
is enabled. Otherwise we may end up blocking in the kernel if the
command ring is full.
Maybe we could change above code to something like:
...
} else {
/* glXSwapBuffersMscOML could have a 0 target_msc, honor it */
if (target_msc = pPriv-last_swap_target pPriv-swap_interval)
target_msc = pPriv-last_swap_target + 1;
*swap_target = target_msc;
}
This would not constrain a given target_msc to honor the exact
swap_interval setting - what my previous -wrong- patch did. For a
target_msc of zero etc., i believe this updated target_msc would
result in the same behaviour as the current code. It wouldn't push
the target_msc into the far future, but only make sure it doesn't
get stuck in the past or at a fixed value in the future, basically
only ensuring the only at most one swap per msc increment
requirement of the OML_sync_control spec.
Yeah, that sounds reasonable. I ran into the 0 bug because a basic
swap is just that; but making sure we don't schedule a bunch for the
same MSC or old MSC is important too, so I'll fix the logic.
A similar error case that could be kind'a solved by the above would
be wrong ordering of swaprequests:
Assume we're at msc 500, last_swap_target something smaller than
e.g., 1000:
glutSolidTeapot();
glXSwapBuffersMscOML(dpy, drawable, 1, 0, 0);
glXSwapBuffersMscOML(dpy, drawable, 9000, 0, 0);
glXSwapBuffersMscOML(dpy, drawable, 8000, 0, 0);
glXSwapBuffersMscOML(dpy, drawable, 7000, 0, 0);
If you follow the current code it would cause roughly the following
sequence, due to scheduling vblank events for execution at 7000,
8000, 9000, 1:
First swap with teapot at 7000, not at 1 as mandated by the spec
(which requires execution of requests in order of submission, ie.
fifo order).
Successive swaps probably at 8000, 9000, 1.
With above code it would be the more correct (less wrong?) order
1, 10001, 10002, 10003.
Heh, yeah that's a which wrong do you want choice. :) Handling the
case where a swap is requested way in the future (or past, depending on
whether you're seeing wrapping) is a bit of a pain. Where do you cut
things off? If the app has been working well but then suddenly misses,
we can assume scheduling caused it to fail and just make a best
effort. The kernel has some code for handling that already in fact; I
think vblank events will be delivered for counts in the past.
Another potential related problem is that calls to DRI2ScheduleSwap()
while there are already swaps pending do not throttle the client. So
Re: [PATCH xinput] test-xi2: print event type name as well.
Peter Hutterer peter.hutte...@who-t.net wrote:
Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
src/test_xi2.c | 31 ++-
1 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/src/test_xi2.c b/src/test_xi2.c
index 53d984f..6fdc4ad 100644
--- a/src/test_xi2.c
+++ b/src/test_xi2.c
@@ -258,6 +258,35 @@ test_sync_grab(Display *display, Window win)
printf(Done\n);
}
+static const char* type_to_name(int evtype)
+{
+const char *name;
+
+switch(evtype) {
+case XI_DeviceChanged:name = DeviceChanged; break;
+case XI_KeyPress: name = KeyPress;break;
+case XI_KeyRelease: name = KeyRelease; break;
+case XI_ButtonPress: name = ButtonPress; break;
+case XI_ButtonRelease:name = ButtonRelease; break;
+case XI_Motion: name = Motion; break;
+case XI_Enter:name = Enter; break;
+case XI_Leave:name = Leave; break;
+case XI_FocusIn: name = FocusIn; break;
+case XI_FocusOut: name = FocusOut;break;
+case XI_HierarchyChanged: name = HierarchyChanged;break;
+case XI_PropertyEvent:name = PropertyEvent; break;
+case XI_RawKeyPress: name = RawKeyPress; break;
+case XI_RawKeyRelease:name = RawKeyRelease; break;
+case XI_RawButtonPress: name = RawButtonPress; break;
+case XI_RawButtonRelease: name = RawButtonRelease;break;
+case XI_RawMotion:name = RawMotion; break;
+default:
+ name = unknown event type; break;
+}
+return name;
+}
+
+
int
test_xi2(Display *display,
int argc,
@@ -341,7 +370,7 @@ test_xi2(Display *display,
cookie-type == GenericEvent
cookie-extension == xi_opcode)
{
-printf(EVENT type %d\n, cookie-evtype);
+printf(EVENT type %d (%s)\n, cookie-evtype,
type_to_name(cookie-evtype));
switch (cookie-evtype)
{
case XI_DeviceChanged:
--
1.6.6.1
Reviewed-by: Fernando Carrijo fcarr...@yahoo.com.br
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] DRI2: fixup handling of last_swap_target
On Mar 8, 2010, at 1:49 AM, Jesse Barnes wrote:
In DRI2SwapBuffers() you have this code for glXSwapBuffersMscOML
handling:
...
} else {
/* glXSwapBuffersMscOML could have a 0 target_msc, honor
it */
*swap_target = target_msc;
}
Now while my previous patch was to restrictive and didn't allow to
honor a 0 target_msc, this one is too liberal. It would allow a
client to schedule many swaps for the same target_msc value, e.g.,
for target_msc = 1000:
Ok, forget about this. Leave it like it is, i.e. a simple assignment
*swap_target = target_msc; no extra check. It can cause some problems
with the semantic of the divisor and remainder stuff later on if we
nudge the target_msc.
If we simply fix the 2nd issue by DRI2ThrottleClient() throttling the
client at the beginning of DRI2SwapBuffers() as soon as a swap is
already pending, then we get the correct fix for the above, and for
ordering swaps correctly, for free :-)
If the completions end up using blits, they'll just queue to the
kernel
and may even stall the server until completion if the no-tearing
option
is enabled. Otherwise we may end up blocking in the kernel if the
command ring is full.
Out of interest: The code in I830CopyRegion first submits the wait
for vblank commands, followed by the copy region commands to the
fifo, then calls intel_sync(). Does this intel_sync() block the
server only until all batchbuffers are submitted to the gpu (and will
execute at some time) or does it really block the server until they
are completely processed - basically until the blit is really fully
completed?
effort. The kernel has some code for handling that already in fact; I
think vblank events will be delivered for counts in the past.
Yes, the kernel takes care. I think i understand the magic there now
(my brain hurts - to many 32 bit wraparounds and signed math on
unsigned ints). The kernel side of scheduling vblank events seems to
be safe - even against vblank counter wraparounds - as long as the
requested count is not more than (2^32 - 2^23) approx. 4 billion
counts ahead of the vblank count, or more than 2^23 = 8 million
counts behind. This even works although the 64 bit values from the
xserver get truncated to 32 bit when assigned to
vbl.request.sequence, as far as i understand it.
The behind case can't happen due to the code in the Intel ddx. The
ahead case one could guard against.
One remaining problem is that all returned vbl.reply.sequence values
from the kernel are only 32 bit, but they're compared against the 64
bit values from the client. Without a virtualized 64 bit vblank count
in the ddx, things could get tricky if the target_msc values grow
larger than 2^32. This can happen if a client passes in large numbers
for good or bad reasons, or even during normal glXSwapBuffers calls
if due to long system uptime one is close to such a 32 bit boundary.
-mario
This otoh...
while (1) glXSwapBuffers();
... i think won't throttle - at least i didn't find code that would
do this. So you'd probably request large number of vblank events in
the DRM for swaps at successive vblank's. At some point i assume the
kernel would run out of memory for new vblank event structs or the
list of pending events inside the DRM would be full - drmWaitVblank
() would fail with ENOMEM - goto blitfallback; would trigger some
kind of behaviour and you'd have some weirdness in display - an
intermix of immediate swaps from the blitfallback and zombie swaps
as the vblank events get scheduled.
Ah yes, this probably is an issue; we should check the swap queue
limit
in DRI2SwapBuffers as well, probably by just calling
DRI2ThrottleClient
at that point.
As weird as above example looks - swapping without drawing inbetween
- it is something that gets e.g., used for a quick dirty
implementation of frame-sequential stereo (with 3d shutter glasses
etc.) on systems that don't support OpenGL quadbuffered stereo
contexts. So its not just something totally far fetched.
Yeah and a malicious app could do it too, so we should handle it.
I thought quite a bit about such effects while looking at your code.
I think a really cool and more bullet-proof implementation of the
OML_sync_control spec that would allow a client to render multiple
frames ahead / have multiple swaps pending without getting into
trouble with a few race-conditions in the current code is possible,
but it would require a more dynamic scheduling of what happens when.
Maybe we can discuss such refinements when your first iteration of
the dri2 swap bits is finished and out there.
It seems like we're in pretty good shape in terms of not hanging the
server at least. Not hanging the client when it passes in bad MSC
values is a good goal though too; I'd definitely be interested in
making that aspect more robust.
--
Jesse Barnes, Intel Open Source Technology Center
