[i-g-t,v3,4/4] chamelium: Dump obtained and reference frames to png on crc error

Commit Message

Paul Kocialkowki July 5, 2017, 8:04 a.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Lyude Paul July 5, 2017, 9:44 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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;
> +
> +	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_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 {

Martin Peres July 6, 2017, 7:41 a.m. UTC | #2

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 <[e-mail]>
>> ---
>>   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 {

Paul Kocialkowki July 6, 2017, 11:31 a.m. UTC | #3

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 <[e-mail]>
> > ---
> >  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,
> > +							   "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;
> > +
> > +	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_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.

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.

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 Kocialkowki July 6, 2017, 11:35 a.m. UTC | #4

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 <[e-mail]>
> > > ---
> > >   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;

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?

> >   * 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 {

Paul Kocialkowki July 6, 2017, 1:33 p.m. UTC | #5

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 <[e-mail]
> > > 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 {

Lyude Paul July 6, 2017, 9:57 p.m. UTC | #6

--snip--
(also sorry this one took a while to get to, had to do a lot of
thinking because I never really solved the problems mentioned here when
I tried working on this...)

On Thu, 2017-07-06 at 16:33 +0300, Paul Kocialkowski wrote:
> On Thu, 2017-07-06 at 14:31 +0300, Paul Kocialkowski wrote:
> > > 
> > > 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).
That is a good point, but there's some things I think you might want to
consider. Mainly that in a test that passes, we of course don't write
any framedumps back to the disk since nothing failed. IMO, I would
-think- that we care a bit more about the performance hit that happens
on passing tests vs. failing tests, since tests aren't really supposed
to fail under ideal conditions anyway. Might be better to verify with
the mupuf and the other people actually running Intel's CI though,
since I'm not one of them.

As well, one advantage we do have here from the chamelium end is that
you can only really be screen grabbing from one port at a time. So you
could actually just track stuff internally in the igt_chamelium API and
when a user tries to download a framedump that we've already
downloaded, we can just hand them back a cached copy of it.

> 
> In spite of that, I think having a common function, called from the
> test
> itself is probably the best approach here.
Not sure if I misspoke here but I didn't mean to imply that I'm against
having functions for doing frame dumping exposed to the callers. I had
already figured there'd probably be situations where just having the
CRC checking do the frame dumping wouldn't be enough.

This being said though, your viewpoint does make me realize it might
not be a great idea to do autoframe dumping in -all- crc checking
functions necessarily, but also makes me realize that this might even
be a requirement if we still want to try keeping around
igt_assert_crc_equal() and not just replace it outright with a function
that doesn't fail the whole test (if we fail the test, there isn't
really a way we can do a framedump from it afterwards). So I would
think we can at least exclude igt_check_crc_equal() from doing
automatic framedumping, but I still think it would be a good idea to
implement igt_assert_crc_equal().

As for the what you're talking about, e.g. doing frame dump comparisons
on VGA, I think the solution might be not to make any of the code for
doing the actual frame comparisons chamelium specific either (except
maybe for the part where we trim the framebuffer we get so it only
contains the actual image dump).

So how about this: let's introduce a generic frame comparison API using
the code you've already written for doing this on VGA with the
chamelium. Make it part of the igt library, and have it just accept
normal pixman images and perform fuzzy comparisons between them. In
doing that, we can introduce a generic dump-frames-on-error API through
there much more easily.

My big aim here is just to make it so that people using igt don't have
to do anything to get frame dumping in their tests, it just "works".

> 
> > 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_i
> > > > d,
> > > > 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_i
> > > > d,
> > > > 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 {

Lyude Paul July 6, 2017, 10:23 p.m. UTC | #7

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 <[e-mail]
> > > > .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 {

Paul Kocialkowki July 10, 2017, 10:12 a.m. UTC | #8

On Thu, 2017-07-06 at 18:23 -0400, Lyude Paul wrote:
> 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 <[e-mail]
> > > > > l
> > > > > .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,
> > > > > +							   "C
> > > > > h
> > > > > ameliu
> > > > > m",
> > > > > +							   "F
> > > > > r
> > > > > 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.

Okay, thanks for the explanation. I understand the intent here and
definitely agree that it would be useful. However, note that
igt_assert_crc_equal is already implemented and calls igt_assert_eq_u32
directly, which will print an error on mismatch.

So I wonder if it's really worth having a separate function
(igt_find_crc_mismatch) only for getting the index. We could just keep
igt_assert_crc_equal as it is and implement igt_check_crc_equal with
similar debug output. That is a duplication of the logic, but it's so
simple that I don't think it's really a problem.

With that in mind, if you still really prefer that we use a separate
function and rewrite igt_assert_crc_equal, then I will do that.

> > > >   * 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_FORMA
> > > > > T
> > > > > _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 {

Paul Kocialkowki July 10, 2017, 10:27 a.m. UTC | #9

On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
> --snip--
> (also sorry this one took a while to get to, had to do a lot of
> thinking because I never really solved the problems mentioned here
> when
> I tried working on this...)
> 
> On Thu, 2017-07-06 at 16:33 +0300, Paul Kocialkowski wrote:
> > On Thu, 2017-07-06 at 14:31 +0300, Paul Kocialkowski wrote:
> > > > 
> > > > 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).
> 
> That is a good point, but there's some things I think you might want
> to
> consider. Mainly that in a test that passes, we of course don't write
> any framedumps back to the disk since nothing failed. IMO, I would
> -think- that we care a bit more about the performance hit that happens
> on passing tests vs. failing tests, since tests aren't really supposed
> to fail under ideal conditions anyway. Might be better to verify with
> the mupuf and the other people actually running Intel's CI though,
> since I'm not one of them.
> 
> As well, one advantage we do have here from the chamelium end is that
> you can only really be screen grabbing from one port at a time. So you
> could actually just track stuff internally in the igt_chamelium API
> and
> when a user tries to download a framedump that we've already
> downloaded, we can just hand them back a cached copy of it.

Either way, it is definitely okay to take the time to dump the frame
when a mismatch occurs if we don't have it already (in the CRC case).

> > 
> > In spite of that, I think having a common function, called from the
> > test
> > itself is probably the best approach here.
> 
> Not sure if I misspoke here but I didn't mean to imply that I'm
> against
> having functions for doing frame dumping exposed to the callers. I had
> already figured there'd probably be situations where just having the
> CRC checking do the frame dumping wouldn't be enough.
> 
> This being said though, your viewpoint does make me realize it might
> not be a great idea to do autoframe dumping in -all- crc checking
> functions necessarily, but also makes me realize that this might even
> be a requirement if we still want to try keeping around
> igt_assert_crc_equal() and not just replace it outright with a
> function
> that doesn't fail the whole test (if we fail the test, there isn't
> really a way we can do a framedump from it afterwards). So I would
> think we can at least exclude igt_check_crc_equal() from doing
> automatic framedumping, but I still think it would be a good idea to
> implement igt_assert_crc_equal().

igt_assert_crc_equal already exists and is used by many other tests, so
we really cannot embed the frame dumping logic there, since these tests
have nothing to do with the chamelium. That's another reason to really
keep the frame dump and crc comparison logic separate.

> As for the what you're talking about, e.g. doing frame dump
> comparisons
> on VGA, I think the solution might be not to make any of the code for
> doing the actual frame comparisons chamelium specific either (except
> maybe for the part where we trim the framebuffer we get so it only
> contains the actual image dump).
> 
> So how about this: let's introduce a generic frame comparison API
> using
> the code you've already written for doing this on VGA with the
> chamelium. Make it part of the igt library, and have it just accept
> normal pixman images and perform fuzzy comparisons between them. In
> doing that, we can introduce a generic dump-frames-on-error API
> through
> there much more easily.
> 
> My big aim here is just to make it so that people using igt don't have
> to do anything to get frame dumping in their tests, it just "works".

I totally agree with this direction.

What I suggest we should do is the following:
* keep CRC functions (igt_assert_crc_equal and igt_check_crc_equal)
common and fully independent from the frame dumping logic, either with a
common crc mismatch detection logic or not (because it's so simple)
* have common frame dump functions that just take a cairo surface and
handle filenames and actually writing the frames
* have the analogue frame detection code made common
* have two assert-style chamnelium-specific wrappers to link frame
comparison (either CRC or analogue) and frame dumping on failure while
still ending with an assert; the CRC fashion would be in charge of
dumping the frame on failure while the frame would be provided to the
analogue comparison fashion.

I will prepare patches in this direction so that you can get a more
concrete idea and we can follow-up the discussion on that basis.

> > > 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_
> > > > > i
> > > > > d,
> > > > > 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_
> > > > > i
> > > > > d,
> > > > > 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 Kocialkowki July 10, 2017, 10:31 a.m. UTC | #10

On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
> --snip--
> (also sorry this one took a while to get to, had to do a lot of
> thinking because I never really solved the problems mentioned here
> when
> I tried working on this...)
> 
> On Thu, 2017-07-06 at 16:33 +0300, Paul Kocialkowski wrote:
> > On Thu, 2017-07-06 at 14:31 +0300, Paul Kocialkowski wrote:
> > > > 
> > > > 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).
> 
> That is a good point, but there's some things I think you might want
> to
> consider. Mainly that in a test that passes, we of course don't write
> any framedumps back to the disk since nothing failed. IMO, I would
> -think- that we care a bit more about the performance hit that happens
> on passing tests vs. failing tests, since tests aren't really supposed
> to fail under ideal conditions anyway. Might be better to verify with
> the mupuf and the other people actually running Intel's CI though,
> since I'm not one of them.
> 
> As well, one advantage we do have here from the chamelium end is that
> you can only really be screen grabbing from one port at a time. So you
> could actually just track stuff internally in the igt_chamelium API
> and when a user tries to download a framedump that we've already
> downloaded, we can just hand them back a cached copy of it.

I forgot to answer this point. I think this bring way too much overhead
and is not really necessary anyway. With the solution I proposed in my
previous email on this thread, the two wrapper functions (one for CRC,
one for analogue frame comparison) will either dump the frame for CRC
comparison (because it was never dumped before) or use the provided one
for analogue comparison, so there is no particular need to track what
was or wasn't dumped before.

> > In spite of that, I think having a common function, called from the
> > test
> > itself is probably the best approach here.
> 
> Not sure if I misspoke here but I didn't mean to imply that I'm
> against
> having functions for doing frame dumping exposed to the callers. I had
> already figured there'd probably be situations where just having the
> CRC checking do the frame dumping wouldn't be enough.
> 
> This being said though, your viewpoint does make me realize it might
> not be a great idea to do autoframe dumping in -all- crc checking
> functions necessarily, but also makes me realize that this might even
> be a requirement if we still want to try keeping around
> igt_assert_crc_equal() and not just replace it outright with a
> function
> that doesn't fail the whole test (if we fail the test, there isn't
> really a way we can do a framedump from it afterwards). So I would
> think we can at least exclude igt_check_crc_equal() from doing
> automatic framedumping, but I still think it would be a good idea to
> implement igt_assert_crc_equal().
> 
> As for the what you're talking about, e.g. doing frame dump
> comparisons
> on VGA, I think the solution might be not to make any of the code for
> doing the actual frame comparisons chamelium specific either (except
> maybe for the part where we trim the framebuffer we get so it only
> contains the actual image dump).
> 
> So how about this: let's introduce a generic frame comparison API
> using
> the code you've already written for doing this on VGA with the
> chamelium. Make it part of the igt library, and have it just accept
> normal pixman images and perform fuzzy comparisons between them. In
> doing that, we can introduce a generic dump-frames-on-error API
> through
> there much more easily.
> 
> My big aim here is just to make it so that people using igt don't have
> to do anything to get frame dumping in their tests, it just "works".
> 
> > 
> > > 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_
> > > > > i
> > > > > d,
> > > > > 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_
> > > > > i
> > > > > d,
> > > > > 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 {

Martin Peres July 10, 2017, 10:33 a.m. UTC | #11

On 10/07/17 13:31, Paul Kocialkowski wrote:
> On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
>>
>> As well, one advantage we do have here from the chamelium end is that
>> you can only really be screen grabbing from one port at a time. So you
>> could actually just track stuff internally in the igt_chamelium API
>> and when a user tries to download a framedump that we've already
>> downloaded, we can just hand them back a cached copy of it.
> 
> I forgot to answer this point. I think this bring way too much overhead
> and is not really necessary anyway. With the solution I proposed in my
> previous email on this thread, the two wrapper functions (one for CRC,
> one for analogue frame comparison) will either dump the frame for CRC
> comparison (because it was never dumped before) or use the provided one
> for analogue comparison, so there is no particular need to track what
> was or wasn't dumped before.

No need to track, just encode it in the filename:
$test-$subtest-error-crc-$crc.png

Just do not override existing files, and you are done :)

Paul Kocialkowki July 10, 2017, 12:06 p.m. UTC | #12

On Mon, 2017-07-10 at 13:33 +0300, Martin Peres wrote:
> On 10/07/17 13:31, Paul Kocialkowski wrote:
> > On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
> > > 
> > > As well, one advantage we do have here from the chamelium end is
> > > that
> > > you can only really be screen grabbing from one port at a time. So
> > > you
> > > could actually just track stuff internally in the igt_chamelium
> > > API
> > > and when a user tries to download a framedump that we've already
> > > downloaded, we can just hand them back a cached copy of it.
> > 
> > I forgot to answer this point. I think this bring way too much
> > overhead
> > and is not really necessary anyway. With the solution I proposed in
> > my
> > previous email on this thread, the two wrapper functions (one for
> > CRC,
> > one for analogue frame comparison) will either dump the frame for
> > CRC
> > comparison (because it was never dumped before) or use the provided
> > one
> > for analogue comparison, so there is no particular need to track
> > what
> > was or wasn't dumped before.
> 
> No need to track, just encode it in the filename:
> $test-$subtest-error-crc-$crc.png
> 
> Just do not override existing files, and you are done :)

I suppose it would be best to have predictible filenames (so not
including the crc) to make it easier to grab the frame for an automated
testing framework, right?

So what about: frame-$test-$subtest-$qualifier.png, with $qualifier
being either "reference" or "dump". I don't think it's necessary to
indicate whether the error comes from crc or analogue frame testing:
this will already be contained in the subtest name.

Martin Peres July 10, 2017, 1:56 p.m. UTC | #13

On 10/07/17 15:06, Paul Kocialkowski wrote:
> On Mon, 2017-07-10 at 13:33 +0300, Martin Peres wrote:
>> On 10/07/17 13:31, Paul Kocialkowski wrote:
>>> On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
>>>>
>>>> As well, one advantage we do have here from the chamelium end is
>>>> that
>>>> you can only really be screen grabbing from one port at a time. So
>>>> you
>>>> could actually just track stuff internally in the igt_chamelium
>>>> API
>>>> and when a user tries to download a framedump that we've already
>>>> downloaded, we can just hand them back a cached copy of it.
>>>
>>> I forgot to answer this point. I think this bring way too much
>>> overhead
>>> and is not really necessary anyway. With the solution I proposed in
>>> my
>>> previous email on this thread, the two wrapper functions (one for
>>> CRC,
>>> one for analogue frame comparison) will either dump the frame for
>>> CRC
>>> comparison (because it was never dumped before) or use the provided
>>> one
>>> for analogue comparison, so there is no particular need to track
>>> what
>>> was or wasn't dumped before.
>>
>> No need to track, just encode it in the filename:
>> $test-$subtest-error-crc-$crc.png
>>
>> Just do not override existing files, and you are done :)
> 
> I suppose it would be best to have predictible filenames (so not
> including the crc) to make it easier to grab the frame for an automated
> testing framework, right? >
> So what about: frame-$test-$subtest-$qualifier.png, with $qualifier
> being either "reference" or "dump". I don't think it's necessary to
> indicate whether the error comes from crc or analogue frame testing:
> this will already be contained in the subtest name.
> 

Well, predictable is actually problematic, since automated systems try 
to reproduce issues, and your idea will lead to overriding (which is 
only not a problem if the CRC is the same).

In the end, what we want is to say in the logs what are the files 
(reference and dump). We'll need to agree on a format, so as an 
automated system can pick it up :)

Paul Kocialkowki July 10, 2017, 2:11 p.m. UTC | #14

On Mon, 2017-07-10 at 16:56 +0300, Martin Peres wrote:
> On 10/07/17 15:06, Paul Kocialkowski wrote:
> > On Mon, 2017-07-10 at 13:33 +0300, Martin Peres wrote:
> > > On 10/07/17 13:31, Paul Kocialkowski wrote:
> > > > On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
> > > > > 
> > > > > As well, one advantage we do have here from the chamelium end
> > > > > is
> > > > > that
> > > > > you can only really be screen grabbing from one port at a
> > > > > time. So
> > > > > you
> > > > > could actually just track stuff internally in the
> > > > > igt_chamelium
> > > > > API
> > > > > and when a user tries to download a framedump that we've
> > > > > already
> > > > > downloaded, we can just hand them back a cached copy of it.
> > > > 
> > > > I forgot to answer this point. I think this bring way too much
> > > > overhead
> > > > and is not really necessary anyway. With the solution I proposed
> > > > in
> > > > my
> > > > previous email on this thread, the two wrapper functions (one
> > > > for
> > > > CRC,
> > > > one for analogue frame comparison) will either dump the frame
> > > > for
> > > > CRC
> > > > comparison (because it was never dumped before) or use the
> > > > provided
> > > > one
> > > > for analogue comparison, so there is no particular need to track
> > > > what
> > > > was or wasn't dumped before.
> > > 
> > > No need to track, just encode it in the filename:
> > > $test-$subtest-error-crc-$crc.png
> > > 
> > > Just do not override existing files, and you are done :)
> > 
> > I suppose it would be best to have predictible filenames (so not
> > including the crc) to make it easier to grab the frame for an
> > automated
> > testing framework, right? >
> > So what about: frame-$test-$subtest-$qualifier.png, with $qualifier
> > being either "reference" or "dump". I don't think it's necessary to
> > indicate whether the error comes from crc or analogue frame testing:
> > this will already be contained in the subtest name.
> > 
> 
> Well, predictable is actually problematic, since automated systems
> try 
> to reproduce issues, and your idea will lead to overriding (which is 
> only not a problem if the CRC is the same).

That is, unless the automated system moves the file around from the
predictable name to whatever suits it best (that may include which
run the result is part of).

> In the end, what we want is to say in the logs what are the files 
> (reference and dump). We'll need to agree on a format, so as an 
> automated system can pick it up :)

Parsing logs sound like a hackish way to do things quite honestly. Just
having a predictable name and moving the file wherever relevant seems a
lot easier on all sides.

Also, since we're making the code for frame dumping independent from
whether it comes from crc or full frame testing, it doesn't play out too
well to carry the crc result until the point of writing the png file.

Martin Peres July 10, 2017, 4:02 p.m. UTC | #15

On 10/07/17 17:11, Paul Kocialkowski wrote:
> On Mon, 2017-07-10 at 16:56 +0300, Martin Peres wrote:
>> On 10/07/17 15:06, Paul Kocialkowski wrote:
>>> On Mon, 2017-07-10 at 13:33 +0300, Martin Peres wrote:
>>>> On 10/07/17 13:31, Paul Kocialkowski wrote:
>>>>> On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
>>>>>>
>>>>>> As well, one advantage we do have here from the chamelium end
>>>>>> is
>>>>>> that
>>>>>> you can only really be screen grabbing from one port at a
>>>>>> time. So
>>>>>> you
>>>>>> could actually just track stuff internally in the
>>>>>> igt_chamelium
>>>>>> API
>>>>>> and when a user tries to download a framedump that we've
>>>>>> already
>>>>>> downloaded, we can just hand them back a cached copy of it.
>>>>>
>>>>> I forgot to answer this point. I think this bring way too much
>>>>> overhead
>>>>> and is not really necessary anyway. With the solution I proposed
>>>>> in
>>>>> my
>>>>> previous email on this thread, the two wrapper functions (one
>>>>> for
>>>>> CRC,
>>>>> one for analogue frame comparison) will either dump the frame
>>>>> for
>>>>> CRC
>>>>> comparison (because it was never dumped before) or use the
>>>>> provided
>>>>> one
>>>>> for analogue comparison, so there is no particular need to track
>>>>> what
>>>>> was or wasn't dumped before.
>>>>
>>>> No need to track, just encode it in the filename:
>>>> $test-$subtest-error-crc-$crc.png
>>>>
>>>> Just do not override existing files, and you are done :)
>>>
>>> I suppose it would be best to have predictible filenames (so not
>>> including the crc) to make it easier to grab the frame for an
>>> automated
>>> testing framework, right? >
>>> So what about: frame-$test-$subtest-$qualifier.png, with $qualifier
>>> being either "reference" or "dump". I don't think it's necessary to
>>> indicate whether the error comes from crc or analogue frame testing:
>>> this will already be contained in the subtest name.
>>>
>>
>> Well, predictable is actually problematic, since automated systems
>> try
>> to reproduce issues, and your idea will lead to overriding (which is
>> only not a problem if the CRC is the same).
> 
> That is, unless the automated system moves the file around from the
> predictable name to whatever suits it best (that may include which
> run the result is part of).

In this case, I would like to see a way to add a prefix, so as we can 
avoid having another step to move files afterwards.

> 
>> In the end, what we want is to say in the logs what are the files
>> (reference and dump). We'll need to agree on a format, so as an
>> automated system can pick it up :)
> 
> Parsing logs sound like a hackish way to do things quite honestly. Just
> having a predictable name and moving the file wherever relevant seems a
> lot easier on all sides.
> 
> Also, since we're making the code for frame dumping independent from
> whether it comes from crc or full frame testing, it doesn't play out too
> well to carry the crc result until the point of writing the png file.
> 

I don't like the idea of not de-duplicating images. Let's say that we 
have 20 frames that fail, it takes 1 month to fix the issue and we have 
10 runs per day, that would amount to 9.3G of redundant data (for a 
frame being 1.5MB), instead of 30 MB.

So, maybe we could have a compromise. Images are stored in $CRC.png and 
new files $prefix-frame-$test-$subtest-$qualifier are generated, 
containing the path to the right file. This is how we solved this issue 
for image comparison in ezbench. This way, we only add 4K per image 
instead of 1.5 MB.

Lyude Paul July 11, 2017, 5:27 p.m. UTC | #16

On Mon, 2017-07-10 at 13:27 +0300, Paul Kocialkowski wrote:
> On Thu, 2017-07-06 at 17:57 -0400, Lyude Paul wrote:
> > --snip--
> > (also sorry this one took a while to get to, had to do a lot of
> > thinking because I never really solved the problems mentioned here
> > when
> > I tried working on this...)
> > 
> > On Thu, 2017-07-06 at 16:33 +0300, Paul Kocialkowski wrote:
> > > On Thu, 2017-07-06 at 14:31 +0300, Paul Kocialkowski wrote:
> > > > > 
> > > > > 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).
> > 
> > That is a good point, but there's some things I think you might
> > want
> > to
> > consider. Mainly that in a test that passes, we of course don't
> > write
> > any framedumps back to the disk since nothing failed. IMO, I would
> > -think- that we care a bit more about the performance hit that
> > happens
> > on passing tests vs. failing tests, since tests aren't really
> > supposed
> > to fail under ideal conditions anyway. Might be better to verify
> > with
> > the mupuf and the other people actually running Intel's CI though,
> > since I'm not one of them.
> > 
> > As well, one advantage we do have here from the chamelium end is
> > that
> > you can only really be screen grabbing from one port at a time. So
> > you
> > could actually just track stuff internally in the igt_chamelium API
> > and
> > when a user tries to download a framedump that we've already
> > downloaded, we can just hand them back a cached copy of it.
> 
> Either way, it is definitely okay to take the time to dump the frame
> when a mismatch occurs if we don't have it already (in the CRC case).
> 
> > > 
> > > In spite of that, I think having a common function, called from
> > > the
> > > test
> > > itself is probably the best approach here.
> > 
> > Not sure if I misspoke here but I didn't mean to imply that I'm
> > against
> > having functions for doing frame dumping exposed to the callers. I
> > had
> > already figured there'd probably be situations where just having
> > the
> > CRC checking do the frame dumping wouldn't be enough.
> > 
> > This being said though, your viewpoint does make me realize it
> > might
> > not be a great idea to do autoframe dumping in -all- crc checking
> > functions necessarily, but also makes me realize that this might
> > even
> > be a requirement if we still want to try keeping around
> > igt_assert_crc_equal() and not just replace it outright with a
> > function
> > that doesn't fail the whole test (if we fail the test, there isn't
> > really a way we can do a framedump from it afterwards). So I would
> > think we can at least exclude igt_check_crc_equal() from doing
> > automatic framedumping, but I still think it would be a good idea
> > to
> > implement igt_assert_crc_equal().
> 
> igt_assert_crc_equal already exists and is used by many other tests,
> so
> we really cannot embed the frame dumping logic there, since these
> tests
> have nothing to do with the chamelium. That's another reason to
> really
> keep the frame dump and crc comparison logic separate.
> 
> > As for the what you're talking about, e.g. doing frame dump
> > comparisons
> > on VGA, I think the solution might be not to make any of the code
> > for
> > doing the actual frame comparisons chamelium specific either
> > (except
> > maybe for the part where we trim the framebuffer we get so it only
> > contains the actual image dump).
> > 
> > So how about this: let's introduce a generic frame comparison API
> > using
> > the code you've already written for doing this on VGA with the
> > chamelium. Make it part of the igt library, and have it just accept
> > normal pixman images and perform fuzzy comparisons between them. In
> > doing that, we can introduce a generic dump-frames-on-error API
> > through
> > there much more easily.
> > 
> > My big aim here is just to make it so that people using igt don't
> > have
> > to do anything to get frame dumping in their tests, it just
> > "works".
> 
> I totally agree with this direction.
> 
> What I suggest we should do is the following:
> * keep CRC functions (igt_assert_crc_equal and igt_check_crc_equal)
> common and fully independent from the frame dumping logic, either
> with a
> common crc mismatch detection logic or not (because it's so simple)
> * have common frame dump functions that just take a cairo surface and
> handle filenames and actually writing the frames
> * have the analogue frame detection code made common
> * have two assert-style chamnelium-specific wrappers to link frame
> comparison (either CRC or analogue) and frame dumping on failure
> while
> still ending with an assert; the CRC fashion would be in charge of
> dumping the frame on failure while the frame would be provided to the
> analogue comparison fashion.

Sounds good to me!
> 
> I will prepare patches in this direction so that you can get a more
> concrete idea and we can follow-up the discussion on that basis.
> 
> > > > 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
> > > > > >  							ed
> > > > > > id_
> > > > > > i
> > > > > > d,
> > > > > > 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
> > > > > >  							ed
> > > > > > id_
> > > > > > i
> > > > > > d,
> > > > > > 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 {

Patch

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
  *
  *	# 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,
+							   "Chamelium",
+							   "FrameDumpPath",
+							    &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;
+
+	for (i = 0; i < a->n_words; i++)
+		if (a->crc[i] != b->crc[i])
+			return false;
+
+	return true;
+}
+
+/**
  * 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_count);
 
-	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_XRGB8888,
-						    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_count);
+				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");
+		}
 
 		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 {