Re: [PATCH] drm/i915: Fix pre-CTG vblank counter

[Mario Kleiner <mario.kleiner.de@gmail.com>]

On 25.09.13 22:55, Daniel Vetter wrote:
> On Wed, Sep 25, 2013 at 07:55:26PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> The old style frame counter increments at the start of active video.
>> However for i915_get_vblank_counter() we want a counter that increments
>> at the start of vblank.
>>
>> Fortunately the low frame counter register also contains the pixel
>> counter for the current frame. We can can compare that against the
>> vblank start pixel count to determine if we need to increment the
>> frame counter by 1 to get the correct answer.
>>
>> Also reorganize the function pointer assignments in intel_irq_init() a
>> bit to avoid confusing people.
>>
>> Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>
>> Just another small vblank related gem I forgot to polish up and send
>> out until Imre started asking me questions about the vblank counter
>> functions.
>
> Hm, I've thought the magic fixup code does take care of that for us? But I
> agree that we should do this explicitly ...
>
> This could explain some of the strange vblank timestamp off failures QA
> has reported (if there's too much delay and the fixup doesn't fire any
> more), care to attach this patch to the relevant bug reports? Searching
> for ts jitter + pre-gen5 should be good enough.
> -Daniel
>

The vblank off code has one known race in that if 
dev->driver->get_vblank_counter does not increment its counter at start 
of vblank, it can have off-by-one vblank. There's a comment in that 
function about that, e.g.,

http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_irq.c#L102

The magic code there only fixes races between the vblank off code and 
the vblank irq handler. Normally that race would not really affect 
applications as long as the vblank_off_delay is as large (5 secs) as it 
is now. But in QA testing, if you exercise that code not the way a 
normal app would do it, i'd expect you to see that error about 4% of the 
time.

The only fix is to fixup the vblank counter in the kms driver, so this 
patch looks like a perfect solution to that. You can add my reviewed-by 
if you wish.

-mario