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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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