[Intel-gfx] [PATCH 2/2] drm/i915: Tighten timestamp around vblank sampling
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Jun 11 16:15:42 UTC 2020
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 at chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala at 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 at 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
More information about the Intel-gfx
mailing list