drm/i915: Synchronize connectors states when switching from poll to irq
Subsystems (2)
PATCH 1/1 drm/i915: Synchronize connectors states when switching from poll to irq
After detecting an IRQ storm, hotplug detection will switch from irq-based detection to poll-based detection. After a short delay or when resetting storm detection from debugfs, detection will switch back to being irq-based.
However, it may occur that polling does not have enough time to detect the current connector state when that second switch takes place. Thus, this sets the appropriate hotplug event bits for the concerned connectors and schedules the hotplug work, that will ensure the connectors states are in sync when switching back to irq.
Without this, no irq will be triggered and the hpd change will be lost.
--- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_hotplug.c +7 -1
@@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)struct intel_connector *intel_connector =to_intel_connector (connector);if (intel_connector->encoder->hpd_pin == i) { -if (connector->polled != intel_connector->polled) +if (connector->polled != intel_connector->polled) {DRM_DEBUG_DRIVER ("Reenabling HPD on connector %s\n" , connector->name); + + dev_priv->hotplug.event_bits |= (1 << i); + } + connector->polled = intel_connector->polled;if (!connector->polled) connector->polled =DRM_CONNECTOR_POLL_HPD ;
@@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) dev_priv->display.hpd_irq_setup (dev_priv);spin_unlock_irq (&dev_priv->irq_lock); +schedule_work (&dev_priv->hotplug.hotplug_work); +intel_runtime_pm_put (dev_priv); }
Signature · 6 lines
2.13.1 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Patchwork
CI✓ Fi.CI.BAT: success for drm/i915: Synchronize connectors states when switching from poll to irq
== Series Details ==
== Series Details ==
Series: drm/i915: Synchronize connectors states when switching from poll to irq
URL : https://patchwork.freedesktop.org/series/26361/
State : success
== Summary ==
Series 26361v1 drm/i915: Synchronize connectors states when switching from poll to irq
https://patchwork.freedesktop.org/api/1.0/series/26361/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-kbl-7560u) fdo#100125 +1
Test kms_flip:
Subgroup basic-flip-vs-wf_vblank:
fail -> PASS (fi-blb-e6850) fdo#99093
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
dmesg-warn -> PASS (fi-pnv-d510) fdo#101597 +1fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#99093 https://bugs.freedesktop.org/show_bug.cgi?id=99093
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:441s fi-bdw-gvtdvm total:279 pass:257 dwarn:8 dfail:0 fail:0 skip:14 time:432s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:360s fi-bsw-n3050 total:279 pass:242 dwarn:1 dfail:0 fail:0 skip:36 time:526s fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:516s fi-byt-j1900 total:279 pass:253 dwarn:2 dfail:0 fail:0 skip:24 time:490s fi-byt-n2820 total:279 pass:249 dwarn:2 dfail:0 fail:0 skip:28 time:482s fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:588s fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:434s fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:416s fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:426s fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:499s fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:474s fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:467s fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:575s fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:580s fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:566s fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:464s fi-skl-6700hq total:279 pass:223 dwarn:1 dfail:0 fail:30 skip:24 time:340s fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:473s fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:482s fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:442s fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:538s fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:406s
21d74e215ad650f0e8e30de609bd65601f0aa11d drm-tip: 2017y-06m-26d-09h-12m-14s UTC integration manifest 304f83f drm/i915: Synchronize connectors states when switching from poll to irq
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5042/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 5 lines · Paul Kocialkowski
After detecting an IRQ storm, hotplug detection will switch from irq-based detection to poll-based detection. After a short delay or when resetting storm detection from debugfs, detection will switch back to being irq-based.
Quoted 8 lines
However, it may occur that polling does not have enough time to detect the current connector state when that second switch takes place. Thus, this sets the appropriate hotplug event bits for the concerned connectors and schedules the hotplug work, that will ensure the connectors states are in sync when switching back to irq.
Without this, no irq will be triggered and the hpd change will be lost.
Does anyone have feedback to provide on this?
It looks like it should be a no-brainer.
Cheers,
Paul
Quoted 44 lines
Signed-off-by Paul Kocialkowski <[email protected]>--- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_hotplug.c +1
-1@@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) struct intel_connector *intel_connector =to_intel_connector (connector);if (intel_connector->encoder->hpd_pin == i) { -if (connector->polled != intel_connector->polled) +if (connector->polled != intel_connector->polled) { DRM_DEBUG_DRIVER("ReenablingHPD on connector %s\n",
connector-Quoted 1 line
name);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ + dev_priv->hotplug.event_bits|= (1 << i);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ } + connector->polled = intel_connector-Quoted 1 line
polled;
drivers/gpu/drm/i915/intel_hotplug.c +0
-0if (!connector->polled) connector->polled =
DRM_CONNECTOR_POLL_HPD;drivers/gpu/drm/i915/intel_hotplug.c +2
-0@@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) dev_priv->display.hpd_irq_setup(dev_priv);spin_unlock_irq (&dev_priv->irq_lock); +schedule_work (&dev_priv->hotplug.hotplug_work); +intel_runtime_pm_put (dev_priv); }
Signature · 6 lines
Paul Kocialkowski <[email protected]> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 37 lines · Paul Kocialkowski
Quoted 5 lines · Paul Kocialkowski
After detecting an IRQ storm, hotplug detection will switch from irq-based detection to poll-based detection. After a short delay or when resetting storm detection from debugfs, detection will switch back to being irq-based.
Quoted 8 lines
However, it may occur that polling does not have enough time to detect the current connector state when that second switch takes place. Thus, this sets the appropriate hotplug event bits for the concerned connectors and schedules the hotplug work, that will ensure the connectors states are in sync when switching back to irq.
Without this, no irq will be triggered and the hpd change will be lost.
Does anyone have feedback to provide on this?
It looks like it should be a no-brainer.Cheers,
Paul
Quoted 15 lines
Signed-off-by Paul Kocialkowski <[email protected]>--- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_hotplug.c +0
-0@@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) struct intel_connector *intel_connector =to_intel_connector (connector);
Quoted 1 line
drivers/gpu/drm/i915/intel_hotplug.c static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) · line 220
Quoted 1 line
drivers/gpu/drm/i915/intel_hotplug.c +0
-0if (intel_connector->encoder->hpd_pin == i) {
So if this hpd pin in intel_connector->encoder is set then that means it got the hpd but because connector->polled is != intel_connector->polled polling didnt detect that connector.
Is that what you are trying to do here?
Manasi
Quoted 35 lines
Quoted 28 lines
drivers/gpu/drm/i915/intel_hotplug.c +1
-1-if (connector->polled != intel_connector->polled) +if (connector->polled != intel_connector->polled) { DRM_DEBUG_DRIVER("ReenablingHPD on connector %s\n",
connector-Quoted 1 line
name);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ + dev_priv->hotplug.event_bits|= (1 << i);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ } + connector->polled = intel_connector-Quoted 1 line
polled;
drivers/gpu/drm/i915/intel_hotplug.c +0
-0if (!connector->polled) connector->polled =
DRM_CONNECTOR_POLL_HPD;drivers/gpu/drm/i915/intel_hotplug.c +2
-0@@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) dev_priv->display.hpd_irq_setup(dev_priv);spin_unlock_irq (&dev_priv->irq_lock); +schedule_work (&dev_priv->hotplug.hotplug_work); +intel_runtime_pm_put (dev_priv); }Signature · 6 lines
Paul Kocialkowski <[email protected]> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 50 lines · Manasi Navare
Quoted 41 lines · Paul Kocialkowski
Quoted 14 lines · Paul Kocialkowski
After detecting an IRQ storm, hotplug detection will switch from irq-based detection to poll-based detection. After a short delay or
when resetting storm detection from debugfs, detection will switch back to being irq-based.However, it may occur that polling does not have enough time to detect
the current connector state when that second switch takes place. Thus,
this sets the appropriate hotplug event bits for the concerned connectors and schedules the hotplug work, that will ensure the connectors states are in sync when switching back to irq.Quoted 2 lines
Without this, no irq will be triggered and the hpd change will be lost.
Does anyone have feedback to provide on this?
It looks like it should be a no-brainer.Cheers,
Paul
Quoted 16 lines
Signed-off-by Paul Kocialkowski <[email protected]m>
--- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_hotplug.c +0
-0@@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) struct intel_connector *intel_connector =to_intel_connector (connector);Quoted 2 lines
drivers/gpu/drm/i915/intel_hotplug.c static void · line 223
Quoted 2 lines
drivers/gpu/drm/i915/intel_hotplug.c +0
-0if (intel_connector->encoder->hpd_pin == i) {So if this hpd pin in
intel_connector->encoderis set then that means it got the hpd but becauseconnector->polledis !=intel_connector->polled
polling didnt detect that connector.
Quoted 1 line
Is that what you are trying to do here?
The first test is simply a way to match the intel_connector to the drm connector. The second one indicates whether we are using polling or irq mode. If they don't match (given the test earlier in the code that only selects hpd pins that use irq mode), it means that we are switching from
polling mode to irq mode.
When that happens, it is necessary to assume that the connector state may have changed (in the time window between the last poll and enabling irq), so the hotplug work is scheduled. It will check whether a hpd state change did happen or not and issue a uevent if that is the case.
Quoted 44 lines
Quoted 44 lines
Quoted 35 lines
drivers/gpu/drm/i915/intel_hotplug.c +1
-1-if (connector->polled != intel_connector->polled) +if (connector->polled != intel_connector->polled) { DRM_DEBUG_DRIVER("Reenabling
HPD on connector %s\n",
connecto
r-Quoted 1 line
name);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ + dev_priv-Quoted 2 lines
hotplug.event_bits
= (1 << i);drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ } + connector->polled =intel_connector-
Quoted 1 line
polled;
drivers/gpu/drm/i915/intel_hotplug.c +0
-0if (!connector->polled) connector->polled =
DRM_CONNECTOR_POLL_HPD;drivers/gpu/drm/i915/intel_hotplug.c +2
-0@@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) dev_priv->display.hpd_irq_setup(dev_priv);spin_unlock_irq (&dev_priv->irq_lock); +schedule_work (&dev_priv->hotplug.hotplug_work); +intel_runtime_pm_put (dev_priv); }Signature · 7 lines
Paul Kocialkowski <[email protected]> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 6 lines
Paul Kocialkowski <[email protected]> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · Paul Kocialkowski
After detecting an IRQ storm, hotplug detection will switch from irq-based detection to poll-based detection. After a short delay or when resetting storm detection from debugfs, detection will switch back to being irq-based.
However, it may occur that polling does not have enough time to detect the current connector state when that second switch takes place.
Some questions so that I understand this better. How short would this have to be for detect to not complete? Is there a way I can easily test this scenario?
Quoted 5 lines
Thus,
this sets the appropriate hotplug event bits for the concerned connectors and schedules the hotplug work, that will ensure the connectors states are in sync when switching back to irq.
Not sure I understand this correctly, looks like you are setting the event_bits even if there was no irq during the polling interval. Is that right?
Quoted 27 lines
Without this, no irq will be triggered and the hpd change will be lost.
Signed-off-by Paul Kocialkowski <[email protected]>--- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_hotplug.c +5
-1@@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)struct intel_connector *intel_connector =to_intel_connector (connector);if (intel_connector->encoder->hpd_pin == i) { -if (connector->polled != intel_connector->polled) +if (connector->polled != intel_connector->polled) {DRM_DEBUG_DRIVER ("Reenabling HPD on connector %s\n" , connector->name); + + dev_priv->hotplug.event_bits |= (1 << i); + } + connector->polled = intel_connector->polled;if (!connector->polled) connector->polled =DRM_CONNECTOR_POLL_HPD ;
drivers/gpu/drm/i915/intel_hotplug.c static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) · line 239
Quoted 4 lines
drivers/gpu/drm/i915/intel_hotplug.c +1
-0@@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) dev_priv->display.hpd_irq_setup (dev_priv);spin_unlock_irq (&dev_priv->irq_lock); +schedule_work (&dev_priv->hotplug.hotplug_work);
How does this work with DP connectors? From what I understand, the event_bits are set for DP based on the return value from intel_dp_hpd_pulse(). But, doesn't this patch set the bits unconditionally?
Quoted 4 lines
drivers/gpu/drm/i915/intel_hotplug.c +1
-0+intel_runtime_pm_put (dev_priv); }
Quoted 6 lines · Dhinakaran
Quoted 5 lines · Paul Kocialkowski
After detecting an IRQ storm, hotplug detection will switch from irq-based detection to poll-based detection. After a short delay or when resetting storm detection from debugfs, detection will switch back to being irq-based.
Quoted 7 lines
Quoted 3 lines
However, it may occur that polling does not have enough time to detect
the current connector state when that second switch takes place.Some questions so that I understand this better. How short would this have to be for detect to not complete? Is there a way I can easily test this scenario?
Well, it doesn't really matter, the point is that it may happen that the connector's hpd line changes in the time window between the last poll (that happens every 100ms) and the moment the irq is enabled back. This time window, however long it may be, always exists and it may happen that this is exactly when the hpd event occurs.
It's possible to test this by triggering an hpd storm, then triggering a regular hpd toggle and directly disabling polling mode via debugfs. I was able to do this with a chamelium and did see the problem take place.
Quoted 8 lines
Quoted 4 lines
Thus,
this sets the appropriate hotplug event bits for the concerned connectors and schedules the hotplug work, that will ensure the connectors states are in sync when switching back to irq.Not sure I understand this correctly, looks like you are setting the
event_bitseven if there was no irq during the polling interval. Is that right?
Yes, you are perfectly right. it is necessary to do this because the event will not be detected either by polling (happening too late) nor by the irq (happening too early). So we must trigger the hotplug work to make it check whether the connectors states changed.
Quoted 43 lines
Quoted 43 lines
Without this, no irq will be triggered and the hpd change will be lost.
Signed-off-by Paul Kocialkowski <[email protected]>--- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_hotplug.c +1
-1@@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) struct intel_connector *intel_connector =to_intel_connector (connector);if (intel_connector->encoder->hpd_pin == i) { -if (connector->polled != intel_connector->polled) +if (connector->polled != intel_connector->polled) { DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
connector-Quoted 1 line
name);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ + dev_priv-Quoted 1 line
hotplug.event_bits |= (1 << i);
drivers/gpu/drm/i915/intel_hotplug.c +2
-0+ } + connector->polled =
intel_connector->polled;drivers/gpu/drm/i915/intel_hotplug.c +0
-0if (!connector->polled) connector->polled =
DRM_CONNECTOR_POLL_HPD;drivers/gpu/drm/i915/intel_hotplug.c +0
-0@@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) dev_priv->display.hpd_irq_setup(dev_priv);spin_unlock_irq (&dev_priv->irq_lock);
Quoted 6 lines
drivers/gpu/drm/i915/intel_hotplug.c static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) · line 239
Quoted 1 line
drivers/gpu/drm/i915/intel_hotplug.c +1
-0+schedule_work (&dev_priv->hotplug.hotplug_work);How does this work with DP connectors? From what I understand, the
event_bitsare set for DP based on the return value fromintel_dp_hpd_pulse(). But, doesn't this patch set the bits unconditionally?
It works the same as other connectors, nothing specific there. The hotplug work will read the connector's state and issue a uevent if its value has changed.
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/i915/intel_hotplug.c +1
-0+intel_runtime_pm_put (dev_priv); }
Signature · 6 lines
Paul Kocialkowski <[email protected]> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx