[Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse

[Manasi Navare]

On Thu, Feb 16, 2017 at 07:18:57PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 16, 2017 at 09:07:53AM -0800, Manasi Navare wrote:
> > On Thu, Feb 16, 2017 at 05:30:07PM +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Apparently some DP sinks are a little nuts and cause HPD to drop
> > > intermittently during modesets. This happens eg. on an ASUS PB287Q.
> > > In oder to recover from this we can't really use the previous
> > > connector status to determine if the link needs retraining, so let's
> > > just ignore that piece of information and do the retrain
> > > unconditionally. We do of course still check whether the link is
> > > supposed to be running or not.
> > > 
> > > Cc: stable at vger.kernel.org
> > > Cc: Palmer Dabbelt <palmer at dabbelt.com>
> > > Reported-by: Palmer Dabbelt <palmer at dabbelt.com>
> > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 024798a9c016..37a746f7fbc3 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4648,11 +4648,18 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > -	} else if (connector->status == connector_status_connected) {
> > > +	} else {
> > >  		/*
> > > -		 * If display was connected already and is still connected
> > > -		 * check links status, there has been known issues of
> > > -		 * link loss triggerring long pulse!!!!
> > > +		 * If display is now connected check links status,
> > > +		 * there has been known issues of link loss triggerring
> > > +		 * long pulse.
> > > +		 *
> > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > +		 * weird HPD ping pong during modesets. So we can apparely
> > > +		 * end up with HPD going low during a modeset, and then
> > > +		 * going back up soon after. And once that happens we must
> > > +		 * retrain the link to get a picture. That's in case no
> > > +		 * userspace component reacted to intermittent HPD dip.
> > >  		 */
> > >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > >  		intel_dp_check_link_status(intel_dp);
> > > --
> > 
> > So here we basically just ignore the connector status and retrain irrespectively.
> 
> We ignore the _previous_ connector status.
> 
> > But that means even if we have newer values now for max link rate/lane count from
> > DPCD, during this retrain we are just using the stale value of intel_dp->link_rate
> > and intel_dp->lane_count. I think intel_dp->link_rate and lane count values
> > should be set to 0 on HPD pulse, they would be set only during a modeset.
> 
> The DPCD has already been parsed by this time.
> 
> -- 
> Ville Syrjälä
> Intel OTC

Yes, we have parsed the DPCD but we dont write to intel_dp->link-rate and intel_dp->lane_count
until we do a modeset (these get written during pre_enable) and these values get used
during the retraining of the link. So at this point we will still use stale values.
Shouldnt we set the link status to BAD and send a hotplug event, like the solution we implemneted
for retraining?

Regards
Manasi

Regards
Manasi