On Thu, 2017-07-06 at 14:35 +0300, Paul Kocialkowski wrote: > 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@linux.intel > > > > .com> > > > > --- > > > > 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, > > > > + "Ch > > > > ameliu > > > > m", > > > > + "Fr > > > > ameDum > > > > pPath", > > > > + &e > > > > rror); > > > > + > > > > 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? It's just so that we can print more detailed debugging info that actually notes which part of the CRC didn't match. This sounds a little useless, but sometimes you can actually tell what's going on by looking at how much of the CRC didn't match. For instance, when I was trying to figure out the (I didn't know this was the cause at the time) ESD issues I was hitting with DisplayPort on the chamelium, the chromeos guys were immediately able to tell it was only a single pixel that was off because only one section of the CRC didn't match, while the rest of it did. > > > > * 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_f > > > > rame_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_f > > > > rame_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(d > > > > ata- > > > > > chamelium, > > > > > > > > + f > > > > rame, > > > > path); > > > > > > > > - expected_crc = > > > > chamelium_calculate_fb_crc_result(fb_crc); > > > > + chamelium_destroy_frame_dump(f > > > > rame); > > > > + } > > > > > > > > - 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 { -- Cheers, Lyude _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
