All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
@ 2019-03-13  0:59 José Roberto de Souza
  2019-03-13 13:40 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: José Roberto de Souza @ 2019-03-13  0:59 UTC (permalink / raw)
  To: igt-dev

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 <imre.deak@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_chamelium.c | 70 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index c2090037..50a553f2 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -45,6 +45,8 @@ typedef struct {
 
 #define HOTPLUG_TIMEOUT 20 /* seconds */
 
+#define FAST_HOTPLUG_SEC_TIMEOUT (1)
+
 #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
 #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
 
@@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct chamelium_port *port)
 	return status;
 }
 
+static drmModeConnection
+connector_status_get(data_t *data, struct chamelium_port *port)
+{
+	drmModeConnector *connector;
+	drmModeConnection status;
+
+	igt_debug("Getting connector state %s...\n", chamelium_port_get_name(port));
+	connector = chamelium_port_get_connector(data->chamelium, port, false);
+	igt_assert(connector);
+	status = connector->connection;
+
+	drmModeFreeConnector(connector);
+	return status;
+}
+
 static void
 wait_for_connector(data_t *data, struct chamelium_port *port,
 		   drmModeConnection status)
@@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct 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);
+
+	/* 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);
+
+	/*
+	 * 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)
@@ -1308,6 +1375,9 @@ igt_main
 
 		connector_subtest("dp-frame-dump", DisplayPort)
 			test_display_frame_dump(&data, port);
+
+		connector_subtest("dp-late-aux-wa", DisplayPort)
+			test_late_aux_wa(&data, port);
 	}
 
 	igt_subtest_group {
-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for tests/chamelium: Add test for hotplug workaround
  2019-03-13  0:59 [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround José Roberto de Souza
@ 2019-03-13 13:40 ` Patchwork
  2019-03-13 16:46 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  2019-03-14 16:59 ` [igt-dev] [PATCH i-g-t] " Imre Deak
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-03-13 13:40 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

== Series Details ==

Series: tests/chamelium: Add test for hotplug workaround
URL   : https://patchwork.freedesktop.org/series/57913/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5737 -> IGTPW_2601
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_create@basic-files:
    - fi-gdg-551:         NOTRUN -> SKIP [fdo#109271] +106

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

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

  * igt@kms_addfb_basic@addfb25-y-tiled-small:
    - fi-byt-n2820:       NOTRUN -> SKIP [fdo#109271] +56

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

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-hsw-peppy:       NOTRUN -> SKIP [fdo#109271] +46

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          PASS -> FAIL [fdo#103167]
    - fi-hsw-peppy:       NOTRUN -> DMESG-FAIL [fdo#102614] / [fdo#107814]
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +48

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103191] / [fdo#107362] +1

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107814]: https://bugs.freedesktop.org/show_bug.cgi?id=107814
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110028]: https://bugs.freedesktop.org/show_bug.cgi?id=110028


Participating hosts (41 -> 35)
------------------------------

  Additional (6): fi-hsw-peppy fi-snb-2520m fi-gdg-551 fi-icl-y fi-byt-n2820 fi-byt-clapper 
  Missing    (12): fi-kbl-soraka fi-ilk-m540 fi-bsw-n3050 fi-byt-j1900 fi-skl-6770hq fi-skl-guc fi-hsw-4200u fi-apl-guc fi-ctg-p8600 fi-kbl-x1275 fi-bsw-kefka fi-bdw-samus 


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

    * IGT: IGT_4883 -> IGTPW_2601

  CI_DRM_5737: d5bb7d77aa77996702426496078a597f30bead58 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2601: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2601/
  IGT_4883: b25e06d6ddf2e42044cd9c93b613cbc7339a8c33 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@kms_chamelium@dp-late-aux-wa

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2601/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for tests/chamelium: Add test for hotplug workaround
  2019-03-13  0:59 [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround José Roberto de Souza
  2019-03-13 13:40 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
@ 2019-03-13 16:46 ` Patchwork
  2019-03-14 16:59 ` [igt-dev] [PATCH i-g-t] " Imre Deak
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-03-13 16:46 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

== Series Details ==

Series: tests/chamelium: Add test for hotplug workaround
URL   : https://patchwork.freedesktop.org/series/57913/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5737_full -> IGTPW_2601_full
====================================================

Summary
-------

  **WARNING**

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

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

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

### IGT changes ###

#### Warnings ####

  * igt@kms_plane_scaling@pipe-b-scaler-with-pixel-format:
    - shard-glk:          SKIP [fdo#109271] / [fdo#109278] -> FAIL

  
New tests
---------

  New tests have been introduced between CI_DRM_5737_full and IGTPW_2601_full:

### New IGT tests (1) ###

  * igt@kms_chamelium@dp-late-aux-wa:
    - Statuses : 5 skip(s)
    - Exec time: [0.0] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_param@invalid-param-set:
    - shard-snb:          NOTRUN -> FAIL [fdo#109674]

  * igt@gem_exec_schedule@preempt-other-chain-blt:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +110

  * igt@i915_pm_rpm@dpms-lpsp:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +32

  * igt@i915_suspend@sysfs-reader:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@kms_atomic_transition@3x-modeset-transitions:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#107956] +1
    - shard-snb:          PASS -> DMESG-WARN [fdo#107956]
    - shard-hsw:          PASS -> DMESG-WARN [fdo#107956]

  * {igt@kms_chamelium@dp-late-aux-wa} (NEW):
    - shard-hsw:          NOTRUN -> SKIP [fdo#109271]

  * igt@kms_color@pipe-a-degamma:
    - shard-glk:          NOTRUN -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_color@pipe-c-ctm-max:
    - shard-apl:          PASS -> FAIL [fdo#108147] +1

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-glk:          NOTRUN -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-256x256-onscreen:
    - shard-kbl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-kbl:          PASS -> FAIL [fdo#103191] / [fdo#103232]
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +3

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-xtiled:
    - shard-glk:          PASS -> FAIL [fdo#107791]

  * igt@kms_flip@flip-vs-suspend:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
    - shard-apl:          PASS -> FAIL [fdo#103167]
    - shard-kbl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167] +6

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-render:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +11

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-f:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]

  * igt@kms_psr@basic:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +5

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_universal_plane@cursor-fb-leak-pipe-d:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +9

  * igt@perf_pmu@rc6:
    - shard-kbl:          PASS -> SKIP [fdo#109271]

  * igt@prime_vgem@basic-fence-flip:
    - shard-kbl:          PASS -> FAIL [fdo#104008]

  * igt@runner@aborted:
    - shard-apl:          NOTRUN -> ( 6 FAIL ) [fdo#109373]

  
#### Possible fixes ####

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-hsw:          DMESG-WARN [fdo#107956] -> PASS
    - shard-kbl:          DMESG-WARN [fdo#107956] -> PASS
    - shard-snb:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_color@pipe-c-legacy-gamma:
    - shard-apl:          FAIL [fdo#104782] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-sliding:
    - shard-kbl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-kbl:          FAIL [fdo#103191] / [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-size-change:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
    - shard-hsw:          FAIL [fdo#105767] -> PASS

  * igt@kms_flip_tiling@flip-changes-tiling-yf:
    - shard-apl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-apl:          FAIL [fdo#103167] -> PASS +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * {igt@kms_plane@plane-position-covered-pipe-a-planes}:
    - shard-glk:          FAIL [fdo#110038] -> PASS

  * {igt@kms_plane@plane-position-covered-pipe-b-planes}:
    - shard-apl:          FAIL [fdo#110038] -> PASS

  * {igt@kms_plane_multiple@atomic-pipe-a-tiling-none}:
    - shard-apl:          FAIL [fdo#110037] -> PASS

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          FAIL [fdo#109016] -> PASS

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-apl:          FAIL [fdo#104894] -> PASS +1
    - shard-kbl:          FAIL [fdo#104894] -> PASS

  * igt@perf@rc6-disable:
    - shard-kbl:          FAIL [fdo#103179] -> 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#103179]: https://bugs.freedesktop.org/show_bug.cgi?id=103179
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#107791]: https://bugs.freedesktop.org/show_bug.cgi?id=107791
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
  [fdo#109674]: https://bugs.freedesktop.org/show_bug.cgi?id=109674
  [fdo#110037]: https://bugs.freedesktop.org/show_bug.cgi?id=110037
  [fdo#110038]: https://bugs.freedesktop.org/show_bug.cgi?id=110038


Participating hosts (10 -> 5)
------------------------------

  Missing    (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u 


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

    * IGT: IGT_4883 -> IGTPW_2601
    * Piglit: piglit_4509 -> None

  CI_DRM_5737: d5bb7d77aa77996702426496078a597f30bead58 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2601: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2601/
  IGT_4883: b25e06d6ddf2e42044cd9c93b613cbc7339a8c33 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2601/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-13  0:59 [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround José Roberto de Souza
  2019-03-13 13:40 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
  2019-03-13 16:46 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2019-03-14 16:59 ` Imre Deak
  2019-03-14 17:59   ` Souza, Jose
  2 siblings, 1 reply; 10+ messages in thread
From: Imre Deak @ 2019-03-14 16:59 UTC (permalink / raw)
  To: José Roberto de Souza; +Cc: igt-dev

Hi Jose,

On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza wrote:
> 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 <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_chamelium.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index c2090037..50a553f2 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -45,6 +45,8 @@ typedef struct {
>  
>  #define HOTPLUG_TIMEOUT 20 /* seconds */
>  
> +#define FAST_HOTPLUG_SEC_TIMEOUT (1)
> +
>  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
>  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
>  
> @@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct chamelium_port *port)
>  	return status;
>  }
>  
> +static drmModeConnection
> +connector_status_get(data_t *data, struct chamelium_port *port)
> +{
> +	drmModeConnector *connector;
> +	drmModeConnection status;
> +
> +	igt_debug("Getting connector state %s...\n", chamelium_port_get_name(port));
> +	connector = chamelium_port_get_connector(data->chamelium, port, false);
> +	igt_assert(connector);
> +	status = connector->connection;
> +
> +	drmModeFreeConnector(connector);
> +	return status;
> +}
> +
>  static void
>  wait_for_connector(data_t *data, struct chamelium_port *port,
>  		   drmModeConnection status)
> @@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct 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?

> +
> +	/* 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.

> +
> +	/*
> +	 * 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)
> @@ -1308,6 +1375,9 @@ igt_main
>  
>  		connector_subtest("dp-frame-dump", DisplayPort)
>  			test_display_frame_dump(&data, port);
> +
> +		connector_subtest("dp-late-aux-wa", DisplayPort)
> +			test_late_aux_wa(&data, port);
>  	}
>  
>  	igt_subtest_group {
> -- 
> 2.21.0
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-14 16:59 ` [igt-dev] [PATCH i-g-t] " Imre Deak
@ 2019-03-14 17:59   ` Souza, Jose
  2019-03-14 19:57     ` Imre Deak
  0 siblings, 1 reply; 10+ messages in thread
From: Souza, Jose @ 2019-03-14 17:59 UTC (permalink / raw)
  To: Deak, Imre; +Cc: igt-dev


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

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:
> > 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 <imre.deak@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_chamelium.c | 70
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index c2090037..50a553f2 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -45,6 +45,8 @@ typedef struct {
> >  
> >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> >  
> > +#define FAST_HOTPLUG_SEC_TIMEOUT (1)
> > +
> >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> >  
> > @@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct
> > chamelium_port *port)
> >  	return status;
> >  }
> >  
> > +static drmModeConnection
> > +connector_status_get(data_t *data, struct chamelium_port *port)
> > +{
> > +	drmModeConnector *connector;
> > +	drmModeConnection status;
> > +
> > +	igt_debug("Getting connector state %s...\n",
> > chamelium_port_get_name(port));
> > +	connector = chamelium_port_get_connector(data->chamelium, port,
> > false);
> > +	igt_assert(connector);
> > +	status = connector->connection;
> > +
> > +	drmModeFreeConnector(connector);
> > +	return status;
> > +}
> > +
> >  static void
> >  wait_for_connector(data_t *data, struct chamelium_port *port,
> >  		   drmModeConnection status)
> > @@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct
> > 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.

> 
> > +
> > +	/* 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.


> 
> > +
> > +	/*
> > +	 * 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)
> > @@ -1308,6 +1375,9 @@ igt_main
> >  
> >  		connector_subtest("dp-frame-dump", DisplayPort)
> >  			test_display_frame_dump(&data, port);
> > +
> > +		connector_subtest("dp-late-aux-wa", DisplayPort)
> > +			test_late_aux_wa(&data, port);
> >  	}
> >  
> >  	igt_subtest_group {
> > -- 
> > 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: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-14 17:59   ` Souza, Jose
@ 2019-03-14 19:57     ` Imre Deak
  2019-03-15  0:00       ` Souza, Jose
  0 siblings, 1 reply; 10+ messages in thread
From: Imre Deak @ 2019-03-14 19:57 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

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:
> > Hi Jose,
> > 
> > On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
> > wrote:
> > > 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 <imre.deak@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  tests/kms_chamelium.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > > 
> > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > index c2090037..50a553f2 100644
> > > --- a/tests/kms_chamelium.c
> > > +++ b/tests/kms_chamelium.c
> > > @@ -45,6 +45,8 @@ typedef struct {
> > >  
> > >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> > >  
> > > +#define FAST_HOTPLUG_SEC_TIMEOUT (1)
> > > +
> > >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> > >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> > >  
> > > @@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct
> > > chamelium_port *port)
> > >  	return status;
> > >  }
> > >  
> > > +static drmModeConnection
> > > +connector_status_get(data_t *data, struct chamelium_port *port)
> > > +{
> > > +	drmModeConnector *connector;
> > > +	drmModeConnection status;
> > > +
> > > +	igt_debug("Getting connector state %s...\n", chamelium_port_get_name(port));
> > > +	connector = chamelium_port_get_connector(data->chamelium, port, false);
> > > +	igt_assert(connector);
> > > +	status = connector->connection;
> > > +
> > > +	drmModeFreeConnector(connector);
> > > +	return status;
> > > +}
> > > +
> > >  static void
> > >  wait_for_connector(data_t *data, struct chamelium_port *port,
> > >  		   drmModeConnection status)
> > > @@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct
> > > 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.
> > > +
> > > +	/* 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).

> > > +
> > > +	/*
> > > +	 * 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)
> > > @@ -1308,6 +1375,9 @@ igt_main
> > >  
> > >  		connector_subtest("dp-frame-dump", DisplayPort)
> > >  			test_display_frame_dump(&data, port);
> > > +
> > > +		connector_subtest("dp-late-aux-wa", DisplayPort)
> > > +			test_late_aux_wa(&data, port);

This also needs the retry logic to be added for DP ports on old
platforms (intel_dp_hotplug() in the driver).

> > >  	}
> > >  
> > >  	igt_subtest_group {
> > > -- 
> > > 2.21.0
> > > 


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-14 19:57     ` Imre Deak
@ 2019-03-15  0:00       ` Souza, Jose
  2019-03-15  1:27         ` Imre Deak
  0 siblings, 1 reply; 10+ messages in thread
From: Souza, Jose @ 2019-03-15  0:00 UTC (permalink / raw)
  To: Deak, Imre; +Cc: igt-dev


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

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:
> > 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:
> > > > 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 <imre.deak@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  tests/kms_chamelium.c | 70
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > > 
> > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > index c2090037..50a553f2 100644
> > > > --- a/tests/kms_chamelium.c
> > > > +++ b/tests/kms_chamelium.c
> > > > @@ -45,6 +45,8 @@ typedef struct {
> > > >  
> > > >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> > > >  
> > > > +#define FAST_HOTPLUG_SEC_TIMEOUT (1)
> > > > +
> > > >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> > > >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> > > >  
> > > > @@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct
> > > > chamelium_port *port)
> > > >  	return status;
> > > >  }
> > > >  
> > > > +static drmModeConnection
> > > > +connector_status_get(data_t *data, struct chamelium_port
> > > > *port)
> > > > +{
> > > > +	drmModeConnector *connector;
> > > > +	drmModeConnection status;
> > > > +
> > > > +	igt_debug("Getting connector state %s...\n",
> > > > chamelium_port_get_name(port));
> > > > +	connector = chamelium_port_get_connector(data-
> > > > >chamelium, port, false);
> > > > +	igt_assert(connector);
> > > > +	status = connector->connection;
> > > > +
> > > > +	drmModeFreeConnector(connector);
> > > > +	return status;
> > > > +}
> > > > +
> > > >  static void
> > > >  wait_for_connector(data_t *data, struct chamelium_port *port,
> > > >  		   drmModeConnection status)
> > > > @@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct
> > > > 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.

> 
> > I can send another patch reducing this value to 1s and hopefully if
> > it
> > do not causes regressions we merge it.
> > > > +
> > > > +	/* 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.

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).
> 
> > > > +
> > > > +	/*
> > > > +	 * 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)
> > > > @@ -1308,6 +1375,9 @@ igt_main
> > > >  
> > > >  		connector_subtest("dp-frame-dump", DisplayPort)
> > > >  			test_display_frame_dump(&data, port);
> > > > +
> > > > +		connector_subtest("dp-late-aux-wa",
> > > > DisplayPort)
> > > > +			test_late_aux_wa(&data, port);
> 
> This also needs the retry logic to be added for DP ports on old
> platforms (intel_dp_hotplug() in the driver).
> 
> > > >  	}
> > > >  
> > > >  	igt_subtest_group {
> > > > -- 
> > > > 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: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-15  0:00       ` Souza, Jose
@ 2019-03-15  1:27         ` Imre Deak
  2019-03-15  2:46           ` Imre Deak
  0 siblings, 1 reply; 10+ messages in thread
From: Imre Deak @ 2019-03-15  1:27 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

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:
> > 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:
> > > > Hi Jose,
> > > > 
> > > > On Tue, Mar 12, 2019 at 05:59:45PM -0700, José Roberto de Souza
> > > > wrote:
> > > > > 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 <imre.deak@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  tests/kms_chamelium.c | 70
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 70 insertions(+)
> > > > > 
> > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > index c2090037..50a553f2 100644
> > > > > --- a/tests/kms_chamelium.c
> > > > > +++ b/tests/kms_chamelium.c
> > > > > @@ -45,6 +45,8 @@ typedef struct {
> > > > >  
> > > > >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> > > > >  
> > > > > +#define FAST_HOTPLUG_SEC_TIMEOUT (1)
> > > > > +
> > > > >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> > > > >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> > > > >  
> > > > > @@ -107,6 +109,21 @@ reprobe_connector(data_t *data, struct
> > > > > chamelium_port *port)
> > > > >  	return status;
> > > > >  }
> > > > >  
> > > > > +static drmModeConnection
> > > > > +connector_status_get(data_t *data, struct chamelium_port
> > > > > *port)
> > > > > +{
> > > > > +	drmModeConnector *connector;
> > > > > +	drmModeConnection status;
> > > > > +
> > > > > +	igt_debug("Getting connector state %s...\n",
> > > > > chamelium_port_get_name(port));
> > > > > +	connector = chamelium_port_get_connector(data-
> > > > > >chamelium, port, false);
> > > > > +	igt_assert(connector);
> > > > > +	status = connector->connection;
> > > > > +
> > > > > +	drmModeFreeConnector(connector);
> > > > > +	return status;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  wait_for_connector(data_t *data, struct chamelium_port *port,
> > > > >  		   drmModeConnection status)
> > > > > @@ -253,6 +270,56 @@ test_basic_hotplug(data_t *data, struct
> > > > > 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.
> 
> > 
> > > I can send another patch reducing this value to 1s and hopefully if
> > > it
> > > do not causes regressions we merge it.
> > > > > +
> > > > > +	/* 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.
> 
> 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
> 
> > 
> > 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).
> > 
> > > > > +
> > > > > +	/*
> > > > > +	 * 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)
> > > > > @@ -1308,6 +1375,9 @@ igt_main
> > > > >  
> > > > >  		connector_subtest("dp-frame-dump", DisplayPort)
> > > > >  			test_display_frame_dump(&data, port);
> > > > > +
> > > > > +		connector_subtest("dp-late-aux-wa",
> > > > > DisplayPort)
> > > > > +			test_late_aux_wa(&data, port);
> > 
> > This also needs the retry logic to be added for DP ports on old
> > platforms (intel_dp_hotplug() in the driver).
> > 
> > > > >  	}
> > > > >  
> > > > >  	igt_subtest_group {
> > > > > -- 
> > > > > 2.21.0
> > > > > 
> > 
> > 


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-15  1:27         ` Imre Deak
@ 2019-03-15  2:46           ` Imre Deak
  2019-03-15 22:09             ` Souza, Jose
  0 siblings, 1 reply; 10+ messages in thread
From: Imre Deak @ 2019-03-15  2:46 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

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:
> [...]
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * 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.

> > 
> > 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).
> > > 
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * 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)
> > > > > > @@ -1308,6 +1375,9 @@ igt_main
> > > > > >  
> > > > > >  		connector_subtest("dp-frame-dump", DisplayPort)
> > > > > >  			test_display_frame_dump(&data, port);
> > > > > > +
> > > > > > +		connector_subtest("dp-late-aux-wa",
> > > > > > DisplayPort)
> > > > > > +			test_late_aux_wa(&data, port);
> > > 
> > > This also needs the retry logic to be added for DP ports on old
> > > platforms (intel_dp_hotplug() in the driver).
> > > 
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest_group {
> > > > > > -- 
> > > > > > 2.21.0
> > > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround
  2019-03-15  2:46           ` Imre Deak
@ 2019-03-15 22:09             ` Souza, Jose
  0 siblings, 0 replies; 10+ messages in thread
From: Souza, Jose @ 2019-03-15 22:09 UTC (permalink / raw)
  To: Deak, Imre; +Cc: igt-dev


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

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:
> > On Fri, Mar 15, 2019 at 02:00:41AM +0200, Souza, Jose wrote:
> > [...]
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * 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

> 
> > > 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).
> > > > 
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * 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)
> > > > > > > @@ -1308,6 +1375,9 @@ igt_main
> > > > > > >  
> > > > > > >  		connector_subtest("dp-frame-dump", DisplayPort)
> > > > > > >  			test_display_frame_dump(&data, port);
> > > > > > > +
> > > > > > > +		connector_subtest("dp-late-aux-wa",
> > > > > > > DisplayPort)
> > > > > > > +			test_late_aux_wa(&data, port);
> > > > 
> > > > This also needs the retry logic to be added for DP ports on old
> > > > platforms (intel_dp_hotplug() in the driver).
> > > > 
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	igt_subtest_group {
> > > > > > > -- 
> > > > > > > 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: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-03-15 22:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13  0:59 [igt-dev] [PATCH i-g-t] tests/chamelium: Add test for hotplug workaround José Roberto de Souza
2019-03-13 13:40 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-03-13 16:46 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2019-03-14 16:59 ` [igt-dev] [PATCH i-g-t] " Imre Deak
2019-03-14 17:59   ` Souza, Jose
2019-03-14 19:57     ` Imre Deak
2019-03-15  0:00       ` Souza, Jose
2019-03-15  1:27         ` Imre Deak
2019-03-15  2:46           ` Imre Deak
2019-03-15 22:09             ` Souza, Jose

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.