Re: [PATCH i-g-t v3 1/4] chamelium: Calculate CRC from framebuffer instead of hardcoding it

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2017-07-06 at 16:14 +0300, Paul Kocialkowski wrote:
> On Wed, 2017-07-05 at 16:34 -0400, Lyude Paul wrote:
> > So a couple of notes here that will make it a lot easier for me to
> > review these in the future
> > 
> >  * When you're doing a new revision of a patch series, it's helpful
> > to
> >    keep it in the same email thread as the original v1 so it's
> > easier
> >    to keep track of in people's mail clients (as well as avoiding
> >    accidentally reviewing older patch versions. I usually do
> > something
> >    like this (other projects might request you do this slightly
> >    differently, but this should be fine here):
> >     * [PATCH 0/2] Cleaning up the alignment of various foos
> >        * [PATCH 1/2] Foo the bar, not the baz
> >        * [PATCH 2/2] Introduce the amazing new foo_bar
> >        * [PATCH v2 0/2] Cleaning up the alignment of various foos
> >           * [PATCH v2 1/2] Foo the bar, not the baz
> >           * [PATCH v2 2/2] Introduce the amazing new foo_bar
> >        * [PATCH v3 0/2] Cleaning up the alignment of various foos
> >           * [PATCH v3 1/2] Foo the bar, not the baz
> >           * [PATCH v3 2/2] Introduce the amazing new foo_bar
> >  * Try not to group unrelated patches together in the same thread.
> > This
> >    also makes sorting through all of them a little more difficult.
> >  * When you make new revisions of patches, it's very useful if you
> > also
> >    include a list of changes you made to the patch since the last
> >    revision. It doesn't need to be very finely detailed, something
> > like
> >    this would suffice:
> >     * Various style fixes
> >     * Rename baz to moo, add cow noises
> >     * Split init_cow() into init_white_cow() and init_black_cow()
> >       instead of handling both kinds of cows in the same function
> >     * Fix documentation
> >     For intel-gpu-tools, it's fine to just stick this in the commit
> >     message. Other projects may request you put the changelog below
> > the
> >     ----- right above the diff stats (this allows the comments not
> > to
> >     get included in the final commit message)
> >  * Unless they are all very small and less important fixes,
> > including
> >    cover letters helps as well since it lets patchwork group
> > together
> >    patch series like this.
> 
> What would you prefer that I do regarding follow-up versions to this
> patchset (and the other one that is still under review)?
Don't worry about that, I went through all the other patches and
reviewed them anyway so there shouldn't be anything left to do :). just
use the guidelines for the future.
> 
> I could split the series per-topic (crc, frame save, time
> improvements)
> and keep those in the same parent thread as their v1.
> 
> > Anyway, back to the actual patch:
> > A good start! Will need a couple of changes though
> > 
> > On Wed, 2017-07-05 at 11:04 +0300, Paul Kocialkowski wrote:
> > > This introduces CRC calculation for reference frames, instead of
> > > using
> > > hardcoded values for them. The rendering of reference frames may
> > > differ
> > > from machine to machine, especially due to font rendering, and
> > > the
> > > frame itself may change with subsequent IGT changes.
> > > 
> > > These differences would cause the CRC checks to fail on different
> > > setups. This allows them to pass regardless of the setup.
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.c
> > > om>
> > > ---
> > >  lib/igt_chamelium.c | 160
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/igt_chamelium.h |   5 ++
> > >  tests/chamelium.c   |  76 ++++++-------------------
> > >  3 files changed, 183 insertions(+), 58 deletions(-)
> > > 
> > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > index bff08c0e..b9d80b6b 100644
> > > --- a/lib/igt_chamelium.c
> > > +++ b/lib/igt_chamelium.c
> > > @@ -94,6 +94,14 @@ struct chamelium_frame_dump {
> > >  	struct chamelium_port *port;
> > >  };
> > >  
> > > +struct chamelium_fb_crc {
> > > +	int fd;
> > > +	struct igt_fb *fb;
> > > +
> > > +	pthread_t thread_id;
> > > +	igt_crc_t *ret;
> > > +};
> > > +
> > 
> > The name of this structure is a little misleading, because now we
> > have
> > an API that exposes both a struct chamelium_fb_crc struct in
> > addition
> > to the igt_crc_t struct. Rename this to something like struct
> > chamelium_fb_crc_work
> > 
> > >  struct chamelium {
> > >  	xmlrpc_env env;
> > >  	xmlrpc_client *client;
> > > @@ -1003,6 +1011,158 @@ int chamelium_get_frame_limit(struct
> > > chamelium *chamelium,
> > >  	return ret;
> > >  }
> > >  
> > > +static uint32_t chamelium_xrgb_hash16(unsigned char *buffer, int
> > > width,
> > > +				      int height, int k, int m)
> > > +{
> > 
> > We're not modifying buffer, so make it a const. As well, feel free
> > to
> > mark this function as inline.
> > 
> > > +	unsigned char r, g, b;
> > > +	uint64_t sum = 0;
> > > +	uint64_t count = 0;
> > > +	uint64_t value;
> > > +	uint32_t hash;
> > > +	int index;
> > > +	int i;
> > > +
> > > +	for (i=0; i < width * height; i++) {
> > > +		if ((i % m) != k)
> > > +			continue;
> > > +
> > > +		index = i * 4;
> > > +
> > > +		r = buffer[index + 2];
> > > +		g = buffer[index + 1];
> > > +		b = buffer[index + 0];
> > > +
> > > +		value = r | (g << 8) | (b << 16);
> > > +		sum += ++count * value;
> > > +	}
> > > +
> > > +	hash = ((sum >> 0) ^ (sum >> 16) ^ (sum >> 32) ^ (sum >>
> > > 48)) & 0xffff;
> > > +
> > > +	return hash;
> > > +}
> > > +
> > > +/**
> > > + * chamelium_calculate_fb_crc:
> > > + * @fd: The drm file descriptor
> > > + * @fb: The framebuffer to calculate the CRC for
> > > + *
> > > + * Calculates a CRC for the provided framebuffer, the same way
> > > as
> > > the Chamelium.
> > 
> > Calculate the CRC for the provided framebuffer, using the
> > Chamelium's
> > CRC algorithm
> > > + * This calculates the CRC in a non-threaded fashion.
> > > + *
> > > + * Returns: The calculated CRC
> > > + */
> > > +igt_crc_t *chamelium_calculate_fb_crc(int fd, struct igt_fb *fb)
> > > +{
> > > +	igt_crc_t *ret;
> > > +	cairo_t *cr;
> > > +	cairo_surface_t *fb_surface;
> > > +	unsigned char *buffer;
> > > +	int n = 4;
> > > +	int w, h;
> > > +	int i, j;
> > > +
> > > +	ret = calloc(1, sizeof(igt_crc_t));
> > > +
> > > +	/* Get the cairo surface for the framebuffer */
> > > +	cr = igt_get_cairo_ctx(fd, fb);
> > > +	fb_surface = cairo_get_target(cr);
> > > +	cairo_surface_reference(fb_surface);
> > > +	cairo_destroy(cr);
> > > +
> > > +	buffer = cairo_image_surface_get_data(fb_surface);
> > > +	w = fb->width;
> > > +	h = fb->height;
> > > +
> > > +	for (i = 0; i < n; i++) {
> > > +		j = n - i - 1;
> > > +		ret->crc[i] = chamelium_xrgb_hash16(buffer, w,
> > > h,
> > > j,
> > > n);
> > > +	}
> > > +
> > > +	ret->n_words = n;
> > > +	cairo_surface_destroy(fb_surface);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void *chamelium_calculate_fb_crc_thread(void *data)
> > > +{
> > > +	struct chamelium_fb_crc *fb_crc = (struct
> > > chamelium_fb_crc
> > > *) data;
> > > +	cairo_t *cr;
> > > +	cairo_surface_t *fb_surface;
> > > +	unsigned char *buffer;
> > > +	int n = 4;
> > > +	int w, h;
> > > +	int i, j;
> > > +
> > > +	/* Get the cairo surface for the framebuffer */
> > > +	cr = igt_get_cairo_ctx(fb_crc->fd, fb_crc->fb);
> > > +	fb_surface = cairo_get_target(cr);
> > > +	cairo_surface_reference(fb_surface);
> > > +	cairo_destroy(cr);
> > > +
> > > +	buffer = cairo_image_surface_get_data(fb_surface);
> > > +	w = fb_crc->fb->width;
> > > +	h = fb_crc->fb->height;
> > > +
> > > +	for (i = 0; i < n; i++) {
> > > +		j = n - i - 1;
> > > +		fb_crc->ret->crc[i] =
> > > chamelium_xrgb_hash16(buffer,
> > > w, h, j, n);
> > > +	}
> > > +
> > > +	fb_crc->ret->n_words = n;
> > > +	cairo_surface_destroy(fb_surface);
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +/**
> > > + * chamelium_calculate_fb_crc_launch:
> > > + * @fd: The drm file descriptor
> > > + * @fb: The framebuffer to calculate the CRC for
> > > + *
> > > + * Launches the CRC calculation for the provided framebuffer,
> > > the
> > > same way as
> > > + * the Chamelium. This calculates the CRC in a threaded fashion.
> > > + * Thread-related information is returned and should be passed
> > > to a
> > > subsequent
> > > + * call to chamelium_calculate_fb_crc_result. It should not be
> > > freed.
> > > + *
> > > + * Returns: An intermediate structure with thread-related
> > > information
> > 
> > The user doesn't need to know about the magic inside the structure,
> > since we're not going to expose it's definition outside of this
> > file
> > anyway.
> > > + */
> > > +struct chamelium_fb_crc *chamelium_calculate_fb_crc_launch(int
> > > fd,
> > > +							   struc
> > > t
> > > igt_fb *fb)
> > > +{
> > > +	struct chamelium_fb_crc *fb_crc;
> > > +
> > > +	fb_crc = calloc(1, sizeof(struct chamelium_fb_crc));
> > > +	fb_crc->ret = calloc(1, sizeof(igt_crc_t));
> > > +	fb_crc->fd = fd;
> > > +	fb_crc->fb = fb;
> > > +
> > > +	pthread_create(&fb_crc->thread_id, NULL,
> > > +		       chamelium_calculate_fb_crc_thread,
> > > fb_crc);
> > > +
> > > +	return fb_crc;
> > > +}
> > > +
> > > +/**
> > > + * chamelium_calculate_fb_crc_result:
> > > + * @fb_crc: An intermediate structure with thread-related
> > > information
> > > + *
> > > + * Provides the result for the previously-launched CRC
> > > calculation.
> > 
> > Blocks until the async CRC calculation is finished, and then
> > returns
> > the result.
> > > + *
> > > + * Returns: The calculated CRC
> > > + */
> > > +igt_crc_t *chamelium_calculate_fb_crc_result(struct
> > > chamelium_fb_crc
> > > *fb_crc)
> > > +{
> > > +	igt_crc_t *ret;
> > > +
> > > +	pthread_join(fb_crc->thread_id, NULL);
> > > +
> > > +	ret = fb_crc->ret;
> > > +	free(fb_crc);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static unsigned int chamelium_get_port_type(struct chamelium
> > > *chamelium,
> > >  					    struct
> > > chamelium_port
> > > *port)
> > >  {
> > > diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> > > index 81322ad2..e51cf4f9 100644
> > > --- a/lib/igt_chamelium.h
> > > +++ b/lib/igt_chamelium.h
> > > @@ -36,6 +36,7 @@
> > >  struct chamelium;
> > >  struct chamelium_port;
> > >  struct chamelium_frame_dump;
> > > +struct chamelium_fb_crc;
> > >  
> > >  struct chamelium *chamelium_init(int drm_fd);
> > >  void chamelium_deinit(struct chamelium *chamelium);
> > > @@ -92,6 +93,10 @@ struct chamelium_frame_dump
> > > *chamelium_port_dump_pixels(struct chamelium *chamel
> > >  							struct
> > > chamelium_port *port,
> > >  							int x,
> > > int
> > > y,
> > >  							int w,
> > > int
> > > h);
> > > +igt_crc_t *chamelium_calculate_fb_crc(int fd, struct igt_fb
> > > *fb);
> > > +struct chamelium_fb_crc *chamelium_calculate_fb_crc_launch(int
> > > fd,
> > > +							   struc
> > > t
> > > igt_fb *fb);
> > > +igt_crc_t *chamelium_calculate_fb_crc_result(struct
> > > chamelium_fb_crc
> > > *fb_crc);
> > >  int chamelium_get_captured_frame_count(struct chamelium
> > > *chamelium);
> > >  int chamelium_get_frame_limit(struct chamelium *chamelium,
> > >  			      struct chamelium_port *port,
> > > diff --git a/tests/chamelium.c b/tests/chamelium.c
> > > index e3067664..3fd2b02c 100644
> > > --- a/tests/chamelium.c
> > > +++ b/tests/chamelium.c
> > > @@ -49,43 +49,6 @@ typedef struct {
> > >  #define HPD_TOGGLE_COUNT_VGA 5
> > >  #define HPD_TOGGLE_COUNT_DP_HDMI 15
> > >  
> > > -/* Pre-calculated CRCs for the pattern fb, for all the modes in
> > > the
> > > default
> > > - * chamelium edid
> > > - */
> > > -struct crc_entry {
> > > -	int width;
> > > -	int height;
> > > -	igt_crc_t crc;
> > > -};
> > > -
> > > -#define CRC_ENTRY(w_, h_, ...) \
> > > -	{ w_, h_, { .n_words = 4, .crc = { __VA_ARGS__ } } }
> > > -
> > > -static const struct crc_entry pattern_fb_crcs[] = {
> > > -	CRC_ENTRY(1920, 1080, 0xf859, 0xa751, 0x8c81, 0x45a1),
> > > -	CRC_ENTRY(1280,  720, 0xcec2, 0x4246, 0x6cfd, 0xeb43),
> > > -	CRC_ENTRY(1024,  768, 0x85e5, 0xf0cd, 0xafe3, 0x7f18),
> > > -	CRC_ENTRY( 800,  600, 0x6b39, 0x32b6, 0x831a, 0xb03e),
> > > -	CRC_ENTRY( 640,  480, 0xa121, 0x2473, 0xb150, 0x8c47),
> > > -};
> > > -#undef CRC_ENTRY
> > > -
> > > -static const igt_crc_t *
> > > -get_precalculated_crc(struct chamelium_port *port, int w, int h)
> > > -{
> > > -	int i;
> > > -	const struct crc_entry *entry;
> > > -
> > > -	for (i = 0; i < ARRAY_SIZE(pattern_fb_crcs); i++) {
> > > -		entry = &pattern_fb_crcs[i];
> > > -
> > > -		if (entry->width == w && entry->height == h)
> > > -			return &entry->crc;
> > > -	}
> > > -
> > > -	return NULL;
> > > -}
> > > -
> > >  static void
> > >  require_connector_present(data_t *data, unsigned int type)
> > >  {
> > > @@ -424,7 +387,8 @@ test_display_crc_single(data_t *data, struct
> > > chamelium_port *port)
> > >  	igt_output_t *output;
> > >  	igt_plane_t *primary;
> > >  	igt_crc_t *crc;
> > > -	const igt_crc_t *expected_crc;
> > > +	igt_crc_t *expected_crc;
> > > +	struct chamelium_fb_crc *fb_crc;
> > >  	struct igt_fb fb;
> > >  	drmModeModeInfo *mode;
> > >  	drmModeConnector *connector;
> > > @@ -447,24 +411,21 @@ test_display_crc_single(data_t *data,
> > > struct
> > > chamelium_port *port)
> > >  						    0, 0, 0,
> > > &fb);
> > >  		igt_assert(fb_id > 0);
> > >  
> > > -		enable_output(data, port, output, mode, &fb);
> > > +		fb_crc = chamelium_calculate_fb_crc_launch(data-
> > > > drm_fd, &fb);
> > > 
> > >  
> > > -		expected_crc = get_precalculated_crc(port,
> > > -						     mode-
> > > > hdisplay,
> > > 
> > > -						     mode-
> > > > vdisplay);
> > > 
> > > -		if (!expected_crc) {
> > > -			igt_warn("No precalculated CRC found for
> > > %dx%d, skipping CRC check\n",
> > > -				 mode->hdisplay, mode-
> > > >vdisplay);
> > > -			goto next;
> > > -		}
> > > +		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);
> > >  
> > > -next:
> > >  		disable_output(data, port, output);
> > >  		igt_remove_fb(data->drm_fd, &fb);
> > >  	}
> > > @@ -480,7 +441,8 @@ test_display_crc_multiple(data_t *data,
> > > struct
> > > chamelium_port *port)
> > >  	igt_output_t *output;
> > >  	igt_plane_t *primary;
> > >  	igt_crc_t *crc;
> > > -	const igt_crc_t *expected_crc;
> > > +	igt_crc_t *expected_crc;
> > > +	struct chamelium_fb_crc *fb_crc;
> > >  	struct igt_fb fb;
> > >  	drmModeModeInfo *mode;
> > >  	drmModeConnector *connector;
> > > @@ -503,15 +465,9 @@ test_display_crc_multiple(data_t *data,
> > > struct
> > > chamelium_port *port)
> > >  						    0, 0, 0,
> > > &fb);
> > >  		igt_assert(fb_id > 0);
> > >  
> > > -		enable_output(data, port, output, mode, &fb);
> > > +		fb_crc = chamelium_calculate_fb_crc_launch(data-
> > > > drm_fd, &fb);
> > > 
> > >  
> > > -		expected_crc = get_precalculated_crc(port, mode-
> > > > hdisplay,
> > > 
> > > -						     mode-
> > > > vdisplay);
> > > 
> > > -		if (!expected_crc) {
> > > -			igt_warn("No precalculated CRC found for
> > > %dx%d, skipping CRC check\n",
> > > -				 mode->hdisplay, mode-
> > > >vdisplay);
> > > -			goto next;
> > > -		}
> > > +		enable_output(data, port, output, mode, &fb);
> > >  
> > >  		/* We want to keep the display running for a
> > > little
> > > bit, since
> > >  		 * there's always the potential the driver isn't
> > > able to keep
> > > @@ -522,11 +478,15 @@ test_display_crc_multiple(data_t *data,
> > > struct
> > > chamelium_port *port)
> > >  						   &captured_fra
> > > me_
> > > c
> > > ount);
> > >  
> > >  		igt_debug("Captured %d frames\n",
> > > captured_frame_count);
> > > +
> > > +		expected_crc =
> > > chamelium_calculate_fb_crc_result(fb_crc);
> > > +
> > >  		for (j = 0; j < captured_frame_count; j++)
> > >  			igt_assert_crc_equal(&crc[j],
> > > expected_crc);
> > > +
> > > +		free(expected_crc);
> > >  		free(crc);
> > >  
> > > -next:
> > >  		disable_output(data, port, output);
> > >  		igt_remove_fb(data->drm_fd, &fb);
> > >  	}
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux