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()
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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx
> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > ---
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
