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.st...@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(¤t->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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
