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

[Manasi Navare <manasi.d.navare@intel.com>]

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