On Fri, Jul 29, 2016 at 02:45:29PM -0700, Manasi Navare wrote:
> On Thu, Jul 28, 2016 at 05:36:14PM -0700, Manasi Navare wrote:
> > On Thu, Jul 28, 2016 at 11:15:22PM +0300, Ville Syrjälä wrote:
> > > On Thu, Jul 28, 2016 at 12:48:53PM -0700, Manasi Navare wrote:
> > > > On Thu, Jul 28, 2016 at 05:50:43PM +0300, ville.syrj...@linux.intel.com
> > > > wrote:
> > > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > >
> > > > > DP link retraining needs to grab some modeset locks to not race with
> > > > > modesets, so we can't really do it safely from the hpd_pulse, lest we
> > > > > risk deadlocking due to MST sideband stuff.
> > > > >
> > > > > Move the link retraining to happen from the hotplug work instead.
> > > > > Doing at the end of intel_dp_detect() seems like a good place in case
> > > > > the sink already got disconnected, in which case retraining is
> > > > > pointless.
> > > > >
> > > > > To determine if we need to schedule the hotplug work, we'll just check
> > > > > the sink lane status without locks from hpd_pulse. A little racy
> > > > > still eg. due to useing intel_dp->lane_count, but no less racy than
> > > > > what we already had. We'll repeat the check in from intel_dp_detect()
> > > > > with proper locking, where we'll also check if the link as actually
> > > > > active or not.
> > > > >
> > > > > Cc: Ander Conselvan de Oliveira
> > > > > <ander.conselvan.de.olive...@intel.com>
> > > > > Cc: Jim Bride <jim.br...@linux.intel.com>
> > > > > Cc: Manasi D Navare <manasi.d.nav...@intel.com>
> > > > > Cc: Durgadoss R <durgados...@intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 154
> > > > > +++++++++++++++++-----------------------
> > > > > 1 file changed, 66 insertions(+), 88 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 4a4184c21989..675b83f57a07 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3842,15 +3842,6 @@ intel_dp_check_mst_status(struct intel_dp
> > > > > *intel_dp)
> > > > > bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > > > > go_again:
> > > > > if (bret == true) {
> > > > > -
> > > > > - /* check link status - esi[10] = 0x200c */
> > > > > - if (intel_dp->active_streams &&
> > > > > - !drm_dp_channel_eq_ok(&esi[10],
> > > > > intel_dp->lane_count)) {
> > > > > - DRM_DEBUG_KMS("channel EQ not ok,
> > > > > retraining\n");
> > > > > - intel_dp_start_link_train(intel_dp);
> > > > > - intel_dp_stop_link_train(intel_dp);
> > > > > - }
> > > > > -
> > > > > DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > > > > ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr,
> > > > > esi, &handled);
> > > > >
> > > > > @@ -3886,34 +3877,42 @@ go_again:
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > -static void
> > > > > -intel_dp_check_link_status(struct intel_dp *intel_dp)
> > > > > +static bool
> > > > > +intel_dp_link_needs_retrain(struct intel_dp *intel_dp)
> > > > > {
> > > > > - struct intel_encoder *intel_encoder =
> > > > > &dp_to_dig_port(intel_dp)->base;
> > > > > - struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > > > > u8 link_status[DP_LINK_STATUS_SIZE];
> > > > >
> > > > > -
> > > > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > > > + if (intel_dp->active_streams == 0)
> > > > > + return false;
> > > > > +
> > > > > + if (intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING)
> > > > > + return true;
> > > > >
> > > > > if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > > DRM_ERROR("Failed to get link status\n");
> > > > > - return;
> > > > > + return false;
> > > > > }
> > > > >
> > > > > - if (!intel_encoder->base.crtc)
> > > > > - return;
> > > > > + return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > > >
> > > > According to the DP spec, we should also check for the clock recovery
> > > > bit in DPCD
> > > > We should also add a check drm_dp_clock_recovery_ok(link_status,
> > > > intel_dp->lane_count)
> > >
> > > I think drm_dp_channel_eq_ok() checks all the bits: INTERLANE_ALIGN_DONE,
> > > CR_DONE, EQ_DONE, and SYMBOL_LOCKED.
> >
> > It only checks for EQ_DONE and INTERLANE_ALIGN_DONE.
> > drm_dp_clock_recovery_ok() checks for DP_LANE_CR_DONE bit.
> > >
> > > Anyways, while I was looking at the short pulse handling a bit more, I
> > > came
> > > to the conclusion that we should rewrite it a bit.
> > >
> > > The CTS docs seem to say that we're expected to read DPCD 0x200-0x205 on
> > > short pulse, and use that information to determine what we should do.
> > > Currently we don't do that.
> > >
> > > So I was thunking that we'd adjust the link_status stuff to read
> > > 0x200-0x207
> > > instead of 0x202-0x207, like so:
> > > -#define DP_LINK_STATUS_SIZE 6
> > > +#define DP_LINK_STATUS_SIZE 8
> > >
> > > static u8 dp_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int
> > > r)
> > > {
> > > - return link_status[r - DP_LANE0_1_STATUS];
> > > + return link_status[r - DP_SINK_COUNT];
> > > }
> > >
> > > And then we'd use something like this as our short pulse handler:
> > >
> > > static bool
> > > intel_dp_short_pulse(struct intel_dp *intel_dp)
> > > {
> > > u8 link_status[DP_LINK_STATUS_SIZE], tmp;
> > >
> > > /*
> > >
> > > * Clearing compliance test variables to allow capturing
> > >
> > > * of values for next automated test request.
> > >
> > > */
> > > intel_dp->compliance_test_active = 0;
> > > intel_dp->compliance_test_type = 0;
> > > intel_dp->compliance_test_data = 0;
> > >
> > > if (!intel_dp_get_link_status(intel_dp, link_status))
> > > return false;
> > >
> > > tmp = drm_dp_link_status(link_status,
> > > DP_DEVICE_SERVICE_IRQ_VECTOR);
> > > if (tmp)
> > > drm_dp_dpcd_writeb(&intel_dp->aux,
> > > DP_DEVICE_SERVICE_IRQ_VECTOR,
> > > tmp);
> > >
> > > if (tmp & DP_AUTOMATED_TEST_REQUEST)
> > > DRM_DEBUG_DRIVER("Test request in short pulse not
> > > handled\n");
> > > if (tmp & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
> > > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> > >
> > > tmp = drm_dp_link_status(link_status,
> > > DP_LANE_ALIGN_STATUS_UPDATED);
> > > if (tmp & DP_DOWNSTREAM_PORT_STATUS_CHANGED)
> > > return false;
> > >
> > > tmp = drm_dp_link_status(link_status, DP_SINK_COUNT);
> > > if (DP_GET_SINK_COUNT(tmp) != intel_dp->sink_count)
> > > return false;
> > >
> > > if (intel_dp->active_streams != 0 &&
> > > !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))
> > > return false;
> > >
> > > return true;
> > > }
> > >
> > > and obviously fill in whatever else sink irq handling we need.
> > >
> > > Thoughts? Does that look sane?
> >
> > Yes all the Link maintenance tests in CTS spec expect DPCD registers
> > 0200h-0205h to be read on loss of symbol lock or loss of clock recovery.
> > So I think it makes sense to read 0200-0205h.
> >
> > >
> > > <snip>
> > > > >
> > > > > static enum drm_connector_status
> > > > > intel_dp_detect(struct drm_connector *connector, bool force)
> > > > > {
> > > > > struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > > - struct intel_digital_port *intel_dig_port =
> > > > > dp_to_dig_port(intel_dp);
> > > > > - struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > > > struct intel_connector *intel_connector =
> > > > > to_intel_connector(connector);
> > > > > + enum drm_connector_status status;
> > > > >
> > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > > > connector->base.id, connector->name);
> > > > >
> > > > > - if (intel_dp->is_mst) {
> > > > > - /* MST devices are disconnected from a monitor POV */
> > > > > - intel_dp_unset_edid(intel_dp);
> > > > > - if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > > > - intel_encoder->type = INTEL_OUTPUT_DP;
> > > > > - return connector_status_disconnected;
> > > > > - }
> > > > > -
> > > > > - /* If full detect is not performed yet, do a full detect */
> > > > > - if (!intel_dp->detect_done)
> > > > > - intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > + status = intel_dp_long_pulse(intel_connector);
> > > > >
> > > > > - intel_dp->detect_done = false;
> > > > > + if (status == connector_status_connected || intel_dp->is_mst)
> > > >
> > > > Whya re we not checking for (!intel_encoder->crtc.base) before
> > > > retraining?
> > > > I had seen issues with connected boot case when connector status is
> > > > connected and if it
> > > > tries to retrain it fails with retries because there is no active pipe.
> > > > In that
> > > > case it should not attempt to retrain.
> > >
> > > I changed it to use intel_dp->active_streams instead. That'll work for MST
> > > as well as SST. Checking for the crtc would only cover SST.
> > >
> > > >
> > > >
> > > > > + intel_dp_link_retrain(intel_dp);
> > > > >
> > > > > - if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > > - return connector_status_connected;
> > > > > - else
> > > > > - return connector_status_disconnected;
> > > > > + return status;
> > > > > }
> > > > >
> > > > > static void
> > > > > @@ -4706,39 +4682,41 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > > > *intel_dig_port, bool long_hpd)
> > > > > port_name(intel_dig_port->port),
> > > > > long_hpd ? "long" : "short");
> > > > >
> > > > > + if (long_hpd)
> > > > > + return IRQ_NONE;
> > > > > +
> > > > > power_domain =
> > > > > intel_display_port_aux_power_domain(intel_encoder);
> > > > > intel_display_power_get(dev_priv, power_domain);
> > > > >
> > > > > - if (long_hpd) {
> > > > > - intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > - if (intel_dp->is_mst)
> > > > > - ret = IRQ_HANDLED;
> > > > > - goto put_power;
> > > > > -
> > > > > - } else {
> > > > > - if (intel_dp->is_mst) {
> > > > > - if (intel_dp_check_mst_status(intel_dp) ==
> > > > > -EINVAL) {
> > > > > - /*
> > > > > - * If we were in MST mode, and device
> > > > > is not
> > > > > - * there, get out of MST mode
> > > > > - */
> > > > > - DRM_DEBUG_KMS("MST device may have
> > > > > disappeared %d vs %d\n",
> > > > > - intel_dp->is_mst,
> > > > > intel_dp->mst_mgr.mst_state);
> > > > > - intel_dp->is_mst = false;
> > > > > -
> > > > > drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > > > > -
> > > > > intel_dp->is_mst);
> > > > > - goto put_power;
> > > > > - }
> > > > > - }
> > > > > -
> > > > > - if (!intel_dp->is_mst) {
> > > > > - if (!intel_dp_short_pulse(intel_dp)) {
> > > > > -
> > > > > intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > - goto put_power;
> > > > > - }
> > > > > + if (intel_dp->is_mst) {
> > > > > + if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> > > > > + /*
> > > > > + * If we were in MST mode, and device is not
> > > > > + * there, get out of MST mode
> > > > > + */
> > > > > + DRM_DEBUG_KMS("MST device may have disappeared
> > > > > %d vs %d\n",
> > > > > + intel_dp->is_mst,
> > > > > intel_dp->mst_mgr.mst_state);
> > > > > + intel_dp->is_mst = false;
> > > > > +
> > > > > drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > > > > +
> > > > > intel_dp->is_mst);
> > > > > + goto put_power;
> > > > > }
> > > > > + } else {
> > > > > + if (!intel_dp_short_pulse(intel_dp))
> > > > > + goto put_power;
> > > > > }
> > > > >
> > > > > + /*
> > > > > + * Link retraining happens from the hotplug work,
> > > > > + * check if we might need to schdule it.
> > > > > + *
> > > > > + * There has been known issues of link loss
> > > > > + * triggerring long pulse, so let's check both
> > > > > + * for short and long pulse.
> > > > > + */
> > > > > + if (intel_dp_link_needs_retrain(intel_dp))
> > > > > + goto put_power;
>
> What happens in case of link loss triggering long pulse, does this link
> retraining
> also get handled in intel_dp_detect()?
Yes, in fact link training is _only_ done from intel_dp_detect(). So if
we detect a link loss from a short hpd, we'll schedule the full
intel_dp_detect() to retrain the link.
>
> Manasi
> > > > > +
> > > > > ret = IRQ_HANDLED;
> > > > >
> > > > > put_power:
> > > > > --
> > > > > 2.7.4
> > > > >
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
