On Mon, Mar 13, 2017 at 04:09:53PM -0700, Manasi Navare wrote:
> On Mon, Mar 13, 2017 at 10:53:23PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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.
> >
> > To actually get read out the EDID and update things properly we
> > also need to nuke the goto out added by commit 7d23e3c37bb3
> > ("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure
> > why that was there. Perhaps to avoid an EDID read if the connector
> > status didn't appear to change, but that sort of thing is quite racy
> > and would have failed anyway if we failed to keep up with the
> > hotplugs (if we missed the HPD down in between two HPD ups). And
> > now that we take this codepath unconditionally we definitely need
> > to drop the goto as otherwise we would never do the EDID read.
> >
> > v2: Drop the goto that made us skip EDID reads entirely. Doh!
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > Cc: Palmer Dabbelt <palmer@xxxxxxxxxxx>
> > Reported-by: Palmer Dabbelt <palmer@xxxxxxxxxxx>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766
> > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index fd96a6cf7326..5c2f1b37b58f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4634,16 +4634,22 @@ 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);
> > drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > - goto out;
> > }
> >
>
> Yes makes sense to me too since we have a check for crtc->active so
> it will retrain only if the link is already up and running with an active crtc
> and intel_dp->lane_count is not 0.
> In case of link failures though when we send the hotplug uevent it will get through
> this path and it will try to retrain which we dont want since the retraining
> should happen only in the atomic_commit at the fallback rate, not here.
> So I guess on link failure, I can set intel_dp->lane_count to 0 to invalidate that
> and avoid retraining here.
I'm not sure we want to expand the link_count abuse here. It was only
added as a hack anyway. But the retraining code should perhaps check
whether the current link parameters are still in the set of
valid/untested parameters and skip the retraining if not.
--
Ville Syrjälä
Intel OTC