| Message ID | 20170313205323.12339-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 13, 2017 at 10:53:23PM +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. > > 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@vger.kernel.org > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > Reported-by: Palmer Dabbelt <palmer@dabbelt.com> > 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@linux.intel.com> > --- > 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 s/apparely/apparently/ > + * 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. Makes sense as the retraining is only applied if the crtc was active (i.e. the connector was previously connected and trained). Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, Mar 13, 2017 at 10:53:23PM +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. > > 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@vger.kernel.org > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > Reported-by: Palmer Dabbelt <palmer@dabbelt.com> > 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@linux.intel.com> > --- > 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. Right? Regards Manasi > /* > -- > 2.10.2 >
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@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. > > > > 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@vger.kernel.org > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Cc: Palmer Dabbelt <palmer@dabbelt.com> > > Reported-by: Palmer Dabbelt <palmer@dabbelt.com> > > 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@linux.intel.com> > > --- > > 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.
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; } /*