On Wed, May 28, 2014 at 06:12:54PM +0900, Michel Dänzer wrote:
Digging out an ooold post of Daniel's...
On 04.03.2014 18:13, Daniel Vetter wrote:
On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote:
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
It still works like that, but there's two fundamental issues with this trick:
- There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened.
Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset)
The hardware counter reset is a problem: 1. vblank_disable_and_save() updates .last 2. modeset/dpms/suspend (hw counter is reset) 3. drm_vblank_get() -> cur_vblank-.last == garbage
The lack of drm_vblank_on() is a problem: 1. drm_vblank_get() 2. drm_vblank_off() 3. modeset/dpms/suspend 4. drm_vblank_get() -> -EINVAL
Another issue: 1. drm_vblank_get() 2. drm_vblank_put() 3. disable timer expires which updates .last ... 4. drm_vblank_off() updates .last again 5. modeset/dpms/suspend 6. drm_vblank_get() -> sequence number doesn't account for the time between 3. and 4. I suppose this isn't a big issue, but I don't like leaking implementation details (the timer delay) into the sequence number.
Now this last one should actually work with the current drm_vblank_pre_modeset() since it does a drm_vblank_get() which will apply the cur_vblank-.last diff, but it also enables the vblank interrupt which is entirely pointless, and also wrong on Intel hardware (well, if we didn't have drm_vblank_off()). Our docs say that we shouldn't have the vblank interrupt enabled+unmasked while the pipe is off.
Anyway it's not a very obvious way to do things. Ie. you're doing the drm_vblank_get() not because you actually want vblank interrupts, but because you want the side effects. I usually prefer straightforward code to magic.
Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well.
I get the impression that you're a bit hung up on the names :) We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me.
- It doesn't work across suspend/resume since no one re-enables the vblank interrupt.
That sounds like a driver bug to me. The driver should re-enable the hardware interrupt on resume if the vblank interrupt is currently enabled by the DRM core.
The interrupt is not enabled due to drm_vblank_off(). There's nothing to undo that which is why I added drm_vblank_on().