Re: [PATCH v5] drm/i915: Enable scanline read based on frame timestamps

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

On Fri, Sep 22, 2017 at 09:11:30PM +0530, Vidya Srinivas wrote:
> From: Uma Shankar <uma.shankar@intel.com>
> 
> For certain platforms on certain encoders, timings are driven
> from port instead of pipe. Thus, we can't rely on pipe scanline
> registers to get the timing information. Some cases scanline
> register read will not be functional.
> This is causing vblank evasion logic to fail since it relies on
> scanline, causing atomic update failure warnings.
> 
> This patch uses pipe framestamp and current timestamp registers
> to calculate scanline. This is an indirect way to get the scanline.
> It helps resolve atomic update failure for gen9 dsi platforms.
> 
> v2: Addressed Ville and Daniel's review comments. Updated the
> register MACROs, handled race condition for register reads,
> extracted timings from the hwmode. Removed the dependency on
> crtc->config to get the encoder type.
> 
> v3: Made get scanline function generic
> 
> v4: Addressed Ville's review comments. Added a flag to decide timestamp
> based scanline reporting. Changed 64bit variables to u32
> 
> v5: Adressed Ville's review comments. Put the scanline compute function
> at the place of caller. Removed hwmode flag from uapi and used a local
> private_flag instead.
> 
> Credits-to: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c  |   50 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h  |    9 +++++++
>  drivers/gpu/drm/i915/intel_drv.h |    2 ++
>  drivers/gpu/drm/i915/intel_dsi.c |    7 ++++++
>  4 files changed, 68 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5d..6d7cd20 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -772,6 +772,53 @@ static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  	return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
>  }
>  
> +/*
> + * On certain encoders on certain platforms, pipe
> + * scanline register will not work to get the scanline,
> + * since the timings are driven from the PORT or issues
> + * with scanline register updates.
> + * This function will use Framestamp and current
> + * timestamp registers to calculate the scanline.
> + */
> +u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)

static

I think sparse would have caught that. Please compile w/ C=1 to make sure
sparse stays happy. (+ run checkpatch ofc)


> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 vblank_start = crtc->base.hwmode.crtc_vblank_start;
> +	u32 vtotal = crtc->base.hwmode.crtc_vtotal;
> +	u32 htotal = crtc->base.hwmode.crtc_htotal;
> +	u32 clock = crtc->base.hwmode.crtc_clock;
> +	u32 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;
> +
> +	/* To avoid the race condition where we might cross into the
> +	 * next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR
> +	 * reads. We make sure we read PIPE_FRMTMSTMP and TIMESTAMP_CTR
> +	 * during the same frame.
> +	 */
> +	do {
> +		/*
> +		 * This field provides read back of the display
> +		 * pipe frame time stamp. The time stamp value
> +		 * is sampled at every start of vertical blank.
> +		 */
> +		scan_prev_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc->pipe));
> +
> +		/*
> +		 * The TIMESTAMP_CTR register has the current
> +		 * time stamp value.
> +		 */
> +		scan_curr_time = I915_READ_FW(IVB_TIMESTAMP_CTR);
> +
> +		scan_post_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc->pipe));
> +	} while (scan_post_time != scan_prev_time);
> +
> +	scanline = div_u64(mul_u32_u32(scan_curr_time - scan_prev_time,
> +				       clock), 1000 * htotal);
> +	scanline = min(scanline, vtotal - 1);
> +	scanline = (scanline + vblank_start) % vtotal;
> +
> +	return scanline;
> +}
> +
>  /* I915_READ_FW, only for fast reads of display block, no need for forcewake etc. */
>  static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  {
> @@ -788,6 +835,9 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	vblank = &crtc->base.dev->vblank[drm_crtc_index(&crtc->base)];
>  	mode = &vblank->hwmode;
>  
> +	if (mode->private_flags & DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)
> +		return __intel_get_crtc_scanline_from_timestamp(crtc);
> +
>  	vtotal = mode->crtc_vtotal;
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>  		vtotal /= 2;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9f03cd0..fbd00cc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8804,6 +8804,15 @@ enum skl_power_gate {
>  #define MIPIO_TXESC_CLK_DIV2			_MMIO(0x160008)
>  #define  GLK_TX_ESC_CLK_DIV2_MASK			0x3FF
>  
> +/* Gen4+ Timestamp and Pipe Frame time stamp registers */
> +#define GEN4_TIMESTAMP		_MMIO(0x2358)
> +#define ILK_TIMESTAMP_HI	_MMIO(0x70070)
> +#define IVB_TIMESTAMP_CTR	_MMIO(0x44070)
> +
> +#define _PIPE_FRMTMSTMP_A		0x70048
> +#define PIPE_FRMTMSTMP(pipe)		\
> +			_MMIO_PIPE2(pipe, _PIPE_FRMTMSTMP_A)
> +
>  /* BXT MIPI clock controls */
>  #define BXT_MAX_VAR_OUTPUT_KHZ			39500
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3078076..ca458f5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -494,6 +494,8 @@ struct intel_crtc_scaler_state {
>  
>  /* drm_mode->private_flags */
>  #define I915_MODE_FLAG_INHERITED 1
> +/* Flag to get scanline using frame time stamps */
> +#define DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<1)

s/DRM_/I915_/

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

Does it still work? :)

>  
>  struct intel_pipe_wm {
>  	struct intel_wm_level wm[5];
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 578254a..f91ccc7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -328,6 +328,9 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>  
>  	/* DSI uses short packets for sync events, so clear mode flags for DSI */
>  	adjusted_mode->flags = 0;
> +	/* Enable Frame time stamo based scanline reporting */
> +	adjusted_mode->private_flags |=
> +			DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
>  
>  	if (IS_GEN9_LP(dev_priv)) {
>  		/* Dual link goes to DSI transcoder A. */
> @@ -1102,6 +1105,10 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  				pixel_format_from_register_bits(fmt));
>  	bpp = pipe_config->pipe_bpp;
>  
> +	/* Enable Frame time stamo based scanline reporting */
> +	adjusted_mode->private_flags |=
> +			DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
> +
>  	/* In terms of pixels */
>  	adjusted_mode->crtc_hdisplay =
>  				I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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