[Intel-gfx] [PATCH v2] drm/i915: Fix MST disable sequence

[Souza, Jose]

On Wed, 2020-01-08 at 16:45 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> When moving the pipe disable & co. function calls from
> haswell_crtc_disable() into the encoder .post_disable() hooks I
> neglected to account for the MST vs. DDI interactions properly.
> This now leads us to call these functions two times for the last
> MST stream (once from the MST code and a second time from the DDI
> code). The calls from the DDI code should only be done for SST
> and not MST. Add the proper check for that.

Oohh I forgot that too.

> 
> This results in an MCE on ICL. My vague theory is that we turn off
> the transcoder clock from the MST code and then we proceed to touch
> something in the DDI code which still depends on that clock causing
> the hardware to become upset. Though I can't really explain why
> Stan's hack of omitting the pipe disable in the MST code would avoid
> the MCE since we should still be turning off the transcoder clock.
> But maybe there's something magic in the hw that keeps the clock on
> as long as the pipe is on. Or maybe the clock isn't the problem and
> we now touch something in the DDI disable code that really does need
> the pipe to be still enabled.
> 
> v2: Rebase to latest drm-tip
> 
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Reported-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/901
> Fixes: 773b4b54351c ("drm/i915: Move stuff from
> haswell_crtc_disable() into encoder .post_disable()")
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 07acd0daca25..6e0a75d1e6ca 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3897,21 +3897,23 @@ static void intel_ddi_post_disable(struct
> intel_encoder *encoder,
>  	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>  
> -	intel_crtc_vblank_off(old_crtc_state);
> +	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST))
> {
> +		intel_crtc_vblank_off(old_crtc_state);
>  
> -	intel_disable_pipe(old_crtc_state);
> +		intel_disable_pipe(old_crtc_state);
>  
> -	if (INTEL_GEN(dev_priv) >= 11)
> -		icl_disable_transcoder_port_sync(old_crtc_state);
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			icl_disable_transcoder_port_sync(old_crtc_state
> );
>  
> -	intel_ddi_disable_transcoder_func(old_crtc_state);
> +		intel_ddi_disable_transcoder_func(old_crtc_state);
>  
> -	intel_dsc_disable(old_crtc_state);
> +		intel_dsc_disable(old_crtc_state);
>  
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		skl_scaler_disable(old_crtc_state);
> -	else
> -		ilk_pfit_disable(old_crtc_state);
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			skl_scaler_disable(old_crtc_state);
> +		else
> +			ilk_pfit_disable(old_crtc_state);
> +	}


Other option would be replace
intel_dig_port->base.post_disable(&intel_dig_port->base,
old_crtc_state, NULL);
in intel_mst_post_disable_dp() by:


intel_ddi_post_disable_dp(encoder, old_crtc_state, old_conn_state);

if (INTEL_GEN(dev_priv) >= 11)
	icl_unmap_plls_to_ports(encoder);

if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port)
	intel_display_power_put_unchecked(dev_priv,
intel_ddi_main_link_aux_domain(dig_port));

if (is_tc_port)
	intel_tc_port_put_link(dig_port);

I guess this goes more with changes that you did in the patch fixed.


Anyway your changes looks good.

Reviewed-by: José Roberto de Souza <jose.souza at intel.com>


>  
>  	/*
>  	 * When called from DP MST code: