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