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