[Intel-gfx] [PATCH i-g-t v3 4/4] chamelium: Dump obtained and reference frames to png on crc error
Paul Kocialkowski
paul.kocialkowski at linux.intel.com
Thu Jul 6 13:33:51 UTC 2017
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 at linux.intel.co
> > > 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 at linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
More information about the Intel-gfx
mailing list