[igt-dev] [PATCH i-g-t 1/4] lib/igt_fb: add functionality of getting framebuffer 16-bit CRC.
Kazlauskas, Nicholas
Nicholas.Kazlauskas at amd.com
Tue Jun 25 16:20:05 UTC 2019
On 6/24/19 10:31 AM, Dingchen Zhang wrote:
> Need framebuffer CRC to validate AMDGPU bypass mode.
>
> For each color component less than 16-bits, padding zero bits,
> loop to update CRC for each RGB compoment in the framebuffer.
> The algorithm is based on DP spec v1.4.
>
> Cc: Harry Wentland <Harry.Wentland at amd.com>
> Cc: Nick Kazlauskas <Nicholas.Kazlauskas at amd.com>
> Change-Id: Ifc2076df6c9b2156db23cae7ad968c61f404d0e4
Drop the Change-Id's from these patches.
> Signed-off-by: Dingchen Zhang <dingchen.zhang at amd.com>
> ---
> lib/igt_fb.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++++++
> lib/igt_fb.h | 2 +
> 2 files changed, 197 insertions(+)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 9d4f905e..2ac943c0 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -65,6 +65,7 @@
> */
>
> #define PIXMAN_invalid 0
> +#define CRC_INIT_16 0x0000
Drop this define, it's only used within the CRC function itself and it's
just zero.
>
> #if CAIRO_VERSION < CAIRO_VERSION_ENCODE(1, 17, 2)
> /*
> @@ -903,6 +904,200 @@ int igt_create_bo_with_dimensions(int fd, int width, int height,
> return fb.gem_handle;
> }
>
> +#define get_u16_bit(x, n) ((x & (1 << n)) >> n )
> +#define set_u16_bit(x, n, val) ((x & ~(1 << n)) | (val << n))
> +/*
> + * update_crc16_dp:
> + * @crc_old: old 16-bit CRC value to be updated
> + * @d: input 16-bit data on which to calculate 16-bit CRC
> + *
> + * CRC algorithm implementation described in DP 1.4 spec Appendix J
> + * the 16-bit CRC IBM is applied, with the following polynomial:
> + *
> + * f(x) = x ^ 16 + x ^ 15 + x ^ 2 + 1
> + *
> + * the MSB is shifted in first, for any color format that is less than 16 bits
> + * per component, the LSB is zero-padded.
> + *
> + * The following implementation is based on the hardware parallel 16-bit CRC
> + * generation and ported to C code.
> + *
> + * Reference: VESA DisplayPort Standard v1.4, appendix J
> + *
> + * Returns:
> + * updated 16-bit CRC value.
> + */
> +static uint16_t update_crc16_dp(uint16_t crc_old, uint16_t d)
> +{
> + uint16_t crc_new = 0; /* 16-bit CRC output */
> +
> + /* internal use */
> + uint16_t b = crc_old;
> + uint8_t val;
> +
> + /* b[15] */
> + val = get_u16_bit(b, 0) ^ get_u16_bit(b, 1) ^ get_u16_bit(b, 2) ^
> + get_u16_bit(b, 3) ^ get_u16_bit(b, 4) ^ get_u16_bit(b, 5) ^
> + get_u16_bit(b, 6) ^ get_u16_bit(b, 7) ^ get_u16_bit(b, 8) ^
> + get_u16_bit(b, 9) ^ get_u16_bit(b, 10) ^ get_u16_bit(b, 11) ^
> + get_u16_bit(b, 12) ^ get_u16_bit(b, 14) ^ get_u16_bit(b, 15) ^
> + get_u16_bit(d, 0) ^ get_u16_bit(d, 1) ^ get_u16_bit(d, 2) ^
> + get_u16_bit(d, 3) ^ get_u16_bit(d, 4) ^ get_u16_bit(d, 5) ^
> + get_u16_bit(d, 6) ^ get_u16_bit(d, 7) ^ get_u16_bit(d, 8) ^
> + get_u16_bit(d, 9) ^ get_u16_bit(d, 10) ^ get_u16_bit(d, 11) ^
> + get_u16_bit(d, 12) ^ get_u16_bit(d, 14) ^ get_u16_bit(d, 15);
> + crc_new = set_u16_bit(crc_new, 15, val);
> +
> + /* b[14] */
> + val = get_u16_bit(b, 12) ^ get_u16_bit(b, 13) ^
> + get_u16_bit(d, 12) ^ get_u16_bit(d, 13);
> + crc_new = set_u16_bit(crc_new, 14, val);
> +
> + /* b[13] */
> + val = get_u16_bit(b, 11) ^ get_u16_bit(b, 12) ^
> + get_u16_bit(d, 11) ^ get_u16_bit(d, 12);
> + crc_new = set_u16_bit(crc_new, 13, val);
> +
> + /* b[12] */
> + val = get_u16_bit(b, 10) ^ get_u16_bit(b, 11) ^
> + get_u16_bit(d, 10) ^ get_u16_bit(d, 11);
> + crc_new = set_u16_bit(crc_new, 12, val);
> +
> + /* b[11] */
> + val = get_u16_bit(b, 9) ^ get_u16_bit(b, 10) ^
> + get_u16_bit(d, 9) ^ get_u16_bit(d, 10);
> + crc_new = set_u16_bit(crc_new, 11, val);
> +
> + /* b[10] */
> + val = get_u16_bit(b, 8) ^ get_u16_bit(b, 9) ^
> + get_u16_bit(d, 8) ^ get_u16_bit(d, 9);
> + crc_new = set_u16_bit(crc_new, 10, val);
> +
> + /* b[9] */
> + val = get_u16_bit(b, 7) ^ get_u16_bit(b, 8) ^
> + get_u16_bit(d, 7) ^ get_u16_bit(d, 8);
> + crc_new = set_u16_bit(crc_new, 9, val);
> +
> + /* b[8] */
> + val = get_u16_bit(b, 6) ^ get_u16_bit(b, 7) ^
> + get_u16_bit(d, 6) ^ get_u16_bit(d, 7);
> + crc_new = set_u16_bit(crc_new, 8, val);
> +
> + /* b[7] */
> + val = get_u16_bit(b, 5) ^ get_u16_bit(b, 6) ^
> + get_u16_bit(d, 5) ^ get_u16_bit(d, 6);
> + crc_new = set_u16_bit(crc_new, 7, val);
> +
> + /* b[6] */
> + val = get_u16_bit(b, 4) ^ get_u16_bit(b, 5) ^
> + get_u16_bit(d, 4) ^ get_u16_bit(d, 5);
> + crc_new = set_u16_bit(crc_new, 6, val);
> +
> + /* b[5] */
> + val = get_u16_bit(b, 3) ^ get_u16_bit(b, 4) ^
> + get_u16_bit(d, 3) ^ get_u16_bit(d, 4);
> + crc_new = set_u16_bit(crc_new, 5, val);
> +
> + /* b[4] */
> + val = get_u16_bit(b, 2) ^ get_u16_bit(b, 3) ^
> + get_u16_bit(d, 2) ^ get_u16_bit(d, 3);
> + crc_new = set_u16_bit(crc_new, 4, val);
> +
> + /* b[3] */
> + val = get_u16_bit(b, 1) ^ get_u16_bit(b, 2) ^ get_u16_bit(b, 15) ^
> + get_u16_bit(d, 1) ^ get_u16_bit(d, 2) ^ get_u16_bit(d, 15);
> + crc_new = set_u16_bit(crc_new, 3, val);
> +
> + /* b[2] */
> + val = get_u16_bit(b, 0) ^ get_u16_bit(b, 1) ^ get_u16_bit(b, 14) ^
> + get_u16_bit(d, 0) ^ get_u16_bit(d, 1) ^ get_u16_bit(d, 14);
> + crc_new = set_u16_bit(crc_new, 2, val);
> +
> + /* b[1] */
> + val = get_u16_bit(b, 1) ^ get_u16_bit(b, 2) ^ get_u16_bit(b, 3) ^
> + get_u16_bit(b, 4) ^ get_u16_bit(b, 5) ^ get_u16_bit(b, 6) ^
> + get_u16_bit(b, 7) ^ get_u16_bit(b, 8) ^ get_u16_bit(b, 9) ^
> + get_u16_bit(b, 10) ^ get_u16_bit(b, 11) ^ get_u16_bit(b, 12) ^
> + get_u16_bit(b, 13) ^ get_u16_bit(b, 14) ^
> + get_u16_bit(d, 1) ^ get_u16_bit(d, 2) ^ get_u16_bit(d, 3) ^
> + get_u16_bit(d, 4) ^ get_u16_bit(d, 5) ^ get_u16_bit(d, 6) ^
> + get_u16_bit(d, 7) ^ get_u16_bit(d, 8) ^ get_u16_bit(d, 9) ^
> + get_u16_bit(d, 10) ^ get_u16_bit(d, 11) ^ get_u16_bit(d, 12) ^
> + get_u16_bit(d, 13) ^ get_u16_bit(d, 14);
> + crc_new = set_u16_bit(crc_new, 1, val);
> +
> + /* b[0] */
> + val = get_u16_bit(b, 0) ^ get_u16_bit(b, 1) ^ get_u16_bit(b, 2) ^
> + get_u16_bit(b, 3) ^ get_u16_bit(b, 4) ^ get_u16_bit(b, 5) ^
> + get_u16_bit(b, 6) ^ get_u16_bit(b, 7) ^ get_u16_bit(b, 8) ^
> + get_u16_bit(b, 9) ^ get_u16_bit(b, 10) ^ get_u16_bit(b, 11) ^
> + get_u16_bit(b, 12) ^ get_u16_bit(b, 13) ^ get_u16_bit(b, 15) ^
> + get_u16_bit(d, 0) ^ get_u16_bit(d, 1) ^ get_u16_bit(d, 2) ^
> + get_u16_bit(d, 3) ^ get_u16_bit(d, 4) ^ get_u16_bit(d, 5) ^
> + get_u16_bit(d, 6) ^ get_u16_bit(d, 7) ^ get_u16_bit(d, 8) ^
> + get_u16_bit(d, 9) ^ get_u16_bit(d, 10) ^ get_u16_bit(d, 11) ^
> + get_u16_bit(d, 12) ^ get_u16_bit(d, 13) ^ get_u16_bit(d, 15);
> + crc_new = set_u16_bit(crc_new, 0, val);
> +
> + return crc_new;
> +}
This is quite verbose, but matching the spec is better than trying to be
overly aggressive with optimization. I'm fine with this.
> +
> +/**
> + * igt_fb_calc_crc:
> + * @fb: pointer to an #igt_fb structure
> + * @crc: pointer to an #igt_crc_t structure
> + *
> + * This function calculate the 16-bit frame CRC of RGB components over all
> + * the active pixels.
> + */
> +void igt_fb_calc_crc(struct igt_fb *fb, igt_crc_t *crc)
> +{
> + int x, y, i;
> + void *ptr;
> + uint8_t *data;
> + uint16_t din;
> +
> + igt_assert(fb && crc);
> +
> + ptr = igt_fb_map_buffer(fb->fd, fb);
> + igt_assert(ptr);
> +
> + /* set for later CRC comparison */
> + crc->has_valid_frame = true;
> + crc->frame = 0;
> + crc->n_words = 3;
> + crc->crc[0] = CRC_INIT_16; /* R */
> + crc->crc[1] = CRC_INIT_16; /* G */
> + crc->crc[2] = CRC_INIT_16; /* B */
> +
> + data = ptr + fb->offsets[0];
> + for (y = 0; y < fb->height; ++y) {
> + for (x = 0; x < fb->width; ++x) {
> + switch (fb->drm_format) {
> + case DRM_FORMAT_XRGB8888:
> + i = x * 4 + y * fb->strides[0];
> +
> + din = data[i + 2] << 8; /* padding-zeros */
> + crc->crc[0] = update_crc16_dp(crc->crc[0], din);
> +
> + /* Green-component */
> + din = data[i + 1] << 8;
> + crc->crc[1] = update_crc16_dp(crc->crc[1], din);
> +
> + /* Blue-component */
> + din = data[i] << 8;
> + crc->crc[2] = update_crc16_dp(crc->crc[2], din);
> + break;
> + default:
> + igt_assert_f(0, "DRM Format Invalid");
> + break;
> + }
> + }
> + }
> +
> + igt_fb_unmap_buffer(fb, ptr);
> +}
> +
> /**
> * igt_paint_color:
> * @cr: cairo drawing context
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index adefebe1..be786911 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -37,6 +37,7 @@
> #include <i915_drm.h>
>
> #include "igt_color_encoding.h"
> +#include "igt_debugfs.h"
This include really only needs to be within igt_fb.c itself as long as
you forward declare igt_crc_t, but you'd need igt_debugfs.h to really
use the igt_crc_t anyway.
I guess it's fine to have to it here.
>
> /*
> * Internal format to denote a buffer compatible with pixman's
> @@ -158,6 +159,7 @@ int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format
> uint64_t modifier, unsigned stride,
> uint64_t *size_ret, unsigned *stride_ret,
> bool *is_dumb);
> +void igt_fb_calc_crc(struct igt_fb *fb, igt_crc_t *crc);
>
> uint64_t igt_fb_mod_to_tiling(uint64_t modifier);
> uint64_t igt_fb_tiling_to_mod(uint64_t tiling);
>
With the fixes I've mentioned, this change is:
Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
Nicholas Kazlauskas
More information about the igt-dev
mailing list