[Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
michel at daenzer.net
Wed May 28 21:11:00 PDT 2014
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
>>> - 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
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
> 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
More information about the dri-devel