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.
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@xxxxxxxxxxxxxxx>
> ---
> 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,
> + struct
> 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,
> + struct
> 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_frame_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
