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

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

On Tue, Sep 19, 2017 at 02:50:03PM +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 may not be functional due to certain hw issues.
> 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 and Maarten's review comments. Used vblank
> hwmode to get the timings. Added a flag to decide timestamp
> based scanline reporting. Changed 64bit variables to u32

The patch subject is missing the 'v4'. Which is perhaps why
I didn't even notice this sitting in my inbox. Hint for next time ;)

> 
> 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_drv.h      |  2 ++
>  drivers/gpu/drm/i915/i915_irq.c      |  4 +++
>  drivers/gpu/drm/i915/i915_reg.h      |  9 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c     |  9 +++++++
>  include/uapi/drm/drm_mode.h          |  3 +++
>  6 files changed, 78 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 28ad5da..eea374d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4085,6 +4085,8 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
>  u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
>  void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>  
> +u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc);

We have just one caller for this, so it should just live next to that
caller.

> +
>  /* intel_dpio_phy.c */
>  void bxt_port_to_phy_channel(struct drm_i915_private *dev_priv, enum port port,
>  			     enum dpio_phy *phy, enum dpio_channel *ch);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 003a928..ccde6c2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -825,6 +825,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>  		vtotal /= 2;
>  
> +	if (mode->flags &
> +		DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)

Ahem, private_flags. The fact that you had to modify a uapi header for
a flag that's only used internally should have have been a red flag
(no pun intentded).

Also indentation is off.

> +		return __intel_get_crtc_scanline_from_timestamp(crtc);

We don't need the vtotal value we computed above, so I think it would
be less confusing if you do this while thing before we compute vtotal.

> +
>  	if (IS_GEN2(dev_priv))
>  		position = I915_READ_FW(PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
>  	else
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 94b40a4..8afb14d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8806,6 +8806,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		0x2358
> +#define ILK_TIMESTAMP_HI	0x70070

_MMIO missing from those two.

> +#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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8599e42..c3e86f3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10353,6 +10353,57 @@ static bool needs_scaling(const struct intel_plane_state *state)
>  	return (src_w != dst_w || src_h != dst_h);
>  }
>  
> +/*
> + * 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)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct drm_vblank_crtc *vblank =
> +		&crtc->base.dev->vblank[drm_crtc_index(&crtc->base)];
> +	const struct drm_display_mode *mode =
> +		&vblank->hwmode;
> +	u32 crtc_vblank_start = mode->crtc_vblank_start;
> +	u32 crtc_vtotal = mode->crtc_vtotal;
> +	u32 crtc_htotal = mode->crtc_htotal;
> +	u32 crtc_clock = mode->crtc_clock;

You can leave the crtc_ prefix out from your local variables. It doesn't
provide us with any useful infromation here, and we don't have it
elsewhere in the vbl timestamp/scanline code either. One should almost
always try to follow existing conventions in the code so that people
don't have to wonder why things happen to different this time around.

> +	u32 scanline, 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,
> +					crtc_clock), 1000 * crtc_htotal);

Hmm. Indentation seems a bit off again. You really should do something
about your editor...

> +	scanline = min(scanline, crtc_vtotal - 1);
> +	scanline = (scanline + crtc_vblank_start) % crtc_vtotal;
> +
> +	return scanline;
> +}
> +
>  int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
>  				    struct drm_crtc_state *crtc_state,
>  				    const struct intel_plane_state *old_plane_state,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 578254a..dd27c19 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -329,6 +329,11 @@ 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;
>  
> +	/* Add flag to enable scanline read using frametimestamp */
> +	if (IS_GEN9_LP(dev_priv))
> +		adjusted_mode->flags =
> +			DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
> +
>  	if (IS_GEN9_LP(dev_priv)) {
>  		/* Dual link goes to DSI transcoder A. */
>  		if (intel_dsi->ports == BIT(PORT_C))
> @@ -1102,6 +1107,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 stamp based scanline reporting */
> +	adjusted_mode->flags |=
> +			DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
> +
>  	/* In terms of pixels */
>  	adjusted_mode->crtc_hdisplay =
>  				I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 34b6bb3..17f4d51 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -85,6 +85,9 @@
>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
>  
> +/* Flag to get scanline using frame time stamps */
> +#define DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<18)
> +
>  /* Picture aspect ratio options */
>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
>  #define DRM_MODE_PICTURE_ASPECT_4_3		1
> -- 
> 1.9.1

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