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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
