On 28.05.2014 20:19, Ville Syrjälä wrote:
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:
- vblank_disable_and_save() updates .last
- modeset/dpms/suspend (hw counter is reset)
- drm_vblank_get() -> cur_vblank-.last == garbage
The lack of drm_vblank_on() is a problem:
- drm_vblank_get()
- drm_vblank_off()
- modeset/dpms/suspend
- drm_vblank_get() -> -EINVAL
I'd summarize these as 'drm_vblank_off() considered harmful'.
Another issue:
- drm_vblank_get()
- drm_vblank_put()
- 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.
Yes, I guess drm_vblank_off() shouldn't call vblank_disable_and_save() if vblank is already disabled.
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.
That's a driver implementation detail. The driver isn't required to keep the hardware interrupt enabled all the time between the enable_vblank() and disable_vblank() hook calls. The DRM core just wants drm_handle_vblank() to be called for any vertical blank periods between them.
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.
No, that's not the only reason. It's also so that drm_handle_vblank() is called for any vertical blank periods occurring while the hardware counter might reset, so the DRM vblank counter gets incremented independently from the hardware counter.
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 :)
Not at all. In fact, the pre/post_modeset names are slightly misleading, as they're not only about modesetting but about preventing the DRM vblank counter from jumping due to hardware counter jumps.
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.
I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway.