[PATCH 1/2] drm/i915: Add support for retrying hotplug

[PATCH 1/2] drm/i915: Add support for retrying hotplug

[José Roberto de Souza]

From: Imre Deak <imre.deak@intel.com>

There is some scenarios that we are aware that sink probe can fail,
so lets add the infrastructure to let hotplug() hook to request
another probe after some time.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  3 +-
 drivers/gpu/drm/i915/intel_ddi.c     | 12 +++---
 drivers/gpu/drm/i915/intel_dp.c      | 12 +++---
 drivers/gpu/drm/i915/intel_drv.h     | 17 +++++++--
 drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_sdvo.c    |  8 ++--
 7 files changed, 79 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 37175414ce89..8149be62a630 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
 	 */
 	synchronize_irq(dev_priv->drm.irq);
 	flush_work(&dev_priv->hotplug.dig_port_work);
-	flush_work(&dev_priv->hotplug.hotplug_work);
+	flush_delayed_work(&dev_priv->hotplug.hotplug_work);
 
 	seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
 	seq_printf(m, "Detected: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bb0e75e43987..5703526bddab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -156,7 +156,7 @@ enum hpd_pin {
 #define HPD_STORM_DEFAULT_THRESHOLD 50
 
 struct i915_hotplug {
-	struct work_struct hotplug_work;
+	struct delayed_work hotplug_work;
 
 	struct {
 		unsigned long last_jiffies;
@@ -168,6 +168,7 @@ struct i915_hotplug {
 		} state;
 	} stats[HPD_NUM_PINS];
 	u32 event_bits;
+	u32 retry_bits;
 	struct delayed_work reenable_work;
 
 	u32 long_port_mask;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ea83071a22c4..1676a87f18cb 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
 	return modeset_pipe(&crtc->base, ctx);
 }
 
-static bool intel_ddi_hotplug(struct intel_encoder *encoder,
-			      struct intel_connector *connector)
+static enum intel_hotplug_state
+intel_ddi_hotplug(struct intel_encoder *encoder,
+		  struct intel_connector *connector,
+		  bool irq_received)
 {
 	struct drm_modeset_acquire_ctx ctx;
-	bool changed;
+	enum intel_hotplug_state state;
 	int ret;
 
-	changed = intel_encoder_hotplug(encoder, connector);
+	state = intel_encoder_hotplug(encoder, connector, irq_received);
 
 	drm_modeset_acquire_init(&ctx, 0);
 
@@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
 	drm_modeset_acquire_fini(&ctx);
 	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
 
-	return changed;
+	return state;
 }
 
 static struct intel_connector *
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1a051c0fbfe..0bb43eaa154c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
  * retrain the link to get a picture. That's in case no
  * userspace component reacted to intermittent HPD dip.
  */
-static bool intel_dp_hotplug(struct intel_encoder *encoder,
-			     struct intel_connector *connector)
+static enum intel_hotplug_state
+intel_dp_hotplug(struct intel_encoder *encoder,
+		 struct intel_connector *connector,
+		 bool irq_received)
 {
 	struct drm_modeset_acquire_ctx ctx;
-	bool changed;
+	enum intel_hotplug_state state;
 	int ret;
 
-	changed = intel_encoder_hotplug(encoder, connector);
+	state = intel_encoder_hotplug(encoder, connector, irq_received);
 
 	drm_modeset_acquire_init(&ctx, 0);
 
@@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
 	drm_modeset_acquire_fini(&ctx);
 	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
 
-	return changed;
+	return state;
 }
 
 static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81ec73e4a083..de62af970dc1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -226,14 +226,21 @@ struct intel_fbdev {
 	struct mutex hpd_lock;
 };
 
+enum intel_hotplug_state {
+	INTEL_HOTPLUG_NOCHANGE,
+	INTEL_HOTPLUG_CHANGED,
+	INTEL_HOTPLUG_RETRY,
+};
+
 struct intel_encoder {
 	struct drm_encoder base;
 
 	enum intel_output_type type;
 	enum port port;
 	unsigned int cloneable;
-	bool (*hotplug)(struct intel_encoder *encoder,
-			struct intel_connector *connector);
+	enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
+					    struct intel_connector *connector,
+					    bool irq_received);
 	enum intel_output_type (*compute_output_type)(struct intel_encoder *,
 						      struct intel_crtc_state *,
 						      struct drm_connector_state *);
@@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
 void intel_dvo_init(struct drm_i915_private *dev_priv);
 /* intel_hotplug.c */
 void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
-bool intel_encoder_hotplug(struct intel_encoder *encoder,
-			   struct intel_connector *connector);
+enum intel_hotplug_state
+intel_encoder_hotplug(struct intel_encoder *encoder,
+		      struct intel_connector *connector,
+		      bool irq_received);
 
 /* legacy fbdev emulation in intel_fbdev.c */
 #ifdef CONFIG_DRM_FBDEV_EMULATION
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index b8937c788f03..3eaa06526f6b 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
 
 #define HPD_STORM_DETECT_PERIOD		1000
 #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
+#define HPD_RETRY_DELAY			1000
 
 /**
  * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
@@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
 	intel_runtime_pm_put(dev_priv, wakeref);
 }
 
-bool intel_encoder_hotplug(struct intel_encoder *encoder,
-			   struct intel_connector *connector)
+enum intel_hotplug_state
+intel_encoder_hotplug(struct intel_encoder *encoder,
+		      struct intel_connector *connector,
+		      bool irq_received)
 {
 	struct drm_device *dev = connector->base.dev;
 	enum drm_connector_status old_status;
@@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
 		drm_helper_probe_detect(&connector->base, NULL, false);
 
 	if (old_status == connector->base.status)
-		return false;
+		return INTEL_HOTPLUG_NOCHANGE;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
 		      connector->base.base.id,
@@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
 		      drm_get_connector_status_name(old_status),
 		      drm_get_connector_status_name(connector->base.status));
 
-	return true;
+	return INTEL_HOTPLUG_CHANGED;
 }
 
 static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
@@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
 		spin_lock_irq(&dev_priv->irq_lock);
 		dev_priv->hotplug.event_bits |= old_bits;
 		spin_unlock_irq(&dev_priv->irq_lock);
-		schedule_work(&dev_priv->hotplug.hotplug_work);
+		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
 	}
 }
 
@@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
 static void i915_hotplug_work_func(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
+		container_of(work, struct drm_i915_private,
+			     hotplug.hotplug_work.work);
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_connector *intel_connector;
 	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	bool changed = false;
+	u32 retry = 0;
 	u32 hpd_event_bits;
+	u32 hpd_retry_bits;
 
 	mutex_lock(&dev->mode_config.mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
@@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	hpd_event_bits = dev_priv->hotplug.event_bits;
 	dev_priv->hotplug.event_bits = 0;
+	hpd_retry_bits = dev_priv->hotplug.retry_bits;
+	dev_priv->hotplug.retry_bits = 0;
 
 	/* Enable polling for connectors which had HPD IRQ storms */
 	intel_hpd_irq_storm_switch_to_polling(dev_priv);
@@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
+		u32 hpd_bit;
+
 		intel_connector = to_intel_connector(connector);
 		if (!intel_connector->encoder)
 			continue;
 		intel_encoder = intel_connector->encoder;
-		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+		hpd_bit = BIT(intel_encoder->hpd_pin);
+		if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
 			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
 				      connector->name, intel_encoder->hpd_pin);
 
