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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Manasi Navare <manasi.d.navare@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Palmer Dabbelt <palmer@dabbelt.com>,
	stable@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Perform link quality check unconditionally during long pulse
Date: Thu, 16 Feb 2017 19:46:26 +0200	[thread overview]
Message-ID: <20170216174626.GJ31595@intel.com> (raw)
In-Reply-To: <20170216172409.GB27437@intel.com>

On Thu, Feb 16, 2017 at 09:24:09AM -0800, Manasi Navare wrote:
> 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@linux.intel.com wrote:
> > > > From: Ville Syrjï¿½lï¿½ <ville.syrjala@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@vger.kernel.org
> > > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > > Reported-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > > > Signed-off-by: Ville Syrjï¿½lï¿½ <ville.syrjala@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.

They're not stale. They're exactly what we used when we set up the
mode that's still being drive out. This is just retraining the link
at the same parameters.

> Shouldnt we set the link status to BAD and send a hotplug event, like the solution we implemneted
> for retraining?
> 
> Regards
> Manasi
> 
> Regards
> Manasi

-- 
Ville Syrjï¿½lï¿½
Intel OTC