[PATCH 06/27] drm/etnaviv: get rid of userptr worker

[Philipp Zabel]

On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> All code paths which populate userptr BOs are fine with the get_pages
> function taking the mmap_sem lock. This allows to get rid of the pretty
> involved architecture with a worker being scheduled if the mmap_sem
> needs to be taken, but instead call GUP directly and allow it to take
> the lock if necessary.
> 
> This simplifies the code a lot and removes the possibility of this
> function returning -EAGAIN, which complicates object population
> handling at the callers.
> 
> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 146 +++++-----------------------------
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h |   3 +-
>  2 files changed, 23 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index a52220eeee45..fcc969fa0e69 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -705,141 +705,41 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>  	return 0;
>  }
>  
> -struct get_pages_work {
> -	struct work_struct work;
> -	struct mm_struct *mm;
> -	struct task_struct *task;
> -	struct etnaviv_gem_object *etnaviv_obj;
> -};
> -
> -static struct page **etnaviv_gem_userptr_do_get_pages(
> -	struct etnaviv_gem_object *etnaviv_obj, struct mm_struct *mm, struct task_struct *task)
> -{
> -	int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> -	struct page **pvec;
> -	uintptr_t ptr;
> -	unsigned int flags = 0;
> -
> -	pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> -	if (!pvec)
> -		return ERR_PTR(-ENOMEM);
> -
> -	if (!etnaviv_obj->userptr.ro)
> -		flags |= FOLL_WRITE;
> -
> -	pinned = 0;
> -	ptr = etnaviv_obj->userptr.ptr;
> -
> -	down_read(&mm->mmap_sem);
> -	while (pinned < npages) {
> -		ret = get_user_pages_remote(task, mm, ptr, npages - pinned,
> -					    flags, pvec + pinned, NULL, NULL);
> -		if (ret < 0)
> -			break;
> -
> -		ptr += ret * PAGE_SIZE;
> -		pinned += ret;
> -	}
> -	up_read(&mm->mmap_sem);
> -
> -	if (ret < 0) {
> -		release_pages(pvec, pinned);
> -		kvfree(pvec);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return pvec;
> -}
> -
> -static void __etnaviv_gem_userptr_get_pages(struct work_struct *_work)
> -{
> -	struct get_pages_work *work = container_of(_work, typeof(*work), work);
> -	struct etnaviv_gem_object *etnaviv_obj = work->etnaviv_obj;
> -	struct page **pvec;
> -
> -	pvec = etnaviv_gem_userptr_do_get_pages(etnaviv_obj, work->mm, work->task);
> -
> -	mutex_lock(&etnaviv_obj->lock);
> -	if (IS_ERR(pvec)) {
> -		etnaviv_obj->userptr.work = ERR_CAST(pvec);
> -	} else {
> -		etnaviv_obj->userptr.work = NULL;
> -		etnaviv_obj->pages = pvec;
> -	}
> -
> -	mutex_unlock(&etnaviv_obj->lock);
> -	drm_gem_object_put_unlocked(&etnaviv_obj->base);
> -
> -	mmput(work->mm);
> -	put_task_struct(work->task);
> -	kfree(work);
> -}
> -
>  static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
>  {
>  	struct page **pvec = NULL;
> -	struct get_pages_work *work;
> -	struct mm_struct *mm;
> -	int ret, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> +	struct etnaviv_gem_userptr *userptr = &etnaviv_obj->userptr;
> +	int ret, pinned = 0, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
>  
>  	might_lock_read(&current->mm->mmap_sem);
>  
> -	if (etnaviv_obj->userptr.work) {
> -		if (IS_ERR(etnaviv_obj->userptr.work)) {
> -			ret = PTR_ERR(etnaviv_obj->userptr.work);
> -			etnaviv_obj->userptr.work = NULL;
> -		} else {
> -			ret = -EAGAIN;
> -		}
> -		return ret;
> -	}
> +	if (userptr->mm != current->mm)
> +		return -EPERM;

I don't pretend to understand the implications of this patch completely.
It looks mostly good to me, but this part seems to limit previously
allowed behaviour. I think this warrants a mention in the commit
message.

regards
Philipp