* [PATCH 1/2] drm/i915: Add support for retrying hotplug
@ 2019-02-22 21:08 José Roberto de Souza
2019-02-22 21:08 ` [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed José Roberto de Souza
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: José Roberto de Souza @ 2019-02-22 21:08 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula
From: Imre Deak <imre.deak@intel.com>
There is some scenarios that we are aware that sink probe can fail,
so lets add the infrastructure to let hotplug() hook to request
another probe after some time.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_ddi.c | 12 +++---
drivers/gpu/drm/i915/intel_dp.c | 12 +++---
drivers/gpu/drm/i915/intel_drv.h | 17 +++++++--
drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
drivers/gpu/drm/i915/intel_sdvo.c | 8 ++--
7 files changed, 79 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 37175414ce89..8149be62a630 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
*/
synchronize_irq(dev_priv->drm.irq);
flush_work(&dev_priv->hotplug.dig_port_work);
- flush_work(&dev_priv->hotplug.hotplug_work);
+ flush_delayed_work(&dev_priv->hotplug.hotplug_work);
seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
seq_printf(m, "Detected: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bb0e75e43987..5703526bddab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -156,7 +156,7 @@ enum hpd_pin {
#define HPD_STORM_DEFAULT_THRESHOLD 50
struct i915_hotplug {
- struct work_struct hotplug_work;
+ struct delayed_work hotplug_work;
struct {
unsigned long last_jiffies;
@@ -168,6 +168,7 @@ struct i915_hotplug {
} state;
} stats[HPD_NUM_PINS];
u32 event_bits;
+ u32 retry_bits;
struct delayed_work reenable_work;
u32 long_port_mask;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ea83071a22c4..1676a87f18cb 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
return modeset_pipe(&crtc->base, ctx);
}
-static bool intel_ddi_hotplug(struct intel_encoder *encoder,
- struct intel_connector *connector)
+static enum intel_hotplug_state
+intel_ddi_hotplug(struct intel_encoder *encoder,
+ struct intel_connector *connector,
+ bool irq_received)
{
struct drm_modeset_acquire_ctx ctx;
- bool changed;
+ enum intel_hotplug_state state;
int ret;
- changed = intel_encoder_hotplug(encoder, connector);
+ state = intel_encoder_hotplug(encoder, connector, irq_received);
drm_modeset_acquire_init(&ctx, 0);
@@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
drm_modeset_acquire_fini(&ctx);
WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
- return changed;
+ return state;
}
static struct intel_connector *
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1a051c0fbfe..0bb43eaa154c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
* retrain the link to get a picture. That's in case no
* userspace component reacted to intermittent HPD dip.
*/
-static bool intel_dp_hotplug(struct intel_encoder *encoder,
- struct intel_connector *connector)
+static enum intel_hotplug_state
+intel_dp_hotplug(struct intel_encoder *encoder,
+ struct intel_connector *connector,
+ bool irq_received)
{
struct drm_modeset_acquire_ctx ctx;
- bool changed;
+ enum intel_hotplug_state state;
int ret;
- changed = intel_encoder_hotplug(encoder, connector);
+ state = intel_encoder_hotplug(encoder, connector, irq_received);
drm_modeset_acquire_init(&ctx, 0);
@@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
drm_modeset_acquire_fini(&ctx);
WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
- return changed;
+ return state;
}
static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81ec73e4a083..de62af970dc1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -226,14 +226,21 @@ struct intel_fbdev {
struct mutex hpd_lock;
};
+enum intel_hotplug_state {
+ INTEL_HOTPLUG_NOCHANGE,
+ INTEL_HOTPLUG_CHANGED,
+ INTEL_HOTPLUG_RETRY,
+};
+
struct intel_encoder {
struct drm_encoder base;
enum intel_output_type type;
enum port port;
unsigned int cloneable;
- bool (*hotplug)(struct intel_encoder *encoder,
- struct intel_connector *connector);
+ enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
+ struct intel_connector *connector,
+ bool irq_received);
enum intel_output_type (*compute_output_type)(struct intel_encoder *,
struct intel_crtc_state *,
struct drm_connector_state *);
@@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
void intel_dvo_init(struct drm_i915_private *dev_priv);
/* intel_hotplug.c */
void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
-bool intel_encoder_hotplug(struct intel_encoder *encoder,
- struct intel_connector *connector);
+enum intel_hotplug_state
+intel_encoder_hotplug(struct intel_encoder *encoder,
+ struct intel_connector *connector,
+ bool irq_received);
/* legacy fbdev emulation in intel_fbdev.c */
#ifdef CONFIG_DRM_FBDEV_EMULATION
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index b8937c788f03..3eaa06526f6b 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
#define HPD_STORM_DETECT_PERIOD 1000
#define HPD_STORM_REENABLE_DELAY (2 * 60 * 1000)
+#define HPD_RETRY_DELAY 1000
/**
* intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
@@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
intel_runtime_pm_put(dev_priv, wakeref);
}
-bool intel_encoder_hotplug(struct intel_encoder *encoder,
- struct intel_connector *connector)
+enum intel_hotplug_state
+intel_encoder_hotplug(struct intel_encoder *encoder,
+ struct intel_connector *connector,
+ bool irq_received)
{
struct drm_device *dev = connector->base.dev;
enum drm_connector_status old_status;
@@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
drm_helper_probe_detect(&connector->base, NULL, false);
if (old_status == connector->base.status)
- return false;
+ return INTEL_HOTPLUG_NOCHANGE;
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
connector->base.base.id,
@@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
drm_get_connector_status_name(old_status),
drm_get_connector_status_name(connector->base.status));
- return true;
+ return INTEL_HOTPLUG_CHANGED;
}
static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
@@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
spin_lock_irq(&dev_priv->irq_lock);
dev_priv->hotplug.event_bits |= old_bits;
spin_unlock_irq(&dev_priv->irq_lock);
- schedule_work(&dev_priv->hotplug.hotplug_work);
+ schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
}
}
@@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
static void i915_hotplug_work_func(struct work_struct *work)
{
struct drm_i915_private *dev_priv =
- container_of(work, struct drm_i915_private, hotplug.hotplug_work);
+ container_of(work, struct drm_i915_private,
+ hotplug.hotplug_work.work);
struct drm_device *dev = &dev_priv->drm;
struct intel_connector *intel_connector;
struct intel_encoder *intel_encoder;
struct drm_connector *connector;
struct drm_connector_list_iter conn_iter;
bool changed = false;
+ u32 retry = 0;
u32 hpd_event_bits;
+ u32 hpd_retry_bits;
mutex_lock(&dev->mode_config.mutex);
DRM_DEBUG_KMS("running encoder hotplug functions\n");
@@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
hpd_event_bits = dev_priv->hotplug.event_bits;
dev_priv->hotplug.event_bits = 0;
+ hpd_retry_bits = dev_priv->hotplug.retry_bits;
+ dev_priv->hotplug.retry_bits = 0;
/* Enable polling for connectors which had HPD IRQ storms */
intel_hpd_irq_storm_switch_to_polling(dev_priv);
@@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter) {
+ u32 hpd_bit;
+
intel_connector = to_intel_connector(connector);
if (!intel_connector->encoder)
continue;
intel_encoder = intel_connector->encoder;
- if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+ hpd_bit = BIT(intel_encoder->hpd_pin);
+ if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
connector->name, intel_encoder->hpd_pin);
- changed |= intel_encoder->hotplug(intel_encoder,
- intel_connector);
+ switch (intel_encoder->hotplug(intel_encoder,
+ intel_connector,
+ hpd_event_bits & hpd_bit)) {
+ case INTEL_HOTPLUG_NOCHANGE:
+ break;
+ case INTEL_HOTPLUG_CHANGED:
+ changed = true;
+ break;
+ case INTEL_HOTPLUG_RETRY:
+ retry |= hpd_bit;
+ break;
+ }
}
}
drm_connector_list_iter_end(&conn_iter);
@@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
if (changed)
drm_kms_helper_hotplug_event(dev);
+
+ if (retry) {
+ spin_lock_irq(&dev_priv->irq_lock);
+ dev_priv->hotplug.retry_bits |= retry;
+ spin_unlock_irq(&dev_priv->irq_lock);
+
+ mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
+ msecs_to_jiffies(HPD_RETRY_DELAY));
+ }
}
@@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
if (queue_dig)
queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
if (queue_hp)
- schedule_work(&dev_priv->hotplug.hotplug_work);
+ schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
}
/**
@@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
void intel_hpd_init_work(struct drm_i915_private *dev_priv)
{
- INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
+ INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
+ i915_hotplug_work_func);
INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
@@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
dev_priv->hotplug.long_port_mask = 0;
dev_priv->hotplug.short_port_mask = 0;
dev_priv->hotplug.event_bits = 0;
+ dev_priv->hotplug.retry_bits = 0;
spin_unlock_irq(&dev_priv->irq_lock);
cancel_work_sync(&dev_priv->hotplug.dig_port_work);
- cancel_work_sync(&dev_priv->hotplug.hotplug_work);
+ cancel_delayed_work_sync(&dev_priv->hotplug.hotplug_work);
cancel_work_sync(&dev_priv->hotplug.poll_init_work);
cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
}
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e7b0884ba5a5..92793475b2b9 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
&intel_sdvo->hotplug_active, 2);
}
-static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
- struct intel_connector *connector)
+static enum intel_hotplug_state
+intel_sdvo_hotplug(struct intel_encoder *encoder,
+ struct intel_connector *connector,
+ bool irq_received)
{
intel_sdvo_enable_hotplug(encoder);
- return intel_encoder_hotplug(encoder, connector);
+ return intel_encoder_hotplug(encoder, connector, irq_received);
}
static bool
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed
2019-02-22 21:08 [PATCH 1/2] drm/i915: Add support for retrying hotplug José Roberto de Souza
@ 2019-02-22 21:08 ` José Roberto de Souza
2019-02-26 14:08 ` Imre Deak
2019-02-22 21:23 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Add support for retrying hotplug Patchwork
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: José Roberto de Souza @ 2019-02-22 21:08 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula
Unpowered type-c dongles can take some time to boot and be
responsible, causing the probe to fail and sink never be detected
without further actions from userspace.
It was not a issue for older platforms because there was a hardware
bridge between DDI/DP ports and type-c controller adding a implicit
delay that hid this issue but ICL have type-c controllers integrated
to the SOC bring this issue to users.
So here if the probe failed when coming from a IRQ it returns
INTEL_HOTPLUG_RETRY that will schedule another run of
i915_hotplug_work_func() after 1 second what is time enough for
those type-c dongles to boot.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1676a87f18cb..96bbcf5c9787 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
struct intel_connector *connector,
bool irq_received)
{
+ struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+ struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
struct drm_modeset_acquire_ctx ctx;
enum intel_hotplug_state state;
int ret;
@@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
drm_modeset_acquire_fini(&ctx);
WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
+ /*
+ * Unpowered type-c dongles can take some time to boot and be
+ * responsible, so here giving some type to those dongles to power up
+ * and then retrying the probe.
+ */
+ if (state == INTEL_HOTPLUG_NOCHANGE &&
+ connector->base.status != connector_status_connected &&
+ irq_received && intel_port_is_tc(dev_priv, encoder->port) &&
+ !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
+ state = INTEL_HOTPLUG_RETRY;
+
return state;
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Add support for retrying hotplug
2019-02-22 21:08 [PATCH 1/2] drm/i915: Add support for retrying hotplug José Roberto de Souza
2019-02-22 21:08 ` [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed José Roberto de Souza
@ 2019-02-22 21:23 ` Patchwork
2019-02-22 21:42 ` ✗ Fi.CI.BAT: failure " Patchwork
2019-02-25 11:02 ` [PATCH 1/2] " Jani Nikula
3 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2019-02-22 21:23 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Add support for retrying hotplug
URL : https://patchwork.freedesktop.org/series/57108/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Add support for retrying hotplug
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3583:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3584:16: warning: expression using sizeof(void)
Commit: drm/i915/icl: Probe again type-c connectors that failed
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Add support for retrying hotplug
2019-02-22 21:08 [PATCH 1/2] drm/i915: Add support for retrying hotplug José Roberto de Souza
2019-02-22 21:08 ` [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed José Roberto de Souza
2019-02-22 21:23 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Add support for retrying hotplug Patchwork
@ 2019-02-22 21:42 ` Patchwork
2019-02-25 11:02 ` [PATCH 1/2] " Jani Nikula
3 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2019-02-22 21:42 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Add support for retrying hotplug
URL : https://patchwork.freedesktop.org/series/57108/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5654 -> Patchwork_12286
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_12286 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_12286, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/57108/revisions/1/mbox/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12286:
### IGT changes ###
#### Possible regressions ####
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7567u: PASS -> FAIL
Known issues
------------
Here are the changes found in Patchwork_12286 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@memory-alloc:
- fi-ivb-3520m: NOTRUN -> SKIP [fdo#109271] +48
* igt@amdgpu/amd_cs_nop@fork-compute0:
- fi-blb-e6850: NOTRUN -> SKIP [fdo#109271] +18
* igt@amdgpu/amd_cs_nop@fork-gfx0:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109315] +17
* igt@debugfs_test@read_all_entries:
- fi-kbl-7567u: PASS -> DMESG-WARN [fdo#103558] / [fdo#105602]
* igt@gem_exec_basic@readonly-bsd1:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109276] +7
* igt@gem_exec_parse@basic-allowed:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109289] +1
* igt@gem_exec_suspend@basic-s3:
- fi-kbl-7567u: PASS -> DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602]
* igt@i915_module_load@reload-with-fault-injection:
- fi-kbl-7567u: PASS -> DMESG-WARN [fdo#105602] / [fdo#108529] +1
* igt@i915_pm_rpm@module-reload:
- fi-kbl-7567u: PASS -> DMESG-WARN [fdo#108529]
* igt@i915_selftest@live_contexts:
- fi-icl-u2: NOTRUN -> DMESG-FAIL [fdo#108569]
* igt@kms_busy@basic-flip-a:
- fi-kbl-7567u: PASS -> SKIP [fdo#109271] / [fdo#109278] +2
- fi-gdg-551: PASS -> FAIL [fdo#103182]
* igt@kms_chamelium@dp-edid-read:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109316] +2
* igt@kms_chamelium@dp-hpd-fast:
- fi-skl-6700k2: NOTRUN -> SKIP [fdo#109271] +41
* igt@kms_chamelium@hdmi-crc-fast:
- fi-icl-u2: NOTRUN -> FAIL [fdo#109635 ]
* igt@kms_chamelium@hdmi-edid-read:
- fi-kbl-7567u: PASS -> FAIL [fdo#109569] +1
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: PASS -> FAIL [fdo#109485]
* igt@kms_chamelium@vga-hpd-fast:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109309] +1
* igt@kms_force_connector_basic@prune-stale-modes:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109285] +3
* igt@kms_frontbuffer_tracking@basic:
- fi-icl-u2: NOTRUN -> FAIL [fdo#103167]
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-kbl-7567u: PASS -> DMESG-FAIL [fdo#105079]
* igt@prime_vgem@basic-fence-flip:
- fi-kbl-7567u: PASS -> SKIP [fdo#109271] +4
#### Possible fixes ####
* igt@i915_module_load@reload:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS
* igt@i915_pm_rpm@module-reload:
- {fi-icl-y}: INCOMPLETE [fdo#108840] -> PASS
* igt@i915_selftest@live_evict:
- fi-bsw-kefka: DMESG-WARN [fdo#107709] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
[fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
[fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
[fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
[fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
[fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
[fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316
[fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
[fdo#109569]: https://bugs.freedesktop.org/show_bug.cgi?id=109569
[fdo#109635 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109635
Participating hosts (43 -> 39)
------------------------------
Additional (3): fi-ivb-3520m fi-icl-u2 fi-skl-6700k2
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510 fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_5654 -> Patchwork_12286
CI_DRM_5654: 30c7f283790b433aa311ef7a7d2b6b428886fb9a @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4851: 2b7dd10a4e2ea0cabff68421fd15e96c99be3cad @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12286: 83788414ca236a50363703647234c6420a46f292 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
83788414ca23 drm/i915/icl: Probe again type-c connectors that failed
0c881f78fc0a drm/i915: Add support for retrying hotplug
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12286/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-02-22 21:08 [PATCH 1/2] drm/i915: Add support for retrying hotplug José Roberto de Souza
` (2 preceding siblings ...)
2019-02-22 21:42 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2019-02-25 11:02 ` Jani Nikula
2019-02-25 11:22 ` Imre Deak
3 siblings, 1 reply; 17+ messages in thread
From: Jani Nikula @ 2019-02-25 11:02 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
On Fri, 22 Feb 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> From: Imre Deak <imre.deak@intel.com>
>
> There is some scenarios that we are aware that sink probe can fail,
> so lets add the infrastructure to let hotplug() hook to request
> another probe after some time.
See also
http://patchwork.freedesktop.org/patch/msgid/20180925071836.24711-1-jani.nikula@intel.com
BR,
Jani.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_ddi.c | 12 +++---
> drivers/gpu/drm/i915/intel_dp.c | 12 +++---
> drivers/gpu/drm/i915/intel_drv.h | 17 +++++++--
> drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
> drivers/gpu/drm/i915/intel_sdvo.c | 8 ++--
> 7 files changed, 79 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 37175414ce89..8149be62a630 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
> */
> synchronize_irq(dev_priv->drm.irq);
> flush_work(&dev_priv->hotplug.dig_port_work);
> - flush_work(&dev_priv->hotplug.hotplug_work);
> + flush_delayed_work(&dev_priv->hotplug.hotplug_work);
>
> seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
> seq_printf(m, "Detected: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb0e75e43987..5703526bddab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -156,7 +156,7 @@ enum hpd_pin {
> #define HPD_STORM_DEFAULT_THRESHOLD 50
>
> struct i915_hotplug {
> - struct work_struct hotplug_work;
> + struct delayed_work hotplug_work;
>
> struct {
> unsigned long last_jiffies;
> @@ -168,6 +168,7 @@ struct i915_hotplug {
> } state;
> } stats[HPD_NUM_PINS];
> u32 event_bits;
> + u32 retry_bits;
> struct delayed_work reenable_work;
>
> u32 long_port_mask;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ea83071a22c4..1676a87f18cb 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
> return modeset_pipe(&crtc->base, ctx);
> }
>
> -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> - struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_ddi_hotplug(struct intel_encoder *encoder,
> + struct intel_connector *connector,
> + bool irq_received)
> {
> struct drm_modeset_acquire_ctx ctx;
> - bool changed;
> + enum intel_hotplug_state state;
> int ret;
>
> - changed = intel_encoder_hotplug(encoder, connector);
> + state = intel_encoder_hotplug(encoder, connector, irq_received);
>
> drm_modeset_acquire_init(&ctx, 0);
>
> @@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> drm_modeset_acquire_fini(&ctx);
> WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>
> - return changed;
> + return state;
> }
>
> static struct intel_connector *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1a051c0fbfe..0bb43eaa154c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
> * retrain the link to get a picture. That's in case no
> * userspace component reacted to intermittent HPD dip.
> */
> -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> - struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_dp_hotplug(struct intel_encoder *encoder,
> + struct intel_connector *connector,
> + bool irq_received)
> {
> struct drm_modeset_acquire_ctx ctx;
> - bool changed;
> + enum intel_hotplug_state state;
> int ret;
>
> - changed = intel_encoder_hotplug(encoder, connector);
> + state = intel_encoder_hotplug(encoder, connector, irq_received);
>
> drm_modeset_acquire_init(&ctx, 0);
>
> @@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
> drm_modeset_acquire_fini(&ctx);
> WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>
> - return changed;
> + return state;
> }
>
> static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81ec73e4a083..de62af970dc1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -226,14 +226,21 @@ struct intel_fbdev {
> struct mutex hpd_lock;
> };
>
> +enum intel_hotplug_state {
> + INTEL_HOTPLUG_NOCHANGE,
> + INTEL_HOTPLUG_CHANGED,
> + INTEL_HOTPLUG_RETRY,
> +};
> +
> struct intel_encoder {
> struct drm_encoder base;
>
> enum intel_output_type type;
> enum port port;
> unsigned int cloneable;
> - bool (*hotplug)(struct intel_encoder *encoder,
> - struct intel_connector *connector);
> + enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
> + struct intel_connector *connector,
> + bool irq_received);
> enum intel_output_type (*compute_output_type)(struct intel_encoder *,
> struct intel_crtc_state *,
> struct drm_connector_state *);
> @@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
> void intel_dvo_init(struct drm_i915_private *dev_priv);
> /* intel_hotplug.c */
> void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> - struct intel_connector *connector);
> +enum intel_hotplug_state
> +intel_encoder_hotplug(struct intel_encoder *encoder,
> + struct intel_connector *connector,
> + bool irq_received);
>
> /* legacy fbdev emulation in intel_fbdev.c */
> #ifdef CONFIG_DRM_FBDEV_EMULATION
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index b8937c788f03..3eaa06526f6b 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>
> #define HPD_STORM_DETECT_PERIOD 1000
> #define HPD_STORM_REENABLE_DELAY (2 * 60 * 1000)
> +#define HPD_RETRY_DELAY 1000
>
> /**
> * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
> @@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> intel_runtime_pm_put(dev_priv, wakeref);
> }
>
> -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> - struct intel_connector *connector)
> +enum intel_hotplug_state
> +intel_encoder_hotplug(struct intel_encoder *encoder,
> + struct intel_connector *connector,
> + bool irq_received)
> {
> struct drm_device *dev = connector->base.dev;
> enum drm_connector_status old_status;
> @@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> drm_helper_probe_detect(&connector->base, NULL, false);
>
> if (old_status == connector->base.status)
> - return false;
> + return INTEL_HOTPLUG_NOCHANGE;
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
> connector->base.base.id,
> @@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> drm_get_connector_status_name(old_status),
> drm_get_connector_status_name(connector->base.status));
>
> - return true;
> + return INTEL_HOTPLUG_CHANGED;
> }
>
> static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
> @@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
> spin_lock_irq(&dev_priv->irq_lock);
> dev_priv->hotplug.event_bits |= old_bits;
> spin_unlock_irq(&dev_priv->irq_lock);
> - schedule_work(&dev_priv->hotplug.hotplug_work);
> + schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> }
> }
>
> @@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
> static void i915_hotplug_work_func(struct work_struct *work)
> {
> struct drm_i915_private *dev_priv =
> - container_of(work, struct drm_i915_private, hotplug.hotplug_work);
> + container_of(work, struct drm_i915_private,
> + hotplug.hotplug_work.work);
> struct drm_device *dev = &dev_priv->drm;
> struct intel_connector *intel_connector;
> struct intel_encoder *intel_encoder;
> struct drm_connector *connector;
> struct drm_connector_list_iter conn_iter;
> bool changed = false;
> + u32 retry = 0;
> u32 hpd_event_bits;
> + u32 hpd_retry_bits;
>
> mutex_lock(&dev->mode_config.mutex);
> DRM_DEBUG_KMS("running encoder hotplug functions\n");
> @@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
>
> hpd_event_bits = dev_priv->hotplug.event_bits;
> dev_priv->hotplug.event_bits = 0;
> + hpd_retry_bits = dev_priv->hotplug.retry_bits;
> + dev_priv->hotplug.retry_bits = 0;
>
> /* Enable polling for connectors which had HPD IRQ storms */
> intel_hpd_irq_storm_switch_to_polling(dev_priv);
> @@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
>
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter) {
> + u32 hpd_bit;
> +
> intel_connector = to_intel_connector(connector);
> if (!intel_connector->encoder)
> continue;
> intel_encoder = intel_connector->encoder;
> - if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> + hpd_bit = BIT(intel_encoder->hpd_pin);
> + if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
> DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> connector->name, intel_encoder->hpd_pin);
>
> - changed |= intel_encoder->hotplug(intel_encoder,
> - intel_connector);
> + switch (intel_encoder->hotplug(intel_encoder,
> + intel_connector,
> + hpd_event_bits & hpd_bit)) {
> + case INTEL_HOTPLUG_NOCHANGE:
> + break;
> + case INTEL_HOTPLUG_CHANGED:
> + changed = true;
> + break;
> + case INTEL_HOTPLUG_RETRY:
> + retry |= hpd_bit;
> + break;
> + }
> }
> }
> drm_connector_list_iter_end(&conn_iter);
> @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
>
> if (changed)
> drm_kms_helper_hotplug_event(dev);
> +
> + if (retry) {
> + spin_lock_irq(&dev_priv->irq_lock);
> + dev_priv->hotplug.retry_bits |= retry;
> + spin_unlock_irq(&dev_priv->irq_lock);
> +
> + mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
> + msecs_to_jiffies(HPD_RETRY_DELAY));
> + }
> }
>
>
> @@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> if (queue_dig)
> queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
> if (queue_hp)
> - schedule_work(&dev_priv->hotplug.hotplug_work);
> + schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> }
>
> /**
> @@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
>
> void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> {
> - INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> + INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> + i915_hotplug_work_func);
> INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
> INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
> INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
> @@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> dev_priv->hotplug.long_port_mask = 0;
> dev_priv->hotplug.short_port_mask = 0;
> dev_priv->hotplug.event_bits = 0;
> + dev_priv->hotplug.retry_bits = 0;
>
> spin_unlock_irq(&dev_priv->irq_lock);
>
> cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> - cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> + cancel_delayed_work_sync(&dev_priv->hotplug.hotplug_work);
> cancel_work_sync(&dev_priv->hotplug.poll_init_work);
> cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
> }
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index e7b0884ba5a5..92793475b2b9 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
> &intel_sdvo->hotplug_active, 2);
> }
>
> -static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
> - struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_sdvo_hotplug(struct intel_encoder *encoder,
> + struct intel_connector *connector,
> + bool irq_received)
> {
> intel_sdvo_enable_hotplug(encoder);
>
> - return intel_encoder_hotplug(encoder, connector);
> + return intel_encoder_hotplug(encoder, connector, irq_received);
> }
>
> static bool
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-02-25 11:02 ` [PATCH 1/2] " Jani Nikula
@ 2019-02-25 11:22 ` Imre Deak
0 siblings, 0 replies; 17+ messages in thread
From: Imre Deak @ 2019-02-25 11:22 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Mon, Feb 25, 2019 at 01:02:45PM +0200, Jani Nikula wrote:
> On Fri, 22 Feb 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> > From: Imre Deak <imre.deak@intel.com>
> >
> > There is some scenarios that we are aware that sink probe can fail,
> > so lets add the infrastructure to let hotplug() hook to request
> > another probe after some time.
>
> See also
>
> http://patchwork.freedesktop.org/patch/msgid/20180925071836.24711-1-jani.nikula@intel.com
Yes, the idea for a generic way to retry the hotplug detection came
after discussing with Ville and Jani about the similar issue with HDMI
sinks. The only difference wrt. Jani's patch above is that here we only
delay the detection if the connector probe thinks it's necessary. This
then also allows for a longer (up to 1sec) delay needed by the buggy
TypeC dongles.
>
> BR,
> Jani.
>
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_ddi.c | 12 +++---
> > drivers/gpu/drm/i915/intel_dp.c | 12 +++---
> > drivers/gpu/drm/i915/intel_drv.h | 17 +++++++--
> > drivers/gpu/drm/i915/intel_hotplug.c | 56 ++++++++++++++++++++++------
> > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++--
> > 7 files changed, 79 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 37175414ce89..8149be62a630 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4296,7 +4296,7 @@ static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
> > */
> > synchronize_irq(dev_priv->drm.irq);
> > flush_work(&dev_priv->hotplug.dig_port_work);
> > - flush_work(&dev_priv->hotplug.hotplug_work);
> > + flush_delayed_work(&dev_priv->hotplug.hotplug_work);
> >
> > seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
> > seq_printf(m, "Detected: %s\n",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bb0e75e43987..5703526bddab 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -156,7 +156,7 @@ enum hpd_pin {
> > #define HPD_STORM_DEFAULT_THRESHOLD 50
> >
> > struct i915_hotplug {
> > - struct work_struct hotplug_work;
> > + struct delayed_work hotplug_work;
> >
> > struct {
> > unsigned long last_jiffies;
> > @@ -168,6 +168,7 @@ struct i915_hotplug {
> > } state;
> > } stats[HPD_NUM_PINS];
> > u32 event_bits;
> > + u32 retry_bits;
> > struct delayed_work reenable_work;
> >
> > u32 long_port_mask;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ea83071a22c4..1676a87f18cb 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -4051,14 +4051,16 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder,
> > return modeset_pipe(&crtc->base, ctx);
> > }
> >
> > -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> > - struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_ddi_hotplug(struct intel_encoder *encoder,
> > + struct intel_connector *connector,
> > + bool irq_received)
> > {
> > struct drm_modeset_acquire_ctx ctx;
> > - bool changed;
> > + enum intel_hotplug_state state;
> > int ret;
> >
> > - changed = intel_encoder_hotplug(encoder, connector);
> > + state = intel_encoder_hotplug(encoder, connector, irq_received);
> >
> > drm_modeset_acquire_init(&ctx, 0);
> >
> > @@ -4080,7 +4082,7 @@ static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> > drm_modeset_acquire_fini(&ctx);
> > WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >
> > - return changed;
> > + return state;
> > }
> >
> > static struct intel_connector *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e1a051c0fbfe..0bb43eaa154c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4734,14 +4734,16 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
> > * retrain the link to get a picture. That's in case no
> > * userspace component reacted to intermittent HPD dip.
> > */
> > -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> > - struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_dp_hotplug(struct intel_encoder *encoder,
> > + struct intel_connector *connector,
> > + bool irq_received)
> > {
> > struct drm_modeset_acquire_ctx ctx;
> > - bool changed;
> > + enum intel_hotplug_state state;
> > int ret;
> >
> > - changed = intel_encoder_hotplug(encoder, connector);
> > + state = intel_encoder_hotplug(encoder, connector, irq_received);
> >
> > drm_modeset_acquire_init(&ctx, 0);
> >
> > @@ -4760,7 +4762,7 @@ static bool intel_dp_hotplug(struct intel_encoder *encoder,
> > drm_modeset_acquire_fini(&ctx);
> > WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >
> > - return changed;
> > + return state;
> > }
> >
> > static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 81ec73e4a083..de62af970dc1 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -226,14 +226,21 @@ struct intel_fbdev {
> > struct mutex hpd_lock;
> > };
> >
> > +enum intel_hotplug_state {
> > + INTEL_HOTPLUG_NOCHANGE,
> > + INTEL_HOTPLUG_CHANGED,
> > + INTEL_HOTPLUG_RETRY,
> > +};
> > +
> > struct intel_encoder {
> > struct drm_encoder base;
> >
> > enum intel_output_type type;
> > enum port port;
> > unsigned int cloneable;
> > - bool (*hotplug)(struct intel_encoder *encoder,
> > - struct intel_connector *connector);
> > + enum intel_hotplug_state (*hotplug)(struct intel_encoder *encoder,
> > + struct intel_connector *connector,
> > + bool irq_received);
> > enum intel_output_type (*compute_output_type)(struct intel_encoder *,
> > struct intel_crtc_state *,
> > struct drm_connector_state *);
> > @@ -2003,8 +2010,10 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
> > void intel_dvo_init(struct drm_i915_private *dev_priv);
> > /* intel_hotplug.c */
> > void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > - struct intel_connector *connector);
> > +enum intel_hotplug_state
> > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > + struct intel_connector *connector,
> > + bool irq_received);
> >
> > /* legacy fbdev emulation in intel_fbdev.c */
> > #ifdef CONFIG_DRM_FBDEV_EMULATION
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> > index b8937c788f03..3eaa06526f6b 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
> >
> > #define HPD_STORM_DETECT_PERIOD 1000
> > #define HPD_STORM_REENABLE_DELAY (2 * 60 * 1000)
> > +#define HPD_RETRY_DELAY 1000
> >
> > /**
> > * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
> > @@ -265,8 +266,10 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> > intel_runtime_pm_put(dev_priv, wakeref);
> > }
> >
> > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > - struct intel_connector *connector)
> > +enum intel_hotplug_state
> > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > + struct intel_connector *connector,
> > + bool irq_received)
> > {
> > struct drm_device *dev = connector->base.dev;
> > enum drm_connector_status old_status;
> > @@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > drm_helper_probe_detect(&connector->base, NULL, false);
> >
> > if (old_status == connector->base.status)
> > - return false;
> > + return INTEL_HOTPLUG_NOCHANGE;
> >
> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
> > connector->base.base.id,
> > @@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > drm_get_connector_status_name(old_status),
> > drm_get_connector_status_name(connector->base.status));
> >
> > - return true;
> > + return INTEL_HOTPLUG_CHANGED;
> > }
> >
> > static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
> > @@ -338,7 +341,7 @@ static void i915_digport_work_func(struct work_struct *work)
> > spin_lock_irq(&dev_priv->irq_lock);
> > dev_priv->hotplug.event_bits |= old_bits;
> > spin_unlock_irq(&dev_priv->irq_lock);
> > - schedule_work(&dev_priv->hotplug.hotplug_work);
> > + schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> > }
> > }
> >
> > @@ -348,14 +351,17 @@ static void i915_digport_work_func(struct work_struct *work)
> > static void i915_hotplug_work_func(struct work_struct *work)
> > {
> > struct drm_i915_private *dev_priv =
> > - container_of(work, struct drm_i915_private, hotplug.hotplug_work);
> > + container_of(work, struct drm_i915_private,
> > + hotplug.hotplug_work.work);
> > struct drm_device *dev = &dev_priv->drm;
> > struct intel_connector *intel_connector;
> > struct intel_encoder *intel_encoder;
> > struct drm_connector *connector;
> > struct drm_connector_list_iter conn_iter;
> > bool changed = false;
> > + u32 retry = 0;
> > u32 hpd_event_bits;
> > + u32 hpd_retry_bits;
> >
> > mutex_lock(&dev->mode_config.mutex);
> > DRM_DEBUG_KMS("running encoder hotplug functions\n");
> > @@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >
> > hpd_event_bits = dev_priv->hotplug.event_bits;
> > dev_priv->hotplug.event_bits = 0;
> > + hpd_retry_bits = dev_priv->hotplug.retry_bits;
> > + dev_priv->hotplug.retry_bits = 0;
> >
> > /* Enable polling for connectors which had HPD IRQ storms */
> > intel_hpd_irq_storm_switch_to_polling(dev_priv);
> > @@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >
> > drm_connector_list_iter_begin(dev, &conn_iter);
> > drm_for_each_connector_iter(connector, &conn_iter) {
> > + u32 hpd_bit;
> > +
> > intel_connector = to_intel_connector(connector);
> > if (!intel_connector->encoder)
> > continue;
> > intel_encoder = intel_connector->encoder;
> > - if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> > + hpd_bit = BIT(intel_encoder->hpd_pin);
> > + if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
> > DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> > connector->name, intel_encoder->hpd_pin);
> >
> > - changed |= intel_encoder->hotplug(intel_encoder,
> > - intel_connector);
> > + switch (intel_encoder->hotplug(intel_encoder,
> > + intel_connector,
> > + hpd_event_bits & hpd_bit)) {
> > + case INTEL_HOTPLUG_NOCHANGE:
> > + break;
> > + case INTEL_HOTPLUG_CHANGED:
> > + changed = true;
> > + break;
> > + case INTEL_HOTPLUG_RETRY:
> > + retry |= hpd_bit;
> > + break;
> > + }
> > }
> > }
> > drm_connector_list_iter_end(&conn_iter);
> > @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >
> > if (changed)
> > drm_kms_helper_hotplug_event(dev);
> > +
> > + if (retry) {
> > + spin_lock_irq(&dev_priv->irq_lock);
> > + dev_priv->hotplug.retry_bits |= retry;
> > + spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > + mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
> > + msecs_to_jiffies(HPD_RETRY_DELAY));
> > + }
> > }
> >
> >
> > @@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > if (queue_dig)
> > queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
> > if (queue_hp)
> > - schedule_work(&dev_priv->hotplug.hotplug_work);
> > + schedule_delayed_work(&dev_priv->hotplug.hotplug_work, 0);
> > }
> >
> > /**
> > @@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
> >
> > void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> > {
> > - INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> > + INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> > + i915_hotplug_work_func);
> > INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
> > INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
> > INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
> > @@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> > dev_priv->hotplug.long_port_mask = 0;
> > dev_priv->hotplug.short_port_mask = 0;
> > dev_priv->hotplug.event_bits = 0;
> > + dev_priv->hotplug.retry_bits = 0;
> >
> > spin_unlock_irq(&dev_priv->irq_lock);
> >
> > cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> > - cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> > + cancel_delayed_work_sync(&dev_priv->hotplug.hotplug_work);
> > cancel_work_sync(&dev_priv->hotplug.poll_init_work);
> > cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index e7b0884ba5a5..92793475b2b9 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1722,12 +1722,14 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
> > &intel_sdvo->hotplug_active, 2);
> > }
> >
> > -static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
> > - struct intel_connector *connector)
> > +static enum intel_hotplug_state
> > +intel_sdvo_hotplug(struct intel_encoder *encoder,
> > + struct intel_connector *connector,
> > + bool irq_received)
> > {
> > intel_sdvo_enable_hotplug(encoder);
> >
> > - return intel_encoder_hotplug(encoder, connector);
> > + return intel_encoder_hotplug(encoder, connector, irq_received);
> > }
> >
> > static bool
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed
2019-02-22 21:08 ` [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed José Roberto de Souza
@ 2019-02-26 14:08 ` Imre Deak
2019-02-26 15:34 ` Jani Nikula
2019-02-28 0:32 ` Souza, Jose
0 siblings, 2 replies; 17+ messages in thread
From: Imre Deak @ 2019-02-26 14:08 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: Jani Nikula, intel-gfx
On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza wrote:
> Unpowered type-c dongles can take some time to boot and be
> responsible, causing the probe to fail and sink never be detected
> without further actions from userspace.
>
> It was not a issue for older platforms because there was a hardware
> bridge between DDI/DP ports and type-c controller adding a implicit
> delay that hid this issue but ICL have type-c controllers integrated
> to the SOC bring this issue to users.
>
> So here if the probe failed when coming from a IRQ it returns
> INTEL_HOTPLUG_RETRY that will schedule another run of
> i915_hotplug_work_func() after 1 second what is time enough for
> those type-c dongles to boot.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1676a87f18cb..96bbcf5c9787 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
> struct intel_connector *connector,
> bool irq_received)
> {
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> struct drm_modeset_acquire_ctx ctx;
> enum intel_hotplug_state state;
> int ret;
> @@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
> drm_modeset_acquire_fini(&ctx);
> WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>
> + /*
> + * Unpowered type-c dongles can take some time to boot and be
> + * responsible, so here giving some type to those dongles to power up
> + * and then retrying the probe.
> + */
> + if (state == INTEL_HOTPLUG_NOCHANGE &&
> + connector->base.status != connector_status_connected &&
> + irq_received && intel_port_is_tc(dev_priv, encoder->port) &&
> + !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
Based on the investigation by Jani et al, we have the similar problem with
HDMI, only during disconnect. So I think we could generalize by retrying
any time there is no change (except for MST where the driver always keeps
the connector in a disconnected state), regardless of the type of the
sink, since a no-change is suspect in any case: why would the sink signal
(with a long pulse) if there is no change?
For HDMI we'd also need to handle this in intel_hdmi.c.
Then Ville suggested to add a Chamelium test for this to IGT, could you
Jose look into that as well? Both the connect and disconnect races could
be tested, both on HDMI and DP, by generating the HPD early/late wrt. to
AUX/DDC starting/stopping to return valid data. I don't know if
Chamelium can do this, you'd have to find that out first.
--Imre
> + state = INTEL_HOTPLUG_RETRY;
> +
> return state;
> }
>
> --
> 2.20.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed
2019-02-26 14:08 ` Imre Deak
@ 2019-02-26 15:34 ` Jani Nikula
2019-02-28 0:32 ` Souza, Jose
1 sibling, 0 replies; 17+ messages in thread
From: Jani Nikula @ 2019-02-26 15:34 UTC (permalink / raw)
To: imre.deak, José Roberto de Souza; +Cc: intel-gfx
On Tue, 26 Feb 2019, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza wrote:
>> Unpowered type-c dongles can take some time to boot and be
>> responsible, causing the probe to fail and sink never be detected
>> without further actions from userspace.
>>
>> It was not a issue for older platforms because there was a hardware
>> bridge between DDI/DP ports and type-c controller adding a implicit
>> delay that hid this issue but ICL have type-c controllers integrated
>> to the SOC bring this issue to users.
>>
>> So here if the probe failed when coming from a IRQ it returns
>> INTEL_HOTPLUG_RETRY that will schedule another run of
>> i915_hotplug_work_func() after 1 second what is time enough for
>> those type-c dongles to boot.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 1676a87f18cb..96bbcf5c9787 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>> struct intel_connector *connector,
>> bool irq_received)
>> {
>> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>> struct drm_modeset_acquire_ctx ctx;
>> enum intel_hotplug_state state;
>> int ret;
>> @@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>> drm_modeset_acquire_fini(&ctx);
>> WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>>
>> + /*
>> + * Unpowered type-c dongles can take some time to boot and be
>> + * responsible, so here giving some type to those dongles to power up
>> + * and then retrying the probe.
>> + */
>> + if (state == INTEL_HOTPLUG_NOCHANGE &&
>> + connector->base.status != connector_status_connected &&
>> + irq_received && intel_port_is_tc(dev_priv, encoder->port) &&
>> + !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
>
> Based on the investigation by Jani et al, we have the similar problem with
> HDMI, only during disconnect. So I think we could generalize by retrying
> any time there is no change (except for MST where the driver always keeps
> the connector in a disconnected state), regardless of the type of the
> sink, since a no-change is suspect in any case: why would the sink signal
> (with a long pulse) if there is no change?
>
> For HDMI we'd also need to handle this in intel_hdmi.c.
>
> Then Ville suggested to add a Chamelium test for this to IGT, could you
> Jose look into that as well? Both the connect and disconnect races could
> be tested, both on HDMI and DP, by generating the HPD early/late wrt. to
> AUX/DDC starting/stopping to return valid data. I don't know if
> Chamelium can do this, you'd have to find that out first.
Guang Bai also kept referencing a pathological customer test case which
we're not privy to.
BR,
Jani.
>
> --Imre
>
>> + state = INTEL_HOTPLUG_RETRY;
>> +
>> return state;
>> }
>>
>> --
>> 2.20.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed
2019-02-26 14:08 ` Imre Deak
2019-02-26 15:34 ` Jani Nikula
@ 2019-02-28 0:32 ` Souza, Jose
2019-02-28 16:06 ` Imre Deak
1 sibling, 1 reply; 17+ messages in thread
From: Souza, Jose @ 2019-02-28 0:32 UTC (permalink / raw)
To: Deak, Imre; +Cc: Nikula, Jani, intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 4359 bytes --]
On Tue, 2019-02-26 at 16:08 +0200, Imre Deak wrote:
> On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza
> wrote:
> > Unpowered type-c dongles can take some time to boot and be
> > responsible, causing the probe to fail and sink never be detected
> > without further actions from userspace.
> >
> > It was not a issue for older platforms because there was a hardware
> > bridge between DDI/DP ports and type-c controller adding a implicit
> > delay that hid this issue but ICL have type-c controllers
> > integrated
> > to the SOC bring this issue to users.
> >
> > So here if the probe failed when coming from a IRQ it returns
> > INTEL_HOTPLUG_RETRY that will schedule another run of
> > i915_hotplug_work_func() after 1 second what is time enough for
> > those type-c dongles to boot.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1676a87f18cb..96bbcf5c9787 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder
> > *encoder,
> > struct intel_connector *connector,
> > bool irq_received)
> > {
> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder-
> > >base);
> > struct drm_modeset_acquire_ctx ctx;
> > enum intel_hotplug_state state;
> > int ret;
> > @@ -4082,6 +4084,17 @@ intel_ddi_hotplug(struct intel_encoder
> > *encoder,
> > drm_modeset_acquire_fini(&ctx);
> > WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> >
> > + /*
> > + * Unpowered type-c dongles can take some time to boot and be
> > + * responsible, so here giving some type to those dongles to
s/type/time
So I don't loose it when doing the next version.
> > power up
> > + * and then retrying the probe.
> > + */
> > + if (state == INTEL_HOTPLUG_NOCHANGE &&
> > + connector->base.status != connector_status_connected &&
> > + irq_received && intel_port_is_tc(dev_priv, encoder->port)
> > &&
> > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
>
> Based on the investigation by Jani et al, we have the similar problem
> with
> HDMI, only during disconnect. So I think we could generalize by
> retrying
> any time there is no change (except for MST where the driver always
> keeps
> the connector in a disconnected state), regardless of the type of the
> sink, since a no-change is suspect in any case: why would the sink
> signal
> (with a long pulse) if there is no change?
The only case that I can think of is the cases were sink do a short
pulse but we don't handle it in the short pulse handler and schedule a
long pulse handler but it should not cause any drawback retry everytime
there is no change in the state and irq_received is set.
What comment could we add about the HDMI here? Something like this
would be fine?
"HDMI sinks are reported as connected by hardware right after unpluging
it, so here also giving some time for hardware to process the unplug so
driver can read it and do the unplug sequence and notify userspace
about the absence of the HDMI sink"
>
> For HDMI we'd also need to handle this in intel_hdmi.c.
It happens in older gens that don't have DDI? Otherwise just the update
above should take care of DP and HDMI DDI ports.
>
> Then Ville suggested to add a Chamelium test for this to IGT, could
> you
> Jose look into that as well? Both the connect and disconnect races
> could
> be tested, both on HDMI and DP, by generating the HPD early/late wrt.
> to
> AUX/DDC starting/stopping to return valid data. I don't know if
> Chamelium can do this, you'd have to find that out first.
I will try put my hands in a Chamelium board otherwise I will play with
trybot to add this tests.
>
> --Imre
>
> > + state = INTEL_HOTPLUG_RETRY;
> > +
> > return state;
> > }
> >
> > --
> > 2.20.1
> >
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed
2019-02-28 0:32 ` Souza, Jose
@ 2019-02-28 16:06 ` Imre Deak
2019-03-13 1:03 ` Souza, Jose
0 siblings, 1 reply; 17+ messages in thread
From: Imre Deak @ 2019-02-28 16:06 UTC (permalink / raw)
To: Souza, Jose; +Cc: Nikula, Jani, intel-gfx
On Thu, Feb 28, 2019 at 02:32:38AM +0200, Souza, Jose wrote:
> [...]
> > > + * and then retrying the probe.
> > > + */
> > > + if (state == INTEL_HOTPLUG_NOCHANGE &&
> > > + connector->base.status != connector_status_connected &&
> > > + irq_received && intel_port_is_tc(dev_priv, encoder->port)
> > > &&
> > > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
> >
> > Based on the investigation by Jani et al, we have the similar problem
> > with
> > HDMI, only during disconnect. So I think we could generalize by
> > retrying
> > any time there is no change (except for MST where the driver always
> > keeps
> > the connector in a disconnected state), regardless of the type of the
> > sink, since a no-change is suspect in any case: why would the sink
> > signal
> > (with a long pulse) if there is no change?
>
> The only case that I can think of is the cases were sink do a short
> pulse but we don't handle it in the short pulse handler and schedule a
> long pulse handler but it should not cause any drawback retry everytime
> there is no change in the state and irq_received is set.
>
> What comment could we add about the HDMI here? Something like this
> would be fine?
>
> "HDMI sinks are reported as connected by hardware right after unpluging
> it, so here also giving some time for hardware to process the unplug so
> driver can read it and do the unplug sequence and notify userspace
> about the absence of the HDMI sink"
It could be more specific, something like:
On many platforms the HDMI live state signal is known to be unreliable,
so we can't use it to detect if a sink is connected or not. Instead we
detect if it's connected based on whether we can read the EDID or not.
That in turn has a problem during disconnect, since the HPD interrupt
may be raised before the DDC lines get disconnected (due to how the
required length of DDC vs. HPD connector pins are specified) and so
we'll still be able to get a valid EDID. To solve this schedule another
detection cycle if this time around we didn't detect any change in the
sink's connection status.
>
> >
> > For HDMI we'd also need to handle this in intel_hdmi.c.
>
> It happens in older gens that don't have DDI? Otherwise just the update
> above should take care of DP and HDMI DDI ports.
I've been told it happens both on old and new HW.
> > Then Ville suggested to add a Chamelium test for this to IGT, could
> > you
> > Jose look into that as well? Both the connect and disconnect races
> > could
> > be tested, both on HDMI and DP, by generating the HPD early/late wrt.
> > to
> > AUX/DDC starting/stopping to return valid data. I don't know if
> > Chamelium can do this, you'd have to find that out first.
>
> I will try put my hands in a Chamelium board otherwise I will play with
> trybot to add this tests.
Okay, thanks.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed
2019-02-28 16:06 ` Imre Deak
@ 2019-03-13 1:03 ` Souza, Jose
0 siblings, 0 replies; 17+ messages in thread
From: Souza, Jose @ 2019-03-13 1:03 UTC (permalink / raw)
To: Deak, Imre; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 3313 bytes --]
On Thu, 2019-02-28 at 18:06 +0200, Imre Deak wrote:
> On Thu, Feb 28, 2019 at 02:32:38AM +0200, Souza, Jose wrote:
> > [...]
> > > > + * and then retrying the probe.
> > > > + */
> > > > + if (state == INTEL_HOTPLUG_NOCHANGE &&
> > > > + connector->base.status !=
> > > > connector_status_connected &&
> > > > + irq_received && intel_port_is_tc(dev_priv, encoder-
> > > > >port)
> > > > &&
> > > > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst)
> > >
> > > Based on the investigation by Jani et al, we have the similar
> > > problem
> > > with
> > > HDMI, only during disconnect. So I think we could generalize by
> > > retrying
> > > any time there is no change (except for MST where the driver
> > > always
> > > keeps
> > > the connector in a disconnected state), regardless of the type of
> > > the
> > > sink, since a no-change is suspect in any case: why would the
> > > sink
> > > signal
> > > (with a long pulse) if there is no change?
> >
> > The only case that I can think of is the cases were sink do a short
> > pulse but we don't handle it in the short pulse handler and
> > schedule a
> > long pulse handler but it should not cause any drawback retry
> > everytime
> > there is no change in the state and irq_received is set.
> >
> > What comment could we add about the HDMI here? Something like this
> > would be fine?
> >
> > "HDMI sinks are reported as connected by hardware right after
> > unpluging
> > it, so here also giving some time for hardware to process the
> > unplug so
> > driver can read it and do the unplug sequence and notify userspace
> > about the absence of the HDMI sink"
>
> It could be more specific, something like:
>
> On many platforms the HDMI live state signal is known to be
> unreliable,
> so we can't use it to detect if a sink is connected or not. Instead
> we
> detect if it's connected based on whether we can read the EDID or
> not.
> That in turn has a problem during disconnect, since the HPD interrupt
> may be raised before the DDC lines get disconnected (due to how the
> required length of DDC vs. HPD connector pins are specified) and so
> we'll still be able to get a valid EDID. To solve this schedule
> another
> detection cycle if this time around we didn't detect any change in
> the
> sink's connection status.
>
> > > For HDMI we'd also need to handle this in intel_hdmi.c.
> >
> > It happens in older gens that don't have DDI? Otherwise just the
> > update
> > above should take care of DP and HDMI DDI ports.
>
> I've been told it happens both on old and new HW.
>
> > > Then Ville suggested to add a Chamelium test for this to IGT,
> > > could
> > > you
> > > Jose look into that as well? Both the connect and disconnect
> > > races
> > > could
> > > be tested, both on HDMI and DP, by generating the HPD early/late
> > > wrt.
> > > to
> > > AUX/DDC starting/stopping to return valid data. I don't know if
> > > Chamelium can do this, you'd have to find that out first.
> >
> > I will try put my hands in a Chamelium board otherwise I will play
> > with
> > trybot to add this tests.
>
> Okay, thanks.
Just sent the new kernel patches and the IGT test for the type-c bug,
still working in the HDMI test.
>
> --Imre
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-03-15 22:22 ` Souza, Jose
@ 2019-03-16 13:16 ` Imre Deak
0 siblings, 0 replies; 17+ messages in thread
From: Imre Deak @ 2019-03-16 13:16 UTC (permalink / raw)
To: Souza, Jose; +Cc: Nikula, Jani, intel-gfx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-03-15 1:39 ` Imre Deak
@ 2019-03-15 22:22 ` Souza, Jose
2019-03-16 13:16 ` Imre Deak
0 siblings, 1 reply; 17+ messages in thread
From: Souza, Jose @ 2019-03-15 22:22 UTC (permalink / raw)
To: Deak, Imre; +Cc: Nikula, Jani, intel-gfx
[-- 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-03-15 0:25 ` Souza, Jose
@ 2019-03-15 1:39 ` Imre Deak
2019-03-15 22:22 ` Souza, Jose
0 siblings, 1 reply; 17+ messages in thread
From: Imre Deak @ 2019-03-15 1:39 UTC (permalink / raw)
To: Souza, Jose; +Cc: Nikula, Jani, intel-gfx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-03-14 16:09 ` Imre Deak
@ 2019-03-15 0:25 ` Souza, Jose
2019-03-15 1:39 ` Imre Deak
0 siblings, 1 reply; 17+ messages in thread
From: Souza, Jose @ 2019-03-15 0:25 UTC (permalink / raw)
To: Deak, Imre; +Cc: Nikula, Jani, intel-gfx
[-- 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Add support for retrying hotplug
2019-03-13 0:58 José Roberto de Souza
@ 2019-03-14 16:09 ` Imre Deak
2019-03-15 0:25 ` Souza, Jose
0 siblings, 1 reply; 17+ messages in thread
From: Imre Deak @ 2019-03-14 16:09 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: Jani Nikula, intel-gfx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] drm/i915: Add support for retrying hotplug
@ 2019-03-13 0:58 José Roberto de Souza
2019-03-14 16:09 ` Imre Deak
0 siblings, 1 reply; 17+ messages in thread
From: José Roberto de Souza @ 2019-03-13 0:58 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-03-16 13:16 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22 21:08 [PATCH 1/2] drm/i915: Add support for retrying hotplug José Roberto de Souza
2019-02-22 21:08 ` [PATCH 2/2] drm/i915/icl: Probe again type-c connectors that failed José Roberto de Souza
2019-02-26 14:08 ` Imre Deak
2019-02-26 15:34 ` Jani Nikula
2019-02-28 0:32 ` Souza, Jose
2019-02-28 16:06 ` Imre Deak
2019-03-13 1:03 ` Souza, Jose
2019-02-22 21:23 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Add support for retrying hotplug Patchwork
2019-02-22 21:42 ` ✗ Fi.CI.BAT: failure " Patchwork
2019-02-25 11:02 ` [PATCH 1/2] " Jani Nikula
2019-02-25 11:22 ` Imre Deak
2019-03-13 0:58 José Roberto de Souza
2019-03-14 16:09 ` Imre Deak
2019-03-15 0:25 ` Souza, Jose
2019-03-15 1:39 ` Imre Deak
2019-03-15 22:22 ` Souza, Jose
2019-03-16 13:16 ` Imre Deak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.