[igt-dev] [Intel-gfx] [PATCH v7 i-g-t 3/4] lib: Add function to hash a framebuffer

[Maxime Ripard]

Hi Rodrigo,

I gave your (and Brian's) patches on a RPi, and there's a couple of
things that need to be fixed.

On Mon, Oct 21, 2019 at 10:00:00PM -0300, Brian Starkey wrote:
> To use writeback buffers as a CRC source, we need to be able to hash
> them. Implement a simple FVA-1a hashing routine for this purpose.
>
> Doing a bytewise hash on the framebuffer directly can be very slow if
> the memory is noncached. By making a copy of each line in the FB first
> (which can take advantage of word-access speedup), we can do the hash
> on a cached copy, which is much faster (10x speedup on my platform).
>
> V6: Simon Sir
>  - Replace #define by plain uint32_t variables
>  - Return -EINVAL in case fb->num_planes != 1
>  - Directly assign the mmap result to ptr
>  - No need to copy the whole stride, just copy fb->width * cpp since
> we're only going to read that
>
> v5: use igt_memcpy_from_wc() instead of plain memcpy, as suggested by
>     Chris Wilson
>
> Signed-off-by: Brian Starkey <brian.starkey at arm.com>
> [rebased and updated to the most recent API]
> Signed-off-by: Liviu Dudau <liviu.dudau at arm.com>
> [rebased and updated the patch to address feedback]
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Reviewed-by: Simon Ser <simon.ser at intel.com>
> ---
>  lib/igt_fb.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_fb.h |  2 ++
>  2 files changed, 70 insertions(+)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 6b674c1b..64d52634 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -3491,6 +3491,74 @@ bool igt_fb_supported_format(uint32_t drm_format)
>  	return false;
>  }
>
> +/*
> + * This implements the FNV-1a hashing algorithm instead of CRC, for
> + * simplicity
> + * http://www.isthe.com/chongo/tech/comp/fnv/index.html
> + *
> + * hash = offset_basis
> + * for each octet_of_data to be hashed
> + *         hash = hash xor octet_of_data
> + *         hash = hash * FNV_prime
> + * return hash
> + *
> + * 32 bit offset_basis = 2166136261
> + * 32 bit FNV_prime = 224 + 28 + 0x93 = 16777619
> + */
> +int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc)
> +{
> +	uint32_t FNV1a_OFFSET_BIAS = 2166136261;
> +	uint32_t FNV1a_PRIME = 16777619;
> +	uint32_t hash;
> +	void *map;
> +	char *ptr, *line = NULL;
> +	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
> +	uint32_t stride = calc_plane_stride(fb, 0);
> +
> +	if (fb->num_planes != 1)
> +		return -EINVAL;
> +
> +	if (fb->is_dumb)
> +		ptr = kmstest_dumb_map_buffer(fb->fd, fb->gem_handle, fb->size,
> +					      PROT_READ);
> +	else
> +		ptr = gem_mmap__gtt(fb->fd, fb->gem_handle, fb->size,
> +				    PROT_READ);

You should be using igt_fb_map_buffer here

> +	/*
> +	 * Framebuffers are often uncached, which can make byte-wise accesses
> +	 * very slow. We copy each line of the FB into a local buffer to speed
> +	 * up the hashing.
> +	 */
> +	line = malloc(stride);
> +	if (!line) {
> +		munmap(map, fb->size);
> +		return -ENOMEM;
> +	}
> +
> +	hash = FNV1a_OFFSET_BIAS;
> +
> +	for (y = 0; y < fb->height; y++, ptr += stride) {
> +
> +		igt_memcpy_from_wc(line, ptr, fb->width * cpp);
> +
> +		for (x = 0; x < fb->width * cpp; x++) {
> +			hash ^= line[x];
> +			hash *= FNV1a_PRIME;
> +		}
> +	}
> +
> +	crc->n_words = 1;
> +	crc->crc[0] = hash;
> +
> +	free(line);
> +	munmap(map, fb->size);

And this will lead to a segfault here, since map has not been
initialized. I'm assuming the intention is to have map be the returned
value of mmap, and ptr to be initialized to that same value, and use
that as your current line pointer later on (the error path from the
malloc has the same issue).

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20200415/542c3325/attachment.sig>