On Tue, Sep 19, 2017 at 02:50:03PM +0530, Vidya Srinivas wrote:
> From: Uma Shankar <uma.shan...@intel.com>
>
> For certain platforms on certain encoders, timings are driven
> from port instead of pipe. Thus, we can't rely on pipe scanline
> registers to get the timing information. Some cases scanline
> register read may not be functional due to certain hw issues.
> This is causing vblank evasion logic to fail since it relies on
> scanline, causing atomic update failure warnings.
>
> This patch uses pipe framestamp and current timestamp registers
> to calculate scanline. This is an indirect way to get the scanline.
> It helps resolve atomic update failure for gen9 dsi platforms.
>
> v2: Addressed Ville and Daniel's review comments. Updated the
> register MACROs, handled race condition for register reads,
> extracted timings from the hwmode. Removed the dependency on
> crtc->config to get the encoder type.
>
> v3: Made get scanline function generic
>
> v4: Addressed Ville and Maarten's review comments. Used vblank
> hwmode to get the timings. Added a flag to decide timestamp
> based scanline reporting. Changed 64bit variables to u32
The patch subject is missing the 'v4'. Which is perhaps why
I didn't even notice this sitting in my inbox. Hint for next time ;)
>
> Credits-to: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shan...@intel.com>
> Signed-off-by: Chandra Konduru <chandra.kond...@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srini...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/i915_irq.c | 4 +++
> drivers/gpu/drm/i915/i915_reg.h | 9 +++++++
> drivers/gpu/drm/i915/intel_display.c | 51
> ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++
> include/uapi/drm/drm_mode.h | 3 +++
> 6 files changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 28ad5da..eea374d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4085,6 +4085,8 @@ void intel_sbi_write(struct drm_i915_private *dev_priv,
> u16 reg, u32 value,
> u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
> void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>
> +u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc);
We have just one caller for this, so it should just live next to that
caller.
> +
> /* intel_dpio_phy.c */
> void bxt_port_to_phy_channel(struct drm_i915_private *dev_priv, enum port
> port,
> enum dpio_phy *phy, enum dpio_channel *ch);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 003a928..ccde6c2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -825,6 +825,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc
> *crtc)
> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> vtotal /= 2;
>
> + if (mode->flags &
> + DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)
Ahem, private_flags. The fact that you had to modify a uapi header for
a flag that's only used internally should have have been a red flag
(no pun intentded).
Also indentation is off.
> + return __intel_get_crtc_scanline_from_timestamp(crtc);
We don't need the vtotal value we computed above, so I think it would
be less confusing if you do this while thing before we compute vtotal.
> +
> if (IS_GEN2(dev_priv))
> position = I915_READ_FW(PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
> else
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 94b40a4..8afb14d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8806,6 +8806,15 @@ enum skl_power_gate {
> #define MIPIO_TXESC_CLK_DIV2 _MMIO(0x160008)
> #define GLK_TX_ESC_CLK_DIV2_MASK 0x3FF
>
> +/* Gen4+ Timestamp and Pipe Frame time stamp registers */
> +#define GEN4_TIMESTAMP 0x2358
> +#define ILK_TIMESTAMP_HI 0x70070
_MMIO missing from those two.
> +#define IVB_TIMESTAMP_CTR _MMIO(0x44070)
> +
> +#define _PIPE_FRMTMSTMP_A 0x70048
> +#define PIPE_FRMTMSTMP(pipe) \
> + _MMIO_PIPE2(pipe, _PIPE_FRMTMSTMP_A)
> +
> /* BXT MIPI clock controls */
> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 8599e42..c3e86f3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10353,6 +10353,57 @@ static bool needs_scaling(const struct
> intel_plane_state *state)
> return (src_w != dst_w || src_h != dst_h);
> }
>
> +/*
> + * On certain encoders on certain platforms, pipe
> + * scanline register will not work to get the scanline,
> + * since the timings are driven from the PORT or issues
> + * with scanline register updates.
> + * This function will use Framestamp and current
> + * timestamp registers to calculate the scanline.
> + */
> +u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + struct drm_vblank_crtc *vblank =
> + &crtc->base.dev->vblank[drm_crtc_index(&crtc->base)];
> + const struct drm_display_mode *mode =
> + &vblank->hwmode;
> + u32 crtc_vblank_start = mode->crtc_vblank_start;
> + u32 crtc_vtotal = mode->crtc_vtotal;
> + u32 crtc_htotal = mode->crtc_htotal;
> + u32 crtc_clock = mode->crtc_clock;
You can leave the crtc_ prefix out from your local variables. It doesn't
provide us with any useful infromation here, and we don't have it
elsewhere in the vbl timestamp/scanline code either. One should almost
always try to follow existing conventions in the code so that people
don't have to wonder why things happen to different this time around.
> + u32 scanline, scan_prev_time, scan_curr_time, scan_post_time;
> +
> + /* To avoid the race condition where we might cross into the
> + * next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR
> + * reads. We make sure we read PIPE_FRMTMSTMP and TIMESTAMP_CTR
> + * during the same frame.
> + */
> + do {
> + /*
> + * This field provides read back of the display
> + * pipe frame time stamp. The time stamp value
> + * is sampled at every start of vertical blank.
> + */
> + scan_prev_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc->pipe));
> +
> + /*
> + * The TIMESTAMP_CTR register has the current
> + * time stamp value.
> + */
> + scan_curr_time = I915_READ_FW(IVB_TIMESTAMP_CTR);
> +
> + scan_post_time = I915_READ_FW(PIPE_FRMTMSTMP(crtc->pipe));
> + } while (scan_post_time != scan_prev_time);
> +
> + scanline = div_u64(mul_u32_u32(scan_curr_time - scan_prev_time,
> + crtc_clock), 1000 * crtc_htotal);
Hmm. Indentation seems a bit off again. You really should do something
about your editor...
> + scanline = min(scanline, crtc_vtotal - 1);
> + scanline = (scanline + crtc_vblank_start) % crtc_vtotal;
> +
> + return scanline;
> +}
> +
> int intel_plane_atomic_calc_changes(const struct intel_crtc_state
> *old_crtc_state,
> struct drm_crtc_state *crtc_state,
> const struct intel_plane_state
> *old_plane_state,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 578254a..dd27c19 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -329,6 +329,11 @@ static bool intel_dsi_compute_config(struct
> intel_encoder *encoder,
> /* DSI uses short packets for sync events, so clear mode flags for DSI
> */
> adjusted_mode->flags = 0;
>
> + /* Add flag to enable scanline read using frametimestamp */
> + if (IS_GEN9_LP(dev_priv))
> + adjusted_mode->flags =
> + DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
> +
> if (IS_GEN9_LP(dev_priv)) {
> /* Dual link goes to DSI transcoder A. */
> if (intel_dsi->ports == BIT(PORT_C))
> @@ -1102,6 +1107,10 @@ static void bxt_dsi_get_pipe_config(struct
> intel_encoder *encoder,
> pixel_format_from_register_bits(fmt));
> bpp = pipe_config->pipe_bpp;
>
> + /* Enable Frame time stamp based scanline reporting */
> + adjusted_mode->flags |=
> + DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
> +
> /* In terms of pixels */
> adjusted_mode->crtc_hdisplay =
> I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 34b6bb3..17f4d51 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -85,6 +85,9 @@
> #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14)
> #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF (8<<14)
>
> +/* Flag to get scanline using frame time stamps */
> +#define DRM_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<18)
> +
> /* Picture aspect ratio options */
> #define DRM_MODE_PICTURE_ASPECT_NONE 0
> #define DRM_MODE_PICTURE_ASPECT_4_3 1
> --
> 1.9.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
