[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 6a90558de213..f93aab165c7e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4289,7 +4289,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 dc63303225fc..f47cec8b7f01 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 7e3b4e8fdf3a..5b840a5d00cc 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -4053,14 +4053,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);
 
@@ -4082,7 +4084,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 f40b3342d82a..f74e72173874 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4738,14 +4738,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);
 
@@ -4764,7 +4766,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 40ebc94b2187..be9fe0268472 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 *);
@@ -2016,8 +2023,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 68f497493d43..05ae7b3704a7 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1838,12 +1838,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.21.0

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

[PATCH 2/2] drm/i915: Enable hotplug retry

[José Roberto de Souza]

Right now we are aware of two cases that needs another hotplug retry:
- Unpowered type-c dongles
- HDMI slow unplug

Both have a complete explanation in the code to schedule another run
of the hotplug handler.

It could have more checks to just trigger the retry in those two
specific cases but why would sink signal a long pulse if there is
no change? Also the drawback of running the hotplug handler again
is really low and that could fix another cases that we are not
aware.

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  | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_hdmi.c | 28 +++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 5b840a5d00cc..afec10dfa360 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -4058,6 +4058,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
 		  struct intel_connector *connector,
 		  bool irq_received)
 {
+	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;
@@ -4084,6 +4085,26 @@ 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 time to those dongles to power up
+	 * and then retrying the probe.
+	 *
+	 * 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.
+	 */
+	if (state == INTEL_HOTPLUG_NOCHANGE && irq_received &&
+	    !dig_port->dp.is_mst)
+		state = INTEL_HOTPLUG_RETRY;
+
 	return state;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index cd422a7b4da0..365705e81f58 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -3025,6 +3025,32 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 		DRM_DEBUG_KMS("CEC notifier get failed\n");
 }
 
+static enum intel_hotplug_state
+intel_hdmi_hotplug(struct intel_encoder *encoder,
+		   struct intel_connector *connector, bool irq_received)
+{
+	enum intel_hotplug_state state;
+
+	state = intel_encoder_hotplug(encoder, connector, irq_received);
+
+	/*
+	 * 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.
+	 */
+	if (state == INTEL_HOTPLUG_NOCHANGE && irq_received)
+		state = INTEL_HOTPLUG_RETRY;
+
+	return state;
+}
+
 void intel_hdmi_init(struct drm_i915_private *dev_priv,
 		     i915_reg_t hdmi_reg, enum port port)
 {
@@ -3048,7 +3074,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
 			 &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS,
 			 "HDMI %c", port_name(port));
 
-	intel_encoder->hotplug = intel_encoder_hotplug;
+	intel_encoder->hotplug = intel_hdmi_hotplug;
 	intel_encoder->compute_config = intel_hdmi_compute_config;
 	if (HAS_PCH_SPLIT(dev_priv)) {
 		intel_encoder->disable = pch_disable_hdmi;
-- 
2.21.0

_______________________________________________
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/57912/
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:3559:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3560:16: warning: expression using sizeof(void)

Commit: drm/i915: Enable hotplug retry
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/57912/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5741 -> Patchwork_12448
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12448 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12448, 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/57912/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7500u:       NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-kbl-7500u:       NOTRUN -> SKIP [fdo#109271] +28

  * igt@gem_exec_basic@gtt-bsd2:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +57

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] +57

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

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-byt-j1900:       PASS -> SKIP [fdo#109271]

  * igt@i915_pm_rpm@basic-rte:
    - fi-byt-j1900:       PASS -> FAIL [fdo#108800]

  * igt@kms_busy@basic-flip-a:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  * igt@kms_busy@basic-flip-c:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

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

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +8

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         PASS -> DMESG-FAIL [fdo#103182]

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-icl-u3:          FAIL [fdo#103375] -> PASS

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       DMESG-WARN [fdo#103841] -> PASS

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

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> 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#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [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#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110028]: https://bugs.freedesktop.org/show_bug.cgi?id=110028


Participating hosts (46 -> 43)
------------------------------

  Additional (3): fi-icl-y fi-byt-clapper fi-snb-2520m 
  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bdw-samus 


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

    * Linux: CI_DRM_5741 -> Patchwork_12448

  CI_DRM_5741: be1c0f990e47106f8a94dedf9d09a4e0ef3d79fc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4884: c46051337b972f8b5a302afb6f603df06fea527d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12448: 93d90e75ff77f2eb327c21bb2e8c27c3686194db @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

93d90e75ff77 drm/i915: Enable hotplug retry
b3bccc6aeb43 drm/i915: Add support for retrying hotplug

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12448/
_______________________________________________
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 Tue, Mar 12, 2019 at 05:58:51PM -0700, José Roberto de Souza 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.
> 
> 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 6a90558de213..f93aab165c7e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4289,7 +4289,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 dc63303225fc..f47cec8b7f01 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 7e3b4e8fdf3a..5b840a5d00cc 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -4053,14 +4053,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);
>  
> @@ -4082,7 +4084,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 f40b3342d82a..f74e72173874 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4738,14 +4738,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);
>  
> @@ -4764,7 +4766,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 40ebc94b2187..be9fe0268472 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 *);
> @@ -2016,8 +2023,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);

Just realized, that this way we'll do hotplug processing for pins with a
shared encoder (DDI DP/HDMI) twice, even if one encoder's hotplug hook
signaled a change (and so there can't be any change on other connectors
sharing this encoder). I suppose preventing calling the hook for a pin that
already signalled a change would make sense, but that's a separate
issue and could still lead to retrying needlessly. So I think we should
prevent the retry here explicitly:

	retry &= ~changed;

(making changed a bitmap).

> +
> +	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 68f497493d43..05ae7b3704a7 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1838,12 +1838,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.21.0
> 
_______________________________________________
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

[Souza, Jose]

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

On Thu, 2019-03-14 at 18:09 +0200, Imre Deak wrote:
> On Tue, Mar 12, 2019 at 05:58:51PM -0700, José Roberto de Souza
> 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.
> > 
> > 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 6a90558de213..f93aab165c7e 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4289,7 +4289,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 dc63303225fc..f47cec8b7f01 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 7e3b4e8fdf3a..5b840a5d00cc 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -4053,14 +4053,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);
> >  
> > @@ -4082,7 +4084,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 f40b3342d82a..f74e72173874 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4738,14 +4738,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);
> >  
> > @@ -4764,7 +4766,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 40ebc94b2187..be9fe0268472 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 *);
> > @@ -2016,8 +2023,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_connect
> > or);
> > +			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);
> 
> Just realized, that this way we'll do hotplug processing for pins
> with a
> shared encoder (DDI DP/HDMI) twice, even if one encoder's hotplug
> hook
> signaled a change (and so there can't be any change on other
> connectors
> sharing this encoder). I suppose preventing calling the hook for a
> pin that
> already signalled a change would make sense, but that's a separate
> issue and could still lead to retrying needlessly. So I think we
> should
> prevent the retry here explicitly:
> 
> 	retry &= ~changed;

I was not aware of this shared hotplug pin, but it will need more than
that, if the first port sharing a pin have changed but the second don't
it would still mark the pin to retry, I will fix that and add the
"retry" to the hotplug DP handler of non-ddi platforms.

> 
> (making changed a bitmap).
> 
> > +
> > +	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 68f497493d43..05ae7b3704a7 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1838,12 +1838,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.21.0
> > 

[-- 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 1/2] drm/i915: Add support for retrying hotplug

[Imre Deak]

On Fri, Mar 15, 2019 at 02:25:36AM +0200, Souza, Jose wrote:
> On Thu, 2019-03-14 at 18:09 +0200, Imre Deak wrote:
> > On Tue, Mar 12, 2019 at 05:58:51PM -0700, José Roberto de Souza
> > 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.
> > > 
> > > 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 6a90558de213..f93aab165c7e 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -4289,7 +4289,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 dc63303225fc..f47cec8b7f01 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 7e3b4e8fdf3a..5b840a5d00cc 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -4053,14 +4053,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);
> > >  
> > > @@ -4082,7 +4084,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 f40b3342d82a..f74e72173874 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4738,14 +4738,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);
> > >  
> > > @@ -4764,7 +4766,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 40ebc94b2187..be9fe0268472 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 *);
> > > @@ -2016,8 +2023,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_connect
> > > or);
> > > +			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);
> > 
> > Just realized, that this way we'll do hotplug processing for pins
> > with a
> > shared encoder (DDI DP/HDMI) twice, even if one encoder's hotplug
> > hook
> > signaled a change (and so there can't be any change on other
> > connectors
> > sharing this encoder). I suppose preventing calling the hook for a
> > pin that
> > already signalled a change would make sense, but that's a separate
> > issue and could still lead to retrying needlessly. So I think we
> > should
> > prevent the retry here explicitly:
> > 
> > 	retry &= ~changed;
> 
> I was not aware of this shared hotplug pin, but it will need more than
> that, if the first port sharing a pin have changed but the second don't
> it would still mark the pin to retry,

For two connectors sharing an encoder their ports and pins will be the same.
So if you do above:

		case INTEL_HOTPLUG_CHANGED:
			changed |= hpd_bit;
			break;

		...

you can just avoid the retries for such changed connectors by

	retry &= ~changed;
	
here.	


> I will fix that and add the
> "retry" to the hotplug DP handler of non-ddi platforms.
> 
> > 
> > (making changed a bitmap).
> > 
> > > +
> > > +	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 68f497493d43..05ae7b3704a7 100644
> > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > @@ -1838,12 +1838,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.21.0
> > > 


_______________________________________________
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

[Souza, Jose]

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

On Fri, 2019-03-15 at 03:39 +0200, Imre Deak wrote:
> On Fri, Mar 15, 2019 at 02:25:36AM +0200, Souza, Jose wrote:
> > On Thu, 2019-03-14 at 18:09 +0200, Imre Deak wrote:
> > > On Tue, Mar 12, 2019 at 05:58:51PM -0700, José Roberto de Souza
> > > 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.
> > > > 
> > > > 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 6a90558de213..f93aab165c7e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -4289,7 +4289,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 dc63303225fc..f47cec8b7f01 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 7e3b4e8fdf3a..5b840a5d00cc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -4053,14 +4053,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);
> > > >  
> > > > @@ -4082,7 +4084,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 f40b3342d82a..f74e72173874 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4738,14 +4738,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);
> > > >  
> > > > @@ -4764,7 +4766,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 40ebc94b2187..be9fe0268472 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 *);
> > > > @@ -2016,8 +2023,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
> > > > _connect
> > > > or);
> > > > +			switch (intel_encoder-
> > > > >hotplug(intel_encoder,
> > > > +						       intel_co
> > > > nnector,
> > > > +						       hpd_even
> > > > t_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);
> > > 
> > > Just realized, that this way we'll do hotplug processing for pins
> > > with a
> > > shared encoder (DDI DP/HDMI) twice, even if one encoder's hotplug
> > > hook
> > > signaled a change (and so there can't be any change on other
> > > connectors
> > > sharing this encoder). I suppose preventing calling the hook for
> > > a
> > > pin that
> > > already signalled a change would make sense, but that's a
> > > separate
> > > issue and could still lead to retrying needlessly. So I think we
> > > should
> > > prevent the retry here explicitly:
> > > 
> > > 	retry &= ~changed;
> > 
> > I was not aware of this shared hotplug pin, but it will need more
> > than
> > that, if the first port sharing a pin have changed but the second
> > don't
> > it would still mark the pin to retry,
> 
> For two connectors sharing an encoder their ports and pins will be
> the same.
> So if you do above:
> 
> 		case INTEL_HOTPLUG_CHANGED:
> 			changed |= hpd_bit;
> 			break;
> 
> 		...
> 
> you can just avoid the retries for such changed connectors by
> 
> 	retry &= ~changed;
> 	
> here.	


So lets say that we have: HDMI-1 on enconder_A and DP-1 on encoder_A,
user hotplug HDMI-1 and when iterating over all connectors in
i915_hotplug_work_func() we have this:

// fist iteration
ret = intel_encoder->hotplug->hotplug(encoder, HDMI-1);
//ret = INTEL_HOTPLUG_CHANGED

// second iteration
ret = intel_encoder->hotplug->hotplug(encoder, DP-1);
//ret = INTEL_HOTPLUG_RETRY


so a 'retry &= ~changed;' in 'case INTEL_HOTPLUG_CHANGED:' alone would
not prevent it to set retry_bits;

Or I did not understood correctly this shared encoder and hpd pin?


> 
> 
> > I will fix that and add the
> > "retry" to the hotplug DP handler of non-ddi platforms.
> > 
> > > (making changed a bitmap).
> > > 
> > > > +
> > > > +	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_DEL
> > > > AY));
> > > > +	}
> > > >  }
> > > >  
> > > >  
> > > > @@ -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 68f497493d43..05ae7b3704a7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > > @@ -1838,12 +1838,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.21.0
> > > > 
> 
> 

[-- 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 1/2] drm/i915: Add support for retrying hotplug

[Imre Deak]

On Sat, Mar 16, 2019 at 12:22:30AM +0200, Souza, Jose wrote:
> [...]
> > > > > @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct
> > > > > work_struct *work)
> > > > >  
> > > > >  	if (changed)
> > > > >  		drm_kms_helper_hotplug_event(dev);
> > > > 
> > > > Just realized, that this way we'll do hotplug processing for
> > > > pins with a shared encoder (DDI DP/HDMI) twice, even if one
> > > > encoder's hotplug hook signaled a change (and so there can't be
> > > > any change on other connectors sharing this encoder). I suppose
> > > > preventing calling the hook for a pin that already signalled a
> > > > change would make sense, but that's a separate issue and could
> > > > still lead to retrying needlessly. So I think we should prevent
> > > > the retry here explicitly:
> > > > 
> > > > 	retry &= ~changed;
> > > 
> > > I was not aware of this shared hotplug pin, but it will need more
> > > than that, if the first port sharing a pin have changed but the
> > > second don't it would still mark the pin to retry,
> > 
> > For two connectors sharing an encoder their ports and pins will be
> > the same. So if you do above:
> > 
> > 		case INTEL_HOTPLUG_CHANGED:
> > 			changed |= hpd_bit;
> > 			break;
> > 
> > 		...
> > 
> > you can just avoid the retries for such changed connectors by
> > 
> > 	retry &= ~changed;
> > 	
> > here.	
> 
> So lets say that we have: HDMI-1 on enconder_A and DP-1 on encoder_A,
> user hotplug HDMI-1 and when iterating over all connectors in
> i915_hotplug_work_func() we have this:
> 
> // fist iteration
> ret = intel_encoder->hotplug->hotplug(encoder, HDMI-1);
> //ret = INTEL_HOTPLUG_CHANGED
> 
> // second iteration
> ret = intel_encoder->hotplug->hotplug(encoder, DP-1);
> //ret = INTEL_HOTPLUG_RETRY
> 
> so a 'retry &= ~changed;' in 'case INTEL_HOTPLUG_CHANGED:' alone would
> not prevent it to set retry_bits;

The bits from retry are cleared after the loop which does prevent it.
Here's the updated diff for the function:

@@ -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 changed = 0;
+	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 |= hpd_bit;
+				break;
+			case INTEL_HOTPLUG_RETRY:
+				retry |= hpd_bit;
+				break;
+			}
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
@@ -389,6 +410,17 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	if (changed)
 		drm_kms_helper_hotplug_event(dev);
+
+	retry &= ~changed;
+
+	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));
+	}
 }
 
 
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx