| Message ID | 20190123185135.19451-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: Delay hotplug sequence for TC ports | expand |
Hi Jose, On Wed, Jan 23, 2019 at 10:51:35AM -0800, José Roberto de Souza wrote: > Some unpowered USB type-c dongles requires some time to power on > before being able to process aux channel transactions. > > It was not a problem for older platforms because there was a hardware > bridge between DDI/DP ports and type-c controller adding a implicit > delay that hid this issue but ICL have type-c controllers integrated > to the SOC, causing sinks to not be detected. > > So here it test if DP type-c port is responsive before proceed with > hotplug sequence, it try 5 times with a 150ms of delay between tries > before giving up. nice root-causing/reverse engineering of this issue. What you add here is in practice software polling. Since we have already a mechanism for that (drm_kms_helper_poll_enable()), I think the better approach would be to use that, switching that on/off as needed, similarly to how you enable/disable tc_wa_work in this patch. Sure the default kms polling time is 10s, not sure if that's even a problem, given that we are talking about a workaround for broken TypeC dongles (an up to 750ms latency is not acceptable for sure). An other, simpler alternative would be to just msleep(), possibly in a loop retrying the probe. We have quite a few things ahead to fix in the TypeC code, so adding some complicated WA like this here feels like an early micro optimization. Especially, since I don't consider HPD processing a critical fast path (if you do the msleep() outside of important locks). Yet another approach would be to take Ville's HPD injection patchset [1] and use that here and everywhere else in our driver in place of calling the kms poll code. That would also provide us with a way to have some IGT test to exercise the driver's HPD processing in general. --Imre [1] git://github.com/vsyrjala/linux.git vlv_chv_gpio_hpd_irq > > If unpowered TBT dongles shows up in the market this approach should > be extended to TBT ports too. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_ddi.c | 4 + > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_hotplug.c | 108 +++++++++++++++++++++++++++ > 4 files changed, 115 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 03db011caa8e..3fd8c3fa6515 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2639,6 +2639,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv, > enum port port); > bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin); > void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin); > +void intel_hotplug_tc_wa_work(struct work_struct *work); > > /* i915_irq.c */ > static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index b0bb8dfc2ed5..2c149843cacf 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -4258,6 +4258,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > goto err; > > intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; > + > + if (IS_ICELAKE(dev_priv) && intel_port_is_tc(dev_priv, port)) > + INIT_DELAYED_WORK(&intel_dig_port->tc_wa_work, > + intel_hotplug_tc_wa_work); > } > > /* In theory we don't need the encoder->type check, but leave it just in > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 33b733d37706..fbb6f78ce733 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1242,6 +1242,8 @@ struct intel_digital_port { > enum intel_display_power_domain ddi_io_power_domain; > bool tc_legacy_port:1; > enum tc_port_type tc_type; > + struct delayed_work tc_wa_work; > + u8 tc_wa_count; > > void (*write_infoframe)(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state, > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c > index e027d2b4abe5..9838b60e8f63 100644 > --- a/drivers/gpu/drm/i915/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > @@ -342,6 +342,74 @@ static void i915_digport_work_func(struct work_struct *work) > } > } > > +#define TC_WA_DELAY_MSEC 150 > +#define TC_WA_TRIES 5 > + > +/* > + * Test if type-c dongle is responsive return true if so otherwise schedule a > + * work to try again and return false. > + */ > +static bool intel_hotplug_tc_wa_test(struct intel_digital_port *dig_port) > +{ > + struct intel_dp *intel_dp = &dig_port->dp; > + u8 buffer; > + > + dig_port->tc_wa_count++; > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DPCD_REV, &buffer, 1) != 1) > + goto not_responsive; > + > + if (!drm_probe_ddc(&intel_dp->aux.ddc)) > + goto not_responsive; > + > + return true; > + > +not_responsive: > + if (dig_port->tc_wa_count < TC_WA_TRIES) { > + const unsigned long delay = msecs_to_jiffies(TC_WA_DELAY_MSEC); > + > + schedule_delayed_work(&dig_port->tc_wa_work, delay); > + } else { > + DRM_DEBUG_KMS("TC port not responsive, giving up\n"); > + } > + > + return false; > +} > + > +void intel_hotplug_tc_wa_work(struct work_struct *work) > +{ > + struct delayed_work *delayed_work = to_delayed_work(work); > + struct intel_digital_port *dig_port = container_of(delayed_work, > + struct intel_digital_port, > + tc_wa_work); > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + struct intel_encoder *intel_encoder = &dig_port->base; > + struct intel_dp *intel_dp = &dig_port->dp; > + struct intel_connector *intel_connector = intel_dp->attached_connector; > + struct drm_device *dev = &dev_priv->drm; > + intel_wakeref_t wakeref; > + bool ret; > + > + wakeref = intel_runtime_pm_get(dev_priv); > + > + if (!intel_digital_port_connected(intel_encoder) || > + dig_port->tc_type != TC_PORT_TYPEC) { > + intel_runtime_pm_put(dev_priv, wakeref); > + return; > + } > + > + mutex_lock(&dev->mode_config.mutex); > + ret = intel_hotplug_tc_wa_test(dig_port); > + if (ret) > + ret = intel_encoder->hotplug(intel_encoder, intel_connector); > + mutex_unlock(&dev->mode_config.mutex); > + > + if (ret) > + drm_kms_helper_hotplug_event(dev); > + > + intel_runtime_pm_put(dev_priv, wakeref); > +} > + > /* > * Handle hotplug events outside the interrupt handler proper. > */ > @@ -377,9 +445,37 @@ static void i915_hotplug_work_func(struct work_struct *work) > continue; > intel_encoder = intel_connector->encoder; > if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { > + struct intel_digital_port *dig_port = enc_to_dig_port(&intel_encoder->base); > + > DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n", > connector->name, intel_encoder->hpd_pin); > > + /* > + * Type-C dongles WA: Unpowered type-C dongles can take > + * some time to be responsible, so let's try to do a > + * DPCD read to check if it is ready, otherwise skip > + * the call to the hotplug callback and schedule a work > + * to try a DPCD read again after a few micro-seconds. > + */ > + if (IS_ICELAKE(dev_priv) && > + intel_port_is_tc(dev_priv, intel_encoder->port)) { > + intel_wakeref_t wakeref; > + bool skip_hotplug = false; > + > + wakeref = intel_runtime_pm_get(dev_priv); > + > + if (intel_digital_port_connected(intel_encoder) && > + dig_port->tc_type == TC_PORT_TYPEC) { > + dig_port->tc_wa_count = 0; > + skip_hotplug = !intel_hotplug_tc_wa_test(dig_port); > + } > + > + intel_runtime_pm_put(dev_priv, wakeref); > + > + if (skip_hotplug) > + continue; > + } > + > changed |= intel_encoder->hotplug(intel_encoder, > intel_connector); > } > @@ -644,6 +740,8 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv) > > void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > { > + struct intel_encoder *encoder; > + > spin_lock_irq(&dev_priv->irq_lock); > > dev_priv->hotplug.long_port_mask = 0; > @@ -656,6 +754,16 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > cancel_work_sync(&dev_priv->hotplug.hotplug_work); > cancel_work_sync(&dev_priv->hotplug.poll_init_work); > cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work); > + > + for_each_intel_encoder(&dev_priv->drm, encoder) { > + struct intel_digital_port *dig_port; > + > + dig_port = enc_to_dig_port(&encoder->base); > + /* tc_wa_work is only initialized for TC with DP ports */ > + if (IS_ICELAKE(dev_priv) && dig_port && dig_port->hpd_pulse) > + cancel_delayed_work_sync(&dig_port->tc_wa_work); > + } > + > } > > bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin) > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 03db011caa8e..3fd8c3fa6515 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2639,6 +2639,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv, enum port port); bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin); void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin); +void intel_hotplug_tc_wa_work(struct work_struct *work); /* i915_irq.c */ static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index b0bb8dfc2ed5..2c149843cacf 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -4258,6 +4258,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) goto err; intel_dig_port->hpd_pulse = intel_dp_hpd_pulse; + + if (IS_ICELAKE(dev_priv) && intel_port_is_tc(dev_priv, port)) + INIT_DELAYED_WORK(&intel_dig_port->tc_wa_work, + intel_hotplug_tc_wa_work); } /* In theory we don't need the encoder->type check, but leave it just in diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 33b733d37706..fbb6f78ce733 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1242,6 +1242,8 @@ struct intel_digital_port { enum intel_display_power_domain ddi_io_power_domain; bool tc_legacy_port:1; enum tc_port_type tc_type; + struct delayed_work tc_wa_work; + u8 tc_wa_count; void (*write_infoframe)(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state, diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c index e027d2b4abe5..9838b60e8f63 100644 --- a/drivers/gpu/drm/i915/intel_hotplug.c +++ b/drivers/gpu/drm/i915/intel_hotplug.c @@ -342,6 +342,74 @@ static void i915_digport_work_func(struct work_struct *work) } } +#define TC_WA_DELAY_MSEC 150 +#define TC_WA_TRIES 5 + +/* + * Test if type-c dongle is responsive return true if so otherwise schedule a + * work to try again and return false. + */ +static bool intel_hotplug_tc_wa_test(struct intel_digital_port *dig_port) +{ + struct intel_dp *intel_dp = &dig_port->dp; + u8 buffer; + + dig_port->tc_wa_count++; + + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DPCD_REV, &buffer, 1) != 1) + goto not_responsive; + + if (!drm_probe_ddc(&intel_dp->aux.ddc)) + goto not_responsive; + + return true; + +not_responsive: + if (dig_port->tc_wa_count < TC_WA_TRIES) { + const unsigned long delay = msecs_to_jiffies(TC_WA_DELAY_MSEC); + + schedule_delayed_work(&dig_port->tc_wa_work, delay); + } else { + DRM_DEBUG_KMS("TC port not responsive, giving up\n"); + } + + return false; +} + +void intel_hotplug_tc_wa_work(struct work_struct *work) +{ + struct delayed_work *delayed_work = to_delayed_work(work); + struct intel_digital_port *dig_port = container_of(delayed_work, + struct intel_digital_port, + tc_wa_work); + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + struct intel_encoder *intel_encoder = &dig_port->base; + struct intel_dp *intel_dp = &dig_port->dp; + struct intel_connector *intel_connector = intel_dp->attached_connector; + struct drm_device *dev = &dev_priv->drm; + intel_wakeref_t wakeref; + bool ret; + + wakeref = intel_runtime_pm_get(dev_priv); + + if (!intel_digital_port_connected(intel_encoder) || + dig_port->tc_type != TC_PORT_TYPEC) { + intel_runtime_pm_put(dev_priv, wakeref); + return; + } + + mutex_lock(&dev->mode_config.mutex); + ret = intel_hotplug_tc_wa_test(dig_port); + if (ret) + ret = intel_encoder->hotplug(intel_encoder, intel_connector); + mutex_unlock(&dev->mode_config.mutex); + + if (ret) + drm_kms_helper_hotplug_event(dev); + + intel_runtime_pm_put(dev_priv, wakeref); +} + /* * Handle hotplug events outside the interrupt handler proper. */ @@ -377,9 +445,37 @@ static void i915_hotplug_work_func(struct work_struct *work) continue; intel_encoder = intel_connector->encoder; if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { + struct intel_digital_port *dig_port = enc_to_dig_port(&intel_encoder->base); + DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n", connector->name, intel_encoder->hpd_pin); + /* + * Type-C dongles WA: Unpowered type-C dongles can take + * some time to be responsible, so let's try to do a + * DPCD read to check if it is ready, otherwise skip + * the call to the hotplug callback and schedule a work + * to try a DPCD read again after a few micro-seconds. + */ + if (IS_ICELAKE(dev_priv) && + intel_port_is_tc(dev_priv, intel_encoder->port)) { + intel_wakeref_t wakeref; + bool skip_hotplug = false; + + wakeref = intel_runtime_pm_get(dev_priv); + + if (intel_digital_port_connected(intel_encoder) && + dig_port->tc_type == TC_PORT_TYPEC) { + dig_port->tc_wa_count = 0; + skip_hotplug = !intel_hotplug_tc_wa_test(dig_port); + } + + intel_runtime_pm_put(dev_priv, wakeref); + + if (skip_hotplug) + continue; + } + changed |= intel_encoder->hotplug(intel_encoder, intel_connector); } @@ -644,6 +740,8 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv) void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) { + struct intel_encoder *encoder; + spin_lock_irq(&dev_priv->irq_lock); dev_priv->hotplug.long_port_mask = 0; @@ -656,6 +754,16 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) cancel_work_sync(&dev_priv->hotplug.hotplug_work); cancel_work_sync(&dev_priv->hotplug.poll_init_work); cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work); + + for_each_intel_encoder(&dev_priv->drm, encoder) { + struct intel_digital_port *dig_port; + + dig_port = enc_to_dig_port(&encoder->base); + /* tc_wa_work is only initialized for TC with DP ports */ + if (IS_ICELAKE(dev_priv) && dig_port && dig_port->hpd_pulse) + cancel_delayed_work_sync(&dig_port->tc_wa_work); + } + } bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin)
Some unpowered USB type-c dongles requires some time to power on before being able to process aux channel transactions. It was not a problem for older platforms because there was a hardware bridge between DDI/DP ports and type-c controller adding a implicit delay that hid this issue but ICL have type-c controllers integrated to the SOC, causing sinks to not be detected. So here it test if DP type-c port is responsive before proceed with hotplug sequence, it try 5 times with a 150ms of delay between tries before giving up. If unpowered TBT dongles shows up in the market this approach should be extended to TBT ports too. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_ddi.c | 4 + drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_hotplug.c | 108 +++++++++++++++++++++++++++ 4 files changed, 115 insertions(+)