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

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

[Vidya Srinivas]

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);
+	}
+
 	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)
+#define GEN7_TIMESTAMP_CTR	_MMIO(0x44070)
+
+#define _PIPE_FRMTMSTMP_A		0x70048
+#define _PIPE_FRMTMSTMP_B		0x71048
+#define _IVB_PIPE_FRMTMSTMP_C	0x72048
+#define PIPE_FRMTMSTMP(pipe)		\
+			_MMIO_PIPE3((pipe), _PIPE_FRMTMSTMP_A, \
+				_PIPE_FRMTMSTMP_B, _IVB_PIPE_FRMTMSTMP_C)
+
 /* 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)
+{
+	struct drm_device *dev = crtc->base.dev;
+	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;
+	u64 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;
+
+	WARN_ON(!crtc_vtotal);
+	if (!crtc_vtotal)
+		return scanline;
+
+	/* 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;
+
+	scanline = div_u64(mul_u64_u32_shr((scan_curr_time - scan_prev_time),
+					crtc_clock, 0), 1000 * crtc_htotal);
+	scanline = min(scanline, (u64)(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,
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 18-09-17 om 15:32 schreef Vidya Srinivas:
> 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);
I really think we shouldn't loop over all encoders for something as critical as __intel_get_crtc_scanline..
> +	}
> +
>  	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)
> +#define GEN7_TIMESTAMP_CTR	_MMIO(0x44070)
> +
> +#define _PIPE_FRMTMSTMP_A		0x70048
> +#define _PIPE_FRMTMSTMP_B		0x71048
> +#define _IVB_PIPE_FRMTMSTMP_C	0x72048
> +#define PIPE_FRMTMSTMP(pipe)		\
> +			_MMIO_PIPE3((pipe), _PIPE_FRMTMSTMP_A, \
> +				_PIPE_FRMTMSTMP_B, _IVB_PIPE_FRMTMSTMP_C)
> +
>  /* 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)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	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;
> +	u64 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;
Aren't all those 0 since they're only set for crtc_state->adjusted_mode, while crtc->mode is assigned to crtc_state->mode? You'd probably need to look at dev->vblank[crtc->pipe].hwmode for those, which should have been passed as parameter..
With a bit of luck you could even use the derived ns values set in drm_calc_timestamping_constants, so you don't have to do the math here yourself.
> +	WARN_ON(!crtc_vtotal);
> +	if (!crtc_vtotal)
> +		return scanline;
> +
> +	/* 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;
> +
> +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time - scan_prev_time),
Isn't mul_u64_u32_div exactly what you want here?
> +					crtc_clock, 0), 1000 * crtc_htotal);
> +	scanline = min(scanline, (u64)(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,


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

On Mon, Sep 18, 2017 at 03:57:38PM +0200, Maarten Lankhorst wrote:
> Op 18-09-17 om 15:32 schreef Vidya Srinivas:
> > 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);
> I really think we shouldn't loop over all encoders for something as critical as __intel_get_crtc_scanline..
> > +	}
> > +
> >  	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)
> > +#define GEN7_TIMESTAMP_CTR	_MMIO(0x44070)
> > +
> > +#define _PIPE_FRMTMSTMP_A		0x70048
> > +#define _PIPE_FRMTMSTMP_B		0x71048
> > +#define _IVB_PIPE_FRMTMSTMP_C	0x72048
> > +#define PIPE_FRMTMSTMP(pipe)		\
> > +			_MMIO_PIPE3((pipe), _PIPE_FRMTMSTMP_A, \
> > +				_PIPE_FRMTMSTMP_B, _IVB_PIPE_FRMTMSTMP_C)
> > +
> >  /* 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)
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	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;
> > +	u64 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;
> Aren't all those 0 since they're only set for crtc_state->adjusted_mode, while crtc->mode is assigned to crtc_state->mode? You'd probably need to look at dev->vblank[crtc->pipe].hwmode for those, which should have been passed as parameter..
> With a bit of luck you could even use the derived ns values set in drm_calc_timestamping_constants, so you don't have to do the math here yourself.

IIRC the derived values ended up having quite a bit of rounding error
in them. Or maybe that just the pixel duration (which IIRC I nuked
already). I would have nuked the line duration too if it wasn't for
nouveau using it. So I think I'd rather not expand its use.

Not sure we'd actually save anything by using the derived value anyway.
We'll still have to do the expensive division at least. Hmm. I guess we
could get rid of one multiplication.

> > +	WARN_ON(!crtc_vtotal);
> > +	if (!crtc_vtotal)
> > +		return scanline;
> > +
> > +	/* 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;
> > +
> > +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time - scan_prev_time),
> Isn't mul_u64_u32_div exactly what you want here?

I think we'd actually want a mul_u32_u32_div(). But that doesn't seem
to exist.

> > +					crtc_clock, 0), 1000 * crtc_htotal);
> > +	scanline = min(scanline, (u64)(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,
> 
> 
> _______________________________________________
> 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

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

[Maarten Lankhorst]

Op 18-09-17 om 16:24 schreef Ville Syrjälä:
> On Mon, Sep 18, 2017 at 03:57:38PM +0200, Maarten Lankhorst wrote:
>> Op 18-09-17 om 15:32 schreef Vidya Srinivas:
>>> 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);
>> I really think we shouldn't loop over all encoders for something as critical as __intel_get_crtc_scanline..
>>> +	}
>>> +
>>>  	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)
>>> +#define GEN7_TIMESTAMP_CTR	_MMIO(0x44070)
>>> +
>>> +#define _PIPE_FRMTMSTMP_A		0x70048
>>> +#define _PIPE_FRMTMSTMP_B		0x71048
>>> +#define _IVB_PIPE_FRMTMSTMP_C	0x72048
>>> +#define PIPE_FRMTMSTMP(pipe)		\
>>> +			_MMIO_PIPE3((pipe), _PIPE_FRMTMSTMP_A, \
>>> +				_PIPE_FRMTMSTMP_B, _IVB_PIPE_FRMTMSTMP_C)
>>> +
>>>  /* 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)
>>> +{
>>> +	struct drm_device *dev = crtc->base.dev;
>>> +	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;
>>> +	u64 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;
>> Aren't all those 0 since they're only set for crtc_state->adjusted_mode, while crtc->mode is assigned to crtc_state->mode? You'd probably need to look at dev->vblank[crtc->pipe].hwmode for those, which should have been passed as parameter..
>> With a bit of luck you could even use the derived ns values set in drm_calc_timestamping_constants, so you don't have to do the math here yourself.
> IIRC the derived values ended up having quite a bit of rounding error
> in them. Or maybe that just the pixel duration (which IIRC I nuked
> already). I would have nuked the line duration too if it wasn't for
> nouveau using it. So I think I'd rather not expand its use.
>
> Not sure we'd actually save anything by using the derived value anyway.
> We'll still have to do the expensive division at least. Hmm. I guess we
> could get rid of one multiplication.
>
>>> +	WARN_ON(!crtc_vtotal);
>>> +	if (!crtc_vtotal)
>>> +		return scanline;
>>> +
>>> +	/* 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;
>>> +
>>> +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time - scan_prev_time),
>> Isn't mul_u64_u32_div exactly what you want here?
> I think we'd actually want a mul_u32_u32_div(). But that doesn't seem
> to exist.
Yeah, couldn't find it either. :(

Why do we even use the macros, can't we simply do the multiplications and divide without?
i915 is only on x86 anyway. :)
>>> +					crtc_clock, 0), 1000 * crtc_htotal);
>>> +	scanline = min(scanline, (u64)(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,
>>
>> _______________________________________________
>> 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

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

[Shankar, Uma]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Maarten Lankhorst
>Sent: Tuesday, September 19, 2017 12:56 PM
>To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
>Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Enable scanline read for gen9 dsi
>
>Op 18-09-17 om 16:24 schreef Ville Syrjälä:
>> On Mon, Sep 18, 2017 at 03:57:38PM +0200, Maarten Lankhorst wrote:
>>> Op 18-09-17 om 15:32 schreef Vidya Srinivas:
>>>> 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);
>>> I really think we shouldn't loop over all encoders for something as critical as
>__intel_get_crtc_scanline..
>>>> +	}
>>>> +
>>>>  	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)
>>>> +#define GEN7_TIMESTAMP_CTR	_MMIO(0x44070)
>>>> +
>>>> +#define _PIPE_FRMTMSTMP_A		0x70048
>>>> +#define _PIPE_FRMTMSTMP_B		0x71048
>>>> +#define _IVB_PIPE_FRMTMSTMP_C	0x72048
>>>> +#define PIPE_FRMTMSTMP(pipe)		\
>>>> +			_MMIO_PIPE3((pipe), _PIPE_FRMTMSTMP_A, \
>>>> +				_PIPE_FRMTMSTMP_B,
>_IVB_PIPE_FRMTMSTMP_C)
>>>> +
>>>>  /* 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) {
>>>> +	struct drm_device *dev = crtc->base.dev;
>>>> +	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;
>>>> +	u64 scanline = 0, scan_prev_time, scan_curr_time, scan_post_time;
>>> Aren't all those 0 since they're only set for crtc_state->adjusted_mode, while
>crtc->mode is assigned to crtc_state->mode? You'd probably need to look at dev-
>>vblank[crtc->pipe].hwmode for those, which should have been passed as
>parameter..
>>> With a bit of luck you could even use the derived ns values set in
>drm_calc_timestamping_constants, so you don't have to do the math here
>yourself.
>> IIRC the derived values ended up having quite a bit of rounding error
>> in them. Or maybe that just the pixel duration (which IIRC I nuked
>> already). I would have nuked the line duration too if it wasn't for
>> nouveau using it. So I think I'd rather not expand its use.
>>
>> Not sure we'd actually save anything by using the derived value anyway.
>> We'll still have to do the expensive division at least. Hmm. I guess
>> we could get rid of one multiplication.
>>

We can use the vblank->mode here to get the timings. Let's not use the
derived stuff, due to any rounding dependency. With the current stuff, we are
getting pretty accurate results. Hope this is ok ?

>>>> +	WARN_ON(!crtc_vtotal);
>>>> +	if (!crtc_vtotal)
>>>> +		return scanline;
>>>> +
>>>> +	/* 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;
>>>> +
>>>> +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time -
>>>> +scan_prev_time),
>>> Isn't mul_u64_u32_div exactly what you want here?
>> I think we'd actually want a mul_u32_u32_div(). But that doesn't seem
>> to exist.
>Yeah, couldn't find it either. :(
>
>Why do we even use the macros, can't we simply do the multiplications and
>divide without?
>i915 is only on x86 anyway. :)

mul_u32_u32 handles it differently using x86 assembly instructions (arch/x86/include/asm/div64.h).
I guess it's more precise than normal u32*u32 multiplication.  

Regards,
Uma Shankar

>>>> +					crtc_clock, 0), 1000 * crtc_htotal);
>>>> +	scanline = min(scanline, (u64)(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,
>>>
>>> _______________________________________________
>>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vidya Srinivas]

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

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);
+
 /* 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)
+		return __intel_get_crtc_scanline_from_timestamp(crtc);
+
 	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
+#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;
+	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);
+	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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Tue, Sep 19, 2017 at 08:49:21AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Maarten Lankhorst
> >Sent: Tuesday, September 19, 2017 12:56 PM
> >To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Enable scanline read for gen9 dsi
> >
> >Op 18-09-17 om 16:24 schreef Ville Syrjälä:
> >> On Mon, Sep 18, 2017 at 03:57:38PM +0200, Maarten Lankhorst wrote:
> >>> Op 18-09-17 om 15:32 schreef Vidya Srinivas:
> >>>> From: Uma Shankar <uma.shankar@intel.com>
<snip>
> >>>> +	WARN_ON(!crtc_vtotal);
> >>>> +	if (!crtc_vtotal)
> >>>> +		return scanline;
> >>>> +
> >>>> +	/* 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;
> >>>> +
> >>>> +	scanline = div_u64(mul_u64_u32_shr((scan_curr_time -
> >>>> +scan_prev_time),
> >>> Isn't mul_u64_u32_div exactly what you want here?
> >> I think we'd actually want a mul_u32_u32_div(). But that doesn't seem
> >> to exist.
> >Yeah, couldn't find it either. :(
> >
> >Why do we even use the macros, can't we simply do the multiplications and
> >divide without?
> >i915 is only on x86 anyway. :)
> 
> mul_u32_u32 handles it differently using x86 assembly instructions (arch/x86/include/asm/div64.h).
> I guess it's more precise than normal u32*u32 multiplication.  

It's a workaround for modern gcc which (for whatever reason) can no
longer figure out that on a 32bit machine we can do this witha a 
normal 32x32->64 multiplication rather than a full blown 64 bit
multiplication, which would be more expensive.

Earlier gcc versions did work this out correctly without handholding,
but somehow they broke it at some point.

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

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

[Ville Syrjälä]

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

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

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, September 22, 2017 6:58 PM
>To: Srinivas, Vidya <vidya.srinivas@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>;
>Kamath, Sunil <sunil.kamath@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Konduru, Chandra <chandra.konduru@intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read based on frame timestamps
>
>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 ;)
>

Ok, will be careful 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.
>

Will remove this.

>> +
>>  /* 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).
>

Yeah, while adding only was not 100% sure about this change. Missed that a private_flag
also exists. Will correct this.

>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.
>

Will do.

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

Will rectify this.

>> +#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.
>

Sure, will change it.

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

Will check this one out and modify.

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

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

[Vidya Srinivas]

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)
+{
+	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)
 
 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

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

[Ville Syrjälä]

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

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

[Vidya Srinivas]

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 flags from uapi and used a local
i915 data structure instead.

v6: Used vblank hwmode to get the timings.

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  |   53 ++++++++++++++++++++++++++++++++++++++
 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, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 91a2c5d..4e44d59 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -772,6 +772,56 @@ 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)
+{
+	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 vblank_start = mode->crtc_vblank_start;
+	u32 vtotal = mode->crtc_vtotal;
+	u32 htotal = mode->crtc_htotal;
+	u32 clock = mode->crtc_clock;
+	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,
+				       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 +838,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)
 
 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

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

[Saarinen, Jani]

HI, 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Ville Syrjälä
> Sent: perjantai 22. syyskuuta 2017 18.48
> To: Srinivas, Vidya <vidya.srinivas@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v5] drm/i915: Enable scanline read based on
> frame timestamps
> 
> 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? :)
According to try-bot, yes:
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_1180/


> 
> >
> >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Fri, 22 Sep 2017, Vidya Srinivas <vidya.srinivas@intel.com> 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 flags from uapi and used a local
> i915 data structure instead.
>
> v6: Used vblank hwmode to get the timings.
>
> 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  |   53 ++++++++++++++++++++++++++++++++++++++
>  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, 71 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5d..4e44d59 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -772,6 +772,56 @@ 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)
> +{
> +	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 vblank_start = mode->crtc_vblank_start;
> +	u32 vtotal = mode->crtc_vtotal;
> +	u32 htotal = mode->crtc_htotal;
> +	u32 clock = mode->crtc_clock;
> +	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,
> +				       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 +838,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)

As Ville wrote,

"""
s/DRM_/I915_/

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

>  
>  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));

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vidya Srinivas]

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 flags from uapi and used a local
i915 data structure instead.

v6: Used vblank hwmode to get the timings.

v7: Fixed sparse warnings, indentation and minor review comments.

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>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  |   53 ++++++++++++++++++++++++++++++++++++++
 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, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 91a2c5d..04aa099 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -772,6 +772,56 @@ 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.
+ */
+static 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 vblank_start = mode->crtc_vblank_start;
+	u32 vtotal = mode->crtc_vtotal;
+	u32 htotal = mode->crtc_htotal;
+	u32 clock = mode->crtc_clock;
+	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,
+					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 +838,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 & I915_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..4170490 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 I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<1)
 
 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..c189726 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 |=
