[Intel-gfx] [PATCH i-g-t v3 4/4] chamelium: Dump obtained and reference frames to png on crc error
Martin Peres
martin.peres at linux.intel.com
Thu Jul 6 07:41:17 UTC 2017
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,
>> + "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;
>> +
>> + 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.
> * 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 {
More information about the Intel-gfx
mailing list