* [PATCH] drm/i915: Enable scanline read for gen9 dsi
@ 2017-09-18 13:41 Vidya Srinivas
2017-09-18 16:49 ` ✗ Fi.CI.BAT: failure for drm/i915: Enable scanline read for gen9 dsi (rev5) Patchwork
2017-09-19 2:31 ` [PATCH] drm/i915: Enable scanline read for gen9 dsi kbuild test robot
0 siblings, 2 replies; 27+ messages in thread
From: Vidya Srinivas @ 2017-09-18 13:41 UTC (permalink / raw)
To: intel-gfx; +Cc: 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 28ad5da..5178330 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 003a928..bb30711 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -814,6 +814,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;
@@ -825,6 +826,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 94b40a4..47d1241 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8806,6 +8806,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 8599e42..c14e8bc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10353,6 +10353,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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH] drm/i915: Enable scanline read for gen9 dsi
@ 2017-09-08 13:48 Vidya Srinivas
2017-09-08 14:47 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Vidya Srinivas @ 2017-09-08 13:48 UTC (permalink / raw)
To: intel-gfx; +Cc: 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.
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 | 5 +++++
drivers/gpu/drm/i915/i915_reg.h | 3 +++
drivers/gpu/drm/i915/intel_dsi.c | 46 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d07d110..4213b54 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
+ enum transcoder cpu_transcoder;
if (!crtc->active)
return -1;
@@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
vtotal /= 2;
+ cpu_transcoder = crtc->config->cpu_transcoder;
+ if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
+ return bxt_dsi_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 9a73ea0..54582de 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8802,6 +8802,9 @@ enum skl_power_gate {
#define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
#define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
+#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
+#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
+
/* BXT MIPI clock controls */
#define BXT_MAX_VAR_OUTPUT_KHZ 39500
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2a0f5d3..d145ba4 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct drm_connector *connector)
return 1;
}
+/*
+ * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ u32 vrefresh = crtc->base.mode.vrefresh;
+ u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
+ uint_fixed_16_16_t ulScanlineTime;
+
+ /*
+ * This field provides read back of the display
+ * pipe frame time stamp. The time stamp value
+ * is sampled at every start of vertical blank.
+ */
+ ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
+
+ /*
+ * The TIMESTAMP_CTR register has the current
+ * time stamp value.
+ */
+ ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
+
+ /* The PORT for DSI will always be 0 since
+ * isolated PORTC cannot be enabled for Gen9
+ * DSI. Hence using PORT_A i.e 0 to extract
+ * the VTOTAL value.
+ */
+ vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
+ WARN_ON(!vtotal);
+ if (!vtotal)
+ return ulScanlineNo2;
+
+ ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
+ ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
+ ulScanlineTime);
+ ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
+
+ return ulScanlineNo2;
+}
+
static void intel_dsi_connector_destroy(struct drm_connector *connector)
{
struct intel_connector *intel_connector = to_intel_connector(connector);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 13:48 Vidya Srinivas
@ 2017-09-08 14:47 ` Ville Syrjälä
2017-09-08 14:55 ` Ville Syrjälä
2017-09-11 13:04 ` Shankar, Uma
0 siblings, 2 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-08 14:47 UTC (permalink / raw)
To: Vidya Srinivas; +Cc: intel-gfx
On Fri, Sep 08, 2017 at 07:18:55PM +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.
>
> 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 | 5 +++++
> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> drivers/gpu/drm/i915/intel_dsi.c | 46 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d07d110..4213b54 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> + enum transcoder cpu_transcoder;
>
> if (!crtc->active)
> return -1;
> @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> vtotal /= 2;
>
> + cpu_transcoder = crtc->config->cpu_transcoder;
Humm. Would be nice to be able to do this without adding more
crtc->config uses. We're pretty much trying to get rid of that guy.
> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> + return bxt_dsi_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 9a73ea0..54582de 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>
> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
Please add proper parametrized define that works for all pipes.
> +
> /* BXT MIPI clock controls */
> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 2a0f5d3..d145ba4 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct drm_connector *connector)
> return 1;
> }
>
> +/*
> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + u32 vrefresh = crtc->base.mode.vrefresh;
> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
Please get rid of the hungarian notation.
> + uint_fixed_16_16_t ulScanlineTime;
> +
> + /*
> + * This field provides read back of the display
> + * pipe frame time stamp. The time stamp value
> + * is sampled at every start of vertical blank.
> + */
> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
> +
> + /*
> + * The TIMESTAMP_CTR register has the current
> + * time stamp value.
> + */
> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
> +
> + /* The PORT for DSI will always be 0 since
> + * isolated PORTC cannot be enabled for Gen9
> + * DSI. Hence using PORT_A i.e 0 to extract
> + * the VTOTAL value.
> + */
> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
This value can be dug out from the hwmode.
> + WARN_ON(!vtotal);
> + if (!vtotal)
> + return ulScanlineNo2;
> +
> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
> + ulScanlineTime);
Something like:
scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
1000 * crtc_htotal);
> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
I think that would have to be something like:
return (scanline + vblank_start) % vtotal;
All in all this looks like a pretty decent approach to the DSI problem.
One concern here is rounding issues and inaccuracies in our
crtc_clock. But since the frame timestamp is sampled at vblank start I
guess we can't accidentally get an answer that's earlier than
vblank_start as long as we really passed vblank start already. That
should make this at least suitable for vblank timestamps. And for the
atomic evade, I guess if we clamp our the scanline before the
+vblank_start such that it never reaches vtotal, we can't be sure that
our vblank evade never indicates that we already reached the start of
vblank prematurely.
So maybe something like:
scaline = div_u64(...);
scanline = min(scanline, vtotal - 1);
return (scanline + vblank_start) % vtotal;
At least that's my thinking atm. Feel free to rip my reasoning to shreds
if you think I'm totally wrong here.
> +
> + return ulScanlineNo2;
> +}
> +
> static void intel_dsi_connector_destroy(struct drm_connector *connector)
> {
> struct intel_connector *intel_connector = to_intel_connector(connector);
> --
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 14:47 ` Ville Syrjälä
@ 2017-09-08 14:55 ` Ville Syrjälä
2017-09-08 15:25 ` Saarinen, Jani
2017-09-08 19:45 ` Daniel Vetter
2017-09-11 13:04 ` Shankar, Uma
1 sibling, 2 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-08 14:55 UTC (permalink / raw)
To: Vidya Srinivas; +Cc: intel-gfx
On Fri, Sep 08, 2017 at 05:47:59PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 08, 2017 at 07:18:55PM +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.
> >
> > 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 | 5 +++++
> > drivers/gpu/drm/i915/i915_reg.h | 3 +++
> > drivers/gpu/drm/i915/intel_dsi.c | 46 ++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d07d110..4213b54 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> > + enum transcoder cpu_transcoder;
> >
> > if (!crtc->active)
> > return -1;
> > @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> > if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > vtotal /= 2;
> >
> > + cpu_transcoder = crtc->config->cpu_transcoder;
>
> Humm. Would be nice to be able to do this without adding more
> crtc->config uses. We're pretty much trying to get rid of that guy.
>
> > + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> > + return bxt_dsi_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 9a73ea0..54582de 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> > #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> > #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> >
> > +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> > +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>
> Please add proper parametrized define that works for all pipes.
Oh, and these shouldn't be called BXT_something. I don't recall when
they got added to the hardware, but I'm pretty sure it was way before
BXT came out.
Another thought that just occurred to me: Maybe we could use these
timestamps as a workaround for the DDI "scanline reads as 0 at the
wrong time" problem. What we could do is check of the scanline counter
reads as 0, and if it does we could switch over to checking the
timestamps instead. Not sure if we should just do the full timestamp
based scanline read like you do here, or we could just check that if the
timestamps look like they're close to vblank_start we just return
vblank_start-1. This could then remove the obnoxious retry loop from the
scanline counter read.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 14:55 ` Ville Syrjälä
@ 2017-09-08 15:25 ` Saarinen, Jani
2017-09-08 19:45 ` Daniel Vetter
1 sibling, 0 replies; 27+ messages in thread
From: Saarinen, Jani @ 2017-09-08 15:25 UTC (permalink / raw)
To: Srinivas, Vidya; +Cc: intel-gfx
HI,
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Ville Syrjälä
> Sent: perjantai 8. syyskuuta 2017 17.55
> To: Srinivas, Vidya <vidya.srinivas@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>
> Another thought that just occurred to me: Maybe we could use these
> timestamps as a workaround for the DDI "scanline reads as 0 at the wrong
> time" problem. What we could do is check of the scanline counter reads as 0,
> and if it does we could switch over to checking the timestamps instead. Not
> sure if we should just do the full timestamp based scanline read like you do
> here, or we could just check that if the timestamps look like they're close to
> vblank_start we just return vblank_start-1. This could then remove the
> obnoxious retry loop from the scanline counter read.
>
Also please use trybot also as dsi system not in pw runs due to known issue.
> --
> Ville Syrjälä
> Intel OTC
Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 14:55 ` Ville Syrjälä
2017-09-08 15:25 ` Saarinen, Jani
@ 2017-09-08 19:45 ` Daniel Vetter
2017-09-08 19:55 ` Chris Wilson
2017-09-11 13:19 ` Shankar, Uma
1 sibling, 2 replies; 27+ messages in thread
From: Daniel Vetter @ 2017-09-08 19:45 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Vidya Srinivas
On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 08, 2017 at 05:47:59PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 08, 2017 at 07:18:55PM +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.
> > >
> > > 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 | 5 +++++
> > > drivers/gpu/drm/i915/i915_reg.h | 3 +++
> > > drivers/gpu/drm/i915/intel_dsi.c | 46 ++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index d07d110..4213b54 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> > > + enum transcoder cpu_transcoder;
> > >
> > > if (!crtc->active)
> > > return -1;
> > > @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> > > if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > vtotal /= 2;
> > >
> > > + cpu_transcoder = crtc->config->cpu_transcoder;
> >
> > Humm. Would be nice to be able to do this without adding more
> > crtc->config uses. We're pretty much trying to get rid of that guy.
> >
> > > + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> > > + return bxt_dsi_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 9a73ea0..54582de 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> > > #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> > > #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> > >
> > > +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> > > +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
> >
> > Please add proper parametrized define that works for all pipes.
>
> Oh, and these shouldn't be called BXT_something. I don't recall when
> they got added to the hardware, but I'm pretty sure it was way before
> BXT came out.
gen5 or maybe gen4.5 iirc.
> Another thought that just occurred to me: Maybe we could use these
> timestamps as a workaround for the DDI "scanline reads as 0 at the
> wrong time" problem. What we could do is check of the scanline counter
> reads as 0, and if it does we could switch over to checking the
> timestamps instead. Not sure if we should just do the full timestamp
> based scanline read like you do here, or we could just check that if the
> timestamps look like they're close to vblank_start we just return
> vblank_start-1. This could then remove the obnoxious retry loop from the
> scanline counter read.
Another concern I have on this is timeframe jitter. If the vblank
timestamp stuff isnt' perfectly accurately spaced, or we have a mismatch
in clocks, then we might think there's still plenty of time before vblank
while we're already racing.
Just a bit of testing won't catch that all that easily unfortunately, and
I'm not sure we have any good igts to stress-test this stuff. I'd advocate
we're playing it defensive and increase the vblank evasion window for this
trick quite a bit to stay on the safe side (maybe 1 full ms or so). Or
much, much better testing.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 19:45 ` Daniel Vetter
@ 2017-09-08 19:55 ` Chris Wilson
2017-09-11 8:52 ` Maarten Lankhorst
2017-09-11 13:19 ` Shankar, Uma
1 sibling, 1 reply; 27+ messages in thread
From: Chris Wilson @ 2017-09-08 19:55 UTC (permalink / raw)
To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx, Vidya Srinivas
Quoting Daniel Vetter (2017-09-08 20:45:11)
> On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
> > Another thought that just occurred to me: Maybe we could use these
> > timestamps as a workaround for the DDI "scanline reads as 0 at the
> > wrong time" problem. What we could do is check of the scanline counter
> > reads as 0, and if it does we could switch over to checking the
> > timestamps instead. Not sure if we should just do the full timestamp
> > based scanline read like you do here, or we could just check that if the
> > timestamps look like they're close to vblank_start we just return
> > vblank_start-1. This could then remove the obnoxious retry loop from the
> > scanline counter read.
>
> Another concern I have on this is timeframe jitter. If the vblank
> timestamp stuff isnt' perfectly accurately spaced, or we have a mismatch
> in clocks, then we might think there's still plenty of time before vblank
> while we're already racing.
You are sort of getting to the point where you just use the ART cpu
clock, using an ewma seeded with the vrefresh and fed with the vblank
intervals as an estimator for how long you have left to the next vblank.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 19:55 ` Chris Wilson
@ 2017-09-11 8:52 ` Maarten Lankhorst
2017-09-11 12:21 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Maarten Lankhorst @ 2017-09-11 8:52 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Ville Syrjälä
Cc: intel-gfx, Vidya Srinivas
Op 08-09-17 om 21:55 schreef Chris Wilson:
> Quoting Daniel Vetter (2017-09-08 20:45:11)
>> On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
>>> Another thought that just occurred to me: Maybe we could use these
>>> timestamps as a workaround for the DDI "scanline reads as 0 at the
>>> wrong time" problem. What we could do is check of the scanline counter
>>> reads as 0, and if it does we could switch over to checking the
>>> timestamps instead. Not sure if we should just do the full timestamp
>>> based scanline read like you do here, or we could just check that if the
>>> timestamps look like they're close to vblank_start we just return
>>> vblank_start-1. This could then remove the obnoxious retry loop from the
>>> scanline counter read.
>> Another concern I have on this is timeframe jitter. If the vblank
>> timestamp stuff isnt' perfectly accurately spaced, or we have a mismatch
>> in clocks, then we might think there's still plenty of time before vblank
>> while we're already racing.
> You are sort of getting to the point where you just use the ART cpu
> clock, using an ewma seeded with the vrefresh and fed with the vblank
> intervals as an estimator for how long you have left to the next vblank.
Agreed, this seems to be the case.. In which case can't we use that for all of DDI to get a
better than scanline resolution for last vblank time by replacing the get_vblank_timestamp hook?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-11 8:52 ` Maarten Lankhorst
@ 2017-09-11 12:21 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-11 12:21 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, Vidya Srinivas
On Mon, Sep 11, 2017 at 10:52:27AM +0200, Maarten Lankhorst wrote:
> Op 08-09-17 om 21:55 schreef Chris Wilson:
> > Quoting Daniel Vetter (2017-09-08 20:45:11)
> >> On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
> >>> Another thought that just occurred to me: Maybe we could use these
> >>> timestamps as a workaround for the DDI "scanline reads as 0 at the
> >>> wrong time" problem. What we could do is check of the scanline counter
> >>> reads as 0, and if it does we could switch over to checking the
> >>> timestamps instead. Not sure if we should just do the full timestamp
> >>> based scanline read like you do here, or we could just check that if the
> >>> timestamps look like they're close to vblank_start we just return
> >>> vblank_start-1. This could then remove the obnoxious retry loop from the
> >>> scanline counter read.
> >> Another concern I have on this is timeframe jitter. If the vblank
> >> timestamp stuff isnt' perfectly accurately spaced, or we have a mismatch
> >> in clocks, then we might think there's still plenty of time before vblank
> >> while we're already racing.
> > You are sort of getting to the point where you just use the ART cpu
> > clock, using an ewma seeded with the vrefresh and fed with the vblank
> > intervals as an estimator for how long you have left to the next vblank.
> Agreed, this seems to be the case.. In which case can't we use that for all of DDI to get a
> better than scanline resolution for last vblank time by replacing the get_vblank_timestamp hook?
Like I said that should be doable with just a simple timestamp check to
fix up the bogus 0.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 19:45 ` Daniel Vetter
2017-09-08 19:55 ` Chris Wilson
@ 2017-09-11 13:19 ` Shankar, Uma
2017-09-11 18:21 ` Ville Syrjälä
1 sibling, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2017-09-11 13:19 UTC (permalink / raw)
To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Daniel Vetter
>Sent: Saturday, September 9, 2017 1:15 AM
>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] drm/i915: Enable scanline read for gen9 dsi
>
>On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
>> On Fri, Sep 08, 2017 at 05:47:59PM +0300, Ville Syrjälä wrote:
>> > On Fri, Sep 08, 2017 at 07:18:55PM +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.
>> > >
>> > > 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 | 5 +++++
>> > > drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> > > drivers/gpu/drm/i915/intel_dsi.c | 46
>> > > ++++++++++++++++++++++++++++++++++++++++
>> > > 4 files changed, 56 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > > b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
>> > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
>> > > + enum transcoder cpu_transcoder;
>> > >
>> > > if (!crtc->active)
>> > > return -1;
>> > > @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct
>intel_crtc *crtc)
>> > > if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> > > vtotal /= 2;
>> > >
>> > > + cpu_transcoder = crtc->config->cpu_transcoder;
>> >
>> > Humm. Would be nice to be able to do this without adding more
>> > crtc->config uses. We're pretty much trying to get rid of that guy.
>> >
>> > > + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>> > > + return bxt_dsi_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 9a73ea0..54582de 100644
>> > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > > @@ -8802,6 +8802,9 @@ enum skl_power_gate {
>> > > #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
>> > > #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>> > >
>> > > +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> > > +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>> >
>> > Please add proper parametrized define that works for all pipes.
>>
>> Oh, and these shouldn't be called BXT_something. I don't recall when
>> they got added to the hardware, but I'm pretty sure it was way before
>> BXT came out.
>
>gen5 or maybe gen4.5 iirc.
>
As per spec, it says BDW+. Will rename them using BDW as prefix.
>> Another thought that just occurred to me: Maybe we could use these
>> timestamps as a workaround for the DDI "scanline reads as 0 at the
>> wrong time" problem. What we could do is check of the scanline counter
>> reads as 0, and if it does we could switch over to checking the
>> timestamps instead. Not sure if we should just do the full timestamp
>> based scanline read like you do here, or we could just check that if
>> the timestamps look like they're close to vblank_start we just return
>> vblank_start-1. This could then remove the obnoxious retry loop from
>> the scanline counter read.
>
>Another concern I have on this is timeframe jitter. If the vblank timestamp stuff
>isnt' perfectly accurately spaced, or we have a mismatch in clocks, then we might
>think there's still plenty of time before vblank while we're already racing.
>
>Just a bit of testing won't catch that all that easily unfortunately, and I'm not sure
>we have any good igts to stress-test this stuff. I'd advocate we're playing it
>defensive and increase the vblank evasion window for this trick quite a bit to stay
>on the safe side (maybe 1 full ms or so). Or much, much better testing.
We tried to use kms_plane_multiple test from IGT to validate this. Atomic update failure
can easily be reproduced using this. With the above approach, we are not seeing those
failures. Also to try to check for traditional method of detecting scanline (using Pipe scanline)
which works for DDI interfaces. We tried to compare the results from both ways, readback from
this register and the calculated one from timestamps on HDMI. The results were pretty similar with a delta of
1. We will try to do more experiments and stress test to get more confidence on the timestamp method.
But till now, it looks pretty promising.
If we increase evade time, it may have an adverse impact on fps. So if scanline reported gives promising
data (similar to what pipe scanline register returns), I think we should stick to that approach.
Regards,
Uma Shankar
>-Daniel
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>_______________________________________________
>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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-11 13:19 ` Shankar, Uma
@ 2017-09-11 18:21 ` Ville Syrjälä
2017-09-12 9:32 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-11 18:21 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Srinivas, Vidya
On Mon, Sep 11, 2017 at 01:19:15PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Daniel Vetter
> >Sent: Saturday, September 9, 2017 1:15 AM
> >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] drm/i915: Enable scanline read for gen9 dsi
> >
> >On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
> >> On Fri, Sep 08, 2017 at 05:47:59PM +0300, Ville Syrjälä wrote:
> >> > On Fri, Sep 08, 2017 at 07:18:55PM +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.
> >> > >
> >> > > 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 | 5 +++++
> >> > > drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >> > > drivers/gpu/drm/i915/intel_dsi.c | 46
> >> > > ++++++++++++++++++++++++++++++++++++++++
> >> > > 4 files changed, 56 insertions(+)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > > b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
> >> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > > @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> >> > > + enum transcoder cpu_transcoder;
> >> > >
> >> > > if (!crtc->active)
> >> > > return -1;
> >> > > @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct
> >intel_crtc *crtc)
> >> > > if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> > > vtotal /= 2;
> >> > >
> >> > > + cpu_transcoder = crtc->config->cpu_transcoder;
> >> >
> >> > Humm. Would be nice to be able to do this without adding more
> >> > crtc->config uses. We're pretty much trying to get rid of that guy.
> >> >
> >> > > + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> >> > > + return bxt_dsi_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 9a73ea0..54582de 100644
> >> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > > @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> >> > > #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> >> > > #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> >> > >
> >> > > +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> >> > > +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
> >> >
> >> > Please add proper parametrized define that works for all pipes.
> >>
> >> Oh, and these shouldn't be called BXT_something. I don't recall when
> >> they got added to the hardware, but I'm pretty sure it was way before
> >> BXT came out.
> >
> >gen5 or maybe gen4.5 iirc.
> >
>
> As per spec, it says BDW+. Will rename them using BDW as prefix.
You're not looking at the right spec then. Looks like ctg/elk is
the right answer indeed. The gen4 spec is a bit confused in that it
doesn't correctly state whether some of the regs apply to gen4 or
ctg/elk. But testing on actual gen4 hardware gives me 0s from all the
pipe timestamp registers, whereas elk gives sane looking value. The
TIMESTAMP register did exist on gen4 already.
There have been some changes in the TIMESTAMP register(s) throughout
the years though. I'll try to summarize below:
bw/cl: TIMESTAMP/0x2358. 64bit register that increments every 16 hclks
ctg/elk: TIMESTAMP/0x2358. 64bit register where the upper 32 bits increment
every 1.024us, the lower part has more 12 bits which means the whole
value increments every 1/4 ns
ilk/snb: TIMESTAMP/0x2358 "64bit" register where the upprt 32 bits increment
every 1 us. Lower part is documented as MBZ. The upprt part is
aliased as TIMESTAMP_HI high at offset 0x70070
ivb+: TIMESTAMP_HI got renamed to TIMESTAMP_CTR and moved to 0x44070
The pipe flip/frame timestamp registers seem to have remained
unchanged ever since ctg/elk.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-11 18:21 ` Ville Syrjälä
@ 2017-09-12 9:32 ` Shankar, Uma
0 siblings, 0 replies; 27+ messages in thread
From: Shankar, Uma @ 2017-09-12 9:32 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, September 11, 2017 11:51 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Daniel Vetter <daniel@ffwll.ch>; intel-gfx@lists.freedesktop.org; Srinivas,
>Vidya <vidya.srinivas@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Mon, Sep 11, 2017 at 01:19:15PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>> >Behalf Of Daniel Vetter
>> >Sent: Saturday, September 9, 2017 1:15 AM
>> >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] drm/i915: Enable scanline read for
>> >gen9 dsi
>> >
>> >On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
>> >> On Fri, Sep 08, 2017 at 05:47:59PM +0300, Ville Syrjälä wrote:
>> >> > On Fri, Sep 08, 2017 at 07:18:55PM +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.
>> >> > >
>> >> > > 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 | 5 +++++
>> >> > > drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> >> > > drivers/gpu/drm/i915/intel_dsi.c | 46
>> >> > > ++++++++++++++++++++++++++++++++++++++++
>> >> > > 4 files changed, 56 insertions(+)
>> >> > >
>> >> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> > > b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
>> >> > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> > > @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
>> >> > > + enum transcoder cpu_transcoder;
>> >> > >
>> >> > > if (!crtc->active)
>> >> > > return -1;
>> >> > > @@ -792,6 +793,10 @@ static int
>> >> > > __intel_get_crtc_scanline(struct
>> >intel_crtc *crtc)
>> >> > > if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> >> > > vtotal /= 2;
>> >> > >
>> >> > > + cpu_transcoder = crtc->config->cpu_transcoder;
>> >> >
>> >> > Humm. Would be nice to be able to do this without adding more
>> >> > crtc->config uses. We're pretty much trying to get rid of that guy.
>> >> >
>> >> > > + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>> >> > > + return bxt_dsi_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 9a73ea0..54582de 100644
>> >> > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> > > @@ -8802,6 +8802,9 @@ enum skl_power_gate {
>> >> > > #define MIPIO_TXESC_CLK_DIV2
> _MMIO(0x160008)
>> >> > > #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>> >> > >
>> >> > > +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> >> > > +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>> >> >
>> >> > Please add proper parametrized define that works for all pipes.
>> >>
>> >> Oh, and these shouldn't be called BXT_something. I don't recall
>> >> when they got added to the hardware, but I'm pretty sure it was way
>> >> before BXT came out.
>> >
>> >gen5 or maybe gen4.5 iirc.
>> >
>>
>> As per spec, it says BDW+. Will rename them using BDW as prefix.
>
>You're not looking at the right spec then. Looks like ctg/elk is the right answer
>indeed. The gen4 spec is a bit confused in that it doesn't correctly state whether
>some of the regs apply to gen4 or ctg/elk. But testing on actual gen4 hardware
>gives me 0s from all the pipe timestamp registers, whereas elk gives sane looking
>value. The TIMESTAMP register did exist on gen4 already.
>
>There have been some changes in the TIMESTAMP register(s) throughout the
>years though. I'll try to summarize below:
>
>bw/cl: TIMESTAMP/0x2358. 64bit register that increments every 16 hclks
>ctg/elk: TIMESTAMP/0x2358. 64bit register where the upper 32 bits increment
> every 1.024us, the lower part has more 12 bits which means the whole
> value increments every 1/4 ns
>ilk/snb: TIMESTAMP/0x2358 "64bit" register where the upprt 32 bits increment
> every 1 us. Lower part is documented as MBZ. The upprt part is
> aliased as TIMESTAMP_HI high at offset 0x70070
>ivb+: TIMESTAMP_HI got renamed to TIMESTAMP_CTR and moved to 0x44070
>
>The pipe flip/frame timestamp registers seem to have remained unchanged ever
>since ctg/elk.
>
Thanks Ville for this info. Will update the Current Timestamp register for Gen4 and Gen7 (declare
as separate macros). Also will define Frame Timestamp generically.
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-08 14:47 ` Ville Syrjälä
2017-09-08 14:55 ` Ville Syrjälä
@ 2017-09-11 13:04 ` Shankar, Uma
2017-09-11 17:50 ` Ville Syrjälä
1 sibling, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2017-09-11 13:04 UTC (permalink / raw)
To: Ville Syrjälä, Srinivas, Vidya; +Cc: intel-gfx
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
>
>On Fri, Sep 08, 2017 at 07:18:55PM +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.
>>
>> 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 | 5 +++++
>> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> drivers/gpu/drm/i915/intel_dsi.c | 46
>> ++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
>> + enum transcoder cpu_transcoder;
>>
>> if (!crtc->active)
>> return -1;
>> @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc
>*crtc)
>> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> vtotal /= 2;
>>
>> + cpu_transcoder = crtc->config->cpu_transcoder;
>
>Humm. Would be nice to be able to do this without adding more
>crtc->config uses. We're pretty much trying to get rid of that guy.
>
Will try to find an alternate way to do this.
>> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>> + return bxt_dsi_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 9a73ea0..54582de 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
>> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
>> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>>
>> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>
>Please add proper parametrized define that works for all pipes.
>
Will add that.
>> +
>> /* BXT MIPI clock controls */
>> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 2a0f5d3..d145ba4 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
>drm_connector *connector)
>> return 1;
>> }
>>
>> +/*
>> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + u32 vrefresh = crtc->base.mode.vrefresh;
>> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>
>Please get rid of the hungarian notation.
>
Yes, will fix this.
>> + uint_fixed_16_16_t ulScanlineTime;
>> +
>> + /*
>> + * This field provides read back of the display
>> + * pipe frame time stamp. The time stamp value
>> + * is sampled at every start of vertical blank.
>> + */
>> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>> +
>> + /*
>> + * The TIMESTAMP_CTR register has the current
>> + * time stamp value.
>> + */
>> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>> +
>> + /* The PORT for DSI will always be 0 since
>> + * isolated PORTC cannot be enabled for Gen9
>> + * DSI. Hence using PORT_A i.e 0 to extract
>> + * the VTOTAL value.
>> + */
>> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>
>This value can be dug out from the hwmode.
>
Yes, will get it from hwmode and drop this change.
>> + WARN_ON(!vtotal);
>> + if (!vtotal)
>> + return ulScanlineNo2;
>> +
>> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
>> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
>> + ulScanlineTime);
>
>Something like:
>scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
> 1000 * crtc_htotal);
>
>> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>
>I think that would have to be something like:
>return (scanline + vblank_start) % vtotal;
>
Yes you are right. It should be vblank_start. Will fix this.
>All in all this looks like a pretty decent approach to the DSI problem.
>
>One concern here is rounding issues and inaccuracies in our crtc_clock. But since
>the frame timestamp is sampled at vblank start I guess we can't accidentally get
>an answer that's earlier than vblank_start as long as we really passed vblank start
>already. That should make this at least suitable for vblank timestamps.
I also feel the same, this situation should never occur.
>And for
>the atomic evade, I guess if we clamp our the scanline before the
>+vblank_start such that it never reaches vtotal, we can't be sure that
>our vblank evade never indicates that we already reached the start of vblank
>prematurely.
>
>So maybe something like:
>scaline = div_u64(...);
>scanline = min(scanline, vtotal - 1);
I am not sure if the value of scanline returned can ever be greater than vtotal -1.
But we can have a check just to be safe. Not sure if I fully got your point here.
>return (scanline + vblank_start) % vtotal;
>
>At least that's my thinking atm. Feel free to rip my reasoning to shreds if you think
>I'm totally wrong here.
>
One more thing we missed is, that the current timestamp is just a 32 bit register value.
It can overflow and wrap around. So a situation can come, where current timestamp will
be less than prev timestamp (read from frame time stamp reg). We need to handle that
situation as well. Will fix that in the next version and resend.
Thanks Ville for your valuable review comments.
Regards,
Uma Shankar
>
>> +
>> + return ulScanlineNo2;
>> +}
>> +
>> static void intel_dsi_connector_destroy(struct drm_connector
>> *connector) {
>> struct intel_connector *intel_connector =
>> to_intel_connector(connector);
>> --
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-11 13:04 ` Shankar, Uma
@ 2017-09-11 17:50 ` Ville Syrjälä
2017-09-12 9:50 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-11 17:50 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Srinivas, Vidya
On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
> >
> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
> >>
> >> 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 | 5 +++++
> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >> drivers/gpu/drm/i915/intel_dsi.c | 46
> >> ++++++++++++++++++++++++++++++++++++++++
> >> 4 files changed, 56 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> >> + enum transcoder cpu_transcoder;
> >>
> >> if (!crtc->active)
> >> return -1;
> >> @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc
> >*crtc)
> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> vtotal /= 2;
> >>
> >> + cpu_transcoder = crtc->config->cpu_transcoder;
> >
> >Humm. Would be nice to be able to do this without adding more
> >crtc->config uses. We're pretty much trying to get rid of that guy.
> >
>
> Will try to find an alternate way to do this.
>
> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> >> + return bxt_dsi_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 9a73ea0..54582de 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> >> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> >>
> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
> >
> >Please add proper parametrized define that works for all pipes.
> >
>
> Will add that.
>
> >> +
> >> /* BXT MIPI clock controls */
> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> >> b/drivers/gpu/drm/i915/intel_dsi.c
> >> index 2a0f5d3..d145ba4 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
> >drm_connector *connector)
> >> return 1;
> >> }
> >>
> >> +/*
> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
> >> + struct drm_device *dev = crtc->base.dev;
> >> + struct drm_i915_private *dev_priv = to_i915(dev);
> >> + u32 vrefresh = crtc->base.mode.vrefresh;
> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
> >
> >Please get rid of the hungarian notation.
> >
>
> Yes, will fix this.
>
> >> + uint_fixed_16_16_t ulScanlineTime;
> >> +
> >> + /*
> >> + * This field provides read back of the display
> >> + * pipe frame time stamp. The time stamp value
> >> + * is sampled at every start of vertical blank.
> >> + */
> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
> >> +
> >> + /*
> >> + * The TIMESTAMP_CTR register has the current
> >> + * time stamp value.
> >> + */
> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
> >> +
> >> + /* The PORT for DSI will always be 0 since
> >> + * isolated PORTC cannot be enabled for Gen9
> >> + * DSI. Hence using PORT_A i.e 0 to extract
> >> + * the VTOTAL value.
> >> + */
> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
> >
> >This value can be dug out from the hwmode.
> >
>
> Yes, will get it from hwmode and drop this change.
>
> >> + WARN_ON(!vtotal);
> >> + if (!vtotal)
> >> + return ulScanlineNo2;
> >> +
> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
> >> + ulScanlineTime);
> >
> >Something like:
> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
> > 1000 * crtc_htotal);
> >
> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
> >
> >I think that would have to be something like:
> >return (scanline + vblank_start) % vtotal;
> >
>
> Yes you are right. It should be vblank_start. Will fix this.
>
> >All in all this looks like a pretty decent approach to the DSI problem.
> >
> >One concern here is rounding issues and inaccuracies in our crtc_clock. But since
> >the frame timestamp is sampled at vblank start I guess we can't accidentally get
> >an answer that's earlier than vblank_start as long as we really passed vblank start
> >already. That should make this at least suitable for vblank timestamps.
>
> I also feel the same, this situation should never occur.
>
> >And for
> >the atomic evade, I guess if we clamp our the scanline before the
> >+vblank_start such that it never reaches vtotal, we can't be sure that
> >our vblank evade never indicates that we already reached the start of vblank
> >prematurely.
> >
> >So maybe something like:
> >scaline = div_u64(...);
> >scanline = min(scanline, vtotal - 1);
>
> I am not sure if the value of scanline returned can ever be greater than vtotal -1.
> But we can have a check just to be safe. Not sure if I fully got your point here.
The point is that the timestamp counter might tick at a slightly faster
rate than we might think. Thus we might end up with more ticks in one
frame than what we calculated as the maximum fom crtc_clock etc. But if
we clamp the value like I suggested then at least we should never get
an answer that tells us we're already past the start of vblank when in
reality we're not.
Of course as Daniel pointed out we might also get into trouble if the
counter ticks slower than expected. That could lead us to think that
we don't need to do the vblank evade when in fact we do.
Oh and there's maybe another race lurking here. We might cross into the
next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR reads. If
that happens we get an answer that's definitely too big for one frame.
I guess we could avoid that particular problem by making sure we really
read PIPE_FRMTMSTMP and TIMESTAMP_CTR during the same frame. Eg.
something like:
do {
prev = PIPE_FRMTMSTMP;
curr = TIMESTAMP_CTR
post = PIPE_FRMTMSTMP
} while (prev != post);
>
> >return (scanline + vblank_start) % vtotal;
> >
> >At least that's my thinking atm. Feel free to rip my reasoning to shreds if you think
> >I'm totally wrong here.
> >
>
> One more thing we missed is, that the current timestamp is just a 32 bit register value.
> It can overflow and wrap around. So a situation can come, where current timestamp will
> be less than prev timestamp (read from frame time stamp reg). We need to handle that
> situation as well. Will fix that in the next version and resend.
Modulo 2^32 math will handle that just fine.
>
> Thanks Ville for your valuable review comments.
>
> Regards,
> Uma Shankar
>
> >
> >> +
> >> + return ulScanlineNo2;
> >> +}
> >> +
> >> static void intel_dsi_connector_destroy(struct drm_connector
> >> *connector) {
> >> struct intel_connector *intel_connector =
> >> to_intel_connector(connector);
> >> --
> >> 1.9.1
> >
> >--
> >Ville Syrjälä
> >Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-11 17:50 ` Ville Syrjälä
@ 2017-09-12 9:50 ` Shankar, Uma
2017-09-12 13:23 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2017-09-12 9:50 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, September 11, 2017 11:20 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx@lists.freedesktop.org;
>Kahola, Mika <mika.kahola@intel.com>; Kamath, Sunil
><sunil.kamath@intel.com>; Konduru, Chandra <chandra.konduru@intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
>> >
>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
>> >>
>> >> 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 | 5 +++++
>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
>> >> ++++++++++++++++++++++++++++++++++++++++
>> >> 4 files changed, 56 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
>> >> + enum transcoder cpu_transcoder;
>> >>
>> >> if (!crtc->active)
>> >> return -1;
>> >> @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct
>> >> intel_crtc
>> >*crtc)
>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> >> vtotal /= 2;
>> >>
>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
>> >
>> >Humm. Would be nice to be able to do this without adding more
>> >crtc->config uses. We're pretty much trying to get rid of that guy.
>> >
>>
>> Will try to find an alternate way to do this.
>>
>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>> >> + return bxt_dsi_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 9a73ea0..54582de 100644
>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
>> >> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>> >>
>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>> >
>> >Please add proper parametrized define that works for all pipes.
>> >
>>
>> Will add that.
>>
>> >> +
>> >> /* BXT MIPI clock controls */
>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> >> b/drivers/gpu/drm/i915/intel_dsi.c
>> >> index 2a0f5d3..d145ba4 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
>> >drm_connector *connector)
>> >> return 1;
>> >> }
>> >>
>> >> +/*
>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>> >> + struct drm_device *dev = crtc->base.dev;
>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>> >
>> >Please get rid of the hungarian notation.
>> >
>>
>> Yes, will fix this.
>>
>> >> + uint_fixed_16_16_t ulScanlineTime;
>> >> +
>> >> + /*
>> >> + * This field provides read back of the display
>> >> + * pipe frame time stamp. The time stamp value
>> >> + * is sampled at every start of vertical blank.
>> >> + */
>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>> >> +
>> >> + /*
>> >> + * The TIMESTAMP_CTR register has the current
>> >> + * time stamp value.
>> >> + */
>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>> >> +
>> >> + /* The PORT for DSI will always be 0 since
>> >> + * isolated PORTC cannot be enabled for Gen9
>> >> + * DSI. Hence using PORT_A i.e 0 to extract
>> >> + * the VTOTAL value.
>> >> + */
>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>> >
>> >This value can be dug out from the hwmode.
>> >
>>
>> Yes, will get it from hwmode and drop this change.
>>
>> >> + WARN_ON(!vtotal);
>> >> + if (!vtotal)
>> >> + return ulScanlineNo2;
>> >> +
>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
>> >> + ulScanlineTime);
>> >
>> >Something like:
>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
>> > 1000 * crtc_htotal);
>> >
>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>> >
>> >I think that would have to be something like:
>> >return (scanline + vblank_start) % vtotal;
>> >
>>
>> Yes you are right. It should be vblank_start. Will fix this.
>>
>> >All in all this looks like a pretty decent approach to the DSI problem.
>> >
>> >One concern here is rounding issues and inaccuracies in our
>> >crtc_clock. But since the frame timestamp is sampled at vblank start
>> >I guess we can't accidentally get an answer that's earlier than
>> >vblank_start as long as we really passed vblank start already. That should
>make this at least suitable for vblank timestamps.
>>
>> I also feel the same, this situation should never occur.
>>
>> >And for
>> >the atomic evade, I guess if we clamp our the scanline before the
>> >+vblank_start such that it never reaches vtotal, we can't be sure
>> >+that
>> >our vblank evade never indicates that we already reached the start of
>> >vblank prematurely.
>> >
>> >So maybe something like:
>> >scaline = div_u64(...);
>> >scanline = min(scanline, vtotal - 1);
>>
>> I am not sure if the value of scanline returned can ever be greater than vtotal -
>1.
>> But we can have a check just to be safe. Not sure if I fully got your point here.
>
>The point is that the timestamp counter might tick at a slightly faster rate than we
>might think. Thus we might end up with more ticks in one frame than what we
>calculated as the maximum fom crtc_clock etc. But if we clamp the value like I
>suggested then at least we should never get an answer that tells us we're already
>past the start of vblank when in reality we're not.
>
>Of course as Daniel pointed out we might also get into trouble if the counter ticks
>slower than expected. That could lead us to think that we don't need to do the
>vblank evade when in fact we do.
>
>Oh and there's maybe another race lurking here. We might cross into the next
>vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR reads. If that
>happens we get an answer that's definitely too big for one frame.
>I guess we could avoid that particular problem by making sure we really read
>PIPE_FRMTMSTMP and TIMESTAMP_CTR during the same frame. Eg.
>something like:
>
>do {
> prev = PIPE_FRMTMSTMP;
> curr = TIMESTAMP_CTR
> post = PIPE_FRMTMSTMP
>} while (prev != post);
>
Got it. Will add this condition to handle the race situation. Thanks for the explanation.
Regards,
Uma Shankar
>
>>
>> >return (scanline + vblank_start) % vtotal;
>> >
>> >At least that's my thinking atm. Feel free to rip my reasoning to
>> >shreds if you think I'm totally wrong here.
>> >
>>
>> One more thing we missed is, that the current timestamp is just a 32 bit register
>value.
>> It can overflow and wrap around. So a situation can come, where
>> current timestamp will be less than prev timestamp (read from frame
>> time stamp reg). We need to handle that situation as well. Will fix that in the
>next version and resend.
>
>Modulo 2^32 math will handle that just fine.
>
>>
>> Thanks Ville for your valuable review comments.
>>
>> Regards,
>> Uma Shankar
>>
>> >
>> >> +
>> >> + return ulScanlineNo2;
>> >> +}
>> >> +
>> >> static void intel_dsi_connector_destroy(struct drm_connector
>> >> *connector) {
>> >> struct intel_connector *intel_connector =
>> >> to_intel_connector(connector);
>> >> --
>> >> 1.9.1
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 9:50 ` Shankar, Uma
@ 2017-09-12 13:23 ` Shankar, Uma
2017-09-12 13:33 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2017-09-12 13:23 UTC (permalink / raw)
To: Shankar, Uma, Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Shankar, Uma
>Sent: Tuesday, September 12, 2017 3:20 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] drm/i915: Enable scanline read for gen9 dsi
>
>
>
>>-----Original Message-----
>>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>>Sent: Monday, September 11, 2017 11:20 PM
>>To: Shankar, Uma <uma.shankar@intel.com>
>>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>;
>>intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>;
>>Kamath, Sunil <sunil.kamath@intel.com>; Konduru, Chandra
>><chandra.konduru@intel.com>
>>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>>
>>On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
>>>
>>>
>>> >-----Original Message-----
>>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>>> >Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
>>> >
>>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
>>> >>
>>> >> 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 | 5 +++++
>>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
>>> >> ++++++++++++++++++++++++++++++++++++++++
>>> >> 4 files changed, 56 insertions(+)
>>> >>
>>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
>>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
>>> >> + enum transcoder cpu_transcoder;
>>> >>
>>> >> if (!crtc->active)
>>> >> return -1;
>>> >> @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct
>>> >> intel_crtc
>>> >*crtc)
>>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>> >> vtotal /= 2;
>>> >>
>>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
>>> >
>>> >Humm. Would be nice to be able to do this without adding more
>>> >crtc->config uses. We're pretty much trying to get rid of that guy.
>>> >
>>>
>>> Will try to find an alternate way to do this.
>>>
>>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>>> >> + return bxt_dsi_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 9a73ea0..54582de 100644
>>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
>>> >> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
>>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>>> >>
>>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>>> >
>>> >Please add proper parametrized define that works for all pipes.
>>> >
>>>
>>> Will add that.
>>>
>>> >> +
>>> >> /* BXT MIPI clock controls */
>>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>>> >>
>>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>>> >> b/drivers/gpu/drm/i915/intel_dsi.c
>>> >> index 2a0f5d3..d145ba4 100644
>>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
>>> >drm_connector *connector)
>>> >> return 1;
>>> >> }
>>> >>
>>> >> +/*
>>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>>> >> + struct drm_device *dev = crtc->base.dev;
>>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
>>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>>> >
>>> >Please get rid of the hungarian notation.
>>> >
>>>
>>> Yes, will fix this.
>>>
>>> >> + uint_fixed_16_16_t ulScanlineTime;
>>> >> +
>>> >> + /*
>>> >> + * This field provides read back of the display
>>> >> + * pipe frame time stamp. The time stamp value
>>> >> + * is sampled at every start of vertical blank.
>>> >> + */
>>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>>> >> +
>>> >> + /*
>>> >> + * The TIMESTAMP_CTR register has the current
>>> >> + * time stamp value.
>>> >> + */
>>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>>> >> +
>>> >> + /* The PORT for DSI will always be 0 since
>>> >> + * isolated PORTC cannot be enabled for Gen9
>>> >> + * DSI. Hence using PORT_A i.e 0 to extract
>>> >> + * the VTOTAL value.
>>> >> + */
>>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>>> >
>>> >This value can be dug out from the hwmode.
>>> >
>>>
>>> Yes, will get it from hwmode and drop this change.
>>>
>>> >> + WARN_ON(!vtotal);
>>> >> + if (!vtotal)
>>> >> + return ulScanlineNo2;
>>> >> +
>>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
>>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
>>> >> + ulScanlineTime);
>>> >
>>> >Something like:
>>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
>>> > 1000 * crtc_htotal);
>>> >
>>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>>> >
>>> >I think that would have to be something like:
>>> >return (scanline + vblank_start) % vtotal;
>>> >
>>>
>>> Yes you are right. It should be vblank_start. Will fix this.
>>>
>>> >All in all this looks like a pretty decent approach to the DSI problem.
>>> >
>>> >One concern here is rounding issues and inaccuracies in our
>>> >crtc_clock. But since the frame timestamp is sampled at vblank start
>>> >I guess we can't accidentally get an answer that's earlier than
>>> >vblank_start as long as we really passed vblank start already. That
>>> >should
>>make this at least suitable for vblank timestamps.
>>>
>>> I also feel the same, this situation should never occur.
>>>
>>> >And for
>>> >the atomic evade, I guess if we clamp our the scanline before the
>>> >+vblank_start such that it never reaches vtotal, we can't be sure
>>> >+that
>>> >our vblank evade never indicates that we already reached the start
>>> >of vblank prematurely.
>>> >
>>> >So maybe something like:
>>> >scaline = div_u64(...);
>>> >scanline = min(scanline, vtotal - 1);
>>>
>>> I am not sure if the value of scanline returned can ever be greater
>>> than vtotal -
>>1.
>>> But we can have a check just to be safe. Not sure if I fully got your point here.
>>
>>The point is that the timestamp counter might tick at a slightly faster
>>rate than we might think. Thus we might end up with more ticks in one
>>frame than what we calculated as the maximum fom crtc_clock etc. But if
>>we clamp the value like I suggested then at least we should never get
>>an answer that tells us we're already past the start of vblank when in reality
>we're not.
>>
>>Of course as Daniel pointed out we might also get into trouble if the
>>counter ticks slower than expected. That could lead us to think that we
>>don't need to do the vblank evade when in fact we do.
>>
Hi Ville,
We tried to test with this condition and are calculating wrong scanlines.
For ex:
[ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534, crtc_vtotal-1 = 1211, min of two = 1211
This causes calculated value to be different from PIPE SCANLINE value read from register.
But if we keep scanline and take the modulo with vtotal after adding the vblank_start (not taking min with vtotal -1),
we are getting scanline equal to (delta of 1 all the time ) what the PIPE SCANLINE register returns for HDMI. We can
use HDMI as a reference to validate if timestamp based calculation aligns to h/w values returned by
Pipe scanline register. So if we do like below:
scanline = div_u64(...);
return (scanline + vblank_start) % vtotal;
It works perfectly fine. Tested it with "kms_plane_multiple" and almost getting the same result all the time with
no atomic update failures. I am not sure on other platforms, but BXT/APL its working fine. Without these changes,
issue can easily be reproduced using this IGT test.
Will send the updated patch, please have a look once.
Thanks & Regards,
Uma Shankar
>>Oh and there's maybe another race lurking here. We might cross into the
>>next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR reads. If
>>that happens we get an answer that's definitely too big for one frame.
>>I guess we could avoid that particular problem by making sure we really
>>read PIPE_FRMTMSTMP and TIMESTAMP_CTR during the same frame. Eg.
>>something like:
>>
>>do {
>> prev = PIPE_FRMTMSTMP;
>> curr = TIMESTAMP_CTR
>> post = PIPE_FRMTMSTMP
>>} while (prev != post);
>>
>
>Got it. Will add this condition to handle the race situation. Thanks for the
>explanation.
>
>Regards,
>Uma Shankar
>
>>
>>>
>>> >return (scanline + vblank_start) % vtotal;
>>> >
>>> >At least that's my thinking atm. Feel free to rip my reasoning to
>>> >shreds if you think I'm totally wrong here.
>>> >
>>>
>>> One more thing we missed is, that the current timestamp is just a 32
>>> bit register
>>value.
>>> It can overflow and wrap around. So a situation can come, where
>>> current timestamp will be less than prev timestamp (read from frame
>>> time stamp reg). We need to handle that situation as well. Will fix
>>> that in the
>>next version and resend.
>>
>>Modulo 2^32 math will handle that just fine.
>>
>>>
>>> Thanks Ville for your valuable review comments.
>>>
>>> Regards,
>>> Uma Shankar
>>>
>>> >
>>> >> +
>>> >> + return ulScanlineNo2;
>>> >> +}
>>> >> +
>>> >> static void intel_dsi_connector_destroy(struct drm_connector
>>> >> *connector) {
>>> >> struct intel_connector *intel_connector =
>>> >> to_intel_connector(connector);
>>> >> --
>>> >> 1.9.1
>>> >
>>> >--
>>> >Ville Syrjälä
>>> >Intel OTC
>>
>>--
>>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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 13:23 ` Shankar, Uma
@ 2017-09-12 13:33 ` Ville Syrjälä
2017-09-12 13:40 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-12 13:33 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Srinivas, Vidya
On Tue, Sep 12, 2017 at 01:23:39PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Shankar, Uma
> >Sent: Tuesday, September 12, 2017 3:20 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] drm/i915: Enable scanline read for gen9 dsi
> >
> >
> >
> >>-----Original Message-----
> >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >>Sent: Monday, September 11, 2017 11:20 PM
> >>To: Shankar, Uma <uma.shankar@intel.com>
> >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>;
> >>intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>;
> >>Kamath, Sunil <sunil.kamath@intel.com>; Konduru, Chandra
> >><chandra.konduru@intel.com>
> >>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >>
> >>On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
> >>>
> >>>
> >>> >-----Original Message-----
> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >>> >Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
> >>> >
> >>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
> >>> >>
> >>> >> 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 | 5 +++++
> >>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
> >>> >> ++++++++++++++++++++++++++++++++++++++++
> >>> >> 4 files changed, 56 insertions(+)
> >>> >>
> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
> >>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> >>> >> + enum transcoder cpu_transcoder;
> >>> >>
> >>> >> if (!crtc->active)
> >>> >> return -1;
> >>> >> @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct
> >>> >> intel_crtc
> >>> >*crtc)
> >>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >>> >> vtotal /= 2;
> >>> >>
> >>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
> >>> >
> >>> >Humm. Would be nice to be able to do this without adding more
> >>> >crtc->config uses. We're pretty much trying to get rid of that guy.
> >>> >
> >>>
> >>> Will try to find an alternate way to do this.
> >>>
> >>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> >>> >> + return bxt_dsi_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 9a73ea0..54582de 100644
> >>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> >>> >> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> >>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> >>> >>
> >>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> >>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
> >>> >
> >>> >Please add proper parametrized define that works for all pipes.
> >>> >
> >>>
> >>> Will add that.
> >>>
> >>> >> +
> >>> >> /* BXT MIPI clock controls */
> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
> >>> >>
> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
> >>> >> index 2a0f5d3..d145ba4 100644
> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
> >>> >drm_connector *connector)
> >>> >> return 1;
> >>> >> }
> >>> >>
> >>> >> +/*
> >>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
> >>> >> + struct drm_device *dev = crtc->base.dev;
> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
> >>> >
> >>> >Please get rid of the hungarian notation.
> >>> >
> >>>
> >>> Yes, will fix this.
> >>>
> >>> >> + uint_fixed_16_16_t ulScanlineTime;
> >>> >> +
> >>> >> + /*
> >>> >> + * This field provides read back of the display
> >>> >> + * pipe frame time stamp. The time stamp value
> >>> >> + * is sampled at every start of vertical blank.
> >>> >> + */
> >>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
> >>> >> +
> >>> >> + /*
> >>> >> + * The TIMESTAMP_CTR register has the current
> >>> >> + * time stamp value.
> >>> >> + */
> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
> >>> >> +
> >>> >> + /* The PORT for DSI will always be 0 since
> >>> >> + * isolated PORTC cannot be enabled for Gen9
> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
> >>> >> + * the VTOTAL value.
> >>> >> + */
> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
> >>> >
> >>> >This value can be dug out from the hwmode.
> >>> >
> >>>
> >>> Yes, will get it from hwmode and drop this change.
> >>>
> >>> >> + WARN_ON(!vtotal);
> >>> >> + if (!vtotal)
> >>> >> + return ulScanlineNo2;
> >>> >> +
> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime),
> >>> >> + ulScanlineTime);
> >>> >
> >>> >Something like:
> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
> >>> > 1000 * crtc_htotal);
> >>> >
> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
> >>> >
> >>> >I think that would have to be something like:
> >>> >return (scanline + vblank_start) % vtotal;
> >>> >
> >>>
> >>> Yes you are right. It should be vblank_start. Will fix this.
> >>>
> >>> >All in all this looks like a pretty decent approach to the DSI problem.
> >>> >
> >>> >One concern here is rounding issues and inaccuracies in our
> >>> >crtc_clock. But since the frame timestamp is sampled at vblank start
> >>> >I guess we can't accidentally get an answer that's earlier than
> >>> >vblank_start as long as we really passed vblank start already. That
> >>> >should
> >>make this at least suitable for vblank timestamps.
> >>>
> >>> I also feel the same, this situation should never occur.
> >>>
> >>> >And for
> >>> >the atomic evade, I guess if we clamp our the scanline before the
> >>> >+vblank_start such that it never reaches vtotal, we can't be sure
> >>> >+that
> >>> >our vblank evade never indicates that we already reached the start
> >>> >of vblank prematurely.
> >>> >
> >>> >So maybe something like:
> >>> >scaline = div_u64(...);
> >>> >scanline = min(scanline, vtotal - 1);
> >>>
> >>> I am not sure if the value of scanline returned can ever be greater
> >>> than vtotal -
> >>1.
> >>> But we can have a check just to be safe. Not sure if I fully got your point here.
> >>
> >>The point is that the timestamp counter might tick at a slightly faster
> >>rate than we might think. Thus we might end up with more ticks in one
> >>frame than what we calculated as the maximum fom crtc_clock etc. But if
> >>we clamp the value like I suggested then at least we should never get
> >>an answer that tells us we're already past the start of vblank when in reality
> >we're not.
> >>
> >>Of course as Daniel pointed out we might also get into trouble if the
> >>counter ticks slower than expected. That could lead us to think that we
> >>don't need to do the vblank evade when in fact we do.
> >>
>
> Hi Ville,
> We tried to test with this condition and are calculating wrong scanlines.
> For ex:
> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534, crtc_vtotal-1 = 1211, min of two = 1211
Well, that scanline number looks totally bogus. How did you calculate it exactly?
> This causes calculated value to be different from PIPE SCANLINE value read from register.
>
> But if we keep scanline and take the modulo with vtotal after adding the vblank_start (not taking min with vtotal -1),
> we are getting scanline equal to (delta of 1 all the time ) what the PIPE SCANLINE register returns for HDMI. We can
> use HDMI as a reference to validate if timestamp based calculation aligns to h/w values returned by
> Pipe scanline register. So if we do like below:
>
> scanline = div_u64(...);
> return (scanline + vblank_start) % vtotal;
>
> It works perfectly fine. Tested it with "kms_plane_multiple" and almost getting the same result all the time with
> no atomic update failures. I am not sure on other platforms, but BXT/APL its working fine. Without these changes,
> issue can easily be reproduced using this IGT test.
>
> Will send the updated patch, please have a look once.
>
> Thanks & Regards,
> Uma Shankar
>
> >>Oh and there's maybe another race lurking here. We might cross into the
> >>next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR reads. If
> >>that happens we get an answer that's definitely too big for one frame.
> >>I guess we could avoid that particular problem by making sure we really
> >>read PIPE_FRMTMSTMP and TIMESTAMP_CTR during the same frame. Eg.
> >>something like:
> >>
> >>do {
> >> prev = PIPE_FRMTMSTMP;
> >> curr = TIMESTAMP_CTR
> >> post = PIPE_FRMTMSTMP
> >>} while (prev != post);
> >>
> >
> >Got it. Will add this condition to handle the race situation. Thanks for the
> >explanation.
> >
> >Regards,
> >Uma Shankar
> >
> >>
> >>>
> >>> >return (scanline + vblank_start) % vtotal;
> >>> >
> >>> >At least that's my thinking atm. Feel free to rip my reasoning to
> >>> >shreds if you think I'm totally wrong here.
> >>> >
> >>>
> >>> One more thing we missed is, that the current timestamp is just a 32
> >>> bit register
> >>value.
> >>> It can overflow and wrap around. So a situation can come, where
> >>> current timestamp will be less than prev timestamp (read from frame
> >>> time stamp reg). We need to handle that situation as well. Will fix
> >>> that in the
> >>next version and resend.
> >>
> >>Modulo 2^32 math will handle that just fine.
> >>
> >>>
> >>> Thanks Ville for your valuable review comments.
> >>>
> >>> Regards,
> >>> Uma Shankar
> >>>
> >>> >
> >>> >> +
> >>> >> + return ulScanlineNo2;
> >>> >> +}
> >>> >> +
> >>> >> static void intel_dsi_connector_destroy(struct drm_connector
> >>> >> *connector) {
> >>> >> struct intel_connector *intel_connector =
> >>> >> to_intel_connector(connector);
> >>> >> --
> >>> >> 1.9.1
> >>> >
> >>> >--
> >>> >Ville Syrjälä
> >>> >Intel OTC
> >>
> >>--
> >>Ville Syrjälä
> >>Intel OTC
> >_______________________________________________
> >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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 13:33 ` Ville Syrjälä
@ 2017-09-12 13:40 ` Shankar, Uma
2017-09-12 13:55 ` Vidya Srinivas
2017-09-12 14:12 ` Ville Syrjälä
0 siblings, 2 replies; 27+ messages in thread
From: Shankar, Uma @ 2017-09-12 13:40 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, September 12, 2017 7:04 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Tue, Sep 12, 2017 at 01:23:39PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>> >Behalf Of Shankar, Uma
>> >Sent: Tuesday, September 12, 2017 3:20 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] drm/i915: Enable scanline read for
>> >gen9 dsi
>> >
>> >
>> >
>> >>-----Original Message-----
>> >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >>Sent: Monday, September 11, 2017 11:20 PM
>> >>To: Shankar, Uma <uma.shankar@intel.com>
>> >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>;
>> >>intel-gfx@lists.freedesktop.org; Kahola, Mika
>> >><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>;
>> >>Konduru, Chandra <chandra.konduru@intel.com>
>> >>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >>
>> >>On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
>> >>>
>> >>>
>> >>> >-----Original Message-----
>> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >>> >Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
>> >>> >
>> >>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
>> >>> >>
>> >>> >> 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 | 5 +++++
>> >>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> >>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
>> >>> >> ++++++++++++++++++++++++++++++++++++++++
>> >>> >> 4 files changed, 56 insertions(+)
>> >>> >>
>> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
>> >>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
>> >>> >> + enum transcoder cpu_transcoder;
>> >>> >>
>> >>> >> if (!crtc->active)
>> >>> >> return -1;
>> >>> >> @@ -792,6 +793,10 @@ static int
>> >>> >> __intel_get_crtc_scanline(struct intel_crtc
>> >>> >*crtc)
>> >>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> >>> >> vtotal /= 2;
>> >>> >>
>> >>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
>> >>> >
>> >>> >Humm. Would be nice to be able to do this without adding more
>> >>> >crtc->config uses. We're pretty much trying to get rid of that guy.
>> >>> >
>> >>>
>> >>> Will try to find an alternate way to do this.
>> >>>
>> >>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>> >>> >> + return bxt_dsi_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 9a73ea0..54582de 100644
>> >>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
>> >>> >> #define MIPIO_TXESC_CLK_DIV2
> _MMIO(0x160008)
>> >>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>> >>> >>
>> >>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> >>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>> >>> >
>> >>> >Please add proper parametrized define that works for all pipes.
>> >>> >
>> >>>
>> >>> Will add that.
>> >>>
>> >>> >> +
>> >>> >> /* BXT MIPI clock controls */
>> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>> >>> >>
>> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
>> >>> >> index 2a0f5d3..d145ba4 100644
>> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
>> >>> >drm_connector *connector)
>> >>> >> return 1;
>> >>> >> }
>> >>> >>
>> >>> >> +/*
>> >>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>> >>> >> + struct drm_device *dev = crtc->base.dev;
>> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
>> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>> >>> >
>> >>> >Please get rid of the hungarian notation.
>> >>> >
>> >>>
>> >>> Yes, will fix this.
>> >>>
>> >>> >> + uint_fixed_16_16_t ulScanlineTime;
>> >>> >> +
>> >>> >> + /*
>> >>> >> + * This field provides read back of the display
>> >>> >> + * pipe frame time stamp. The time stamp value
>> >>> >> + * is sampled at every start of vertical blank.
>> >>> >> + */
>> >>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>> >>> >> +
>> >>> >> + /*
>> >>> >> + * The TIMESTAMP_CTR register has the current
>> >>> >> + * time stamp value.
>> >>> >> + */
>> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>> >>> >> +
>> >>> >> + /* The PORT for DSI will always be 0 since
>> >>> >> + * isolated PORTC cannot be enabled for Gen9
>> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
>> >>> >> + * the VTOTAL value.
>> >>> >> + */
>> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>> >>> >
>> >>> >This value can be dug out from the hwmode.
>> >>> >
>> >>>
>> >>> Yes, will get it from hwmode and drop this change.
>> >>>
>> >>> >> + WARN_ON(!vtotal);
>> >>> >> + if (!vtotal)
>> >>> >> + return ulScanlineNo2;
>> >>> >> +
>> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
>> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime -
>ulPrevTime),
>> >>> >> + ulScanlineTime);
>> >>> >
>> >>> >Something like:
>> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
>> >>> > 1000 * crtc_htotal);
>> >>> >
>> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>> >>> >
>> >>> >I think that would have to be something like:
>> >>> >return (scanline + vblank_start) % vtotal;
>> >>> >
>> >>>
>> >>> Yes you are right. It should be vblank_start. Will fix this.
>> >>>
>> >>> >All in all this looks like a pretty decent approach to the DSI problem.
>> >>> >
>> >>> >One concern here is rounding issues and inaccuracies in our
>> >>> >crtc_clock. But since the frame timestamp is sampled at vblank
>> >>> >start I guess we can't accidentally get an answer that's earlier
>> >>> >than vblank_start as long as we really passed vblank start
>> >>> >already. That should
>> >>make this at least suitable for vblank timestamps.
>> >>>
>> >>> I also feel the same, this situation should never occur.
>> >>>
>> >>> >And for
>> >>> >the atomic evade, I guess if we clamp our the scanline before the
>> >>> >+vblank_start such that it never reaches vtotal, we can't be sure
>> >>> >+that
>> >>> >our vblank evade never indicates that we already reached the
>> >>> >start of vblank prematurely.
>> >>> >
>> >>> >So maybe something like:
>> >>> >scaline = div_u64(...);
>> >>> >scanline = min(scanline, vtotal - 1);
>> >>>
>> >>> I am not sure if the value of scanline returned can ever be
>> >>> greater than vtotal -
>> >>1.
>> >>> But we can have a check just to be safe. Not sure if I fully got your point
>here.
>> >>
>> >>The point is that the timestamp counter might tick at a slightly
>> >>faster rate than we might think. Thus we might end up with more
>> >>ticks in one frame than what we calculated as the maximum fom
>> >>crtc_clock etc. But if we clamp the value like I suggested then at
>> >>least we should never get an answer that tells us we're already past
>> >>the start of vblank when in reality
>> >we're not.
>> >>
>> >>Of course as Daniel pointed out we might also get into trouble if
>> >>the counter ticks slower than expected. That could lead us to think
>> >>that we don't need to do the vblank evade when in fact we do.
>> >>
>>
>> Hi Ville,
>> We tried to test with this condition and are calculating wrong scanlines.
>> For ex:
>> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534,
>crtc_vtotal-1 = 1211, min of two = 1211
>
>Well, that scanline number looks totally bogus. How did you calculate it exactly?
>
If we have multiple scans on the same frame (no new flip being issued). Prev timestamp
value which is read from Frametime Stamp will remain same, but current time stamp
will keep on incrementing. So assume if 10 times the same frame has been scanned and we
are in 11 iteration and at that time a new flip is issued (where we are doing these checks).
At that moment the delta of curr - prev will still have a huge value and will surely give results,
adding the lines scanned in earlier iterations of the same frame. Thus by doing the modulo operation
using vtotal we can really check for the scanline in the current iteration of the scan.
Thus getting a high value of scanline is expected.
Regards,
Uma Shankar
>> This causes calculated value to be different from PIPE SCANLINE value read
>from register.
>>
>> But if we keep scanline and take the modulo with vtotal after adding
>> the vblank_start (not taking min with vtotal -1), we are getting
>> scanline equal to (delta of 1 all the time ) what the PIPE SCANLINE
>> register returns for HDMI. We can use HDMI as a reference to validate if
>timestamp based calculation aligns to h/w values returned by Pipe scanline
>register. So if we do like below:
>>
>> scanline = div_u64(...);
>> return (scanline + vblank_start) % vtotal;
>>
>> It works perfectly fine. Tested it with "kms_plane_multiple" and
>> almost getting the same result all the time with no atomic update
>> failures. I am not sure on other platforms, but BXT/APL its working fine.
>Without these changes, issue can easily be reproduced using this IGT test.
>>
>> Will send the updated patch, please have a look once.
>>
>> Thanks & Regards,
>> Uma Shankar
>>
>> >>Oh and there's maybe another race lurking here. We might cross into
>> >>the next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR
>> >>reads. If that happens we get an answer that's definitely too big for one
>frame.
>> >>I guess we could avoid that particular problem by making sure we
>> >>really read PIPE_FRMTMSTMP and TIMESTAMP_CTR during the same frame.
>Eg.
>> >>something like:
>> >>
>> >>do {
>> >> prev = PIPE_FRMTMSTMP;
>> >> curr = TIMESTAMP_CTR
>> >> post = PIPE_FRMTMSTMP
>> >>} while (prev != post);
>> >>
>> >
>> >Got it. Will add this condition to handle the race situation. Thanks
>> >for the explanation.
>> >
>> >Regards,
>> >Uma Shankar
>> >
>> >>
>> >>>
>> >>> >return (scanline + vblank_start) % vtotal;
>> >>> >
>> >>> >At least that's my thinking atm. Feel free to rip my reasoning to
>> >>> >shreds if you think I'm totally wrong here.
>> >>> >
>> >>>
>> >>> One more thing we missed is, that the current timestamp is just a
>> >>> 32 bit register
>> >>value.
>> >>> It can overflow and wrap around. So a situation can come, where
>> >>> current timestamp will be less than prev timestamp (read from
>> >>> frame time stamp reg). We need to handle that situation as well.
>> >>> Will fix that in the
>> >>next version and resend.
>> >>
>> >>Modulo 2^32 math will handle that just fine.
>> >>
>> >>>
>> >>> Thanks Ville for your valuable review comments.
>> >>>
>> >>> Regards,
>> >>> Uma Shankar
>> >>>
>> >>> >
>> >>> >> +
>> >>> >> + return ulScanlineNo2;
>> >>> >> +}
>> >>> >> +
>> >>> >> static void intel_dsi_connector_destroy(struct drm_connector
>> >>> >> *connector) {
>> >>> >> struct intel_connector *intel_connector =
>> >>> >> to_intel_connector(connector);
>> >>> >> --
>> >>> >> 1.9.1
>> >>> >
>> >>> >--
>> >>> >Ville Syrjälä
>> >>> >Intel OTC
>> >>
>> >>--
>> >>Ville Syrjälä
>> >>Intel OTC
>> >_______________________________________________
>> >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
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 13:40 ` Shankar, Uma
@ 2017-09-12 13:55 ` Vidya Srinivas
2017-09-12 14:12 ` Ville Syrjälä
1 sibling, 0 replies; 27+ messages in thread
From: Vidya Srinivas @ 2017-09-12 13:55 UTC (permalink / raw)
To: intel-gfx; +Cc: 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.
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_dsi.c | 59 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 79 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1cc31a5..fc84136 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 bxt_dsi_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..40fa2cb 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 bxt_dsi_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..864924c86 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_A, _IVB_PIPE_FRMTMSTMP_C)
+
/* BXT MIPI clock controls */
#define BXT_MAX_VAR_OUTPUT_KHZ 39500
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2a0f5d3..d000e01 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1621,6 +1621,65 @@ static int intel_dsi_get_modes(struct drm_connector *connector)
return 1;
}
+/*
+ * 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 bxt_dsi_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 = (scanline + crtc_vblank_start) % crtc_vtotal;
+
+ return scanline;
+}
+
static void intel_dsi_connector_destroy(struct drm_connector *connector)
{
struct intel_connector *intel_connector = to_intel_connector(connector);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 13:40 ` Shankar, Uma
2017-09-12 13:55 ` Vidya Srinivas
@ 2017-09-12 14:12 ` Ville Syrjälä
2017-09-12 14:21 ` Shankar, Uma
1 sibling, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-12 14:12 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Srinivas, Vidya
On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, September 12, 2017 7:04 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >
> >On Tue, Sep 12, 2017 at 01:23:39PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> >> >Behalf Of Shankar, Uma
> >> >Sent: Tuesday, September 12, 2017 3:20 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] drm/i915: Enable scanline read for
> >> >gen9 dsi
> >> >
> >> >
> >> >
> >> >>-----Original Message-----
> >> >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >>Sent: Monday, September 11, 2017 11:20 PM
> >> >>To: Shankar, Uma <uma.shankar@intel.com>
> >> >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>;
> >> >>intel-gfx@lists.freedesktop.org; Kahola, Mika
> >> >><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>;
> >> >>Konduru, Chandra <chandra.konduru@intel.com>
> >> >>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >> >>
> >> >>On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
> >> >>>
> >> >>>
> >> >>> >-----Original Message-----
> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >>> >Sent: Friday, September 8, 2017 8:18 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 for gen9 dsi
> >> >>> >
> >> >>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
> >> >>> >>
> >> >>> >> 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 | 5 +++++
> >> >>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >> >>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
> >> >>> >> ++++++++++++++++++++++++++++++++++++++++
> >> >>> >> 4 files changed, 56 insertions(+)
> >> >>> >>
> >> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> >>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54 100644
> >> >>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0 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;
> >> >>> >> + enum transcoder cpu_transcoder;
> >> >>> >>
> >> >>> >> if (!crtc->active)
> >> >>> >> return -1;
> >> >>> >> @@ -792,6 +793,10 @@ static int
> >> >>> >> __intel_get_crtc_scanline(struct intel_crtc
> >> >>> >*crtc)
> >> >>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> >>> >> vtotal /= 2;
> >> >>> >>
> >> >>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
> >> >>> >
> >> >>> >Humm. Would be nice to be able to do this without adding more
> >> >>> >crtc->config uses. We're pretty much trying to get rid of that guy.
> >> >>> >
> >> >>>
> >> >>> Will try to find an alternate way to do this.
> >> >>>
> >> >>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> >> >>> >> + return bxt_dsi_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 9a73ea0..54582de 100644
> >> >>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> >> >>> >> #define MIPIO_TXESC_CLK_DIV2
> > _MMIO(0x160008)
> >> >>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> >> >>> >>
> >> >>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> >> >>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
> >> >>> >
> >> >>> >Please add proper parametrized define that works for all pipes.
> >> >>> >
> >> >>>
> >> >>> Will add that.
> >> >>>
> >> >>> >> +
> >> >>> >> /* BXT MIPI clock controls */
> >> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
> >> >>> >>
> >> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> >> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
> >> >>> >> index 2a0f5d3..d145ba4 100644
> >> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> >>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
> >> >>> >drm_connector *connector)
> >> >>> >> return 1;
> >> >>> >> }
> >> >>> >>
> >> >>> >> +/*
> >> >>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
> >> >>> >> + struct drm_device *dev = crtc->base.dev;
> >> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
> >> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
> >> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
> >> >>> >
> >> >>> >Please get rid of the hungarian notation.
> >> >>> >
> >> >>>
> >> >>> Yes, will fix this.
> >> >>>
> >> >>> >> + uint_fixed_16_16_t ulScanlineTime;
> >> >>> >> +
> >> >>> >> + /*
> >> >>> >> + * This field provides read back of the display
> >> >>> >> + * pipe frame time stamp. The time stamp value
> >> >>> >> + * is sampled at every start of vertical blank.
> >> >>> >> + */
> >> >>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
> >> >>> >> +
> >> >>> >> + /*
> >> >>> >> + * The TIMESTAMP_CTR register has the current
> >> >>> >> + * time stamp value.
> >> >>> >> + */
> >> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
> >> >>> >> +
> >> >>> >> + /* The PORT for DSI will always be 0 since
> >> >>> >> + * isolated PORTC cannot be enabled for Gen9
> >> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
> >> >>> >> + * the VTOTAL value.
> >> >>> >> + */
> >> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
> >> >>> >
> >> >>> >This value can be dug out from the hwmode.
> >> >>> >
> >> >>>
> >> >>> Yes, will get it from hwmode and drop this change.
> >> >>>
> >> >>> >> + WARN_ON(!vtotal);
> >> >>> >> + if (!vtotal)
> >> >>> >> + return ulScanlineNo2;
> >> >>> >> +
> >> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
> >> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime -
> >ulPrevTime),
> >> >>> >> + ulScanlineTime);
> >> >>> >
> >> >>> >Something like:
> >> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
> >> >>> > 1000 * crtc_htotal);
> >> >>> >
> >> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
> >> >>> >
> >> >>> >I think that would have to be something like:
> >> >>> >return (scanline + vblank_start) % vtotal;
> >> >>> >
> >> >>>
> >> >>> Yes you are right. It should be vblank_start. Will fix this.
> >> >>>
> >> >>> >All in all this looks like a pretty decent approach to the DSI problem.
> >> >>> >
> >> >>> >One concern here is rounding issues and inaccuracies in our
> >> >>> >crtc_clock. But since the frame timestamp is sampled at vblank
> >> >>> >start I guess we can't accidentally get an answer that's earlier
> >> >>> >than vblank_start as long as we really passed vblank start
> >> >>> >already. That should
> >> >>make this at least suitable for vblank timestamps.
> >> >>>
> >> >>> I also feel the same, this situation should never occur.
> >> >>>
> >> >>> >And for
> >> >>> >the atomic evade, I guess if we clamp our the scanline before the
> >> >>> >+vblank_start such that it never reaches vtotal, we can't be sure
> >> >>> >+that
> >> >>> >our vblank evade never indicates that we already reached the
> >> >>> >start of vblank prematurely.
> >> >>> >
> >> >>> >So maybe something like:
> >> >>> >scaline = div_u64(...);
> >> >>> >scanline = min(scanline, vtotal - 1);
> >> >>>
> >> >>> I am not sure if the value of scanline returned can ever be
> >> >>> greater than vtotal -
> >> >>1.
> >> >>> But we can have a check just to be safe. Not sure if I fully got your point
> >here.
> >> >>
> >> >>The point is that the timestamp counter might tick at a slightly
> >> >>faster rate than we might think. Thus we might end up with more
> >> >>ticks in one frame than what we calculated as the maximum fom
> >> >>crtc_clock etc. But if we clamp the value like I suggested then at
> >> >>least we should never get an answer that tells us we're already past
> >> >>the start of vblank when in reality
> >> >we're not.
> >> >>
> >> >>Of course as Daniel pointed out we might also get into trouble if
> >> >>the counter ticks slower than expected. That could lead us to think
> >> >>that we don't need to do the vblank evade when in fact we do.
> >> >>
> >>
> >> Hi Ville,
> >> We tried to test with this condition and are calculating wrong scanlines.
> >> For ex:
> >> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534,
> >crtc_vtotal-1 = 1211, min of two = 1211
> >
> >Well, that scanline number looks totally bogus. How did you calculate it exactly?
> >
>
> If we have multiple scans on the same frame (no new flip being issued). Prev timestamp
> value which is read from Frametime Stamp will remain same, but current time stamp
> will keep on incrementing.
The frame timestamp should get sampled on every vblank, whereas the flip
timestamp only when a flip occurs. Are you using the correct timestamp
register?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 14:12 ` Ville Syrjälä
@ 2017-09-12 14:21 ` Shankar, Uma
2017-09-12 15:06 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2017-09-12 14:21 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, September 12, 2017 7:43 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Tuesday, September 12, 2017 7:04 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
>> ><vidya.srinivas@intel.com>
>> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >
>> >> >>>
>> >> >>> >-----Original Message-----
>> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >>> >Sent: Friday, September 8, 2017 8:18 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 for gen9
>> >> >>> >dsi
>> >> >>> >
>> >> >>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
>> >> >>> >>
>> >> >>> >> 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 | 5 +++++
>> >> >>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> >> >>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
>> >> >>> >> ++++++++++++++++++++++++++++++++++++++++
>> >> >>> >> 4 files changed, 56 insertions(+)
>> >> >>> >>
>> >> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> >>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54
>> >> >>> >> 100644
>> >> >>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> >>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0
>> >> >>> >> 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;
>> >> >>> >> + enum transcoder cpu_transcoder;
>> >> >>> >>
>> >> >>> >> if (!crtc->active)
>> >> >>> >> return -1;
>> >> >>> >> @@ -792,6 +793,10 @@ static int
>> >> >>> >> __intel_get_crtc_scanline(struct intel_crtc
>> >> >>> >*crtc)
>> >> >>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> >> >>> >> vtotal /= 2;
>> >> >>> >>
>> >> >>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
>> >> >>> >
>> >> >>> >Humm. Would be nice to be able to do this without adding more
>> >> >>> >crtc->config uses. We're pretty much trying to get rid of that guy.
>> >> >>> >
>> >> >>>
>> >> >>> Will try to find an alternate way to do this.
>> >> >>>
>> >> >>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
>> >> >>> >> + return bxt_dsi_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 9a73ea0..54582de
>> >> >>> >> 100644
>> >> >>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> >>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate { #define
>> >> >>> >> MIPIO_TXESC_CLK_DIV2
>> > _MMIO(0x160008)
>> >> >>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>> >> >>> >>
>> >> >>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> >> >>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>> >> >>> >
>> >> >>> >Please add proper parametrized define that works for all pipes.
>> >> >>> >
>> >> >>>
>> >> >>> Will add that.
>> >> >>>
>> >> >>> >> +
>> >> >>> >> /* BXT MIPI clock controls */
>> >> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>> >> >>> >>
>> >> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> >> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
>> >> >>> >> index 2a0f5d3..d145ba4 100644
>> >> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> >>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
>> >> >>> >drm_connector *connector)
>> >> >>> >> return 1;
>> >> >>> >> }
>> >> >>> >>
>> >> >>> >> +/*
>> >> >>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>> >> >>> >> + struct drm_device *dev = crtc->base.dev;
>> >> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
>> >> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>> >> >>> >
>> >> >>> >Please get rid of the hungarian notation.
>> >> >>> >
>> >> >>>
>> >> >>> Yes, will fix this.
>> >> >>>
>> >> >>> >> + uint_fixed_16_16_t ulScanlineTime;
>> >> >>> >> +
>> >> >>> >> + /*
>> >> >>> >> + * This field provides read back of the display
>> >> >>> >> + * pipe frame time stamp. The time stamp value
>> >> >>> >> + * is sampled at every start of vertical blank.
>> >> >>> >> + */
>> >> >>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>> >> >>> >> +
>> >> >>> >> + /*
>> >> >>> >> + * The TIMESTAMP_CTR register has the current
>> >> >>> >> + * time stamp value.
>> >> >>> >> + */
>> >> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>> >> >>> >> +
>> >> >>> >> + /* The PORT for DSI will always be 0 since
>> >> >>> >> + * isolated PORTC cannot be enabled for Gen9
>> >> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
>> >> >>> >> + * the VTOTAL value.
>> >> >>> >> + */
>> >> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>> >> >>> >
>> >> >>> >This value can be dug out from the hwmode.
>> >> >>> >
>> >> >>>
>> >> >>> Yes, will get it from hwmode and drop this change.
>> >> >>>
>> >> >>> >> + WARN_ON(!vtotal);
>> >> >>> >> + if (!vtotal)
>> >> >>> >> + return ulScanlineNo2;
>> >> >>> >> +
>> >> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
>> >> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime -
>> >ulPrevTime),
>> >> >>> >> + ulScanlineTime);
>> >> >>> >
>> >> >>> >Something like:
>> >> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
>> >> >>> > 1000 * crtc_htotal);
>> >> >>> >
>> >> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>> >> >>> >
>> >> >>> >I think that would have to be something like:
>> >> >>> >return (scanline + vblank_start) % vtotal;
>> >> >>> >
>> >> >>>
>> >> >>> Yes you are right. It should be vblank_start. Will fix this.
>> >> >>>
>> >> >>> >All in all this looks like a pretty decent approach to the DSI problem.
>> >> >>> >
>> >> >>> >One concern here is rounding issues and inaccuracies in our
>> >> >>> >crtc_clock. But since the frame timestamp is sampled at vblank
>> >> >>> >start I guess we can't accidentally get an answer that's
>> >> >>> >earlier than vblank_start as long as we really passed vblank
>> >> >>> >start already. That should
>> >> >>make this at least suitable for vblank timestamps.
>> >> >>>
>> >> >>> I also feel the same, this situation should never occur.
>> >> >>>
>> >> >>> >And for
>> >> >>> >the atomic evade, I guess if we clamp our the scanline before
>> >> >>> >the
>> >> >>> >+vblank_start such that it never reaches vtotal, we can't be
>> >> >>> >+sure that
>> >> >>> >our vblank evade never indicates that we already reached the
>> >> >>> >start of vblank prematurely.
>> >> >>> >
>> >> >>> >So maybe something like:
>> >> >>> >scaline = div_u64(...);
>> >> >>> >scanline = min(scanline, vtotal - 1);
>> >> >>>
>> >> >>> I am not sure if the value of scanline returned can ever be
>> >> >>> greater than vtotal -
>> >> >>1.
>> >> >>> But we can have a check just to be safe. Not sure if I fully
>> >> >>> got your point
>> >here.
>> >> >>
>> >> >>The point is that the timestamp counter might tick at a slightly
>> >> >>faster rate than we might think. Thus we might end up with more
>> >> >>ticks in one frame than what we calculated as the maximum fom
>> >> >>crtc_clock etc. But if we clamp the value like I suggested then
>> >> >>at least we should never get an answer that tells us we're
>> >> >>already past the start of vblank when in reality
>> >> >we're not.
>> >> >>
>> >> >>Of course as Daniel pointed out we might also get into trouble if
>> >> >>the counter ticks slower than expected. That could lead us to
>> >> >>think that we don't need to do the vblank evade when in fact we do.
>> >> >>
>> >>
>> >> Hi Ville,
>> >> We tried to test with this condition and are calculating wrong scanlines.
>> >> For ex:
>> >> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534,
>> >crtc_vtotal-1 = 1211, min of two = 1211
>> >
>> >Well, that scanline number looks totally bogus. How did you calculate it
>exactly?
>> >
>>
>> If we have multiple scans on the same frame (no new flip being
>> issued). Prev timestamp value which is read from Frametime Stamp will
>> remain same, but current time stamp will keep on incrementing.
>
>The frame timestamp should get sampled on every vblank, whereas the flip
>timestamp only when a flip occurs. Are you using the correct timestamp register?
>
Yes, we are using what is there in the patch.
Name Pipe A Frame Time Stamp
Symbol PIPE_FRMTMSTMP_A
Start 0x70048
End 0x7004B
Its behaving as FLIP Timestamp though (not being updated on every vblank_start).
Atleast with the readback what we get on APL.
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 14:21 ` Shankar, Uma
@ 2017-09-12 15:06 ` Ville Syrjälä
2017-09-13 8:24 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-12 15:06 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Srinivas, Vidya
On Tue, Sep 12, 2017 at 02:21:42PM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, September 12, 2017 7:43 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >
> >On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Tuesday, September 12, 2017 7:04 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
> >> ><vidya.srinivas@intel.com>
> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >> >
> >> >> >>>
> >> >> >>> >-----Original Message-----
> >> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >> >>> >Sent: Friday, September 8, 2017 8:18 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 for gen9
> >> >> >>> >dsi
> >> >> >>> >
> >> >> >>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
> >> >> >>> >>
> >> >> >>> >> 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 | 5 +++++
> >> >> >>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >> >> >>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
> >> >> >>> >> ++++++++++++++++++++++++++++++++++++++++
> >> >> >>> >> 4 files changed, 56 insertions(+)
> >> >> >>> >>
> >> >> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> >> >>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54
> >> >> >>> >> 100644
> >> >> >>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >> >>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0
> >> >> >>> >> 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;
> >> >> >>> >> + enum transcoder cpu_transcoder;
> >> >> >>> >>
> >> >> >>> >> if (!crtc->active)
> >> >> >>> >> return -1;
> >> >> >>> >> @@ -792,6 +793,10 @@ static int
> >> >> >>> >> __intel_get_crtc_scanline(struct intel_crtc
> >> >> >>> >*crtc)
> >> >> >>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> >> >> >>> >> vtotal /= 2;
> >> >> >>> >>
> >> >> >>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
> >> >> >>> >
> >> >> >>> >Humm. Would be nice to be able to do this without adding more
> >> >> >>> >crtc->config uses. We're pretty much trying to get rid of that guy.
> >> >> >>> >
> >> >> >>>
> >> >> >>> Will try to find an alternate way to do this.
> >> >> >>>
> >> >> >>> >> + if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> >> >> >>> >> + return bxt_dsi_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 9a73ea0..54582de
> >> >> >>> >> 100644
> >> >> >>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >> >>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate { #define
> >> >> >>> >> MIPIO_TXESC_CLK_DIV2
> >> > _MMIO(0x160008)
> >> >> >>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
> >> >> >>> >>
> >> >> >>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
> >> >> >>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
> >> >> >>> >
> >> >> >>> >Please add proper parametrized define that works for all pipes.
> >> >> >>> >
> >> >> >>>
> >> >> >>> Will add that.
> >> >> >>>
> >> >> >>> >> +
> >> >> >>> >> /* BXT MIPI clock controls */
> >> >> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
> >> >> >>> >>
> >> >> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> >> >> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
> >> >> >>> >> index 2a0f5d3..d145ba4 100644
> >> >> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> >> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> >> >>> >> @@ -1621,6 +1621,52 @@ static int intel_dsi_get_modes(struct
> >> >> >>> >drm_connector *connector)
> >> >> >>> >> return 1;
> >> >> >>> >> }
> >> >> >>> >>
> >> >> >>> >> +/*
> >> >> >>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
> >> >> >>> >> + struct drm_device *dev = crtc->base.dev;
> >> >> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
> >> >> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
> >> >> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
> >> >> >>> >
> >> >> >>> >Please get rid of the hungarian notation.
> >> >> >>> >
> >> >> >>>
> >> >> >>> Yes, will fix this.
> >> >> >>>
> >> >> >>> >> + uint_fixed_16_16_t ulScanlineTime;
> >> >> >>> >> +
> >> >> >>> >> + /*
> >> >> >>> >> + * This field provides read back of the display
> >> >> >>> >> + * pipe frame time stamp. The time stamp value
> >> >> >>> >> + * is sampled at every start of vertical blank.
> >> >> >>> >> + */
> >> >> >>> >> + ulPrevTime = I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
> >> >> >>> >> +
> >> >> >>> >> + /*
> >> >> >>> >> + * The TIMESTAMP_CTR register has the current
> >> >> >>> >> + * time stamp value.
> >> >> >>> >> + */
> >> >> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
> >> >> >>> >> +
> >> >> >>> >> + /* The PORT for DSI will always be 0 since
> >> >> >>> >> + * isolated PORTC cannot be enabled for Gen9
> >> >> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
> >> >> >>> >> + * the VTOTAL value.
> >> >> >>> >> + */
> >> >> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
> >> >> >>> >
> >> >> >>> >This value can be dug out from the hwmode.
> >> >> >>> >
> >> >> >>>
> >> >> >>> Yes, will get it from hwmode and drop this change.
> >> >> >>>
> >> >> >>> >> + WARN_ON(!vtotal);
> >> >> >>> >> + if (!vtotal)
> >> >> >>> >> + return ulScanlineNo2;
> >> >> >>> >> +
> >> >> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh);
> >> >> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime -
> >> >ulPrevTime),
> >> >> >>> >> + ulScanlineTime);
> >> >> >>> >
> >> >> >>> >Something like:
> >> >> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
> >> >> >>> > 1000 * crtc_htotal);
> >> >> >>> >
> >> >> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
> >> >> >>> >
> >> >> >>> >I think that would have to be something like:
> >> >> >>> >return (scanline + vblank_start) % vtotal;
> >> >> >>> >
> >> >> >>>
> >> >> >>> Yes you are right. It should be vblank_start. Will fix this.
> >> >> >>>
> >> >> >>> >All in all this looks like a pretty decent approach to the DSI problem.
> >> >> >>> >
> >> >> >>> >One concern here is rounding issues and inaccuracies in our
> >> >> >>> >crtc_clock. But since the frame timestamp is sampled at vblank
> >> >> >>> >start I guess we can't accidentally get an answer that's
> >> >> >>> >earlier than vblank_start as long as we really passed vblank
> >> >> >>> >start already. That should
> >> >> >>make this at least suitable for vblank timestamps.
> >> >> >>>
> >> >> >>> I also feel the same, this situation should never occur.
> >> >> >>>
> >> >> >>> >And for
> >> >> >>> >the atomic evade, I guess if we clamp our the scanline before
> >> >> >>> >the
> >> >> >>> >+vblank_start such that it never reaches vtotal, we can't be
> >> >> >>> >+sure that
> >> >> >>> >our vblank evade never indicates that we already reached the
> >> >> >>> >start of vblank prematurely.
> >> >> >>> >
> >> >> >>> >So maybe something like:
> >> >> >>> >scaline = div_u64(...);
> >> >> >>> >scanline = min(scanline, vtotal - 1);
> >> >> >>>
> >> >> >>> I am not sure if the value of scanline returned can ever be
> >> >> >>> greater than vtotal -
> >> >> >>1.
> >> >> >>> But we can have a check just to be safe. Not sure if I fully
> >> >> >>> got your point
> >> >here.
> >> >> >>
> >> >> >>The point is that the timestamp counter might tick at a slightly
> >> >> >>faster rate than we might think. Thus we might end up with more
> >> >> >>ticks in one frame than what we calculated as the maximum fom
> >> >> >>crtc_clock etc. But if we clamp the value like I suggested then
> >> >> >>at least we should never get an answer that tells us we're
> >> >> >>already past the start of vblank when in reality
> >> >> >we're not.
> >> >> >>
> >> >> >>Of course as Daniel pointed out we might also get into trouble if
> >> >> >>the counter ticks slower than expected. That could lead us to
> >> >> >>think that we don't need to do the vblank evade when in fact we do.
> >> >> >>
> >> >>
> >> >> Hi Ville,
> >> >> We tried to test with this condition and are calculating wrong scanlines.
> >> >> For ex:
> >> >> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534,
> >> >crtc_vtotal-1 = 1211, min of two = 1211
> >> >
> >> >Well, that scanline number looks totally bogus. How did you calculate it
> >exactly?
> >> >
> >>
> >> If we have multiple scans on the same frame (no new flip being
> >> issued). Prev timestamp value which is read from Frametime Stamp will
> >> remain same, but current time stamp will keep on incrementing.
> >
> >The frame timestamp should get sampled on every vblank, whereas the flip
> >timestamp only when a flip occurs. Are you using the correct timestamp register?
> >
>
> Yes, we are using what is there in the patch.
> Name Pipe A Frame Time Stamp
> Symbol PIPE_FRMTMSTMP_A
> Start 0x70048
> End 0x7004B
>
> Its behaving as FLIP Timestamp though (not being updated on every vblank_start).
> Atleast with the readback what we get on APL.
Then it's broken and probably can't be used without having a decent idea
of how long the frame actually is. Which probably means we'd need
something like what Chris suggested.
Hmm. It's not a command mode display is it?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-12 15:06 ` Ville Syrjälä
@ 2017-09-13 8:24 ` Shankar, Uma
2017-09-13 17:36 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Shankar, Uma @ 2017-09-13 8:24 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, September 12, 2017 8:36 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Tue, Sep 12, 2017 at 02:21:42PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Tuesday, September 12, 2017 7:43 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
>> ><vidya.srinivas@intel.com>
>> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >
>> >On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >Sent: Tuesday, September 12, 2017 7:04 PM
>> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
>> >> ><vidya.srinivas@intel.com>
>> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >> >
>> >> >> >>>
>> >> >> >>> >-----Original Message-----
>> >> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >> >>> >Sent: Friday, September 8, 2017 8:18 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 for
>> >> >> >>> >gen9 dsi
>> >> >> >>> >
>> >> >> >>> >On Fri, Sep 08, 2017 at 07:18:55PM +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.
>> >> >> >>> >>
>> >> >> >>> >> 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 | 5 +++++
>> >> >> >>> >> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> >> >> >>> >> drivers/gpu/drm/i915/intel_dsi.c | 46
>> >> >> >>> >> ++++++++++++++++++++++++++++++++++++++++
>> >> >> >>> >> 4 files changed, 56 insertions(+)
>> >> >> >>> >>
>> >> >> >>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> >> >> >>> >> b/drivers/gpu/drm/i915/i915_drv.h index d07d110..4213b54
>> >> >> >>> >> 100644
>> >> >> >>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> >> >>> >> @@ -4077,6 +4077,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 bxt_dsi_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..31aa7f0
>> >> >> >>> >> 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;
>> >> >> >>> >> + enum transcoder cpu_transcoder;
>> >> >> >>> >>
>> >> >> >>> >> if (!crtc->active)
>> >> >> >>> >> return -1;
>> >> >> >>> >> @@ -792,6 +793,10 @@ static int
>> >> >> >>> >> __intel_get_crtc_scanline(struct intel_crtc
>> >> >> >>> >*crtc)
>> >> >> >>> >> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> >> >> >>> >> vtotal /= 2;
>> >> >> >>> >>
>> >> >> >>> >> + cpu_transcoder = crtc->config->cpu_transcoder;
>> >> >> >>> >
>> >> >> >>> >Humm. Would be nice to be able to do this without adding
>> >> >> >>> >more
>> >> >> >>> >crtc->config uses. We're pretty much trying to get rid of that guy.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Will try to find an alternate way to do this.
>> >> >> >>>
>> >> >> >>> >> + if (IS_BROXTON(dev_priv) &&
>transcoder_is_dsi(cpu_transcoder))
>> >> >> >>> >> + return bxt_dsi_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 9a73ea0..54582de
>> >> >> >>> >> 100644
>> >> >> >>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >>> >> @@ -8802,6 +8802,9 @@ enum skl_power_gate { #define
>> >> >> >>> >> MIPIO_TXESC_CLK_DIV2
>> >> > _MMIO(0x160008)
>> >> >> >>> >> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>> >> >> >>> >>
>> >> >> >>> >> +#define BXT_TIMESTAMP_CTR _MMIO(0x44070)
>> >> >> >>> >> +#define BXT_PIPE_FRMTMSTMP_A _MMIO(0x70048)
>> >> >> >>> >
>> >> >> >>> >Please add proper parametrized define that works for all pipes.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Will add that.
>> >> >> >>>
>> >> >> >>> >> +
>> >> >> >>> >> /* BXT MIPI clock controls */
>> >> >> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>> >> >> >>> >>
>> >> >> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> index 2a0f5d3..d145ba4 100644
>> >> >> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> @@ -1621,6 +1621,52 @@ static int
>> >> >> >>> >> intel_dsi_get_modes(struct
>> >> >> >>> >drm_connector *connector)
>> >> >> >>> >> return 1;
>> >> >> >>> >> }
>> >> >> >>> >>
>> >> >> >>> >> +/*
>> >> >> >>> >> + * 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 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>> >> >> >>> >> + struct drm_device *dev = crtc->base.dev;
>> >> >> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> >> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
>> >> >> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>> >> >> >>> >
>> >> >> >>> >Please get rid of the hungarian notation.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Yes, will fix this.
>> >> >> >>>
>> >> >> >>> >> + uint_fixed_16_16_t ulScanlineTime;
>> >> >> >>> >> +
>> >> >> >>> >> + /*
>> >> >> >>> >> + * This field provides read back of the display
>> >> >> >>> >> + * pipe frame time stamp. The time stamp value
>> >> >> >>> >> + * is sampled at every start of vertical blank.
>> >> >> >>> >> + */
>> >> >> >>> >> + ulPrevTime =
>I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>> >> >> >>> >> +
>> >> >> >>> >> + /*
>> >> >> >>> >> + * The TIMESTAMP_CTR register has the current
>> >> >> >>> >> + * time stamp value.
>> >> >> >>> >> + */
>> >> >> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>> >> >> >>> >> +
>> >> >> >>> >> + /* The PORT for DSI will always be 0 since
>> >> >> >>> >> + * isolated PORTC cannot be enabled for Gen9
>> >> >> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
>> >> >> >>> >> + * the VTOTAL value.
>> >> >> >>> >> + */
>> >> >> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>> >> >> >>> >
>> >> >> >>> >This value can be dug out from the hwmode.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Yes, will get it from hwmode and drop this change.
>> >> >> >>>
>> >> >> >>> >> + WARN_ON(!vtotal);
>> >> >> >>> >> + if (!vtotal)
>> >> >> >>> >> + return ulScanlineNo2;
>> >> >> >>> >> +
>> >> >> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal *
>vrefresh);
>> >> >> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime
>-
>> >> >ulPrevTime),
>> >> >> >>> >> +
> ulScanlineTime);
>> >> >> >>> >
>> >> >> >>> >Something like:
>> >> >> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
>> >> >> >>> > 1000 * crtc_htotal);
>> >> >> >>> >
>> >> >> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>> >> >> >>> >
>> >> >> >>> >I think that would have to be something like:
>> >> >> >>> >return (scanline + vblank_start) % vtotal;
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Yes you are right. It should be vblank_start. Will fix this.
>> >> >> >>>
>> >> >> >>> >All in all this looks like a pretty decent approach to the DSI problem.
>> >> >> >>> >
>> >> >> >>> >One concern here is rounding issues and inaccuracies in our
>> >> >> >>> >crtc_clock. But since the frame timestamp is sampled at
>> >> >> >>> >vblank start I guess we can't accidentally get an answer
>> >> >> >>> >that's earlier than vblank_start as long as we really
>> >> >> >>> >passed vblank start already. That should
>> >> >> >>make this at least suitable for vblank timestamps.
>> >> >> >>>
>> >> >> >>> I also feel the same, this situation should never occur.
>> >> >> >>>
>> >> >> >>> >And for
>> >> >> >>> >the atomic evade, I guess if we clamp our the scanline
>> >> >> >>> >before the
>> >> >> >>> >+vblank_start such that it never reaches vtotal, we can't
>> >> >> >>> >+be sure that
>> >> >> >>> >our vblank evade never indicates that we already reached
>> >> >> >>> >the start of vblank prematurely.
>> >> >> >>> >
>> >> >> >>> >So maybe something like:
>> >> >> >>> >scaline = div_u64(...);
>> >> >> >>> >scanline = min(scanline, vtotal - 1);
>> >> >> >>>
>> >> >> >>> I am not sure if the value of scanline returned can ever be
>> >> >> >>> greater than vtotal -
>> >> >> >>1.
>> >> >> >>> But we can have a check just to be safe. Not sure if I fully
>> >> >> >>> got your point
>> >> >here.
>> >> >> >>
>> >> >> >>The point is that the timestamp counter might tick at a
>> >> >> >>slightly faster rate than we might think. Thus we might end up
>> >> >> >>with more ticks in one frame than what we calculated as the
>> >> >> >>maximum fom crtc_clock etc. But if we clamp the value like I
>> >> >> >>suggested then at least we should never get an answer that
>> >> >> >>tells us we're already past the start of vblank when in
>> >> >> >>reality
>> >> >> >we're not.
>> >> >> >>
>> >> >> >>Of course as Daniel pointed out we might also get into trouble
>> >> >> >>if the counter ticks slower than expected. That could lead us
>> >> >> >>to think that we don't need to do the vblank evade when in fact we do.
>> >> >> >>
>> >> >>
>> >> >> Hi Ville,
>> >> >> We tried to test with this condition and are calculating wrong scanlines.
>> >> >> For ex:
>> >> >> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534,
>> >> >crtc_vtotal-1 = 1211, min of two = 1211
>> >> >
>> >> >Well, that scanline number looks totally bogus. How did you
>> >> >calculate it
>> >exactly?
>> >> >
>> >>
>> >> If we have multiple scans on the same frame (no new flip being
>> >> issued). Prev timestamp value which is read from Frametime Stamp
>> >> will remain same, but current time stamp will keep on incrementing.
>> >
>> >The frame timestamp should get sampled on every vblank, whereas the
>> >flip timestamp only when a flip occurs. Are you using the correct timestamp
>register?
>> >
>>
>> Yes, we are using what is there in the patch.
>> Name Pipe A Frame Time Stamp
>> Symbol PIPE_FRMTMSTMP_A
>> Start 0x70048
>> End 0x7004B
>>
>> Its behaving as FLIP Timestamp though (not being updated on every
>vblank_start).
>> Atleast with the readback what we get on APL.
>
>Then it's broken and probably can't be used without having a decent idea of how
>long the frame actually is. Which probably means we'd need something like what
>Chris suggested.
>
Hi Ville,
On further experiments we figured out that, frame time stamp is not updated if the
vblank interrupt gets disabled (which is currently controlled through vblank get and put).
We tried to forcefully enable vblank interrupt by doing an extra vblank get during crtc_enable.
By doing this, we see that frame timestamp is updating at every vblank.
So not sure what should be the best approach to deal with this. I don't think, keeping
vblank interrupt enabled always is a good idea. Ideally frame time stamp should get updated
even if no physical vblank interrupt is coming, or is the behavior expected ?
>Hmm. It's not a command mode display is it?
>
No, it's a single link video mode panel. Command Mode is not even enabled in upstream as of now.
Regards,
Uma Shankar
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-13 8:24 ` Shankar, Uma
@ 2017-09-13 17:36 ` Ville Syrjälä
2017-09-14 11:47 ` Shankar, Uma
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-09-13 17:36 UTC (permalink / raw)
To: Shankar, Uma; +Cc: intel-gfx, Srinivas, Vidya
On Wed, Sep 13, 2017 at 08:24:38AM +0000, Shankar, Uma wrote:
>
>
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, September 12, 2017 8:36 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >
> >On Tue, Sep 12, 2017 at 02:21:42PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Tuesday, September 12, 2017 7:43 PM
> >> >To: Shankar, Uma <uma.shankar@intel.com>
> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
> >> ><vidya.srinivas@intel.com>
> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >> >
> >> >On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
> >> >> If we have multiple scans on the same frame (no new flip being
> >> >> issued). Prev timestamp value which is read from Frametime Stamp
> >> >> will remain same, but current time stamp will keep on incrementing.
> >> >
> >> >The frame timestamp should get sampled on every vblank, whereas the
> >> >flip timestamp only when a flip occurs. Are you using the correct timestamp
> >register?
> >> >
> >>
> >> Yes, we are using what is there in the patch.
> >> Name Pipe A Frame Time Stamp
> >> Symbol PIPE_FRMTMSTMP_A
> >> Start 0x70048
> >> End 0x7004B
> >>
> >> Its behaving as FLIP Timestamp though (not being updated on every
> >vblank_start).
> >> Atleast with the readback what we get on APL.
> >
> >Then it's broken and probably can't be used without having a decent idea of how
> >long the frame actually is. Which probably means we'd need something like what
> >Chris suggested.
> >
>
> Hi Ville,
> On further experiments we figured out that, frame time stamp is not updated if the
> vblank interrupt gets disabled (which is currently controlled through vblank get and put).
> We tried to forcefully enable vblank interrupt by doing an extra vblank get during crtc_enable.
> By doing this, we see that frame timestamp is updating at every vblank.
Well, that's rather unfortuante. I guess we'd have to keep the vblank
interrupt unmasked all the time. Hopefully we could still disable it in
IER.
So we'd unmask the interrupt permanently, and just toggle the IER bit as
needed. That should be doable but somewhat annoying because it's exactly
the opposite of what we do normally.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
2017-09-13 17:36 ` Ville Syrjälä
@ 2017-09-14 11:47 ` Shankar, Uma
0 siblings, 0 replies; 27+ messages in thread
From: Shankar, Uma @ 2017-09-14 11:47 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Srinivas, Vidya
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, September 13, 2017 11:07 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Wed, Sep 13, 2017 at 08:24:38AM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Tuesday, September 12, 2017 8:36 PM
>> >To: Shankar, Uma <uma.shankar@intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
>> ><vidya.srinivas@intel.com>
>> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >
>> >On Tue, Sep 12, 2017 at 02:21:42PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >> >Sent: Tuesday, September 12, 2017 7:43 PM
>> >> >To: Shankar, Uma <uma.shankar@intel.com>
>> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
>> >> ><vidya.srinivas@intel.com>
>> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >> >
>> >> >On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
>> >> >> If we have multiple scans on the same frame (no new flip being
>> >> >> issued). Prev timestamp value which is read from Frametime Stamp
>> >> >> will remain same, but current time stamp will keep on incrementing.
>> >> >
>> >> >The frame timestamp should get sampled on every vblank, whereas
>> >> >the flip timestamp only when a flip occurs. Are you using the
>> >> >correct timestamp
>> >register?
>> >> >
>> >>
>> >> Yes, we are using what is there in the patch.
>> >> Name Pipe A Frame Time Stamp
>> >> Symbol PIPE_FRMTMSTMP_A
>> >> Start 0x70048
>> >> End 0x7004B
>> >>
>> >> Its behaving as FLIP Timestamp though (not being updated on every
>> >vblank_start).
>> >> Atleast with the readback what we get on APL.
>> >
>> >Then it's broken and probably can't be used without having a decent
>> >idea of how long the frame actually is. Which probably means we'd
>> >need something like what Chris suggested.
>> >
>>
>> Hi Ville,
>> On further experiments we figured out that, frame time stamp is not
>> updated if the vblank interrupt gets disabled (which is currently controlled
>through vblank get and put).
>> We tried to forcefully enable vblank interrupt by doing an extra vblank get
>during crtc_enable.
>> By doing this, we see that frame timestamp is updating at every vblank.
>
>Well, that's rather unfortuante. I guess we'd have to keep the vblank interrupt
>unmasked all the time. Hopefully we could still disable it in IER.
>
>So we'd unmask the interrupt permanently, and just toggle the IER bit as needed.
>That should be doable but somewhat annoying because it's exactly the opposite
>of what we do normally.
>
We tried controlling Vblank through IER, keeping IMR always unmasked as you suggested.
This is causing frame time stamp properly getting updated at every vblank. We will send the
patch for the same for review. Thanks Ville for your suggestion.
Regards,
Uma Shankar
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-09-19 2:32 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-18 13:41 [PATCH] drm/i915: Enable scanline read for gen9 dsi Vidya Srinivas
2017-09-18 16:49 ` ✗ Fi.CI.BAT: failure for drm/i915: Enable scanline read for gen9 dsi (rev5) Patchwork
2017-09-19 2:31 ` [PATCH] drm/i915: Enable scanline read for gen9 dsi kbuild test robot
-- strict thread matches above, loose matches on Subject: below --
2017-09-08 13:48 Vidya Srinivas
2017-09-08 14:47 ` Ville Syrjälä
2017-09-08 14:55 ` Ville Syrjälä
2017-09-08 15:25 ` Saarinen, Jani
2017-09-08 19:45 ` Daniel Vetter
2017-09-08 19:55 ` Chris Wilson
2017-09-11 8:52 ` Maarten Lankhorst
2017-09-11 12:21 ` Ville Syrjälä
2017-09-11 13:19 ` Shankar, Uma
2017-09-11 18:21 ` Ville Syrjälä
2017-09-12 9:32 ` Shankar, Uma
2017-09-11 13:04 ` Shankar, Uma
2017-09-11 17:50 ` Ville Syrjälä
2017-09-12 9:50 ` Shankar, Uma
2017-09-12 13:23 ` Shankar, Uma
2017-09-12 13:33 ` Ville Syrjälä
2017-09-12 13:40 ` Shankar, Uma
2017-09-12 13:55 ` Vidya Srinivas
2017-09-12 14:12 ` Ville Syrjälä
2017-09-12 14:21 ` Shankar, Uma
2017-09-12 15:06 ` Ville Syrjälä
2017-09-13 8:24 ` Shankar, Uma
2017-09-13 17:36 ` Ville Syrjälä
2017-09-14 11:47 ` Shankar, Uma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.