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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 30, 2026 at 05:32:47PM +0000, sashiko-bot@kernel.org wrote:
> 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?

DSI, sigh. 

I suppose for I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP we could
just wait for the frame timestamp to change. Or perhaps even use
that approach unconditionally for all platforms that have frame
timestamps. But I'll probably need to reverse engineer the actual
behavior of the frame timestamp during timing generator startup...

For now I guess I could just skip this for anything where
the scanline counter supposedly doesn't work.

-- 
Ville Syrjälä
Intel