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

Mon Jul 10 10:27:51 UTC 2017

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