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

Liviu Dudau
2019-01-16T11:20:09Z

On Tue, Jan 15, 2019 at 06:47:47PM +0000, Chris Wilson wrote:
> Quoting Liviu Dudau (2019-01-15 17:47:44)
> > +int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc)
> > +{
> > +#define FNV1a_OFFSET_BIAS 2166136261
> > +#define 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->is_dumb)
> > + map = kmstest_dumb_map_buffer(fb->fd, fb->gem_handle, fb->size,
> > + PROT_READ);
> > + else
> > + map = gem_mmap__gtt(fb->fd, fb->gem_handle, fb->size,
> > + PROT_READ);
> > + ptr = map;
> > +
> > + /*
> > + * 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) {
> > +
> > + memcpy(line, ptr, stride);
>
> igt_memcpy_from_wc() for the reasons cited above.
> -Chris

Hi Chris,

Thanks for pointing out the function, I was not aware of it!

Now, looking at the implementation, and ignoring the fact that it is in a
file called igt_x86.c, it looks to me that it will end up calling memcpy
anyway for Arm drivers. Not being a GCC expert, I am wondering if the
ifunc() wrapper around resolve_memcpy_from_wc() will not actually
prevent the compiler from choosing an optimised version of memcpy for Arm.

I can refresh the patch if you think it is safe to use igt_memcpy_from_wc() for
non-x86 architectures.

Best regards,
Liviu

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