Re: [Intel-gfx] [PATCH v2] drm/i915: Fix MST disable sequence
On Fri, Jan 10, 2020 at 06:40:38PM +, Souza, Jose wrote: > On Wed, 2020-01-08 at 18:20 +0200, Ville Syrjälä wrote: > > On Wed, Jan 08, 2020 at 04:09:31PM +, Souza, Jose wrote: > > > On Wed, 2020-01-08 at 16:45 +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä > > > > > > > > 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 > > > > Cc: Manasi Navare > > > > Reported-by: Stanislav Lisovskiy > > > > 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ä > > > > --- > > > > 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(_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'm thinking we should start looking at the enable side too so that we can come up with some kind of scheme that isn't totally out of sync for enable vs. disable. -- Ville Syrjälä Intel ___ Intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Fix MST disable sequence
On Wed, 2020-01-08 at 18:20 +0200, Ville Syrjälä wrote: > On Wed, Jan 08, 2020 at 04:09:31PM +, Souza, Jose wrote: > > On Wed, 2020-01-08 at 16:45 +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > > > 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 > > > Cc: Manasi Navare > > > Reported-by: Stanislav Lisovskiy > > > 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ä > > > --- > > > 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(_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 > > Ta. > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Fix MST disable sequence
On Wed, Jan 08, 2020 at 04:09:31PM +, Souza, Jose wrote: > On Wed, 2020-01-08 at 16:45 +0200, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > 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 > > Cc: Manasi Navare > > Reported-by: Stanislav Lisovskiy > > 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ä > > --- > > 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(_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. > > 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 Ta. -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Fix MST disable sequence
On Wed, 2020-01-08 at 16:45 +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > 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 > Cc: Manasi Navare > Reported-by: Stanislav Lisovskiy > 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ä > --- > 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(_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 > > /* >* When called from DP MST code: ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Fix MST disable sequence
From: Ville Syrjälä 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. 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 Cc: Manasi Navare Reported-by: Stanislav Lisovskiy 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ä --- 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); + } /* * When called from DP MST code: -- 2.24.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx