| Message ID | 20250304152917.3407080-6-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dp: Fix link training interrupted by HPD pulse | expand |
On Tue, 04 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > After link training - both in case of a passing and failing LT result - > a work is scheduled to check the link state. This check should take > place after the link training is completed by disabling the link > training pattern and setting intel_dp::link_trained=true. Atm, the work > is scheduled before these steps, which may result in checking the link > state too early (and thus not retraining the link as expected). > > Fix the above by scheduling the link check work after link training is > complete. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > .../gpu/drm/i915/display/intel_dp_link_training.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > index 3906c11acc968..c1be073b9fc48 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1110,6 +1110,7 @@ intel_dp_128b132b_intra_hop(struct intel_dp *intel_dp, > void intel_dp_stop_link_train(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state) > { > + struct intel_display *display = to_intel_display(intel_dp); > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > > intel_dp->link_trained = true; Side note, it's a bit misleading that we set this even if link training failed... but we probably depend on it. > @@ -1124,6 +1125,13 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp, > } > > intel_hpd_unblock(encoder); > + > + if (!display->hotplug.ignore_long_hpd && We already poke at this directly elsewhere in the file, but I'm not a fan of doing so. We should have a function and use that. The fact that we can do anything doesn't mean we should. I guess that's for another patch, another time. > + intel_dp->link.seq_train_failures < 2) { It's not great that the magic 2 is duplicated. It was fine in one place. Regardless, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > + int delay_ms = intel_dp->link.seq_train_failures ? 0 : 2000; > + > + intel_encoder_link_check_queue_work(encoder, delay_ms); > + } > } > > static bool > @@ -1628,7 +1636,6 @@ void intel_dp_start_link_train(struct intel_atomic_state *state, > lt_dbg(intel_dp, DP_PHY_DPRX, "Forcing link training failure\n"); > } else if (passed) { > intel_dp->link.seq_train_failures = 0; > - intel_encoder_link_check_queue_work(encoder, 2000); > return; > } > > @@ -1651,10 +1658,8 @@ void intel_dp_start_link_train(struct intel_atomic_state *state, > return; > } > > - if (intel_dp->link.seq_train_failures < 2) { > - intel_encoder_link_check_queue_work(encoder, 0); > + if (intel_dp->link.seq_train_failures < 2) > return; > - } > > if (intel_dp_schedule_fallback_link_training(state, intel_dp, crtc_state)) > return;
On Tue, Mar 04, 2025 at 07:56:28PM +0200, Jani Nikula wrote: > On Tue, 04 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > > After link training - both in case of a passing and failing LT result - > > a work is scheduled to check the link state. This check should take > > place after the link training is completed by disabling the link > > training pattern and setting intel_dp::link_trained=true. Atm, the work > > is scheduled before these steps, which may result in checking the link > > state too early (and thus not retraining the link as expected). > > > > Fix the above by scheduling the link check work after link training is > > complete. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > .../gpu/drm/i915/display/intel_dp_link_training.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > index 3906c11acc968..c1be073b9fc48 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > @@ -1110,6 +1110,7 @@ intel_dp_128b132b_intra_hop(struct intel_dp *intel_dp, > > void intel_dp_stop_link_train(struct intel_dp *intel_dp, > > const struct intel_crtc_state *crtc_state) > > { > > + struct intel_display *display = to_intel_display(intel_dp); > > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > > > > intel_dp->link_trained = true; > > Side note, it's a bit misleading that we set this even if link training > failed... but we probably depend on it. Yes, it's used in the sense that the link is active. I guess it could be renamed to link_active accordingly (can follow up with that). > > > @@ -1124,6 +1125,13 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp, > > } > > > > intel_hpd_unblock(encoder); > > + > > + if (!display->hotplug.ignore_long_hpd && > > We already poke at this directly elsewhere in the file, but I'm not a > fan of doing so. We should have a function and use that. Agreed, can follow up with this separately. > The fact that we can do anything doesn't mean we should. > > I guess that's for another patch, another time. > > > + intel_dp->link.seq_train_failures < 2) { > > It's not great that the magic 2 is duplicated. It was fine in one place. Ok, will add a macro for it. > Regardless, > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > + int delay_ms = intel_dp->link.seq_train_failures ? 0 : 2000; > > + > > + intel_encoder_link_check_queue_work(encoder, delay_ms); > > + } > > } > > > > static bool > > @@ -1628,7 +1636,6 @@ void intel_dp_start_link_train(struct intel_atomic_state *state, > > lt_dbg(intel_dp, DP_PHY_DPRX, "Forcing link training failure\n"); > > } else if (passed) { > > intel_dp->link.seq_train_failures = 0; > > - intel_encoder_link_check_queue_work(encoder, 2000); > > return; > > } > > > > @@ -1651,10 +1658,8 @@ void intel_dp_start_link_train(struct intel_atomic_state *state, > > return; > > } > > > > - if (intel_dp->link.seq_train_failures < 2) { > > - intel_encoder_link_check_queue_work(encoder, 0); > > + if (intel_dp->link.seq_train_failures < 2) > > return; > > - } > > > > if (intel_dp_schedule_fallback_link_training(state, intel_dp, crtc_state)) > > > > > return; > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c index 3906c11acc968..c1be073b9fc48 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1110,6 +1110,7 @@ intel_dp_128b132b_intra_hop(struct intel_dp *intel_dp, void intel_dp_stop_link_train(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) { + struct intel_display *display = to_intel_display(intel_dp); struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; intel_dp->link_trained = true; @@ -1124,6 +1125,13 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp, } intel_hpd_unblock(encoder); + + if (!display->hotplug.ignore_long_hpd && + intel_dp->link.seq_train_failures < 2) { + int delay_ms = intel_dp->link.seq_train_failures ? 0 : 2000; + + intel_encoder_link_check_queue_work(encoder, delay_ms); + } } static bool @@ -1628,7 +1636,6 @@ void intel_dp_start_link_train(struct intel_atomic_state *state, lt_dbg(intel_dp, DP_PHY_DPRX, "Forcing link training failure\n"); } else if (passed) { intel_dp->link.seq_train_failures = 0; - intel_encoder_link_check_queue_work(encoder, 2000); return; } @@ -1651,10 +1658,8 @@ void intel_dp_start_link_train(struct intel_atomic_state *state, return; } - if (intel_dp->link.seq_train_failures < 2) { - intel_encoder_link_check_queue_work(encoder, 0); + if (intel_dp->link.seq_train_failures < 2) return; - } if (intel_dp_schedule_fallback_link_training(state, intel_dp, crtc_state)) return;
After link training - both in case of a passing and failing LT result - a work is scheduled to check the link state. This check should take place after the link training is completed by disabling the link training pattern and setting intel_dp::link_trained=true. Atm, the work is scheduled before these steps, which may result in checking the link state too early (and thus not retraining the link as expected). Fix the above by scheduling the link check work after link training is complete. Signed-off-by: Imre Deak <imre.deak@intel.com> --- .../gpu/drm/i915/display/intel_dp_link_training.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)