[PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Dhinakaran Pandiyan]

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(-)

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

[PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Dhinakaran Pandiyan]

We are too late in the enabling sequence to back out cleanly, not updating
state tracking variables, like intel_dp->active_mst_links in this
instance, results in incorrect behaviour further along.

v2: Fixed int v/s bool comparison

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 7e3e01607643..110e7ff22ef7 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 				       connector->port,
 				       pipe_config->pbn,
 				       pipe_config->dp_m_n.tu);
-	if (ret == false) {
+	if (!ret)
 		DRM_ERROR("failed to allocate vcpi\n");
-		return;
-	}
-
 
 	intel_dp->active_mst_links++;
 	temp = I915_READ(DP_TP_STATUS(port));
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Manasi Navare]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915/mst: Do not retrain new links

[Patchwork]

== 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 : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4504 -> Patchwork_9709 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9709 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9709, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46797/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9709:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9709 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)
      fi-skl-6260u:       PASS -> DMESG-FAIL (fdo#107174, fdo#106560)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       PASS -> FAIL (fdo#100368)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    igt@pm_rpm@basic-pci-d3-state:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_workarounds:
      fi-kbl-7560u:       DMESG-FAIL -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174


== Participating hosts (46 -> 42) ==

  Additional (1): fi-bdw-gvtdvm 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4504 -> Patchwork_9709

  CI_DRM_4504: 0ba31c45beee4f7cfc17c61205294c322a9891bf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4566: 7270e39a0e6238804827ea7f8db433ad743ed745 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9709: 26dd4c1cfa881ec7be3cf6cc77984fccbb2f9c87 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

26dd4c1cfa88 drm/i915/mst: Continue state updates even if AUX writes fail.
fa117b217014 drm/i915/mst: Do not retrain new links

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9709/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Nathan Ciobanu]

On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan wrote:
> We are too late in the enabling sequence to back out cleanly, not updating
> state tracking variables, like intel_dp->active_mst_links in this
> instance, results in incorrect behaviour further along.
> 
> v2: Fixed int v/s bool comparison
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
Tested-by: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7e3e01607643..110e7ff22ef7 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  				       connector->port,
>  				       pipe_config->pbn,
>  				       pipe_config->dp_m_n.tu);
> -	if (ret == false) {
> +	if (!ret)
>  		DRM_ERROR("failed to allocate vcpi\n");
> -		return;
> -	}
> -
>  
>  	intel_dp->active_mst_links++;
>  	temp = I915_READ(DP_TP_STATUS(port));
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v2,1/2] drm/i915/mst: Do not retrain new links

[Patchwork]

== 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_4504_full -> Patchwork_9709_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9709_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9709_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_9709_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 +1

    igt@kms_cursor_legacy@short-flip-after-cursor-atomic-transitions-varying-size:
      shard-snb:          PASS -> SKIP +2

    
== Known issues ==

  Here are the changes found in Patchwork_9709_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-glk:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_rotation_crc@primary-rotation-90:
      shard-apl:          PASS -> FAIL (fdo#103925)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-kbl:          FAIL (fdo#103355) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  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_4504 -> Patchwork_9709

  CI_DRM_4504: 0ba31c45beee4f7cfc17c61205294c322a9891bf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4566: 7270e39a0e6238804827ea7f8db433ad743ed745 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9709: 26dd4c1cfa881ec7be3cf6cc77984fccbb2f9c87 @ 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/Patchwork_9709/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Manasi Navare]

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
> > > );
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Dhinakaran Pandiyan]

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
> > );
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Nathan Ciobanu]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Rodrigo Vivi]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Dhinakaran Pandiyan]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Rodrigo Vivi]

On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan wrote:
> We are too late in the enabling sequence to back out cleanly, not updating
> state tracking variables, like intel_dp->active_mst_links in this
> instance, results in incorrect behaviour further along.

I agree with you, although I'm not fully convinced that we need to call the
update payload if vcpi allocation failed...

so imho this entire function should be reworked to be like this:

+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -217,7 +217,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
        enum port port = intel_dig_port->base.port;
        struct intel_connector *connector =
                to_intel_connector(conn_state->connector);
