Re: [PATCH 1/2] intel: Mark up with valgrind intrinsics to reduce false positives

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/09/2012 11:43 AM, Chris Wilson wrote:
> In particular, declare the hidden CPU mmaps to valgrind so that it knows
> about those memory regions.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=35071
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Acked-by: Ben Widawsky <ben@xxxxxxxxxxxx>

> ---
>  configure.ac             |    3 ++
>  intel/Makefile.am        |    1 +
>  intel/intel_bufmgr_gem.c |   52 +++++++++++++++++++++++++++++++++------------
>  3 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 773167f..3ca7b79 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -258,6 +258,9 @@ if test "x$INTEL" != "xno" -o "x$RADEON" != "xno"; then
>      fi
>  fi
>  
> +PKG_CHECK_MODULES(VALGRIND, [valgrind],
> +		  AC_DEFINE([HAVE_VALGRIND], 1, [Use valgrind intrinsics to suppress false warings]),)
> +
>  AM_CONDITIONAL(HAVE_INTEL, [test "x$INTEL" != "xno"])
>  AM_CONDITIONAL(HAVE_RADEON, [test "x$RADEON" != "xno"])
>  if test "x$RADEON" = xyes; then
> diff --git a/intel/Makefile.am b/intel/Makefile.am
> index 581c8c0..e313614 100644
> --- a/intel/Makefile.am
> +++ b/intel/Makefile.am
> @@ -28,6 +28,7 @@ AM_CFLAGS = \
>  	-I$(top_srcdir)/intel \
>  	$(PTHREADSTUBS_CFLAGS) \
>  	$(PCIACCESS_CFLAGS) \
> +	$(VALGRIND_CFLAGS) \
>  	-I$(top_srcdir)/include/drm
>  
>  libdrm_intel_la_LTLIBRARIES = libdrm_intel.la
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 2b4fab1..4d30e62 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -62,6 +62,16 @@
>  
>  #include "i915_drm.h"
>  
> +#ifdef HAVE_VALGRIND
> +#include <valgrind.h>
> +#include <memcheck.h>
> +#define VG(x) x
> +#else
> +#define VG(x)
> +#endif
> +
> +#define VG_CLEAR(s) VG(memset(&s, 0, sizeof(s)))
> +
>  #define DBG(...) do {					\
>  	if (bufmgr_gem->bufmgr.debug)			\
>  		fprintf(stderr, __VA_ARGS__);		\
> @@ -538,7 +548,7 @@ drm_intel_gem_bo_busy(drm_intel_bo *bo)
>  	struct drm_i915_gem_busy busy;
>  	int ret;
>  
> -	memset(&busy, 0, sizeof(busy));
> +	VG_CLEAR(busy);
>  	busy.handle = bo_gem->gem_handle;
>  
>  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> @@ -552,6 +562,7 @@ drm_intel_gem_bo_madvise_internal(drm_intel_bufmgr_gem *bufmgr_gem,
>  {
>  	struct drm_i915_gem_madvise madv;
>  
> +	VG_CLEAR(madv);
>  	madv.handle = bo_gem->gem_handle;
>  	madv.madv = state;
>  	madv.retained = 1;
> @@ -679,7 +690,8 @@ retry:
>  			return NULL;
>  
>  		bo_gem->bo.size = bo_size;
> -		memset(&create, 0, sizeof(create));
> +
> +		VG_CLEAR(create);
>  		create.size = bo_size;
>  
>  		ret = drmIoctl(bufmgr_gem->fd,
> @@ -835,7 +847,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  	if (!bo_gem)
>  		return NULL;
>  
> -	memset(&open_arg, 0, sizeof(open_arg));
> +	VG_CLEAR(open_arg);
>  	open_arg.name = handle;
>  	ret = drmIoctl(bufmgr_gem->fd,
>  		       DRM_IOCTL_GEM_OPEN,
> @@ -858,7 +870,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  	bo_gem->global_name = handle;
>  	bo_gem->reusable = false;
>  
> -	memset(&get_tiling, 0, sizeof(get_tiling));
> +	VG_CLEAR(get_tiling);
>  	get_tiling.handle = bo_gem->gem_handle;
>  	ret = drmIoctl(bufmgr_gem->fd,
>  		       DRM_IOCTL_I915_GEM_GET_TILING,
> @@ -889,6 +901,7 @@ drm_intel_gem_bo_free(drm_intel_bo *bo)
>  
>  	DRMLISTDEL(&bo_gem->vma_list);
>  	if (bo_gem->mem_virtual) {
> +		VG(VALGRIND_FREELIKE_BLOCK(bo_gem->mem_virtual, 0));
>  		munmap(bo_gem->mem_virtual, bo_gem->bo.size);
>  		bufmgr_gem->vma_count--;
>  	}
> @@ -898,7 +911,7 @@ drm_intel_gem_bo_free(drm_intel_bo *bo)
>  	}
>  
>  	/* Close this object */
> -	memset(&close, 0, sizeof(close));
> +	VG_CLEAR(close);
>  	close.handle = bo_gem->gem_handle;
>  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close);
>  	if (ret != 0) {
> @@ -1103,7 +1116,7 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  		DBG("bo_map: %d (%s), map_count=%d\n",
>  		    bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
>  
> -		memset(&mmap_arg, 0, sizeof(mmap_arg));
> +		VG_CLEAR(mmap_arg);
>  		mmap_arg.handle = bo_gem->gem_handle;
>  		mmap_arg.offset = 0;
>  		mmap_arg.size = bo->size;
> @@ -1120,12 +1133,14 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return ret;
>  		}
> +		VG(VALGRIND_MALLOCLIKE_BLOCK(mmap_arg.addr_ptr, mmap_arg.size, 0, 1));
>  		bo_gem->mem_virtual = (void *)(uintptr_t) mmap_arg.addr_ptr;
>  	}
>  	DBG("bo_map: %d (%s) -> %p\n", bo_gem->gem_handle, bo_gem->name,
>  	    bo_gem->mem_virtual);
>  	bo->virtual = bo_gem->mem_virtual;
>  
> +	VG_CLEAR(set_domain);
>  	set_domain.handle = bo_gem->gem_handle;
>  	set_domain.read_domains = I915_GEM_DOMAIN_CPU;
>  	if (write_enable)
> @@ -1168,7 +1183,7 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  		DBG("bo_map_gtt: mmap %d (%s), map_count=%d\n",
>  		    bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
>  
> -		memset(&mmap_arg, 0, sizeof(mmap_arg));
> +		VG_CLEAR(mmap_arg);
>  		mmap_arg.handle = bo_gem->gem_handle;
>  
>  		/* Get the fake offset back... */
> @@ -1211,6 +1226,7 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  	    bo_gem->gtt_virtual);
>  
>  	/* Now move it to the GTT domain so that the CPU caches are flushed */
> +	VG_CLEAR(set_domain);
>  	set_domain.handle = bo_gem->gem_handle;
>  	set_domain.read_domains = I915_GEM_DOMAIN_GTT;
>  	set_domain.write_domain = I915_GEM_DOMAIN_GTT;
> @@ -1232,7 +1248,6 @@ static int drm_intel_gem_bo_unmap(drm_intel_bo *bo)
>  {
>  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
>  	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
> -	struct drm_i915_gem_sw_finish sw_finish;
>  	int ret = 0;
>  
>  	if (bo == NULL)
> @@ -1250,11 +1265,14 @@ static int drm_intel_gem_bo_unmap(drm_intel_bo *bo)
>  	}
>  
>  	if (bo_gem->mapped_cpu_write) {
> +		struct drm_i915_gem_sw_finish sw_finish;
> +
>  		/* Cause a flush to happen if the buffer's pinned for
>  		 * scanout, so the results show up in a timely manner.
>  		 * Unlike GTT set domains, this only does work if the
>  		 * buffer should be scanout-related.
>  		 */
> +		VG_CLEAR(sw_finish);
>  		sw_finish.handle = bo_gem->gem_handle;
>  		ret = drmIoctl(bufmgr_gem->fd,
>  			       DRM_IOCTL_I915_GEM_SW_FINISH,
> @@ -1291,7 +1309,7 @@ drm_intel_gem_bo_subdata(drm_intel_bo *bo, unsigned long offset,
>  	struct drm_i915_gem_pwrite pwrite;
>  	int ret;
>  
> -	memset(&pwrite, 0, sizeof(pwrite));
> +	VG_CLEAR(pwrite);
>  	pwrite.handle = bo_gem->gem_handle;
>  	pwrite.offset = offset;
>  	pwrite.size = size;
> @@ -1316,6 +1334,7 @@ drm_intel_gem_get_pipe_from_crtc_id(drm_intel_bufmgr *bufmgr, int crtc_id)
>  	struct drm_i915_get_pipe_from_crtc_id get_pipe_from_crtc_id;
>  	int ret;
>  
> +	VG_CLEAR(get_pipe_from_crtc_id);
>  	get_pipe_from_crtc_id.crtc_id = crtc_id;
>  	ret = drmIoctl(bufmgr_gem->fd,
>  		       DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID,
> @@ -1342,7 +1361,7 @@ drm_intel_gem_bo_get_subdata(drm_intel_bo *bo, unsigned long offset,
>  	struct drm_i915_gem_pread pread;
>  	int ret;
>  
> -	memset(&pread, 0, sizeof(pread));
> +	VG_CLEAR(pread);
>  	pread.handle = bo_gem->gem_handle;
>  	pread.offset = offset;
>  	pread.size = size;
> @@ -1382,6 +1401,7 @@ drm_intel_gem_bo_start_gtt_access(drm_intel_bo *bo, int write_enable)
>  	struct drm_i915_gem_set_domain set_domain;
>  	int ret;
>  
> +	VG_CLEAR(set_domain);
>  	set_domain.handle = bo_gem->gem_handle;
>  	set_domain.read_domains = I915_GEM_DOMAIN_GTT;
>  	set_domain.write_domain = write_enable ? I915_GEM_DOMAIN_GTT : 0;
> @@ -1690,6 +1710,7 @@ drm_intel_gem_bo_exec(drm_intel_bo *bo, int used,
>  	 */
>  	drm_intel_add_validate_buffer(bo);
>  
> +	VG_CLEAR(execbuf);
>  	execbuf.buffers_ptr = (uintptr_t) bufmgr_gem->exec_objects;
>  	execbuf.buffer_count = bufmgr_gem->exec_count;
>  	execbuf.batch_start_offset = 0;
> @@ -1769,6 +1790,7 @@ drm_intel_gem_bo_mrb_exec2(drm_intel_bo *bo, int used,
>  	 */
>  	drm_intel_add_validate_buffer2(bo, 0);
>  
> +	VG_CLEAR(execbuf);
>  	execbuf.buffers_ptr = (uintptr_t)bufmgr_gem->exec2_objects;
>  	execbuf.buffer_count = bufmgr_gem->exec_count;
>  	execbuf.batch_start_offset = 0;
> @@ -1833,7 +1855,7 @@ drm_intel_gem_bo_pin(drm_intel_bo *bo, uint32_t alignment)
>  	struct drm_i915_gem_pin pin;
>  	int ret;
>  
> -	memset(&pin, 0, sizeof(pin));
> +	VG_CLEAR(pin);
>  	pin.handle = bo_gem->gem_handle;
>  	pin.alignment = alignment;
>  
> @@ -1855,7 +1877,7 @@ drm_intel_gem_bo_unpin(drm_intel_bo *bo)
>  	struct drm_i915_gem_unpin unpin;
>  	int ret;
>  
> -	memset(&unpin, 0, sizeof(unpin));
> +	VG_CLEAR(unpin);
>  	unpin.handle = bo_gem->gem_handle;
>  
>  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_UNPIN, &unpin);
> @@ -1941,16 +1963,18 @@ drm_intel_gem_bo_flink(drm_intel_bo *bo, uint32_t * name)
>  {
>  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
>  	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
> -	struct drm_gem_flink flink;
>  	int ret;
>  
>  	if (!bo_gem->global_name) {
> -		memset(&flink, 0, sizeof(flink));
> +		struct drm_gem_flink flink;
> +
> +		VG_CLEAR(flink);
>  		flink.handle = bo_gem->gem_handle;
>  
>  		ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_FLINK, &flink);
>  		if (ret != 0)
>  			return -errno;
> +
>  		bo_gem->global_name = flink.name;
>  		bo_gem->reusable = false;
>  

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux