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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
>
> > +
> > /* 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
