On Thu, 2017-07-06 at 14:31 +0300, Paul Kocialkowski wrote: > Hi, > > On Wed, 2017-07-05 at 17:44 -0400, 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@xxxxxxxxxxxxxx > > > m> > > > --- > > > 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 > > That definitely makes sense. By the way, what approach would you > recommend for > thishandling? Mupuf was suggesting to have a common configuration > structure > instead of declaring either global variables or static ones with > getters/setters. This is probably becoming more and more of a > necessity as we > add more common config options. > > However, I think we should still allow specific parts of IGT to do the > parsing > themselves (especially in the case of chamelium) so that the common > config > structure only has common fields (and does not, for instance, contain > the > chamelium port configuration). > > > > * > > > * # 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, > > > + "Chame > > > liu > > > m", > > > + "Frame > > > Dum > > > pPath", > > > + &erro > > > r); > > > + > > > 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; > > > + > > > + 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 > > * 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_fram > > > e_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_XR > > > GB8 > > > 888, > > > - LOCAL_DRM_FOR > > > MAT > > > _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_fram > > > e_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, > > > > > > + fram > > > e, > > > path); > > > > > > - expected_crc = > > > chamelium_calculate_fb_crc_result(fb_crc); > > > + chamelium_destroy_frame_dump(fram > > > e); > > > + } > > > > > > - 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. > > I don't think it makes so much sense to do this in the CRC checking > functions, > just because they are semantically expected to do one thing: CRC > checking, and > doing frame dumps seems like going overboard. > > On the other hand, I do agree that the dumping and saving part can and > should be > made common, but maybe as a separate function. So that would be two > calls for > the tests: one to check the crc and one to dump and save the frame. A strong case to support this vision: in VGA frame testing, we have already dumped the frame and don't do CRC checking, yet we also need to save the frames if there is a mismatch. It would be a shame that the dumping logic becomes part of the CRC functions, since that would mean duplicating that logic for VGA testing (as it's currently done in the version I just sent out). In spite of that, I think having a common function, called from the test itself is probably the best approach here. > I have also duplicated that logic in upcoming VGA frame testing, so > there is definitely a need for less duplication. > > > 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. > > Fair enough, it just felt weird to commit two functions that were > nearly the > exact same, but I have no problem with doing this in two separate > patches. > > > > > > > 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
