Re: [PATCH 1/2] drm/i915: Enable scanline read for gen9 dsi

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

On Mon, Sep 18, 2017 at 07:02:21PM +0530, Vidya Srinivas wrote:
> From: Uma Shankar <uma.shankar@intel.com>
> 
> For gen9 platforms, dsi timings are driven from port instead of pipe
> (unlike ddi). Thus, we can't rely on pipe registers to get the timing
> information. Even 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
> 
> 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      |  7 +++++
>  drivers/gpu/drm/i915/i915_reg.h      | 11 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 80 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1cc31a5..d9efe83 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 gen9_get_scanline(struct intel_crtc *crtc);
> +
>  /* 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 5d391e6..47668dd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -781,6 +781,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	struct drm_vblank_crtc *vblank;
>  	enum pipe pipe = crtc->pipe;
>  	int position, vtotal;
> +	struct intel_encoder *encoder;
>  
>  	if (!crtc->active)
>  		return -1;
> @@ -792,6 +793,12 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>  		vtotal /= 2;
>  
> +	if (IS_BROXTON(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> +		for_each_encoder_on_crtc(crtc->base.dev, &crtc->base, encoder)
> +			if (encoder->type == INTEL_OUTPUT_DSI)
> +				return gen9_get_scanline(crtc);
> +	}

We're going to want a better way to do this. I think what we could so is
stuff some kind of flag into hwmode->private_flags to indicate that we
should use the frame timestamps instead of the scanline counter.

> +
>  	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 0b03260..85168ee 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8802,6 +8802,17 @@ 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_CTR	_MMIO(MCHBAR_MIRROR_BASE + 0x2358)

Just 0x2358. Should be called just GEN4_TIMESTAMP.

If we're going to define that, then we should also define
'ILK_TIMESTAMP_HI 0x70070' for completeness. ILK preferred over GEN5
since this one in particular is a display register.

> +#define GEN7_TIMESTAMP_CTR	_MMIO(0x44070)

I would call it 'IVB_TIMESTAMP_CTR' since it's a display register
and doesn't apply to VLV/CHV.

> +
> +#define _PIPE_FRMTMSTMP_A		0x70048
> +#define _PIPE_FRMTMSTMP_B		0x71048
> +#define _IVB_PIPE_FRMTMSTMP_C	0x72048

Leave the pipe C out.

> +#define PIPE_FRMTMSTMP(pipe)		\
> +			_MMIO_PIPE3((pipe), _PIPE_FRMTMSTMP_A, \
> +				_PIPE_FRMTMSTMP_B, _IVB_PIPE_FRMTMSTMP_C)

_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 0871807..601032f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10352,6 +10352,66 @@ static bool needs_scaling(const struct intel_plane_state *state)
>  	return (src_w != dst_w || src_h != dst_h);
>  }
>  
> +/*
> + * For Gen9 DSI, pipe scanline register will not
> + * work to get the scanline since the timings
> + * are driven from the PORT (unlike DDI encoders).
> + * This function will use Framestamp and current
> + * timestamp registers to calculate the scanline.
> + */
> +u32 gen9_get_scanline(struct intel_crtc *crtc)

Nothing gen9 specific in this function. If we depend on TIMESTAMP_CTR
then it applies to ivb+, so we maybe want to put that into the name, or
maybe not.

Maybe call it '__intel_get_crtc_scanline_from_timestamp()' or something
like that.

> +{
> +	struct drm_device *dev = crtc->base.dev;

Useless variable.

> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	u32 crtc_vblank_start = crtc->base.mode.crtc_vblank_start;
> +	u32 crtc_vtotal = crtc->base.mode.crtc_vtotal;
> +	u32 crtc_htotal = crtc->base.mode.crtc_htotal;
> +	u32 crtc_clock = crtc->base.mode.crtc_clock;

vblank->hwmode

> +	u64 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;

everything can be u32.

> +
> +	WARN_ON(!crtc_vtotal);
> +	if (!crtc_vtotal)
> +		return scanline;

We don't check for that anywhere else, so no point in doing it here
either.  __intel_get_crtc_scanline() checks for crtc->active which I
guess we might want to follow here. IIRC I added that check to make
it safe to call __intel_get_crtc_scanline() from tracepoints at any
time. But normally we shouldn't need that check. 

> +
> +	/* 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(GEN7_TIMESTAMP_CTR);
> +
> +		scan_post_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc->pipe));
> +	} while (scan_post_time != scan_prev_time);
> +
> +	/*
> +	 * Since the register is 32 bit and the values
> +	 * can overflow and wrap around, making sure
> +	 * current time accounts for the register
> +	 * wrap
> +	 */
> +	if (scan_curr_time < scan_prev_time)
> +		scan_curr_time += 0x100000000;

Not needed.

> +
> +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time - scan_prev_time),

Just mul_u32_u32(). Useless parens around the subtraction.

> +					crtc_clock, 0), 1000 * crtc_htotal);
> +	scanline = min(scanline, (u64)(crtc_vtotal - 1));

The cast can be nuked when the types are corrected.

> +	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,
> -- 
> 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