[Intel-gfx] [PATCH i-g-t v3 4/4] chamelium: Dump obtained and reference frames to png on crc error
Lyude Paul
lyude at redhat.com
Thu Jul 6 22:23:41 UTC 2017
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 at 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
More information about the Intel-gfx
mailing list