From: Jesse B. <jes...@in...> - 2007-09-26 18:43:32
Attachments:
mesa-core-vblank-support-5.patch
|
Per the discussion in the other vblank thread, this patch does several things: - adds a new getMSC hook to __DRIdrawableRec - updates glXGetVideoSyncSGI to use the new hook if present - adds new vblank fields to __DRIdrawablePrivateRec - adds a new driDrawableGetMSC32 vblank.c routine this new function takes a drawable private so it can set pipe flags for example when it calls into the DRM - adds a wrapper for the new vblank.c routine in dri_util.c for symmetry - updates driCreateNewDrawable to init the new vblank fields and set the callback for the new per-drawable getMSC routine - adds a __DriverAPI hook for GetDrawableMSC - updates the drivers to use driDrawableGetMSC32 in their DriverAPI initialization I'm not sure about the compatibility implications of these changes, it seems like touching __DRIdrawableRec and __DriverAPIRec may affect binary compatibility with existing drivers, is there anything else? If compatibility isn't a concern, I can go ahead and remove the per-screen getMSC altogether, along with its associated __DriverAPI function pointers and remove some code. Also, I decided against cleaning up the screen and drawable callback wrappers in dri_util.c. It seems they'll be needed if we ever add full OML extension support. I made the new stuff fit in with that scheme. Any thoughts? Thanks, Jesse include/GL/internal/dri_interface.h | 11 ++++++ src/glx/x11/glxcmds.c | 34 +++++++++++++++----- src/mesa/drivers/dri/common/dri_util.c | 12 +++++++ src/mesa/drivers/dri/common/dri_util.h | 17 ++++++++++ src/mesa/drivers/dri/common/vblank.c | 39 +++++++++++++++++++++-- src/mesa/drivers/dri/common/vblank.h | 3 + src/mesa/drivers/dri/ffb/ffb_xmesa.c | 1 src/mesa/drivers/dri/i810/i810screen.c | 1 src/mesa/drivers/dri/i915/intel_screen.c | 1 src/mesa/drivers/dri/i915tex/intel_screen.c | 1 src/mesa/drivers/dri/i965/intel_blit.c | 3 + src/mesa/drivers/dri/i965/intel_buffers.c | 41 +++++++++++++++++++++++++ src/mesa/drivers/dri/i965/intel_context.c | 14 ++++---- src/mesa/drivers/dri/i965/intel_context.h | 7 ---- src/mesa/drivers/dri/i965/intel_screen.c | 1 src/mesa/drivers/dri/i965/server/i830_common.h | 9 +++++ src/mesa/drivers/dri/mach64/mach64_screen.c | 1 src/mesa/drivers/dri/mga/mga_xmesa.c | 1 src/mesa/drivers/dri/nouveau/nouveau_screen.c | 2 - src/mesa/drivers/dri/r128/r128_screen.c | 1 src/mesa/drivers/dri/radeon/radeon_screen.c | 2 + src/mesa/drivers/dri/sis/sis_screen.c | 1 src/mesa/drivers/dri/tdfx/tdfx_screen.c | 1 src/mesa/drivers/dri/unichrome/via_screen.c | 1 24 files changed, 180 insertions(+), 25 deletions(-) |
From: Ian R. <id...@us...> - 2007-09-27 01:20:59
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jesse Barnes wrote: > Per the discussion in the other vblank thread, this patch does several > things: > - adds a new getMSC hook to __DRIdrawableRec > - updates glXGetVideoSyncSGI to use the new hook if present > - adds new vblank fields to __DRIdrawablePrivateRec > - adds a new driDrawableGetMSC32 vblank.c routine > this new function takes a drawable private so it can set pipe > flags for example when it calls into the DRM > - adds a wrapper for the new vblank.c routine in dri_util.c > for symmetry > - updates driCreateNewDrawable to init the new vblank fields > and set the callback for the new per-drawable getMSC routine > - adds a __DriverAPI hook for GetDrawableMSC > - updates the drivers to use driDrawableGetMSC32 in their > DriverAPI initialization > > I'm not sure about the compatibility implications of these changes, it > seems like touching __DRIdrawableRec and __DriverAPIRec may affect > binary compatibility with existing drivers, is there anything else? > > If compatibility isn't a concern, I can go ahead and remove the > per-screen getMSC altogether, along with its associated __DriverAPI > function pointers and remove some code. > > Also, I decided against cleaning up the screen and drawable callback > wrappers in dri_util.c. It seems they'll be needed if we ever add full > OML extension support. I made the new stuff fit in with that scheme. > > Any thoughts? I have to admit that I haven't been keeping as up to date with this thread as I should have. I've blocked off some time tomorrow morning to fully review this patch and the discussion. I'll probably have some questions for you, if you're on IRC. I do have one question now. What happens (or is intended to happen) if the currently bound drawable is not a window? Also, the current code already deviates from the spec. :( I somehow overlooked the bit in GLX_SGI_video_sync that says: glXGetVideoSyncSGI returns GLX_BAD_CONTEXT if there is no current GLXContext. glXWaitVideoSyncSGI returns GLX_BAD_CONTEXT if the current context is not direct, or if there is no current context. It looks like your patch maintains this behavior. Since the current drawable can only be None if there is no context, I believe that returning GLX_BAD_CONTEXT when getDrawable returns NULL would be correct. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFG+wVxX1gOwKyEAw8RAjTUAJ9qq6ExOdIMu5l5zsbPDztUKtayvQCgjCUH C+1jSvSgc0PX8KculQqI8Z8= =hqR7 -----END PGP SIGNATURE----- |
From: Jesse B. <jes...@in...> - 2007-09-27 17:52:46
|
On Wednesday, September 26, 2007 6:20 pm Ian Romanick wrote: > I have to admit that I haven't been keeping as up to date with this > thread as I should have. I've blocked off some time tomorrow morning > to fully review this patch and the discussion. I'll probably have > some questions for you, if you're on IRC. Yeah, I'm on IRC today. > I do have one question now. What happens (or is intended to happen) > if the currently bound drawable is not a window? > > Also, the current code already deviates from the spec. :( I somehow > overlooked the bit in GLX_SGI_video_sync that says: > > glXGetVideoSyncSGI returns GLX_BAD_CONTEXT if there is no current > GLXContext. > > glXWaitVideoSyncSGI returns GLX_BAD_CONTEXT if the current context > is not direct, or if there is no current context. > > It looks like your patch maintains this behavior. Since the current > drawable can only be None if there is no context, I believe that > returning GLX_BAD_CONTEXT when getDrawable returns NULL would be > correct. So rather than the "else if" that falls back to the old screen method you'd rather see us return GLX_BAD_CONTEXT at that point? That's easy enough to fix... ping me on IRC if you see anything else. Thanks, Jesse |
From: Michel <mi...@tu...> - 2007-09-28 09:39:28
|
On Wed, 2007-09-26 at 11:41 -0700, Jesse Barnes wrote: > > I'm not sure about the compatibility implications of these changes, it > seems like touching __DRIdrawableRec and __DriverAPIRec may affect > binary compatibility with existing drivers, is there anything else? > > If compatibility isn't a concern, I can go ahead and remove the > per-screen getMSC altogether, along with its associated __DriverAPI > function pointers and remove some code. dri_interface.h defines the interface between the loader (libGL or AIGLX) and the driver (*_dri.so). So you can't remove anything there and can only add stuff at the end in order to remain backwards compatible. __DriverAPIRec is internal to the driver AFAICT. > diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c > index c30e66f..4a4296e 100644 > --- a/src/mesa/drivers/dri/common/dri_util.c > +++ b/src/mesa/drivers/dri/common/dri_util.c > @@ -663,6 +674,7 @@ static void *driCreateNewDrawable(__DRInativeDisplay *dpy, > pdraw->swapBuffersMSC = driSwapBuffersMSC; > pdraw->frameTracking = NULL; > pdraw->queryFrameTracking = driQueryFrameTracking; > + pdraw->getMSC = driDrawableGetMSC; I think this needs guarding with if (driCompareGLXAPIVersion (20070925) >= 0) or a new driver will scribble past the end of __DRIdrawableRec with an old loader. You'll need to actually bump the API version as well though. :) > diff --git a/src/mesa/drivers/dri/common/vblank.c b/src/mesa/drivers/dri/common/vblank.c > index e7ed545..3974ee6 100644 > --- a/src/mesa/drivers/dri/common/vblank.c > +++ b/src/mesa/drivers/dri/common/vblank.c > @@ -50,19 +50,24 @@ > * currently always returns a \c sequence of type \c unsigned. > * > * \param priv Pointer to the DRI screen private struct. > + * \param drawablePrivate pointer to the DRI drawable private struct > * \param count Storage to hold MSC counter. > * \return Zero is returned on success. A negative errno value > * is returned on failure. > */ > -int driGetMSC32( __DRIscreenPrivate * priv, int64_t * count ) > +int driDrawableGetMSC32( __DRIscreenPrivate * priv, > + __DRIdrawablePrivate * dPriv, Mismatch between name of the added parameter in comment and code. > /* Don't wait for anything. Just get the current refresh count. */ > - > + Adding whitespace at the end of the line. > diff --git a/src/mesa/drivers/dri/i965/intel_context.c b/src/mesa/drivers/dri/i965/intel_context.c > index 1fbf571..05a9aec 100644 > --- a/src/mesa/drivers/dri/i965/intel_context.c > +++ b/src/mesa/drivers/dri/i965/intel_context.c > @@ -582,8 +583,9 @@ GLboolean intelMakeCurrent(__DRIcontextPrivate *driContextPriv, > > if ( intel->driDrawable != driDrawPriv ) { > /* Shouldn't the readbuffer be stored also? */ > - driDrawableInitVBlank( driDrawPriv, intel->vblank_flags, > - &intel->vbl_seq ); > + driDrawableInitVBlank( driDrawPriv, > + driDrawPriv->vblFlags, > + &driDrawPriv->vblSeq); driDrawableInitVBlank could take just driDrawPriv as a parameter now? > intel->driDrawable = driDrawPriv; > intelWindowMoved( intel ); This isn't new with your changes, but we should probably call intelWindowMoved before driDrawableInitVBlank, to make sure VBLANK_FLAG_SECONDARY is set for the latter when necessary. Can you make the corresponding changes to the i915 driver as well? Looking good overall, thanks for working on this! -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Ian R. <id...@us...> - 2007-09-28 18:03:05
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jesse Barnes wrote: > On Wednesday, September 26, 2007 6:20 pm Ian Romanick wrote: >> I do have one question now. What happens (or is intended to happen) >> if the currently bound drawable is not a window? Any thoughts on this? >> Also, the current code already deviates from the spec. :( I somehow >> overlooked the bit in GLX_SGI_video_sync that says: >> >> glXGetVideoSyncSGI returns GLX_BAD_CONTEXT if there is no current >> GLXContext. >> >> glXWaitVideoSyncSGI returns GLX_BAD_CONTEXT if the current context >> is not direct, or if there is no current context. >> >> It looks like your patch maintains this behavior. Since the current >> drawable can only be None if there is no context, I believe that >> returning GLX_BAD_CONTEXT when getDrawable returns NULL would be >> correct. > > So rather than the "else if" that falls back to the old screen method > you'd rather see us return GLX_BAD_CONTEXT at that point? That's easy > enough to fix... ping me on IRC if you see anything else. I was thinking of something like: if (pdraw == NULL) { return GLX_BAD_CONTEXT; } if (pdraw->getMSC != NULL) { .... } else if (psc->driScreen.getMSC != NULL) { .... } return (ret == 0) ? 0 : GLX_BAD_CONTEXT; I only have a couple other comments. 1. As Michael pointed out, anything in the driver that accesses pdraw->getMSC needs to check the API version first. 2. The comment in the function header that the MSC "will never decrease" should be amended. I should be able to guarantee that it will never decrease for a given drawable, With two displays at different refresh rates, an application could see two calls to getMSC for different drawables return decreasing values. 3. I'm not very familiar with how the Intel hardware works, but does the code guarantee that the MSC won't decrease if a window is moved from one display to another? 4. Only semi-related. Have you given any thought to implementing getSBC? Other than that, it looks fine to me. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFG/UHCX1gOwKyEAw8RAtTDAKCa8RTZHs/A2KZaJou9a6+fMJl4EgCZAWW9 AklurSsVdM3Aty02yvgv/sU= =w89h -----END PGP SIGNATURE----- |
From: Jesse B. <jes...@in...> - 2007-09-28 18:58:19
|
On Friday, September 28, 2007 11:02 am Ian Romanick wrote: > Jesse Barnes wrote: > > On Wednesday, September 26, 2007 6:20 pm Ian Romanick wrote: > >> I do have one question now. What happens (or is intended to > >> happen) if the currently bound drawable is not a window? > > Any thoughts on this? No, I'm not sure what should happen in this case. Doing a vblank sync'd buffer swap or vblank wait on an offscreen drawable doesn't make much sense does it? In what cases might those calls occur? > I was thinking of something like: > > if (pdraw == NULL) { > return GLX_BAD_CONTEXT; > } > > if (pdraw->getMSC != NULL) { > .... > } else if (psc->driScreen.getMSC != NULL) { > .... > } > > return (ret == 0) ? 0 : GLX_BAD_CONTEXT; Ok, that looks good. I'll fix it up. > I only have a couple other comments. > > 1. As Michael pointed out, anything in the driver that accesses > pdraw->getMSC needs to check the API version first. Done (I'll do a quick double check though). > 2. The comment in the function header that the MSC "will never > decrease" should be amended. I should be able to guarantee that it > will never decrease for a given drawable, With two displays at > different refresh rates, an application could see two calls to getMSC > for different drawables return decreasing values. Done. > 3. I'm not very familiar with how the Intel hardware works, but does > the code guarantee that the MSC won't decrease if a window is moved > from one display to another? No, in that case the MSC will change and possibly decrease. But drivers can handle that case by tracking which output a given drawable is on (or mostly on), so that the drawable is sync'd to the right value. > 4. Only semi-related. Have you given any thought to implementing > getSBC? Only briefly. It seems it could be done similarly though--by adding an sbcSeq value to the drawable and letting drivers update it... Jesse |
From: Jesse B. <jes...@in...> - 2007-09-28 19:33:07
Attachments:
mesa-core-vblank-support-7.patch
|
Ok, this version includes the changes suggested by Michel and Ian. - update ABI version - fixup glXGetVideoSyncSGI and glXWaitVideoSyncSGI semantics - cleanup driDrawableInitVBlank prototype - update all drivers to the new scheme The last bit especially needs review. The changes were fairly mechanical, but there were some slight differences between what the various drivers were doing with vblank counts. I've only tested on 965GM so far (which works nicely), but any fallout from the changes should be easy to debug. Thanks for your feedback so far, hopefully things will look good enough this time around that I can push it upstream. Thanks, Jesse |
From: Ian R. <id...@us...> - 2007-09-28 19:51:14
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jesse Barnes wrote: > On Friday, September 28, 2007 11:02 am Ian Romanick wrote: >> Jesse Barnes wrote: >>> On Wednesday, September 26, 2007 6:20 pm Ian Romanick wrote: >>>> I do have one question now. What happens (or is intended to >>>> happen) if the currently bound drawable is not a window? >> Any thoughts on this? > > No, I'm not sure what should happen in this case. Doing a vblank sync'd > buffer swap or vblank wait on an offscreen drawable doesn't make much > sense does it? In what cases might those calls occur? The spec doesn't say that an application can't call glXGetViewSyncSGI when a pbuffer is bound, so you can be sure that someone does. >> I was thinking of something like: >> >> if (pdraw == NULL) { >> return GLX_BAD_CONTEXT; >> } >> >> if (pdraw->getMSC != NULL) { >> .... >> } else if (psc->driScreen.getMSC != NULL) { >> .... >> } >> >> return (ret == 0) ? 0 : GLX_BAD_CONTEXT; > > Ok, that looks good. I'll fix it up. I take that back. I looked at the unpatched code (instead of just looking at the patch). The code already returns GLX_BAD_CONTEXT if the return from __glXGetCurrentContext is NULL. This means that pdraw *cannot* be NULL. >> I only have a couple other comments. >> >> 1. As Michael pointed out, anything in the driver that accesses >> pdraw->getMSC needs to check the API version first. > > Done (I'll do a quick double check though). > >> 2. The comment in the function header that the MSC "will never >> decrease" should be amended. I should be able to guarantee that it >> will never decrease for a given drawable, With two displays at >> different refresh rates, an application could see two calls to getMSC >> for different drawables return decreasing values. > > Done. > >> 3. I'm not very familiar with how the Intel hardware works, but does >> the code guarantee that the MSC won't decrease if a window is moved >> from one display to another? > > No, in that case the MSC will change and possibly decrease. But drivers > can handle that case by tracking which output a given drawable is on > (or mostly on), so that the drawable is sync'd to the right value. That's bad. The MSC for a drawable should never decrease. I can easily envision cases where that would cause problems for apps. Drivers will have to make sure that getMSC returns values that only increase. We can easily do that by keeping two values in the drawable private. One value tracks a base MSC, and the second value tracks the initial MSC on the current pipe when the base MSC was set. The driver's getMSC would then return "base + (pipe_current_MSC - pipe_base_MSC)". -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFG/VskX1gOwKyEAw8RAkObAKCbs0Lv0FDSV175cBY2wJ6WQlHtrwCgmsXM +cqEexR02GHLJVGLhTh786Y= =1NXb -----END PGP SIGNATURE----- |
From: Jesse B. <jes...@in...> - 2007-09-28 20:08:19
|
On Friday, September 28, 2007 12:51 pm Ian Romanick wrote: > > No, I'm not sure what should happen in this case. Doing a vblank > > sync'd buffer swap or vblank wait on an offscreen drawable doesn't > > make much sense does it? In what cases might those calls occur? > > The spec doesn't say that an application can't call glXGetViewSyncSGI > when a pbuffer is bound, so you can be sure that someone does. Yeah, the question is: what should it do? With the posted code, I think it'll get either the primary pipe's counter (if the drawable has never been a window) or the last pipe it was associated with. That seems sufficient... > >> I was thinking of something like: > >> > >> if (pdraw == NULL) { > >> return GLX_BAD_CONTEXT; > >> } > >> > >> if (pdraw->getMSC != NULL) { > >> .... > >> } else if (psc->driScreen.getMSC != NULL) { > >> .... > >> } > >> > >> return (ret == 0) ? 0 : GLX_BAD_CONTEXT; > > > > Ok, that looks good. I'll fix it up. > > I take that back. I looked at the unpatched code (instead of just > looking at the patch). The code already returns GLX_BAD_CONTEXT if > the return from __glXGetCurrentContext is NULL. This means that > pdraw *cannot* be NULL. Yeah I looked again right after posting the last patch and the current code seems ok. I'll remove the pdraw == NULL checks. > > No, in that case the MSC will change and possibly decrease. But > > drivers can handle that case by tracking which output a given > > drawable is on (or mostly on), so that the drawable is sync'd to > > the right value. > > That's bad. The MSC for a drawable should never decrease. I can > easily envision cases where that would cause problems for apps. > > Drivers will have to make sure that getMSC returns values that only > increase. We can easily do that by keeping two values in the > drawable private. One value tracks a base MSC, and the second value > tracks the initial MSC on the current pipe when the base MSC was set. > The driver's getMSC would then return "base + (pipe_current_MSC - > pipe_base_MSC)". Wouldn't we want to increment the base value by the difference between the last pipe value and the current one? But yeah, handling a drawable that moves between outputs with different vblank counters is a little tricky, I think we could handle making it monotonic in driDrwableGetMSC32 by using per-drawable, per-pipe vblSeq values as you suggest. Jesse |
From: Michel <mi...@tu...> - 2007-10-01 09:07:59
|
On Fri, 2007-09-28 at 12:31 -0700, Jesse Barnes wrote: > Ok, this version includes the changes suggested by Michel and Ian. > - update ABI version > - fixup glXGetVideoSyncSGI and glXWaitVideoSyncSGI semantics Do you mean making sure the MSC never decreases for a given drawable? I see you changed the comments about this, but I don't see how the code actually ensures it. Can you elaborate? > - cleanup driDrawableInitVBlank prototype > - update all drivers to the new scheme > > The last bit especially needs review. The changes were fairly > mechanical, but there were some slight differences between what the > various drivers were doing with vblank counts. Looks good to me. BTW, you could have saved yourself some work here by starting after the i915-unification merge. :) > - unsigned int interval = driGetVBlankInterval(dPriv, intel->vblank_flags); > + unsigned int interval = driGetVBlankInterval(dPriv, dPriv->vblFlags); I guess driGetVBlankInterval could be changed to take just dPriv as well (sorry I didn't think of this before). -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-10-01 20:06:48
|
On Monday, October 1, 2007 2:07:44 am Michel D=C3=A4nzer wrote: > On Fri, 2007-09-28 at 12:31 -0700, Jesse Barnes wrote: > > Ok, this version includes the changes suggested by Michel and Ian. > > - update ABI version > > - fixup glXGetVideoSyncSGI and glXWaitVideoSyncSGI semantics > > Do you mean making sure the MSC never decreases for a given drawable? I > see you changed the comments about this, but I don't see how the code > actually ensures it. Can you elaborate? Well, it doesn't yet, but it's supposed to. See the last exchange between = idr=20 and myself. > > - cleanup driDrawableInitVBlank prototype > > - update all drivers to the new scheme > > > > The last bit especially needs review. The changes were fairly > > mechanical, but there were some slight differences between what the > > various drivers were doing with vblank counts. > > Looks good to me. BTW, you could have saved yourself some work here by > starting after the i915-unification merge. :) Yeah. :) I don't think it'll be too hard to merge though. > > - unsigned int interval =3D driGetVBlankInterval(dPriv, > > intel->vblank_flags); + unsigned int interval =3D > > driGetVBlankInterval(dPriv, dPriv->vblFlags); > > I guess driGetVBlankInterval could be changed to take just dPriv as well > (sorry I didn't think of this before). I should have caught this too. Will fix. Jesse |
From: Jesse B. <jes...@in...> - 2007-10-22 23:01:51
Attachments:
mesa-core-vblank-support-8.patch
|
On Friday, September 28, 2007 1:06 pm Jesse Barnes wrote: > On Friday, September 28, 2007 12:51 pm Ian Romanick wrote: > > > No, I'm not sure what should happen in this case. Doing a vblank > > > sync'd buffer swap or vblank wait on an offscreen drawable > > > doesn't make much sense does it? In what cases might those calls > > > occur? > > > > The spec doesn't say that an application can't call > > glXGetViewSyncSGI when a pbuffer is bound, so you can be sure that > > someone does. > > Yeah, the question is: what should it do? With the posted code, I > think it'll get either the primary pipe's counter (if the drawable > has never been a window) or the last pipe it was associated with. > That seems sufficient... Sorry for the delay, got sidetracked for a few weeeks with other stuff. Any comment on the "driDrawableGetMSC32() on pbuffer" case? Maybe returning GLX_BAD_VALUE (or some other EINVAL eqivalent) makes more sense? > > >> I was thinking of something like: > > >> > > >> if (pdraw == NULL) { > > >> return GLX_BAD_CONTEXT; > > >> } > > >> > > >> if (pdraw->getMSC != NULL) { > > >> .... > > >> } else if (psc->driScreen.getMSC != NULL) { > > >> .... > > >> } > > >> > > >> return (ret == 0) ? 0 : GLX_BAD_CONTEXT; > > > > > > Ok, that looks good. I'll fix it up. > > > > I take that back. I looked at the unpatched code (instead of just > > looking at the patch). The code already returns GLX_BAD_CONTEXT if > > the return from __glXGetCurrentContext is NULL. This means that > > pdraw *cannot* be NULL. > > Yeah I looked again right after posting the last patch and the > current code seems ok. I'll remove the pdraw == NULL checks. I fixed this. The patch is a little cleaner for it. > > > No, in that case the MSC will change and possibly decrease. But > > > drivers can handle that case by tracking which output a given > > > drawable is on (or mostly on), so that the drawable is sync'd to > > > the right value. > > > > That's bad. The MSC for a drawable should never decrease. I can > > easily envision cases where that would cause problems for apps. > > > > Drivers will have to make sure that getMSC returns values that only > > increase. We can easily do that by keeping two values in the > > drawable private. One value tracks a base MSC, and the second > > value tracks the initial MSC on the current pipe when the base MSC > > was set. The driver's getMSC would then return "base + > > (pipe_current_MSC - pipe_base_MSC)". > > Wouldn't we want to increment the base value by the difference > between the last pipe value and the current one? > > But yeah, handling a drawable that moves between outputs with > different vblank counters is a little tricky, I think we could handle > making it monotonic in driDrwableGetMSC32 by using per-drawable, > per-pipe vblSeq values as you suggest. Thinking about this more, I think we can make the counter not decrease, but I don't think we can avoid bad behavior. There are two possible failure modes: 1) counter goes backward when the window moves to another screen In this case, unless the application continuously updates its internal vblank counter (used for later calls to VideoSync), this may cause the next VideoSync call to timeout in the DRM (3s iirc). This is a fairly obvious failure. 2) we fix (1) and the counter jumps a large amount when the window moves to another screen In this case, the app may or may not see the same timeout as (1), depending on refresh rates and the size of the jump. The real problem here is two screens running at different refresh rates, since we can avoid large jumps when the refresh rates are the same. Since the VideoSyncSGI extension was designed with genlocking in mind, I'm inclined to leave it alone. At the very least, it seems like it should be dealt with in a different patch from this one, but more likely we'll just have to warn application developers that the vblank value they see is associated with the screen their window is on, so they need to be careful to properly deal with changes... Any other thoughts? Any objections to me pushing the attached? Thanks, Jesse |
From: Michel <mi...@tu...> - 2007-10-23 14:32:56
|
On Mon, 2007-10-22 at 16:00 -0700, Jesse Barnes wrote: > On Friday, September 28, 2007 1:06 pm Jesse Barnes wrote: > > On Friday, September 28, 2007 12:51 pm Ian Romanick wrote: > > > > > No, in that case the MSC will change and possibly decrease. But > > > > drivers can handle that case by tracking which output a given > > > > drawable is on (or mostly on), so that the drawable is sync'd to > > > > the right value. > > > > > > That's bad. The MSC for a drawable should never decrease. I can > > > easily envision cases where that would cause problems for apps. > > > > > > Drivers will have to make sure that getMSC returns values that only > > > increase. We can easily do that by keeping two values in the > > > drawable private. One value tracks a base MSC, and the second > > > value tracks the initial MSC on the current pipe when the base MSC > > > was set. The driver's getMSC would then return "base + > > > (pipe_current_MSC - pipe_base_MSC)". > > > > Wouldn't we want to increment the base value by the difference > > between the last pipe value and the current one? > > > > But yeah, handling a drawable that moves between outputs with > > different vblank counters is a little tricky, I think we could handle > > making it monotonic in driDrwableGetMSC32 by using per-drawable, > > per-pipe vblSeq values as you suggest. > > Thinking about this more, I think we can make the counter not decrease, > but I don't think we can avoid bad behavior. Why not, with something like the scheme Ian outlined above? -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jes...@in...> - 2007-10-23 17:09:52
|
On Tuesday, October 23, 2007 7:32 am Michel D=C3=A4nzer wrote: > > Thinking about this more, I think we can make the counter not > > decrease, but I don't think we can avoid bad behavior. > > Why not, with something like the scheme Ian outlined above? You snipped out the reasons: we'll get bad behavior of one sort or=20 another no matter what, unless we have genlocked displays. Jesse |
From: Jesse B. <jes...@in...> - 2007-10-23 17:33:55
|
On Tuesday, October 23, 2007 10:03 am Jesse Barnes wrote: > On Tuesday, October 23, 2007 7:32 am Michel D=C3=A4nzer wrote: > > > Thinking about this more, I think we can make the counter not > > > decrease, but I don't think we can avoid bad behavior. > > > > Why not, with something like the scheme Ian outlined above? > > You snipped out the reasons: we'll get bad behavior of one sort or > another no matter what, unless we have genlocked displays. Ok, you guys banged this into my head on IRC. So the calculation will=20 be: msc +=3D last_pipe_vblank_count - cur_pipeX_vblank_count like Ian said (where last_pipeX_vblank_count is updated everytime the=20 drawable moves onto pipe X). This gets a little ugly since I'll need=20 to track both a per-drawable MSC count as well as per-pipe=20 last_pipeX_vblank_count values. And unfortunately, since the drawable=20 movement is tracked in per-driver SAREA fields, this code can't be=20 generic. Ugg. Thanks, Jesse |
From: Jesse B. <jes...@in...> - 2007-10-24 23:14:46
Attachments:
mesa-core-vblank-support-10.patch
|
On Tuesday, October 23, 2007 10:32 am Jesse Barnes wrote: > On Tuesday, October 23, 2007 10:03 am Jesse Barnes wrote: > > On Tuesday, October 23, 2007 7:32 am Michel D=C3=A4nzer wrote: > > > > Thinking about this more, I think we can make the counter not > > > > decrease, but I don't think we can avoid bad behavior. > > > > > > Why not, with something like the scheme Ian outlined above? > > > > You snipped out the reasons: we'll get bad behavior of one sort or > > another no matter what, unless we have genlocked displays. > > Ok, you guys banged this into my head on IRC. So the calculation > will be: > msc +=3D last_pipe_vblank_count - cur_pipeX_vblank_count > like Ian said (where last_pipeX_vblank_count is updated everytime the > drawable moves onto pipe X). This gets a little ugly since I'll need > to track both a per-drawable MSC count as well as per-pipe > last_pipeX_vblank_count values. And unfortunately, since the > drawable movement is tracked in per-driver SAREA fields, this code > can't be generic. Ugg. Well at least some of the code can be generic, fortunately. Here's what=20 I have. The extension support seems to have changed a bit since my=20 last patch (this one is against Mesa HEAD as of yesterday), and I'm no=20 longer sure about the compatibility issues with the patch. It still has some bugs. When moving windows between screens, Mesa seems=20 to lose track of the right vblank count to sync against sometimes, so=20 my test app's calls to glXWaitVideoSyncSGI return immediately. Moving=20 the window back and forth between the screens a few times fixes this=20 problem though, maybe there's a race in the update of base_msc and=20 which pipe to sync against? On the plus side, I never see the MSC count decrease, so at least that=20 part is working I think. Jesse |
From: Jesse B. <jes...@in...> - 2007-10-24 23:26:16
Attachments:
mesa-core-vblank-support-11.patch
|
Oops, this one fixes a couple of places where I was miscalculating the actual MSC value. Jesse |
From: Michel <mi...@tu...> - 2007-10-25 09:02:17
|
On Wed, 2007-10-24 at 16:10 -0700, Jesse Barnes wrote: > On Tuesday, October 23, 2007 10:32 am Jesse Barnes wrote: > > On Tuesday, October 23, 2007 10:03 am Jesse Barnes wrote: > > > On Tuesday, October 23, 2007 7:32 am Michel Dänzer wrote: > > > > > Thinking about this more, I think we can make the counter not > > > > > decrease, but I don't think we can avoid bad behavior. > > > > > > > > Why not, with something like the scheme Ian outlined above? > > > > > > You snipped out the reasons: we'll get bad behavior of one sort or > > > another no matter what, unless we have genlocked displays. > > > > Ok, you guys banged this into my head on IRC. So the calculation > > will be: > > msc += last_pipe_vblank_count - cur_pipeX_vblank_count > > like Ian said (where last_pipeX_vblank_count is updated everytime the > > drawable moves onto pipe X). This gets a little ugly since I'll need > > to track both a per-drawable MSC count as well as per-pipe > > last_pipeX_vblank_count values. And unfortunately, since the > > drawable movement is tracked in per-driver SAREA fields, this code > > can't be generic. Ugg. > > Well at least some of the code can be generic, fortunately. Here's what > I have. The extension support seems to have changed a bit since my > last patch (this one is against Mesa HEAD as of yesterday), and I'm no > longer sure about the compatibility issues with the patch. > > It still has some bugs. When moving windows between screens, Mesa seems > to lose track of the right vblank count to sync against sometimes, so > my test app's calls to glXWaitVideoSyncSGI return immediately. Moving > the window back and forth between the screens a few times fixes this > problem though, maybe there's a race in the update of base_msc and > which pipe to sync against? There can't really be a race with a single context... If this is still an issue with your updated patch, it might be related to some of the issues below. > On the plus side, I never see the MSC count decrease, so at least that > part is working I think. Nice! > diff --git a/configs/default b/configs/default The configs/ changes probably shouldn't be in here. > diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h > index e7fbf8e..f1d236a 100644 > --- a/include/GL/internal/dri_interface.h > +++ b/include/GL/internal/dri_interface.h > @@ -189,6 +189,18 @@ struct __DRImediaStreamCounterExtensionRec { > int (*waitForMSC)(__DRIdrawable *drawable, > int64_t target_msc, int64_t divisor, int64_t remainder, > int64_t * msc, int64_t * sbc); > + > + /** > + * Like the screen version of getMSC, but also takes a drawable so that > + * the appropriate pipe's counter can be retrieved. > + * > + * Get the number of vertical refreshes since some point in time before > + * this function was first called (i.e., system start up). > + * > + * \since Internal API version 20070925. __DRImediaStreamCounterExtensionRec has its own versioning now, you need to bump __DRI_MEDIA_STREAM_COUNTER_VERSION to 2. > diff --git a/src/glx/x11/glxcmds.c b/src/glx/x11/glxcmds.c > index 707e398..cbc498a 100644 > --- a/src/glx/x11/glxcmds.c > +++ b/src/glx/x11/glxcmds.c [...] > + if ( psc->driScreen.private ) { Should be if ( psc->msc && psc->driScreen.private ) { > + /* > + * Try to use getDrawableMSC first so we get the right > + * counter... > + */ > + if (psc->msc->getDrawableMSC) > + ret = (*psc->msc->getDrawableMSC)( &psc->driScreen, > + pdraw->private, > + & temp); > + else > + ret = (*psc->msc->getMSC)( &psc->driScreen, & temp); I think you need to verify that (psc->msc->base.version >= 2) before accessing psc->msc->getDrawableMSC. > diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c > index d59ea0d..d023231 100644 > --- a/src/mesa/drivers/dri/common/dri_util.c > +++ b/src/mesa/drivers/dri/common/dri_util.c > @@ -471,6 +480,8 @@ static void *driCreateNewDrawable(__DRIscreen *screen, > pdp->numBackClipRects = 0; > pdp->pClipRects = NULL; > pdp->pBackClipRects = NULL; > + pdp->vblSeq = 0; > + pdp->vblFlags = 0; > > psp = (__DRIscreenPrivate *)screen->private; > pdp->driScreenPriv = psp; > @@ -485,6 +496,11 @@ static void *driCreateNewDrawable(__DRIscreen *screen, > pdraw->private = pdp; > pdraw->destroyDrawable = driDestroyDrawable; > pdraw->swapBuffers = driSwapBuffers; /* called by glXSwapBuffers() */ > + if (driCompareGLXAPIVersion(20070925) >= 0) { > + pdp->msc = 0; > + pdp->base_msc = 0; > + } > + I'm not sure this API version needs to be checked here or even bumped. Either way though, all added fields should probably be treated the same way. > diff --git a/src/mesa/drivers/dri/common/dri_util.h b/src/mesa/drivers/dri/common/dri_util.h > index 91992a9..1e5301f 100644 > --- a/src/mesa/drivers/dri/common/dri_util.h > +++ b/src/mesa/drivers/dri/common/dri_util.h [...] > + /** > + * \name Monotonic MSC tracking > + * > + * Low level driver is responsible for updating base_msc, primary and > + * secondary_vblank_base values so that higher level code can calculate > + * a new msc value or msc target for a WaitMSC call. The new value will > + * be: > + * if (pipe == primary) > + * msc = base_msc + get_vblank_count(primary) - primary_vblank_base; > + * else > + * msc = base_msc + get_vblank_count(secondary) - secondary_vblank_base; > + * > + * And for waiting on a value, core code will use: > + * actual_target = target_msc - base_msc + > + * (primary|secondary)_vblank_base; > + */ > + /*@{*/ > + int64_t msc; > + int64_t base_msc; > + int64_t primary_vblank_base; > + int64_t secondary_vblank_base; Shouldn't these be unsigned? Do we really need two vblank_base values? Couldn't there be just one for the current pipe? Also, I'm not sure the msc field is needed here, couldn't the driver just increase base_msc by <current sequence of old pipe> - vblank_base? Actually I think something like that is needed anyway, as the msc field could be stale at that point? (BTW, the i915 driver changes for this seem to be missing.) > diff --git a/src/mesa/drivers/dri/common/vblank.c b/src/mesa/drivers/dri/common/vblank.c > index 3b5acfe..032cd40 100644 > --- a/src/mesa/drivers/dri/common/vblank.c > +++ b/src/mesa/drivers/dri/common/vblank.c [...] > - *count = (int64_t)vbl.reply.sequence; > + > + if ( dPriv && !(dPriv->vblFlags & VBLANK_FLAG_SECONDARY) ) { > + /* Primary pipe */ > + dPriv->msc = dPriv->base_msc + vbl.reply.sequence - > + dPriv->primary_vblank_base; > + } else if (dPriv && (dPriv->vblFlags & VBLANK_FLAG_SECONDARY) ) { > + /* Secondary pipe */ > + dPriv->msc = dPriv->base_msc + vbl.reply.sequence - > + dPriv->secondary_vblank_base; > + } else { > + /* Old driver (no knowledge of per-drawable MSC callback) */ > + dPriv->msc += vbl.reply.sequence; > + } The last else block doesn't seem to make sense - for one, it could only seem to be hit if dPriv == NULL, in which case it would segfault. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jes...@in...> - 2007-10-25 17:34:50
|
On Thursday, October 25, 2007 2:02 am Michel D=C3=A4nzer wrote: > > It still has some bugs. When moving windows between screens, Mesa > > seems to lose track of the right vblank count to sync against > > sometimes, so my test app's calls to glXWaitVideoSyncSGI return > > immediately. Moving the window back and forth between the screens > > a few times fixes this problem though, maybe there's a race in the > > update of base_msc and which pipe to sync against? > > There can't really be a race with a single context... If this is > still an issue with your updated patch, it might be related to some > of the issues below. I was thinking the internal state may be updated by the intelWindowMoved=20 function after the client calls glXWaitVideoSyncSGI but before it=20 returned... So not really a race between two threads, but possible=20 confusion anyway. Or is that not possible? > > On the plus side, I never see the MSC count decrease, so at least > > that part is working I think. > > Nice! > > > diff --git a/configs/default b/configs/default > > The configs/ changes probably shouldn't be in here. Yeah, just a bit of frustration avoidance that snuck in... > > + * this function was first called (i.e., system start up). > > + * > > + * \since Internal API version 20070925. > > __DRImediaStreamCounterExtensionRec has its own versioning now, you > need to bump __DRI_MEDIA_STREAM_COUNTER_VERSION to 2. Ah ok, I missed that. > > + if ( psc->driScreen.private ) { > > Should be > > if ( psc->msc && psc->driScreen.private ) { Ok. > > > + /* > > + * Try to use getDrawableMSC first so we get the right > > + * counter... > > + */ > > + if (psc->msc->getDrawableMSC) > > + ret =3D (*psc->msc->getDrawableMSC)( &psc->driScreen, > > + pdraw->private, > > + & temp); > > + else > > + ret =3D (*psc->msc->getMSC)( &psc->driScreen, & temp); > > I think you need to verify that (psc->msc->base.version >=3D 2) before > accessing psc->msc->getDrawableMSC. Ok. > > pdraw->swapBuffers =3D driSwapBuffers; /* called by > > glXSwapBuffers() */=20 > > + if (driCompareGLXAPIVersion(20070925) >=3D 0) { > > + pdp->msc =3D 0; > > + pdp->base_msc =3D 0; > > + } > > + > > I'm not sure this API version needs to be checked here or even > bumped. Either way though, all added fields should probably be > treated the same way. I guess I'll just remove the check then. > > + /*@{*/ > > + int64_t msc; > > + int64_t base_msc; > > + int64_t primary_vblank_base; > > + int64_t secondary_vblank_base; > > Shouldn't these be unsigned? Elsewhere we pass around an int64_t *msc, so I just kept it consistent. =20 But yes, the values are inherently unsigned. > Do we really need two vblank_base values? Couldn't there be just one > for the current pipe? Yeah, you're right I could just consolidate them like the vblFlags=20 field. > Also, I'm not sure the msc field is needed here, couldn't the driver > just increase base_msc by <current sequence of old pipe> - > vblank_base? Actually I think something like that is needed anyway, > as the msc field could be stale at that point? Yeah, I was thinking the same thing after I posted, I'll drop the msc=20 field too. > (BTW, the i915 driver changes for this seem to be missing.) Yeah, I've been testing on 965. I'll add the 915 changes when I have=20 965 working, then test there. > > - *count =3D (int64_t)vbl.reply.sequence; > > + > > + if ( dPriv && !(dPriv->vblFlags & VBLANK_FLAG_SECONDARY) ) { > > + /* Primary pipe */ > > + dPriv->msc =3D dPriv->base_msc + vbl.reply.sequence - > > + dPriv->primary_vblank_base; > > + } else if (dPriv && (dPriv->vblFlags & VBLANK_FLAG_SECONDARY) ) > > { + /* Secondary pipe */ > > + dPriv->msc =3D dPriv->base_msc + vbl.reply.sequence - > > + dPriv->secondary_vblank_base; > > + } else { > > + /* Old driver (no knowledge of per-drawable MSC callback) */ > > + dPriv->msc +=3D vbl.reply.sequence; > > + } > > The last else block doesn't seem to make sense - for one, it could > only seem to be hit if dPriv =3D=3D NULL, in which case it would > segfault. Oh yeah, another reason for dropping dPriv->msc. :) Obviously this=20 codepath was untested. It really should just return vbl.reply.sequence=20 as the count if dPriv is NULL (indicating a call from an old driver). Thanks, Jesse |
From: Michel <mi...@tu...> - 2007-10-26 08:14:07
|
On Thu, 2007-10-25 at 10:25 -0700, Jesse Barnes wrote: > On Thursday, October 25, 2007 2:02 am Michel Dänzer wrote: > > > It still has some bugs. When moving windows between screens, Mesa > > > seems to lose track of the right vblank count to sync against > > > sometimes, so my test app's calls to glXWaitVideoSyncSGI return > > > immediately. Moving the window back and forth between the screens > > > a few times fixes this problem though, maybe there's a race in the > > > update of base_msc and which pipe to sync against? > > > > There can't really be a race with a single context... If this is > > still an issue with your updated patch, it might be related to some > > of the issues below. > > I was thinking the internal state may be updated by the intelWindowMoved > function after the client calls glXWaitVideoSyncSGI but before it > returned... So not really a race between two threads, but possible > confusion anyway. Or is that not possible? Shouldn't be - the driver only calls WindowMoved after taking the DRM lock (generally during rendering operations) or when the window is bound to a context. There could probably be races between several threads though, some kind of mutex might be necessary to protect against those. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jes...@in...> - 2007-10-26 16:27:36
|
On Friday, October 26, 2007 1:14 am Michel D=C3=A4nzer wrote: > On Thu, 2007-10-25 at 10:25 -0700, Jesse Barnes wrote: > > On Thursday, October 25, 2007 2:02 am Michel D=C3=A4nzer wrote: > > > > It still has some bugs. When moving windows between screens, > > > > Mesa seems to lose track of the right vblank count to sync > > > > against sometimes, so my test app's calls to > > > > glXWaitVideoSyncSGI return immediately. Moving the window back > > > > and forth between the screens a few times fixes this problem > > > > though, maybe there's a race in the update of base_msc and > > > > which pipe to sync against? > > > > > > There can't really be a race with a single context... If this is > > > still an issue with your updated patch, it might be related to > > > some of the issues below. > > > > I was thinking the internal state may be updated by the > > intelWindowMoved function after the client calls > > glXWaitVideoSyncSGI but before it returned... So not really a race > > between two threads, but possible confusion anyway. Or is that not > > possible? > > Shouldn't be - the driver only calls WindowMoved after taking the DRM > lock (generally during rendering operations) or when the window is > bound to a context. > > There could probably be races between several threads though, some > kind of mutex might be necessary to protect against those. Yeah, it should only be called from makecurrent and the contendedlock=20 functions, right? I was thinking it would be called asynchronously=20 somehow, but I guess not, so I must have a problem in the WaitMSC=20 function somewhere. As for actual multithreaded programs, how is that dealt with in the rest=20 of Mesa? Jesse |
From: Jesse B. <jes...@in...> - 2007-10-26 18:03:53
Attachments:
mesa-core-vblank-support-12.patch
|
On Thursday, October 25, 2007 10:25 am Jesse Barnes wrote: > Oh yeah, another reason for dropping dPriv->msc. :) Obviously this > codepath was untested. It really should just return > vbl.reply.sequence as the count if dPriv is NULL (indicating a call > from an old driver). Ok, I fixed all the problems you mentioned and the last of the bugs I found. Things work beautifully with my test program now, so hopefully I can push this soon. The i915 changes are untested however, I won't be able to test them until I get home. The conversion was pretty straightforward though, I just removed the intel_fb specific vblank_flags and vbl_seq values and switched to using the ones in the drawable private instead. Hopefully this didn't break page flipping. Thanks a lot for all your review, I know it's a pain, but I think the patch is a lot better for it. Jesse |
From: Michel <mi...@tu...> - 2007-10-29 10:01:31
|
On Fri, 2007-10-26 at 10:59 -0700, Jesse Barnes wrote: > > diff --git a/src/glx/x11/glxcmds.c b/src/glx/x11/glxcmds.c > index 707e398..25eaccc 100644 > --- a/src/glx/x11/glxcmds.c > +++ b/src/glx/x11/glxcmds.c > @@ -2919,8 +2928,10 @@ int __glXGetInternalVersion(void) > * any DRI driver built to any previous version. > * 20060314 - Added support for GLX_MESA_copy_sub_buffer. > * 20070105 - Added support for damage reporting. > + * 20070925 - Added support for per-drawable getMSC callbacks, which > + * allows the core to support vblank events on multiple pipes. > */ > - return 20070105; > + return 20070925; > } So is this still necessary/useful? > diff --git a/src/mesa/drivers/dri/common/dri_util.h b/src/mesa/drivers/dri/common/dri_util.h > index 91992a9..74b54a5 100644 > --- a/src/mesa/drivers/dri/common/dri_util.h > +++ b/src/mesa/drivers/dri/common/dri_util.h > @@ -318,6 +326,32 @@ struct __DRIdrawablePrivateRec { > /*@}*/ > > /** > + * \name Vertical blank tracking information > + * Used for waiting on vertical blank events. > + */ > + /*@{*/ > + unsigned int vblSeq; > + unsigned int vblFlags; > + /*@}*/ > + > + /** > + * \name Monotonic MSC tracking > + * > + * Low level driver is responsible for updating msc_base and > + * vblSeq values so that higher level code can calculate > + * a new msc value or msc target for a WaitMSC call. The new value > + * will be: > + * msc = msc_base + get_vblank_count(primary) - vblank_base; This should probably say something other than 'primary' now. > @@ -131,8 +180,10 @@ int driWaitForMSC32( __DRIdrawablePrivate *priv, > return GLX_BAD_CONTEXT; > } > > + *msc = vblank_to_msc(priv, vbl.reply.sequence); > + > dont_wait = 0; > - if (target_msc != 0 && vbl.reply.sequence == target) > + if (target_msc != 0 && *msc == target) Shouldn't the latter be something like *msc == target_msc now? > diff --git a/src/mesa/drivers/dri/i915/intel_buffers.c b/src/mesa/drivers/dri/i915/intel_buffers.c > index 46a67b1..67ea226 100644 > --- a/src/mesa/drivers/dri/i915/intel_buffers.c > +++ b/src/mesa/drivers/dri/i915/intel_buffers.c > > + /* > + * Update msc_base from old pipe > + */ > + driDrawableGetMSC32(dPriv->driScreenPriv, dPriv, &count); > + dPriv->msc_base = count; > + /* > + * Then get new vblank_base and vblSeq values > + */ > + dPriv->vblFlags = flags; > + driGetCurrentVBlank(dPriv, dPriv->vblFlags, &dPriv->vblSeq); > + dPriv->vblank_base = dPriv->vblSeq; I think this breaks the code following immediately, which waits for pending flips on the old plane (guess I should have commented that...). I'd put the new code around the same place where intel_fb->vblank_flags was updated previously. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: <kr...@bi...> - 2007-10-29 18:28:57
|
T24gMTAvMjkvMDcsIE1pY2hlbCBEw6RuemVyIDxtaWNoZWxAdHVuZ3N0ZW5ncmFwaGljcy5jb20+ IHdyb3RlOgo+Cj4gT24gRnJpLCAyMDA3LTEwLTI2IGF0IDEwOjU5IC0wNzAwLCBKZXNzZSBCYXJu ZXMgd3JvdGU6Cj4gPgo+ID4gZGlmZiAtLWdpdCBhL3NyYy9nbHgveDExL2dseGNtZHMuYyBiL3Ny Yy9nbHgveDExL2dseGNtZHMuYwo+ID4gaW5kZXggNzA3ZTM5OC4uMjVlYWNjYyAxMDA2NDQKPiA+ IC0tLSBhL3NyYy9nbHgveDExL2dseGNtZHMuYwo+ID4gKysrIGIvc3JjL2dseC94MTEvZ2x4Y21k cy5jCj4gPiBAQCAtMjkxOSw4ICsyOTI4LDEwIEBAIGludCBfX2dsWEdldEludGVybmFsVmVyc2lv bih2b2lkKQo+ID4gICAgICAgKiAgICAgICAgICAgIGFueSBEUkkgZHJpdmVyIGJ1aWx0IHRvIGFu eSBwcmV2aW91cyB2ZXJzaW9uLgo+ID4gICAgICAgKiAyMDA2MDMxNCAtIEFkZGVkIHN1cHBvcnQg Zm9yIEdMWF9NRVNBX2NvcHlfc3ViX2J1ZmZlci4KPiA+ICAgICAgICogMjAwNzAxMDUgLSBBZGRl ZCBzdXBwb3J0IGZvciBkYW1hZ2UgcmVwb3J0aW5nLgo+ID4gKyAgICAgKiAyMDA3MDkyNSAtIEFk ZGVkIHN1cHBvcnQgZm9yIHBlci1kcmF3YWJsZSBnZXRNU0MgY2FsbGJhY2tzLCB3aGljaAo+ID4g KyAgICAgKiAgICAgICAgICAgIGFsbG93cyB0aGUgY29yZSB0byBzdXBwb3J0IHZibGFuayBldmVu dHMgb24gbXVsdGlwbGUgcGlwZXMuCj4gPiAgICAgICAqLwo+ID4gLSAgICByZXR1cm4gMjAwNzAx MDU7Cj4gPiArICAgIHJldHVybiAyMDA3MDkyNTsKPiA+ICB9Cj4KPiBTbyBpcyB0aGlzIHN0aWxs IG5lY2Vzc2FyeS91c2VmdWw/CgpObywgYW5kIEkgc2hvdWxkIHByb2JhYmx5IHJlbW92ZSB0aGUg YXBpX3ZlcnNpb24gYXJndW1lbnQgZnJvbSB0aGUKQ3JlYXRlTmV3U2NyZWVuIGVudHJ5IHBvaW50 IG5vdyB0aGF0IHRoZSBEUkkgZHJpdmVyIGFsbG9jYXRlcyBzdG9yYWdlCmZvciB0aGUgZXh0ZW5z aW9uIHN0cnVjdHMuICBUaGUgaXNzdWUgdXNlZCB0byBiZSB0aGF0IHRoZSBEUkkgZHJpdmVyCmRp ZG4ndCBrbm93IHdoaWNoIHZlcnNpb24gb2YgdGhlIERSSSBpbnRlcmZhY2UgdGhlIGxvYWRlciB3 YXMgY29tcGlsZWQKYWdhaW5zdCBhbmQgYXMgc3VjaCBjb3VsZG4ndCBmaWxsIG91dCBmaWVsZHMg YXBwZW5kZWQgdG8gdGhlIHN0cnVjdAp0aGF0IHRoZSBsb2FkZXIgZGlkbid0IGtub3cgYWJvdXQu CgpUaGlzIGlzIG5vIGxvbmdlciBhbiBpc3N1ZSwgbm93IHRoYXQgdGhlIGRyaXZlciByZXR1cm5z IHN0cnVjdHMgb2YKZnVuY3Rpb24gcG9pbnRlcnMgYWxsb2NhdGVkIGJ5IHRoZSBkcml2ZXIsIHNv IHdlIHNob3VsZCBiZSBhYmxlIHRvCmRyb3AgdGhpcyBhcmd1bWVudC4gIFRoZSBjb3JlIEdMWCBv YmplY3RzIChTY3JlZW4sIERyYXdhYmxlIGFuZApDb250ZXh0KSBhcmUgc3RpbGwgYWxsb2NhdGVk IHRoZSBvbGQgd2F5LCBhbmQgaXQncyBvbmUgb2YgdGhlIGxhc3QKdGhpbmcgSSdtIHRoaW5raW5n IGFib3V0IGNoYW5naW5nIGluIHRoZSBEUkkgaW50ZXJmYWNlLiAgSW5zdGVhZCBvZgp0aGUgbG9h ZGVyIHByb3ZpZGluZyBhIHN0cnVjdCBvZiBmdW5jdGlvbiBwb2ludGVycyB0byBmaWxsIG91dCB3 ZQpjb3VsZCBkbyBzb21ldGhpbmcgbGlrZSB0aGlzOgoKICBzdHJ1Y3QgX19EUklzY3JlZW5JbnRl cmZhY2VSZWMgewogICAgdm9pZCAoKmRlc3Ryb3lTY3JlZW4pKF9fRFJJc2NyZWVuICpzY3JlZW4p OwogICAgLi4uCiAgfTsKCiAgc3RydWN0IF9fRFJJc2NyZWVuUmVjIHsKICAgIGNvbnN0IF9fRFJJ c2NyZWVuSW50ZXJmYWNlICppbnRlcmZhY2U7CiAgICB2b2lkICpwcml2YXRlOwogIH07CgphbmQg Y3JlYXRlTmV3U2NyZWVuIHdpbGwgcmV0dXJuIGEgX19EUklzY3JlZW5SZWMgYWxsb2NhdGVkIGJ5 IHRoZQpkcml2ZXIsIHdoZXJlIHByaXZhdGUgaXMgZm9yIHRoZSBsb2FkZXIgdG8gdXNlIHRvIHBv aW50IGJhY2sgdG8gaXRzCnNjcmVlbiBwcml2YXRlLiAgVGhpcyB3aWxsIGxldCB1cyBleHRlbmQg dGhlIGNvcmUgR0xYIG9iamVjdHMgdG9vLAp3aXRob3V0IHRoZSBkcml2ZXIgd29ycnlpbmcgYWJv dXQgaG93IG1hbnkgb2YgdGhlIGZ1bmN0aW9uIHBvaW50ZXIKZmllbGRzIHRoZSBsb2FkZXIga25v d3MgYWJvdXQuICBBbmQgd2UgZG9uJ3QgY2FycnkgdGhlIHNhbWUgc2V0IG9mCmZ1bmN0aW9uIHBv aW50ZXJzIGFyb3VuZCBpbiBldmVyeSBfX0RSSWRyYXdhYmxlLgoKQW55d2F5LCBJIGR1bm5vLCBz ZWVtcyBsaWtlIGEgbG90IG9mIGNodXJuIGZvciBhIHNtYWxsIGRldGFpbCAtIHNvcnJ5CmZvciBo aWphY2tpbmcgdGhlIHRocmVhZCwganVzdCBzYXcgYW4gZXhjdXNlIHRvIGdldCB0aGlzIG91dCA6 KQoKS3Jpc3RpYW4K |