| Message ID | 20170705080435.26789-2-paul.kocialkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
NAK. You're right that these don't actually give us any advantage over just using CRCs and are just slower, however I left these tests in here moreso just so we had something to actually test the frame dumping functions so that we could avoid regressing them by accident since we're the only users of those functions right now. If I recall properly, isn't there a list of tests in igt's source that they use for determining which tests to run on the CI? I think a better solution would be to just disable this for CI runs, and maybe add some comments pointing out that this test is only really useful for developers making changes to the chamelium library. On Wed, 2017-07-05 at 11:04 +0300, Paul Kocialkowski wrote: > The frame dump tests provide no additional functionality over CRC > tests > and are considerably slower. Thus, these tests should be considered > as > poorer duplicates and removed. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > --- > tests/chamelium.c | 53 ------------------------------------------- > ---------- > 1 file changed, 53 deletions(-) > > diff --git a/tests/chamelium.c b/tests/chamelium.c > index 3fd2b02c..5cf8b3af 100644 > --- a/tests/chamelium.c > +++ b/tests/chamelium.c > @@ -496,53 +496,6 @@ test_display_crc_multiple(data_t *data, struct > chamelium_port *port) > } > > static void > -test_display_frame_dump(data_t *data, struct chamelium_port *port) > -{ > - igt_display_t display; > - igt_output_t *output; > - igt_plane_t *primary; > - struct igt_fb fb; > - struct chamelium_frame_dump *frame; > - drmModeModeInfo *mode; > - drmModeConnector *connector; > - int fb_id, i, j; > - > - reset_state(data, port); > - > - output = prepare_output(data, &display, port); > - connector = chamelium_port_get_connector(data->chamelium, > port, false); > - primary = igt_output_get_plane_type(output, > DRM_PLANE_TYPE_PRIMARY); > - igt_assert(primary); > - > - for (i = 0; i < connector->count_modes; i++) { > - mode = &connector->modes[i]; > - fb_id = igt_create_color_pattern_fb(data->drm_fd, > - mode->hdisplay, > mode->vdisplay, > - DRM_FORMAT_XRGB8 > 888, > - LOCAL_DRM_FORMAT > _MOD_NONE, > - 0, 0, 0, &fb); > - igt_assert(fb_id > 0); > - > - enable_output(data, port, output, mode, &fb); > - > - igt_debug("Reading frame dumps from > Chamelium...\n"); > - chamelium_capture(data->chamelium, port, 0, 0, 0, 0, > 5); > - for (j = 0; j < 5; j++) { > - frame = chamelium_read_captured_frame( > - data->chamelium, j); > - chamelium_assert_frame_eq(data->chamelium, > frame, &fb); > - chamelium_destroy_frame_dump(frame); > - } > - > - disable_output(data, port, output); > - igt_remove_fb(data->drm_fd, &fb); > - } > - > - drmModeFreeConnector(connector); > - igt_display_fini(&display); > -} > - > -static void > test_hpd_without_ddc(data_t *data, struct chamelium_port *port) > { > struct udev_monitor *mon = igt_watch_hotplug(); > @@ -695,9 +648,6 @@ igt_main > > connector_subtest("dp-crc-multiple", DisplayPort) > test_display_crc_multiple(&data, port); > - > - connector_subtest("dp-frame-dump", DisplayPort) > - test_display_frame_dump(&data, port); > } > > igt_subtest_group { > @@ -752,9 +702,6 @@ igt_main > > connector_subtest("hdmi-crc-multiple", HDMIA) > test_display_crc_multiple(&data, port); > - > - connector_subtest("hdmi-frame-dump", HDMIA) > - test_display_frame_dump(&data, port); > } > > igt_subtest_group {
On 05/07/17 23:53, Lyude Paul wrote: > NAK. You're right that these don't actually give us any advantage over > just using CRCs and are just slower, however I left these tests in here > moreso just so we had something to actually test the frame dumping > functions so that we could avoid regressing them by accident since > we're the only users of those functions right now. > > If I recall properly, isn't there a list of tests in igt's source that > they use for determining which tests to run on the CI? I think a better > solution would be to just disable this for CI runs, and maybe add some > comments pointing out that this test is only really useful for > developers making changes to the chamelium library. The general direction of IGT is to make sure that all tests are being executed by default, very much alike piglit. I however do understand what you mean and I would then propose that we start defining naming conventions that allow filtering tests based on how level directive. For instance, we could have every test that need it export a slow and fast version, so as to easily be able to filter stuff out. I guess we need to start a separate thread for that. As for this particular test, I would say that it could be simplified to prevent downloading so many images (all the exported resolutions * all the connected connectors). We could instead test one resolution per connector and call it good-enough. By better testing stuff, we just increase the likeliness of the test being completely blacklisted due to its excessive execution time. In this case, the better is the enemy of good.
On Wed, 2017-07-05 at 16:53 -0400, Lyude Paul wrote: > NAK. You're right that these don't actually give us any advantage over > just using CRCs and are just slower, however I left these tests in > here > moreso just so we had something to actually test the frame dumping > functions so that we could avoid regressing them by accident since > we're the only users of those functions right now. Also, note that the VGA testing patch (that I just sent to the list) does make use of these functions (maybe not all of them though), since using the CRC is not possible for VGA. > If I recall properly, isn't there a list of tests in igt's source that > they use for determining which tests to run on the CI? I think a > better > solution would be to just disable this for CI runs, and maybe add some > comments pointing out that this test is only really useful for > developers making changes to the chamelium library. > > On Wed, 2017-07-05 at 11:04 +0300, Paul Kocialkowski wrote: > > The frame dump tests provide no additional functionality over CRC > > tests > > and are considerably slower. Thus, these tests should be considered > > as > > poorer duplicates and removed. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > > --- > > tests/chamelium.c | 53 ------------------------------------------- > > ---------- > > 1 file changed, 53 deletions(-) > > > > diff --git a/tests/chamelium.c b/tests/chamelium.c > > index 3fd2b02c..5cf8b3af 100644 > > --- a/tests/chamelium.c > > +++ b/tests/chamelium.c > > @@ -496,53 +496,6 @@ test_display_crc_multiple(data_t *data, struct > > chamelium_port *port) > > } > > > > static void > > -test_display_frame_dump(data_t *data, struct chamelium_port *port) > > -{ > > - igt_display_t display; > > - igt_output_t *output; > > - igt_plane_t *primary; > > - struct igt_fb fb; > > - struct chamelium_frame_dump *frame; > > - drmModeModeInfo *mode; > > - drmModeConnector *connector; > > - int fb_id, i, j; > > - > > - reset_state(data, port); > > - > > - output = prepare_output(data, &display, port); > > - connector = chamelium_port_get_connector(data->chamelium, > > port, false); > > - primary = igt_output_get_plane_type(output, > > DRM_PLANE_TYPE_PRIMARY); > > - igt_assert(primary); > > - > > - for (i = 0; i < connector->count_modes; i++) { > > - mode = &connector->modes[i]; > > - fb_id = igt_create_color_pattern_fb(data->drm_fd, > > - mode->hdisplay, > > mode->vdisplay, > > - DRM_FORMAT_XRGB > > 8 > > 888, > > - LOCAL_DRM_FORMA > > T > > _MOD_NONE, > > - 0, 0, 0, &fb); > > - igt_assert(fb_id > 0); > > - > > - enable_output(data, port, output, mode, &fb); > > - > > - igt_debug("Reading frame dumps from > > Chamelium...\n"); > > - chamelium_capture(data->chamelium, port, 0, 0, 0, > > 0, > > 5); > > - for (j = 0; j < 5; j++) { > > - frame = chamelium_read_captured_frame( > > - data->chamelium, j); > > - chamelium_assert_frame_eq(data->chamelium, > > frame, &fb); > > - chamelium_destroy_frame_dump(frame); > > - } > > - > > - disable_output(data, port, output); > > - igt_remove_fb(data->drm_fd, &fb); > > - } > > - > > - drmModeFreeConnector(connector); > > - igt_display_fini(&display); > > -} > > - > > -static void > > test_hpd_without_ddc(data_t *data, struct chamelium_port *port) > > { > > struct udev_monitor *mon = igt_watch_hotplug(); > > @@ -695,9 +648,6 @@ igt_main > > > > connector_subtest("dp-crc-multiple", DisplayPort) > > test_display_crc_multiple(&data, port); > > - > > - connector_subtest("dp-frame-dump", DisplayPort) > > - test_display_frame_dump(&data, port); > > } > > > > igt_subtest_group { > > @@ -752,9 +702,6 @@ igt_main > > > > connector_subtest("hdmi-crc-multiple", HDMIA) > > test_display_crc_multiple(&data, port); > > - > > - connector_subtest("hdmi-frame-dump", HDMIA) > > - test_display_frame_dump(&data, port); > > } > > > > igt_subtest_group {
diff --git a/tests/chamelium.c b/tests/chamelium.c index 3fd2b02c..5cf8b3af 100644 --- a/tests/chamelium.c +++ b/tests/chamelium.c @@ -496,53 +496,6 @@ test_display_crc_multiple(data_t *data, struct chamelium_port *port) } static void -test_display_frame_dump(data_t *data, struct chamelium_port *port) -{ - igt_display_t display; - igt_output_t *output; - igt_plane_t *primary; - struct igt_fb fb; - struct chamelium_frame_dump *frame; - drmModeModeInfo *mode; - drmModeConnector *connector; - int fb_id, i, j; - - reset_state(data, port); - - output = prepare_output(data, &display, port); - connector = chamelium_port_get_connector(data->chamelium, port, false); - primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); - igt_assert(primary); - - for (i = 0; i < connector->count_modes; i++) { - mode = &connector->modes[i]; - fb_id = igt_create_color_pattern_fb(data->drm_fd, - mode->hdisplay, mode->vdisplay, - DRM_FORMAT_XRGB8888, - LOCAL_DRM_FORMAT_MOD_NONE, - 0, 0, 0, &fb); - igt_assert(fb_id > 0); - - enable_output(data, port, output, mode, &fb); - - igt_debug("Reading frame dumps from Chamelium...\n"); - chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 5); - for (j = 0; j < 5; j++) { - frame = chamelium_read_captured_frame( - data->chamelium, j); - chamelium_assert_frame_eq(data->chamelium, frame, &fb); - chamelium_destroy_frame_dump(frame); - } - - disable_output(data, port, output); - igt_remove_fb(data->drm_fd, &fb); - } - - drmModeFreeConnector(connector); - igt_display_fini(&display); -} - -static void test_hpd_without_ddc(data_t *data, struct chamelium_port *port) { struct udev_monitor *mon = igt_watch_hotplug(); @@ -695,9 +648,6 @@ igt_main connector_subtest("dp-crc-multiple", DisplayPort) test_display_crc_multiple(&data, port); - - connector_subtest("dp-frame-dump", DisplayPort) - test_display_frame_dump(&data, port); } igt_subtest_group { @@ -752,9 +702,6 @@ igt_main connector_subtest("hdmi-crc-multiple", HDMIA) test_display_crc_multiple(&data, port); - - connector_subtest("hdmi-frame-dump", HDMIA) - test_display_frame_dump(&data, port); } igt_subtest_group {
The frame dump tests provide no additional functionality over CRC tests and are considerably slower. Thus, these tests should be considered as poorer duplicates and removed. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> --- tests/chamelium.c | 53 ----------------------------------------------------- 1 file changed, 53 deletions(-)