> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Imre
> Deak
> Sent: Friday, July 12, 2024 7:27 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx>
> Subject: [PATCH 1/3] drm/i915/dp: Retrain SST links via a modeset commit
>
> Instead of direct calls of the link training functions, use a modeset commit
> to retrain a DP link in SST mode, similarly to how this is done in DP-MST
> mode. Originally the current way was chosen presumedly, because there
> wasn't a well-established way in place for the driver to do an internal (vs.
> userspace/kernel client) commit. Since then such internal commits became a
> common place (initial-, HDMI/TC link reset commit), so there is no reason to
> handle the DP-SST link-retraining case differently.
>
> At the end of the current sequence the HW reported a FIFO underrun -
> without other issues visible to users - because during retraining the link's
> encoder/port was disabled/re-enabled without also disabling/re-enabling
> the corresponding pipe/transcoder (as required by the spec); the
> corresponding underrun error message was suppressed as a known issue.
> Based on Ankit's test on DG2 the underrun error was still reported as it got
> detected with some (vblank) delay wrt. other platforms. Switching to a
> modeset commit resolves these underrun related issues.
>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
LGTM,
Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 67 ++++---------------------
> 1 file changed, 9 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index d4b1b18453dca..f83128ac60756 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5242,8 +5242,6 @@ static int intel_dp_retrain_link(struct
> intel_encoder *encoder, {
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> - struct intel_crtc *crtc;
> - bool mst_output = false;
> u8 pipe_mask;
> int ret;
>
> @@ -5272,64 +5270,17 @@ static int intel_dp_retrain_link(struct
> intel_encoder *encoder,
> encoder->base.base.id, encoder->base.name,
> str_yes_no(intel_dp->link.force_retrain));
>
> - 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);
> + ret = intel_modeset_commit_pipes(dev_priv, pipe_mask, ctx);
> + if (ret == -EDEADLK)
> + return ret;
>
> - if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) {
> - mst_output = true;
> - break;
> - }
> + intel_dp->link.force_retrain = false;
>
> - /* Suppress underruns caused by re-training */
> - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe,
> false);
> - if (crtc_state->has_pch_encoder)
> - intel_set_pch_fifo_underrun_reporting(dev_priv,
> -
> 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));
> -
> - goto out;
> - }
> -
> - 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);
> -
> - intel_dp->link_trained = false;
> -
> - intel_dp_check_frl_training(intel_dp);
> - intel_dp_pcon_dsc_configure(intel_dp, crtc_state);
> - intel_dp_start_link_train(NULL, intel_dp, crtc_state);
> - intel_dp_stop_link_train(intel_dp, crtc_state);
> - break;
> - }
> -
> - 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);
> -
> - /* Keep underrun reporting disabled until things are stable
> */
> - intel_crtc_wait_for_next_vblank(crtc);
> -
> - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe,
> true);
> - if (crtc_state->has_pch_encoder)
> - intel_set_pch_fifo_underrun_reporting(dev_priv,
> -
> intel_crtc_pch_transcoder(crtc), true);
> - }
> -
> -out:
> - if (ret != -EDEADLK)
> - intel_dp->link.force_retrain = false;
> + if (ret)
> + 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;
> }
> --
> 2.44.2
