From: José Roberto de Souza <hidden> Date: 2019-03-13 00:59:53
It is know that some unpowered type-c dongles can take some time to
boot and be responsible in the DDC/aux transaction lines so a
workaround was implemented in kernel(drm/i915: Enable hotplug retry)
to fix it but is possible that this could happen to other DP sinks.
So this test will try to simulate the sceneario described above, it
will disable the DDC lines and plug the connector, the hotplug should
fail and then enabling the DDC lines kernel should report the
connector as connected.
The workaround will reprobe connector after 1 second after kernel
gives up on the first try to probe the connector, so that is why a
smaller timeout to detect hotplug was needed.
Cc: Imre Deak <redacted>
Signed-off-by: José Roberto de Souza <redacted>
---
tests/kms_chamelium.c | 70 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
@@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)igt_hpd_storm_reset(data->drm_fd);}+/*+*TestkernelworkaroundforsinksthattakessometimetohavetheDDC/aux+*channelresponsiveafterthehotplug+*/+staticvoid+test_late_aux_wa(data_t*data,structchamelium_port*port)+{+structudev_monitor*mon=igt_watch_hotplug();+drmModeConnectionstatus;++/* Reset will unplug all connectors */+reset_state(data,NULL);++/* Check if it device can act on hotplugs fast enough for this test */+igt_flush_hotplugs(mon);+chamelium_plug(data->chamelium,port);+igt_assert(igt_hotplug_detected(mon,FAST_HOTPLUG_SEC_TIMEOUT));+status=connector_status_get(data,port);+igt_require(status==DRM_MODE_CONNECTED);++igt_flush_hotplugs(mon);+chamelium_unplug(data->chamelium,port);+igt_assert(igt_hotplug_detected(mon,FAST_HOTPLUG_SEC_TIMEOUT));+status=connector_status_get(data,port);+igt_require(status==DRM_MODE_DISCONNECTED);++/* It is fast enough, lets disable the DDC lines and plug again */+igt_flush_hotplugs(mon);+chamelium_port_set_ddc_state(data->chamelium,port,false);+chamelium_plug(data->chamelium,port);+igt_assert(!chamelium_port_get_ddc_state(data->chamelium,port));++/*+*Givesometimetokerneltrytoprocesshotplugbutitshouldfail+*/+igt_hotplug_detected(mon,FAST_HOTPLUG_SEC_TIMEOUT);+status=connector_status_get(data,port);+igt_assert(status==DRM_MODE_DISCONNECTED);++/*+*EnabletheDDClineandthekernelworkaroundshouldreprobeand+*reportasconnected+*/+chamelium_port_set_ddc_state(data->chamelium,port,true);+igt_assert(chamelium_port_get_ddc_state(data->chamelium,port));+igt_assert(igt_hotplug_detected(mon,FAST_HOTPLUG_SEC_TIMEOUT));+status=connector_status_get(data,port);+igt_assert(status==DRM_MODE_CONNECTED);+}+staticvoidtest_edid_read(data_t*data,structchamelium_port*port,intedid_id,constunsignedchar*edid)
From: Imre Deak <hidden> Date: 2019-03-14 16:59:41
Hi Jose,
On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza wrote:
quoted hunk
It is know that some unpowered type-c dongles can take some time to
boot and be responsible in the DDC/aux transaction lines so a
workaround was implemented in kernel(drm/i915: Enable hotplug retry)
to fix it but is possible that this could happen to other DP sinks.
So this test will try to simulate the sceneario described above, it
will disable the DDC lines and plug the connector, the hotplug should
fail and then enabling the DDC lines kernel should report the
connector as connected.
The workaround will reprobe connector after 1 second after kernel
gives up on the first try to probe the connector, so that is why a
smaller timeout to detect hotplug was needed.
Cc: Imre Deak <redacted>
Signed-off-by: José Roberto de Souza <redacted>
---
tests/kms_chamelium.c | 70 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
@@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)igt_hpd_storm_reset(data->drm_fd);}+/*+*TestkernelworkaroundforsinksthattakessometimetohavetheDDC/aux+*channelresponsiveafterthehotplug+*/+staticvoid+test_late_aux_wa(data_t*data,structchamelium_port*port)+{+structudev_monitor*mon=igt_watch_hotplug();+drmModeConnectionstatus;++/* Reset will unplug all connectors */+reset_state(data,NULL);++/* Check if it device can act on hotplugs fast enough for this test */+igt_flush_hotplugs(mon);+chamelium_plug(data->chamelium,port);+igt_assert(igt_hotplug_detected(mon,FAST_HOTPLUG_SEC_TIMEOUT));+status=connector_status_get(data,port);+igt_require(status==DRM_MODE_CONNECTED);++igt_flush_hotplugs(mon);+chamelium_unplug(data->chamelium,port);+igt_assert(igt_hotplug_detected(mon,FAST_HOTPLUG_SEC_TIMEOUT));+status=connector_status_get(data,port);+igt_require(status==DRM_MODE_DISCONNECTED);
Do you know on which platforms the hotplug processing is that slow and
what could be the reason?
quoted hunk
++ /* It is fast enough, lets disable the DDC lines and plug again */+ igt_flush_hotplugs(mon);+ chamelium_port_set_ddc_state(data->chamelium, port, false);+ chamelium_plug(data->chamelium, port);+ igt_assert(!chamelium_port_get_ddc_state(data->chamelium, port));++ /*+ * Give some time to kernel try to process hotplug but it should fail+ */+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);+ status = connector_status_get(data, port);+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here? After the
previous hotplug event above where the state was disconnected already there
shouldn't have been any change to the state, hence there shouldn't be
any event sent by the driver.
quoted hunk
++ /*+ * Enable the DDC line and the kernel workaround should reprobe and+ * report as connected+ */+ chamelium_port_set_ddc_state(data->chamelium, port, true);+ igt_assert(chamelium_port_get_ddc_state(data->chamelium, port));+ igt_assert(igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT));+ status = connector_status_get(data, port);+ igt_assert(status == DRM_MODE_CONNECTED);+}+ static void test_edid_read(data_t *data, struct chamelium_port *port, int edid_id, const unsigned char *edid)
From: Souza, Jose <hidden> Date: 2019-03-14 17:59:36
On Thu, 2019-03-14 at 18:59 +0200, Imre Deak wrote:
Hi Jose,
On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
wrote:
quoted
It is know that some unpowered type-c dongles can take some time to
boot and be responsible in the DDC/aux transaction lines so a
workaround was implemented in kernel(drm/i915: Enable hotplug
retry)
to fix it but is possible that this could happen to other DP sinks.
So this test will try to simulate the sceneario described above, it
will disable the DDC lines and plug the connector, the hotplug
should
fail and then enabling the DDC lines kernel should report the
connector as connected.
The workaround will reprobe connector after 1 second after kernel
gives up on the first try to probe the connector, so that is why a
smaller timeout to detect hotplug was needed.
Cc: Imre Deak <redacted>
Signed-off-by: José Roberto de Souza <redacted>
---
tests/kms_chamelium.c | 70
+++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
chamelium_port *port, int toggle_count)
igt_hpd_storm_reset(data->drm_fd);
}
+/*
+ * Test kernel workaround for sinks that takes some time to have
the DDC/aux
+ * channel responsive after the hotplug
+ */
+static void
+test_late_aux_wa(data_t *data, struct chamelium_port *port)
+{
+ struct udev_monitor *mon = igt_watch_hotplug();
+ drmModeConnection status;
+
+ /* Reset will unplug all connectors */
+ reset_state(data, NULL);
+
+ /* Check if it device can act on hotplugs fast enough for this
test */
+ igt_flush_hotplugs(mon);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_CONNECTED);
+
+ igt_flush_hotplugs(mon);
+ chamelium_unplug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_DISCONNECTED);
Do you know on which platforms the hotplug processing is that slow
and
what could be the reason?
I have tested on ICL and WHL and both don't need more than 1 second,
the 20s timeout was set by the first patch(c99f8b7a3) adding Chamelium
tests but there is not other information about why 20s(in the first
versions of the patch it was 30s).
I can send another patch reducing this value to 1s and hopefully if it
do not causes regressions we merge it.
quoted
+
+ /* It is fast enough, lets disable the DDC lines and plug again
*/
+ igt_flush_hotplugs(mon);
+ chamelium_port_set_ddc_state(data->chamelium, port, false);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(!chamelium_port_get_ddc_state(data->chamelium,
port));
+
+ /*
+ * Give some time to kernel try to process hotplug but it
should fail
+ */
+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here? After the
previous hotplug event above where the state was disconnected already
there
shouldn't have been any change to the state, hence there shouldn't be
any event sent by the driver.
It is not signaled, that I why there is not igt_assert() on this
igt_hotplug_detected() but call it will poll/sleep for the time we
want.
quoted
+
+ /*
+ * Enable the DDC line and the kernel workaround should reprobe
and
+ * report as connected
+ */
+ chamelium_port_set_ddc_state(data->chamelium, port, true);
+ igt_assert(chamelium_port_get_ddc_state(data->chamelium,
port));
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_CONNECTED);
+}
+
static void
test_edid_read(data_t *data, struct chamelium_port *port,
int edid_id, const unsigned char *edid)
From: Imre Deak <hidden> Date: 2019-03-14 19:57:53
On Thu, Mar 14, 2019 at 07:59:34PM +0200, Souza, Jose wrote:
On Thu, 2019-03-14 at 18:59 +0200, Imre Deak wrote:
quoted
Hi Jose,
On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
wrote:
quoted
It is know that some unpowered type-c dongles can take some time to
boot and be responsible in the DDC/aux transaction lines so a
workaround was implemented in kernel(drm/i915: Enable hotplug
retry)
to fix it but is possible that this could happen to other DP sinks.
So this test will try to simulate the sceneario described above, it
will disable the DDC lines and plug the connector, the hotplug
should
fail and then enabling the DDC lines kernel should report the
connector as connected.
The workaround will reprobe connector after 1 second after kernel
gives up on the first try to probe the connector, so that is why a
smaller timeout to detect hotplug was needed.
Cc: Imre Deak <redacted>
Signed-off-by: José Roberto de Souza <redacted>
---
tests/kms_chamelium.c | 70
+++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
chamelium_port *port, int toggle_count)
igt_hpd_storm_reset(data->drm_fd);
}
+/*
+ * Test kernel workaround for sinks that takes some time to have the DDC/aux
+ * channel responsive after the hotplug
+ */
+static void
+test_late_aux_wa(data_t *data, struct chamelium_port *port)
+{
+ struct udev_monitor *mon = igt_watch_hotplug();
+ drmModeConnection status;
+
+ /* Reset will unplug all connectors */
+ reset_state(data, NULL);
+
+ /* Check if it device can act on hotplugs fast enough for this test */
+ igt_flush_hotplugs(mon);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_CONNECTED);
+
+ igt_flush_hotplugs(mon);
+ chamelium_unplug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_DISCONNECTED);
Do you know on which platforms the hotplug processing is that slow
and what could be the reason?
I have tested on ICL and WHL and both don't need more than 1 second,
the 20s timeout was set by the first patch(c99f8b7a3) adding Chamelium
tests but there is not other information about why 20s(in the first
versions of the patch it was 30s).
Ok, sounds to me that HOTPLUG_TIMEOUT was added only to check if hotplug
detection works at all (all relevant places using it have an
igt_assert() on it).
Still not sure if we need the above two checks, since I think the
assumptions/checks later in the function should hold anyway. Yes, we
may not exercise the hotplug retry path in the driver, but I don't think
there is a good way to ensure that anyway as I wrote below.
I can send another patch reducing this value to 1s and hopefully if it
do not causes regressions we merge it.
quoted
quoted
+
+ /* It is fast enough, lets disable the DDC lines and plug again
*/
+ igt_flush_hotplugs(mon);
+ chamelium_port_set_ddc_state(data->chamelium, port, false);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(!chamelium_port_get_ddc_state(data->chamelium,
port));
+
+ /*
+ * Give some time to kernel try to process hotplug but it
should fail
+ */
+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here? After the
previous hotplug event above where the state was disconnected already
there
shouldn't have been any change to the state, hence there shouldn't be
any event sent by the driver.
It is not signaled, that I why there is not igt_assert() on this
igt_hotplug_detected() but call it will poll/sleep for the time we
want.
But then I don't see how it will work. The sequence is:
<connector is in disconnected state, corresponding event delivered>
1. disable DDC
2. generate a plug event
3. wait for the plug event delivery with 1 sec timeout
4. re-enable DDC
5. wait for the plug event delivery (that should be triggered by the new
retry logic in the driver)
Since after 2. DDC is disabled the driver hotplug handler will conclude
that the connector is still disconnected and hence doesn't generate any
hotplug event. B/c of this 3. will time out after 1 sec.
So in 4. we'll re-enable DDC only after 1 sec after the plug event
(interrupt) generated in 2. Since the retry in the driver happens after
1 sec from the plug interrupt as well the retry processing could easily
race with the DDC re-enabling in 4. and thus the detection could fail.
Since I don't see a good way to ensure that we re-enable DDC after the
first detection cycle ran (but not too late missing the retry cycle) I
would rather suggest a simple wait at 3., let's say 500msec. With that
things should always work. We may not always exercise the driver's retry
path if there was a long scheduling delay, but that's unlikely.
However a scheduling delay after 2. and before 4. could cause a
detection failure. To avoid that I'd also check the elapsed time
starting from right before 2. until right after 4. and run the sequence
again if the elapsed time was too close to 1sec (and hence detection
possibly failed because of the race described above).
quoted
quoted
+
+ /*
+ * Enable the DDC line and the kernel workaround should reprobe
and
+ * report as connected
+ */
+ chamelium_port_set_ddc_state(data->chamelium, port, true);
+ igt_assert(chamelium_port_get_ddc_state(data->chamelium,
port));
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_CONNECTED);
+}
+
static void
test_edid_read(data_t *data, struct chamelium_port *port,
int edid_id, const unsigned char *edid)
From: Souza, Jose <hidden> Date: 2019-03-15 00:00:45
On Thu, 2019-03-14 at 21:57 +0200, Imre Deak wrote:
On Thu, Mar 14, 2019 at 07:59:34PM +0200, Souza, Jose wrote:
quoted
On Thu, 2019-03-14 at 18:59 +0200, Imre Deak wrote:
quoted
Hi Jose,
On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
wrote:
quoted
It is know that some unpowered type-c dongles can take some
time to
boot and be responsible in the DDC/aux transaction lines so a
workaround was implemented in kernel(drm/i915: Enable hotplug
retry)
to fix it but is possible that this could happen to other DP
sinks.
So this test will try to simulate the sceneario described
above, it
will disable the DDC lines and plug the connector, the hotplug
should
fail and then enabling the DDC lines kernel should report the
connector as connected.
The workaround will reprobe connector after 1 second after
kernel
gives up on the first try to probe the connector, so that is
why a
smaller timeout to detect hotplug was needed.
Cc: Imre Deak <redacted>
Signed-off-by: José Roberto de Souza <redacted>
---
tests/kms_chamelium.c | 70
+++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
chamelium_port *port, int toggle_count)
igt_hpd_storm_reset(data->drm_fd);
}
+/*
+ * Test kernel workaround for sinks that takes some time to
have the DDC/aux
+ * channel responsive after the hotplug
+ */
+static void
+test_late_aux_wa(data_t *data, struct chamelium_port *port)
+{
+ struct udev_monitor *mon = igt_watch_hotplug();
+ drmModeConnection status;
+
+ /* Reset will unplug all connectors */
+ reset_state(data, NULL);
+
+ /* Check if it device can act on hotplugs fast enough
for this test */
+ igt_flush_hotplugs(mon);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_CONNECTED);
+
+ igt_flush_hotplugs(mon);
+ chamelium_unplug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_DISCONNECTED);
Do you know on which platforms the hotplug processing is that
slow
and what could be the reason?
I have tested on ICL and WHL and both don't need more than 1
second,
the 20s timeout was set by the first patch(c99f8b7a3) adding
Chamelium
tests but there is not other information about why 20s(in the first
versions of the patch it was 30s).
Ok, sounds to me that HOTPLUG_TIMEOUT was added only to check if
hotplug
detection works at all (all relevant places using it have an
igt_assert() on it).
Still not sure if we need the above two checks, since I think the
assumptions/checks later in the function should hold anyway. Yes, we
may not exercise the hotplug retry path in the driver, but I don't
think
there is a good way to ensure that anyway as I wrote below.
Yeah we should remove the igt_assert() from igt_hotplug_detected() in
those two above.
quoted
I can send another patch reducing this value to 1s and hopefully if
it
do not causes regressions we merge it.
quoted
quoted
+
+ /* It is fast enough, lets disable the DDC lines and
plug again
*/
+ igt_flush_hotplugs(mon);
+ chamelium_port_set_ddc_state(data->chamelium, port,
false);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(!chamelium_port_get_ddc_state(data-
quoted
chamelium,
port));
+
+ /*
+ * Give some time to kernel try to process hotplug but
it
should fail
+ */
+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here? After
the
previous hotplug event above where the state was disconnected
already
there
shouldn't have been any change to the state, hence there
shouldn't be
any event sent by the driver.
It is not signaled, that I why there is not igt_assert() on this
igt_hotplug_detected() but call it will poll/sleep for the time we
want.
But then I don't see how it will work. The sequence is:
<connector is in disconnected state, corresponding event delivered>
1. disable DDC
2. generate a plug event
3. wait for the plug event delivery with 1 sec timeout
4. re-enable DDC
5. wait for the plug event delivery (that should be triggered by the
new
retry logic in the driver)
Since after 2. DDC is disabled the driver hotplug handler will
conclude
that the connector is still disconnected and hence doesn't generate
any
hotplug event. B/c of this 3. will time out after 1 sec.
So in 4. we'll re-enable DDC only after 1 sec after the plug event
(interrupt) generated in 2. Since the retry in the driver happens
after
1 sec from the plug interrupt as well the retry processing could
easily
race with the DDC re-enabling in 4. and thus the detection could
fail.
You are not taking in the account the time that kernel will take to
process that, I measured just the time spend in the hotplug() hook on
my ICL.
[185950.212037] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185950.212109] [drm:i915_hotplug_work_func [i915]] hotplug()
took=673123725 nsec(673 msec) ret=1
[185950.956536] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from disconnected to connected
[185950.957068] [drm:i915_hotplug_work_func [i915]] hotplug()
took=60222955 nsec(60 msec) ret=1
[185953.480877] [drm:drm_dp_dpcd_access] Too many retries, giving up.
First error: -110
[185953.480969] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185953.481038] [drm:i915_hotplug_work_func [i915]] hotplug()
took=672309486 nsec(672 msec) ret=1
More than half of a second retrying until it gives up and change/keep
the status to disconnected.
So in my rough estimation:
t 0s = IGT disables DDC and do the hotplug(1 and 2 from your sequence)
t 0.7s = kernel gives up and keep connector as disconnected
t 1s = IGT read connector as disconnected and enables DCC(3 and 4 from
your sequence)
t 1.7 = kernel try to probe again
t 1.8 = kernel probe and mark connector as connected
t 2s = IGT read connector as connected(5 from your sequence)
Maybe to avoid test failures the second timeout should be bigger
Since I don't see a good way to ensure that we re-enable DDC after
the
first detection cycle ran (but not too late missing the retry cycle)
I
would rather suggest a simple wait at 3., let's say 500msec. With
that
things should always work. We may not always exercise the driver's
retry
path if there was a long scheduling delay, but that's unlikely.
However a scheduling delay after 2. and before 4. could cause a
detection failure. To avoid that I'd also check the elapsed time
starting from right before 2. until right after 4. and run the
sequence
again if the elapsed time was too close to 1sec (and hence detection
possibly failed because of the race described above).
quoted
quoted
quoted
+
+ /*
+ * Enable the DDC line and the kernel workaround should
reprobe
and
+ * report as connected
+ */
+ chamelium_port_set_ddc_state(data->chamelium, port,
true);
+ igt_assert(chamelium_port_get_ddc_state(data-
From: Imre Deak <hidden> Date: 2019-03-15 01:27:13
On Fri, Mar 15, 2019 at 02:00:41AM +0200, Souza, Jose wrote:
On Thu, 2019-03-14 at 21:57 +0200, Imre Deak wrote:
quoted
On Thu, Mar 14, 2019 at 07:59:34PM +0200, Souza, Jose wrote:
quoted
On Thu, 2019-03-14 at 18:59 +0200, Imre Deak wrote:
quoted
Hi Jose,
On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
wrote:
quoted
It is know that some unpowered type-c dongles can take some
time to
boot and be responsible in the DDC/aux transaction lines so a
workaround was implemented in kernel(drm/i915: Enable hotplug
retry)
to fix it but is possible that this could happen to other DP
sinks.
So this test will try to simulate the sceneario described
above, it
will disable the DDC lines and plug the connector, the hotplug
should
fail and then enabling the DDC lines kernel should report the
connector as connected.
The workaround will reprobe connector after 1 second after
kernel
gives up on the first try to probe the connector, so that is
why a
smaller timeout to detect hotplug was needed.
Cc: Imre Deak <redacted>
Signed-off-by: José Roberto de Souza <redacted>
---
tests/kms_chamelium.c | 70
+++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
chamelium_port *port, int toggle_count)
igt_hpd_storm_reset(data->drm_fd);
}
+/*
+ * Test kernel workaround for sinks that takes some time to
have the DDC/aux
+ * channel responsive after the hotplug
+ */
+static void
+test_late_aux_wa(data_t *data, struct chamelium_port *port)
+{
+ struct udev_monitor *mon = igt_watch_hotplug();
+ drmModeConnection status;
+
+ /* Reset will unplug all connectors */
+ reset_state(data, NULL);
+
+ /* Check if it device can act on hotplugs fast enough
for this test */
+ igt_flush_hotplugs(mon);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_CONNECTED);
+
+ igt_flush_hotplugs(mon);
+ chamelium_unplug(data->chamelium, port);
+ igt_assert(igt_hotplug_detected(mon,
FAST_HOTPLUG_SEC_TIMEOUT));
+ status = connector_status_get(data, port);
+ igt_require(status == DRM_MODE_DISCONNECTED);
Do you know on which platforms the hotplug processing is that
slow
and what could be the reason?
I have tested on ICL and WHL and both don't need more than 1
second,
the 20s timeout was set by the first patch(c99f8b7a3) adding
Chamelium
tests but there is not other information about why 20s(in the first
versions of the patch it was 30s).
Ok, sounds to me that HOTPLUG_TIMEOUT was added only to check if
hotplug
detection works at all (all relevant places using it have an
igt_assert() on it).
Still not sure if we need the above two checks, since I think the
assumptions/checks later in the function should hold anyway. Yes, we
may not exercise the hotplug retry path in the driver, but I don't
think
there is a good way to ensure that anyway as I wrote below.
Yeah we should remove the igt_assert() from igt_hotplug_detected() in
those two above.
quoted
quoted
I can send another patch reducing this value to 1s and hopefully if
it
do not causes regressions we merge it.
quoted
quoted
+
+ /* It is fast enough, lets disable the DDC lines and
plug again
*/
+ igt_flush_hotplugs(mon);
+ chamelium_port_set_ddc_state(data->chamelium, port,
false);
+ chamelium_plug(data->chamelium, port);
+ igt_assert(!chamelium_port_get_ddc_state(data-
quoted
chamelium,
port));
+
+ /*
+ * Give some time to kernel try to process hotplug but
it
should fail
+ */
+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here? After
the
previous hotplug event above where the state was disconnected
already
there
shouldn't have been any change to the state, hence there
shouldn't be
any event sent by the driver.
It is not signaled, that I why there is not igt_assert() on this
igt_hotplug_detected() but call it will poll/sleep for the time we
want.
But then I don't see how it will work. The sequence is:
<connector is in disconnected state, corresponding event delivered>
1. disable DDC
2. generate a plug event
3. wait for the plug event delivery with 1 sec timeout
4. re-enable DDC
5. wait for the plug event delivery (that should be triggered by the
new
retry logic in the driver)
Since after 2. DDC is disabled the driver hotplug handler will
conclude
that the connector is still disconnected and hence doesn't generate
any
hotplug event. B/c of this 3. will time out after 1 sec.
So in 4. we'll re-enable DDC only after 1 sec after the plug event
(interrupt) generated in 2. Since the retry in the driver happens
after
1 sec from the plug interrupt as well the retry processing could
easily
race with the DDC re-enabling in 4. and thus the detection could
fail.
You are not taking in the account the time that kernel will take to
process that, I measured just the time spend in the hotplug() hook on
my ICL.
[185950.212037] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185950.212109] [drm:i915_hotplug_work_func [i915]] hotplug()
took=673123725 nsec(673 msec) ret=1
[185950.956536] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from disconnected to connected
[185950.957068] [drm:i915_hotplug_work_func [i915]] hotplug()
took=60222955 nsec(60 msec) ret=1
[185953.480877] [drm:drm_dp_dpcd_access] Too many retries, giving up.
First error: -110
[185953.480969] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185953.481038] [drm:i915_hotplug_work_func [i915]] hotplug()
took=672309486 nsec(672 msec) ret=1
More than half of a second retrying until it gives up and change/keep
the status to disconnected.
670msec for detection to fail sounds strange, where is that coming from?
AUX reads should time out much earlier even with all the retries.
So in my rough estimation:
t 0s = IGT disables DDC and do the hotplug(1 and 2 from your sequence)
t 0.7s = kernel gives up and keep connector as disconnected
t 1s = IGT read connector as disconnected and enables DCC(3 and 4 from
your sequence)
t 1.7 = kernel try to probe again
t 1.8 = kernel probe and mark connector as connected
t 2s = IGT read connector as connected(5 from your sequence)
Maybe to avoid test failures the second timeout should be bigger
quoted
Since I don't see a good way to ensure that we re-enable DDC after
the
first detection cycle ran (but not too late missing the retry cycle)
I
would rather suggest a simple wait at 3., let's say 500msec. With
that
things should always work. We may not always exercise the driver's
retry
path if there was a long scheduling delay, but that's unlikely.
However a scheduling delay after 2. and before 4. could cause a
detection failure. To avoid that I'd also check the elapsed time
starting from right before 2. until right after 4. and run the
sequence
again if the elapsed time was too close to 1sec (and hence detection
possibly failed because of the race described above).
quoted
quoted
quoted
+
+ /*
+ * Enable the DDC line and the kernel workaround should
reprobe
and
+ * report as connected
+ */
+ chamelium_port_set_ddc_state(data->chamelium, port,
true);
+ igt_assert(chamelium_port_get_ddc_state(data-
From: Imre Deak <hidden> Date: 2019-03-15 02:46:27
On Fri, Mar 15, 2019 at 03:27:08AM +0200, Imre Deak wrote:
On Fri, Mar 15, 2019 at 02:00:41AM +0200, Souza, Jose wrote:
[...]
quoted
quoted
quoted
quoted
quoted
+
+ /*
+ * Give some time to kernel try to process hotplug but
it
should fail
+ */
+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here? After
the
previous hotplug event above where the state was disconnected
already
there
shouldn't have been any change to the state, hence there
shouldn't be
any event sent by the driver.
It is not signaled, that I why there is not igt_assert() on this
igt_hotplug_detected() but call it will poll/sleep for the time we
want.
But then I don't see how it will work. The sequence is:
<connector is in disconnected state, corresponding event delivered>
1. disable DDC
2. generate a plug event
3. wait for the plug event delivery with 1 sec timeout
4. re-enable DDC
5. wait for the plug event delivery (that should be triggered by the
new
retry logic in the driver)
Since after 2. DDC is disabled the driver hotplug handler will
conclude
that the connector is still disconnected and hence doesn't generate
any
hotplug event. B/c of this 3. will time out after 1 sec.
So in 4. we'll re-enable DDC only after 1 sec after the plug event
(interrupt) generated in 2. Since the retry in the driver happens
after
1 sec from the plug interrupt as well the retry processing could
easily
race with the DDC re-enabling in 4. and thus the detection could
fail.
You are not taking in the account the time that kernel will take to
process that, I measured just the time spend in the hotplug() hook on
my ICL.
[185950.212037] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185950.212109] [drm:i915_hotplug_work_func [i915]] hotplug()
took=673123725 nsec(673 msec) ret=1
[185950.956536] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from disconnected to connected
[185950.957068] [drm:i915_hotplug_work_func [i915]] hotplug()
took=60222955 nsec(60 msec) ret=1
[185953.480877] [drm:drm_dp_dpcd_access] Too many retries, giving up.
First error: -110
[185953.480969] [drm:intel_encoder_hotplug [i915]] [CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185953.481038] [drm:i915_hotplug_work_func [i915]] hotplug()
took=672309486 nsec(672 msec) ret=1
More than half of a second retrying until it gives up and change/keep
the status to disconnected.
670msec for detection to fail sounds strange, where is that coming from?
AUX reads should time out much earlier even with all the retries.
Ah, could be the 4msec (max) AUX HW timeout on ICL. That with retries
adds up to 5*32*4ms = 640msec about what you have measured.
But up to SKL the AUX HW timeout is only 1.6msec giving a 256msec delay,
so the hotplug retry could happen as soon as ~1.3sec after the plug event.
So I guess a 1 sec delay/poll at step 3. is ok along with some
explanation about the duration like the above calculation. I think it
could still be possible for this 1 sec delay/poll to last longer than
1.3sec (due to scheduling) in which case the detection would fail on
some platforms. So I'd still add the time measurement between step 2.
and 4. as described below and rerun the test if it was > 1.2sec and
detection failed.
quoted
So in my rough estimation:
t 0s = IGT disables DDC and do the hotplug(1 and 2 from your sequence)
t 0.7s = kernel gives up and keep connector as disconnected
t 1s = IGT read connector as disconnected and enables DCC(3 and 4 from
your sequence)
t 1.7 = kernel try to probe again
t 1.8 = kernel probe and mark connector as connected
t 2s = IGT read connector as connected(5 from your sequence)
Maybe to avoid test failures the second timeout should be bigger
quoted
Since I don't see a good way to ensure that we re-enable DDC after
the
first detection cycle ran (but not too late missing the retry cycle)
I
would rather suggest a simple wait at 3., let's say 500msec. With
that
things should always work. We may not always exercise the driver's
retry
path if there was a long scheduling delay, but that's unlikely.
However a scheduling delay after 2. and before 4. could cause a
detection failure. To avoid that I'd also check the elapsed time
starting from right before 2. until right after 4. and run the
sequence
again if the elapsed time was too close to 1sec (and hence detection
possibly failed because of the race described above).
quoted
quoted
quoted
+
+ /*
+ * Enable the DDC line and the kernel workaround should
reprobe
and
+ * report as connected
+ */
+ chamelium_port_set_ddc_state(data->chamelium, port,
true);
+ igt_assert(chamelium_port_get_ddc_state(data-
From: Souza, Jose <hidden> Date: 2019-03-15 22:09:39
On Fri, 2019-03-15 at 04:46 +0200, Imre Deak wrote:
On Fri, Mar 15, 2019 at 03:27:08AM +0200, Imre Deak wrote:
quoted
On Fri, Mar 15, 2019 at 02:00:41AM +0200, Souza, Jose wrote:
[...]
quoted
quoted
quoted
quoted
quoted
+
+ /*
+ * Give some time to kernel try to process hotplug but
it
should fail
+ */
+ igt_hotplug_detected(mon, FAST_HOTPLUG_SEC_TIMEOUT);
+ status = connector_status_get(data, port);
+ igt_assert(status == DRM_MODE_DISCONNECTED);
Hm, how does a second disconnect event get signaled here?
After
the
previous hotplug event above where the state was
disconnected
already
there
shouldn't have been any change to the state, hence there
shouldn't be
any event sent by the driver.
It is not signaled, that I why there is not igt_assert() on
this
igt_hotplug_detected() but call it will poll/sleep for the
time we
want.
But then I don't see how it will work. The sequence is:
<connector is in disconnected state, corresponding event
delivered>
1. disable DDC
2. generate a plug event
3. wait for the plug event delivery with 1 sec timeout
4. re-enable DDC
5. wait for the plug event delivery (that should be triggered
by the
new
retry logic in the driver)
Since after 2. DDC is disabled the driver hotplug handler will
conclude
that the connector is still disconnected and hence doesn't
generate
any
hotplug event. B/c of this 3. will time out after 1 sec.
So in 4. we'll re-enable DDC only after 1 sec after the plug
event
(interrupt) generated in 2. Since the retry in the driver
happens
after
1 sec from the plug interrupt as well the retry processing
could
easily
race with the DDC re-enabling in 4. and thus the detection
could
fail.
You are not taking in the account the time that kernel will take
to
process that, I measured just the time spend in the hotplug()
hook on
my ICL.
[185950.212037] [drm:intel_encoder_hotplug [i915]]
[CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185950.212109] [drm:i915_hotplug_work_func [i915]] hotplug()
took=673123725 nsec(673 msec) ret=1
[185950.956536] [drm:intel_encoder_hotplug [i915]]
[CONNECTOR:196:DP-1]
status updated from disconnected to connected
[185950.957068] [drm:i915_hotplug_work_func [i915]] hotplug()
took=60222955 nsec(60 msec) ret=1
[185953.480877] [drm:drm_dp_dpcd_access] Too many retries, giving
up.
First error: -110
[185953.480969] [drm:intel_encoder_hotplug [i915]]
[CONNECTOR:196:DP-1]
status updated from connected to disconnected
[185953.481038] [drm:i915_hotplug_work_func [i915]] hotplug()
took=672309486 nsec(672 msec) ret=1
More than half of a second retrying until it gives up and
change/keep
the status to disconnected.
670msec for detection to fail sounds strange, where is that coming
from?
AUX reads should time out much earlier even with all the retries.
Ah, could be the 4msec (max) AUX HW timeout on ICL. That with retries
adds up to 5*32*4ms = 640msec about what you have measured.
But up to SKL the AUX HW timeout is only 1.6msec giving a 256msec
delay,
so the hotplug retry could happen as soon as ~1.3sec after the plug
event.
So I guess a 1 sec delay/poll at step 3. is ok along with some
explanation about the duration like the above calculation. I think it
could still be possible for this 1 sec delay/poll to last longer than
1.3sec (due to scheduling) in which case the detection would fail on
some platforms. So I'd still add the time measurement between step 2.
and 4. as described below and rerun the test if it was > 1.2sec and
detection failed.
Yeah sound good measure time between 2 and 4 and retry, I will also the
comments that you requested.
Thanks for the reviews :D
quoted
quoted
So in my rough estimation:
t 0s = IGT disables DDC and do the hotplug(1 and 2 from your
sequence)
t 0.7s = kernel gives up and keep connector as disconnected
t 1s = IGT read connector as disconnected and enables DCC(3 and 4
from
your sequence)
t 1.7 = kernel try to probe again
t 1.8 = kernel probe and mark connector as connected
t 2s = IGT read connector as connected(5 from your sequence)
Maybe to avoid test failures the second timeout should be bigger
quoted
Since I don't see a good way to ensure that we re-enable DDC
after
the
first detection cycle ran (but not too late missing the retry
cycle)
I
would rather suggest a simple wait at 3., let's say 500msec.
With
that
things should always work. We may not always exercise the
driver's
retry
path if there was a long scheduling delay, but that's unlikely.
However a scheduling delay after 2. and before 4. could cause a
detection failure. To avoid that I'd also check the elapsed
time
starting from right before 2. until right after 4. and run the
sequence
again if the elapsed time was too close to 1sec (and hence
detection
possibly failed because of the race described above).
quoted
quoted
quoted
+
+ /*
+ * Enable the DDC line and the kernel workaround should
reprobe
and
+ * report as connected
+ */
+ chamelium_port_set_ddc_state(data->chamelium, port,
true);
+ igt_assert(chamelium_port_get_ddc_state(data-