From: Jesse B. <jb...@vi...> - 2007-09-18 22:03:06
|
Both the generic DRM vblank-rework and Intel specific pipe/plane swapping have uncovered some vblank related problems which we discussed at XDS last week. Unfortunately, no matter what we do (including the "do nothing" option), some applications will break some of the time in the new world order. Basically we have a few vblank related bits of code: 1) DRM_IOCTL_WAIT_VBLANK - core DRM vblank wait ioctl 2) driver interrupt code - increments appropriate vblank counter 3) DRM_I915_VBLANK_SWAP - Intel specific scheduled swap ioctl 4) SAREA private data - used for tracking which gfx plane to swap 5) glX*VideoSyncSGI - GL interfaces for sync'ing to vblank events As it stands, DRM_IOCTL_WAIT_VBLANK is downright broken in the new world of dyanmically controlled outputs and CRTCs (at least for i915 and radeon): a client trying to sync against the second CRTC that doesn't pass _DRM_VBLANK_SECONDARY will only work if one CRTC is enabled, due to the way current interrupt handlers increment the respective vblank counters (i.e. they increment the correct counter if both CRTCs are generating events, but only the primary counter if only one CRTC vblank interrupt is enabled). The Intel specific DRM_I915_VBLANK_SWAP is a really nice interface, and is the only reliable way to get tear free vblank swap on a loaded system. However, what it really cares about is display planes (in the Intel sense), so it uses the _DRM_VBLANK_SECONDARY flag to indicate whether it wants to flip plane A or B. Whether or not to pass the _DRM_VBLANK_SECONDARY flag is determined by DRI code based on the SAREA private data that describes how much of a given client's window is visible on either pipe. This should work fine as of last week's mods and only the DDX and DRM code have to be aware of potential pipe->plane swapping due to hardware limitations. The vblank-rework branch of the DRM tree tries to address (1) and (2) by splitting the logic for handling CRTCs and their associated vblank interrupts into discrete paths, but this defeats the original purpose of the driver interrupt code that tries to fall back to a single counter, which is due to limitations in (5), namely that the glX*VideoSyncSGI APIs can only handle a single pipe. So, what to do? One way of making the glX*VideoSyncSGI interfaces behave more or less as expected would be to make them more like DRM_I915_VBLANK_SWAP internally, i.e. using SAREA values to determine which pipe needs to be sync'd against by passing in the display plane the client is most tied to (this would imply making the Intel specific SAREA plane info more generic), letting the DRM take care of the rest. Another option (which could be done in addition to the above) would be to add some new CRTC-aware interfaces along with ways at getting at current CRTC/display plane for a client (does GL already have this?). And no matter the outcome, we should encourage people to use interfaces like DRM_I915_VBLANK_SWAP rather than glXWaitVideoSyncSGI, otherwise they'll be highly susceptible to unpredictable scheduling hiccups. Any other thoughts? Thanks, Jesse |
From: Keith W. <ke...@tu...> - 2007-09-18 22:08:56
|
Jesse Barnes wrote: > Both the generic DRM vblank-rework and Intel specific pipe/plane > swapping have uncovered some vblank related problems which we discussed > at XDS last week. Unfortunately, no matter what we do (including > the "do nothing" option), some applications will break some of the time > in the new world order. > > Basically we have a few vblank related bits of code: > 1) DRM_IOCTL_WAIT_VBLANK - core DRM vblank wait ioctl > 2) driver interrupt code - increments appropriate vblank counter > 3) DRM_I915_VBLANK_SWAP - Intel specific scheduled swap ioctl > 4) SAREA private data - used for tracking which gfx plane to swap > 5) glX*VideoSyncSGI - GL interfaces for sync'ing to vblank events > > As it stands, DRM_IOCTL_WAIT_VBLANK is downright broken in the new world > of dyanmically controlled outputs and CRTCs (at least for i915 and > radeon): a client trying to sync against the second CRTC that doesn't > pass _DRM_VBLANK_SECONDARY will only work if one CRTC is enabled, due > to the way current interrupt handlers increment the respective vblank > counters (i.e. they increment the correct counter if both CRTCs are > generating events, but only the primary counter if only one CRTC vblank > interrupt is enabled). > > The Intel specific DRM_I915_VBLANK_SWAP is a really nice interface, and > is the only reliable way to get tear free vblank swap on a loaded > system. However, what it really cares about is display planes (in the > Intel sense), so it uses the _DRM_VBLANK_SECONDARY flag to indicate > whether it wants to flip plane A or B. Whether or not to pass the > _DRM_VBLANK_SECONDARY flag is determined by DRI code based on the SAREA > private data that describes how much of a given client's window is > visible on either pipe. This should work fine as of last week's mods > and only the DDX and DRM code have to be aware of potential pipe->plane > swapping due to hardware limitations. > > The vblank-rework branch of the DRM tree tries to address (1) and (2) by > splitting the logic for handling CRTCs and their associated vblank > interrupts into discrete paths, but this defeats the original purpose > of the driver interrupt code that tries to fall back to a single > counter, which is due to limitations in (5), namely that the > glX*VideoSyncSGI APIs can only handle a single pipe. > > So, what to do? One way of making the glX*VideoSyncSGI interfaces > behave more or less as expected would be to make them more like > DRM_I915_VBLANK_SWAP internally, i.e. using SAREA values to determine > which pipe needs to be sync'd against by passing in the display plane > the client is most tied to (this would imply making the Intel specific > SAREA plane info more generic), letting the DRM take care of the rest. If the SGI glx extensions aren't matching the hardware capabilities, I think it's appropriate to start talking about new extensions exposing behaviour we can support... It might be worth taking a look over the fence at the wgl world and see if there's anything useful there that might be adapted. Keit |
From: Torgeir V. <to...@po...> - 2007-09-18 22:11:26
|
On 18 Sep 2007, at 22:54, Jesse Barnes wrote: > Any other thoughts? Please do add the option of retrieving a serial number of the vsync irq. It is very handy when debugging video playback that suffers from judder. -- Torgeir Veimo to...@po... |
From: Jesse B. <jb...@vi...> - 2007-09-18 22:15:26
|
On Tuesday, September 18, 2007 3:10 pm Torgeir Veimo wrote: > On 18 Sep 2007, at 22:54, Jesse Barnes wrote: > > Any other thoughts? > > Please do add the option of retrieving a serial number of the vsync > irq. It is very handy when debugging video playback that suffers from > judder. This should be possible already using glXGetVideoSyncSGI. Does that not work for you? Jesse |
From: Michel <mi...@tu...> - 2007-09-19 10:52:31
|
On Tue, 2007-09-18 at 14:54 -0700, Jesse Barnes wrote: > > As it stands, DRM_IOCTL_WAIT_VBLANK is downright broken in the new world > of dyanmically controlled outputs and CRTCs (at least for i915 and > radeon): a client trying to sync against the second CRTC that doesn't > pass _DRM_VBLANK_SECONDARY will only work if one CRTC is enabled, due > to the way current interrupt handlers increment the respective vblank > counters (i.e. they increment the correct counter if both CRTCs are > generating events, but only the primary counter if only one CRTC vblank > interrupt is enabled). Yes, I made it that way to allow old Mesa drivers that don't know anything about secondary vblank to work. The trouble is that at least xf86-video-intel currently never enables vblank interrupts for pipe B only, which defeats that purpose. It's really too bad I screwed up trying to make all this backwards compatible, so I'm afraid it looks like we have to bite the bullet and make incompatible changes again to hopefully make things right finally. > The vblank-rework branch of the DRM tree tries to address (1) and (2) by > splitting the logic for handling CRTCs and their associated vblank > interrupts into discrete paths, but this defeats the original purpose > of the driver interrupt code that tries to fall back to a single > counter, which is due to limitations in (5), namely that the > glX*VideoSyncSGI APIs can only handle a single pipe. Right, it would be nice if we could preserve the above with vblank-rework. Or, I guess another option would be to stop caring about old Mesa drivers that don't know about secondary vblank and to just make sure things work as well as possible when vblank interrupts are enabled for both pipes. But note that 'old drivers' currently includes i965 and all Radeon drivers. > One way of making the glX*VideoSyncSGI interfaces behave more or less > as expected would be to make them more like DRM_I915_VBLANK_SWAP > internally, i.e. using SAREA values to determine which pipe needs to > be sync'd against by passing in the display plane the client is most > tied to (this would imply making the Intel specific SAREA plane info > more generic), Not necessarily - the driver could provide its own versions of the GetMSC and WaitForMSC hooks, or we could make the generic ones use a new driver hook to determine whether to use secondary vblank. > letting the DRM take care of the rest. Yes, this has been my idea, but I haven't got around to actually playing with it. > And no matter the outcome, we should encourage people to use interfaces > like DRM_I915_VBLANK_SWAP rather than glXWaitVideoSyncSGI, otherwise > they'll be highly susceptible to unpredictable scheduling hiccups. Right, i.e. right now use GLX_SGI_swap_control instead of GLX_SGI_video_sync whenever possible. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-09-19 16:59:26
|
On Wednesday, September 19, 2007 3:52 am Michel D=C3=A4nzer wrote: > On Tue, 2007-09-18 at 14:54 -0700, Jesse Barnes wrote: > > As it stands, DRM_IOCTL_WAIT_VBLANK is downright broken in the new > > world of dyanmically controlled outputs and CRTCs (at least for > > i915 and radeon): a client trying to sync against the second CRTC > > that doesn't pass _DRM_VBLANK_SECONDARY will only work if one CRTC > > is enabled, due to the way current interrupt handlers increment the > > respective vblank counters (i.e. they increment the correct counter > > if both CRTCs are generating events, but only the primary counter > > if only one CRTC vblank interrupt is enabled). > > Yes, I made it that way to allow old Mesa drivers that don't know > anything about secondary vblank to work. The trouble is that at least > xf86-video-intel currently never enables vblank interrupts for pipe B > only, which defeats that purpose. It's really too bad I screwed up > trying to make all this backwards compatible, so I'm afraid it looks > like we have to bite the bullet and make incompatible changes again > to hopefully make things right finally. Well I'm not sure you screwed it up, before we had randr1.2 enabled=20 drivers that scheme made a lot of sense. :) > > The vblank-rework branch of the DRM tree tries to address (1) and > > (2) by splitting the logic for handling CRTCs and their associated > > vblank interrupts into discrete paths, but this defeats the > > original purpose of the driver interrupt code that tries to fall > > back to a single counter, which is due to limitations in (5), > > namely that the glX*VideoSyncSGI APIs can only handle a single > > pipe. > > Right, it would be nice if we could preserve the above with > vblank-rework. > > Or, I guess another option would be to stop caring about old Mesa > drivers that don't know about secondary vblank and to just make sure > things work as well as possible when vblank interrupts are enabled > for both pipes. But note that 'old drivers' currently includes i965 > and all Radeon drivers. Given that some of this will break no matter what, I like this option=20 better. > > One way of making the glX*VideoSyncSGI interfaces behave more or > > less as expected would be to make them more like > > DRM_I915_VBLANK_SWAP internally, i.e. using SAREA values to > > determine which pipe needs to be sync'd against by passing in the > > display plane the client is most tied to (this would imply making > > the Intel specific SAREA plane info more generic), > > Not necessarily - the driver could provide its own versions of the > GetMSC and WaitForMSC hooks, or we could make the generic ones use a > new driver hook to determine whether to use secondary vblank. Yeah, common code would be best. So: - use the vblank-rework tree to make per-CRTC vblank counters (as you say, this breaks some setups but will fix others) - add code to Mesa so GetMSC/WaitForMSC set DRM_VBLANK_SECONDARY correctly That should make the current stack work fairly well. And when there's a=20 real need, we can look at adding multipipe aware extensions. I can finish up the Intel bits, but the vblank-rework tree still needs=20 mach64, mga, r128 and via updated. And the Mesa parts of those drivers=20 need updating to handle multiple pipes. Anyone have time to tackle=20 those? Thanks, Jesse |
From: Ian R. <id...@us...> - 2007-09-19 17:20:22
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jesse Barnes wrote: > Both the generic DRM vblank-rework and Intel specific pipe/plane > swapping have uncovered some vblank related problems which we discussed > at XDS last week. Unfortunately, no matter what we do (including > the "do nothing" option), some applications will break some of the time > in the new world order. > > Basically we have a few vblank related bits of code: > 1) DRM_IOCTL_WAIT_VBLANK - core DRM vblank wait ioctl > 2) driver interrupt code - increments appropriate vblank counter > 3) DRM_I915_VBLANK_SWAP - Intel specific scheduled swap ioctl > 4) SAREA private data - used for tracking which gfx plane to swap > 5) glX*VideoSyncSGI - GL interfaces for sync'ing to vblank events > > As it stands, DRM_IOCTL_WAIT_VBLANK is downright broken in the new world > of dyanmically controlled outputs and CRTCs (at least for i915 and > radeon): a client trying to sync against the second CRTC that doesn't > pass _DRM_VBLANK_SECONDARY will only work if one CRTC is enabled, due > to the way current interrupt handlers increment the respective vblank > counters (i.e. they increment the correct counter if both CRTCs are > generating events, but only the primary counter if only one CRTC vblank > interrupt is enabled). We basically implemented DRM_IOCTL_WAIT_VBLANK as an easy way to migrate existing driver swap code to support glXSwapIntervalSGI. > The Intel specific DRM_I915_VBLANK_SWAP is a really nice interface, and > is the only reliable way to get tear free vblank swap on a loaded > system. However, what it really cares about is display planes (in the > Intel sense), so it uses the _DRM_VBLANK_SECONDARY flag to indicate > whether it wants to flip plane A or B. Whether or not to pass the > _DRM_VBLANK_SECONDARY flag is determined by DRI code based on the SAREA > private data that describes how much of a given client's window is > visible on either pipe. This should work fine as of last week's mods > and only the DDX and DRM code have to be aware of potential pipe->plane > swapping due to hardware limitations. > > The vblank-rework branch of the DRM tree tries to address (1) and (2) by > splitting the logic for handling CRTCs and their associated vblank > interrupts into discrete paths, but this defeats the original purpose > of the driver interrupt code that tries to fall back to a single > counter, which is due to limitations in (5), namely that the > glX*VideoSyncSGI APIs can only handle a single pipe. glXVideoSyncSGI and glXSwapIntervalSGI really are crappy interfaces. OpenML supplanted them with glXWaitForMscOML and glXSwapBuffersMscOML. The newer interfaces are a superset of the old, and we should look to implementing them as such. > So, what to do? One way of making the glX*VideoSyncSGI interfaces > behave more or less as expected would be to make them more like > DRM_I915_VBLANK_SWAP internally, i.e. using SAREA values to determine > which pipe needs to be sync'd against by passing in the display plane > the client is most tied to (this would imply making the Intel specific > SAREA plane info more generic), letting the DRM take care of the rest. > > Another option (which could be done in addition to the above) would be > to add some new CRTC-aware interfaces along with ways at getting at > current CRTC/display plane for a client (does GL already have this?). No. Core GL doesn't even know about windows or screens. GLX knows about displays and drawables, and that's it. > And no matter the outcome, we should encourage people to use interfaces > like DRM_I915_VBLANK_SWAP rather than glXWaitVideoSyncSGI, otherwise > they'll be highly susceptible to unpredictable scheduling hiccups. Right. Something like DRM_I915_VBLANK_SWAP should be used to implement either glXSwapIntervalSGI or glXSwapBuffersMscOML. To be honest, I'm not sure what real-world use an application could have for the glXWait interfaces. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFG8VpBX1gOwKyEAw8RAmmfAJ0eq1B1CEkp/ofo86kvGBgIK5MptwCffpHA vaBiGHXYPuh9wi0MS+Zttk0= =U+Cb -----END PGP SIGNATURE----- |
From: Michel <mi...@tu...> - 2007-09-21 09:51:12
|
On Wed, 2007-09-19 at 09:59 -0700, Jesse Barnes wrote: > On Wednesday, September 19, 2007 3:52 am Michel Dänzer wrote: > > On Tue, 2007-09-18 at 14:54 -0700, Jesse Barnes wrote: > > > As it stands, DRM_IOCTL_WAIT_VBLANK is downright broken in the new > > > world of dyanmically controlled outputs and CRTCs (at least for > > > i915 and radeon): a client trying to sync against the second CRTC > > > that doesn't pass _DRM_VBLANK_SECONDARY will only work if one CRTC > > > is enabled, due to the way current interrupt handlers increment the > > > respective vblank counters (i.e. they increment the correct counter > > > if both CRTCs are generating events, but only the primary counter > > > if only one CRTC vblank interrupt is enabled). > > > > Yes, I made it that way to allow old Mesa drivers that don't know > > anything about secondary vblank to work. The trouble is that at least > > xf86-video-intel currently never enables vblank interrupts for pipe B > > only, which defeats that purpose. It's really too bad I screwed up > > trying to make all this backwards compatible, so I'm afraid it looks > > like we have to bite the bullet and make incompatible changes again > > to hopefully make things right finally. > > Well I'm not sure you screwed it up, before we had randr1.2 enabled > drivers that scheme made a lot of sense. :) Except I was assuming Intel LVDS panels were driven by pipe A... > > > One way of making the glX*VideoSyncSGI interfaces behave more or > > > less as expected would be to make them more like > > > DRM_I915_VBLANK_SWAP internally, i.e. using SAREA values to > > > determine which pipe needs to be sync'd against by passing in the > > > display plane the client is most tied to (this would imply making > > > the Intel specific SAREA plane info more generic), > > > > Not necessarily - the driver could provide its own versions of the > > GetMSC and WaitForMSC hooks, or we could make the generic ones use a > > new driver hook to determine whether to use secondary vblank. > > Yeah, common code would be best. > > So: > - use the vblank-rework tree to make per-CRTC vblank counters (as > you > say, this breaks some setups but will fix others) Out of curiosity, what setups are you thinking of that this will fix on its own? Can't think of any offhand. > - add code to Mesa so GetMSC/WaitForMSC set DRM_VBLANK_SECONDARY > correctly One idea (with some handwaving :) would be the common code keeping around a pointer to the driver's vblank_flags variable or keeping track of the flags per drawable in some other sensible way. > That should make the current stack work fairly well. And when there's a > real need, we can look at adding multipipe aware extensions. My gut feeling is we'd be better off without apps or even toolkits dealing with this directly. So long as everything's keyed off the drawable, the GLX implementation should mostly be able to do the right thing on its own. One exception being cloned (or at least overlapping) setups where the CRTC modes aren't in sync. My idea for that has been a driconf option to choose which CRTC to sync to in case the drawable is visible on both CRTCs). -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-09-21 14:59:41
|
On Friday, September 21, 2007 2:51:02 am Michel D=C3=A4nzer wrote: > > So: > > - use the vblank-rework tree to make per-CRTC vblank counters (as > > you > > say, this breaks some setups but will fix others) > > Out of curiosity, what setups are you thinking of that this will fix on > its own? Can't think of any offhand. It should fix the case where a client is waiting on pipe B when vblank=20 interrupts on pipe A go from off to on. Currently, that'll cause the clien= t=20 to switch to the wrong vblank counter after pipe A's interrupts become=20 active. In the vblank rework tree, it'll either not work in the first plac= e=20 (because it's using the wrong counter) or it'll work and keep working due t= o=20 passing in DRM_VBLANK_SECONDARY. I think this is the correct behavior. > > - add code to Mesa so GetMSC/WaitForMSC set DRM_VBLANK_SECONDARY > > correctly > > One idea (with some handwaving :) would be the common code keeping > around a pointer to the driver's vblank_flags variable or keeping track > of the flags per drawable in some other sensible way. Yeah, if it can be kept generic I think that would be good. > > That should make the current stack work fairly well. And when there's a > > real need, we can look at adding multipipe aware extensions. > > My gut feeling is we'd be better off without apps or even toolkits > dealing with this directly. So long as everything's keyed off the > drawable, the GLX implementation should mostly be able to do the right > thing on its own. One exception being cloned (or at least overlapping) > setups where the CRTC modes aren't in sync. My idea for that has been a > driconf option to choose which CRTC to sync to in case the drawable is > visible on both CRTCs). Yeah, makes sense. If we get this right there shouldn't be much need for=20 exposing clients to the pipe layouts directly. Jesse |
From: Michel <mi...@tu...> - 2007-09-21 15:04:27
|
On Fri, 2007-09-21 at 07:59 -0700, Jesse Barnes wrote: > On Friday, September 21, 2007 2:51:02 am Michel Dänzer wrote: > > > So: > > > - use the vblank-rework tree to make per-CRTC vblank counters (as > > > you > > > say, this breaks some setups but will fix others) > > > > Out of curiosity, what setups are you thinking of that this will fix on > > its own? Can't think of any offhand. > > It should fix the case where a client is waiting on pipe B when vblank > interrupts on pipe A go from off to on. Currently, that'll cause the client > to switch to the wrong vblank counter after pipe A's interrupts become > active. In the vblank rework tree, it'll either not work in the first place > (because it's using the wrong counter) or it'll work and keep working due to > passing in DRM_VBLANK_SECONDARY. I think this is the correct > behavior. Unless I'm missing something, this would only be true if xf86-video-intel ever enabled vblank interrupts for pipe B only, which it doesn't (with a DRM that supports vblank interrupts on both pipes independently). It always leaves them enabled for pipe A, even when pipe A is actually disabled. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-09-21 19:47:15
Attachments:
mesa-core-vblank-support-2.patch
|
On Friday, September 21, 2007 2:51 am Michel D=C3=A4nzer wrote: > > - add code to Mesa so GetMSC/WaitForMSC set DRM_VBLANK_SECONDARY > > correctly > > One idea (with some handwaving :) would be the common code keeping > around a pointer to the driver's vblank_flags variable or keeping > track of the flags per drawable in some other sensible way. I like the idea, here's something concrete. I put the vblank stuff into=20 the screen private, but I'm not sure if that's the best place=20 (logically the vblank counters are screen specific), are there X=20 compatibility issues with changing that structure? Porting over other drivers should be trivial... Thanks, Jesse |
From: Michel <mi...@tu...> - 2007-09-24 08:25:38
|
On Fri, 2007-09-21 at 12:46 -0700, Jesse Barnes wrote: > On Friday, September 21, 2007 2:51 am Michel Dänzer wrote: > > > - add code to Mesa so GetMSC/WaitForMSC set DRM_VBLANK_SECONDARY > > > correctly > > > > One idea (with some handwaving :) would be the common code keeping > > around a pointer to the driver's vblank_flags variable or keeping > > track of the flags per drawable in some other sensible way. > > I like the idea, here's something concrete. I put the vblank stuff into > the screen private, but I'm not sure if that's the best place > (logically the vblank counters are screen specific), The drawable private would seem more appropriate, as these are drawable attributes. > are there X compatibility issues with changing that structure? AFAICT no - the __DRI*Private* types seem opaque outside of *_dri.so . -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-09-24 17:07:17
|
On Monday, September 24, 2007 1:25 am Michel D=C3=A4nzer wrote: > On Fri, 2007-09-21 at 12:46 -0700, Jesse Barnes wrote: > > On Friday, September 21, 2007 2:51 am Michel D=C3=A4nzer wrote: > > > > - add code to Mesa so GetMSC/WaitForMSC set > > > > DRM_VBLANK_SECONDARY correctly > > > > > > One idea (with some handwaving :) would be the common code > > > keeping around a pointer to the driver's vblank_flags variable or > > > keeping track of the flags per drawable in some other sensible > > > way. > > > > I like the idea, here's something concrete. I put the vblank stuff > > into the screen private, but I'm not sure if that's the best place > > (logically the vblank counters are screen specific), > > The drawable private would seem more appropriate, as these are > drawable attributes. Ok, I'll move it over to the drawable private, assuming there's a way to=20 get at that structure from all the paths we care about. > > are there X compatibility issues with changing that structure? > > AFAICT no - the __DRI*Private* types seem opaque outside of *_dri.so Great. I'll go ahead and push after I test again. There's no hurry on=20 converting the other drivers; their respective maintainers can move=20 them over when they have time. Thanks, Jesse |
From: Jesse B. <jb...@vi...> - 2007-09-25 20:34:15
|
On Monday, September 24, 2007 1:25 am Michel D=C3=A4nzer wrote: > On Fri, 2007-09-21 at 12:46 -0700, Jesse Barnes wrote: > > On Friday, September 21, 2007 2:51 am Michel D=C3=A4nzer wrote: > > > > - add code to Mesa so GetMSC/WaitForMSC set > > > > DRM_VBLANK_SECONDARY correctly > > > > > > One idea (with some handwaving :) would be the common code > > > keeping around a pointer to the driver's vblank_flags variable or > > > keeping track of the flags per drawable in some other sensible > > > way. > > > > I like the idea, here's something concrete. I put the vblank stuff > > into the screen private, but I'm not sure if that's the best place > > (logically the vblank counters are screen specific), > > The drawable private would seem more appropriate, as these are > drawable attributes. Here's a new version that moves the new fields over to the drawable=20 private. I added a new drawable hook to implement the callback, and in=20 the process discovered that all the drivers I could find either set=20 their MSC routines to NULL or used the generic calls. So I didn't bother creating a new driver API hook for the new call; I=20 just set it directly to the version in vblank.c in=20 driCreateNewDrawable(). Would it make sense to rip out all the wrappers in dri_util.c, set=20 everything directly in driCreateNewDrawable() and=20 driUtilCreateNewScreen()? It seems that drivers that set their MSC routines to NULL will return=20 GL_BAD_CONTEXT from calls like glXWaitVideoSyncSGI(), and if e.g.=20 drmWaitVBlank() failed clients would get the same result, so=20 compatibility would be preserved... Or should I add a new driver hook for the new per-drawable getMSC=20 function and update every driver? Thanks, Jesse |
From: Michel <mi...@tu...> - 2007-09-26 07:08:32
|
On Tue, 2007-09-25 at 13:32 -0700, Jesse Barnes wrote: > On Monday, September 24, 2007 1:25 am Michel Dänzer wrote: > > On Fri, 2007-09-21 at 12:46 -0700, Jesse Barnes wrote: > > > On Friday, September 21, 2007 2:51 am Michel Dänzer wrote: > > > > > - add code to Mesa so GetMSC/WaitForMSC set > > > > > DRM_VBLANK_SECONDARY correctly > > > > > > > > One idea (with some handwaving :) would be the common code > > > > keeping around a pointer to the driver's vblank_flags variable or > > > > keeping track of the flags per drawable in some other sensible > > > > way. > > > > > > I like the idea, here's something concrete. I put the vblank stuff > > > into the screen private, but I'm not sure if that's the best place > > > (logically the vblank counters are screen specific), > > > > The drawable private would seem more appropriate, as these are > > drawable attributes. > > Here's a new version Where's that? :) > that moves the new fields over to the drawable private. I added a new > drawable hook to implement the callback, and in the process discovered > that all the drivers I could find either set their MSC routines to > NULL or used the generic calls. > > So I didn't bother creating a new driver API hook for the new call; I > just set it directly to the version in vblank.c in > driCreateNewDrawable(). > > Would it make sense to rip out all the wrappers in dri_util.c, set > everything directly in driCreateNewDrawable() and > driUtilCreateNewScreen()? What exactly do you mean by 'all the wrappers'? > It seems that drivers that set their MSC routines to NULL will return > GL_BAD_CONTEXT from calls like glXWaitVideoSyncSGI(), and if e.g. > drmWaitVBlank() failed clients would get the same result, so > compatibility would be preserved... Isn't the presence or absence of the driver hooks used to decide whether or not to advertise the GLX extension(s) though? -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Ian R. <id...@us...> - 2007-09-27 00:42:53
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Michel Dänzer wrote: > On Tue, 2007-09-25 at 13:32 -0700, Jesse Barnes wrote: >> It seems that drivers that set their MSC routines to NULL will return >> GL_BAD_CONTEXT from calls like glXWaitVideoSyncSGI(), and if e.g. >> drmWaitVBlank() failed clients would get the same result, so >> compatibility would be preserved... > > Isn't the presence or absence of the driver hooks used to decide whether > or not to advertise the GLX extension(s) though? Not currently. The driver has to set the callback and tell libGL to enable the extension. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFG+vxAX1gOwKyEAw8RAmOpAJwIbNfioXXEX2QOT0SkXDjIe8878gCfUZY5 1wl1bl7w0jTvC6vsaA0RfEY= =L060 -----END PGP SIGNATURE----- |
From: Jesse B. <jb...@vi...> - 2007-09-26 14:53:47
|
On Wednesday, September 26, 2007 12:08:13 am Michel D=C3=A4nzer wrote: > On Tue, 2007-09-25 at 13:32 -0700, Jesse Barnes wrote: > > On Monday, September 24, 2007 1:25 am Michel D=C3=A4nzer wrote: > > > On Fri, 2007-09-21 at 12:46 -0700, Jesse Barnes wrote: > > > > On Friday, September 21, 2007 2:51 am Michel D=C3=A4nzer wrote: > > > > > > - add code to Mesa so GetMSC/WaitForMSC set > > > > > > DRM_VBLANK_SECONDARY correctly > > > > > > > > > > One idea (with some handwaving :) would be the common code > > > > > keeping around a pointer to the driver's vblank_flags variable or > > > > > keeping track of the flags per drawable in some other sensible > > > > > way. > > > > > > > > I like the idea, here's something concrete. I put the vblank stuff > > > > into the screen private, but I'm not sure if that's the best place > > > > (logically the vblank counters are screen specific), > > > > > > The drawable private would seem more appropriate, as these are > > > drawable attributes. > > > > Here's a new version > > Where's that? :) Oops. :) I'll resend when I get back to the machine with the code... > > that moves the new fields over to the drawable private. I added a new > > drawable hook to implement the callback, and in the process discovered > > that all the drivers I could find either set their MSC routines to > > NULL or used the generic calls. > > > > So I didn't bother creating a new driver API hook for the new call; I > > just set it directly to the version in vblank.c in > > driCreateNewDrawable(). > > > > Would it make sense to rip out all the wrappers in dri_util.c, set > > everything directly in driCreateNewDrawable() and > > driUtilCreateNewScreen()? > > What exactly do you mean by 'all the wrappers'? There are wrappers in dri_util.c for this code. The drivers then point the= ir=20 Driver API callbacks at them (rather than using the routines in vblank.c=20 directly for example), so it's just an extra level of indirection that=20 doesn't seem to buy us anything, though maybe there are out of tree drivers= =20 that take advantage of this setup... > > It seems that drivers that set their MSC routines to NULL will return > > GL_BAD_CONTEXT from calls like glXWaitVideoSyncSGI(), and if e.g. > > drmWaitVBlank() failed clients would get the same result, so > > compatibility would be preserved... > > Isn't the presence or absence of the driver hooks used to decide whether > or not to advertise the GLX extension(s) though? The driver hooks would still be there, they'd just directly use the=20 appropriate call instead of calling the dri_util.c wrappers. Jesse |
From: Michel <mi...@tu...> - 2007-09-26 15:11:45
|
On Wed, 2007-09-26 at 07:53 -0700, Jesse Barnes wrote: > On Wednesday, September 26, 2007 12:08:13 am Michel Dänzer wrote: > > On Tue, 2007-09-25 at 13:32 -0700, Jesse Barnes wrote: > > > > > > that moves the new fields over to the drawable private. I added a new > > > drawable hook to implement the callback, and in the process discovered > > > that all the drivers I could find either set their MSC routines to > > > NULL or used the generic calls. > > > > > > So I didn't bother creating a new driver API hook for the new call; I > > > just set it directly to the version in vblank.c in > > > driCreateNewDrawable(). > > > > > > Would it make sense to rip out all the wrappers in dri_util.c, set > > > everything directly in driCreateNewDrawable() and > > > driUtilCreateNewScreen()? > > > > What exactly do you mean by 'all the wrappers'? > > There are wrappers in dri_util.c for this code. The drivers then point their > Driver API callbacks at them (rather than using the routines in vblank.c > directly for example), so it's just an extra level of indirection that > doesn't seem to buy us anything, [...] AFAICT all drivers point the DriverAPI callbacks to the vblank.c functions, so I'm still not sure what you're getting at, but maybe your patch will clarify. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-09-26 16:02:47
|
On Wednesday, September 26, 2007 8:11:19 am Michel D=C3=A4nzer wrote: > On Wed, 2007-09-26 at 07:53 -0700, Jesse Barnes wrote: > > On Wednesday, September 26, 2007 12:08:13 am Michel D=C3=A4nzer wrote: > > > On Tue, 2007-09-25 at 13:32 -0700, Jesse Barnes wrote: > > > > that moves the new fields over to the drawable private. I added a > > > > new drawable hook to implement the callback, and in the process > > > > discovered that all the drivers I could find either set their MSC > > > > routines to NULL or used the generic calls. > > > > > > > > So I didn't bother creating a new driver API hook for the new call;= I > > > > just set it directly to the version in vblank.c in > > > > driCreateNewDrawable(). > > > > > > > > Would it make sense to rip out all the wrappers in dri_util.c, set > > > > everything directly in driCreateNewDrawable() and > > > > driUtilCreateNewScreen()? > > > > > > What exactly do you mean by 'all the wrappers'? > > > > There are wrappers in dri_util.c for this code. The drivers then point > > their Driver API callbacks at them (rather than using the routines in > > vblank.c directly for example), so it's just an extra level of > > indirection that doesn't seem to buy us anything, [...] > > AFAICT all drivers point the DriverAPI callbacks to the vblank.c > functions, so I'm still not sure what you're getting at, but maybe your > patch will clarify. Err yeah I was describing it backwards. The __DRIscreen hooks for the MSC= =20 stuff all point to dri_util.c wrapper functions that end up calling the=20 driver hooks. However, drivers always set their hooks to either NULL or to= =20 the routines in vblank.c. So we could just set the __DRIscreen hooks to=20 point to vblank.c directly since none of the drivers appear to have custom= =20 hooks for these calls... Jesse |
From: Michel <mi...@tu...> - 2007-09-26 16:39:47
|
On Wed, 2007-09-26 at 08:31 -0700, Jesse Barnes wrote: > On Wednesday, September 26, 2007 8:11:19 am Michel Dänzer wrote: > > On Wed, 2007-09-26 at 07:53 -0700, Jesse Barnes wrote: > > > On Wednesday, September 26, 2007 12:08:13 am Michel Dänzer wrote: > > > > On Tue, 2007-09-25 at 13:32 -0700, Jesse Barnes wrote: > > > > > that moves the new fields over to the drawable private. I added a > > > > > new drawable hook to implement the callback, and in the process > > > > > discovered that all the drivers I could find either set their MSC > > > > > routines to NULL or used the generic calls. > > > > > > > > > > So I didn't bother creating a new driver API hook for the new call; I > > > > > just set it directly to the version in vblank.c in > > > > > driCreateNewDrawable(). > > > > > > > > > > Would it make sense to rip out all the wrappers in dri_util.c, set > > > > > everything directly in driCreateNewDrawable() and > > > > > driUtilCreateNewScreen()? > > > > > > > > What exactly do you mean by 'all the wrappers'? > > > > > > There are wrappers in dri_util.c for this code. The drivers then point > > > their Driver API callbacks at them (rather than using the routines in > > > vblank.c directly for example), so it's just an extra level of > > > indirection that doesn't seem to buy us anything, [...] > > > > AFAICT all drivers point the DriverAPI callbacks to the vblank.c > > functions, so I'm still not sure what you're getting at, but maybe your > > patch will clarify. > > Err yeah I was describing it backwards. The __DRIscreen hooks for the MSC > stuff all point to dri_util.c wrapper functions that end up calling the > driver hooks. However, drivers always set their hooks to either NULL or to > the routines in vblank.c. So we could just set the __DRIscreen hooks to > point to vblank.c directly since none of the drivers appear to have custom > hooks for these calls... I see, thanks for clarifying. Ian Romanick can probably tell us more about this. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2007-09-26 17:28:26
Attachments:
mesa-core-vblank-support-4.patch
|
On Wednesday, September 26, 2007 9:39 am Michel D=C3=A4nzer wrote: > > Err yeah I was describing it backwards. The __DRIscreen hooks for > > the MSC stuff all point to dri_util.c wrapper functions that end up > > calling the driver hooks. However, drivers always set their hooks > > to either NULL or to the routines in vblank.c. So we could just > > set the __DRIscreen hooks to point to vblank.c directly since none > > of the drivers appear to have custom hooks for these calls... > > I see, thanks for clarifying. Ian Romanick can probably tell us more > about this. And for reference, here's the updated patch. Jesse |
From: <kr...@bi...> - 2007-09-26 17:44:12
|
T24gOS8yNi8wNywgTWljaGVsIETDpG56ZXIgPG1pY2hlbEB0dW5nc3RlbmdyYXBoaWNzLmNvbT4g d3JvdGU6Cj4KPiBPbiBXZWQsIDIwMDctMDktMjYgYXQgMDg6MzEgLTA3MDAsIEplc3NlIEJhcm5l cyB3cm90ZToKPiA+IE9uIFdlZG5lc2RheSwgU2VwdGVtYmVyIDI2LCAyMDA3IDg6MTE6MTkgYW0g TWljaGVsIETDpG56ZXIgd3JvdGU6Cj4gPiA+IE9uIFdlZCwgMjAwNy0wOS0yNiBhdCAwNzo1MyAt MDcwMCwgSmVzc2UgQmFybmVzIHdyb3RlOgo+ID4gPiA+IE9uIFdlZG5lc2RheSwgU2VwdGVtYmVy IDI2LCAyMDA3IDEyOjA4OjEzIGFtIE1pY2hlbCBEw6RuemVyIHdyb3RlOgo+ID4gPiA+ID4gT24g VHVlLCAyMDA3LTA5LTI1IGF0IDEzOjMyIC0wNzAwLCBKZXNzZSBCYXJuZXMgd3JvdGU6Cj4gPiA+ ID4gPiA+IHRoYXQgbW92ZXMgdGhlIG5ldyBmaWVsZHMgb3ZlciB0byB0aGUgZHJhd2FibGUgcHJp dmF0ZS4gIEkgYWRkZWQgYQo+ID4gPiA+ID4gPiBuZXcgZHJhd2FibGUgaG9vayB0byBpbXBsZW1l bnQgdGhlIGNhbGxiYWNrLCBhbmQgaW4gdGhlIHByb2Nlc3MKPiA+ID4gPiA+ID4gZGlzY292ZXJl ZCB0aGF0IGFsbCB0aGUgZHJpdmVycyBJIGNvdWxkIGZpbmQgZWl0aGVyIHNldCB0aGVpciBNU0MK PiA+ID4gPiA+ID4gcm91dGluZXMgdG8gTlVMTCBvciB1c2VkIHRoZSBnZW5lcmljIGNhbGxzLgo+ ID4gPiA+ID4gPgo+ID4gPiA+ID4gPiBTbyBJIGRpZG4ndCBib3RoZXIgY3JlYXRpbmcgYSBuZXcg ZHJpdmVyIEFQSSBob29rIGZvciB0aGUgbmV3IGNhbGw7IEkKPiA+ID4gPiA+ID4ganVzdCBzZXQg aXQgZGlyZWN0bHkgdG8gdGhlIHZlcnNpb24gaW4gdmJsYW5rLmMgaW4KPiA+ID4gPiA+ID4gZHJp Q3JlYXRlTmV3RHJhd2FibGUoKS4KPiA+ID4gPiA+ID4KPiA+ID4gPiA+ID4gV291bGQgaXQgbWFr ZSBzZW5zZSB0byByaXAgb3V0IGFsbCB0aGUgd3JhcHBlcnMgaW4gZHJpX3V0aWwuYywgc2V0Cj4g PiA+ID4gPiA+IGV2ZXJ5dGhpbmcgZGlyZWN0bHkgaW4gZHJpQ3JlYXRlTmV3RHJhd2FibGUoKSBh bmQKPiA+ID4gPiA+ID4gZHJpVXRpbENyZWF0ZU5ld1NjcmVlbigpPwo+ID4gPiA+ID4KPiA+ID4g PiA+IFdoYXQgZXhhY3RseSBkbyB5b3UgbWVhbiBieSAnYWxsIHRoZSB3cmFwcGVycyc/Cj4gPiA+ ID4KPiA+ID4gPiBUaGVyZSBhcmUgd3JhcHBlcnMgaW4gZHJpX3V0aWwuYyBmb3IgdGhpcyBjb2Rl LiAgVGhlIGRyaXZlcnMgdGhlbiBwb2ludAo+ID4gPiA+IHRoZWlyIERyaXZlciBBUEkgY2FsbGJh Y2tzIGF0IHRoZW0gKHJhdGhlciB0aGFuIHVzaW5nIHRoZSByb3V0aW5lcyBpbgo+ID4gPiA+IHZi bGFuay5jIGRpcmVjdGx5IGZvciBleGFtcGxlKSwgc28gaXQncyBqdXN0IGFuIGV4dHJhIGxldmVs IG9mCj4gPiA+ID4gaW5kaXJlY3Rpb24gdGhhdCBkb2Vzbid0IHNlZW0gdG8gYnV5IHVzIGFueXRo aW5nLCBbLi4uXQo+ID4gPgo+ID4gPiBBRkFJQ1QgYWxsIGRyaXZlcnMgcG9pbnQgdGhlIERyaXZl ckFQSSBjYWxsYmFja3MgdG8gdGhlIHZibGFuay5jCj4gPiA+IGZ1bmN0aW9ucywgc28gSSdtIHN0 aWxsIG5vdCBzdXJlIHdoYXQgeW91J3JlIGdldHRpbmcgYXQsIGJ1dCBtYXliZSB5b3VyCj4gPiA+ IHBhdGNoIHdpbGwgY2xhcmlmeS4KPiA+Cj4gPiBFcnIgeWVhaCBJIHdhcyBkZXNjcmliaW5nIGl0 IGJhY2t3YXJkcy4gIFRoZSBfX0RSSXNjcmVlbiBob29rcyBmb3IgdGhlIE1TQwo+ID4gc3R1ZmYg YWxsIHBvaW50IHRvIGRyaV91dGlsLmMgd3JhcHBlciBmdW5jdGlvbnMgdGhhdCBlbmQgdXAgY2Fs bGluZyB0aGUKPiA+IGRyaXZlciBob29rcy4gIEhvd2V2ZXIsIGRyaXZlcnMgYWx3YXlzIHNldCB0 aGVpciBob29rcyB0byBlaXRoZXIgTlVMTCBvciB0bwo+ID4gdGhlIHJvdXRpbmVzIGluIHZibGFu ay5jLiAgU28gd2UgY291bGQganVzdCBzZXQgdGhlIF9fRFJJc2NyZWVuIGhvb2tzIHRvCj4gPiBw b2ludCB0byB2YmxhbmsuYyBkaXJlY3RseSBzaW5jZSBub25lIG9mIHRoZSBkcml2ZXJzIGFwcGVh ciB0byBoYXZlIGN1c3RvbQo+ID4gaG9va3MgZm9yIHRoZXNlIGNhbGxzLi4uCj4KPiBJIHNlZSwg dGhhbmtzIGZvciBjbGFyaWZ5aW5nLiBJYW4gUm9tYW5pY2sgY2FuIHByb2JhYmx5IHRlbGwgdXMg bW9yZQo+IGFib3V0IHRoaXMuCgpJIHRoaW5rIHRoZSBwb2ludCB3YXMgdGhhdCBhIGRyaXZlciB0 aGF0IGRpZG4ndCBpbXBsZW1lbnQgdGhlIHZibGFuawpjb3VudGluZyB1c2luZyB0aGUgaW9jdGwg Y291bGQgaW1wbGVtZW50IHRoYXQgYnkgc2V0dGluZyB0aGUgRHJpdmVyQVBJCmhvb2tzIHRvIHBv aW50IHRvIGl0cyBvd24gaW1wbGVtZW50YXRpb24gb2YgdGhvc2UgZnVuY3Rpb25zLiAgTm8KZHJp dmVyIGRvZXMgdGhpcywgdGhvdWdoLgoKY2hlZXJzLApLcmlzdGlhbgo= |