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

[Paul Kocialkowski <paul.kocialkowski@linux.intel.com>]

On Thu, 2017-07-06 at 14:31 +0300, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed, 2017-07-05 at 17:44 -0400, Lyude Paul wrote:
> > On Wed, 2017-07-05 at 11:04 +0300, Paul Kocialkowski wrote:
> > > When a CRC comparison error occurs, it is quite useful to get a
> > > dump
> > > of both the frame obtained from the chamelium and the reference in
> > > order
> > > to compare them.
> > > 
> > > This implements the frame dump, with a configurable path that
> > > enables
> > > the use of this feature.
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.co
> > > m>
> > > ---
> > >  lib/igt_chamelium.c |  21 +++++++++++
> > >  lib/igt_chamelium.h |   1 +
> > >  lib/igt_debugfs.c   |  20 ++++++++++
> > >  lib/igt_debugfs.h   |   1 +
> > >  tests/chamelium.c   | 104 ++++++++++++++++++++-------------------
> > > -
> > > ------------
> > >  5 files changed, 82 insertions(+), 65 deletions(-)
> > > 
> > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > index ef51ef68..9aca6842 100644
> > > --- a/lib/igt_chamelium.c
> > > +++ b/lib/igt_chamelium.c
> > > @@ -57,6 +57,7 @@
> > >   * |[<!-- language="plain" -->
> > >   *	[Chamelium]
> > >   *	URL=http://chameleon:9992 # The URL used for connecting
> > > to
> > > the Chamelium's RPC server
> > > + *	FrameDumpPath=/tmp # The path to dump frames that fail
> > > comparison checks
> > 
> > While no one else really cares about creating frame dumps yet, it's
> > possible someone else may in the future if we ever end up taking
> > more
> > advantage of automated testing systems like this. So I'd stick this
> > in
> > the generic non-chamelium specific section in the config file
> 
> That definitely makes sense. By the way, what approach would you
> recommend for
> thishandling? Mupuf was suggesting to have a common configuration
> structure
> instead of declaring either global variables or static ones with
> getters/setters. This is probably becoming more and more of a
> necessity as we
> add more common config options.
> 
> However, I think we should still allow specific parts of IGT to do the
> parsing
> themselves (especially in the case of chamelium) so that the common
> config
> structure only has common fields (and does not, for instance, contain
> the
> chamelium port configuration).
> 
> > >   *
> > >   *	# The rest of the sections are used for defining
> > > connector
> > > mappings.
> > >   *	# This is required so any tests using the Chamelium
> > > know
> > > which connector
> > > @@ -115,11 +116,26 @@ struct chamelium {
> > >  	struct chamelium_edid *edids;
> > >  	struct chamelium_port *ports;
> > >  	int port_count;
> > > +
> > > +	char *frame_dump_path;
> > >  };
> > >  
> > >  static struct chamelium *cleanup_instance;
> > >  
> > >  /**
> > > + * chamelium_get_frame_dump_path:
> > > + * @chamelium: The Chamelium instance to use
> > > + *
> > > + * Retrieves the path to dump frames to.
> > > + *
> > > + * Returns: a string with the frame dump path
> > > + */
> > > +char *chamelium_get_frame_dump_path(struct chamelium *chamelium)
> > > +{
> > > +	return chamelium->frame_dump_path;
> > > +}
> > > +
> > > +/**
> > >   * chamelium_get_ports:
> > >   * @chamelium: The Chamelium instance to use
> > >   * @count: Where to store the number of ports
> > > @@ -1338,6 +1354,11 @@ static bool chamelium_read_config(struct
> > > chamelium *chamelium, int drm_fd)
> > >  		return false;
> > >  	}
> > >  
> > > +	chamelium->frame_dump_path =
> > > g_key_file_get_string(igt_key_file,
> > > +							   "Chame
> > > liu
> > > m",
> > > +							   "Frame
> > > Dum
> > > pPath",
> > > +							    &erro
> > > r);
> > > +
> > >  	return chamelium_read_port_mappings(chamelium, drm_fd);
> > >  }
> > >  
> > > diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> > > index 908e03d1..aa881971 100644
> > > --- a/lib/igt_chamelium.h
> > > +++ b/lib/igt_chamelium.h
> > > @@ -42,6 +42,7 @@ struct chamelium *chamelium_init(int drm_fd);
> > >  void chamelium_deinit(struct chamelium *chamelium);
> > >  void chamelium_reset(struct chamelium *chamelium);
> > >  
> > > +char *chamelium_get_frame_dump_path(struct chamelium *chamelium);
> > >  struct chamelium_port **chamelium_get_ports(struct chamelium
> > > *chamelium,
> > >  					    int *count);
> > >  unsigned int chamelium_port_get_type(const struct chamelium_port
> > > *port);
> > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > index 80f25c61..dcb4e0a7 100644
> > > --- a/lib/igt_debugfs.c
> > > +++ b/lib/igt_debugfs.c
> > > @@ -282,6 +282,26 @@ bool igt_debugfs_search(int device, const
> > > char
> > > *filename, const char *substring)
> > >   */
> > >  
> > >  /**
> > > + * igt_check_crc_equal:
> > > + * @a: first pipe CRC value
> > > + * @b: second pipe CRC value
> > > + *
> > > + * Compares two CRC values and return whether they match.
> > > + *
> > > + * Returns: A boolean indicating whether the CRC values match
> > > + */
> > > +bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < a->n_words; i++)
> > > +		if (a->crc[i] != b->crc[i])
> > > +			return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > 
> > Make this a separate patch, and instead of having another function
> > do
> > the CRC calculations just have something like this:
> > 
> >  * static int igt_find_crc_mismatch(const igt_crc_t *a, const
> > igt_crc_t
> >    *b): returns the index of the first CRC mismatch, 0 if none was
> >    found
> >  * bool igt_check_crc_equal(): uses igt_find_crc_mismatch() to
> > figure
> >    out if anything mismatched, and return true if something did (as
> >    well, also spit out some debugging info mentioning there was a
> >    mismatch)
> >  * void igt_assert_crc_equal(): uses igt_find_crc_mismatch() to
> > figure
> >    out if anything mismatched. If the assertion fails, use
> >    igt_assert_eq() on the mismatched crc so we still get a useful
> > error
> >    message on CRC failures.
> > 
> > There isn't much code required to actually compare CRCs, however I'd
> > still prefer only having one function doing the actual comparison
> > logic
> > here so we only have one piece of code to update if we need to make
> > changes to it in the future.
> > 
> > Mupuf, your opinion on this? ^
> > 
> > > +/**
> > >   * igt_assert_crc_equal:
> > >   * @a: first pipe CRC value
> > >   * @b: second pipe CRC value
> > > diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> > > index 7b846a83..2695cbda 100644
> > > --- a/lib/igt_debugfs.h
> > > +++ b/lib/igt_debugfs.h
> > > @@ -113,6 +113,7 @@ enum intel_pipe_crc_source {
> > >          INTEL_PIPE_CRC_SOURCE_MAX,
> > >  };
> > >  
> > > +bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b);
> > >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t
> > > *b);
> > >  char *igt_crc_to_string(igt_crc_t *crc);
> > >  
> > > diff --git a/tests/chamelium.c b/tests/chamelium.c
> > > index 5cf8b3af..3d95c05c 100644
> > > --- a/tests/chamelium.c
> > > +++ b/tests/chamelium.c
> > > @@ -381,7 +381,7 @@ disable_output(data_t *data,
> > >  }
> > >  
> > >  static void
> > > -test_display_crc_single(data_t *data, struct chamelium_port
> > > *port)
> > > +test_display_crc(data_t *data, struct chamelium_port *port, int
> > > count)
> > >  {
> > >  	igt_display_t display;
> > >  	igt_output_t *output;
> > > @@ -390,9 +390,14 @@ test_display_crc_single(data_t *data, struct
> > > chamelium_port *port)
> > >  	igt_crc_t *expected_crc;
> > >  	struct chamelium_fb_crc *fb_crc;
> > >  	struct igt_fb fb;
> > > +	struct chamelium_frame_dump *frame;
> > >  	drmModeModeInfo *mode;
> > >  	drmModeConnector *connector;
> > > -	int fb_id, i;
> > > +	int fb_id, i, j, captured_frame_count;
> > > +	const char *connector_name;
> > > +	char *frame_dump_path;
> > > +	char path[PATH_MAX];
> > > +	bool eq;
> > >  
> > >  	reset_state(data, port);
> > >  
> > > @@ -401,6 +406,9 @@ test_display_crc_single(data_t *data, struct
> > > chamelium_port *port)
> > >  	primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > >  	igt_assert(primary);
> > >  
> > > +	connector_name = kmstest_connector_type_str(connector-
> > > > connector_type);
> > > 
> > > +	frame_dump_path = chamelium_get_frame_dump_path(data-
> > > > chamelium);
> > > 
> > > +
> > >  	for (i = 0; i < connector->count_modes; i++) {
> > >  		mode = &connector->modes[i];
> > >  		fb_id = igt_create_color_pattern_fb(data->drm_fd,
> > > @@ -415,74 +423,40 @@ test_display_crc_single(data_t *data, struct
> > > chamelium_port *port)
> > >  
> > >  		enable_output(data, port, output, mode, &fb);
> > >  
> > > -		igt_debug("Testing single CRC fetch\n");
> > > -
> > > -		crc = chamelium_get_crc_for_area(data->chamelium,
> > > port,
> > > -						 0, 0, 0, 0);
> > > -
> > > -		expected_crc =
> > > chamelium_calculate_fb_crc_result(fb_crc);
> > > -
> > > -		igt_assert_crc_equal(crc, expected_crc);
> > > -		free(expected_crc);
> > > -		free(crc);
> > > -
> > > -		disable_output(data, port, output);
> > > -		igt_remove_fb(data->drm_fd, &fb);
> > > -	}
> > > -
> > > -	drmModeFreeConnector(connector);
> > > -	igt_display_fini(&display);
> > > -}
> > > -
> > > -static void
> > > -test_display_crc_multiple(data_t *data, struct chamelium_port
> > > *port)
> > > -{
> > > -	igt_display_t display;
> > > -	igt_output_t *output;
> > > -	igt_plane_t *primary;
> > > -	igt_crc_t *crc;
> > > -	igt_crc_t *expected_crc;
> > > -	struct chamelium_fb_crc *fb_crc;
> > > -	struct igt_fb fb;
> > > -	drmModeModeInfo *mode;
> > > -	drmModeConnector *connector;
> > > -	int fb_id, i, j, captured_frame_count;
> > > +		chamelium_capture(data->chamelium, port, 0, 0, 0,
> > > 0,
> > > count);
> > > +		crc = chamelium_read_captured_crcs(data-
> > > >chamelium,
> > > +						   &captured_fram
> > > e_c
> > > ount);
> > >  
> > > -	reset_state(data, port);
> > > +		igt_assert(captured_frame_count == count);
> > >  
> > > -	output = prepare_output(data, &display, port);
> > > -	connector = chamelium_port_get_connector(data->chamelium,
> > > port, false);
> > > -	primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > -	igt_assert(primary);
> > > +		igt_debug("Captured %d frames\n",
> > > captured_frame_count);
> > >  
> > > -	for (i = 0; i < connector->count_modes; i++) {
> > > -		mode = &connector->modes[i];
> > > -		fb_id = igt_create_color_pattern_fb(data->drm_fd,
> > > -						    mode-
> > > >hdisplay,
> > > -						    mode-
> > > >vdisplay,
> > > -						    DRM_FORMAT_XR
> > > GB8
> > > 888,
> > > -						    LOCAL_DRM_FOR
> > > MAT
> > > _MOD_NONE,
> > > -						    0, 0, 0,
> > > &fb);
> > > -		igt_assert(fb_id > 0);
> > > +		expected_crc =
> > > chamelium_calculate_fb_crc_result(fb_crc);
> > >  
> > > -		fb_crc = chamelium_calculate_fb_crc_launch(data-
> > > > drm_fd, &fb);
> > > 
> > > +		for (j = 0; j < captured_frame_count; j++) {
> > > +			eq = igt_check_crc_equal(&crc[j],
> > > expected_crc);
> > > +			if (!eq && frame_dump_path) {
> > > +				frame =
> > > chamelium_read_captured_frame(data->chamelium,
> > > +								 
> > >    
> > >   j);
> > >  
> > > -		enable_output(data, port, output, mode, &fb);
> > > +				igt_debug("Dumping reference and
> > > chamelium frames to %s...\n",
> > > +					  frame_dump_path);
> > >  
> > > -		/* We want to keep the display running for a
> > > little
> > > bit, since
> > > -		 * there's always the potential the driver isn't
> > > able to keep
> > > -		 * the display running properly for very long
> > > -		 */
> > > -		chamelium_capture(data->chamelium, port, 0, 0, 0,
> > > 0,
> > > 3);
> > > -		crc = chamelium_read_captured_crcs(data-
> > > >chamelium,
> > > -						   &captured_fram
> > > e_c
> > > ount);
> > > +				snprintf(path, PATH_MAX,
> > > "%s/frame-
> > > reference-%s.png",
> > > +					 frame_dump_path,
> > > connector_name);
> > > +				igt_write_fb_to_png(data->drm_fd,
> > > &fb, path);
> > >  
> > > -		igt_debug("Captured %d frames\n",
> > > captured_frame_count);
> > > +				snprintf(path, PATH_MAX,
> > > "%s/frame-
> > > chamelium-%s.png",
> > > +					 frame_dump_path,
> > > connector_name);
> > > +				chamelium_write_frame_to_png(data
> > > -
> > > > chamelium,
> > > 
> > > +							     fram
> > > e,
> > > path);
> > >  
> > > -		expected_crc =
> > > chamelium_calculate_fb_crc_result(fb_crc);
> > > +				chamelium_destroy_frame_dump(fram
> > > e);
> > > +			}
> > >  
> > > -		for (j = 0; j < captured_frame_count; j++)
> > > -			igt_assert_crc_equal(&crc[j],
> > > expected_crc);
> > > +			igt_fail_on_f(!eq,
> > > +				      "Chamelium frame CRC
> > > mismatch
> > > with reference\n");
> > > +		}
> > 
> > There's lots of potential here for copy pasta to form in the future,
> > since the API here puts a lot of work on the caller to set things up
> > for frame dumping. IMO, it would be worth it to teach the CRC
> > checking
> > functions to automatically do frame dumps on mismatch if the CRC
> > source
> > supports it. This will save us from having to have separate frame
> > dump
> > APIs in the future if we ever end up adding support for other kinds
> > of
> > automated test equipment.
> 
> I don't think it makes so much sense to do this in the CRC checking
> functions,
> just because they are semantically expected to do one thing: CRC
> checking, and
> doing frame dumps seems like going overboard.
> 
> On the other hand, I do agree that the dumping and saving part can and
> should be
> made common, but maybe as a separate function. So that would be two
> calls for
> the tests: one to check the crc and one to dump and save the frame.

A strong case to support this vision: in VGA frame testing, we have
already dumped the frame and don't do CRC checking, yet we also need to
save the frames if there is a mismatch.

It would be a shame that the dumping logic becomes part of the CRC
functions, since that would mean duplicating that logic for VGA testing
(as it's currently done in the version I just sent out).

In spite of that, I think having a common function, called from the test
itself is probably the best approach here.

> I have also duplicated that logic in upcoming VGA frame testing, so
> there is definitely a need for less duplication.
> 
> > As well, I like how you removed the redundancy between
> > test_display_crc_single() and test_display_crc_multiple(). However
> > since those are somewhat unrelated changes to the code path for
> > these
> > tests it would be better to have that re-factoring as a separate
> > patch
> > so as to make it easier for anyone who might need to bisect this
> > code
> > in the future.
> 
> Fair enough, it just felt weird to commit two functions that were
> nearly the
> exact same, but I have no problem with doing this in two separate
> patches.
> 
> > >  
> > >  		free(expected_crc);
> > >  		free(crc);
> > > @@ -644,10 +618,10 @@ igt_main
> > >  							edid_id,
> > > alt_edid_id);
> > >  
> > >  		connector_subtest("dp-crc-single", DisplayPort)
> > > -			test_display_crc_single(&data, port);
> > > +			test_display_crc(&data, port, 1);
> > >  
> > >  		connector_subtest("dp-crc-multiple", DisplayPort)
> > > -			test_display_crc_multiple(&data, port);
> > > +			test_display_crc(&data, port, 3);
> > >  	}
> > >  
> > >  	igt_subtest_group {
> > > @@ -698,10 +672,10 @@ igt_main
> > >  							edid_id,
> > > alt_edid_id);
> > >  
> > >  		connector_subtest("hdmi-crc-single", HDMIA)
> > > -			test_display_crc_single(&data, port);
> > > +			test_display_crc(&data, port, 1);
> > >  
> > >  		connector_subtest("hdmi-crc-multiple", HDMIA)
> > > -			test_display_crc_multiple(&data, port);
> > > +			test_display_crc(&data, port, 3);
> > >  	}
> > >  
> > >  	igt_subtest_group {
-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx