[Intel-gfx] [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

Thu Jan 19 20:39:14 UTC 2017

On Thu, Jan 19, 2017 at 09:59:54PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 19, 2017 at 11:38:56AM -0800, Manasi Navare wrote:
> > On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > > > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > > > I have verified this patch with the latest drm-tip and it is also
> > > > > > absolutely necessary to ensure the link gets properly retrained
> > > > > > after link-status is BAD and after anew modeset is triggered by
> > > > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > > > it resets the link failure values and link retraining does not happen
> > > > > > at lower link rates.
> > > > > 
> > > > > Why are you resetting the failure values in this function? You should
> > > > > only do that when a long pulse is actually detected IMO (and yes, 
> > > > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > > > now).
> > > > >
> > > > 
> > > > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > > > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > > > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > > > mean that we have lost the information about the link rate and lane count at which link training failed.
> > > > 
> > > > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > > > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > > > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > > > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> > > 
> > > No. What you're asking for is a change in behaviour (to call
> > > intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> > > actual long pulse). That was never how this code worked. The point of
> > > the flag was to avoid calling it twice when processing the HPD since it
> > > was directly called from intel_dp_hpd_pulse() and then again from
> > > ->detect(). Since we no longer call it directly from
> > > intel_dp_hpd_pulse() the flag is in fact useless.
> > >
> > 
> > No I think there is some confusion. I am not asking the change in behaviour. I dont
> > want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.
> 
> That is precisely what you're asking since intel_dp_hpd_pulse() would
> be the only place left that would reset the flag.
> 
> > I want to avoid calling it twice and which exactly was the point of adding that flag.
> 
> The commit that added the flag did exactly what I stated above. It
> didn't avoid any calls via mode enumeration, it only avoid the double
> call during actual hpd handling.
> 
> Which is also how the code behaved before someone tried to move the
> long hpd handling into the hpd pulse handler, and it's how the code
> behaves now that I moved the long hpd handling back into ->detect().
> 
> > The long pulse should
> > be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
> > we do set the detect_done flag to false and it does a full detect through ->detect(). But then
> > for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
> > call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)
> > 
> > Manasi 
> > > While the change you ask for may be desirable, history has shown
> > > that the DP code is very fragile, so I don't think we should make
> > > the link state property depend on something that may need to be
> > > reverted if a regression crops up. And so I think you should just
> > > change your new code to work with the existing scheme. We can push
> > > the detect_done optimization in afterwards since we should then be
> > > able to revert it without having to revert everything related to
> > > the link status property.
> > >

So currently it reads all the dpcd values on hotplug and then it rereads 
everything on drm_helper_probe_single_connector_modes()
So in case of link failure I set the maxlinkrate and lane count to the values lower
than the values at which link training failed, but when userspace tries to redo a modeset,
it calls drm_helper_probe_single_connector_modes() and calls detect() which calls long_pulse
and rewrites the max link rate and lane count and so instead of training at lower values it
trains again at the same values .
So the other alternative is splitting the intel_dp_long_pulse() and do parts of it like
read the dpcd registers i.e call intel_dp_detect_dpcd() and set the max link rate and
max lane count in intel_dp_hpd_pulse(). That way we set the max values only once at
hotplug and avoid them getting rewritten during mode enumeration.

Any thoughts Ville/Jani/Daniel?

Manasi
> > > > 
> > > > Manasi 
> > > > > > 
> > > > > > Ville/Jani could you please review this patch?
> > > > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > > > soon to get it fixed.
> > > > > > 
> > > > > > Regards
> > > > > > Manasi
> > > > > > 
> > > > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > > From: "Navare, Manasi D" <manasi.d.navare at intel.com>
> > > > > > > 
> > > > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > > > flag after long hpd handling leads to the code being executed again during
> > > > > > > mode enumeration.
> > > > > > > 
> > > > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > > > status.
> > > > > > > 
> > > > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > > > hotplug between suspend/resume.
> > > > > > > 
> > > > > > > v2:
> > > > > > > Allow full detect after encoder reset. (Ville)
> > > > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > > > Commit message changes.
> > > > > > > 
> > > > > > > Cc: stable at vger.kernel.org
> > > > > > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira at intel.com>
> > > > > > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index fb12896..6732c17 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > > > >  	intel_dp_set_edid(intel_dp);
> > > > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > > > >  		status = connector_status_connected;
> > > > > > > -	intel_dp->detect_done = true;
> > > > > > >  
> > > > > > >  	/* Try to read the source of the interrupt */
> > > > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > > > >  		      connector->base.id, connector->name);
> > > > > > >  
> > > > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > > > -	if (!intel_dp->detect_done)
> > > > > > > +	if (!intel_dp->detect_done) {
> > > > > > > +		intel_dp->detect_done = true;
> > > > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > > -
> > > > > > > -	intel_dp->detect_done = false;
> > > > > > > +	}
> > > > > > >  
> > > > > > >  	return status;
> > > > > >  }
> > > > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > > > >  	if (lspcon->active)
> > > > > > >  		lspcon_resume(lspcon);
> > > > > > >  
> > > > > > > +	intel_dp->detect_done = false;
> > > > > > > +
> > > > > > >  	pps_lock(intel_dp);
> > > > > > >  
> > > > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > > -- 
> > > > > > > 2.7.4
> > > > > > > 
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC