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@xxxxxxxxxxxxxx>
> ---
> 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.
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
