Re: [Intel-gfx] [PATCH v4 17/18] drm/i915: Fix vblank timestamps with VRR

From: "Navare, Manasi" <manasi.d.navare@intel.com>
To: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v4 17/18] drm/i915: Fix vblank timestamps with VRR
Date: Thu, 21 Jan 2021 15:05:32 -0800	[thread overview]
Message-ID: <20210121230532.GE26641@labuser-Z97X-UD5H> (raw)
In-Reply-To: <20210113220935.4151-18-manasi.d.navare@intel.com>

On Wed, Jan 13, 2021 at 02:09:34PM -0800, Manasi Navare wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To get sensible vblank timestamping behaviour we need to feed
> the vmax based timings to the vblank code, otherwise it'll chop
> off the scanline counter when it exceeds the minumum vtotal.
> 
> Additionally with VRR we have three cases to consider when we
> generate the vblank timestamp:
> 1) we are in vertical active
>   -> nothing special needs to be done, just return the current
>      scanout position and the core will calculate the timestamp
>      corresponding to the past time when the current vertical
>      active started
> 2) we are in vertical blank and no push has been sent
>   -> the hardware will keep extending the vblank presumably
>      to its maximum length, so we make the timestmap match the
>      expected time when the max length vblank will end. Since
>      the timings used for this are now based on vmax nothing
>      special actually needs to be done
> 3) we are in vblank and a push has been sent so the vblank is
>    about to terminate
>   -> presumably we want the timestmap to accurately reflect
>      when the vblank will terminate, so we use the sampled
>      frame timestamp vs. current timestamp to guesstimate
>      how far along the vblank exit we are, and then we
>      adjust the reported scanout position accordingly so
>      that the core will see that the vblank is close to
>      ending.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_display.c    | 17 ++++++++++++-----
>  .../gpu/drm/i915/display/intel_display_types.h  |  4 ++++
>  drivers/gpu/drm/i915/display/intel_vrr.c        |  4 ++++
>  drivers/gpu/drm/i915/i915_irq.c                 | 15 ++++++++++++++-
>  4 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 4ed279f034be..d989baa44c37 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13835,10 +13835,17 @@ intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	const struct drm_display_mode *adjusted_mode =
> -		&crtc_state->hw.adjusted_mode;
> +	struct drm_display_mode adjusted_mode =
> +		crtc_state->hw.adjusted_mode;
> +
> +	if (crtc_state->vrr.enable) {
> +		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
> +		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
> +		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
> +		crtc->vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
> +	}
>  
> -	drm_calc_timestamping_constants(&crtc->base, adjusted_mode);
> +	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
>  
>  	crtc->mode_flags = crtc_state->mode_flags;
>  
> @@ -13872,8 +13879,8 @@ intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  	if (IS_GEN(dev_priv, 2)) {
>  		int vtotal;
>  
> -		vtotal = adjusted_mode->crtc_vtotal;
> -		if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		vtotal = adjusted_mode.crtc_vtotal;
> +		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
>  			vtotal /= 2;
>  
>  		crtc->scanline_offset = vtotal - 1;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index aa0842028414..3fee613617f1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -679,6 +679,8 @@ struct intel_crtc_scaler_state {
>  #define I915_MODE_FLAG_DSI_USE_TE1 (1<<4)
>  /* Flag to indicate mipi dsi periodic command mode where we do not get TE */
>  #define I915_MODE_FLAG_DSI_PERIODIC_CMD_MODE (1<<5)
> +/* Do tricks to make vblank timestamps sane with VRR? */
> +#define I915_MODE_FLAG_VRR (1<<6)
>  
>  struct intel_wm_level {
>  	bool enable;
> @@ -1186,6 +1188,8 @@ struct intel_crtc {
>  	/* I915_MODE_FLAG_* */
>  	u8 mode_flags;
>  
> +	u16 vmax_vblank_start;
> +
>  	struct intel_display_power_domain_set enabled_power_domains;
>  	struct intel_overlay *overlay;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 9a18c36e4a9a..a494d3ecb1b5 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -134,6 +134,8 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  	 */
>  	crtc_state->vrr.pipeline_full =
>  		min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay - 4 - 1);
> +
> +	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>  }
>  
>  void intel_vrr_enable(struct intel_encoder *encoder,
> @@ -202,4 +204,6 @@ void intel_vrr_get_config(struct intel_crtc *crtc,
>  		crtc_state->vrr.flipline = intel_de_read(dev_priv, TRANS_VRR_FLIPLINE(cpu_transcoder)) + 1;
>  	crtc_state->vrr.vmax = intel_de_read(dev_priv, TRANS_VRR_VMAX(cpu_transcoder)) + 1;
>  	crtc_state->vrr.vmin = intel_de_read(dev_priv, TRANS_VRR_VMIN(cpu_transcoder)) + 1;
> +
> +	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8505ceca87d5..e81afcb2f43e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -893,7 +893,20 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
>  	if (stime)
>  		*stime = ktime_get();
>  
> -	if (use_scanline_counter) {
> +	if (crtc->mode_flags & I915_MODE_FLAG_VRR) {
> +		int scanlines = intel_crtc_scanlines_since_frame_timestamp(crtc);
> +
> +		position = __intel_get_crtc_scanline(crtc);
> +
> +		/*
> +		 * Already exiting vblank? If so, shift our position
> +		 * so it looks like we're already apporaching the full
> +		 * vblank end. This should make the generated timestamp
> +		 * more or less match when the active portion will start.
> +		 */
> +		if (position >= vbl_start && scanlines < position)
> +			position = min(crtc->vmax_vblank_start + scanlines, vtotal - 1);
> +	} if (use_scanline_counter) {
>  		/* No obvious pixelcount register. Only query vertical
>  		 * scanout position from Display scan line register.
>  		 */
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx