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

Paul Kocialkowski paul.kocialkowski at linux.intel.com
Mon Jul 10 10:12:54 UTC 2017


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 <paul.kocialkowski at linux.inte
> > > > > 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 Kocialkowski <paul.kocialkowski at linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland


More information about the Intel-gfx mailing list