| Message ID | 20170627105311.28975-5-paul.kocialkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I -think- this one might be okay but I just got reminded of a rather not-obvious part of the DRM spec for hotplugging, and I need to check that this doesn't actually break that. will get back to you in a bit with an actual review :) On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote: > This renames the reprobe_connector function to update_connector and > ensures that full reprobe of the connector is only done when really > necessary (e.g. when changing the EDID). > > A full reprobe takes time and is not required for updating the > connector > state. Thus, this allows executing tests faster. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > --- > tests/chamelium.c | 34 ++++++++++++++++++---------------- > 1 file changed, 18 insertions(+), 16 deletions(-) > > diff --git a/tests/chamelium.c b/tests/chamelium.c > index b412c6a7..346018a5 100644 > --- a/tests/chamelium.c > +++ b/tests/chamelium.c > @@ -103,13 +103,14 @@ require_connector_present(data_t *data, > unsigned int type) > } > > static drmModeConnection > -reprobe_connector(data_t *data, struct chamelium_port *port) > +update_connector(data_t *data, struct chamelium_port *port, bool > reprobe) > { > drmModeConnector *connector; > drmModeConnection status; > > - igt_debug("Reprobing %s...\n", > chamelium_port_get_name(port)); > - connector = chamelium_port_get_connector(data->chamelium, > port, true); > + igt_debug("Updating %s...\n", > chamelium_port_get_name(port)); > + connector = chamelium_port_get_connector(data->chamelium, > port, > + reprobe); > igt_assert(connector); > status = connector->connection; > > @@ -119,7 +120,7 @@ reprobe_connector(data_t *data, struct > chamelium_port *port) > > static void > wait_for_connector(data_t *data, struct chamelium_port *port, > - drmModeConnection status) > + drmModeConnection status, bool reprobe) > { > bool finished = false; > > @@ -132,7 +133,7 @@ wait_for_connector(data_t *data, struct > chamelium_port *port, > * that hpd events work in the event that hpd doesn't work > on the system > */ > igt_until_timeout(HOTPLUG_TIMEOUT) { > - if (reprobe_connector(data, port) == status) { > + if (update_connector(data, port, reprobe) == status) > { > finished = true; > return; > } > @@ -151,11 +152,12 @@ reset_state(data_t *data, struct chamelium_port > *port) > chamelium_reset(data->chamelium); > > if (port) { > - wait_for_connector(data, port, > DRM_MODE_DISCONNECTED); > + wait_for_connector(data, port, > DRM_MODE_DISCONNECTED, false); > } else { > for (p = 0; p < data->port_count; p++) { > port = data->ports[p]; > - wait_for_connector(data, port, > DRM_MODE_DISCONNECTED); > + wait_for_connector(data, port, > DRM_MODE_DISCONNECTED, > + false); > } > } > } > @@ -175,7 +177,7 @@ test_basic_hotplug(data_t *data, struct > chamelium_port *port, int toggle_count) > /* Check if we get a sysfs hotplug event */ > chamelium_plug(data->chamelium, port); > igt_assert(igt_hotplug_detected(mon, > HOTPLUG_TIMEOUT)); > - igt_assert_eq(reprobe_connector(data, port), > + igt_assert_eq(update_connector(data, port, false), > DRM_MODE_CONNECTED); > > igt_flush_hotplugs(mon); > @@ -183,7 +185,7 @@ test_basic_hotplug(data_t *data, struct > chamelium_port *port, int toggle_count) > /* Now check if we get a hotplug from disconnection > */ > chamelium_unplug(data->chamelium, port); > igt_assert(igt_hotplug_detected(mon, > HOTPLUG_TIMEOUT)); > - igt_assert_eq(reprobe_connector(data, port), > + igt_assert_eq(update_connector(data, port, false), > DRM_MODE_DISCONNECTED); > } > > @@ -204,7 +206,7 @@ test_edid_read(data_t *data, struct > chamelium_port *port, > > chamelium_port_set_edid(data->chamelium, port, edid_id); > chamelium_plug(data->chamelium, port); > - wait_for_connector(data, port, DRM_MODE_CONNECTED); > + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); > > igt_assert(kmstest_get_property(data->drm_fd, connector- > >connector_id, > DRM_MODE_OBJECT_CONNECTOR, > "EDID", NULL, > @@ -247,13 +249,13 @@ try_suspend_resume_hpd(data_t *data, struct > chamelium_port *port, > > igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); > if (port) { > - igt_assert_eq(reprobe_connector(data, port), > connected ? > + igt_assert_eq(update_connector(data, port, false), > connected ? > DRM_MODE_DISCONNECTED : > DRM_MODE_CONNECTED); > } else { > for (p = 0; p < data->port_count; p++) { > port = data->ports[p]; > - igt_assert_eq(reprobe_connector(data, port), > connected ? > - DRM_MODE_DISCONNECTED : > + igt_assert_eq(update_connector(data, port, > false), > + connected ? > DRM_MODE_DISCONNECTED : > DRM_MODE_CONNECTED); > } > > @@ -317,7 +319,7 @@ test_suspend_resume_edid_change(data_t *data, > struct chamelium_port *port, > /* First plug in the port */ > chamelium_port_set_edid(data->chamelium, port, edid_id); > chamelium_plug(data->chamelium, port); > - wait_for_connector(data, port, DRM_MODE_CONNECTED); > + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); > > igt_flush_hotplugs(mon); > > @@ -352,7 +354,7 @@ prepare_output(data_t *data, > chamelium_port_set_edid(data->chamelium, port, data- > >edid_id); > > chamelium_plug(data->chamelium, port); > - wait_for_connector(data, port, DRM_MODE_CONNECTED); > + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); > > igt_display_init(display, data->drm_fd); > output = igt_output_from_connector(display, connector); > @@ -590,7 +592,7 @@ test_hpd_without_ddc(data_t *data, struct > chamelium_port *port) > chamelium_plug(data->chamelium, port); > > igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); > - igt_assert_eq(reprobe_connector(data, port), > DRM_MODE_CONNECTED); > + igt_assert_eq(update_connector(data, port, false), > DRM_MODE_CONNECTED); > > igt_cleanup_hotplug(mon); > }
On Tue, 2017-06-27 at 17:26 -0400, Lyude Paul wrote: > I -think- this one might be okay but I just got reminded of a rather > not-obvious part of the DRM spec for hotplugging, and I need to check > that this doesn't actually break that. will get back to you in a bit > with an actual review :) Martin was really expecting this to come around, so I hope it is in accordance with the DRM spec. At least, it works properly with i915 on SKL. Thanks for double-checking! > On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote: > > This renames the reprobe_connector function to update_connector and > > ensures that full reprobe of the connector is only done when really > > necessary (e.g. when changing the EDID). > > > > A full reprobe takes time and is not required for updating the > > connector > > state. Thus, this allows executing tests faster. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > > --- > > tests/chamelium.c | 34 ++++++++++++++++++---------------- > > 1 file changed, 18 insertions(+), 16 deletions(-) > > > > diff --git a/tests/chamelium.c b/tests/chamelium.c > > index b412c6a7..346018a5 100644 > > --- a/tests/chamelium.c > > +++ b/tests/chamelium.c > > @@ -103,13 +103,14 @@ require_connector_present(data_t *data, > > unsigned int type) > > } > > > > static drmModeConnection > > -reprobe_connector(data_t *data, struct chamelium_port *port) > > +update_connector(data_t *data, struct chamelium_port *port, bool > > reprobe) > > { > > drmModeConnector *connector; > > drmModeConnection status; > > > > - igt_debug("Reprobing %s...\n", > > chamelium_port_get_name(port)); > > - connector = chamelium_port_get_connector(data->chamelium, > > port, true); > > + igt_debug("Updating %s...\n", > > chamelium_port_get_name(port)); > > + connector = chamelium_port_get_connector(data->chamelium, > > port, > > + reprobe); > > igt_assert(connector); > > status = connector->connection; > > > > @@ -119,7 +120,7 @@ reprobe_connector(data_t *data, struct > > chamelium_port *port) > > > > static void > > wait_for_connector(data_t *data, struct chamelium_port *port, > > - drmModeConnection status) > > + drmModeConnection status, bool reprobe) > > { > > bool finished = false; > > > > @@ -132,7 +133,7 @@ wait_for_connector(data_t *data, struct > > chamelium_port *port, > > * that hpd events work in the event that hpd doesn't work > > on the system > > */ > > igt_until_timeout(HOTPLUG_TIMEOUT) { > > - if (reprobe_connector(data, port) == status) { > > + if (update_connector(data, port, reprobe) == status) > > { > > finished = true; > > return; > > } > > @@ -151,11 +152,12 @@ reset_state(data_t *data, struct chamelium_port > > *port) > > chamelium_reset(data->chamelium); > > > > if (port) { > > - wait_for_connector(data, port, > > DRM_MODE_DISCONNECTED); > > + wait_for_connector(data, port, > > DRM_MODE_DISCONNECTED, false); > > } else { > > for (p = 0; p < data->port_count; p++) { > > port = data->ports[p]; > > - wait_for_connector(data, port, > > DRM_MODE_DISCONNECTED); > > + wait_for_connector(data, port, > > DRM_MODE_DISCONNECTED, > > + false); > > } > > } > > } > > @@ -175,7 +177,7 @@ test_basic_hotplug(data_t *data, struct > > chamelium_port *port, int toggle_count) > > /* Check if we get a sysfs hotplug event */ > > chamelium_plug(data->chamelium, port); > > igt_assert(igt_hotplug_detected(mon, > > HOTPLUG_TIMEOUT)); > > - igt_assert_eq(reprobe_connector(data, port), > > + igt_assert_eq(update_connector(data, port, false), > > DRM_MODE_CONNECTED); > > > > igt_flush_hotplugs(mon); > > @@ -183,7 +185,7 @@ test_basic_hotplug(data_t *data, struct > > chamelium_port *port, int toggle_count) > > /* Now check if we get a hotplug from disconnection > > */ > > chamelium_unplug(data->chamelium, port); > > igt_assert(igt_hotplug_detected(mon, > > HOTPLUG_TIMEOUT)); > > - igt_assert_eq(reprobe_connector(data, port), > > + igt_assert_eq(update_connector(data, port, false), > > DRM_MODE_DISCONNECTED); > > } > > > > @@ -204,7 +206,7 @@ test_edid_read(data_t *data, struct > > chamelium_port *port, > > > > chamelium_port_set_edid(data->chamelium, port, edid_id); > > chamelium_plug(data->chamelium, port); > > - wait_for_connector(data, port, DRM_MODE_CONNECTED); > > + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); > > > > igt_assert(kmstest_get_property(data->drm_fd, connector- > > > connector_id, > > > > DRM_MODE_OBJECT_CONNECTOR, > > "EDID", NULL, > > @@ -247,13 +249,13 @@ try_suspend_resume_hpd(data_t *data, struct > > chamelium_port *port, > > > > igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); > > if (port) { > > - igt_assert_eq(reprobe_connector(data, port), > > connected ? > > + igt_assert_eq(update_connector(data, port, false), > > connected ? > > DRM_MODE_DISCONNECTED : > > DRM_MODE_CONNECTED); > > } else { > > for (p = 0; p < data->port_count; p++) { > > port = data->ports[p]; > > - igt_assert_eq(reprobe_connector(data, port), > > connected ? > > - DRM_MODE_DISCONNECTED : > > + igt_assert_eq(update_connector(data, port, > > false), > > + connected ? > > DRM_MODE_DISCONNECTED : > > DRM_MODE_CONNECTED); > > } > > > > @@ -317,7 +319,7 @@ test_suspend_resume_edid_change(data_t *data, > > struct chamelium_port *port, > > /* First plug in the port */ > > chamelium_port_set_edid(data->chamelium, port, edid_id); > > chamelium_plug(data->chamelium, port); > > - wait_for_connector(data, port, DRM_MODE_CONNECTED); > > + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); > > > > igt_flush_hotplugs(mon); > > > > @@ -352,7 +354,7 @@ prepare_output(data_t *data, > > chamelium_port_set_edid(data->chamelium, port, data- > > > edid_id); > > > > > > chamelium_plug(data->chamelium, port); > > - wait_for_connector(data, port, DRM_MODE_CONNECTED); > > + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); > > > > igt_display_init(display, data->drm_fd); > > output = igt_output_from_connector(display, connector); > > @@ -590,7 +592,7 @@ test_hpd_without_ddc(data_t *data, struct > > chamelium_port *port) > > chamelium_plug(data->chamelium, port); > > > > igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); > > - igt_assert_eq(reprobe_connector(data, port), > > DRM_MODE_CONNECTED); > > + igt_assert_eq(update_connector(data, port, false), > > DRM_MODE_CONNECTED); > > > > igt_cleanup_hotplug(mon); > > }
diff --git a/tests/chamelium.c b/tests/chamelium.c index b412c6a7..346018a5 100644 --- a/tests/chamelium.c +++ b/tests/chamelium.c @@ -103,13 +103,14 @@ require_connector_present(data_t *data, unsigned int type) } static drmModeConnection -reprobe_connector(data_t *data, struct chamelium_port *port) +update_connector(data_t *data, struct chamelium_port *port, bool reprobe) { drmModeConnector *connector; drmModeConnection status; - igt_debug("Reprobing %s...\n", chamelium_port_get_name(port)); - connector = chamelium_port_get_connector(data->chamelium, port, true); + igt_debug("Updating %s...\n", chamelium_port_get_name(port)); + connector = chamelium_port_get_connector(data->chamelium, port, + reprobe); igt_assert(connector); status = connector->connection; @@ -119,7 +120,7 @@ reprobe_connector(data_t *data, struct chamelium_port *port) static void wait_for_connector(data_t *data, struct chamelium_port *port, - drmModeConnection status) + drmModeConnection status, bool reprobe) { bool finished = false; @@ -132,7 +133,7 @@ wait_for_connector(data_t *data, struct chamelium_port *port, * that hpd events work in the event that hpd doesn't work on the system */ igt_until_timeout(HOTPLUG_TIMEOUT) { - if (reprobe_connector(data, port) == status) { + if (update_connector(data, port, reprobe) == status) { finished = true; return; } @@ -151,11 +152,12 @@ reset_state(data_t *data, struct chamelium_port *port) chamelium_reset(data->chamelium); if (port) { - wait_for_connector(data, port, DRM_MODE_DISCONNECTED); + wait_for_connector(data, port, DRM_MODE_DISCONNECTED, false); } else { for (p = 0; p < data->port_count; p++) { port = data->ports[p]; - wait_for_connector(data, port, DRM_MODE_DISCONNECTED); + wait_for_connector(data, port, DRM_MODE_DISCONNECTED, + false); } } } @@ -175,7 +177,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count) /* Check if we get a sysfs hotplug event */ chamelium_plug(data->chamelium, port); igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); - igt_assert_eq(reprobe_connector(data, port), + igt_assert_eq(update_connector(data, port, false), DRM_MODE_CONNECTED); igt_flush_hotplugs(mon); @@ -183,7 +185,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count) /* Now check if we get a hotplug from disconnection */ chamelium_unplug(data->chamelium, port); igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); - igt_assert_eq(reprobe_connector(data, port), + igt_assert_eq(update_connector(data, port, false), DRM_MODE_DISCONNECTED); } @@ -204,7 +206,7 @@ test_edid_read(data_t *data, struct chamelium_port *port, chamelium_port_set_edid(data->chamelium, port, edid_id); chamelium_plug(data->chamelium, port); - wait_for_connector(data, port, DRM_MODE_CONNECTED); + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); igt_assert(kmstest_get_property(data->drm_fd, connector->connector_id, DRM_MODE_OBJECT_CONNECTOR, "EDID", NULL, @@ -247,13 +249,13 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port, igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); if (port) { - igt_assert_eq(reprobe_connector(data, port), connected ? + igt_assert_eq(update_connector(data, port, false), connected ? DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED); } else { for (p = 0; p < data->port_count; p++) { port = data->ports[p]; - igt_assert_eq(reprobe_connector(data, port), connected ? - DRM_MODE_DISCONNECTED : + igt_assert_eq(update_connector(data, port, false), + connected ? DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED); } @@ -317,7 +319,7 @@ test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port, /* First plug in the port */ chamelium_port_set_edid(data->chamelium, port, edid_id); chamelium_plug(data->chamelium, port); - wait_for_connector(data, port, DRM_MODE_CONNECTED); + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); igt_flush_hotplugs(mon); @@ -352,7 +354,7 @@ prepare_output(data_t *data, chamelium_port_set_edid(data->chamelium, port, data->edid_id); chamelium_plug(data->chamelium, port); - wait_for_connector(data, port, DRM_MODE_CONNECTED); + wait_for_connector(data, port, DRM_MODE_CONNECTED, true); igt_display_init(display, data->drm_fd); output = igt_output_from_connector(display, connector); @@ -590,7 +592,7 @@ test_hpd_without_ddc(data_t *data, struct chamelium_port *port) chamelium_plug(data->chamelium, port); igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT)); - igt_assert_eq(reprobe_connector(data, port), DRM_MODE_CONNECTED); + igt_assert_eq(update_connector(data, port, false), DRM_MODE_CONNECTED); igt_cleanup_hotplug(mon); }
This renames the reprobe_connector function to update_connector and ensures that full reprobe of the connector is only done when really necessary (e.g. when changing the EDID). A full reprobe takes time and is not required for updating the connector state. Thus, this allows executing tests faster. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> --- tests/chamelium.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)