+			I915_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 |=
+			I915_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

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

[Ville Syrjälä]

On Mon, Sep 25, 2017 at 04:23: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 flags from uapi and used a local
> i915 data structure instead.
> 
> v6: Used vblank hwmode to get the timings.
> 
> v7: Fixed sparse warnings, indentation and minor review comments.
> 
> 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>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c  |   53 ++++++++++++++++++++++++++++++++++++++
>  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, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5d..04aa099 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -772,6 +772,56 @@ 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.
> + */
> +static 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 vblank_start = mode->crtc_vblank_start;
> +	u32 vtotal = mode->crtc_vtotal;
> +	u32 htotal = mode->crtc_htotal;
> +	u32 clock = mode->crtc_clock;
> +	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,
> +					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 +838,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 & I915_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..4170490 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 I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<1)
>  
>  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..c189726 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 |=
> +			I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;

I just realized that this is in the wrong place. We don't want it for
VLV/CHV.

>  
>  	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 |=
> +			I915_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

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

[Shankar, Uma]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Ville Syrjälä
>Sent: Monday, September 25, 2017 6:46 PM
>To: Srinivas, Vidya <vidya.srinivas@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable scanline read based on frame
>timestamps
>
>On Mon, Sep 25, 2017 at 04:23: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 flags from uapi and
>> used a local
>> i915 data structure instead.
>>
>> v6: Used vblank hwmode to get the timings.
>>
>> v7: Fixed sparse warnings, indentation and minor review comments.
>>
>> 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>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c  |   53
>++++++++++++++++++++++++++++++++++++++
>>  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, 71 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c index 91a2c5d..04aa099 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -772,6 +772,56 @@ 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.
>> + */
>> +static 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 vblank_start = mode->crtc_vblank_start;
>> +	u32 vtotal = mode->crtc_vtotal;
>> +	u32 htotal = mode->crtc_htotal;
>> +	u32 clock = mode->crtc_clock;
>> +	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,
>> +					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 +838,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 &
>I915_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..4170490 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
>> +I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<1)
>>
>>  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..c189726 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 |=
>> +			I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
>
>I just realized that this is in the wrong place. We don't want it for VLV/CHV.
>

Yes indeed. Will fix and resend. 

Regards,
Uma Shankar

>>
>>  	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 |=
>> +			I915_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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx