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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 25, 2013 at 05:12:54AM +0200, Mario Kleiner wrote:
> 
> 
> On 23.09.13 12:02, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The DSL register increments at the start of horizontal sync, so it
> > manages to miss the entire active portion of the current line.
> >
> > Improve the get_scanoutpos accuracy a bit when the scanout position is
> > close to the start or end of vblank. We can do that by double checking
> > the DSL value against the vblank status bit from ISR.
> >
> > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4f74f0c..14b42d9 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -567,6 +567,47 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
> >   	return I915_READ(reg);
> >   }
> >
> > +static bool g4x_pipe_in_vblank(struct drm_device *dev, enum pipe pipe)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	uint32_t status;
> > +
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		status = pipe == PIPE_A ?
> > +			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
> > +			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
> > +
> > +		return I915_READ(VLV_ISR) & status;
> > +	} else if (IS_G4X(dev)) {
> > +		status = pipe == PIPE_A ?
> > +			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
> > +			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
> > +
> > +		return I915_READ(ISR) & status;
> > +	} else if (INTEL_INFO(dev)->gen < 7) {
> > +		status = pipe == PIPE_A ?
> > +			DE_PIPEA_VBLANK :
> > +			DE_PIPEB_VBLANK;
> > +
> > +		return I915_READ(DEISR) & status;
> > +	} else {
> > +		switch (pipe) {
> > +		default:
> > +		case PIPE_A:
> > +			status = DE_PIPEA_VBLANK_IVB;
> > +			break;
> > +		case PIPE_B:
> > +			status = DE_PIPEB_VBLANK_IVB;
> > +			break;
> > +		case PIPE_C:
> > +			status = DE_PIPEC_VBLANK_IVB;
> > +			break;
> > +		}
> > +
> > +		return I915_READ(DEISR) & status;
> > +	}
> > +}
> > +
> >   static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> >   			     int *vpos, int *hpos)
> >   {
> > @@ -616,6 +657,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> >   		 * scanout position from Display scan line register.
> >   		 */
> >   		position = I915_READ(PIPEDSL(pipe)) & 0x1fff;
> > +
> > +		/*
> > +		 * The scanline counter increments at the leading edge
> > +		 * of hsync, ie. it completely misses the active portion
> > +		 * of the line. Fix up the counter at both edges of vblank
> > +		 * to get a more accurate picture whether we're in vblank
> > +		 * or not.
> > +		 */
> > +		in_vbl = g4x_pipe_in_vblank(dev, pipe);
> > +		if ((in_vbl && position == vbl_start - 1) ||
> > +		    (!in_vbl && position == vbl_end - 1))
> > +			position = (position + 1) % vtotal;
> >   	} else {
> >   		/* Have access to pixelcount since start of frame.
> >   		 * We can split this into vertical and horizontal
> >
> 
> 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.

> Also it adds back one lock protected, therefore potentially slow, 
> register read into the time critical code.

I don't think a single register read should be _that_ slow even
with all the extra junk we do. And of course we can fix that problem.

-- 
Ville Syrjälä
Intel OTC