Re: [igt-dev] [PATCH i-g-t v3 1/3] lib/igt_chamelium Added chamelium_frame_match_or_dump which returns bool that the captured frame matches

[Kunal Joshi <kunal1.joshi@intel.com>]

On 2020-01-21 at 15:29:48 +0200, Petri Latvala wrote:
> On Tue, Jan 21, 2020 at 11:24:13AM +0530, Kunal Joshi wrote:
> > Added chamelium_frame_match_or_dump which returns bool that the captured
> > frame matches with reference framebuffer.
> > 
> > (v2)
> >         Removed previously added function chamelium_assert_frame_dump_eq.
> > 
> > (v3)
> > 	No change.
> > 
> > Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> > Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> > Suggested-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  lib/igt_chamelium.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_chamelium.h |  5 +++++
> >  2 files changed, 70 insertions(+)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index 9971f51..db1f2b6 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -1632,6 +1632,71 @@ void chamelium_assert_frame_match_or_dump(struct chamelium *chamelium,
> >  }
> >  
> >  /**
> > + * chamelium_assert_frame_match_or_dump:
> > + * @chamelium: The chamelium instance the frame dump belongs to
> > + * @frame: The chamelium frame dump to match
> > + * @fb: pointer to an #igt_fb structure
> > + * @check: the type of frame matching check to use
> > + *
> > + * Returns bool that the provided captured frame matches the reference
> > + * frame from the framebuffer. If they do not, this saves the reference
> > + * and captured frames to a png file.
> > + */
> > +bool chamelium_frame_match_or_dump(struct chamelium *chamelium,
> > +				   struct chamelium_port *port,
> > +				   const struct chamelium_frame_dump *frame,
> > +				   struct igt_fb *fb,
> > +				   enum chamelium_check check)
> > +{
> > +	cairo_surface_t *reference;
> > +	cairo_surface_t *capture;
> > +	igt_crc_t *reference_crc;
> > +	igt_crc_t *capture_crc;
> > +	bool match;
> > +
> > +	/* Grab the reference frame from framebuffer */
> > +	reference = igt_get_cairo_surface(chamelium->drm_fd, fb);
> > +
> > +	/* Grab the captured frame from chamelium */
> > +	capture = convert_frame_dump_argb32(frame);
> > +
> > +	switch (check) {
> > +	case CHAMELIUM_CHECK_ANALOG:
> > +		match = igt_check_analog_frame_match(reference, capture);
> > +		break;
> > +	case CHAMELIUM_CHECK_CHECKERBOARD:
> > +		match = igt_check_checkerboard_frame_match(reference, capture);
> > +		break;
> > +	default:
> > +		igt_assert(false);
> > +	}
> > +
> > +	if (!match && igt_frame_dump_is_enabled()) {
> > +		reference_crc = malloc(sizeof(igt_crc_t));
> > +		igt_assert(reference_crc);
> > +
> > +		/* Calculate the reference frame CRC. */
> > +		chamelium_do_calculate_fb_crc(reference, reference_crc);
> > +
> > +		/* Get the captured frame CRC from the Chamelium. */
> > +		capture_crc = chamelium_get_crc_for_area(chamelium, port, 0, 0,
> > +							 0, 0);
> > +		igt_assert(capture_crc);
> > +
> > +		compared_frames_dump(reference, capture, reference_crc,
> > +				     capture_crc);
> > +
> > +		free(reference_crc);
> > +		free(capture_crc);
> > +	}
> > +
> > +	cairo_surface_destroy(reference);
> > +	cairo_surface_destroy(capture);
> > +
> > +	return match;
> > +}
> 
> This copy of chamelium_assert_frame_match_or_dump is
> unnecessary. The typical structure is that we have a bool-returning
> function for code that wants to do its own checks afterwards, and
> another one that 1) calls the bool-returning function 2) asserts
> success. Like so:
> 
> 
Petri if you see the function test_pipe_ctm, we loop for different
delta values If the of the delta value has a match then only we assert,
I think chamelium_frame_dump_match returning a bool value is required,
I will use chamelium_assert_frame_match_or_dump in the rest of
the functions where we need to assert without any further checks.

Thanks and regards
Kunal

> void chamelium_assert_frame_match_or_dump(struct chamelium *chamelium,
>   					  const struct chamelium_frame_dump *frame,
>   					  struct igt_fb *fb,
>   					  enum chamelium_check check)
> {
>   igt_assert(chamelium_frame_match_or_dump(chamelium, frame, fb, check));
> }
> 
> 
> 
> 
> -- 
> Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev