Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Mon, 25 Oct 2010 17:13:58 +0300, Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: There isn't API that allows application atomically query for msc changes and schedule swaps. If msc changes dramatically between query and scheduling application would schedule swap to happen at wrong time. Because of API limitations driver has to make msc increment for each vblank affecting the drawable. This thread ended without an updated patch or a Rb: line. Is this ready for the server? -- keith.pack...@intel.com pgpqJjGr3wdhP.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On 28/10/10 21:12 +0200, ext Alex Deucher wrote: On Thu, Oct 28, 2010 at 2:20 PM, Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: On Oct 28, 2010, at 6:02 PM, Jesse Barnes wrote: On Thu, 28 Oct 2010 18:47:09 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: Most of what you have in (b) is pretty straightfoward; even the shared drawable case shouldn't be too bad, since each X connection could have bits indicating whether the counter has been picked up after a CRTC move. One option would be adding crct id parameter to calls. glXGetMscBaseRateOML would return rate, base msc and pipe id where this msc value is valid. Now all MSC calls would take the returned pipe id as parameter. If pipe id doesn't match current crtc any more then call would fail. This would allow complex applications to pass same pipe id to different context. Negative side is that API would have to be changed to include extra parameter. Yeah that would be a good extension though; we may as well expose the fact that different display pipes exist on the system, and have corresponding MSCs. Old applications using SGI_video_sync or existing OML behavior would work like they do today (with an MSC value that may jump, which we could fix with the virtualize count), and new ones would be pipe aware. I also like option b) the most, define new spec and api instead of working around the old specs limitations. Another way of doing it, in the same spirit, would be some generation counter. Starts with 1, increments each time that something changes in the configuration which could influence the display timing and mess with the schedule the app had in mind. If the drawable changes crtc's. If the crtc changes its video mode (esp. refresh rate) or configuration (mirrored/extended desktop, synchronized to other crtc's etc.), dpms change etc. A get call could return the current count, other calls could return the count that was valid at time of their processing. E.g., intel_swap_events could code more info like generation count, crtcid. Apps could pass in the count to the msc related functions and those would fail on mismatch to the current count. One could also have one special don't care value (e.g., 0) that says I don't care about an isolated glitch, because i'm not prepared to handle this anyway. Just do something to make sure i don't hang, e.g., fall through a blocking glXWaitMscOml() call or swap the buffers immediately on glXSwapBuffersMscOML(). I'm also for exposing rather more than less information, like pipe configuration, or the ability for the app to decide what it wants, e.g., * If the drawable covers multiple crtc's, or is in mirror display mode, should one of them define when to swap and the other should show tearing, or should each of them sync its swap separately, which looks nice, but can throttle redraw rates or possibly exhaust resources if the crtc's run and largely different rates. Windows has the concept of a primary display which defines the swap timing on extended desktops. The non-primary display just shows tearing. Mac OS behaves similar, except that you don't have control over which is the primary display, and some (sometimes buggy) heuristic decides for you and gives you the fun of working around it by replugging monitors and other fun things. I like control. The current intel and radeon ddx in page flipped mode will swap each crtc separately. Tear free, but with throttled framerate, as swap completion == swap completion of the last involved crtc. This btw. is a problem for the returned timestamps and timing if the crtc's run at different refresh rates, as the app doesn't know to which crtc the swap completion timestamp belongs. And it changes over time. For blitted copy-swaps you get tearfree on the assigned crtc for a drawable and tearing on the other one. Another approach would be to define swap times in system (gettimeofday() time). Specify a swap deadline tWhen and the system tries to swap at the earliest vblank with a time tNow = tWhen. Then one doesn't have to care too much about changes in msc rates. The NV_present_video http://www.opengl.org/registry/specs/NV/present_video.txt extension does something similar for presentatio of video buffers. My own toolkit does this as well and for user code it's a natural way to specify presentation times, especially if it has to synchronize presentation with other modalities like sound, digital i/o, eye tracking etc. My code just uses the glXGetSyncValuesOML() call to translate a user-provided system time tWhen into a corresponding target_msc for glXSwapBuffersMscOML(). When we're at defining new api (christmas time is coming, i got lots of wishes), a new swapbuffers call could also define what to do if a presentation deadline can't be met. E.g., instead of a delayed swap it could drop the
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On 27/10/10 17:57 +0200, ext Jesse Barnes wrote: On Wed, 27 Oct 2010 17:15:28 +0200 Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: On Oct 26, 2010, at 7:08 PM, Jesse Barnes wrote: On Tue, 26 Oct 2010 19:19:11 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: SGI_video_sync: The kernel maintains a video sync counter for each physical hardware pipe in a system; the counter is incremented upon the completion of the display of each full frame of video data. An OpenGL context always corresponds to a pipe. Right, this is the rule we break; we don't have a 1:1 correspondence between gfx pipes and display pipes. The single video sync counter is shared by all GLXContexts. Yes. You have to extent both OML_sync_control and SGI_video_sync to expose separate MSC for different CRTCs. I can see race condition even with events. * Client checks for event (none yet in queue) * Client prepares to call some blocking msc call * Window manager decides to move the window * xserver send MSC change event * Client calls blocking MSC call But maybe there is solution. All blocking calls could fail if MSC base changes. Client would have to query for new base and rate before trying to issue same request again. Yeah, that might work; I agree there's a race even with events that we'll need to handle. But even with that race handled I'd like the server to fail gracefully rather than hanging the app if an old MSC value is passed in (though in that case we could print an error message since we could assume an app error as long as the app was using the extended version of the spec). For swap interval it would be enough if DDX would notify DRI2 about crtc changes with offset (msc_for_new_crtc - msc_for_old_crtc) that can be applied to swaptarget. Yeah, just jumping to the new value might be ok in general. Hardcore libraries like Mario's can do something fancier with the extensions above to avoid unintended behavior. I agree. Pauli's offset fix would fix the common glXSwapBuffers() case for now and make most apps happy. We should do that. My current hack works fine (due to the underlying vsync'ing of the ddx's) as long as an app uses glXSwapBuffers and has swap_interval left at its default of 1. I'd expect most apps to do that, as it was the only supported setting (except for 0) for a long time, and all other operating systems i know of (Linux + proprietary drivers, all Windows, all OS/X) only obey a swap_interval = 0 or 1, but this fix would fix it for all swap_interval settings. For the oml_sync_control case i see these options, and each of them makes me dizzy and uneasy in a different way, probably because i didn't think it through clearly: a) As Michel Daenzer proposed earlier, give each drawable its own virtualized msc. Initialize it at drawable creation time to the true msc of the initial crtc. Then just use the current msc and info about crtc transitions to update the virtualized msc. This way we'd be probably closest to the spirit of the current spec, all msc's would always increase monotonically instead of jumping back and forth and crtc transitions would be handled properly without the app even noticing or needing to care. If multiple drawable's are created on the same crtc and stay there, they'll have the same msc's, so bufferswaps, waits and other events can be synchronized across drawables and timestamps and counts compared meaningfully between them. That would be nice to have. Downside: As soon as a drawable moves away to another crtc with different count, this beauty breaks down and the app would have large problems finding out what just happened to it and how to relate the msc's of different drawables to each other again. Possibly impossible to recover with all those virtualized counts. Also it's tricky to implement. We would need to translate forward and backward between hardware msc's and virtualized msc each time we get any event from the kernel or schedule one and keep track of which crtc was assigned when all the time, also in all queued vblank events and all returned vblank and swap events. The fact that we currently have 64 bit msc counters in userspace and spec, but only 32 bit counters in kernel space and all the wraparound issues doesn't make it simpler to get right and race-free. b) Extend the spec: If a crtc transition is detected, make all calls that rely on the msc (glXSwapBuffersMsc(), glXWaitForMsc()) fail with some error code until the app has called glXGetSyncValuesOML() again to fetch the new, updated msc for the new crtc. I like this because i think it is simpler to implement correctly and because the apps still see what is really going on.
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Thu, 28 Oct 2010 18:47:09 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: Most of what you have in (b) is pretty straightfoward; even the shared drawable case shouldn't be too bad, since each X connection could have bits indicating whether the counter has been picked up after a CRTC move. One option would be adding crct id parameter to calls. glXGetMscBaseRateOML would return rate, base msc and pipe id where this msc value is valid. Now all MSC calls would take the returned pipe id as parameter. If pipe id doesn't match current crtc any more then call would fail. This would allow complex applications to pass same pipe id to different context. Negative side is that API would have to be changed to include extra parameter. Yeah that would be a good extension though; we may as well expose the fact that different display pipes exist on the system, and have corresponding MSCs. Old applications using SGI_video_sync or existing OML behavior would work like they do today (with an MSC value that may jump, which we could fix with the virtualize count), and new ones would be pipe aware. -- Jesse Barnes, Intel Open Source Technology Center ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On 28/10/10 18:06 +0200, Ville Syrjälä wrote: On Thu, Oct 28, 2010 at 05:47:09PM +0200, ext Pauli Nieminen wrote: One option would be adding crct id parameter to calls. glXGetMscBaseRateOML would return rate, base msc and pipe id where this msc value is valid. Now all MSC calls would take the returned pipe id as parameter. If pipe id doesn't match current crtc any more then call would fail. This would allow complex applications to pass same pipe id to different context. Negative side is that API would have to be changed to include extra parameter. And what about cases involving multiple CRTCs? Would the user just choose one? And what about the other CRTCs then, could some suffer from tearing or should more buffers be added to the mix to allow the CRTCs to scan out of different buffers and flip at their own rate? But for common case it should be enough to use N-buffering for good performance and fairly good timing. Driver can do fairly good decision when flip should happen for each CRTCs. Some clients might want to be aware of mirrored or extended rendering target. But that can be exposed in separate extension. Pauli ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Thu, Oct 28, 2010 at 05:47:09PM +0200, ext Pauli Nieminen wrote: One option would be adding crct id parameter to calls. glXGetMscBaseRateOML would return rate, base msc and pipe id where this msc value is valid. Now all MSC calls would take the returned pipe id as parameter. If pipe id doesn't match current crtc any more then call would fail. This would allow complex applications to pass same pipe id to different context. Negative side is that API would have to be changed to include extra parameter. And what about cases involving multiple CRTCs? Would the user just choose one? And what about the other CRTCs then, could some suffer from tearing or should more buffers be added to the mix to allow the CRTCs to scan out of different buffers and flip at their own rate? -- Ville Syrjälä ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Oct 28, 2010, at 6:02 PM, Jesse Barnes wrote: On Thu, 28 Oct 2010 18:47:09 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: Most of what you have in (b) is pretty straightfoward; even the shared drawable case shouldn't be too bad, since each X connection could have bits indicating whether the counter has been picked up after a CRTC move. One option would be adding crct id parameter to calls. glXGetMscBaseRateOML would return rate, base msc and pipe id where this msc value is valid. Now all MSC calls would take the returned pipe id as parameter. If pipe id doesn't match current crtc any more then call would fail. This would allow complex applications to pass same pipe id to different context. Negative side is that API would have to be changed to include extra parameter. Yeah that would be a good extension though; we may as well expose the fact that different display pipes exist on the system, and have corresponding MSCs. Old applications using SGI_video_sync or existing OML behavior would work like they do today (with an MSC value that may jump, which we could fix with the virtualize count), and new ones would be pipe aware. I also like option b) the most, define new spec and api instead of working around the old specs limitations. Another way of doing it, in the same spirit, would be some generation counter. Starts with 1, increments each time that something changes in the configuration which could influence the display timing and mess with the schedule the app had in mind. If the drawable changes crtc's. If the crtc changes its video mode (esp. refresh rate) or configuration (mirrored/extended desktop, synchronized to other crtc's etc.), dpms change etc. A get call could return the current count, other calls could return the count that was valid at time of their processing. E.g., intel_swap_events could code more info like generation count, crtcid. Apps could pass in the count to the msc related functions and those would fail on mismatch to the current count. One could also have one special don't care value (e.g., 0) that says I don't care about an isolated glitch, because i'm not prepared to handle this anyway. Just do something to make sure i don't hang, e.g., fall through a blocking glXWaitMscOml() call or swap the buffers immediately on glXSwapBuffersMscOML(). I'm also for exposing rather more than less information, like pipe configuration, or the ability for the app to decide what it wants, e.g., * If the drawable covers multiple crtc's, or is in mirror display mode, should one of them define when to swap and the other should show tearing, or should each of them sync its swap separately, which looks nice, but can throttle redraw rates or possibly exhaust resources if the crtc's run and largely different rates. Windows has the concept of a primary display which defines the swap timing on extended desktops. The non-primary display just shows tearing. Mac OS behaves similar, except that you don't have control over which is the primary display, and some (sometimes buggy) heuristic decides for you and gives you the fun of working around it by replugging monitors and other fun things. I like control. The current intel and radeon ddx in page flipped mode will swap each crtc separately. Tear free, but with throttled framerate, as swap completion == swap completion of the last involved crtc. This btw. is a problem for the returned timestamps and timing if the crtc's run at different refresh rates, as the app doesn't know to which crtc the swap completion timestamp belongs. And it changes over time. For blitted copy-swaps you get tearfree on the assigned crtc for a drawable and tearing on the other one. Another approach would be to define swap times in system (gettimeofday () time). Specify a swap deadline tWhen and the system tries to swap at the earliest vblank with a time tNow = tWhen. Then one doesn't have to care too much about changes in msc rates. The NV_present_video http://www.opengl.org/registry/specs/NV/ present_video.txt extension does something similar for presentatio of video buffers. My own toolkit does this as well and for user code it's a natural way to specify presentation times, especially if it has to synchronize presentation with other modalities like sound, digital i/o, eye tracking etc. My code just uses the glXGetSyncValuesOML() call to translate a user-provided system time tWhen into a corresponding target_msc for glXSwapBuffersMscOML(). When we're at defining new api (christmas time is coming, i got lots of wishes), a new swapbuffers call could also define what to do if a presentation deadline can't be met. E.g., instead of a delayed swap it could drop the swap and completely skip a bufferswap request to get presentation timing back on schedule, so skipped frame errors can't accumulate. Something like that could be interesting for
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Thu, Oct 28, 2010 at 2:20 PM, Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: On Oct 28, 2010, at 6:02 PM, Jesse Barnes wrote: On Thu, 28 Oct 2010 18:47:09 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: Most of what you have in (b) is pretty straightfoward; even the shared drawable case shouldn't be too bad, since each X connection could have bits indicating whether the counter has been picked up after a CRTC move. One option would be adding crct id parameter to calls. glXGetMscBaseRateOML would return rate, base msc and pipe id where this msc value is valid. Now all MSC calls would take the returned pipe id as parameter. If pipe id doesn't match current crtc any more then call would fail. This would allow complex applications to pass same pipe id to different context. Negative side is that API would have to be changed to include extra parameter. Yeah that would be a good extension though; we may as well expose the fact that different display pipes exist on the system, and have corresponding MSCs. Old applications using SGI_video_sync or existing OML behavior would work like they do today (with an MSC value that may jump, which we could fix with the virtualize count), and new ones would be pipe aware. I also like option b) the most, define new spec and api instead of working around the old specs limitations. Another way of doing it, in the same spirit, would be some generation counter. Starts with 1, increments each time that something changes in the configuration which could influence the display timing and mess with the schedule the app had in mind. If the drawable changes crtc's. If the crtc changes its video mode (esp. refresh rate) or configuration (mirrored/extended desktop, synchronized to other crtc's etc.), dpms change etc. A get call could return the current count, other calls could return the count that was valid at time of their processing. E.g., intel_swap_events could code more info like generation count, crtcid. Apps could pass in the count to the msc related functions and those would fail on mismatch to the current count. One could also have one special don't care value (e.g., 0) that says I don't care about an isolated glitch, because i'm not prepared to handle this anyway. Just do something to make sure i don't hang, e.g., fall through a blocking glXWaitMscOml() call or swap the buffers immediately on glXSwapBuffersMscOML(). I'm also for exposing rather more than less information, like pipe configuration, or the ability for the app to decide what it wants, e.g., * If the drawable covers multiple crtc's, or is in mirror display mode, should one of them define when to swap and the other should show tearing, or should each of them sync its swap separately, which looks nice, but can throttle redraw rates or possibly exhaust resources if the crtc's run and largely different rates. Windows has the concept of a primary display which defines the swap timing on extended desktops. The non-primary display just shows tearing. Mac OS behaves similar, except that you don't have control over which is the primary display, and some (sometimes buggy) heuristic decides for you and gives you the fun of working around it by replugging monitors and other fun things. I like control. The current intel and radeon ddx in page flipped mode will swap each crtc separately. Tear free, but with throttled framerate, as swap completion == swap completion of the last involved crtc. This btw. is a problem for the returned timestamps and timing if the crtc's run at different refresh rates, as the app doesn't know to which crtc the swap completion timestamp belongs. And it changes over time. For blitted copy-swaps you get tearfree on the assigned crtc for a drawable and tearing on the other one. Another approach would be to define swap times in system (gettimeofday() time). Specify a swap deadline tWhen and the system tries to swap at the earliest vblank with a time tNow = tWhen. Then one doesn't have to care too much about changes in msc rates. The NV_present_video http://www.opengl.org/registry/specs/NV/present_video.txt extension does something similar for presentatio of video buffers. My own toolkit does this as well and for user code it's a natural way to specify presentation times, especially if it has to synchronize presentation with other modalities like sound, digital i/o, eye tracking etc. My code just uses the glXGetSyncValuesOML() call to translate a user-provided system time tWhen into a corresponding target_msc for glXSwapBuffersMscOML(). When we're at defining new api (christmas time is coming, i got lots of wishes), a new swapbuffers call could also define what to do if a presentation deadline can't be met. E.g., instead of a delayed swap it could drop the swap and completely skip a bufferswap request to get presentation timing back on schedule, so skipped frame errors can't accumulate. Something
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Oct 26, 2010, at 7:08 PM, Jesse Barnes wrote: On Tue, 26 Oct 2010 19:19:11 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: SGI_video_sync: The kernel maintains a video sync counter for each physical hardware pipe in a system; the counter is incremented upon the completion of the display of each full frame of video data. An OpenGL context always corresponds to a pipe. Right, this is the rule we break; we don't have a 1:1 correspondence between gfx pipes and display pipes. The single video sync counter is shared by all GLXContexts. Yes. You have to extent both OML_sync_control and SGI_video_sync to expose separate MSC for different CRTCs. I can see race condition even with events. * Client checks for event (none yet in queue) * Client prepares to call some blocking msc call * Window manager decides to move the window * xserver send MSC change event * Client calls blocking MSC call But maybe there is solution. All blocking calls could fail if MSC base changes. Client would have to query for new base and rate before trying to issue same request again. Yeah, that might work; I agree there's a race even with events that we'll need to handle. But even with that race handled I'd like the server to fail gracefully rather than hanging the app if an old MSC value is passed in (though in that case we could print an error message since we could assume an app error as long as the app was using the extended version of the spec). For swap interval it would be enough if DDX would notify DRI2 about crtc changes with offset (msc_for_new_crtc - msc_for_old_crtc) that can be applied to swaptarget. Yeah, just jumping to the new value might be ok in general. Hardcore libraries like Mario's can do something fancier with the extensions above to avoid unintended behavior. I agree. Pauli's offset fix would fix the common glXSwapBuffers() case for now and make most apps happy. We should do that. My current hack works fine (due to the underlying vsync'ing of the ddx's) as long as an app uses glXSwapBuffers and has swap_interval left at its default of 1. I'd expect most apps to do that, as it was the only supported setting (except for 0) for a long time, and all other operating systems i know of (Linux + proprietary drivers, all Windows, all OS/X) only obey a swap_interval = 0 or 1, but this fix would fix it for all swap_interval settings. For the oml_sync_control case i see these options, and each of them makes me dizzy and uneasy in a different way, probably because i didn't think it through clearly: a) As Michel Daenzer proposed earlier, give each drawable its own virtualized msc. Initialize it at drawable creation time to the true msc of the initial crtc. Then just use the current msc and info about crtc transitions to update the virtualized msc. This way we'd be probably closest to the spirit of the current spec, all msc's would always increase monotonically instead of jumping back and forth and crtc transitions would be handled properly without the app even noticing or needing to care. If multiple drawable's are created on the same crtc and stay there, they'll have the same msc's, so bufferswaps, waits and other events can be synchronized across drawables and timestamps and counts compared meaningfully between them. That would be nice to have. Downside: As soon as a drawable moves away to another crtc with different count, this beauty breaks down and the app would have large problems finding out what just happened to it and how to relate the msc's of different drawables to each other again. Possibly impossible to recover with all those virtualized counts. Also it's tricky to implement. We would need to translate forward and backward between hardware msc's and virtualized msc each time we get any event from the kernel or schedule one and keep track of which crtc was assigned when all the time, also in all queued vblank events and all returned vblank and swap events. The fact that we currently have 64 bit msc counters in userspace and spec, but only 32 bit counters in kernel space and all the wraparound issues doesn't make it simpler to get right and race-free. b) Extend the spec: If a crtc transition is detected, make all calls that rely on the msc (glXSwapBuffersMsc(), glXWaitForMsc()) fail with some error code until the app has called glXGetSyncValuesOML() again to fetch the new, updated msc for the new crtc. I like this because i think it is simpler to implement correctly and because the apps still see what is really going on. Toolkits like mine which require very tight (=paranoid) control about timing don't like too much abstraction and virtualization. Downside: Less elegant? What if multiple threads (somewhat likely) will use blocking calls on the same drawable? One thread might query the msc and clear the 'dirty bit', but other threads may not notice? Need a per-thread dirty
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On 26/10/10 03:00 +0200, ext Mario Kleiner wrote:
On Oct 25, 2010, at 6:52 PM, Jesse Barnes wrote:
On Mon, 25 Oct 2010 17:13:58 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
There isn't API that allows application atomically query for msc
changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
CC: Kristian Høgsberg k...@bitplanet.net
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
This one scares me a little. We added this so we could also catch
drawables moving between screens with different msc bases, so this
patch could cause a lot of false positives (no question that the specs
could use some additions here though).
Making it a debug message that only shows up with -verbose would be
fine though.
--
Jesse Barnes, Intel Open Source Technology Center
I agree with Jesse, as a debug message at -verbose it would be fine.
The fix that you added to common code doesn't fix the root cause. In that
case error message is correct because common code is working around driver
bug for single case that happens to be common. Driver that hits that error
line shouldn't expose OML_sync_control because application can't use it
reliably in that case.
But if you really want only verbose message I'm happy to change the patch.
-mario
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Tue, 26 Oct 2010 11:39:11 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
On 26/10/10 03:00 +0200, ext Mario Kleiner wrote:
On Oct 25, 2010, at 6:52 PM, Jesse Barnes wrote:
On Mon, 25 Oct 2010 17:13:58 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
There isn't API that allows application atomically query for msc
changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
CC: Kristian Høgsberg k...@bitplanet.net
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
-if (current_msc pPriv-last_swap_target)
+if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+xf86DrvMsg(pScreen-myNum, X_ERROR,
+[DRI2] %s: GetMSC returned swap count that is
in
+past. Working around driver bug.\n, __func__);
+}
}
This one scares me a little. We added this so we could also catch
drawables moving between screens with different msc bases, so this
patch could cause a lot of false positives (no question that the specs
could use some additions here though).
Making it a debug message that only shows up with -verbose would be
fine though.
--
Jesse Barnes, Intel Open Source Technology Center
I agree with Jesse, as a debug message at -verbose it would be fine.
The fix that you added to common code doesn't fix the root cause. In that
case error message is correct because common code is working around driver
bug for single case that happens to be common. Driver that hits that error
line shouldn't expose OML_sync_control because application can't use it
reliably in that case.
It's not just OML_sync_control; it's also SGI_video_sync and
SGI_swap_interval (also part of the core EGL spec). Disabling these
due to unsynced multihead would be a bit harsh; it would also be hard
because it's something that can change at runtime (what if a single
head config with these extensions exposed becomes multihead with
variable rate MSC due to a monitor getting plugged in?).
That's why I think we need to fix the spec instead; it would be easy
enough to add a new event to notify clients when the MSC rate and base
change due to a move to a new pipe, someone just needs to write it up
and code it (shouldn't be too hard).
But if you really want only verbose message I'm happy to change the patch.
Yes please, until we get the spec and such worked out. These sorts of
messages always confuse users; we end up with a bunch of bug reports
and OSVs patching it out anyway.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On 26/10/10 17:41 +0200, ext Jesse Barnes wrote:
On Tue, 26 Oct 2010 11:39:11 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
On 26/10/10 03:00 +0200, ext Mario Kleiner wrote:
On Oct 25, 2010, at 6:52 PM, Jesse Barnes wrote:
On Mon, 25 Oct 2010 17:13:58 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
There isn't API that allows application atomically query for msc
changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
CC: Kristian Høgsberg k...@bitplanet.net
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is
in
+ past. Working around driver bug.\n, __func__);
+ }
}
This one scares me a little. We added this so we could also catch
drawables moving between screens with different msc bases, so this
patch could cause a lot of false positives (no question that the specs
could use some additions here though).
Making it a debug message that only shows up with -verbose would be
fine though.
--
Jesse Barnes, Intel Open Source Technology Center
I agree with Jesse, as a debug message at -verbose it would be fine.
The fix that you added to common code doesn't fix the root cause. In that
case error message is correct because common code is working around driver
bug for single case that happens to be common. Driver that hits that error
line shouldn't expose OML_sync_control because application can't use it
reliably in that case.
It's not just OML_sync_control; it's also SGI_video_sync and
SGI_swap_interval (also part of the core EGL spec). Disabling these
due to unsynced multihead would be a bit harsh; it would also be hard
because it's something that can change at runtime (what if a single
head config with these extensions exposed becomes multihead with
variable rate MSC due to a monitor getting plugged in?).
That's why I think we need to fix the spec instead; it would be easy
enough to add a new event to notify clients when the MSC rate and base
change due to a move to a new pipe, someone just needs to write it up
and code it (shouldn't be too hard).
SGI_video_sync:
The kernel maintains a video sync counter for
each physical hardware pipe in a system; the counter is incremented
upon the completion of the display of each full frame of video data. An
OpenGL context always corresponds to a pipe.
The single video sync counter is shared by all GLXContexts.
Yes. You have to extent both OML_sync_control and SGI_video_sync to expose
separate MSC for different CRTCs.
I can see race condition even with events.
* Client checks for event (none yet in queue)
* Client prepares to call some blocking msc call
* Window manager decides to move the window
* xserver send MSC change event
* Client calls blocking MSC call
But maybe there is solution. All blocking calls could fail if MSC base
changes. Client would have to query for new base and rate before trying to
issue same request again.
For swap interval it would be enough if DDX would notify DRI2 about crtc
changes with offset (msc_for_new_crtc - msc_for_old_crtc) that can be applied
to swaptarget.
But if you really want only verbose message I'm happy to change the patch.
Yes please, until we get the spec and such worked out. These sorts of
messages always confuse users; we end up with a bunch of bug reports
and OSVs patching it out anyway.
That is too bad
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Tue, 26 Oct 2010 19:19:11 +0300 Pauli Nieminen ext-pauli.niemi...@nokia.com wrote: SGI_video_sync: The kernel maintains a video sync counter for each physical hardware pipe in a system; the counter is incremented upon the completion of the display of each full frame of video data. An OpenGL context always corresponds to a pipe. Right, this is the rule we break; we don't have a 1:1 correspondence between gfx pipes and display pipes. The single video sync counter is shared by all GLXContexts. Yes. You have to extent both OML_sync_control and SGI_video_sync to expose separate MSC for different CRTCs. I can see race condition even with events. * Client checks for event (none yet in queue) * Client prepares to call some blocking msc call * Window manager decides to move the window * xserver send MSC change event * Client calls blocking MSC call But maybe there is solution. All blocking calls could fail if MSC base changes. Client would have to query for new base and rate before trying to issue same request again. Yeah, that might work; I agree there's a race even with events that we'll need to handle. But even with that race handled I'd like the server to fail gracefully rather than hanging the app if an old MSC value is passed in (though in that case we could print an error message since we could assume an app error as long as the app was using the extended version of the spec). For swap interval it would be enough if DDX would notify DRI2 about crtc changes with offset (msc_for_new_crtc - msc_for_old_crtc) that can be applied to swaptarget. Yeah, just jumping to the new value might be ok in general. Hardcore libraries like Mario's can do something fancier with the extensions above to avoid unintended behavior. Thanks again for all your work on this. These are good improvements. -- Jesse Barnes, Intel Open Source Technology Center ___ 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
[PATCH 2/2] DRI2: Add error message when working around driver bug
There isn't API that allows application atomically query for msc changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
CC: Kristian Høgsberg k...@bitplanet.net
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
/*
--
1.7.0.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Mon, 25 Oct 2010 17:13:58 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
There isn't API that allows application atomically query for msc changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
CC: Kristian Høgsberg k...@bitplanet.net
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
This one scares me a little. We added this so we could also catch
drawables moving between screens with different msc bases, so this
patch could cause a lot of false positives (no question that the specs
could use some additions here though).
Making it a debug message that only shows up with -verbose would be
fine though.
--
Jesse Barnes, Intel Open Source Technology Center
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Oct 25, 2010, at 6:52 PM, Jesse Barnes wrote:
On Mon, 25 Oct 2010 17:13:58 +0300
Pauli Nieminen ext-pauli.niemi...@nokia.com wrote:
There isn't API that allows application atomically query for msc
changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
CC: Kristian Høgsberg k...@bitplanet.net
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
This one scares me a little. We added this so we could also catch
drawables moving between screens with different msc bases, so this
patch could cause a lot of false positives (no question that the specs
could use some additions here though).
Making it a debug message that only shows up with -verbose would be
fine though.
--
Jesse Barnes, Intel Open Source Technology Center
I agree with Jesse, as a debug message at -verbose it would be fine.
-mario
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On 07/10/10 00:35 +0200, ext Mario Kleiner wrote:
On Oct 6, 2010, at 1:05 PM, Pauli Nieminen wrote:
There isn't API that allows application atomically query for msc
changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Hi Paul,
this is not a driver bug which should be reported as an error, but
simply handling of what happens if a drawable is moved by the user
from one display head to a different display head, which likely has a
different msc count because it runs at a different refresh rate or
was enabled later than the first crtc.
I understand.
Same problem may happen for application using OML_sync_control. That why I
think it is important to notify that we are hitting work around.
Therefore i don't think this error printout should be added. It is
arguably not perfect, but probably good enough for the most common
cases. The correct solution would be to virtualize the msc counter
for each drawable and keep track of crtc changes and compensate for
those at each change. I have some ideas on how to do this properly,
just didn't have the time to test them.
Do you add some callback to common code that DDX has to call when drawable
moves to different CRTC? Parameters could be msc for old and new crtc. Then
common code could add msc offset to drawable that driver can use to schedule
swaps at correct time.
best,
-mario
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
/*
--
1.7.0.4
___
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
*
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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/2] DRI2: Add error message when working around driver bug
There isn't API that allows application atomically query for msc changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
/*
--
1.7.0.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] DRI2: Add error message when working around driver bug
On Oct 6, 2010, at 1:05 PM, Pauli Nieminen wrote:
There isn't API that allows application atomically query for msc
changes
and schedule swaps. If msc changes dramatically between query and
scheduling application would schedule swap to happen at wrong time.
Because of API limitations driver has to make msc increment for each
vblank affecting the drawable.
Hi Paul,
this is not a driver bug which should be reported as an error, but
simply handling of what happens if a drawable is moved by the user
from one display head to a different display head, which likely has a
different msc count because it runs at a different refresh rate or
was enabled later than the first crtc.
Therefore i don't think this error printout should be added. It is
arguably not perfect, but probably good enough for the most common
cases. The correct solution would be to virtualize the msc counter
for each drawable and keep track of crtc changes and compensate for
those at each change. I have some ideas on how to do this properly,
just didn't have the time to test them.
best,
-mario
Signed-off-by: Pauli Nieminen ext-pauli.niemi...@nokia.com
---
hw/xfree86/dri2/dri2.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index d9b9d57..d70c115 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
if (!(*ds-GetMSC)(pDraw, ust, current_msc))
pPriv-last_swap_target = 0;
- if (current_msc pPriv-last_swap_target)
+ if (current_msc pPriv-last_swap_target) {
pPriv-last_swap_target = current_msc;
-
+ xf86DrvMsg(pScreen-myNum, X_ERROR,
+ [DRI2] %s: GetMSC returned swap count that is in
+ past. Working around driver bug.\n, __func__);
+ }
}
/*
--
1.7.0.4
___
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
*
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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