-       int ret;
        uint32_t temp;
 
        /* MST encoders are bound to a crtc, not to a connector,
@@ -233,25 +232,15 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 
        drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
 
-       if (intel_dp->active_mst_links == 0)
+       if (intel_dp->active_mst_links++ == 0)
                intel_dig_port->base.pre_enable(&intel_dig_port->base,
                                                pipe_config, NULL);
 
-       ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
-                                      connector->port,
-                                      pipe_config->pbn,
-                                      pipe_config->dp_m_n.tu);
-       if (ret == false) {
+       if (drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr, connector->port,
+                                    pipe_config->pbn, pipe_config->dp_m_n.tu))
+               drm_dp_update_payload_part1(&intel_dp->mst_mgr);
+       else
                DRM_ERROR("failed to allocate vcpi\n");
-               return;
-       }
-
-
-       intel_dp->active_mst_links++;
-       temp = I915_READ(DP_TP_STATUS(port));
-       I915_WRITE(DP_TP_STATUS(port), temp);
-
-       ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);


probably with
-       temp = I915_READ(DP_TP_STATUS(port));
-       I915_WRITE(DP_TP_STATUS(port), temp);

in a separated patch. No idea why we read this staus reg and write back.
I didn't see on spec any indication that this cleans it or that we need that
for cleaning or anything else.

> 
> v2: Fixed int v/s bool comparison
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7e3e01607643..110e7ff22ef7 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  				       connector->port,
>  				       pipe_config->pbn,
>  				       pipe_config->dp_m_n.tu);
> -	if (ret == false) {
> +	if (!ret)
>  		DRM_ERROR("failed to allocate vcpi\n");
> -		return;
> -	}
> -
>  
>  	intel_dp->active_mst_links++;
>  	temp = I915_READ(DP_TP_STATUS(port));
> -- 
> 2.17.1
> 
> _______________________________________________
> 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

Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Dhinakaran Pandiyan]

On Wed, 2018-07-18 at 22:43 -0700, Rodrigo Vivi wrote:
> On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan wrote:
> > 
> > We are too late in the enabling sequence to back out cleanly, not
> > updating
> > state tracking variables, like intel_dp->active_mst_links in this
> > instance, results in incorrect behaviour further along.
> I agree with you, although I'm not fully convinced that we need to
> call the
> update payload if vcpi allocation failed...

But there is more payload update code in intel_mst_enable_dp(), that
would get executed regardless of this diff below. We'll have to rewrite
pre_enable, enable, disable and post_disable if the idea is avoid sink
transactions after the first failure. It does make sense to do all of
that as it avoids printing error messages in dmesg when we very well
know the branch device is disconnected, but this should be a separate
change. My idea was to bring pre_enable/enable in line with
disable/post_disable.


> 
> so imho this entire function should be reworked to be like this:
> 
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -217,7 +217,6 @@ static void intel_mst_pre_enable_dp(struct
> intel_encoder *encoder,
>         enum port port = intel_dig_port->base.port;
>         struct intel_connector *connector =
>                 to_intel_connector(conn_state->connector);
> -       int ret;
>         uint32_t temp;
>  
>         /* MST encoders are bound to a crtc, not to a connector,
> @@ -233,25 +232,15 @@ static void intel_mst_pre_enable_dp(struct
> intel_encoder *encoder,
>  
>         drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> >port, true);
>  
> -       if (intel_dp->active_mst_links == 0)
> +       if (intel_dp->active_mst_links++ == 0)
>                 intel_dig_port->base.pre_enable(&intel_dig_port-
> >base,
>                                                 pipe_config, NULL);
>  
> -       ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> -                                      connector->port,
> -                                      pipe_config->pbn,
> -                                      pipe_config->dp_m_n.tu);
> -       if (ret == false) {
> +       if (drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr, connector-
> >port,
> +                                    pipe_config->pbn, pipe_config-
> >dp_m_n.tu))
> +               drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> +       else
>                 DRM_ERROR("failed to allocate vcpi\n");
> -               return;
> -       }
> -
> -
> -       intel_dp->active_mst_links++;
> -       temp = I915_READ(DP_TP_STATUS(port));
> -       I915_WRITE(DP_TP_STATUS(port), temp);
> -
> -       ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> 
> 
> probably with
> -       temp = I915_READ(DP_TP_STATUS(port));
> -       I915_WRITE(DP_TP_STATUS(port), temp);
> 
> in a separated patch. No idea why we read this staus reg and write
> back.
It is clearing the ACT status bit by writing a 1. Bspec has this
under DP_TP_STATUS:24

-DK

> I didn't see on spec any indication that this cleans it or that we
> need that
> for cleaning or anything else.
> 
> > 
> > 
> > v2: Fixed int v/s bool comparison
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 7e3e01607643..110e7ff22ef7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  				       connector->port,
> >  				       pipe_config->pbn,
> >  				       pipe_config->dp_m_n.tu);
> > -	if (ret == false) {
> > +	if (!ret)
> >  		DRM_ERROR("failed to allocate vcpi\n");
> > -		return;
> > -	}
> > -
> >  
> >  	intel_dp->active_mst_links++;
> >  	temp = I915_READ(DP_TP_STATUS(port));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Rodrigo Vivi]

On Thu, Jul 19, 2018 at 11:51:40AM -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-07-18 at 22:43 -0700, Rodrigo Vivi wrote:
> > On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > We are too late in the enabling sequence to back out cleanly, not
> > > updating
> > > state tracking variables, like intel_dp->active_mst_links in this
> > > instance, results in incorrect behaviour further along.
> > I agree with you, although I'm not fully convinced that we need to
> > call the
> > update payload if vcpi allocation failed...
> 
> But there is more payload update code in intel_mst_enable_dp(),

oh... the part2 indeed...

> that
> would get executed regardless of this diff below. We'll have to rewrite
> pre_enable, enable, disable and post_disable if the idea is avoid sink
> transactions after the first failure. It does make sense to do all of
> that as it avoids printing error messages in dmesg when we very well
> know the branch device is disconnected, but this should be a separate
> change.

fair enough...

> My idea was to bring pre_enable/enable in line with
> disable/post_disable.

makes sense... I just saw it is similar on payload update failure.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> > 
> > so imho this entire function should be reworked to be like this:
> > 
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -217,7 +217,6 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >         enum port port = intel_dig_port->base.port;
> >         struct intel_connector *connector =
> >                 to_intel_connector(conn_state->connector);
> > -       int ret;
> >         uint32_t temp;
> >  
> >         /* MST encoders are bound to a crtc, not to a connector,
> > @@ -233,25 +232,15 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  
> >         drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector-
> > >port, true);
> >  
> > -       if (intel_dp->active_mst_links == 0)
> > +       if (intel_dp->active_mst_links++ == 0)
> >                 intel_dig_port->base.pre_enable(&intel_dig_port-
> > >base,
> >                                                 pipe_config, NULL);
> >  
> > -       ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> > -                                      connector->port,
> > -                                      pipe_config->pbn,
> > -                                      pipe_config->dp_m_n.tu);
> > -       if (ret == false) {
> > +       if (drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr, connector-
> > >port,
> > +                                    pipe_config->pbn, pipe_config-
> > >dp_m_n.tu))
> > +               drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> > +       else
> >                 DRM_ERROR("failed to allocate vcpi\n");
> > -               return;
> > -       }
> > -
> > -
> > -       intel_dp->active_mst_links++;
> > -       temp = I915_READ(DP_TP_STATUS(port));
> > -       I915_WRITE(DP_TP_STATUS(port), temp);
> > -
> > -       ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> > 
> > 
> > probably with
> > -       temp = I915_READ(DP_TP_STATUS(port));
> > -       I915_WRITE(DP_TP_STATUS(port), temp);
> > 
> > in a separated patch. No idea why we read this staus reg and write
> > back.
> It is clearing the ACT status bit by writing a 1. Bspec has this
> under DP_TP_STATUS:24
> 
> -DK
> 
> > I didn't see on spec any indication that this cleans it or that we
> > need that
> > for cleaning or anything else.
> > 
> > > 
> > > 
> > > v2: Fixed int v/s bool comparison
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 7e3e01607643..110e7ff22ef7 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  				       connector->port,
> > >  				       pipe_config->pbn,
> > >  				       pipe_config->dp_m_n.tu);
> > > -	if (ret == false) {
> > > +	if (!ret)
> > >  		DRM_ERROR("failed to allocate vcpi\n");
> > > -		return;
> > > -	}
> > > -
> > >  
> > >  	intel_dp->active_mst_links++;
> > >  	temp = I915_READ(DP_TP_STATUS(port));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Dhinakaran Pandiyan]

On Thu, 2018-07-19 at 16:37 -0700, Rodrigo Vivi wrote:
> On Thu, Jul 19, 2018 at 11:51:40AM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Wed, 2018-07-18 at 22:43 -0700, Rodrigo Vivi wrote:
> > > 
> > > On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > 
> > > > 
> > > > We are too late in the enabling sequence to back out cleanly,
> > > > not
> > > > updating
> > > > state tracking variables, like intel_dp->active_mst_links in
> > > > this
> > > > instance, results in incorrect behaviour further along.
> > > I agree with you, although I'm not fully convinced that we need
> > > to
> > > call the
> > > update payload if vcpi allocation failed...
> > But there is more payload update code in intel_mst_enable_dp(),
> oh... the part2 indeed...
> 
> > 
> > that
> > would get executed regardless of this diff below. We'll have to
> > rewrite
> > pre_enable, enable, disable and post_disable if the idea is avoid
> > sink
> > transactions after the first failure. It does make sense to do all
> > of
> > that as it avoids printing error messages in dmesg when we very
> > well
> > know the branch device is disconnected, but this should be a
> > separate
> > change.
> fair enough...
> 
> > 
> > My idea was to bring pre_enable/enable in line with
> > disable/post_disable.
> makes sense... I just saw it is similar on payload update failure.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks for the review, I will push this with 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107281

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915/mst: Do not retrain new links

[Patchwork]

== 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 : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4541 -> Patchwork_9767 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46797/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9767 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-whl-u:           PASS -> DMESG-FAIL (fdo#106560)
      {fi-skl-iommu}:     PASS -> DMESG-FAIL (fdo#107174, fdo#106560)

    igt@gem_exec_suspend@basic-s3:
      {fi-cfl-8109u}:     PASS -> INCOMPLETE (fdo#107187)

    igt@gem_mmap_gtt@basic-small-bo:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719) +2

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-no-display:
      fi-glk-j4005:       DMESG-WARN (fdo#106725) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107187 https://bugs.freedesktop.org/show_bug.cgi?id=107187


== Participating hosts (51 -> 45) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper 


== 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


== Linux commits ==

6b9f66af4ed5 drm/i915/mst: Continue state updates even if AUX writes fail.
2919e89d9e8a drm/i915/mst: Do not retrain new links

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9767/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v2,1/2] drm/i915/mst: Do not retrain new links

[Patchwork]

== 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

    
== 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/Patchwork_9767/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: failure for series starting with [v2,1/2] drm/i915/mst: Do not retrain new links

[Dhinakaran Pandiyan]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2,1/2] drm/i915/mst: Do not retrain new links

[Patchwork]

== 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_4547 -> Patchwork_9772 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9772 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9772, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46797/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9772:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_objects:
      fi-cnl-psr:         NOTRUN -> DMESG-FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_9772 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-no-display:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#105395) +5
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725)

    igt@drv_module_reload@basic-reload-inject:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106248, fdo#106725)

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#106947, fdo#106560)

    igt@drv_selftest@live_workarounds:
      fi-cnl-psr:         NOTRUN -> DMESG-FAIL (fdo#107292)
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#107292)

    igt@gem_busy@basic-hang-default:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

    igt@gem_exec_suspend@basic-s3:
      {fi-skl-caroline}:  NOTRUN -> INCOMPLETE (fdo#104108)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    {igt@kms_psr@cursor_plane_move}:
      fi-cnl-psr:         NOTRUN -> DMESG-FAIL (fdo#107372) +1

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#107372)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-bxt-dsi:         DMESG-FAIL (fdo#106560) -> PASS

    igt@gem_exec_create@basic:
      fi-glk-j4005:       DMESG-WARN (fdo#106745) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105395 https://bugs.freedesktop.org/show_bug.cgi?id=105395
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (48 -> 44) ==

  Additional (3): fi-byt-j1900 fi-skl-caroline fi-cnl-psr 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-8809g fi-byt-clapper 


== Build changes ==

    * Linux: CI_DRM_4547 -> Patchwork_9772

  CI_DRM_4547: 0a7ab192a697e951b2404f3c1ce42c5fa74f9ed1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4575: fe908a01012c9daafafb3410b9407725ca9d4f21 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9772: a5daa0cdd4535fa550e7b560882bd3269564792d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a5daa0cdd453 drm/i915/mst: Continue state updates even if AUX writes fail.
3ea02c8fa102 drm/i915/mst: Do not retrain new links

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9772/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Dhinakaran Pandiyan]

On Wed, 2018-07-25 at 11:39 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-07-19 at 16:37 -0700, Rodrigo Vivi wrote:
> > 
> > On Thu, Jul 19, 2018 at 11:51:40AM -0700, Dhinakaran Pandiyan
> > wrote:
> > > 
> > > 
> > > On Wed, 2018-07-18 at 22:43 -0700, Rodrigo Vivi wrote:
> > > > 
> > > > 
> > > > On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > We are too late in the enabling sequence to back out cleanly,
> > > > > not
> > > > > updating
> > > > > state tracking variables, like intel_dp->active_mst_links in
> > > > > this
> > > > > instance, results in incorrect behaviour further along.
> > > > I agree with you, although I'm not fully convinced that we need
> > > > to
> > > > call the
> > > > update payload if vcpi allocation failed...
> > > But there is more payload update code in intel_mst_enable_dp(),
> > oh... the part2 indeed...
> > 
> > > 
> > > 
> > > that
> > > would get executed regardless of this diff below. We'll have to
> > > rewrite
> > > pre_enable, enable, disable and post_disable if the idea is avoid
> > > sink
> > > transactions after the first failure. It does make sense to do
> > > all
> > > of
> > > that as it avoids printing error messages in dmesg when we very
> > > well
> > > know the branch device is disconnected, but this should be a
> > > separate
> > > change.
> > fair enough...
> > 
> > > 
> > > 
> > > My idea was to bring pre_enable/enable in line with
> > > disable/post_disable.
> > makes sense... I just saw it is similar on payload update failure.
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Thanks for the review, I will push this with 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107281

Pushed after checking the logs of failures - none of them were related
to the patch series.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx