| Message ID | 20240520185822.3725844-7-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp_mst: Enable link training fallback | expand |
On Mon, May 20, 2024 at 09:58:04PM +0300, Imre Deak wrote: > Instead of direct calls to the link train functions, retrain the link > via a commit modeset. The direct call means that the output port will be > disabled/re-enabled while the rest of the pipeline (transcoder) is > active, which doesn't seem to work on MST at least. It leads to > underruns and black screen, presumedly because the transcoder is not > disabled/re-enabled along the port. > > Leave switching to a commit modeset on SST for a later patchset, as that > seems to work ok currently (though better to using a commit there too, > due to the suppressed underruns). > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 81e620dd33bb7..120f7b420807b 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5147,6 +5147,7 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > struct intel_crtc *crtc; > u8 pipe_mask; > + bool mst_output = false; nit: maybe move that up one line to maintain a bit more of a steady slope > int ret; > > if (!intel_dp_is_connected(intel_dp)) > @@ -5177,6 +5178,11 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state = > to_intel_crtc_state(crtc->base.state); > > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > + mst_output = true; > + break; > + } I was pondering if we need a bit more care to make sure all the pipes agree, but I suppose if that wasn't the case check_digital_port_conflicts() would have a failed at its job. So this seems fine. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > /* Suppress underruns caused by re-training */ > intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > if (crtc_state->has_pch_encoder) > @@ -5184,16 +5190,23 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > intel_crtc_pch_transcoder(crtc), false); > } > > + /* TODO: use a modeset for SST as well. */ > + if (mst_output) { > + ret = intel_modeset_commit_pipes(dev_priv, pipe_mask, ctx); > + > + if (ret && ret != -EDEADLK) > + drm_dbg_kms(&dev_priv->drm, > + "[ENCODER:%d:%s] link retraining failed: %pe\n", > + encoder->base.base.id, encoder->base.name, > + ERR_PTR(ret)); > + > + return ret; > + } > + > for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) { > const struct intel_crtc_state *crtc_state = > to_intel_crtc_state(crtc->base.state); > > - /* retrain on the MST master transcoder */ > - if (DISPLAY_VER(dev_priv) >= 12 && > - intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && > - !intel_dp_mst_is_master_trans(crtc_state)) > - continue; > - > intel_dp_check_frl_training(intel_dp); > intel_dp_pcon_dsc_configure(intel_dp, crtc_state); > intel_dp_start_link_train(intel_dp, crtc_state); > -- > 2.43.3
On Thu, May 23, 2024 at 03:58:30PM +0300, Ville Syrjälä wrote: > On Mon, May 20, 2024 at 09:58:04PM +0300, Imre Deak wrote: > > Instead of direct calls to the link train functions, retrain the link > > via a commit modeset. The direct call means that the output port will be > > disabled/re-enabled while the rest of the pipeline (transcoder) is > > active, which doesn't seem to work on MST at least. It leads to > > underruns and black screen, presumedly because the transcoder is not > > disabled/re-enabled along the port. > > > > Leave switching to a commit modeset on SST for a later patchset, as that > > seems to work ok currently (though better to using a commit there too, > > due to the suppressed underruns). > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++++++++++------ > > 1 file changed, 19 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 81e620dd33bb7..120f7b420807b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -5147,6 +5147,7 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > struct intel_crtc *crtc; > > u8 pipe_mask; > > + bool mst_output = false; > > nit: maybe move that up one line to maintain a bit more of a steady slope Ok. > > int ret; > > > > if (!intel_dp_is_connected(intel_dp)) > > @@ -5177,6 +5178,11 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state = > > to_intel_crtc_state(crtc->base.state); > > > > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > > + mst_output = true; > > + break; > > + } > > I was pondering if we need a bit more care to make sure all > the pipes agree, but I suppose if that wasn't the case > check_digital_port_conflicts() would have a failed at its > job. So this seems fine. Yes, mixed SST/MST CRTCs connected to the same encoder is what you mean I guess. It would have caused a failure elsewhere or make the commit added here fail during atomic check. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > + > > /* Suppress underruns caused by re-training */ > > intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > > if (crtc_state->has_pch_encoder) > > @@ -5184,16 +5190,23 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > > intel_crtc_pch_transcoder(crtc), false); > > } > > > > + /* TODO: use a modeset for SST as well. */ > > + if (mst_output) { > > + ret = intel_modeset_commit_pipes(dev_priv, pipe_mask, ctx); > > + > > + if (ret && ret != -EDEADLK) > > + drm_dbg_kms(&dev_priv->drm, > > + "[ENCODER:%d:%s] link retraining failed: %pe\n", > > + encoder->base.base.id, encoder->base.name, > > + ERR_PTR(ret)); > > + > > + return ret; > > + } > > + > > for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) { > > const struct intel_crtc_state *crtc_state = > > to_intel_crtc_state(crtc->base.state); > > > > - /* retrain on the MST master transcoder */ > > - if (DISPLAY_VER(dev_priv) >= 12 && > > - intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && > > - !intel_dp_mst_is_master_trans(crtc_state)) > > - continue; > > - > > intel_dp_check_frl_training(intel_dp); > > intel_dp_pcon_dsc_configure(intel_dp, crtc_state); > > intel_dp_start_link_train(intel_dp, crtc_state); > > -- > > 2.43.3 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 81e620dd33bb7..120f7b420807b 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5147,6 +5147,7 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, struct intel_dp *intel_dp = enc_to_intel_dp(encoder); struct intel_crtc *crtc; u8 pipe_mask; + bool mst_output = false; int ret; if (!intel_dp_is_connected(intel_dp)) @@ -5177,6 +5178,11 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { + mst_output = true; + break; + } + /* Suppress underruns caused by re-training */ intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); if (crtc_state->has_pch_encoder) @@ -5184,16 +5190,23 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, intel_crtc_pch_transcoder(crtc), false); } + /* TODO: use a modeset for SST as well. */ + if (mst_output) { + ret = intel_modeset_commit_pipes(dev_priv, pipe_mask, ctx); + + if (ret && ret != -EDEADLK) + drm_dbg_kms(&dev_priv->drm, + "[ENCODER:%d:%s] link retraining failed: %pe\n", + encoder->base.base.id, encoder->base.name, + ERR_PTR(ret)); + + return ret; + } + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) { const struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); - /* retrain on the MST master transcoder */ - if (DISPLAY_VER(dev_priv) >= 12 && - intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && - !intel_dp_mst_is_master_trans(crtc_state)) - continue; - intel_dp_check_frl_training(intel_dp); intel_dp_pcon_dsc_configure(intel_dp, crtc_state); intel_dp_start_link_train(intel_dp, crtc_state);
Instead of direct calls to the link train functions, retrain the link via a commit modeset. The direct call means that the output port will be disabled/re-enabled while the rest of the pipeline (transcoder) is active, which doesn't seem to work on MST at least. It leads to underruns and black screen, presumedly because the transcoder is not disabled/re-enabled along the port. Leave switching to a commit modeset on SST for a later patchset, as that seems to work ok currently (though better to using a commit there too, due to the suppressed underruns). Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)