-			changed |= intel_encoder->hotplug(intel_encoder,
-							  intel_connector);
+			switch (intel_encoder->hotplug(intel_encoder,
+						       intel_connector,
+						       hpd_event_bits & hpd_bit)) {
+			case INTEL_HOTPLUG_NOCHANGE:
+				break;
+			case INTEL_HOTPLUG_CHANGED:
+				changed = true;
+				break;
+			case INTEL_HOTPLUG_RETRY:
+				retry |= hpd_bit;
+				break;
+			}
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
@@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
+
+	if (retry) {
+		spin_lock_irq(&dev_priv->irq_lock);
+		dev_priv->hotplug.retry_bits |= retry;
+		spin_unlock_irq(&dev_priv->irq_lock);
+
+		mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
+				 msecs_to_jiffies(HPD_RETRY_DELAY));
+	}
 }
 
 
@@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 	if (queue_dig)
 		queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
 	if (queue_hp)
-		schedule_work(&dev_priv->hotplug.hotplug_work);
+		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
 }
 
 /**
@@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
 
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 {
-	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
+	INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
+			  i915_hotplug_work_func);
 	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
 	INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
 	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
@@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 	dev_priv->hotplug.long_port_mask = 0;
 	dev_priv->hotplug.short_port_mask = 0;
 	dev_priv->hotplug.event_bits = 0;
+	dev_priv->hotplug.retry_bits = 0;
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
-	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
+	cancel_delayed_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);
 }
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e7b0884ba5a5..92793475b2b9 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
 			     &intel_sdvo->hotplug_active, 2);
 }
 
-static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
-			       struct intel_connector *connector)
+static enum intel_hotplug_state
+intel_sdvo_hotplug(struct intel_encoder *encoder,
+		   struct intel_connector *connector,
+		   bool irq_received)
 {
 	intel_sdvo_enable_hotplug(encoder);
 
-	return intel_encoder_hotplug(encoder, connector);
+	return intel_encoder_hotplug(encoder, connector, irq_received);
 }
 
 static bool
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[José Roberto de Souza]

Unpowered type-c dongles can take some time to boot and be
responsible, causing the probe to fail and sink never be detected
without further actions from userspace.

It was not a issue 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 bring this issue to users.

So here if the probe failed when coming from a IRQ it returns
INTEL_HOTPLUG_RETRY that will schedule another run of
i915_hotplug_work_func() after 1 second what is time enough for
those type-c dongles to boot.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1676a87f18cb..96bbcf5c9787 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
 		  struct intel_connector *connector,
 		  bool irq_received)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 	struct drm_modeset_acquire_ctx ctx;
 	enum intel_hotplug_state state;
 	int ret;
@@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
 	drm_modeset_acquire_fini(&ctx);
 	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
 
+	/*
+	 * Unpowered type-c dongles can take some time to boot and be
+	 * responsible, so here giving some type to those dongles to power up
+	 * and then retrying the probe.
+	 */
+	if (state == INTEL_HOTPLUG_NOCHANGE &&
+	    connector->base.status != connector_status_connected &&
+	    irq_received && intel_port_is_tc(dev_priv, encoder->port) &&
+	    !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
+		state = INTEL_HOTPLUG_RETRY;
+
 	return state;
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Add support for retrying hotplug

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add support for retrying hotplug
URL   : https://patchwork.freedesktop.org/series/57108/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Add support for retrying hotplug
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3583:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3584:16: warning: expression using sizeof(void)

Commit: drm/i915/icl: Probe again type-c connectors that failed
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Add support for retrying hotplug

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add support for retrying hotplug
URL   : https://patchwork.freedesktop.org/series/57108/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5654 -> Patchwork_12286
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12286 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12286, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57108/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12286:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7567u:       PASS -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12286 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@memory-alloc:
    - fi-ivb-3520m:       NOTRUN -> SKIP [fdo#109271] +48

  * igt@amdgpu/amd_cs_nop@fork-compute0:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +18

  * igt@amdgpu/amd_cs_nop@fork-gfx0:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109315] +17

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#103558] / [fdo#105602]

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_parse@basic-allowed:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109289] +1

  * igt@gem_exec_suspend@basic-s3:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602]

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#105602] / [fdo#108529] +1

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#108529]

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       PASS -> SKIP [fdo#109271] / [fdo#109278] +2
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  * igt@kms_chamelium@dp-edid-read:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109316] +2

  * igt@kms_chamelium@dp-hpd-fast:
    - fi-skl-6700k2:      NOTRUN -> SKIP [fdo#109271] +41

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-icl-u2:          NOTRUN -> FAIL [fdo#109635 ]

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-kbl-7567u:       PASS -> FAIL [fdo#109569] +1

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       PASS -> FAIL [fdo#109485]

  * igt@kms_chamelium@vga-hpd-fast:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109309] +1

  * igt@kms_force_connector_basic@prune-stale-modes:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-kbl-7567u:       PASS -> DMESG-FAIL [fdo#105079]

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7567u:       PASS -> SKIP [fdo#109271] +4

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@i915_pm_rpm@module-reload:
    - {fi-icl-y}:         INCOMPLETE [fdo#108840] -> PASS

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       DMESG-WARN [fdo#107709] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#109569]: https://bugs.freedesktop.org/show_bug.cgi?id=109569
  [fdo#109635 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109635 


Participating hosts (43 -> 39)
------------------------------

  Additional (3): fi-ivb-3520m fi-icl-u2 fi-skl-6700k2 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5654 -> Patchwork_12286

  CI_DRM_5654: 30c7f283790b433aa311ef7a7d2b6b428886fb9a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4851: 2b7dd10a4e2ea0cabff68421fd15e96c99be3cad @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12286: 83788414ca236a50363703647234c6420a46f292 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

83788414ca23 drm/i915/icl: Probe again type-c connectors that failed
0c881f78fc0a drm/i915: Add support for retrying hotplug

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12286/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug

[Jani Nikula]

On Fri, 22 Feb 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> From: Imre Deak <imre.deak@intel.com>
>
> There is some scenarios that we are aware that sink probe can fail,
> so lets add the infrastructure to let hotplug() hook to request
> another probe after some time.

See also

http://patchwork.freedesktop.org/patch/msgid/20180925071836.24711-1-jani.nikula@intel.com

BR,
Jani.

>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_ddi.c     | 12 +++---
>  drivers/gpu/drm/i915/intel_dp.c      | 12 +++---
>  drivers/gpu/drm/i915/intel_drv.h     | 17 +++++++--
>  drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_sdvo.c    |  8 ++--
>  7 files changed, 79 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 37175414ce89..8149be62a630 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
>  	 */
>  	synchronize_irq(dev_priv->drm.irq);
>  	flush_work(&dev_priv->hotplug.dig_port_work);
> -	flush_work(&dev_priv->hotplug.hotplug_work);
> +	flush_delayed_work(&dev_priv->hotplug.hotplug_work);
>  
>  	seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
>  	seq_printf(m, "Detected: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb0e75e43987..5703526bddab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -156,7 +156,7 @@ enum hpd_pin {
>  #define HPD_STORM_DEFAULT_THRESHOLD 50
>  
>  struct i915_hotplug {
> -	struct work_struct hotplug_work;
> +	struct delayed_work hotplug_work;
>  
>  	struct {
>  		unsigned long last_jiffies;
> @@ -168,6 +168,7 @@ struct i915_hotplug {
>  		} state;
>  	} stats[HPD_NUM_PINS];
>  	u32 event_bits;
> +	u32 retry_bits;
>  	struct delayed_work reenable_work;
>  
>  	u32 long_port_mask;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ea83071a22c4..1676a87f18cb 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
>  	return modeset_pipe(&crtc->base, ctx);
>  }
>  
> -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> -			      struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_ddi_hotplug(struct intel_encoder *encoder,
> +		  struct intel_connector *connector,
> +		  bool irq_received)
>  {
>  	struct drm_modeset_acquire_ctx ctx;
> -	bool changed;
> +	enum intel_hotplug_state state;
>  	int ret;
>  
> -	changed = intel_encoder_hotplug(encoder, connector);
> +	state = intel_encoder_hotplug(encoder, connector, irq_received);
>  
>  	drm_modeset_acquire_init(&ctx, 0);
>  
> @@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
>  	drm_modeset_acquire_fini(&ctx);
>  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> -	return changed;
> +	return state;
>  }
>  
>  static struct intel_connector *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1a051c0fbfe..0bb43eaa154c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
>   * retrain the link to get a picture. That's in case no
>   * userspace component reacted to intermittent HPD dip.
>   */
> -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> -			     struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_dp_hotplug(struct intel_encoder *encoder,
> +		 struct intel_connector *connector,
> +		 bool irq_received)
>  {
>  	struct drm_modeset_acquire_ctx ctx;
> -	bool changed;
> +	enum intel_hotplug_state state;
>  	int ret;
>  
> -	changed = intel_encoder_hotplug(encoder, connector);
> +	state = intel_encoder_hotplug(encoder, connector, irq_received);
>  
>  	drm_modeset_acquire_init(&ctx, 0);
>  
> @@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
>  	drm_modeset_acquire_fini(&ctx);
>  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> -	return changed;
> +	return state;
>  }
>  
>  static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81ec73e4a083..de62af970dc1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -226,14 +226,21 @@ struct intel_fbdev {
>  	struct mutex hpd_lock;
>  };
>  
> +enum intel_hotplug_state {
> +	INTEL_HOTPLUG_NOCHANGE,
> +	INTEL_HOTPLUG_CHANGED,
> +	INTEL_HOTPLUG_RETRY,
> +};
> +
>  struct intel_encoder {
>  	struct drm_encoder base;
>  
>  	enum intel_output_type type;
>  	enum port port;
>  	unsigned int cloneable;
> -	bool (*hotplug)(struct intel_encoder *encoder,
> -			struct intel_connector *connector);
> +	enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
> +					    struct intel_connector *connector,
> +					    bool irq_received);
>  	enum intel_output_type (*compute_output_type)(struct intel_encoder *,
>  						      struct intel_crtc_state *,
>  						      struct drm_connector_state *);
> @@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
>  void intel_dvo_init(struct drm_i915_private *dev_priv);
>  /* intel_hotplug.c */
>  void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> -			   struct intel_connector *connector);
> +enum intel_hotplug_state
> +intel_encoder_hotplug(struct intel_encoder *encoder,
> +		      struct intel_connector *connector,
> +		      bool irq_received);
>  
>  /* legacy fbdev emulation in intel_fbdev.c */
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index b8937c788f03..3eaa06526f6b 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>  
>  #define HPD_STORM_DETECT_PERIOD		1000
>  #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
> +#define HPD_RETRY_DELAY			1000
>  
>  /**
>   * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
> @@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  	intel_runtime_pm_put(dev_priv, wakeref);
>  }
>  
> -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> -			   struct intel_connector *connector)
> +enum intel_hotplug_state
> +intel_encoder_hotplug(struct intel_encoder *encoder,
> +		      struct intel_connector *connector,
> +		      bool irq_received)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	enum drm_connector_status old_status;
> @@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
>  		drm_helper_probe_detect(&connector->base, NULL, false);
>  
>  	if (old_status == connector->base.status)
> -		return false;
> +		return INTEL_HOTPLUG_NOCHANGE;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
>  		      connector->base.base.id,
> @@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
>  		      drm_get_connector_status_name(old_status),
>  		      drm_get_connector_status_name(connector->base.status));
>  
> -	return true;
> +	return INTEL_HOTPLUG_CHANGED;
>  }
>  
>  static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
> @@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
>  		spin_lock_irq(&dev_priv->irq_lock);
>  		dev_priv->hotplug.event_bits |= old_bits;
>  		spin_unlock_irq(&dev_priv->irq_lock);
> -		schedule_work(&dev_priv->hotplug.hotplug_work);
> +		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
>  	}
>  }
>  
> @@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
>  static void i915_hotplug_work_func(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
> -		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
> +		container_of(work, struct drm_i915_private,
> +			     hotplug.hotplug_work.work);
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_connector *intel_connector;
>  	struct intel_encoder *intel_encoder;
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	bool changed = false;
> +	u32 retry = 0;
>  	u32 hpd_event_bits;
> +	u32 hpd_retry_bits;
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
> @@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	hpd_event_bits = dev_priv->hotplug.event_bits;
>  	dev_priv->hotplug.event_bits = 0;
> +	hpd_retry_bits = dev_priv->hotplug.retry_bits;
> +	dev_priv->hotplug.retry_bits = 0;
>  
>  	/* Enable polling for connectors which had HPD IRQ storms */
>  	intel_hpd_irq_storm_switch_to_polling(dev_priv);
> @@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> +		u32 hpd_bit;
> +
>  		intel_connector = to_intel_connector(connector);
>  		if (!intel_connector->encoder)
>  			continue;
>  		intel_encoder = intel_connector->encoder;
> -		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +		hpd_bit = BIT(intel_encoder->hpd_pin);
> +		if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
>  			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
>  				      connector->name, intel_encoder->hpd_pin);
>  
> -			changed |= intel_encoder->hotplug(intel_encoder,
> -							  intel_connector);
> +			switch (intel_encoder->hotplug(intel_encoder,
> +						       intel_connector,
> +						       hpd_event_bits & hpd_bit)) {
> +			case INTEL_HOTPLUG_NOCHANGE:
> +				break;
> +			case INTEL_HOTPLUG_CHANGED:
> +				changed = true;
> +				break;
> +			case INTEL_HOTPLUG_RETRY:
> +				retry |= hpd_bit;
> +				break;
> +			}
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	if (changed)
>  		drm_kms_helper_hotplug_event(dev);
> +
> +	if (retry) {
> +		spin_lock_irq(&dev_priv->irq_lock);
> +		dev_priv->hotplug.retry_bits |= retry;
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +
> +		mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
> +				 msecs_to_jiffies(HPD_RETRY_DELAY));
> +	}
>  }
>  
>  
> @@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  	if (queue_dig)
>  		queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
>  	if (queue_hp)
> -		schedule_work(&dev_priv->hotplug.hotplug_work);
> +		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
>  }
>  
>  /**
> @@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
>  
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  {
> -	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> +	INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> +			  i915_hotplug_work_func);
>  	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
>  	INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
>  	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
> @@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
>  	dev_priv->hotplug.long_port_mask = 0;
>  	dev_priv->hotplug.short_port_mask = 0;
>  	dev_priv->hotplug.event_bits = 0;
> +	dev_priv->hotplug.retry_bits = 0;
>  
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
>  	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> -	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> +	cancel_delayed_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);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index e7b0884ba5a5..92793475b2b9 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
>  			     &intel_sdvo->hotplug_active, 2);
>  }
>  
> -static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
> -			       struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_sdvo_hotplug(struct intel_encoder *encoder,
> +		   struct intel_connector *connector,
> +		   bool irq_received)
>  {
>  	intel_sdvo_enable_hotplug(encoder);
>  
> -	return intel_encoder_hotplug(encoder, connector);
> +	return intel_encoder_hotplug(encoder, connector, irq_received);
>  }
>  
>  static bool

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug

[Imre Deak]

On Mon, Feb 25, 2019 at 01:02:45PM +0200, Jani Nikula wrote:
> On Fri, 22 Feb 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> > From: Imre Deak <imre.deak@intel.com>
> >
> > There is some scenarios that we are aware that sink probe can fail,
> > so lets add the infrastructure to let hotplug() hook to request
> > another probe after some time.
> 
> See also
> 
> http://patchwork.freedesktop.org/patch/msgid/20180925071836.24711-1-jani.nikula@intel.com

Yes, the idea for a generic way to retry the hotplug detection came
after discussing with Ville and Jani about the similar issue with HDMI
sinks. The only difference wrt. Jani's patch above is that here we only
delay the detection if the connector probe thinks it's necessary. This
then also allows for a longer (up to 1sec) delay needed by the buggy
TypeC dongles.

> 
> BR,
> Jani.
> 
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
> >  drivers/gpu/drm/i915/intel_ddi.c     | 12 +++---
> >  drivers/gpu/drm/i915/intel_dp.c      | 12 +++---
> >  drivers/gpu/drm/i915/intel_drv.h     | 17 +++++++--
> >  drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_sdvo.c    |  8 ++--
> >  7 files changed, 79 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 37175414ce89..8149be62a630 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
> >  	 */
> >  	synchronize_irq(dev_priv->drm.irq);
> >  	flush_work(&dev_priv->hotplug.dig_port_work);
> > -	flush_work(&dev_priv->hotplug.hotplug_work);
> > +	flush_delayed_work(&dev_priv->hotplug.hotplug_work);
> >  
> >  	seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
> >  	seq_printf(m, "Detected: %s\n",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bb0e75e43987..5703526bddab 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -156,7 +156,7 @@ enum hpd_pin {
> >  #define HPD_STORM_DEFAULT_THRESHOLD 50
> >  
> >  struct i915_hotplug {
> > -	struct work_struct hotplug_work;
> > +	struct delayed_work hotplug_work;
> >  
> >  	struct {
> >  		unsigned long last_jiffies;
> > @@ -168,6 +168,7 @@ struct i915_hotplug {
> >  		} state;
> >  	} stats[HPD_NUM_PINS];
> >  	u32 event_bits;
> > +	u32 retry_bits;
> >  	struct delayed_work reenable_work;
> >  
> >  	u32 long_port_mask;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ea83071a22c4..1676a87f18cb 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
> >  	return modeset_pipe(&crtc->base, ctx);
> >  }
> >  
> > -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> > -			      struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_ddi_hotplug(struct intel_encoder *encoder,
> > +		  struct intel_connector *connector,
> > +		  bool irq_received)
> >  {
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	bool changed;
> > +	enum intel_hotplug_state state;
> >  	int ret;
> >  
> > -	changed = intel_encoder_hotplug(encoder, connector);
> > +	state = intel_encoder_hotplug(encoder, connector, irq_received);
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> >  
> > @@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> >  	drm_modeset_acquire_fini(&ctx);
> >  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >  
> > -	return changed;
> > +	return state;
> >  }
> >  
> >  static struct intel_connector *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e1a051c0fbfe..0bb43eaa154c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
> >   * retrain the link to get a picture. That's in case no
> >   * userspace component reacted to intermittent HPD dip.
> >   */
> > -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> > -			     struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_dp_hotplug(struct intel_encoder *encoder,
> > +		 struct intel_connector *connector,
> > +		 bool irq_received)
> >  {
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	bool changed;
> > +	enum intel_hotplug_state state;
> >  	int ret;
> >  
> > -	changed = intel_encoder_hotplug(encoder, connector);
> > +	state = intel_encoder_hotplug(encoder, connector, irq_received);
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> >  
> > @@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
> >  	drm_modeset_acquire_fini(&ctx);
> >  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >  
> > -	return changed;
> > +	return state;
> >  }
> >  
> >  static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 81ec73e4a083..de62af970dc1 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -226,14 +226,21 @@ struct intel_fbdev {
> >  	struct mutex hpd_lock;
> >  };
> >  
> > +enum intel_hotplug_state {
> > +	INTEL_HOTPLUG_NOCHANGE,
> > +	INTEL_HOTPLUG_CHANGED,
> > +	INTEL_HOTPLUG_RETRY,
> > +};
> > +
> >  struct intel_encoder {
> >  	struct drm_encoder base;
> >  
> >  	enum intel_output_type type;
> >  	enum port port;
> >  	unsigned int cloneable;
> > -	bool (*hotplug)(struct intel_encoder *encoder,
> > -			struct intel_connector *connector);
> > +	enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
> > +					    struct intel_connector *connector,
> > +					    bool irq_received);
> >  	enum intel_output_type (*compute_output_type)(struct intel_encoder *,
> >  						      struct intel_crtc_state *,
> >  						      struct drm_connector_state *);
> > @@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
> >  void intel_dvo_init(struct drm_i915_private *dev_priv);
> >  /* intel_hotplug.c */
> >  void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > -			   struct intel_connector *connector);
> > +enum intel_hotplug_state
> > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > +		      struct intel_connector *connector,
> > +		      bool irq_received);
> >  
> >  /* legacy fbdev emulation in intel_fbdev.c */
> >  #ifdef CONFIG_DRM_FBDEV_EMULATION
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> > index b8937c788f03..3eaa06526f6b 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
> >  
> >  #define HPD_STORM_DETECT_PERIOD		1000
> >  #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
> > +#define HPD_RETRY_DELAY			1000
> >  
> >  /**
> >   * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
> > @@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> >  	intel_runtime_pm_put(dev_priv, wakeref);
> >  }
> >  
> > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > -			   struct intel_connector *connector)
> > +enum intel_hotplug_state
> > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > +		      struct intel_connector *connector,
> > +		      bool irq_received)
> >  {
> >  	struct drm_device *dev = connector->base.dev;
> >  	enum drm_connector_status old_status;
> > @@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> >  		drm_helper_probe_detect(&connector->base, NULL, false);
> >  
> >  	if (old_status == connector->base.status)
> > -		return false;
> > +		return INTEL_HOTPLUG_NOCHANGE;
> >  
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
> >  		      connector->base.base.id,
> > @@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> >  		      drm_get_connector_status_name(old_status),
> >  		      drm_get_connector_status_name(connector->base.status));
> >  
> > -	return true;
> > +	return INTEL_HOTPLUG_CHANGED;
> >  }
> >  
> >  static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
> > @@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
> >  		spin_lock_irq(&dev_priv->irq_lock);
> >  		dev_priv->hotplug.event_bits |= old_bits;
> >  		spin_unlock_irq(&dev_priv->irq_lock);
> > -		schedule_work(&dev_priv->hotplug.hotplug_work);
> > +		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> >  	}
> >  }
> >  
> > @@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
> >  static void i915_hotplug_work_func(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > -		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
> > +		container_of(work, struct drm_i915_private,
> > +			     hotplug.hotplug_work.work);
> >  	struct drm_device *dev = &dev_priv->drm;
> >  	struct intel_connector *intel_connector;
> >  	struct intel_encoder *intel_encoder;
> >  	struct drm_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	bool changed = false;
> > +	u32 retry = 0;
> >  	u32 hpd_event_bits;
> > +	u32 hpd_retry_bits;
> >  
> >  	mutex_lock(&dev->mode_config.mutex);
> >  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
> > @@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >  
> >  	hpd_event_bits = dev_priv->hotplug.event_bits;
> >  	dev_priv->hotplug.event_bits = 0;
> > +	hpd_retry_bits = dev_priv->hotplug.retry_bits;
> > +	dev_priv->hotplug.retry_bits = 0;
> >  
> >  	/* Enable polling for connectors which had HPD IRQ storms */
> >  	intel_hpd_irq_storm_switch_to_polling(dev_priv);
> > @@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >  
> >  	drm_connector_list_iter_begin(dev, &conn_iter);
> >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > +		u32 hpd_bit;
> > +
> >  		intel_connector = to_intel_connector(connector);
> >  		if (!intel_connector->encoder)
> >  			continue;
> >  		intel_encoder = intel_connector->encoder;
> > -		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> > +		hpd_bit = BIT(intel_encoder->hpd_pin);
> > +		if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
> >  			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> >  				      connector->name, intel_encoder->hpd_pin);
> >  
> > -			changed |= intel_encoder->hotplug(intel_encoder,
> > -							  intel_connector);
> > +			switch (intel_encoder->hotplug(intel_encoder,
> > +						       intel_connector,
> > +						       hpd_event_bits & hpd_bit)) {
> > +			case INTEL_HOTPLUG_NOCHANGE:
> > +				break;
> > +			case INTEL_HOTPLUG_CHANGED:
> > +				changed = true;
> > +				break;
> > +			case INTEL_HOTPLUG_RETRY:
> > +				retry |= hpd_bit;
> > +				break;
> > +			}
> >  		}
> >  	}
> >  	drm_connector_list_iter_end(&conn_iter);
> > @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >  
> >  	if (changed)
> >  		drm_kms_helper_hotplug_event(dev);
> > +
> > +	if (retry) {
> > +		spin_lock_irq(&dev_priv->irq_lock);
> > +		dev_priv->hotplug.retry_bits |= retry;
> > +		spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > +		mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
> > +				 msecs_to_jiffies(HPD_RETRY_DELAY));
> > +	}
> >  }
> >  
> >  
> > @@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> >  	if (queue_dig)
> >  		queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
> >  	if (queue_hp)
> > -		schedule_work(&dev_priv->hotplug.hotplug_work);
> > +		schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> >  }
> >  
> >  /**
> > @@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
> >  
> >  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> >  {
> > -	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> > +	INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> > +			  i915_hotplug_work_func);
> >  	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
> >  	INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
> >  	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
> > @@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> >  	dev_priv->hotplug.long_port_mask = 0;
> >  	dev_priv->hotplug.short_port_mask = 0;
> >  	dev_priv->hotplug.event_bits = 0;
> > +	dev_priv->hotplug.retry_bits = 0;
> >  
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  
> >  	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> > -	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> > +	cancel_delayed_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);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index e7b0884ba5a5..92793475b2b9 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
> >  			     &intel_sdvo->hotplug_active, 2);
> >  }
> >  
> > -static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
> > -			       struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_sdvo_hotplug(struct intel_encoder *encoder,
> > +		   struct intel_connector *connector,
> > +		   bool irq_received)
> >  {
> >  	intel_sdvo_enable_hotplug(encoder);
> >  
> > -	return intel_encoder_hotplug(encoder, connector);
> > +	return intel_encoder_hotplug(encoder, connector, irq_received);
> >  }
> >  
> >  static bool
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[Imre Deak]

On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza wrote:
> Unpowered type-c dongles can take some time to boot and be
> responsible, causing the probe to fail and sink never be detected
> without further actions from userspace.
> 
> It was not a issue 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 bring this issue to users.
> 
> So here if the probe failed when coming from a IRQ it returns
> INTEL_HOTPLUG_RETRY that will schedule another run of
> i915_hotplug_work_func() after 1 second what is time enough for
> those type-c dongles to boot.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1676a87f18cb..96bbcf5c9787 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>  		  struct intel_connector *connector,
>  		  bool irq_received)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>  	struct drm_modeset_acquire_ctx ctx;
>  	enum intel_hotplug_state state;
>  	int ret;
> @@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>  	drm_modeset_acquire_fini(&ctx);
>  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> +	/*
> +	 * Unpowered type-c dongles can take some time to boot and be
> +	 * responsible, so here giving some type to those dongles to power up
> +	 * and then retrying the probe.
> +	 */
> +	if (state == INTEL_HOTPLUG_NOCHANGE &&
> +	    connector->base.status != connector_status_connected &&
> +	    irq_received && intel_port_is_tc(dev_priv, encoder->port) &&
> +	    !dig_port->tc_legacy_port && !dig_port->dp.is_mst)

Based on the investigation by Jani et al, we have the similar problem with
HDMI, only during disconnect. So I think we could generalize by retrying
any time there is no change (except for MST where the driver always keeps
the connector in a disconnected state), regardless of the type of the
sink, since a no-change is suspect in any case: why would the sink signal
(with a long pulse) if there is no change?

For HDMI we'd also need to handle this in intel_hdmi.c.

Then Ville suggested to add a Chamelium test for this to IGT, could you
Jose look into that as well? Both the connect and disconnect races could
be tested, both on HDMI and DP, by generating the HPD early/late wrt. to
AUX/DDC starting/stopping to return valid data. I don't know if
Chamelium can do this, you'd have to find that out first.

--Imre

> +		state = INTEL_HOTPLUG_RETRY;
> +
>  	return state;
>  }
>  
> -- 
> 2.20.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[Jani Nikula]

On Tue, 26 Feb 2019, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza wrote:
>> Unpowered type-c dongles can take some time to boot and be
>> responsible, causing the probe to fail and sink never be detected
>> without further actions from userspace.
>> 
>> It was not a issue 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 bring this issue to users.
>> 
>> So here if the probe failed when coming from a IRQ it returns
>> INTEL_HOTPLUG_RETRY that will schedule another run of
>> i915_hotplug_work_func() after 1 second what is time enough for
>> those type-c dongles to boot.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 1676a87f18cb..96bbcf5c9787 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>>  		  struct intel_connector *connector,
>>  		  bool irq_received)
>>  {
>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>>  	struct drm_modeset_acquire_ctx ctx;
>>  	enum intel_hotplug_state state;
>>  	int ret;
>> @@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>>  	drm_modeset_acquire_fini(&ctx);
>>  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>>  
>> +	/*
>> +	 * Unpowered type-c dongles can take some time to boot and be
>> +	 * responsible, so here giving some type to those dongles to power up
>> +	 * and then retrying the probe.
>> +	 */
>> +	if (state == INTEL_HOTPLUG_NOCHANGE &&
>> +	    connector->base.status != connector_status_connected &&
>> +	    irq_received && intel_port_is_tc(dev_priv, encoder->port) &&
>> +	    !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
>
> Based on the investigation by Jani et al, we have the similar problem with
> HDMI, only during disconnect. So I think we could generalize by retrying
> any time there is no change (except for MST where the driver always keeps
> the connector in a disconnected state), regardless of the type of the
> sink, since a no-change is suspect in any case: why would the sink signal
> (with a long pulse) if there is no change?
>
> For HDMI we'd also need to handle this in intel_hdmi.c.
>
> Then Ville suggested to add a Chamelium test for this to IGT, could you
> Jose look into that as well? Both the connect and disconnect races could
> be tested, both on HDMI and DP, by generating the HPD early/late wrt. to
> AUX/DDC starting/stopping to return valid data. I don't know if
> Chamelium can do this, you'd have to find that out first.

Guang Bai also kept referencing a pathological customer test case which
we're not privy to.

BR,
Jani.

>
> --Imre
>
>> +		state = INTEL_HOTPLUG_RETRY;
>> +
>>  	return state;
>>  }
>>  
>> -- 
>> 2.20.1
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 4359 bytes --]

On Tue, 2019-02-26 at 16:08 +0200, Imre Deak wrote:
> On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza
> wrote:
> > Unpowered type-c dongles can take some time to boot and be
> > responsible, causing the probe to fail and sink never be detected
> > without further actions from userspace.
> > 
> > It was not a issue 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 bring this issue to users.
> > 
> > So here if the probe failed when coming from a IRQ it returns
> > INTEL_HOTPLUG_RETRY that will schedule another run of
> > i915_hotplug_work_func() after 1 second what is time enough for
> > those type-c dongles to boot.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1676a87f18cb..96bbcf5c9787 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder
> > *encoder,
> >  		  struct intel_connector *connector,
> >  		  bool irq_received)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder-
> > >base);
> >  	struct drm_modeset_acquire_ctx ctx;
> >  	enum intel_hotplug_state state;
> >  	int ret;
> > @@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder
> > *encoder,
> >  	drm_modeset_acquire_fini(&ctx);
> >  	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >  
> > +	/*
> > +	 * Unpowered type-c dongles can take some time to boot and be
> > +	 * responsible, so here giving some type to those dongles to 

s/type/time
So I don't loose it when doing the next version.

> > power up
> > +	 * and then retrying the probe.
> > +	 */
> > +	if (state == INTEL_HOTPLUG_NOCHANGE &&
> > +	    connector->base.status != connector_status_connected &&
> > +	    irq_received && intel_port_is_tc(dev_priv, encoder->port)
> > &&
> > +	    !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
> 
> Based on the investigation by Jani et al, we have the similar problem
> with
> HDMI, only during disconnect. So I think we could generalize by
> retrying
> any time there is no change (except for MST where the driver always
> keeps
> the connector in a disconnected state), regardless of the type of the
> sink, since a no-change is suspect in any case: why would the sink
> signal
> (with a long pulse) if there is no change?

The only case that I can think of is the cases were sink do a short
pulse but we don't handle it in the short pulse handler and schedule a
long pulse handler but it should not cause any drawback retry everytime
there is no change in the state and irq_received is set.

What comment could we add about the HDMI here? Something like this
would be fine?

"HDMI sinks are reported as connected by hardware right after unpluging
it, so here also giving some time for hardware to process the unplug so
driver can read it and do the unplug sequence and notify userspace
about the absence of the HDMI sink"

> 
> For HDMI we'd also need to handle this in intel_hdmi.c.

It happens in older gens that don't have DDI? Otherwise just the update
above should take care of DP and HDMI DDI ports.

> 
> Then Ville suggested to add a Chamelium test for this to IGT, could
> you
> Jose look into that as well? Both the connect and disconnect races
> could
> be tested, both on HDMI and DP, by generating the HPD early/late wrt.
> to
> AUX/DDC starting/stopping to return valid data. I don't know if
> Chamelium can do this, you'd have to find that out first.

I will try put my hands in a Chamelium board otherwise I will play with
trybot to add this tests.

> 
> --Imre
> 
> > +		state = INTEL_HOTPLUG_RETRY;
> > +
> >  	return state;
> >  }
> >  
> > -- 
> > 2.20.1
> > 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[Imre Deak]

On Thu, Feb 28, 2019 at 02:32:38AM +0200, Souza, Jose wrote:
> [...]
> > > +	 * and then retrying the probe.
> > > +	 */
> > > +	if (state == INTEL_HOTPLUG_NOCHANGE &&
> > > +	    connector->base.status != connector_status_connected &&
> > > +	    irq_received && intel_port_is_tc(dev_priv, encoder->port)
> > > &&
> > > +	    !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
> > 
> > Based on the investigation by Jani et al, we have the similar problem
> > with
> > HDMI, only during disconnect. So I think we could generalize by
> > retrying
> > any time there is no change (except for MST where the driver always
> > keeps
> > the connector in a disconnected state), regardless of the type of the
> > sink, since a no-change is suspect in any case: why would the sink
> > signal
> > (with a long pulse) if there is no change?
> 
> The only case that I can think of is the cases were sink do a short
> pulse but we don't handle it in the short pulse handler and schedule a
> long pulse handler but it should not cause any drawback retry everytime
> there is no change in the state and irq_received is set.
> 
> What comment could we add about the HDMI here? Something like this
> would be fine?
> 
> "HDMI sinks are reported as connected by hardware right after unpluging
> it, so here also giving some time for hardware to process the unplug so
> driver can read it and do the unplug sequence and notify userspace
> about the absence of the HDMI sink"

It could be more specific, something like:

On many platforms the HDMI live state signal is known to be unreliable,
so we can't use it to detect if a sink is connected or not. Instead we
detect if it's connected based on whether we can read the EDID or not.
That in turn has a problem during disconnect, since the HPD interrupt
may be raised before the DDC lines get disconnected (due to how the
required length of DDC vs. HPD connector pins are specified) and so
we'll still be able to get a valid EDID. To solve this schedule another
detection cycle if this time around we didn't detect any change in the
sink's connection status.

> 
> > 
> > For HDMI we'd also need to handle this in intel_hdmi.c.
> 
> It happens in older gens that don't have DDI? Otherwise just the update
> above should take care of DP and HDMI DDI ports.

I've been told it happens both on old and new HW.

> > Then Ville suggested to add a Chamelium test for this to IGT, could
> > you
> > Jose look into that as well? Both the connect and disconnect races
> > could
> > be tested, both on HDMI and DP, by generating the HPD early/late wrt.
> > to
> > AUX/DDC starting/stopping to return valid data. I don't know if
> > Chamelium can do this, you'd have to find that out first.
> 
> I will try put my hands in a Chamelium board otherwise I will play with
> trybot to add this tests.

Okay, thanks.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 3313 bytes --]

On Thu, 2019-02-28 at 18:06 +0200, Imre Deak wrote:
> On Thu, Feb 28, 2019 at 02:32:38AM +0200, Souza, Jose wrote:
> > [...]
> > > > +	 * and then retrying the probe.
> > > > +	 */
> > > > +	if (state == INTEL_HOTPLUG_NOCHANGE &&
> > > > +	    connector->base.status !=
> > > > connector_status_connected &&
> > > > +	    irq_received && intel_port_is_tc(dev_priv, encoder-
> > > > >port)
> > > > &&
> > > > +	    !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
> > > 
> > > Based on the investigation by Jani et al, we have the similar
> > > problem
> > > with
> > > HDMI, only during disconnect. So I think we could generalize by
> > > retrying
> > > any time there is no change (except for MST where the driver
> > > always
> > > keeps
> > > the connector in a disconnected state), regardless of the type of
> > > the
> > > sink, since a no-change is suspect in any case: why would the
> > > sink
> > > signal
> > > (with a long pulse) if there is no change?
> > 
> > The only case that I can think of is the cases were sink do a short
> > pulse but we don't handle it in the short pulse handler and
> > schedule a
> > long pulse handler but it should not cause any drawback retry
> > everytime
> > there is no change in the state and irq_received is set.
> > 
> > What comment could we add about the HDMI here? Something like this
> > would be fine?
> > 
> > "HDMI sinks are reported as connected by hardware right after
> > unpluging
> > it, so here also giving some time for hardware to process the
> > unplug so
> > driver can read it and do the unplug sequence and notify userspace
> > about the absence of the HDMI sink"
> 
> It could be more specific, something like:
> 
> On many platforms the HDMI live state signal is known to be
> unreliable,
> so we can't use it to detect if a sink is connected or not. Instead
> we
> detect if it's connected based on whether we can read the EDID or
> not.
> That in turn has a problem during disconnect, since the HPD interrupt
> may be raised before the DDC lines get disconnected (due to how the
> required length of DDC vs. HPD connector pins are specified) and so
> we'll still be able to get a valid EDID. To solve this schedule
> another
> detection cycle if this time around we didn't detect any change in
> the
> sink's connection status.
> 
> > > For HDMI we'd also need to handle this in intel_hdmi.c.
> > 
> > It happens in older gens that don't have DDI? Otherwise just the
> > update
> > above should take care of DP and HDMI DDI ports.
> 
> I've been told it happens both on old and new HW.
> 
> > > Then Ville suggested to add a Chamelium test for this to IGT,
> > > could
> > > you
> > > Jose look into that as well? Both the connect and disconnect
> > > races
> > > could
> > > be tested, both on HDMI and DP, by generating the HPD early/late
> > > wrt.
> > > to
> > > AUX/DDC starting/stopping to return valid data. I don't know if
> > > Chamelium can do this, you'd have to find that out first.
> > 
> > I will try put my hands in a Chamelium board otherwise I will play
> > with
> > trybot to add this tests.
> 
> Okay, thanks.


Just sent the new kernel patches and the IGT test for the type-c bug,
still working in the HDMI test.

> 
> --Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed

[José Roberto de Souza]

Unpowered type-c dongles can take some time to boot and be
responsible, causing the probe to fail and sink never be detected
without further actions from userspace.

It was not a issue 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 bring this issue to users.

So here after the first probe interation over every connector with
a hotplug event set, it sleeps for half a second to give some time to
dongles to be ready and then try to probe again every type-c
connector that failed in the initial probe.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c      |  3 +++
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_hotplug.c | 39 +++++++++++++++++++++++-----
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cf709835fb9a..b91b9700755f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6968,6 +6968,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
 
+	if (intel_port_is_tc(dev_priv, port) && !intel_dig_port->tc_legacy_port)
+		intel_dig_port->tc_delay_wa_needed = true;
+
 	return true;
 
 fail:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fd50c962eaa3..3ae95679c2a9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1257,7 +1257,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
-	bool tc_legacy_port:1;
+	bool tc_legacy_port:1, tc_delay_wa_needed:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 72e0203bcbb2..eeac29f2dc70 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -343,7 +343,7 @@ static void i915_digport_work_func(struct work_struct *work)
 }
 
 static bool
-i915_hotplug_iterate(struct drm_device *dev, u32 hpd_event_bits)
+i915_hotplug_iterate(struct drm_device *dev, u32 hpd_event_bits, u32 *hpd_tc_delay_wa)
 {
 	struct drm_connector_list_iter conn_iter;
 	struct drm_connector *connector;
@@ -356,16 +356,27 @@ i915_hotplug_iterate(struct drm_device *dev, u32 hpd_event_bits)
 
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
-
 		if (!intel_encoder)
 			continue;
 
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			bool ret;
+
 			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
 				      connector->name, intel_encoder->hpd_pin);
 
-			changed |= intel_encoder->hotplug(intel_encoder,
-							  intel_connector);
+			ret = intel_encoder->hotplug(intel_encoder,
+						     intel_connector);
+			changed |= ret;
+
+			if (hpd_tc_delay_wa && !ret &&
+			    connector->status != connector_status_connected) {
+				struct intel_digital_port *dig_port = enc_to_dig_port(&intel_encoder->base);
+
+				if (dig_port && dig_port->tc_delay_wa_needed &&
+				    !dig_port->dp.is_mst)
+					*hpd_tc_delay_wa |= (1 << intel_encoder->hpd_pin);
+			}
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
@@ -382,7 +393,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
 	struct drm_device *dev = &dev_priv->drm;
 	bool changed;
-	u32 hpd_event_bits;
+	u32 hpd_event_bits, hpd_tc_delay_wa = 0;
 
 	mutex_lock(&dev->mode_config.mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
@@ -397,12 +408,28 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	changed = i915_hotplug_iterate(dev, hpd_event_bits);
+	changed = i915_hotplug_iterate(dev, hpd_event_bits, &hpd_tc_delay_wa);
 
 	mutex_unlock(&dev->mode_config.mutex);
 
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
+
+	/*
+	 * Unpowered type-c dongles can take some time to boot and be
+	 * responsible, so here giving some type to those dongles to power up
+	 * and then probing again.
+	 */
+	if (hpd_tc_delay_wa) {
+		msleep(500);
+
+		mutex_lock(&dev->mode_config.mutex);
+		changed = i915_hotplug_iterate(dev, hpd_tc_delay_wa, NULL);
+		mutex_unlock(&dev->mode_config.mutex);
+
+		if (changed)
+			drm_kms_helper_hotplug_event(dev);
+	}
 }
 
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx