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

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2020-01-08 at 18:20 +0200, Ville Syrjälä wrote:
> On Wed, Jan 08, 2020 at 04:09:31PM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-08 at 16:45 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@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@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Reported-by: Stanislav Lisovskiy <stanislav.lisovskiy@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@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);
> 
> Yeah, the current way is a bit of a mess. We probably want to think
> of
> ways to make it less sucky.

Can I go forward and implement the above and undoing this patch?

> 
> > I guess this goes more with changes that you did in the patch
> > fixed.
> 
> Indeed, a more mechanichal change for now seems more in line with the
> original patch.
> 
> > 
> > Anyway your changes looks good.
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Ta.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx