On Thu, 2017-07-06 at 10:41 +0300, Martin Peres wrote: > On 06/07/17 00:44, Lyude Paul wrote: > > On Wed, 2017-07-05 at 11:04 +0300, Paul Kocialkowski wrote: > > > When a CRC comparison error occurs, it is quite useful to get a dump > > > of both the frame obtained from the chamelium and the reference in > > > order > > > to compare them. > > > > > > This implements the frame dump, with a configurable path that enables > > > the use of this feature. > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxxxxxx> > > > --- > > > lib/igt_chamelium.c | 21 +++++++++++ > > > lib/igt_chamelium.h | 1 + > > > lib/igt_debugfs.c | 20 ++++++++++ > > > lib/igt_debugfs.h | 1 + > > > tests/chamelium.c | 104 ++++++++++++++++++++-------------------- > > > ------------ > > > 5 files changed, 82 insertions(+), 65 deletions(-) > > > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c > > > index ef51ef68..9aca6842 100644 > > > --- a/lib/igt_chamelium.c > > > +++ b/lib/igt_chamelium.c > > > @@ -57,6 +57,7 @@ > > > * |[<!-- language="plain" --> > > > * [Chamelium] > > > * URL=http://chameleon:9992 # The URL used for connecting to > > > the Chamelium's RPC server > > > + * FrameDumpPath=/tmp # The path to dump frames that fail > > > comparison checks > > > > While no one else really cares about creating frame dumps yet, it's > > possible someone else may in the future if we ever end up taking more > > advantage of automated testing systems like this. So I'd stick this in > > the generic non-chamelium specific section in the config file > > > > > * > > > * # The rest of the sections are used for defining connector > > > mappings. > > > * # This is required so any tests using the Chamelium know > > > which connector > > > @@ -115,11 +116,26 @@ struct chamelium { > > > struct chamelium_edid *edids; > > > struct chamelium_port *ports; > > > int port_count; > > > + > > > + char *frame_dump_path; > > > }; > > > > > > static struct chamelium *cleanup_instance; > > > > > > /** > > > + * chamelium_get_frame_dump_path: > > > + * @chamelium: The Chamelium instance to use > > > + * > > > + * Retrieves the path to dump frames to. > > > + * > > > + * Returns: a string with the frame dump path > > > + */ > > > +char *chamelium_get_frame_dump_path(struct chamelium *chamelium) > > > +{ > > > + return chamelium->frame_dump_path; > > > +} > > > + > > > +/** > > > * chamelium_get_ports: > > > * @chamelium: The Chamelium instance to use > > > * @count: Where to store the number of ports > > > @@ -1338,6 +1354,11 @@ static bool chamelium_read_config(struct > > > chamelium *chamelium, int drm_fd) > > > return false; > > > } > > > > > > + chamelium->frame_dump_path = > > > g_key_file_get_string(igt_key_file, > > > + "Chameliu > > > m", > > > + "FrameDum > > > pPath", > > > + &error); > > > + > > > return chamelium_read_port_mappings(chamelium, drm_fd); > > > } > > > > > > diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h > > > index 908e03d1..aa881971 100644 > > > --- a/lib/igt_chamelium.h > > > +++ b/lib/igt_chamelium.h > > > @@ -42,6 +42,7 @@ struct chamelium *chamelium_init(int drm_fd); > > > void chamelium_deinit(struct chamelium *chamelium); > > > void chamelium_reset(struct chamelium *chamelium); > > > > > > +char *chamelium_get_frame_dump_path(struct chamelium *chamelium); > > > struct chamelium_port **chamelium_get_ports(struct chamelium > > > *chamelium, > > > int *count); > > > unsigned int chamelium_port_get_type(const struct chamelium_port > > > *port); > > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c > > > index 80f25c61..dcb4e0a7 100644 > > > --- a/lib/igt_debugfs.c > > > +++ b/lib/igt_debugfs.c > > > @@ -282,6 +282,26 @@ bool igt_debugfs_search(int device, const char > > > *filename, const char *substring) > > > */ > > > > > > /** > > > + * igt_check_crc_equal: > > > + * @a: first pipe CRC value > > > + * @b: second pipe CRC value > > > + * > > > + * Compares two CRC values and return whether they match. > > > + * > > > + * Returns: A boolean indicating whether the CRC values match > > > + */ > > > +bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b) > > > +{ > > > + int i; > > I would like to see: > > if (a->n_words != b->n_words) > return false; Very good suggestion! I'll take that in in the next revision. > > > + > > > + for (i = 0; i < a->n_words; i++) > > > + if (a->crc[i] != b->crc[i]) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > > Make this a separate patch, and instead of having another function do > > the CRC calculations just have something like this: > > > > * static int igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t > > *b): returns the index of the first CRC mismatch, 0 if none was > > found > > Sounds good, but no error should return -1, as to differentiate if the > first word was already different. I don't understand the point of getting the index of the CRC mismatch at all. The only relevant information here should be whether it matches or not (which would be covered by igt_check_crc_equal). Can you ellaborate on this? > > * bool igt_check_crc_equal(): uses igt_find_crc_mismatch() to figure > > out if anything mismatched, and return true if something did (as > > well, also spit out some debugging info mentioning there was a > > mismatch) > > * void igt_assert_crc_equal(): uses igt_find_crc_mismatch() to figure > > out if anything mismatched. If the assertion fails, use > > igt_assert_eq() on the mismatched crc so we still get a useful error > > message on CRC failures. > > > > There isn't much code required to actually compare CRCs, however I'd > > still prefer only having one function doing the actual comparison logic > > here so we only have one piece of code to update if we need to make > > changes to it in the future. > > > > Mupuf, your opinion on this? ^ > > > > > +/** > > > * igt_assert_crc_equal: > > > * @a: first pipe CRC value > > > * @b: second pipe CRC value > > > diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h > > > index 7b846a83..2695cbda 100644 > > > --- a/lib/igt_debugfs.h > > > +++ b/lib/igt_debugfs.h > > > @@ -113,6 +113,7 @@ enum intel_pipe_crc_source { > > > INTEL_PIPE_CRC_SOURCE_MAX, > > > }; > > > > > > +bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b); > > > void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b); > > > char *igt_crc_to_string(igt_crc_t *crc); > > > > > > diff --git a/tests/chamelium.c b/tests/chamelium.c > > > index 5cf8b3af..3d95c05c 100644 > > > --- a/tests/chamelium.c > > > +++ b/tests/chamelium.c > > > @@ -381,7 +381,7 @@ disable_output(data_t *data, > > > } > > > > > > static void > > > -test_display_crc_single(data_t *data, struct chamelium_port *port) > > > +test_display_crc(data_t *data, struct chamelium_port *port, int > > > count) > > > { > > > igt_display_t display; > > > igt_output_t *output; > > > @@ -390,9 +390,14 @@ test_display_crc_single(data_t *data, struct > > > chamelium_port *port) > > > igt_crc_t *expected_crc; > > > struct chamelium_fb_crc *fb_crc; > > > struct igt_fb fb; > > > + struct chamelium_frame_dump *frame; > > > drmModeModeInfo *mode; > > > drmModeConnector *connector; > > > - int fb_id, i; > > > + int fb_id, i, j, captured_frame_count; > > > + const char *connector_name; > > > + char *frame_dump_path; > > > + char path[PATH_MAX]; > > > + bool eq; > > > > > > reset_state(data, port); > > > > > > @@ -401,6 +406,9 @@ test_display_crc_single(data_t *data, struct > > > chamelium_port *port) > > > primary = igt_output_get_plane_type(output, > > > DRM_PLANE_TYPE_PRIMARY); > > > igt_assert(primary); > > > > > > + connector_name = kmstest_connector_type_str(connector- > > > > connector_type); > > > > > > + frame_dump_path = chamelium_get_frame_dump_path(data- > > > > chamelium); > > > > > > + > > > for (i = 0; i < connector->count_modes; i++) { > > > mode = &connector->modes[i]; > > > fb_id = igt_create_color_pattern_fb(data->drm_fd, > > > @@ -415,74 +423,40 @@ test_display_crc_single(data_t *data, struct > > > chamelium_port *port) > > > > > > enable_output(data, port, output, mode, &fb); > > > > > > - igt_debug("Testing single CRC fetch\n"); > > > - > > > - crc = chamelium_get_crc_for_area(data->chamelium, > > > port, > > > - 0, 0, 0, 0); > > > - > > > - expected_crc = > > > chamelium_calculate_fb_crc_result(fb_crc); > > > - > > > - igt_assert_crc_equal(crc, expected_crc); > > > - free(expected_crc); > > > - free(crc); > > > - > > > - disable_output(data, port, output); > > > - igt_remove_fb(data->drm_fd, &fb); > > > - } > > > - > > > - drmModeFreeConnector(connector); > > > - igt_display_fini(&display); > > > -} > > > - > > > -static void > > > -test_display_crc_multiple(data_t *data, struct chamelium_port *port) > > > -{ > > > - igt_display_t display; > > > - igt_output_t *output; > > > - igt_plane_t *primary; > > > - igt_crc_t *crc; > > > - igt_crc_t *expected_crc; > > > - struct chamelium_fb_crc *fb_crc; > > > - struct igt_fb fb; > > > - drmModeModeInfo *mode; > > > - drmModeConnector *connector; > > > - int fb_id, i, j, captured_frame_count; > > > + chamelium_capture(data->chamelium, port, 0, 0, 0, 0, > > > count); > > > + crc = chamelium_read_captured_crcs(data->chamelium, > > > + &captured_frame_c > > > ount); > > > > > > - reset_state(data, port); > > > + igt_assert(captured_frame_count == count); > > > > > > - 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); > > > + igt_debug("Captured %d frames\n", > > > captured_frame_count); > > > > > > - 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); > > > + expected_crc = > > > chamelium_calculate_fb_crc_result(fb_crc); > > > > > > - fb_crc = chamelium_calculate_fb_crc_launch(data- > > > > drm_fd, &fb); > > > > > > + for (j = 0; j < captured_frame_count; j++) { > > > + eq = igt_check_crc_equal(&crc[j], > > > expected_crc); > > > + if (!eq && frame_dump_path) { > > > + frame = > > > chamelium_read_captured_frame(data->chamelium, > > > + > > > j); > > > > > > - enable_output(data, port, output, mode, &fb); > > > + igt_debug("Dumping reference and > > > chamelium frames to %s...\n", > > > + frame_dump_path); > > > > > > - /* We want to keep the display running for a little > > > bit, since > > > - * there's always the potential the driver isn't > > > able to keep > > > - * the display running properly for very long > > > - */ > > > - chamelium_capture(data->chamelium, port, 0, 0, 0, 0, > > > 3); > > > - crc = chamelium_read_captured_crcs(data->chamelium, > > > - &captured_frame_c > > > ount); > > > + snprintf(path, PATH_MAX, "%s/frame- > > > reference-%s.png", > > > + frame_dump_path, > > > connector_name); > > > + igt_write_fb_to_png(data->drm_fd, > > > &fb, path); > > > > > > - igt_debug("Captured %d frames\n", > > > captured_frame_count); > > > + snprintf(path, PATH_MAX, "%s/frame- > > > chamelium-%s.png", > > > + frame_dump_path, > > > connector_name); > > > + chamelium_write_frame_to_png(data- > > > > chamelium, > > > > > > + frame, > > > path); > > > > > > - expected_crc = > > > chamelium_calculate_fb_crc_result(fb_crc); > > > + chamelium_destroy_frame_dump(frame); > > > + } > > > > > > - for (j = 0; j < captured_frame_count; j++) > > > - igt_assert_crc_equal(&crc[j], expected_crc); > > > + igt_fail_on_f(!eq, > > > + "Chamelium frame CRC mismatch > > > with reference\n"); > > > + } > > > > There's lots of potential here for copy pasta to form in the future, > > since the API here puts a lot of work on the caller to set things up > > for frame dumping. IMO, it would be worth it to teach the CRC checking > > functions to automatically do frame dumps on mismatch if the CRC source > > supports it. This will save us from having to have separate frame dump > > APIs in the future if we ever end up adding support for other kinds of > > automated test equipment. > > > > As well, I like how you removed the redundancy between > > test_display_crc_single() and test_display_crc_multiple(). However > > since those are somewhat unrelated changes to the code path for these > > tests it would be better to have that re-factoring as a separate patch > > so as to make it easier for anyone who might need to bisect this code > > in the future. > > > > > > > > free(expected_crc); > > > free(crc); > > > @@ -644,10 +618,10 @@ igt_main > > > edid_id, > > > alt_edid_id); > > > > > > connector_subtest("dp-crc-single", DisplayPort) > > > - test_display_crc_single(&data, port); > > > + test_display_crc(&data, port, 1); > > > > > > connector_subtest("dp-crc-multiple", DisplayPort) > > > - test_display_crc_multiple(&data, port); > > > + test_display_crc(&data, port, 3); > > > } > > > > > > igt_subtest_group { > > > @@ -698,10 +672,10 @@ igt_main > > > edid_id, > > > alt_edid_id); > > > > > > connector_subtest("hdmi-crc-single", HDMIA) > > > - test_display_crc_single(&data, port); > > > + test_display_crc(&data, port, 1); > > > > > > connector_subtest("hdmi-crc-multiple", HDMIA) > > > - test_display_crc_multiple(&data, port); > > > + test_display_crc(&data, port, 3); > > > } > > > > > > igt_subtest_group { -- Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxxxxxx> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, FInland _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
