Re: [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 25, 2013 at 11:11:30AM +0300, Ville Syrjälä wrote:
> On Wed, Sep 25, 2013 at 05:12:54AM +0200, Mario Kleiner wrote:
> > This one i don't know. I think i can't follow the logic, but i don't 
> > know enough about the way the intel hw counts.
> > 
> > Do you mean the counter increments when the scanline is over, instead of 
> > when it begins?
> 
> Let me draw a picture of the scanline (not to scale):
> 
>  |XXXXXXXXXXXXX|-----|___________|---|
>   horiz. active       horiz. sync
>  ^                   ^
>  |                   |
>  first pixel         this is where the
>  of the line         scanline counter increments
> 
> > With this correction by +1 at the edges of vblank, the scanlines at 
> > vbl_start and vbl_end would be reported twice, for two successive 
> > scanline durations, that seems a bit weird and asymmetric to the rest of 
> > the scanline positions. Wouldn't it make more sense to simply always add 
> > 1 for a smaller overall error, given that hblank is shorter than the 
> > active scanout part of a scanline?
> 
> Since the counter increments too late, drm_handle_vblank()
> may get the wrong idea ie. something like this may happen:
> 
> 1. vblank irq triggered
> 2. drm_handle_vblank() gets called
> 3. i915_get_crtc_scanoutpos() returns vbl_start-1 as the scanline
> 4. delta_ns calculation gets confused and tries to correct for it
> 
> Now, the correction you do for delta_ns should handle this, but
> I don't like having such kludges in common code, and we can handle
> it in the driver as I've demonstrated. But yeah, I suppose it can
> make the error slightly less stable.
> 
> For some other uses (atomic page flip stuff) of the scanline position,
> I definitely want this correction since I need accurate information
> whether the position has passed vblank start.

At least on modern platforms I think we should take a good look at the
timestamp registers. With those the only thing we essentially need to do
is to correct the spot where the timestamp is taken to make it suitable
for OML_sync ... But doing such a switch is something for future work ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch