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

["Michel Dänzer" <michel@daenzer.net>]

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