[1/6] drm/amdgpu: move taking mmap_sem into get_user_pages

Submitted by Christian König on Sept. 5, 2017, 3:37 p.m.

Details

Message ID 1504625871-1756-1-git-send-email-deathsimple@vodafone.de
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Christian König Sept. 5, 2017, 3:37 p.m.
From: Christian König <christian.koenig@amd.com>

This didn't helped as intended, just simplify the code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 12 +-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  4 ----
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  4 ++++
 3 files changed, 5 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 4ab35db..e6e04d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -497,18 +497,14 @@  static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
 	struct amdgpu_bo_list_entry *e;
 	struct list_head duplicates;
-	bool need_mmap_lock = false;
 	unsigned i, tries = 10;
 	int r;
 
 	INIT_LIST_HEAD(&p->validated);
 
 	p->bo_list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle);
-	if (p->bo_list) {
-		need_mmap_lock = p->bo_list->first_userptr !=
-			p->bo_list->num_entries;
+	if (p->bo_list)
 		amdgpu_bo_list_get_list(p->bo_list, &p->validated);
-	}
 
 	INIT_LIST_HEAD(&duplicates);
 	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
@@ -516,9 +512,6 @@  static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	if (p->uf_entry.robj)
 		list_add(&p->uf_entry.tv.head, &p->validated);
 
-	if (need_mmap_lock)
-		down_read(&current->mm->mmap_sem);
-
 	while (1) {
 		struct list_head need_pages;
 		unsigned i;
@@ -670,9 +663,6 @@  static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 
 error_free_pages:
 
-	if (need_mmap_lock)
-		up_read(&current->mm->mmap_sem);
-
 	if (p->bo_list) {
 		for (i = p->bo_list->first_userptr;
 		     i < p->bo_list->num_entries; ++i) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index f1e61b3..b0d45c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -318,8 +318,6 @@  int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
-		down_read(&current->mm->mmap_sem);
-
 		r = amdgpu_ttm_tt_get_user_pages(bo->tbo.ttm,
 						 bo->tbo.ttm->pages);
 		if (r)
@@ -334,8 +332,6 @@  int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 		amdgpu_bo_unreserve(bo);
 		if (r)
 			goto free_pages;
-
-		up_read(&current->mm->mmap_sem);
 	}
 
 	r = drm_gem_handle_create(filp, gobj, &handle);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 07c3f11..d2ce1a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -622,6 +622,8 @@  int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
 	if (!(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY))
 		flags |= FOLL_WRITE;
 
+	down_read(&current->mm->mmap_sem);
+
 	if (gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) {
 		/* check that we only use anonymous memory
 		   to prevent problems with writeback */
@@ -657,6 +659,8 @@  int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
 
 	} while (pinned < ttm->num_pages);
 
+	up_read(&current->mm->mmap_sem);
+
 	return 0;
 
 release_pages:

Comments

I made one comment on patch 5. Other than that, this series is
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

This is the second week I'll get very little productive work done,
because I'm busy debugging memory manager changes and patching KFD to
keep it working. This series will require some changes to userptr
handling in KFD. Once KFD upstreaming is done, you'll have to change
amdgpu_amdkfd_gpuvm code when you're messing around with the memory
manager code. Feels like you're trying to get in all your changes as
long as that's still my problem. :P

The more you keep changing the memory manager, the longer this whole
upstreaming exercise will take me. My biggest motivation to finish
upstreaming is, so that I can stop adapting your memory manager patches.
This is not helping! ;)

Cheers,
  Felix


On 2017-09-05 11:37 AM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> This didn't helped as intended, just simplify the code.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 12 +-----------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  4 ----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  4 ++++
>  3 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 4ab35db..e6e04d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -497,18 +497,14 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
>  	struct amdgpu_bo_list_entry *e;
>  	struct list_head duplicates;
> -	bool need_mmap_lock = false;
>  	unsigned i, tries = 10;
>  	int r;
>  
>  	INIT_LIST_HEAD(&p->validated);
>  
>  	p->bo_list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle);
> -	if (p->bo_list) {
> -		need_mmap_lock = p->bo_list->first_userptr !=
> -			p->bo_list->num_entries;
> +	if (p->bo_list)
>  		amdgpu_bo_list_get_list(p->bo_list, &p->validated);
> -	}
>  
>  	INIT_LIST_HEAD(&duplicates);
>  	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
> @@ -516,9 +512,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  	if (p->uf_entry.robj)
>  		list_add(&p->uf_entry.tv.head, &p->validated);
>  
> -	if (need_mmap_lock)
> -		down_read(&current->mm->mmap_sem);
> -
>  	while (1) {
>  		struct list_head need_pages;
>  		unsigned i;
> @@ -670,9 +663,6 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  
>  error_free_pages:
>  
> -	if (need_mmap_lock)
> -		up_read(&current->mm->mmap_sem);
> -
>  	if (p->bo_list) {
>  		for (i = p->bo_list->first_userptr;
>  		     i < p->bo_list->num_entries; ++i) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index f1e61b3..b0d45c8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -318,8 +318,6 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
> -		down_read(&current->mm->mmap_sem);
> -
>  		r = amdgpu_ttm_tt_get_user_pages(bo->tbo.ttm,
>  						 bo->tbo.ttm->pages);
>  		if (r)
> @@ -334,8 +332,6 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>  		amdgpu_bo_unreserve(bo);
>  		if (r)
>  			goto free_pages;
> -
> -		up_read(&current->mm->mmap_sem);
>  	}
>  
>  	r = drm_gem_handle_create(filp, gobj, &handle);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 07c3f11..d2ce1a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -622,6 +622,8 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>  	if (!(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY))
>  		flags |= FOLL_WRITE;
>  
> +	down_read(&current->mm->mmap_sem);
> +
>  	if (gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) {
>  		/* check that we only use anonymous memory
>  		   to prevent problems with writeback */
> @@ -657,6 +659,8 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>  
>  	} while (pinned < ttm->num_pages);
>  
> +	up_read(&current->mm->mmap_sem);
> +
>  	return 0;
>  
>  release_pages:
I found another problem as I was looking at adapting KFD's userptr code.


On 2017-09-05 11:37 AM, Christian König wrote:
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -622,6 +622,8 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>  	if (!(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY))
>  		flags |= FOLL_WRITE;
>  
> +	down_read(&current->mm->mmap_sem);
> +
>  	if (gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) {
>  		/* check that we only use anonymous memory
>  		   to prevent problems with writeback */
> @@ -657,6 +659,8 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>  
>  	} while (pinned < ttm->num_pages);
>  
> +	up_read(&current->mm->mmap_sem);
> +
>  	return 0;
>  
>  release_pages:

You need another up_read in the release_pages error handing path.

Regards,
  Felix