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: > 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 I'd summarize these as 'drm_vblank_off() considered harmful'. > 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. 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. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
