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

[sashiko-bot@kernel.org]

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()?

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