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@xxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx> > > > Reported-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx> > > > 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@xxxxxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxx> > > Ta. > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
