Re: [PATCH 03/10] drm/i915: Handle joined pipes inside intel_crtc_vblank_{on, off}()

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

On Fri, Oct 30, 2026 at 05:26:33PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] drm/i915: NULL state dereference in intel_crtc_vblank_off()
> --
> 
> commit c6499a1bf58e345d4e50ee3b23d7821c66712bdd
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm/i915: Handle joined pipes inside intel_crtc_vblank_{on, off}()
> 
> This commit moves the joined-pipe iteration into intel_crtc_vblank_on()
> and intel_crtc_vblank_off(), renaming the single-pipe helpers to
> intel_pipe_vblank_on() and intel_pipe_vblank_off(). Callers are updated to
> pass struct intel_atomic_state alongside the CRTC state.
> 
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index ea0cdb7822f39..381cdf7d5131c 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> 
> [ ... ]
> 
> > @@ -1455,7 +1455,7 @@ static void gen11_dsi_post_disable(struct intel_atomic_state *state,
> >  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >  
> > -	intel_crtc_vblank_off(old_crtc_state);
> > +	intel_crtc_vblank_off(state, old_crtc_state);
> >  
> >  	/* step2d,e: disable transcoder and wait */
> >  	gen11_dsi_disable_transcoder(encoder);
> > diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> > index 5b8968197fbcf..b814b84c650fd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> > @@ -265,7 +265,7 @@ static void hsw_post_disable_crt(struct intel_atomic_state *state,
> >  	struct intel_display *display = to_intel_display(encoder);
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >  
> > -	intel_crtc_vblank_off(old_crtc_state);
> > +	intel_crtc_vblank_off(state, old_crtc_state);
> >  
> >  	intel_disable_transcoder(old_crtc_state);
> >  
> 
> [ ... ]
> 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index 10ed9bdfee763..a28f4a088e587 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> 
> [ ... ]
> 
> > @@ -165,6 +165,34 @@ void intel_crtc_vblank_off(const struct intel_crtc_state *crtc_state)
> >  	flush_work(&display->irq.vblank_notify_work);
> >  }
> >  
> > +void intel_crtc_vblank_on(struct intel_atomic_state *state,
> > +			  const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_display *display = to_intel_display(state);
> > +	struct intel_crtc *pipe_crtc;
> > +
> > +	for_each_pipe_crtc_modeset_enable(display, pipe_crtc, crtc_state) {
> > +		const struct intel_crtc_state *pipe_crtc_state =
> > +			intel_atomic_get_new_crtc_state(state, pipe_crtc);
> > +
> > +		intel_pipe_vblank_on(pipe_crtc_state);
> > +	}
> > +}
> > +
> > +void intel_crtc_vblank_off(struct intel_atomic_state *state,
> > +			   const struct intel_crtc_state *old_crtc_state)
> > +{
> > +	struct intel_display *display = to_intel_display(state);
> > +	struct intel_crtc *pipe_crtc;
> > +
> > +	for_each_pipe_crtc_modeset_disable(display, pipe_crtc, old_crtc_state) {
> > +		const struct intel_crtc_state *old_pipe_crtc_state =
> > +			intel_atomic_get_old_crtc_state(state, pipe_crtc);
> > +
> > +		intel_pipe_vblank_off(old_pipe_crtc_state);
> > +	}
> > +}
> 
> [Severity: High]
> Can state be NULL when intel_crtc_vblank_off() is called from
> encoder->post_disable()?
> 
> During initial hardware readout or resume in
> intel_modeset_setup_hw_state(), intel_sanitize_encoder() disables an
> encoder whose connector is active while its pipe is inactive by invoking
> the disable hooks with a NULL atomic state:
> 
> drivers/gpu/drm/i915/display/intel_modeset_setup.c:intel_sanitize_encoder() {
> 	...
> 	if (connector && !has_active_crtc) {
> 		...
> 		if (crtc_state) {
> 			...
> 			/* FIXME NULL atomic state passed! */
> 			if (encoder->disable)
> 				encoder->disable(NULL, encoder, crtc_state,
> 						 connector->base.state);
> 			if (encoder->post_disable)
> 				encoder->post_disable(NULL, encoder, crtc_state,
> 						      connector->base.state);
> 		}
> 	}
> }
> 
> Previously, gen11_dsi_post_disable(), intel_dsi_post_disable() (on
> Broxton and Geminilake), and hsw_post_disable_crt() did not dereference
> state and passed old_crtc_state directly to intel_crtc_vblank_off(). With
> this change, those callbacks pass state = NULL to intel_crtc_vblank_off(),
> which dereferences state in both to_intel_display(state) and
> intel_atomic_get_old_crtc_state(state, pipe_crtc).
> 
> Would it make sense to keep using intel_pipe_vblank_off(old_crtc_state) in
> the non-joiner post_disable callbacks, or handle a NULL state inside
> intel_crtc_vblank_off()?

I thought about it but apparently forgot to note it in the commit
message; My conclusion was that the encoder sanitize stuff should
never happen on the platforms where we do the vblank_off() from the
encoder hooks. On earlier platforms the vblank_off() is in
.crtc_disable() and intel_crtc_disable_noatomic_begin() creates
a temporary atomic state for that.

I've also occasionally pondered about creating a temporary
atomic state for intel_sanitize_encoder() as well, just to get
rid of even the theoretical possibility of a NULL state. But
that would probably involve some actual thinking...

-- 
Ville Syrjälä
Intel