Re: [Intel-gfx] [PATCH 2/2] drm/i915: Tighten timestamp around vblank sampling

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

On Thu, Jun 11, 2020 at 01:30:38PM +0100, Chris Wilson wrote:
> Tighten the timestamp queries before/after the register read so that we
> have less uncertainity for when the read actually took place. This is
> more apt for the older generations where it is not a simple single
> register read. Whether we are able to discern an improvement in our
> sampling accuracy remains to be seen.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apart from the code getting a bit uglier can't really think
of any downsides at least. Upsides (if any) I guess we shall
see from the ci reports.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 57 ++++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8e823ba25f5f..9c44df8ecce7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -713,7 +713,9 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
>   * This function will use Framestamp and current
>   * timestamp registers to calculate the scanline.
>   */
> -static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
> +static u32
> +__intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc,
> +					 ktime_t *stime, ktime_t *etime)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct drm_vblank_crtc *vblank =
> @@ -737,6 +739,9 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
>  		 * pipe frame time stamp. The time stamp value
>  		 * is sampled at every start of vertical blank.
>  		 */
> +		if (stime)
> +			*stime = ktime_get();
> +
>  		scan_prev_time = intel_de_read_fw(dev_priv,
>  						  PIPE_FRMTMSTMP(crtc->pipe));
>  
> @@ -746,6 +751,9 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
>  		 */
>  		scan_curr_time = intel_de_read_fw(dev_priv, IVB_TIMESTAMP_CTR);
>  
> +		if (etime)
> +			*etime = ktime_get();
> +
>  		scan_post_time = intel_de_read_fw(dev_priv,
>  						  PIPE_FRMTMSTMP(crtc->pipe));
>  	} while (scan_post_time != scan_prev_time);
> @@ -762,7 +770,8 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
>   * intel_de_read_fw(), only for fast reads of display block, no need for
>   * forcewake etc.
>   */
> -static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> +static int __intel_get_crtc_scanline(struct intel_crtc *crtc,
> +				     ktime_t *stime, ktime_t *etime)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -771,23 +780,34 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	enum pipe pipe = crtc->pipe;
>  	int position, vtotal;
>  
> -	if (!crtc->active)
> +	if (!crtc->active) {
> +		if (stime)
> +			*stime = 0;
> +		if (etime)
> +			*etime = 0;
>  		return -1;
> +	}
>  
>  	vblank = &crtc->base.dev->vblank[drm_crtc_index(&crtc->base)];
>  	mode = &vblank->hwmode;
>  
>  	if (crtc->mode_flags & I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)
> -		return __intel_get_crtc_scanline_from_timestamp(crtc);
> +		return __intel_get_crtc_scanline_from_timestamp(crtc,
> +								stime,
> +								etime);
>  
>  	vtotal = mode->crtc_vtotal;
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>  		vtotal /= 2;
>  
> +	if (stime)
> +		*stime = ktime_get();
>  	if (IS_GEN(dev_priv, 2))
>  		position = intel_de_read_fw(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
>  	else
>  		position = intel_de_read_fw(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
> +	if (etime)
> +		*etime = ktime_get();
>  
>  	/*
>  	 * On HSW, the DSL reg (0x70000) appears to return 0 if we
> @@ -806,7 +826,13 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  
>  		for (i = 0; i < 100; i++) {
>  			udelay(1);
> +
> +			if (stime)
> +				*stime = ktime_get();
>  			temp = intel_de_read_fw(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
> +			if (etime)
> +				*etime = ktime_get();
> +
>  			if (temp != position) {
>  				position = temp;
>  				break;
> @@ -866,21 +892,25 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>  
>  	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
>  
> -	/* Get optional system timestamp before query. */
> -	if (stime)
> -		*stime = ktime_get();
> -
>  	if (use_scanline_counter) {
>  		/* No obvious pixelcount register. Only query vertical
>  		 * scanout position from Display scan line register.
>  		 */
> -		position = __intel_get_crtc_scanline(crtc);
> +		position = __intel_get_crtc_scanline(crtc, stime, etime);
>  	} else {
> +		/* Get optional system timestamp before query. */
> +		if (stime)
> +			*stime = ktime_get();
> +
>  		/* Have access to pixelcount since start of frame.
>  		 * We can split this into vertical and horizontal
>  		 * scanout position.
>  		 */
> -		position = (intel_de_read_fw(dev_priv, PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT;
> +		position = intel_de_read_fw(dev_priv, PIPEFRAMEPIXEL(pipe));
> +
> +		/* Get optional system timestamp after query. */
> +		if (etime)
> +			*etime = ktime_get();
>  
>  		/* convert to pixel counts */
>  		vbl_start *= htotal;
> @@ -896,6 +926,7 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>  		 * matches how the scanline counter based position works since
>  		 * the scanline counter doesn't count the two half lines.
>  		 */
> +		position = (position & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT;
>  		if (position >= vtotal)
>  			position = vtotal - 1;
>  
> @@ -911,10 +942,6 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>  		position = (position + htotal - hsync_start) % vtotal;
>  	}
>  
> -	/* Get optional system timestamp after query. */
> -	if (etime)
> -		*etime = ktime_get();
> -
>  	/* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> @@ -956,7 +983,7 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	int position;
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	position = __intel_get_crtc_scanline(crtc);
> +	position = __intel_get_crtc_scanline(crtc, NULL, NULL);
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
>  	return position;
> -- 
> 2.27.0

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx