[PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[José Roberto de Souza]

When suspending or unloading the driver, it needs to release the
TC ports so HW can change it state without wait for driver handshake.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  1 +
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/i915_irq.c      |  1 +
 drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
 6 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index acb516308262..14331c396278 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 	intel_hpd_cancel_work(dev_priv);
+	intel_hpd_suspend(dev_priv);
 
 	intel_suspend_encoders(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0c8438de3c1b..96d5ddc36f4e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2792,6 +2792,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_hpd_suspend(struct drm_i915_private *dev_priv);
 
 /* i915_irq.c */
 static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d7e47d6082de..23084d227e2a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
 {
 	drm_irq_uninstall(&dev_priv->drm);
 	intel_hpd_cancel_work(dev_priv);
+	intel_hpd_suspend(dev_priv);
 	dev_priv->runtime_pm.irqs_enabled = false;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5258c9d654f4..27c6163426d6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
 	return false;
 }
 
+/*
+ * intel_digital_port_force_disconnection - Used to force port disconnection or
+ * release any control from display driver before going to a state that driver
+ * will not handle interruptions.
+ */
+void intel_digital_port_force_disconnection(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port;
+
+	dig_port = enc_to_dig_port(&encoder->base);
+	/* Skip fake MST ports */
+	if (!dig_port)
+		return;
+
+	if (INTEL_GEN(dev_priv) >= 11)
+		icl_tc_phy_disconnect(dev_priv, dig_port);
+}
+
 static struct edid *
 intel_dp_get_edid(struct intel_dp *intel_dp)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6772e9974751..bc30f107b430 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void intel_digital_port_force_disconnection(struct intel_encoder *encoder);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 42e61e10f517..d6745b7b79d5 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -655,3 +655,12 @@ void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin)
 	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
+
+void intel_hpd_suspend(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = &dev_priv->drm;
+	struct intel_encoder *encoder;
+
+	for_each_intel_encoder(dev, encoder)
+		intel_digital_port_force_disconnection(encoder);
+}
-- 
2.19.1

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

[PATCH 2/3] drm/i915: Call intel_hpd_cancel_work() from intel_hpd_suspend()

[José Roberto de Souza]

intel_hpd_suspend() was added to have a more generic function to
handle all the sequences needed when hotplug detection will be
suspended, so lets call intel_hpd_cancel_work() from
intel_hpd_suspend() and make it static.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 1 -
 drivers/gpu/drm/i915/i915_drv.h      | 1 -
 drivers/gpu/drm/i915/i915_irq.c      | 1 -
 drivers/gpu/drm/i915/intel_hotplug.c | 4 +++-
 4 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 14331c396278..a6ba1a953ffc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1910,7 +1910,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 	intel_dp_mst_suspend(dev_priv);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
-	intel_hpd_cancel_work(dev_priv);
 	intel_hpd_suspend(dev_priv);
 
 	intel_suspend_encoders(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 96d5ddc36f4e..98547c242121 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2787,7 +2787,6 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 			   u32 pin_mask, u32 long_mask);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
 void intel_hpd_init_work(struct drm_i915_private *dev_priv);
-void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
 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);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 23084d227e2a..70addda1bd38 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4995,7 +4995,6 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
 void intel_irq_uninstall(struct drm_i915_private *dev_priv)
 {
 	drm_irq_uninstall(&dev_priv->drm);
-	intel_hpd_cancel_work(dev_priv);
 	intel_hpd_suspend(dev_priv);
 	dev_priv->runtime_pm.irqs_enabled = false;
 }
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index d6745b7b79d5..86b7457c6c8d 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -613,7 +613,7 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 			  intel_hpd_irq_storm_reenable_work);
 }
 
-void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
+static void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 {
 	spin_lock_irq(&dev_priv->irq_lock);
 
@@ -661,6 +661,8 @@ void intel_hpd_suspend(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_encoder *encoder;
 
+	intel_hpd_cancel_work(dev_priv);
+
 	for_each_intel_encoder(dev, encoder)
 		intel_digital_port_force_disconnection(encoder);
 }
-- 
2.19.1

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

[PATCH 3/3] drm/i915/icl: Delay hotplug processing for tc ports

[José Roberto de Souza]

Some 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 gens because there was a bridge
between DP port and USB-C controller adding some delay but ICL
handles type-C native.

So here trying to do a aux channel transaction at each 150ms for up 5
times, before giving up.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_dp.c      | 14 +++++
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++-
 drivers/gpu/drm/i915/intel_hotplug.c | 76 ++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 98547c242121..85950fb1fab1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2792,6 +2792,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
 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_hpd_suspend(struct drm_i915_private *dev_priv);
+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_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 27c6163426d6..e9060e325109 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5307,6 +5307,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
 	intel_dp_mst_encoder_cleanup(intel_dig_port);
 	if (intel_dp_is_edp(intel_dp)) {
@@ -5323,6 +5324,15 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 			unregister_reboot_notifier(&intel_dp->edp_notifier);
 			intel_dp->edp_notifier.notifier_call = NULL;
 		}
+	} else {
+		/*
+		 * ICL TC legacy HDMI ports is destroyed by this callback so
+		 * it is necessary check if is DP before cancel delayed works
+		 * as it is not initialized for legacy HDMI ports.
+		 */
+		if (IS_ICELAKE(dev_priv) &&
+		    intel_dig_port->base.type == INTEL_OUTPUT_DP)
+			cancel_delayed_work_sync(&intel_dp->tc_wa_work);
 	}
 
 	intel_dp_aux_fini(intel_dp);
@@ -6686,6 +6696,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
 
+	if (IS_ICELAKE(dev_priv) && !intel_dp_is_edp(intel_dp))
+		INIT_DELAYED_WORK(&intel_dp->tc_wa_work,
+				  intel_hotplug_tc_wa_work);
+
 	return true;
 
 fail:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bc30f107b430..61cfa4984c11 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1138,7 +1138,11 @@ struct intel_dp {
 	int panel_power_cycle_delay;
 	int backlight_on_delay;
 	int backlight_off_delay;
-	struct delayed_work panel_vdd_work;
+	union {
+		struct delayed_work panel_vdd_work;
+		struct delayed_work tc_wa_work;
+	};
+	u8 tc_wa_count;
 	bool want_panel_vdd;
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 86b7457c6c8d..cfd469bce52c 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -342,6 +342,64 @@ static void i915_digport_work_func(struct work_struct *work)
 	}
 }
 
+#define TC_WA_DELAY_MSEC 150
+#define TC_WA_TRIES 5
+
+/*
+ * Test if TC 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_dp *intel_dp)
+{
+	u8 buff;
+
+	intel_dp->tc_wa_count++;
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DPCD_REV, &buff, 1) != 1)
+		goto not_responsive;
+
+	if (!drm_probe_ddc(&intel_dp->aux.ddc))
+		goto not_responsive;
+
+	return true;
+
+not_responsive:
+	if (intel_dp->tc_wa_count < TC_WA_TRIES) {
+		unsigned long delay;
+
+		delay = msecs_to_jiffies(TC_WA_DELAY_MSEC);
+		schedule_delayed_work(&intel_dp->tc_wa_work, delay);
+	} else {
+		DRM_DEBUG_KMS("TC not responsive, giving up\n");
+	}
+
+	return false;
+}
+
+void intel_hotplug_tc_wa_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp, tc_wa_work);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct drm_device *dev = &dev_priv->drm;
+	bool ret;
+
+	if (!intel_digital_port_connected(intel_encoder))
+		return;
+
+	mutex_lock(&dev->mode_config.mutex);
+	ret = intel_hotplug_tc_wa_test(intel_dp);
+	if (ret)
+		ret = intel_encoder->hotplug(intel_encoder, intel_connector);
+	mutex_unlock(&dev->mode_config.mutex);
+
+	if (ret)
+		drm_kms_helper_hotplug_event(dev);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -377,9 +435,27 @@ 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);
 
+			/*
+			 * TC WA: TC dongles can takes some time to be
+			 * responsible, so let's try to do a DPCD read to check
+			 * if it is ready, otherwise try again in a few msecs.
+			 */
+			if (IS_ICELAKE(dev_priv) &&
+			    intel_digital_port_connected(intel_encoder) &&
+			    dig_port->tc_type != TC_PORT_LEGACY) {
+				struct intel_dp *intel_dp;
+
+				intel_dp = enc_to_intel_dp(&intel_encoder->base);
+				intel_dp->tc_wa_count = 0;
+				if (!intel_hotplug_tc_wa_test(intel_dp))
+					continue;
+			}
+
 			changed |= intel_encoder->hotplug(intel_encoder,
 							  intel_connector);
 		}
-- 
2.19.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/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver
URL   : https://patchwork.freedesktop.org/series/52195/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/icl: Release TC ports when unloading or suspending driver
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3708:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3709:16: warning: expression using sizeof(void)

Commit: drm/i915: Call intel_hpd_cancel_work() from intel_hpd_suspend()
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3709:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3708:16: warning: expression using sizeof(void)

