| Message ID | 20180718171943.3246-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan wrote: > The short pulse handler checks if channel equalization is okay and > goes onto retrain a link if there are active MST links. This retraining > path is not meant for new MST connections, but due to a bug elsewhere, if > active_mst_links is < 0 the boolean check for active_mst_links passes and This bug is probably around the way we track the active_mst_links and we are probably decrementing it more times than the available links and since its an int variable it goes to negative which is not the expected behaviour. Why not change this active_mst_links variable to be an unsigned int since we do not expect any negative values for this anyways. That way we can still check against just intel_dp->active_mst_links as opposed checking for it being greater than 0 and would also not need a WARN_ON here. Manasi > we proceed to retrain a new link. This results in a sequence of failed link > training attempts, most likely due to the hardware not setup for link > training at that point i.e., missing the DDI pre_enable sequence. > > [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not ok, retraining > [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* Timeout waiting for DDI BUF C idle bit > > The above error gives us a hint something went wrong before link > training started. > > Check for a positive value of active_mst_links and throw in a warning for > invalid active_mst_links as debug aid. > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b45b08420c1f..2d61ff01cf51 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > int ret = 0; > int retry; > bool handled; > + > + WARN_ON_ONCE(intel_dp->active_mst_links < 0); > bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > go_again: > if (bret == true) { > > /* check link status - esi[10] = 0x200c */ > - if (intel_dp->active_mst_links && > + if (intel_dp->active_mst_links > 0 && > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > DRM_DEBUG_KMS("channel EQ not ok, retraining\n"); > intel_dp_start_link_train(intel_dp); > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 18, 2018 at 01:34:12PM -0700, Dhinakaran Pandiyan wrote: > On Wed, 2018-07-18 at 10:45 -0700, Manasi Navare wrote: > > On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan wrote: > > > > > > The short pulse handler checks if channel equalization is okay and > > > goes onto retrain a link if there are active MST links. This > > > retraining > > > path is not meant for new MST connections, but due to a bug > > > elsewhere, if > > > active_mst_links is < 0 the boolean check for active_mst_links > > > passes and > > This bug is probably around the way we track the active_mst_links and > > we are > > probably decrementing it more times than the available links > > Yeah, that indeed is one aspect of the bug. > > > and since its an int > > variable it goes to negative which is not the expected behaviour. > > Why not change this active_mst_links variable to be an unsigned int > > since > > we do not expect any negative values for this anyways. > > That way we can still check against just intel_dp->active_mst_links > > as opposed checking > > for it being greater than 0 and would also not need a WARN_ON here. > > I did not get this, mind sharing code diff? My question was if negative values for active_mst_links are never allowed then why cant we define it as an unsigned int and avoid thrwoing a Warning later. Also I think the following check can be added in intel_mst_post_disable_dp(): if (intel_dp->active_mst_links) intel_dp->active_mst_link--; to avoid getting negative values in the first place. Manasi > > -DK > > > > > Manasi > > > > > > > > we proceed to retrain a new link. This results in a sequence of > > > failed link > > > training attempts, most likely due to the hardware not setup for > > > link > > > training at that point i.e., missing the DDI pre_enable sequence. > > > > > > [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not ok, > > > retraining > > > [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* Timeout > > > waiting for DDI BUF C idle bit > > > > > > The above error gives us a hint something went wrong before link > > > training started. > > > > > > Check for a positive value of active_mst_links and throw in a > > > warning for > > > invalid active_mst_links as debug aid. > > > > > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index b45b08420c1f..2d61ff01cf51 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct intel_dp > > > *intel_dp) > > > int ret = 0; > > > int retry; > > > bool handled; > > > + > > > + WARN_ON_ONCE(intel_dp->active_mst_links < 0); > > > bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > > > go_again: > > > if (bret == true) { > > > > > > /* check link status - esi[10] = 0x200c */ > > > - if (intel_dp->active_mst_links && > > > + if (intel_dp->active_mst_links > 0 && > > > !drm_dp_channel_eq_ok(&esi[10], > > > intel_dp->lane_count)) { > > > DRM_DEBUG_KMS("channel EQ not ok, > > > retraining\n"); > > > intel_dp_start_link_train(intel_dp > > > );
On Wed, 2018-07-18 at 10:45 -0700, Manasi Navare wrote: > On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan wrote: > > > > The short pulse handler checks if channel equalization is okay and > > goes onto retrain a link if there are active MST links. This > > retraining > > path is not meant for new MST connections, but due to a bug > > elsewhere, if > > active_mst_links is < 0 the boolean check for active_mst_links > > passes and > This bug is probably around the way we track the active_mst_links and > we are > probably decrementing it more times than the available links Yeah, that indeed is one aspect of the bug. > and since its an int > variable it goes to negative which is not the expected behaviour. > Why not change this active_mst_links variable to be an unsigned int > since > we do not expect any negative values for this anyways. > That way we can still check against just intel_dp->active_mst_links > as opposed checking > for it being greater than 0 and would also not need a WARN_ON here. I did not get this, mind sharing code diff? -DK > > Manasi > > > > > we proceed to retrain a new link. This results in a sequence of > > failed link > > training attempts, most likely due to the hardware not setup for > > link > > training at that point i.e., missing the DDI pre_enable sequence. > > > > [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not ok, > > retraining > > [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* Timeout > > waiting for DDI BUF C idle bit > > > > The above error gives us a hint something went wrong before link > > training started. > > > > Check for a positive value of active_mst_links and throw in a > > warning for > > invalid active_mst_links as debug aid. > > > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index b45b08420c1f..2d61ff01cf51 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct intel_dp > > *intel_dp) > > int ret = 0; > > int retry; > > bool handled; > > + > > + WARN_ON_ONCE(intel_dp->active_mst_links < 0); > > bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > > go_again: > > if (bret == true) { > > > > /* check link status - esi[10] = 0x200c */ > > - if (intel_dp->active_mst_links && > > + if (intel_dp->active_mst_links > 0 && > > !drm_dp_channel_eq_ok(&esi[10], > > intel_dp->lane_count)) { > > DRM_DEBUG_KMS("channel EQ not ok, > > retraining\n"); > > intel_dp_start_link_train(intel_dp > > );
On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan wrote: > The short pulse handler checks if channel equalization is okay and > goes onto retrain a link if there are active MST links. This retraining > path is not meant for new MST connections, but due to a bug elsewhere, if > active_mst_links is < 0 the boolean check for active_mst_links passes and > we proceed to retrain a new link. This results in a sequence of failed link > training attempts, most likely due to the hardware not setup for link > training at that point i.e., missing the DDI pre_enable sequence. > > [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not ok, retraining > [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* Timeout waiting for DDI BUF C idle bit > > The above error gives us a hint something went wrong before link > training started. > > Check for a positive value of active_mst_links and throw in a warning for > invalid active_mst_links as debug aid. > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Tested-by: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b45b08420c1f..2d61ff01cf51 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > int ret = 0; > int retry; > bool handled; > + > + WARN_ON_ONCE(intel_dp->active_mst_links < 0); > bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > go_again: > if (bret == true) { > > /* check link status - esi[10] = 0x200c */ > - if (intel_dp->active_mst_links && > + if (intel_dp->active_mst_links > 0 && > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > DRM_DEBUG_KMS("channel EQ not ok, retraining\n"); > intel_dp_start_link_train(intel_dp); > -- > 2.17.1 >
On Wed, Jul 18, 2018 at 02:30:18PM -0700, Dhinakaran Pandiyan wrote: > On Wed, 2018-07-18 at 13:31 -0700, Manasi Navare wrote: > > On Wed, Jul 18, 2018 at 01:34:12PM -0700, Dhinakaran Pandiyan wrote: > > > > > > On Wed, 2018-07-18 at 10:45 -0700, Manasi Navare wrote: > > > > > > > > On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan > > > > wrote: > > > > > > > > > > > > > > > The short pulse handler checks if channel equalization is okay > > > > > and > > > > > goes onto retrain a link if there are active MST links. This > > > > > retraining > > > > > path is not meant for new MST connections, but due to a bug > > > > > elsewhere, if > > > > > active_mst_links is < 0 the boolean check for active_mst_links > > > > > passes and > > > > This bug is probably around the way we track the active_mst_links > > > > and > > > > we are > > > > probably decrementing it more times than the available links > > > Yeah, that indeed is one aspect of the bug. > > > > > > > > > > > and since its an int > > > > variable it goes to negative which is not the expected behaviour. > > > > Why not change this active_mst_links variable to be an unsigned > > > > int > > > > since > > > > we do not expect any negative values for this anyways. > > > > That way we can still check against just intel_dp- > > > > >active_mst_links > > > > as opposed checking > > > > for it being greater than 0 and would also not need a WARN_ON > > > > here. > > > I did not get this, mind sharing code diff? > > My question was if negative values for active_mst_links are never > > allowed > > then why cant we define it as an unsigned int and avoid thrwoing a > > Warning later. > Hmm. I still do not get how defining it an unsigned int will prevent > the decrement op from making intel_dp->active_mst_links == true. > > > > > Also I think the following check can be added in > > intel_mst_post_disable_dp(): > > > > if (intel_dp->active_mst_links) > > intel_dp->active_mst_link--; > > That's just band-aid, we will still go through the post_disable > sequence while not decrementing ->active_mst_links. yeap, we don't want to hide the other bug, but prevent the current worst case while we warn the existence of the other bug. So, Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > to avoid getting negative values in the first place. > > > > Manasi > > > > > > > > > > > -DK > > > > > > > > > > > > > > > Manasi > > > > > > > > > > > > > > > > > > > we proceed to retrain a new link. This results in a sequence of > > > > > failed link > > > > > training attempts, most likely due to the hardware not setup > > > > > for > > > > > link > > > > > training at that point i.e., missing the DDI pre_enable > > > > > sequence. > > > > > > > > > > [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not > > > > > ok, > > > > > retraining > > > > > [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* > > > > > Timeout > > > > > waiting for DDI BUF C idle bit > > > > > > > > > > The above error gives us a hint something went wrong before > > > > > link > > > > > training started. > > > > > > > > > > Check for a positive value of active_mst_links and throw in a > > > > > warning for > > > > > invalid active_mst_links as debug aid. > > > > > > > > > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c > > > > > om> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index b45b08420c1f..2d61ff01cf51 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct > > > > > intel_dp > > > > > *intel_dp) > > > > > int ret = 0; > > > > > int retry; > > > > > bool handled; > > > > > + > > > > > + WARN_ON_ONCE(intel_dp->active_mst_links < 0); > > > > > bret = intel_dp_get_sink_irq_esi(intel_dp, > > > > > esi); > > > > > go_again: > > > > > if (bret == true) { > > > > > > > > > > /* check link status - esi[10] = > > > > > 0x200c */ > > > > > - if (intel_dp->active_mst_links && > > > > > + if (intel_dp->active_mst_links > 0 && > > > > > !drm_dp_channel_eq_ok(&esi[10], > > > > > intel_dp->lane_count)) { > > > > > DRM_DEBUG_KMS("channel EQ not > > > > > ok, > > > > > retraining\n"); > > > > > intel_dp_start_link_train(inte > > > > > l_dp > > > > > ); > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2018-07-18 at 13:31 -0700, Manasi Navare wrote: > On Wed, Jul 18, 2018 at 01:34:12PM -0700, Dhinakaran Pandiyan wrote: > > > > On Wed, 2018-07-18 at 10:45 -0700, Manasi Navare wrote: > > > > > > On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan > > > wrote: > > > > > > > > > > > > The short pulse handler checks if channel equalization is okay > > > > and > > > > goes onto retrain a link if there are active MST links. This > > > > retraining > > > > path is not meant for new MST connections, but due to a bug > > > > elsewhere, if > > > > active_mst_links is < 0 the boolean check for active_mst_links > > > > passes and > > > This bug is probably around the way we track the active_mst_links > > > and > > > we are > > > probably decrementing it more times than the available links > > Yeah, that indeed is one aspect of the bug. > > > > > > > > and since its an int > > > variable it goes to negative which is not the expected behaviour. > > > Why not change this active_mst_links variable to be an unsigned > > > int > > > since > > > we do not expect any negative values for this anyways. > > > That way we can still check against just intel_dp- > > > >active_mst_links > > > as opposed checking > > > for it being greater than 0 and would also not need a WARN_ON > > > here. > > I did not get this, mind sharing code diff? > My question was if negative values for active_mst_links are never > allowed > then why cant we define it as an unsigned int and avoid thrwoing a > Warning later. Hmm. I still do not get how defining it an unsigned int will prevent the decrement op from making intel_dp->active_mst_links == true. > > Also I think the following check can be added in > intel_mst_post_disable_dp(): > > if (intel_dp->active_mst_links) > intel_dp->active_mst_link--; That's just band-aid, we will still go through the post_disable sequence while not decrementing ->active_mst_links. > > to avoid getting negative values in the first place. > > Manasi > > > > > > > -DK > > > > > > > > > > > Manasi > > > > > > > > > > > > > > > we proceed to retrain a new link. This results in a sequence of > > > > failed link > > > > training attempts, most likely due to the hardware not setup > > > > for > > > > link > > > > training at that point i.e., missing the DDI pre_enable > > > > sequence. > > > > > > > > [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not > > > > ok, > > > > retraining > > > > [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* > > > > Timeout > > > > waiting for DDI BUF C idle bit > > > > > > > > The above error gives us a hint something went wrong before > > > > link > > > > training started. > > > > > > > > Check for a positive value of active_mst_links and throw in a > > > > warning for > > > > invalid active_mst_links as debug aid. > > > > > > > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c > > > > om> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index b45b08420c1f..2d61ff01cf51 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct > > > > intel_dp > > > > *intel_dp) > > > > int ret = 0; > > > > int retry; > > > > bool handled; > > > > + > > > > + WARN_ON_ONCE(intel_dp->active_mst_links < 0); > > > > bret = intel_dp_get_sink_irq_esi(intel_dp, > > > > esi); > > > > go_again: > > > > if (bret == true) { > > > > > > > > /* check link status - esi[10] = > > > > 0x200c */ > > > > - if (intel_dp->active_mst_links && > > > > + if (intel_dp->active_mst_links > 0 && > > > > !drm_dp_channel_eq_ok(&esi[10], > > > > intel_dp->lane_count)) { > > > > DRM_DEBUG_KMS("channel EQ not > > > > ok, > > > > retraining\n"); > > > > intel_dp_start_link_train(inte > > > > l_dp > > > > ); > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2018-07-25 at 20:28 +0000, Patchwork wrote: > == Series Details == > > Series: series starting with [v2,1/2] drm/i915/mst: Do not retrain > new links > URL : https://patchwork.freedesktop.org/series/46797/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_4541_full -> Patchwork_9767_full = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_9767_full absolutely > need to be > verified manually. > > If you think the reported changes have nothing to do with the > changes > introduced in Patchwork_9767_full, please notify your bug team to > allow them > to document this new failure mode, which will reduce false > positives in CI. > > > > == Possible new issues == > > Here are the unknown changes that may have been introduced in > Patchwork_9767_full: > > === IGT changes === > > ==== Possible regressions ==== > > igt@gem_eio@in-flight-contexts-10ms: > shard-snb: PASS -> FAIL > > > ==== Warnings ==== > > igt@gem_mocs_settings@mocs-rc6-blt: > shard-kbl: SKIP -> PASS > > igt@gem_mocs_settings@mocs-rc6-bsd2: > shard-kbl: PASS -> SKIP > None of these three failures look related to the series, Cc Martin. > > == Known issues == > > Here are the changes found in Patchwork_9767_full that come from > known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@kms_flip@flip-vs-expired-vblank-interruptible: > shard-glk: PASS -> FAIL (fdo#102887, fdo#105363) > > > ==== Possible fixes ==== > > igt@gem_ctx_isolation@vcs0-s3: > shard-glk: FAIL (fdo#103375) -> PASS > > igt@kms_setmode@basic: > shard-apl: FAIL (fdo#99912) -> PASS > > igt@perf@blocking: > shard-hsw: FAIL (fdo#102252) -> PASS > > > fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 > fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887 > fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375 > fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363 > fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 > > > == Participating hosts (5 -> 5) == > > No changes in participating hosts > > > == Build changes == > > * Linux: CI_DRM_4541 -> Patchwork_9767 > > CI_DRM_4541: 3e18e4c6c008597f4ff952d7a3457bd310ce945c @ > git://anongit.freedesktop.org/gfx-ci/linux > IGT_4574: 24c5e07783222b5d6cf86003e8e545033e09bb3c @ > git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_9767: 6b9f66af4ed5f70dc4127b38e2fb98e799a0e4f9 @ > git://anongit.freedesktop.org/gfx-ci/linux > piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ > git://anongit.freedesktop.org/piglit > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw > ork_9767/shards.html > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b45b08420c1f..2d61ff01cf51 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) int ret = 0; int retry; bool handled; + + WARN_ON_ONCE(intel_dp->active_mst_links < 0); bret = intel_dp_get_sink_irq_esi(intel_dp, esi); go_again: if (bret == true) { /* check link status - esi[10] = 0x200c */ - if (intel_dp->active_mst_links && + if (intel_dp->active_mst_links > 0 && !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { DRM_DEBUG_KMS("channel EQ not ok, retraining\n"); intel_dp_start_link_train(intel_dp);
The short pulse handler checks if channel equalization is okay and goes onto retrain a link if there are active MST links. This retraining path is not meant for new MST connections, but due to a bug elsewhere, if active_mst_links is < 0 the boolean check for active_mst_links passes and we proceed to retrain a new link. This results in a sequence of failed link training attempts, most likely due to the hardware not setup for link training at that point i.e., missing the DDI pre_enable sequence. [ 80.301272] [drm:intel_dp_check_mst_status] channel EQ not ok, retraining [ 80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* Timeout waiting for DDI BUF C idle bit The above error gives us a hint something went wrong before link training started. Check for a positive value of active_mst_links and throw in a warning for invalid active_mst_links as debug aid. Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)