Re: [PATCH 06/10] drm/i915: Wait for timing generator to start in intel_crtc_vblank_on() on HSW+

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915: intel_pipe_vblank_on() scanline wait times out on DSI
--

commit a024447aed84d4ab8f0e7bb2ee11d21475e5b408
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm/i915: Wait for timing generator to start in intel_crtc_vblank_on() on HSW+

This commit waits for the pipe scanline counter to start moving inside
intel_pipe_vblank_on() on Haswell and newer platforms before enabling
vblank interrupts.

> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index a28f4a088e587..209d5a18ee8d9 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -128,8 +128,21 @@ u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
>  
>  void intel_pipe_vblank_on(const struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> +	/*
> +	 * Make sure the timing generator has really started so
> +	 * the sampled frame/scanline counter values don't jump
> +	 * erratically.
> +	 *
> +	 * TODO do this for older platforms as well after
> +	 * rearranging the modeset sequence a bit.
> +	 */
> +	if (DISPLAY_VER(display) >= 9 ||
> +	    display->platform.broadwell || display->platform.haswell)
> +		intel_wait_for_pipe_scanline_moving(crtc);

[Severity: Medium]
Will calling intel_wait_for_pipe_scanline_moving() unconditionally on
DISPLAY_VER(display) >= 9 time out for 100 ms and log a false error on
Broxton/Geminilake DSI and Gen11+ DSI command mode?

intel_wait_for_pipe_scanline_moving() calls wait_for_pipe_scanline_moving() ->
pipe_scanline_is_moving(), which directly polls PIPEDSL:

drivers/gpu/drm/i915/display/intel_vblank.c:pipe_scanline_is_moving() {
	intel_reg_t reg = PIPEDSL(display, pipe);
	u32 line1, line2;

	line1 = intel_de_read(display, reg) & PIPEDSL_LINE_MASK;
	msleep(5);
	line2 = intel_de_read(display, reg) & PIPEDSL_LINE_MASK;

	return line1 != line2;
}

On Broxton (DISPLAY_VER 9) and Geminilake (DISPLAY_VER 10) MIPI DSI, timings
are driven from the port rather than the CPU transcoder, and PIPEDSL does not
increment. Because of this, intel_dsi_compute_config() and
bxt_dsi_get_pipe_config() set I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP, and
__intel_get_crtc_scanline() skips reading PIPEDSL:

drivers/gpu/drm/i915/display/intel_vblank.c:__intel_get_crtc_scanline() {
	if (crtc->mode_flags & I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)
		return __intel_get_crtc_scanline_from_timestamp(crtc);

	vtotal = intel_mode_vtotal(mode);

	position = intel_de_read_fw(display, PIPEDSL(display, pipe)) & PIPEDSL_LINE_MASK;
	...
}

When bxt_dsi_enable() -> intel_crtc_vblank_on() -> intel_pipe_vblank_on() runs
(or during intel_modeset_setup_hw_state()), polling PIPEDSL will see a static
value and time out.

Similarly, on Gen11+ DSI command mode (where I915_MODE_FLAG_DSI_USE_TE0 or
I915_MODE_FLAG_DSI_USE_TE1 is set in crtc_state->mode_flags),
gen11_dsi_configure_transcoder() configures the transcoder in CMD_MODE_TE_GATE
without periodic updates. When gen11_dsi_enable() calls intel_crtc_vblank_on()
during intel_enable_crtc(), no frame update has been requested yet because
icl_dsi_frame_update() is only called later in intel_update_crtc() ->
intel_pipe_update_end():

drivers/gpu/drm/i915/display/intel_crtc.c:intel_pipe_update_end() {
	...
	if (DISPLAY_VER(display) >= 11 &&
	    intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI))
		icl_dsi_frame_update(new_crtc_state);
	...
}

Should intel_pipe_vblank_on() skip intel_wait_for_pipe_scanline_moving() when
crtc_state->mode_flags has I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP or
I915_MODE_FLAG_DSI_USE_TE0 / I915_MODE_FLAG_DSI_USE_TE1 set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030171327.29829-1-ville.syrjala@linux.intel.com?part=6