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

[Lyude Paul]

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 {