Re: [PATCH 6/7] drm/radeon: take VM lock before BO locks

["Christian König" <deathsimple@vodafone.de>]

Ah, crap. Drop the last two, they are still work in progress.

I was on the wrong branch while sending them,
Christian.

Am 27.11.2014 um 14:48 schrieb Christian König:
> From: Christian König <christian.koenig@amd.com>
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/radeon/radeon.h     |  2 +-
>   drivers/gpu/drm/radeon/radeon_cs.c  |  9 +++++--
>   drivers/gpu/drm/radeon/radeon_gem.c | 17 ++++++++++----
>   drivers/gpu/drm/radeon/radeon_kms.c |  5 ++++
>   drivers/gpu/drm/radeon/radeon_vm.c  | 47 +++++++++----------------------------
>   5 files changed, 36 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 3680cf0..699446a 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -925,7 +925,7 @@ struct radeon_vm_id {
>   };
>   
>   struct radeon_vm {
> -	struct mutex		mutex;
> +	struct mutex		lock;
>   
>   	struct rb_root		va;
>   
> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
> index fb776cb..7a90378 100644
> --- a/drivers/gpu/drm/radeon/radeon_cs.c
> +++ b/drivers/gpu/drm/radeon/radeon_cs.c
> @@ -195,6 +195,7 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p)
>   	if (p->cs_flags & RADEON_CS_USE_VM)
>   		p->vm_bos = radeon_vm_get_bos(p->rdev, p->ib.vm,
>   					      &p->validated);
> +
>   	if (need_mmap_lock)
>   		down_read(&current->mm->mmap_sem);
>   
> @@ -571,7 +572,6 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev,
>   	if (parser->ring == R600_RING_TYPE_UVD_INDEX)
>   		radeon_uvd_note_usage(rdev);
>   
> -	mutex_lock(&vm->mutex);
>   	r = radeon_bo_vm_update_pte(parser, vm);
>   	if (r) {
>   		goto out;
> @@ -592,7 +592,6 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev,
>   	}
>   
>   out:
> -	mutex_unlock(&vm->mutex);
>   	return r;
>   }
>   
> @@ -696,6 +695,8 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	}
>   
>   	r = radeon_cs_ib_fill(rdev, &parser);
> +	if (parser.cs_flags & RADEON_CS_USE_VM)
> +		mutex_lock(&parser.ib.vm->lock);
>   	if (!r) {
>   		r = radeon_cs_parser_relocs(&parser);
>   		if (r && r != -ERESTARTSYS)
> @@ -704,6 +705,8 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   
>   	if (r) {
>   		radeon_cs_parser_fini(&parser, r, false);
> +		if (parser.cs_flags & RADEON_CS_USE_VM)
> +			mutex_unlock(&parser.ib.vm->lock);
>   		up_read(&rdev->exclusive_lock);
>   		r = radeon_cs_handle_lockup(rdev, r);
>   		return r;
> @@ -721,6 +724,8 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	}
>   out:
>   	radeon_cs_parser_fini(&parser, r, true);
> +	if (parser.cs_flags & RADEON_CS_USE_VM)
> +		mutex_unlock(&parser.ib.vm->lock);
>   	up_read(&rdev->exclusive_lock);
>   	r = radeon_cs_handle_lockup(rdev, r);
>   	return r;
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
> index 6162bd2..4eafec6 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -150,8 +150,10 @@ int radeon_gem_object_open(struct drm_gem_object *obj, struct drm_file *file_pri
>   		return 0;
>   	}
>   
> +	mutex_lock(&vm->lock);
>   	r = radeon_bo_reserve(rbo, false);
>   	if (r) {
> +		mutex_unlock(&vm->lock);
>   		return r;
>   	}
>   
> @@ -162,6 +164,7 @@ int radeon_gem_object_open(struct drm_gem_object *obj, struct drm_file *file_pri
>   		++bo_va->ref_count;
>   	}
>   	radeon_bo_unreserve(rbo);
> +	mutex_unlock(&vm->lock);
>   
>   	return 0;
>   }
> @@ -180,8 +183,10 @@ void radeon_gem_object_close(struct drm_gem_object *obj,
>   		return;
>   	}
>   
> +	mutex_lock(&vm->lock);
>   	r = radeon_bo_reserve(rbo, true);
>   	if (r) {
> +		mutex_unlock(&vm->lock);
>   		dev_err(rdev->dev, "leaking bo va because "
>   			"we fail to reserve bo (%d)\n", r);
>   		return;
> @@ -193,6 +198,7 @@ void radeon_gem_object_close(struct drm_gem_object *obj,
>   		}
>   	}
>   	radeon_bo_unreserve(rbo);
> +	mutex_unlock(&vm->lock);
>   }
>   
>   static int radeon_gem_handle_lockup(struct radeon_device *rdev, int r)
> @@ -576,17 +582,13 @@ static void radeon_gem_va_update_vm(struct radeon_device *rdev,
>   			goto error_unreserve;
>   	}
>   
> -	mutex_lock(&bo_va->vm->mutex);
>   	r = radeon_vm_clear_freed(rdev, bo_va->vm);
>   	if (r)
> -		goto error_unlock;
> +		goto error_unreserve;
>   
>   	if (bo_va->it.start)
>   		r = radeon_vm_bo_update(rdev, bo_va, &bo_va->bo->tbo.mem);
>   
> -error_unlock:
> -	mutex_unlock(&bo_va->vm->mutex);
> -
>   error_unreserve:
>   	ttm_eu_backoff_reservation(&ticket, &list);
>   
> @@ -662,14 +664,18 @@ int radeon_gem_va_ioctl(struct drm_device *dev, void *data,
>   		return -ENOENT;
>   	}
>   	rbo = gem_to_radeon_bo(gobj);
> +	mutex_lock(&fpriv->vm.lock);
>   	r = radeon_bo_reserve(rbo, false);
>   	if (r) {
> +		mutex_unlock(&fpriv->vm.lock);
>   		args->operation = RADEON_VA_RESULT_ERROR;
>   		drm_gem_object_unreference_unlocked(gobj);
>   		return r;
>   	}
>   	bo_va = radeon_vm_bo_find(&fpriv->vm, rbo);
>   	if (!bo_va) {
> +		radeon_bo_unreserve(rbo);
> +		mutex_unlock(&fpriv->vm.lock);
>   		args->operation = RADEON_VA_RESULT_ERROR;
>   		drm_gem_object_unreference_unlocked(gobj);
>   		return -ENOENT;
> @@ -698,6 +704,7 @@ int radeon_gem_va_ioctl(struct drm_device *dev, void *data,
>   		args->operation = RADEON_VA_RESULT_ERROR;
>   	}
>   out:
> +	mutex_unlock(&fpriv->vm.lock);
>   	drm_gem_object_unreference_unlocked(gobj);
>   	return r;
>   }
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index f4dd26a..d994006 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -613,8 +613,10 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   		}
>   
>   		if (rdev->accel_working) {
> +			mutex_lock(&vm->lock);
>   			r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false);
>   			if (r) {
> +				mutex_unlock(&vm->lock);
>   				radeon_vm_fini(rdev, vm);
>   				kfree(fpriv);
>   				return r;
> @@ -628,6 +630,7 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   						  RADEON_VA_IB_OFFSET,
>   						  RADEON_VM_PAGE_READABLE |
>   						  RADEON_VM_PAGE_SNOOPED);
> +			mutex_unlock(&vm->lock);
>   			if (r) {
>   				radeon_vm_fini(rdev, vm);
>   				kfree(fpriv);
> @@ -662,12 +665,14 @@ void radeon_driver_postclose_kms(struct drm_device *dev,
>   		int r;
>   
>   		if (rdev->accel_working) {
> +			mutex_lock(&vm->lock);
>   			r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false);
>   			if (!r) {
>   				if (vm->ib_bo_va)
>   					radeon_vm_bo_rmv(rdev, vm->ib_bo_va);
>   				radeon_bo_unreserve(rdev->ring_tmp_bo.bo);
>   			}
> +			mutex_unlock(&vm->lock);
>   		}
>   
>   		radeon_vm_fini(rdev, vm);
> diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c
> index cde48c4..658183f 100644
> --- a/drivers/gpu/drm/radeon/radeon_vm.c
> +++ b/drivers/gpu/drm/radeon/radeon_vm.c
> @@ -172,7 +172,7 @@ struct radeon_bo_list *radeon_vm_get_bos(struct radeon_device *rdev,
>    * Allocate an id for the vm (cayman+).
>    * Returns the fence we need to sync to (if any).
>    *
> - * Global and local mutex must be locked!
> + * Mutex must be locked!
>    */
>   struct radeon_fence *radeon_vm_grab_id(struct radeon_device *rdev,
>   				       struct radeon_vm *vm, int ring)
> @@ -231,7 +231,7 @@ struct radeon_fence *radeon_vm_grab_id(struct radeon_device *rdev,
>    *
>    * Flush the vm (cayman+).
>    *
> - * Global and local mutex must be locked!
> + * Mutex must be locked!
>    */
>   void radeon_vm_flush(struct radeon_device *rdev,
>   		     struct radeon_vm *vm,
> @@ -263,7 +263,7 @@ void radeon_vm_flush(struct radeon_device *rdev,
>    * Fence the vm (cayman+).
>    * Set the fence used to protect page table and id.
>    *
> - * Global and local mutex must be locked!
> + * Mutex must be locked!
>    */
>   void radeon_vm_fence(struct radeon_device *rdev,
>   		     struct radeon_vm *vm,
> @@ -314,7 +314,7 @@ struct radeon_bo_va *radeon_vm_bo_find(struct radeon_vm *vm,
>    * Add @bo to the list of bos associated with the vm
>    * Returns newly added bo_va or NULL for failure
>    *
> - * Object has to be reserved!
> + * Mutex must be locked and object has to be reserved!
>    */
>   struct radeon_bo_va *radeon_vm_bo_add(struct radeon_device *rdev,
>   				      struct radeon_vm *vm,
> @@ -336,9 +336,7 @@ struct radeon_bo_va *radeon_vm_bo_add(struct radeon_device *rdev,
>   	INIT_LIST_HEAD(&bo_va->bo_list);
>   	INIT_LIST_HEAD(&bo_va->vm_status);
>   
> -	mutex_lock(&vm->mutex);
>   	list_add_tail(&bo_va->bo_list, &bo->va);
> -	mutex_unlock(&vm->mutex);
>   
>   	return bo_va;
>   }
> @@ -441,7 +439,8 @@ error_unreserve:
>    * Validate and set the offset requested within the vm address space.
>    * Returns 0 for success, error for failure.
>    *
> - * Object has to be reserved and gets unreserved by this function!
> + * Mutex must be locked and object has to be reserved and gets
> + * unreserved by this function!
>    */
>   int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>   			  struct radeon_bo_va *bo_va,
> @@ -472,14 +471,12 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>   		eoffset = last_pfn = 0;
>   	}
>   
> -	mutex_lock(&vm->mutex);
>   	if (bo_va->it.start || bo_va->it.last) {
>   		if (bo_va->addr) {
>   			/* add a clone of the bo_va to clear the old address */
>   			struct radeon_bo_va *tmp;
>   			tmp = kzalloc(sizeof(struct radeon_bo_va), GFP_KERNEL);
>   			if (!tmp) {
> -				mutex_unlock(&vm->mutex);
>   				return -ENOMEM;
>   			}
>   			tmp->it.start = bo_va->it.start;
> @@ -509,7 +506,6 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>   			dev_err(rdev->dev, "bo %p va 0x%010Lx conflict with "
>   				"(bo %p 0x%010lx 0x%010lx)\n", bo_va->bo,
>   				soffset, tmp->bo, tmp->it.start, tmp->it.last);
> -			mutex_unlock(&vm->mutex);
>   			return -EINVAL;
>   		}
>   		bo_va->it.start = soffset;
> @@ -537,9 +533,6 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>   		if (vm->page_tables[pt_idx].bo)
>   			continue;
>   
> -		/* drop mutex to allocate and clear page table */
> -		mutex_unlock(&vm->mutex);
> -
>   		r = radeon_bo_create(rdev, RADEON_VM_PTE_COUNT * 8,
>   				     RADEON_GPU_PAGE_SIZE, true,
>   				     RADEON_GEM_DOMAIN_VRAM, 0,
> @@ -554,21 +547,10 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>   			return r;
>   		}
>   
> -		/* aquire mutex again */
> -		mutex_lock(&vm->mutex);
> -		if (vm->page_tables[pt_idx].bo) {
> -			/* someone else allocated the pt in the meantime */
> -			mutex_unlock(&vm->mutex);
> -			radeon_bo_unref(&pt);
> -			mutex_lock(&vm->mutex);
> -			continue;
> -		}
> -
>   		vm->page_tables[pt_idx].addr = 0;
>   		vm->page_tables[pt_idx].bo = pt;
>   	}
>   
> -	mutex_unlock(&vm->mutex);
>   	return 0;
>   }
>   
> @@ -627,7 +609,7 @@ static uint32_t radeon_vm_page_flags(uint32_t flags)
>    * and updates the page directory (cayman+).
>    * Returns 0 for success, error for failure.
>    *
> - * Global and local mutex must be locked!
> + * Mutex must be locked!
>    */
>   int radeon_vm_update_page_directory(struct radeon_device *rdev,
>   				    struct radeon_vm *vm)
> @@ -717,8 +699,6 @@ int radeon_vm_update_page_directory(struct radeon_device *rdev,
>    * @pe_end: last PTE to handle
>    * @addr: addr those PTEs should point to
>    * @flags: hw mapping flags
> - *
> - * Global and local mutex must be locked!
>    */
>   static void radeon_vm_frag_ptes(struct radeon_device *rdev,
>   				struct radeon_ib *ib,
> @@ -798,7 +778,6 @@ static void radeon_vm_frag_ptes(struct radeon_device *rdev,
>    *
>    * Update the page tables in the range @start - @end (cayman+).
>    *
> - * Global and local mutex must be locked!
>    */
>   static int radeon_vm_update_ptes(struct radeon_device *rdev,
>   				 struct radeon_vm *vm,
> @@ -870,7 +849,6 @@ static int radeon_vm_update_ptes(struct radeon_device *rdev,
>    *
>    * Fence the page tables in the range @start - @end (cayman+).
>    *
> - * Global and local mutex must be locked!
>    */
>   static void radeon_vm_fence_pts(struct radeon_vm *vm,
>   				uint64_t start, uint64_t end,
> @@ -896,7 +874,7 @@ static void radeon_vm_fence_pts(struct radeon_vm *vm,
>    * Fill in the page table entries for @bo (cayman+).
>    * Returns 0 for success, -EINVAL for failure.
>    *
> - * Object have to be reserved and mutex must be locked!
> + * Mutex must be locked and BOs have to be reserved!
>    */
>   int radeon_vm_bo_update(struct radeon_device *rdev,
>   			struct radeon_bo_va *bo_va,
> @@ -1097,7 +1075,7 @@ int radeon_vm_clear_invalids(struct radeon_device *rdev,
>    *
>    * Remove @bo_va->bo from the requested vm (cayman+).
>    *
> - * Object have to be reserved!
> + * Mutex must be locked and object has to be reserved!
>    */
>   void radeon_vm_bo_rmv(struct radeon_device *rdev,
>   		      struct radeon_bo_va *bo_va)
> @@ -1106,7 +1084,6 @@ void radeon_vm_bo_rmv(struct radeon_device *rdev,
>   
>   	list_del(&bo_va->bo_list);
>   
> -	mutex_lock(&vm->mutex);
>   	interval_tree_remove(&bo_va->it, &vm->va);
>   	spin_lock(&vm->status_lock);
>   	list_del(&bo_va->vm_status);
> @@ -1119,8 +1096,6 @@ void radeon_vm_bo_rmv(struct radeon_device *rdev,
>   		kfree(bo_va);
>   	}
>   	spin_unlock(&vm->status_lock);
> -
> -	mutex_unlock(&vm->mutex);
>   }
>   
>   /**
> @@ -1168,7 +1143,7 @@ int radeon_vm_init(struct radeon_device *rdev, struct radeon_vm *vm)
>   		vm->ids[i].flushed_updates = NULL;
>   		vm->ids[i].last_id_use = NULL;
>   	}
> -	mutex_init(&vm->mutex);
> +	mutex_init(&vm->lock);
>   	vm->va = RB_ROOT;
>   	spin_lock_init(&vm->status_lock);
>   	INIT_LIST_HEAD(&vm->invalidated);
> @@ -1245,5 +1220,5 @@ void radeon_vm_fini(struct radeon_device *rdev, struct radeon_vm *vm)
>   		radeon_fence_unref(&vm->ids[i].last_id_use);
>   	}
>   
> -	mutex_destroy(&vm->mutex);
> +	mutex_destroy(&vm->lock);
>   }

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