[Intel-gfx] [PATCH 1/3] intel: Expect caller to guarantee thread-safety of individual bo

[Eric Anholt]

On Wed,  2 Dec 2009 11:43:14 +0000, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  intel/intel_bufmgr_gem.c |   27 +--------------------------
>  1 files changed, 1 insertions(+), 26 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 3b4d3cf..e0585ca 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -841,8 +841,6 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  	struct drm_i915_gem_set_domain set_domain;
>  	int ret;
>  
> -	pthread_mutex_lock(&bufmgr_gem->lock);
> -
>  	/* Allow recursive mapping. Mesa may recursively map buffers with
>  	 * nested display loops.
>  	 */
> @@ -865,7 +863,6 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  				"%s:%d: Error mapping buffer %d (%s): %s .\n",
>  				__FILE__, __LINE__, bo_gem->gem_handle,
>  				bo_gem->name, strerror(errno));
> -			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return ret;
>  		}
>  		bo_gem->mem_virtual = (void *)(uintptr_t) mmap_arg.addr_ptr;
> @@ -889,12 +886,9 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  		fprintf(stderr, "%s:%d: Error setting to CPU domain %d: %s\n",
>  			__FILE__, __LINE__, bo_gem->gem_handle,
>  			strerror(errno));
> -		pthread_mutex_unlock(&bufmgr_gem->lock);
>  		return ret;
>  	}
>  
> -	pthread_mutex_unlock(&bufmgr_gem->lock);
> -
>  	return 0;
>  }
>  
> @@ -905,8 +899,6 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  	struct drm_i915_gem_set_domain set_domain;
>  	int ret;
>  
> -	pthread_mutex_lock(&bufmgr_gem->lock);
> -
>  	/* Get a mapping of the buffer if we haven't before. */
>  	if (bo_gem->gtt_virtual == NULL) {
>  		struct drm_i915_gem_mmap_gtt mmap_arg;
> @@ -929,7 +921,6 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  				__FILE__, __LINE__,
>  				bo_gem->gem_handle, bo_gem->name,
>  				strerror(errno));
> -			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return ret;
>  		}
>  
> @@ -943,7 +934,6 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  				__FILE__, __LINE__,
>  				bo_gem->gem_handle, bo_gem->name,
>  				strerror(errno));
> -			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return errno;
>  		}
>  	}
> @@ -969,27 +959,19 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  			strerror(errno));
>  	}
>  
> -	pthread_mutex_unlock(&bufmgr_gem->lock);
> -
>  	return ret;
>  }
>  
>  int drm_intel_gem_bo_unmap_gtt(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;
> -	int ret = 0;
> -
>  	if (bo == NULL)
>  		return 0;
>  
>  	assert(bo_gem->gtt_virtual != NULL);
> -
> -	pthread_mutex_lock(&bufmgr_gem->lock);
>  	bo->virtual = NULL;
> -	pthread_mutex_unlock(&bufmgr_gem->lock);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int drm_intel_gem_bo_unmap(drm_intel_bo *bo)
> @@ -1004,8 +986,6 @@ static int drm_intel_gem_bo_unmap(drm_intel_bo *bo)
>  
>  	assert(bo_gem->mem_virtual != NULL);
>  
> -	pthread_mutex_lock(&bufmgr_gem->lock);
> -
>  	/* Cause a flush to happen if the buffer's pinned for scanout, so the
>  	 * results show up in a timely manner.
>  	 */
> @@ -1017,7 +997,6 @@ static int drm_intel_gem_bo_unmap(drm_intel_bo *bo)
>  	} while (ret == -1 && errno == EINTR);
>  
>  	bo->virtual = NULL;
> -	pthread_mutex_unlock(&bufmgr_gem->lock);
>  	return 0;
>  }

This is an actual change in the intended behavior of the library.
Mesa's multithreaded handling expected mapping to be reentrant with no
additional protection.  So I'm uncomfortable with these hunks.

>  
> @@ -1186,8 +1165,6 @@ drm_intel_gem_bo_emit_reloc(drm_intel_bo *bo, uint32_t offset,
>  	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
>  	drm_intel_bo_gem *target_bo_gem = (drm_intel_bo_gem *) target_bo;
>  
> -	pthread_mutex_lock(&bufmgr_gem->lock);
> -
>  	/* Create a new relocation list if needed */
>  	if (bo_gem->relocs == NULL)
>  		drm_intel_setup_reloc_list(bo);
> @@ -1222,8 +1199,6 @@ drm_intel_gem_bo_emit_reloc(drm_intel_bo *bo, uint32_t offset,
>  
>  	bo_gem->reloc_count++;
>  
> -	pthread_mutex_unlock(&bufmgr_gem->lock);
> -
>  	return 0;
>  }

I liked this hunk, being safe as far as Mesa's concerned, and the
highest thing in the profile for mutex locking.