[RFC PATCH] drm: Fix off-by-one races on vblank disable

[Andrew Lutomirski]

2011/11/17 Michel Dänzer <michel at daenzer.net>:
> [ Dropping intel-gfx list from CC, as it automatically rejects posts
> from non-subscribers ]
>
> On Mit, 2011-11-16 at 21:39 -0800, Andy Lutomirski wrote:
>> There are two possible races when disabling vblanks.  If the IRQ
>> fired but the hardware didn't update its counter yet, then we store
>> too low a hardware counter.  (Sensible hardware never does this.
>> Apparently not all hardware is sensible.)
>
> The thing is, 'the' IRQ and 'the' hardware counter aren't necessarily
> about the same thing. We want an IRQ which triggers at the beginning of
> vertical blank, but the Radeon hardware counters increment at the
> beginning of scanout, i.e. at the end of vertical blank. Does that make
> the hardware 'broken' or 'not sensible'?

Perhaps not.  I think that using that counter as the return value of
get_vblank_counter is a bit unfortunate in that case, though.  Anyway,
I'm not particularly attached to the comment as written.

Am I correct in guessing that Radeon fires its vblank irq at the
beginning of vblank instead of at the end  And doesn't this mean that
Radeon has a separate issue when vblank_get happens during the vblank
interval?  A trick similar to drm_calc_vbltimestamp_from_scanoutpos
ought to be able to fix that.

>
>> If, on the other hand, the counter updated but the IRQ didn't fire
>> yet, we store too high a counter.
>>
>> We handled the former case with a heuristic based on timestamps and
>> we did not handle the latter case.  By saving a little more state,
>> we can handle both cases exactly: all we need to do is watch for
>> changes in the difference between the hardware and software vblank
>> counts.
>
> I'm afraid that can't work:
>
> Some (AFAIR also Intel) hardware resets the counter to 0 when the CRTC
> is disabled / enabled (e.g. for DPMS, or a modeset). That's why we ended
> up only counting interrupts while the IRQ is enabled, and only using the
> hardware counter to fill in while the IRQ is disabled. The hardware
> counter cannot be assumed to have any defined behaviour between enabling
> and disabling the IRQ.
>
> To compensate for this, the drivers call drm_vblank_pre_modeset (which
> enables the IRQ, which also updates the virtual counter from the
> hardware counter) before disabling the CRTC and drm_vblank_post_modeset
> (which disables the IRQ, which also records the hardware counter) after
> enabling the CRTC.
>

Shouldn't this be fixable, though, by adding appropriate logic in
drm_vblank_pre_modeset and drm_vblank_post_modeset?


Would this be a lot simpler if drm had a function (or call to the
driver) to compute both the vblank count and the last vblank
timestamp?  That way the irq handler, drm_update_vblank_count,
vblank_disable_and_save, and pre/post modeset could keep everything
exactly in sync.  IIRC i915 has this capability in hardware, and if
Radeon indeed updates the frame count exactly at the end of the vblank
interval, it could compute this exactly as well.


--Andy