Re: [PATCH] intel: Adding locks for drm objects synchronization.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 05, 2014 at 02:51:38PM -0400, Rafal Sapala wrote:
> The changes make sure that members of the bufmgr_gem and bo_gem
> name lists are sychronized between threads
> when using the create from prime and create from name methods.
> 
> Signed-off-by: Rafal Sapala <rafal.a.sapala@intel.com>
> ---
>  intel/intel_bufmgr_gem.c |   28 ++++++++++++++++++++++++----
>  1 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 007a6d8..243f563 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -871,12 +871,14 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  	 * alternating names for the front/back buffer a linear search
>  	 * provides a sufficiently fast match.
>  	 */
> +	pthread_mutex_lock(&bufmgr_gem->lock);
>  	for (list = bufmgr_gem->named.next;
>  	     list != &bufmgr_gem->named;
>  	     list = list->next) {
>  		bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
>  		if (bo_gem->global_name == handle) {
>  			drm_intel_gem_bo_reference(&bo_gem->bo);
> +			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return &bo_gem->bo;
>  		}
>  	}
> @@ -889,6 +891,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  	if (ret != 0) {
>  		DBG("Couldn't reference %s handle 0x%08x: %s\n",
>  		    name, handle, strerror(errno));
> +		pthread_mutex_unlock(&bufmgr_gem->lock);
>  		return NULL;
>  	}
>          /* Now see if someone has used a prime handle to get this
> @@ -901,13 +904,16 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  		bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
>  		if (bo_gem->gem_handle == open_arg.handle) {
>  			drm_intel_gem_bo_reference(&bo_gem->bo);
> +			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return &bo_gem->bo;
>  		}
>  	}
>  
>  	bo_gem = calloc(1, sizeof(*bo_gem));
> -	if (!bo_gem)
> +	if (!bo_gem) {
> +		pthread_mutex_unlock(&bufmgr_gem->lock);
>  		return NULL;
> +	}
>  
>  	bo_gem->bo.size = open_arg.size;
>  	bo_gem->bo.offset = 0;
> @@ -929,6 +935,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  		       &get_tiling);
>  	if (ret != 0) {
>  		drm_intel_gem_bo_unreference(&bo_gem->bo);
> +		pthread_mutex_unlock(&bufmgr_gem->lock);
>  		return NULL;
>  	}
>  	bo_gem->tiling_mode = get_tiling.tiling_mode;
> @@ -938,6 +945,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>  
>  	DRMINITLISTHEAD(&bo_gem->vma_list);
>  	DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> +	pthread_mutex_unlock(&bufmgr_gem->lock);
>  	DBG("bo_create_from_handle: %d (%s)\n", handle, bo_gem->name);
>  
>  	return &bo_gem->bo;
> @@ -2502,25 +2510,29 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
>  	 * for named buffers, we must not create two bo's pointing at the same
>  	 * kernel object
>  	 */
> +	pthread_mutex_lock(&bufmgr_gem->lock);
>  	for (list = bufmgr_gem->named.next;
>  	     list != &bufmgr_gem->named;
>  	     list = list->next) {
>  		bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
>  		if (bo_gem->gem_handle == handle) {
>  			drm_intel_gem_bo_reference(&bo_gem->bo);
> +			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return &bo_gem->bo;
>  		}
>  	}
>  
>  	if (ret) {
>  	  fprintf(stderr,"ret is %d %d\n", ret, errno);
> +	  pthread_mutex_unlock(&bufmgr_gem->lock);
>  		return NULL;
>  	}
>  
>  	bo_gem = calloc(1, sizeof(*bo_gem));
> -	if (!bo_gem)
> +	if (!bo_gem) {
> +		pthread_mutex_unlock(&bufmgr_gem->lock);
>  		return NULL;
> -
> +	}
>  	/* Determine size of bo.  The fd-to-handle ioctl really should
>  	 * return the size, but it doesn't.  If we have kernel 3.12 or
>  	 * later, we can lseek on the prime fd to get the size.  Older
> @@ -2548,6 +2560,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
>  
>  	DRMINITLISTHEAD(&bo_gem->vma_list);
>  	DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> +	pthread_mutex_unlock(&bufmgr_gem->lock);
>  
>  	VG_CLEAR(get_tiling);
>  	get_tiling.handle = bo_gem->gem_handle;
> @@ -2572,8 +2585,10 @@ drm_intel_bo_gem_export_to_prime(drm_intel_bo *bo, int *prime_fd)
>  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
>  	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
>  
> +	pthread_mutex_lock(&bufmgr_gem->lock);
>          if (DRMLISTEMPTY(&bo_gem->name_list))
>                  DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> +	pthread_mutex_unlock(&bufmgr_gem->lock);
>  
>  	if (drmPrimeHandleToFD(bufmgr_gem->fd, bo_gem->gem_handle,
>  			       DRM_CLOEXEC, prime_fd) != 0)
> @@ -2597,15 +2612,20 @@ drm_intel_gem_bo_flink(drm_intel_bo *bo, uint32_t * name)
>  		VG_CLEAR(flink);
>  		flink.handle = bo_gem->gem_handle;
>  
> +		pthread_mutex_lock(&bufmgr_gem->lock);
> +
>  		ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_FLINK, &flink);
> -		if (ret != 0)
> +		if (ret != 0) {
> +			pthread_mutex_unlock(&bufmgr_gem->lock);
>  			return -errno;
> +		}
>  
>  		bo_gem->global_name = flink.name;
>  		bo_gem->reusable = false;
>  
>                  if (DRMLISTEMPTY(&bo_gem->name_list))
>                          DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> +		pthread_mutex_unlock(&bufmgr_gem->lock);
>  	}
>  
>  	*name = bo_gem->global_name;
> -- 
> 1.7.1
> 
> --------------------------------------------------------------------
> 
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.

I can't merge patches with this disclaimer ...

Patch looks good otherwise. Is there a testcase for it? As long as it's a
stand-alone C testcase I can easily do the integration with igt myself
quickly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch