Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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().

-- 
Ville Syrjälä
Intel OTC