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

[Mario Kleiner <mario.kleiner@tuebingen.mpg.de>]

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?

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?

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

-mario
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx