| Message ID | 20191022005952.tkdtftzbxk4b2lzk@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for testing writeback connectors | expand |
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@arm.com> > [rebased and updated to the most recent API] > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > [rebased and updated the patch to address feedback] > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Reviewed-by: Simon Ser <simon.ser@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
Hi Maxime, First of all, thank you a lot for your review and for testing this patchset. I'm going to prepare a new version trying to address all issues highlight by you, Petri, and Simon (I'm already working in a new version). Just a note, I run this test on top of VKMS in a virtual machine (x86). Best Regards On 04/15, Maxime Ripard wrote: > 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@arm.com> > > [rebased and updated to the most recent API] > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > > [rebased and updated the patch to address feedback] > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Reviewed-by: Simon Ser <simon.ser@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
Hi, On Thu, Apr 16, 2020 at 08:49:42AM -0400, Rodrigo Siqueira wrote: > First of all, thank you a lot for your review and for testing this > patchset. I'm going to prepare a new version trying to address all > issues highlight by you, Petri, and Simon (I'm already working in a new > version). That would be awesome, thanks :) > Just a note, I run this test on top of VKMS in a virtual machine (x86). Yeah, that's what I assumed. Cc me when the new version will be ready, I'll give it a shot on an arm32 board. Maxime
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); + + /* + * 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); + + return 0; +#undef FNV1a_OFFSET_BIAS +#undef FNV1a_PRIME +} + /** * igt_format_is_yuv: * @drm_format: drm fourcc diff --git a/lib/igt_fb.h b/lib/igt_fb.h index 69132b41..d2394638 100644 --- a/lib/igt_fb.h +++ b/lib/igt_fb.h @@ -202,5 +202,7 @@ int igt_format_plane_bpp(uint32_t drm_format, int plane); void igt_format_array_fill(uint32_t **formats_array, unsigned int *count, bool allow_yuv); +int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc); + #endif /* __IGT_FB_H__ */
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@arm.com> [rebased and updated to the most recent API] Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> [rebased and updated the patch to address feedback] Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Reviewed-by: Simon Ser <simon.ser@intel.com> --- lib/igt_fb.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/igt_fb.h | 2 ++ 2 files changed, 70 insertions(+)