Commit: drm/i915/icl: Delay hotplug processing for tc ports
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3708:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3709:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver
URL   : https://patchwork.freedesktop.org/series/52195/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5103 -> Patchwork_10761 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-byt-clapper:     PASS -> WARN (fdo#108688)

    igt@drv_selftest@live_execlists:
      fi-apl-guc:         PASS -> DMESG-WARN (fdo#108622)

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    igt@pm_rpm@module-reload:
      fi-byt-clapper:     PASS -> FAIL (fdo#108675)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-hsw-4770r:       DMESG-WARN (fdo#105602) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
    ==== Warnings ====

    igt@drv_selftest@live_contexts:
      fi-icl-u:           INCOMPLETE (fdo#108315) -> DMESG-FAIL (fdo#108569)

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
  fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569
  fdo#108622 https://bugs.freedesktop.org/show_bug.cgi?id=108622
  fdo#108675 https://bugs.freedesktop.org/show_bug.cgi?id=108675
  fdo#108688 https://bugs.freedesktop.org/show_bug.cgi?id=108688


== Participating hosts (54 -> 46) ==

  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-pnv-d510 


== Build changes ==

    * Linux: CI_DRM_5103 -> Patchwork_10761

  CI_DRM_5103: 23c1138030ad65402f698ab0b356e2f55722bc77 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4712: a3ede1b535ac8137f6949c468edd7054453d5dae @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10761: 095a2f133f3627819a2ab8d231db3b730da8f54a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

095a2f133f36 drm/i915/icl: Delay hotplug processing for tc ports
084d18c66f29 drm/i915: Call intel_hpd_cancel_work() from intel_hpd_suspend()
7fa3afcc75d4 drm/i915/icl: Release TC ports when unloading or suspending driver

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver
URL   : https://patchwork.freedesktop.org/series/52195/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5103_full -> Patchwork_10761_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries_display_off:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108)

    igt@gem_cpu_reloc@full:
      shard-skl:          NOTRUN -> INCOMPLETE (fdo#108073)

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_chv_cursor_fail@pipe-a-64x64-right-edge:
      shard-skl:          NOTRUN -> FAIL (fdo#104671)

    igt@kms_color@pipe-b-legacy-gamma:
      shard-apl:          PASS -> FAIL (fdo#104782)

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-apl:          PASS -> FAIL (fdo#103191, fdo#103232)

    igt@kms_cursor_crc@cursor-64x64-random:
      shard-glk:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-64x64-sliding:
      shard-skl:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_draw_crc@draw-method-rgb565-mmap-wc-untiled:
      shard-skl:          NOTRUN -> FAIL (fdo#103184)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-move:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-farfromfence:
      shard-skl:          NOTRUN -> FAIL (fdo#105682) +1

    igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary:
      shard-skl:          PASS -> FAIL (fdo#105682)

    igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-mmap-gtt:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-mmap-wc:
      shard-skl:          NOTRUN -> FAIL (fdo#103167) +1

    igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +2

    igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
      shard-skl:          PASS -> FAIL (fdo#107815, fdo#108145)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@pm_rpm@debugfs-read:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807) +1

    
    ==== Possible fixes ====

    igt@gem_exec_reuse@baggage:
      shard-apl:          DMESG-WARN (fdo#108690) -> PASS

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
      shard-kbl:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_color@pipe-a-degamma:
      shard-apl:          FAIL (fdo#108145, fdo#104782) -> PASS

    igt@kms_color@pipe-b-ctm-max:
      shard-apl:          FAIL (fdo#108147) -> PASS

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +1

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
      shard-glk:          FAIL (fdo#103167) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-glk:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
      shard-apl:          FAIL (fdo#103166) -> PASS +2

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108073 https://bugs.freedesktop.org/show_bug.cgi?id=108073
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
  fdo#108690 https://bugs.freedesktop.org/show_bug.cgi?id=108690
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5103 -> Patchwork_10761

  CI_DRM_5103: 23c1138030ad65402f698ab0b356e2f55722bc77 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4712: a3ede1b535ac8137f6949c468edd7054453d5dae @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10761: 095a2f133f3627819a2ab8d231db3b730da8f54a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver
URL   : https://patchwork.freedesktop.org/series/52195/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5152 -> Patchwork_10842 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#107724)

    igt@gem_exec_suspend@basic-s3:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#107732, fdo#108070)

    igt@i915_selftest@live_contexts:
      fi-bsw-kefka:       PASS -> DMESG-FAIL (fdo#108656)

    igt@i915_selftest@live_hangcheck:
      fi-icl-u:           PASS -> INCOMPLETE (fdo#108315)

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    igt@prime_vgem@basic-fence-flip:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#103182)

    
    ==== Possible fixes ====

    igt@i915_module_load@reload:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-WARN (fdo#102614) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103182 https://bugs.freedesktop.org/show_bug.cgi?id=103182
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#107732 https://bugs.freedesktop.org/show_bug.cgi?id=107732
  fdo#108070 https://bugs.freedesktop.org/show_bug.cgi?id=108070
  fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
  fdo#108656 https://bugs.freedesktop.org/show_bug.cgi?id=108656


== Participating hosts (51 -> 47) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_5152 -> Patchwork_10842

  CI_DRM_5152: 458538284af8fa513c9d1404a8651931d6c79ddf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4718: 8ac5cfb4db9c7bc593beec18a6be1e2ff163106c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10842: 717b90b72bc3f8c6e369bdfcf74399204481a380 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

717b90b72bc3 drm/i915/icl: Delay hotplug processing for tc ports
dcd4650c2e3b drm/i915: Call intel_hpd_cancel_work() from intel_hpd_suspend()
b59497b57993 drm/i915/icl: Release TC ports when unloading or suspending driver

== Logs ==

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

Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Imre Deak]

On Wed, Nov 07, 2018 at 04:05:52PM -0800, José Roberto de Souza wrote:
> When suspending or unloading the driver, it needs to release the
> TC ports so HW can change it state without wait for driver handshake.

According to 
https://bugs.freedesktop.org/show_bug.cgi?id=108070#c26

this patch should fix the bug reported at
https://bugs.freedesktop.org/show_bug.cgi?id=108070#c17

but, I can't see how the change here would fix the corresponding problem
described in
https://bugs.freedesktop.org/show_bug.cgi?id=108070#c18

Would you explain?

I think there are more fundamental problems in TypeC HPD handling as we
discussed earlier here on the list, which should be fixed first:

- Switching to/from type C mode from the interrupt handler without
  considering if we have an active mode or an ongoing AUX transfer.

- Not having any way for handling the case where we'd do a modeset after
  an HPD disconnect interrupt, like in the case of
  common-hpd-after-suspend in the bug report.

More comments below:

> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/i915_irq.c      |  1 +
>  drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
>  6 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index acb516308262..14331c396278 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  	intel_hpd_cancel_work(dev_priv);
> +	intel_hpd_suspend(dev_priv);

What about runtime suspend? We won't receive HPD interrupts after that
either, so we'd need to disconnect in that case too according to the
spec.

And how are we handling resume then where we'll be in a disconnected
state due to the change in this patch and would continue to do a
modeset (to restore the mode we had before suspend).

>  
>  	intel_suspend_encoders(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0c8438de3c1b..96d5ddc36f4e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2792,6 +2792,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_hpd_suspend(struct drm_i915_private *dev_priv);
>  
>  /* i915_irq.c */
>  static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d7e47d6082de..23084d227e2a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
>  {
>  	drm_irq_uninstall(&dev_priv->drm);
>  	intel_hpd_cancel_work(dev_priv);
> +	intel_hpd_suspend(dev_priv);
>  	dev_priv->runtime_pm.irqs_enabled = false;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5258c9d654f4..27c6163426d6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	return false;
>  }
>  
> +/*
> + * intel_digital_port_force_disconnection - Used to force port disconnection or
> + * release any control from display driver before going to a state that driver
> + * will not handle interruptions.
> + */
> +void intel_digital_port_force_disconnection(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port;
> +
> +	dig_port = enc_to_dig_port(&encoder->base);
> +	/* Skip fake MST ports */
> +	if (!dig_port)
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		icl_tc_phy_disconnect(dev_priv, dig_port);
> +}
> +
>  static struct edid *
>  intel_dp_get_edid(struct intel_dp *intel_dp)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6772e9974751..bc30f107b430 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct intel_encoder *encoder);
> +void intel_digital_port_force_disconnection(struct intel_encoder *encoder);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index 42e61e10f517..d6745b7b79d5 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -655,3 +655,12 @@ void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin)
>  	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> +
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	struct intel_encoder *encoder;
> +
> +	for_each_intel_encoder(dev, encoder)
> +		intel_digital_port_force_disconnection(encoder);
> +}
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Souza, Jose]

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

On Wed, 2018-11-28 at 13:34 +0200, Imre Deak wrote:
> On Wed, Nov 07, 2018 at 04:05:52PM -0800, José Roberto de Souza
> wrote:
> > When suspending or unloading the driver, it needs to release the
> > TC ports so HW can change it state without wait for driver
> > handshake.
> 
> According to 
> https://bugs.freedesktop.org/show_bug.cgi?id=108070#c26
> 
> this patch should fix the bug reported at
> https://bugs.freedesktop.org/show_bug.cgi?id=108070#c17
> 
> but, I can't see how the change here would fix the corresponding
> problem
> described in
> https://bugs.freedesktop.org/show_bug.cgi?id=108070#c18
> 
> Would you explain?
> 
> I think there are more fundamental problems in TypeC HPD handling as
> we
> discussed earlier here on the list, which should be fixed first:
> 
> - Switching to/from type C mode from the interrupt handler without
>   considering if we have an active mode or an ongoing AUX transfer.

Yes that is still missing, I plan to work on that after I deliver a few
tasks. Also I read the emails in discussion that you started about
that.

> 
> - Not having any way for handling the case where we'd do a modeset
> after
>   an HPD disconnect interrupt, like in the case of
>   common-hpd-after-suspend in the bug report.

For what I understood about this test it will suspend, disconnect
chamelium board from source, wakeup and check if connector state match,
as we did not marked port as safe before suspending we are left in a
invalid state where PD firmware do not trigger a interruption after
wakeup.

> 
> More comments below:
> 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c      |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/i915_irq.c      |  1 +
> >  drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
> >  6 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index acb516308262..14331c396278 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct drm_device
> > *dev)
> >  
> >  	intel_runtime_pm_disable_interrupts(dev_priv);
> >  	intel_hpd_cancel_work(dev_priv);
> > +	intel_hpd_suspend(dev_priv);
> 
> What about runtime suspend? We won't receive HPD interrupts after
> that
> either, so we'd need to disconnect in that case too according to the
> spec.

Oh my bad I was thinking that the regular suspend would handle this too
but I was wrong.

So I will add a intel_hpd_suspend() call to intel_runtime_suspend()
leaving the i915_drv.c changes as this.

diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index b1d23c73c147..948914a79c67 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1905,6 +1905,7 @@ static int i915_drm_suspend(struct drm_device
*dev)

        intel_runtime_pm_disable_interrupts(dev_priv);
        intel_hpd_cancel_work(dev_priv);
+       intel_hpd_suspend(dev_priv);

        intel_suspend_encoders(dev_priv);

@@ -2909,6 +2910,7 @@ static int intel_runtime_suspend(struct device
*kdev)
        intel_uc_suspend(dev_priv);

        intel_runtime_pm_disable_interrupts(dev_priv);
+       intel_hpd_suspend(dev_priv);

        intel_uncore_suspend(dev_priv);

Also I will drop the next patch as we don't call
intel_hpd_cancel_work() in intel_runtime_suspend() path.


> 
> And how are we handling resume then where we'll be in a disconnected
> state due to the change in this patch and would continue to do a
> modeset (to restore the mode we had before suspend).

The drm_kms_helper_poll_enable() calls will trigger the call to the
detection function of the connectors that will call
intel_digital_port_connected() thal will call ICL TC functions that
will take care of update everything.

> 
> >  
> >  	intel_suspend_encoders(dev_priv);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 0c8438de3c1b..96d5ddc36f4e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2792,6 +2792,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_hpd_suspend(struct drm_i915_private *dev_priv);
> >  
> >  /* i915_irq.c */
> >  static inline void i915_queue_hangcheck(struct drm_i915_private
> > *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index d7e47d6082de..23084d227e2a 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct
> > drm_i915_private *dev_priv)
> >  {
> >  	drm_irq_uninstall(&dev_priv->drm);
> >  	intel_hpd_cancel_work(dev_priv);
> > +	intel_hpd_suspend(dev_priv);
> >  	dev_priv->runtime_pm.irqs_enabled = false;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 5258c9d654f4..27c6163426d6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct
> > intel_encoder *encoder)
> >  	return false;
> >  }
> >  
> > +/*
> > + * intel_digital_port_force_disconnection - Used to force port
> > disconnection or
> > + * release any control from display driver before going to a state
> > that driver
> > + * will not handle interruptions.
> > + */
> > +void intel_digital_port_force_disconnection(struct intel_encoder
> > *encoder)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_digital_port *dig_port;
> > +
> > +	dig_port = enc_to_dig_port(&encoder->base);
> > +	/* Skip fake MST ports */
> > +	if (!dig_port)
> > +		return;
> > +
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		icl_tc_phy_disconnect(dev_priv, dig_port);
> > +}
> > +
> >  static struct edid *
> >  intel_dp_get_edid(struct intel_dp *intel_dp)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 6772e9974751..bc30f107b430 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp
> > *intel_dp);
> >  int intel_dp_link_required(int pixel_clock, int bpp);
> >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > +void intel_digital_port_force_disconnection(struct intel_encoder
> > *encoder);
> >  
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector
> > *intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> > b/drivers/gpu/drm/i915/intel_hotplug.c
> > index 42e61e10f517..d6745b7b79d5 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -655,3 +655,12 @@ void intel_hpd_enable(struct drm_i915_private
> > *dev_priv, enum hpd_pin pin)
> >  	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> > +
> > +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = &dev_priv->drm;
> > +	struct intel_encoder *encoder;
> > +
> > +	for_each_intel_encoder(dev, encoder)
> > +		intel_digital_port_force_disconnection(encoder);
> > +}
> > -- 
> > 2.19.1
> > 

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

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

_______________________________________________
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/3] drm/i915/icl: Release TC ports when unloading or suspending driver (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver (rev2)
URL   : https://patchwork.freedesktop.org/series/52195/
State : failure

== Summary ==

Applying: drm/i915/icl: Release TC ports when unloading or suspending driver
error: corrupt patch at line 8
error: could not build fake ancestor
Patch failed at 0001 drm/i915/icl: Release TC ports when unloading or suspending driver
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Imre Deak]

On Wed, Nov 28, 2018 at 11:54:21PM +0200, Souza, Jose wrote:
> On Wed, 2018-11-28 at 13:34 +0200, Imre Deak wrote:
> > On Wed, Nov 07, 2018 at 04:05:52PM -0800, José Roberto de Souza
> > wrote:
> > > When suspending or unloading the driver, it needs to release the
> > > TC ports so HW can change it state without wait for driver
> > > handshake.
> > 
> > According to 
> > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c26
> > 
> > this patch should fix the bug reported at
> > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c17
> > 
> > but, I can't see how the change here would fix the corresponding
> > problem
> > described in
> > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c18
> > 
> > Would you explain?
> > 
> > I think there are more fundamental problems in TypeC HPD handling as
> > we
> > discussed earlier here on the list, which should be fixed first:
> > 
> > - Switching to/from type C mode from the interrupt handler without
> >   considering if we have an active mode or an ongoing AUX transfer.
> 
> Yes that is still missing, I plan to work on that after I deliver a few
> tasks. Also I read the emails in discussion that you started about
> that.

The point is that imo those fundamental problems should be fixed first.

> 
> > 
> > - Not having any way for handling the case where we'd do a modeset
> > after
> >   an HPD disconnect interrupt, like in the case of
> >   common-hpd-after-suspend in the bug report.
> 
> For what I understood about this test it will suspend, disconnect
> chamelium board from source, wakeup and check if connector state match,
> as we did not marked port as safe before suspending we are left in a
> invalid state where PD firmware do not trigger a interruption after
> wakeup.

No, it's not about the lack of HPD interrupts. What happens is that the
sink disappears during system suspend/resume, but we still have an
active mode enabled on the corresponding port. We try to restore this
mode during resume but that modeset will fail.

> 
> > 
> > More comments below:
> > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c      |  1 +
> > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/i915_irq.c      |  1 +
> > >  drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
> > >  6 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index acb516308262..14331c396278 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct drm_device
> > > *dev)
> > >  
> > >  	intel_runtime_pm_disable_interrupts(dev_priv);
> > >  	intel_hpd_cancel_work(dev_priv);
> > > +	intel_hpd_suspend(dev_priv);
> > 
> > What about runtime suspend? We won't receive HPD interrupts after
> > that
> > either, so we'd need to disconnect in that case too according to the
> > spec.
> 
> Oh my bad I was thinking that the regular suspend would handle this too
> but I was wrong.
> 
> So I will add a intel_hpd_suspend() call to intel_runtime_suspend()
> leaving the i915_drv.c changes as this.
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index b1d23c73c147..948914a79c67 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1905,6 +1905,7 @@ static int i915_drm_suspend(struct drm_device
> *dev)
> 
>         intel_runtime_pm_disable_interrupts(dev_priv);
>         intel_hpd_cancel_work(dev_priv);
> +       intel_hpd_suspend(dev_priv);
> 
>         intel_suspend_encoders(dev_priv);
> 
> @@ -2909,6 +2910,7 @@ static int intel_runtime_suspend(struct device
> *kdev)
>         intel_uc_suspend(dev_priv);
> 
>         intel_runtime_pm_disable_interrupts(dev_priv);
> +       intel_hpd_suspend(dev_priv);
> 
>         intel_uncore_suspend(dev_priv);
> 
> Also I will drop the next patch as we don't call
> intel_hpd_cancel_work() in intel_runtime_suspend() path.
> 
> 
> > 
> > And how are we handling resume then where we'll be in a disconnected
> > state due to the change in this patch and would continue to do a
> > modeset (to restore the mode we had before suspend).
> 
> The drm_kms_helper_poll_enable() calls will trigger the call to the
> detection function of the connectors that will call
> intel_digital_port_connected() thal will call ICL TC functions that
> will take care of update everything.

No detection will be triggered, since we're not polling this port (and
polling can be disabled as a whole). In any case that would be too late,
since we're trying to restore the mode before we call
drm_kms_helper_poll_enable().  Also doing a detection is not enough
here, since if detection fails (as it would in our case) we still have a
mode that we need to restore. The corresponding modeset would fail, but
we don't have a way to fail it gracefully as I wrote above.

> 
> > 
> > >  
> > >  	intel_suspend_encoders(dev_priv);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0c8438de3c1b..96d5ddc36f4e 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2792,6 +2792,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_hpd_suspend(struct drm_i915_private *dev_priv);
> > >  
> > >  /* i915_irq.c */
> > >  static inline void i915_queue_hangcheck(struct drm_i915_private
> > > *dev_priv)
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index d7e47d6082de..23084d227e2a 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct
> > > drm_i915_private *dev_priv)
> > >  {
> > >  	drm_irq_uninstall(&dev_priv->drm);
> > >  	intel_hpd_cancel_work(dev_priv);
> > > +	intel_hpd_suspend(dev_priv);
> > >  	dev_priv->runtime_pm.irqs_enabled = false;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 5258c9d654f4..27c6163426d6 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct
> > > intel_encoder *encoder)
> > >  	return false;
> > >  }
> > >  
> > > +/*
> > > + * intel_digital_port_force_disconnection - Used to force port
> > > disconnection or
> > > + * release any control from display driver before going to a state
> > > that driver
> > > + * will not handle interruptions.
> > > + */
> > > +void intel_digital_port_force_disconnection(struct intel_encoder
> > > *encoder)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +	struct intel_digital_port *dig_port;
> > > +
> > > +	dig_port = enc_to_dig_port(&encoder->base);
> > > +	/* Skip fake MST ports */
> > > +	if (!dig_port)
> > > +		return;
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > +		icl_tc_phy_disconnect(dev_priv, dig_port);
> > > +}
> > > +
> > >  static struct edid *
> > >  intel_dp_get_edid(struct intel_dp *intel_dp)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 6772e9974751..bc30f107b430 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp
> > > *intel_dp);
> > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > +void intel_digital_port_force_disconnection(struct intel_encoder
> > > *encoder);
> > >  
> > >  /* intel_dp_aux_backlight.c */
> > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector
> > > *intel_connector);
> > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> > > b/drivers/gpu/drm/i915/intel_hotplug.c
> > > index 42e61e10f517..d6745b7b79d5 100644
> > > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > > @@ -655,3 +655,12 @@ void intel_hpd_enable(struct drm_i915_private
> > > *dev_priv, enum hpd_pin pin)
> > >  	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
> > >  	spin_unlock_irq(&dev_priv->irq_lock);
> > >  }
> > > +
> > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct drm_device *dev = &dev_priv->drm;
> > > +	struct intel_encoder *encoder;
> > > +
> > > +	for_each_intel_encoder(dev, encoder)
> > > +		intel_digital_port_force_disconnection(encoder);
> > > +}
> > > -- 
> > > 2.19.1
> > > 


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

Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Souza, Jose]

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

On Thu, 2018-11-29 at 15:52 +0200, Imre Deak wrote:
> On Wed, Nov 28, 2018 at 11:54:21PM +0200, Souza, Jose wrote:
> > On Wed, 2018-11-28 at 13:34 +0200, Imre Deak wrote:
> > > On Wed, Nov 07, 2018 at 04:05:52PM -0800, José Roberto de Souza
> > > wrote:
> > > > When suspending or unloading the driver, it needs to release
> > > > the
> > > > TC ports so HW can change it state without wait for driver
> > > > handshake.
> > > 
> > > According to 
> > > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c26
> > > 
> > > this patch should fix the bug reported at
> > > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c17
> > > 
> > > but, I can't see how the change here would fix the corresponding
> > > problem
> > > described in
> > > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c18
> > > 
> > > Would you explain?
> > > 
> > > I think there are more fundamental problems in TypeC HPD handling
> > > as
> > > we
> > > discussed earlier here on the list, which should be fixed first:
> > > 
> > > - Switching to/from type C mode from the interrupt handler
> > > without
> > >   considering if we have an active mode or an ongoing AUX
> > > transfer.
> > 
> > Yes that is still missing, I plan to work on that after I deliver a
> > few
> > tasks. Also I read the emails in discussion that you started about
> > that.
> 
> The point is that imo those fundamental problems should be fixed
> first.
> 
> > > - Not having any way for handling the case where we'd do a
> > > modeset
> > > after
> > >   an HPD disconnect interrupt, like in the case of
> > >   common-hpd-after-suspend in the bug report.
> > 
> > For what I understood about this test it will suspend, disconnect
> > chamelium board from source, wakeup and check if connector state
> > match,
> > as we did not marked port as safe before suspending we are left in
> > a
> > invalid state where PD firmware do not trigger a interruption after
> > wakeup.
> 
> No, it's not about the lack of HPD interrupts. What happens is that
> the
> sink disappears during system suspend/resume, but we still have an
> active mode enabled on the corresponding port. We try to restore this
> mode during resume but that modeset will fail.

Before suspend, all display output is disabled so there is no active
modeset.

> 
> > > More comments below:
> > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c      |  1 +
> > > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > > >  drivers/gpu/drm/i915/i915_irq.c      |  1 +
> > > >  drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > > >  drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
> > > >  6 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index acb516308262..14331c396278 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct
> > > > drm_device
> > > > *dev)
> > > >  
> > > >  	intel_runtime_pm_disable_interrupts(dev_priv);
> > > >  	intel_hpd_cancel_work(dev_priv);
> > > > +	intel_hpd_suspend(dev_priv);
> > > 
> > > What about runtime suspend? We won't receive HPD interrupts after
> > > that
> > > either, so we'd need to disconnect in that case too according to
> > > the
> > > spec.
> > 
> > Oh my bad I was thinking that the regular suspend would handle this
> > too
> > but I was wrong.
> > 
> > So I will add a intel_hpd_suspend() call to intel_runtime_suspend()
> > leaving the i915_drv.c changes as this.
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index b1d23c73c147..948914a79c67 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1905,6 +1905,7 @@ static int i915_drm_suspend(struct drm_device
> > *dev)
> > 
> >         intel_runtime_pm_disable_interrupts(dev_priv);
> >         intel_hpd_cancel_work(dev_priv);
> > +       intel_hpd_suspend(dev_priv);
> > 
> >         intel_suspend_encoders(dev_priv);
> > 
> > @@ -2909,6 +2910,7 @@ static int intel_runtime_suspend(struct
> > device
> > *kdev)
> >         intel_uc_suspend(dev_priv);
> > 
> >         intel_runtime_pm_disable_interrupts(dev_priv);
> > +       intel_hpd_suspend(dev_priv);
> > 
> >         intel_uncore_suspend(dev_priv);
> > 
> > Also I will drop the next patch as we don't call
> > intel_hpd_cancel_work() in intel_runtime_suspend() path.
> > 
> > 
> > > And how are we handling resume then where we'll be in a
> > > disconnected
> > > state due to the change in this patch and would continue to do a
> > > modeset (to restore the mode we had before suspend).
> > 
> > The drm_kms_helper_poll_enable() calls will trigger the call to the
> > detection function of the connectors that will call
> > intel_digital_port_connected() thal will call ICL TC functions that
> > will take care of update everything.
> 
> No detection will be triggered, since we're not polling this port
> (and
> polling can be disabled as a whole). In any case that would be too
> late,
> since we're trying to restore the mode before we call
> drm_kms_helper_poll_enable().  Also doing a detection is not enough
> here, since if detection fails (as it would in our case) we still
> have a
> mode that we need to restore. The corresponding modeset would fail,
> but
> we don't have a way to fail it gracefully as I wrote above.


For the regular suspend it will run the connector detection functions
for sure when trying to restore the previous modeset.
About the runtime resume I need to check if with the polling disabled
it will still update the connector state.


> 
> > > >  
> > > >  	intel_suspend_encoders(dev_priv);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0c8438de3c1b..96d5ddc36f4e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2792,6 +2792,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_hpd_suspend(struct drm_i915_private *dev_priv);
> > > >  
> > > >  /* i915_irq.c */
> > > >  static inline void i915_queue_hangcheck(struct
> > > > drm_i915_private
> > > > *dev_priv)
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > index d7e47d6082de..23084d227e2a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct
> > > > drm_i915_private *dev_priv)
> > > >  {
> > > >  	drm_irq_uninstall(&dev_priv->drm);
> > > >  	intel_hpd_cancel_work(dev_priv);
> > > > +	intel_hpd_suspend(dev_priv);
> > > >  	dev_priv->runtime_pm.irqs_enabled = false;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 5258c9d654f4..27c6163426d6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct
> > > > intel_encoder *encoder)
> > > >  	return false;
> > > >  }
> > > >  
> > > > +/*
> > > > + * intel_digital_port_force_disconnection - Used to force port
> > > > disconnection or
> > > > + * release any control from display driver before going to a
> > > > state
> > > > that driver
> > > > + * will not handle interruptions.
> > > > + */
> > > > +void intel_digital_port_force_disconnection(struct
> > > > intel_encoder
> > > > *encoder)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > >base.dev);
> > > > +	struct intel_digital_port *dig_port;
> > > > +
> > > > +	dig_port = enc_to_dig_port(&encoder->base);
> > > > +	/* Skip fake MST ports */
> > > > +	if (!dig_port)
> > > > +		return;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > +		icl_tc_phy_disconnect(dev_priv, dig_port);
> > > > +}
> > > > +
> > > >  static struct edid *
> > > >  intel_dp_get_edid(struct intel_dp *intel_dp)
> > > >  {
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 6772e9974751..bc30f107b430 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp
> > > > *intel_dp);
> > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > >  bool intel_digital_port_connected(struct intel_encoder
> > > > *encoder);
> > > > +void intel_digital_port_force_disconnection(struct
> > > > intel_encoder
> > > > *encoder);
> > > >  
> > > >  /* intel_dp_aux_backlight.c */
> > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector
> > > > *intel_connector);
> > > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> > > > b/drivers/gpu/drm/i915/intel_hotplug.c
> > > > index 42e61e10f517..d6745b7b79d5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > > > @@ -655,3 +655,12 @@ void intel_hpd_enable(struct
> > > > drm_i915_private
> > > > *dev_priv, enum hpd_pin pin)
> > > >  	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
> > > >  	spin_unlock_irq(&dev_priv->irq_lock);
> > > >  }
> > > > +
> > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +	struct drm_device *dev = &dev_priv->drm;
> > > > +	struct intel_encoder *encoder;
> > > > +
> > > > +	for_each_intel_encoder(dev, encoder)
> > > > +		intel_digital_port_force_disconnection(encoder)
> > > > ;
> > > > +}
> > > > -- 
> > > > 2.19.1
> > > > 
> 
> 

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

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

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

Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Imre Deak]

On Thu, Nov 29, 2018 at 10:18:40PM +0200, Souza, Jose wrote:
> On Thu, 2018-11-29 at 15:52 +0200, Imre Deak wrote:
> > On Wed, Nov 28, 2018 at 11:54:21PM +0200, Souza, Jose wrote:
> > > On Wed, 2018-11-28 at 13:34 +0200, Imre Deak wrote:
> > > > On Wed, Nov 07, 2018 at 04:05:52PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > When suspending or unloading the driver, it needs to release
> > > > > the
> > > > > TC ports so HW can change it state without wait for driver
> > > > > handshake.
> > > > 
> > > > According to 
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c26
> > > > 
> > > > this patch should fix the bug reported at
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c17
> > > > 
> > > > but, I can't see how the change here would fix the corresponding
> > > > problem
> > > > described in
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=108070#c18
> > > > 
> > > > Would you explain?
> > > > 
> > > > I think there are more fundamental problems in TypeC HPD handling
> > > > as
> > > > we
> > > > discussed earlier here on the list, which should be fixed first:
> > > > 
> > > > - Switching to/from type C mode from the interrupt handler
> > > > without
> > > >   considering if we have an active mode or an ongoing AUX
> > > > transfer.
> > > 
> > > Yes that is still missing, I plan to work on that after I deliver a
> > > few
> > > tasks. Also I read the emails in discussion that you started about
> > > that.
> > 
> > The point is that imo those fundamental problems should be fixed
> > first.
> > 
> > > > - Not having any way for handling the case where we'd do a
> > > > modeset
> > > > after
> > > >   an HPD disconnect interrupt, like in the case of
> > > >   common-hpd-after-suspend in the bug report.
> > > 
> > > For what I understood about this test it will suspend, disconnect
> > > chamelium board from source, wakeup and check if connector state
> > > match,
> > > as we did not marked port as safe before suspending we are left in
> > > a
> > > invalid state where PD firmware do not trigger a interruption after
> > > wakeup.
> > 
> > No, it's not about the lack of HPD interrupts. What happens is that
> > the
> > sink disappears during system suspend/resume, but we still have an
> > active mode enabled on the corresponding port. We try to restore this
> > mode during resume but that modeset will fail.
> 
> Before suspend, all display output is disabled so there is no active
> modeset.

Yes, there is:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5129/fi-icl-u2/dmesg0.log :

<7>[  204.806237] [drm:intel_atomic_check [i915]] [CONNECTOR:209:HDMI-A-2] Limiting display bpp to 24 instead of EDID bpp 24, requested bpp 36, max platform bpp 36
...
<7>[  204.806411] [drm:intel_dump_pipe_config [i915]] [CRTC:80:pipe A][modeset]
...
<7>[  204.809132] [drm:intel_enable_pipe [i915]] enabling pipe A
...
<7>[  204.909519] [IGT] kms_chamelium: executing
...
<7>[  204.947440] [IGT] kms_chamelium: starting subtest common-hpd-after-suspend
...
<6>[  205.936472] PM: suspend entry (deep)

After which we will try to restore this active mode during resume.

> 
> > 
> > > > More comments below:
> > > > 
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c      |  1 +
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > > > >  drivers/gpu/drm/i915/i915_irq.c      |  1 +
> > > > >  drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > > > >  drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
> > > > >  6 files changed, 32 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > index acb516308262..14331c396278 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct
> > > > > drm_device
> > > > > *dev)
> > > > >  
> > > > >  	intel_runtime_pm_disable_interrupts(dev_priv);
> > > > >  	intel_hpd_cancel_work(dev_priv);
> > > > > +	intel_hpd_suspend(dev_priv);
> > > > 
> > > > What about runtime suspend? We won't receive HPD interrupts after
> > > > that
> > > > either, so we'd need to disconnect in that case too according to
> > > > the
> > > > spec.
> > > 
> > > Oh my bad I was thinking that the regular suspend would handle this
> > > too
> > > but I was wrong.
> > > 
> > > So I will add a intel_hpd_suspend() call to intel_runtime_suspend()
> > > leaving the i915_drv.c changes as this.
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index b1d23c73c147..948914a79c67 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1905,6 +1905,7 @@ static int i915_drm_suspend(struct drm_device
> > > *dev)
> > > 
> > >         intel_runtime_pm_disable_interrupts(dev_priv);
> > >         intel_hpd_cancel_work(dev_priv);
> > > +       intel_hpd_suspend(dev_priv);
> > > 
> > >         intel_suspend_encoders(dev_priv);
> > > 
> > > @@ -2909,6 +2910,7 @@ static int intel_runtime_suspend(struct
> > > device
> > > *kdev)
> > >         intel_uc_suspend(dev_priv);
> > > 
> > >         intel_runtime_pm_disable_interrupts(dev_priv);
> > > +       intel_hpd_suspend(dev_priv);
> > > 
> > >         intel_uncore_suspend(dev_priv);
> > > 
> > > Also I will drop the next patch as we don't call
> > > intel_hpd_cancel_work() in intel_runtime_suspend() path.
> > > 
> > > 
> > > > And how are we handling resume then where we'll be in a
> > > > disconnected
> > > > state due to the change in this patch and would continue to do a
> > > > modeset (to restore the mode we had before suspend).
> > > 
> > > The drm_kms_helper_poll_enable() calls will trigger the call to the
> > > detection function of the connectors that will call
> > > intel_digital_port_connected() thal will call ICL TC functions that
> > > will take care of update everything.
> > 
> > No detection will be triggered, since we're not polling this port
> > (and
> > polling can be disabled as a whole). In any case that would be too
> > late,
> > since we're trying to restore the mode before we call
> > drm_kms_helper_poll_enable().  Also doing a detection is not enough
> > here, since if detection fails (as it would in our case) we still
> > have a
> > mode that we need to restore. The corresponding modeset would fail,
> > but
> > we don't have a way to fail it gracefully as I wrote above.
> 
> 
> For the regular suspend it will run the connector detection functions
> for sure when trying to restore the previous modeset.

Not sure what you mean.
From drm_kms_helper_poll_enable():

	if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
		return;

so we won't do anything in this function if polling is globally disabled.

	if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT |            
                                         DRM_CONNECTOR_POLL_DISCONNECT))         
                        poll = true;

so we won't do anything if we haven't enabled polling for the connector.
Generally we don't enable polling for connectors which provide HPD
interrupts.

There is actually a detection work scheduled from intel_hpd_init(), but
again that will run only after we tried to restore the pre-suspend mode.
Also note what I wrote about detection being not enough for us. The sink
can be gone by the time we resume and we will still try to restore the
mode.

> About the runtime resume I need to check if with the polling disabled
> it will still update the connector state.
> 
> 
> > 
> > > > >  
> > > > >  	intel_suspend_encoders(dev_priv);
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 0c8438de3c1b..96d5ddc36f4e 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -2792,6 +2792,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_hpd_suspend(struct drm_i915_private *dev_priv);
> > > > >  
> > > > >  /* i915_irq.c */
> > > > >  static inline void i915_queue_hangcheck(struct
> > > > > drm_i915_private
> > > > > *dev_priv)
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > index d7e47d6082de..23084d227e2a 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  {
> > > > >  	drm_irq_uninstall(&dev_priv->drm);
> > > > >  	intel_hpd_cancel_work(dev_priv);
> > > > > +	intel_hpd_suspend(dev_priv);
> > > > >  	dev_priv->runtime_pm.irqs_enabled = false;
> > > > >  }
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 5258c9d654f4..27c6163426d6 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct
> > > > > intel_encoder *encoder)
> > > > >  	return false;
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * intel_digital_port_force_disconnection - Used to force port
> > > > > disconnection or
> > > > > + * release any control from display driver before going to a
> > > > > state
> > > > > that driver
> > > > > + * will not handle interruptions.
> > > > > + */
> > > > > +void intel_digital_port_force_disconnection(struct
> > > > > intel_encoder
> > > > > *encoder)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > > >base.dev);
> > > > > +	struct intel_digital_port *dig_port;
> > > > > +
> > > > > +	dig_port = enc_to_dig_port(&encoder->base);
> > > > > +	/* Skip fake MST ports */
> > > > > +	if (!dig_port)
> > > > > +		return;
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > > > +		icl_tc_phy_disconnect(dev_priv, dig_port);
> > > > > +}
> > > > > +
> > > > >  static struct edid *
> > > > >  intel_dp_get_edid(struct intel_dp *intel_dp)
> > > > >  {
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 6772e9974751..bc30f107b430 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp
> > > > > *intel_dp);
> > > > >  int intel_dp_link_required(int pixel_clock, int bpp);
> > > > >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > >  bool intel_digital_port_connected(struct intel_encoder
> > > > > *encoder);
> > > > > +void intel_digital_port_force_disconnection(struct
> > > > > intel_encoder
> > > > > *encoder);
> > > > >  
> > > > >  /* intel_dp_aux_backlight.c */
> > > > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector
> > > > > *intel_connector);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> > > > > b/drivers/gpu/drm/i915/intel_hotplug.c
> > > > > index 42e61e10f517..d6745b7b79d5 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > > > > @@ -655,3 +655,12 @@ void intel_hpd_enable(struct
> > > > > drm_i915_private
> > > > > *dev_priv, enum hpd_pin pin)
> > > > >  	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
> > > > >  	spin_unlock_irq(&dev_priv->irq_lock);
> > > > >  }
> > > > > +
> > > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> > > > > +{
> > > > > +	struct drm_device *dev = &dev_priv->drm;
> > > > > +	struct intel_encoder *encoder;
> > > > > +
> > > > > +	for_each_intel_encoder(dev, encoder)
> > > > > +		intel_digital_port_force_disconnection(encoder)
> > > > > ;
> > > > > +}
> > > > > -- 
> > > > > 2.19.1
> > > > > 
> > 
> > 


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