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

[Russell King - ARM Linux]

On Fri, Dec 01, 2017 at 05:38:51PM +0100, Philipp Zabel wrote:
> 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.

The point of the complexity is to be able to grab the mmap_sem avoiding
any locking dependencies that would normally occur if it were done in
the ioctl.

However, drm locking has changed quite a bit over the last year, and
this is probably not be needed anymore - I assume that Lucas has
thoroughly verified the locking dependencies.  However, I notice i915
still retains this code though.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up