Hi Jerome, > -----Original Message----- > From: Jerome Glisse [mailto:j.glisse@xxxxxxxxx] > Sent: Wednesday, May 09, 2012 11:46 PM > To: Inki Dae > Cc: airlied@xxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > kyungmin.park@xxxxxxxxxxx; sw0312.kim@xxxxxxxxxxx; linux-mm@xxxxxxxxx > Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature. > > On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: > > this feature is used to import user space region allocated by malloc() > or > > mmaped into a gem. and to guarantee the pages to user space not to be > > swapped out, the VMAs within the user space would be locked and then > unlocked > > when the pages are released. > > > > but this lock might result in significant degradation of system > performance > > because the pages couldn't be swapped out so we limit user-desired > userptr > > size to pre-defined. > > > > Signed-off-by: Inki Dae <inki.dae@xxxxxxxxxxx> > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > > > Again i would like feedback from mm people (adding cc). I am not sure Thank you, I missed adding mm as cc. > locking the vma is the right anwser as i said in my previous mail, > userspace can munlock it in your back, maybe VM_RESERVED is better. I know that with VM_RESERVED flag, also we can avoid the pages from being swapped out. but these pages should be unlocked anytime we want because we could allocate all pages on system and lock them, which in turn, it may result in significant deterioration of system performance.(maybe other processes requesting free memory would be blocked) so I used VM_LOCKED flags instead. but I'm not sure this way is best also. > Anyway even not considering that you don't check at all that process > don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK Thank you for your advices. > for how it's done. Also you mlock complete vma but the userptr you get > might be inside say 16M vma and you only care about 1M of userptr, if > you mark the whole vma as locked than anytime a new page is fault in > the vma else where than in the buffer you are interested then it got > allocated for ever until the gem buffer is destroy, i am not sure of > what happen to the vma on next malloc if it grows or not (i would > think it won't grow at it would have different flags than new > anonymous memory). > > The whole business of directly using malloced memory for gpu is fishy > and i would really like to get it right rather than relying on never > hitting strange things like page migration, vma merging, or worse > things like over locking pages and stealing memory. > Your comments are very helpful to me and I will consider some cases I missed and you pointed out for next patch. Thanks, Inki Dae > Cheers, > Jerome > > > > --- > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 + > > drivers/gpu/drm/exynos/exynos_drm_gem.c | 334 > +++++++++++++++++++++++++++++++ > > drivers/gpu/drm/exynos/exynos_drm_gem.h | 17 ++- > > include/drm/exynos_drm.h | 26 +++- > > 4 files changed, 376 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c > b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > index 1e68ec2..e8ae3f1 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > @@ -220,6 +220,8 @@ static struct drm_ioctl_desc exynos_ioctls[] = { > > DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MAP_OFFSET, > > exynos_drm_gem_map_offset_ioctl, DRM_UNLOCKED | > > DRM_AUTH), > > + DRM_IOCTL_DEF_DRV(EXYNOS_GEM_USERPTR, > > + exynos_drm_gem_userptr_ioctl, DRM_UNLOCKED), > > DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MMAP, > > exynos_drm_gem_mmap_ioctl, DRM_UNLOCKED | DRM_AUTH), > > DRM_IOCTL_DEF_DRV(EXYNOS_GEM_GET, > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c > b/drivers/gpu/drm/exynos/exynos_drm_gem.c > > index e6abb66..ccc6e3d 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > > @@ -68,6 +68,83 @@ static int check_gem_flags(unsigned int flags) > > return 0; > > } > > > > +static struct vm_area_struct *get_vma(struct vm_area_struct *vma) > > +{ > > + struct vm_area_struct *vma_copy; > > + > > + vma_copy = kmalloc(sizeof(*vma_copy), GFP_KERNEL); > > + if (!vma_copy) > > + return NULL; > > + > > + if (vma->vm_ops && vma->vm_ops->open) > > + vma->vm_ops->open(vma); > > + > > + if (vma->vm_file) > > + get_file(vma->vm_file); > > + > > + memcpy(vma_copy, vma, sizeof(*vma)); > > + > > + vma_copy->vm_mm = NULL; > > + vma_copy->vm_next = NULL; > > + vma_copy->vm_prev = NULL; > > + > > + return vma_copy; > > +} > > + > > + > > +static void put_vma(struct vm_area_struct *vma) > > +{ > > + if (!vma) > > + return; > > + > > + if (vma->vm_ops && vma->vm_ops->close) > > + vma->vm_ops->close(vma); > > + > > + if (vma->vm_file) > > + fput(vma->vm_file); > > + > > + kfree(vma); > > +} > > + > > +/* > > + * lock_userptr_vma - lock VMAs within user address space > > + * > > + * this function locks vma within user address space to avoid pages > > + * to the userspace from being swapped out. > > + * if this vma isn't locked, the pages to the userspace could be > swapped out > > + * so unprivileged user might access different pages and dma of any > device > > + * could access physical memory region not intended once swap-in. > > + */ > > +static int lock_userptr_vma(struct exynos_drm_gem_buf *buf, unsigned > int lock) > > +{ > > + struct vm_area_struct *vma; > > + unsigned long start, end; > > + > > + start = buf->userptr; > > + end = buf->userptr + buf->size - 1; > > + > > + down_write(¤t->mm->mmap_sem); > > + > > + do { > > + vma = find_vma(current->mm, start); > > + if (!vma) { > > + up_write(¤t->mm->mmap_sem); > > + return -EFAULT; > > + } > > + > > + if (lock) > > + vma->vm_flags |= VM_LOCKED; > > + else > > + vma->vm_flags &= ~VM_LOCKED; > > + > > + start = vma->vm_end + 1; > > + } while (vma->vm_end < end); > > + > > + up_write(¤t->mm->mmap_sem); > > + > > + return 0; > > +} > > + > > static void update_vm_cache_attr(struct exynos_drm_gem_obj *obj, > > struct vm_area_struct *vma) > > { > > @@ -256,6 +333,44 @@ static void exynos_drm_gem_put_pages(struct > drm_gem_object *obj) > > /* add some codes for UNCACHED type here. TODO */ > > } > > > > +static void exynos_drm_put_userptr(struct drm_gem_object *obj) > > +{ > > + struct exynos_drm_gem_obj *exynos_gem_obj; > > + struct exynos_drm_gem_buf *buf; > > + struct vm_area_struct *vma; > > + int npages; > > + > > + exynos_gem_obj = to_exynos_gem_obj(obj); > > + buf = exynos_gem_obj->buffer; > > + vma = exynos_gem_obj->vma; > > + > > + if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) { > > + put_vma(exynos_gem_obj->vma); > > + goto out; > > + } > > + > > + npages = buf->size >> PAGE_SHIFT; > > + > > + if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR && !buf->pfnmap) > > + lock_userptr_vma(buf, 0); > > + > > + npages--; > > + while (npages >= 0) { > > + if (buf->write) > > + set_page_dirty_lock(buf->pages[npages]); > > + > > + put_page(buf->pages[npages]); > > + npages--; > > + } > > + > > +out: > > + kfree(buf->pages); > > + buf->pages = NULL; > > + > > + kfree(buf->sgt); > > + buf->sgt = NULL; > > +} > > + > > static int exynos_drm_gem_handle_create(struct drm_gem_object *obj, > > struct drm_file *file_priv, > > unsigned int *handle) > > @@ -295,6 +410,8 @@ void exynos_drm_gem_destroy(struct > exynos_drm_gem_obj *exynos_gem_obj) > > > > if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG) > > exynos_drm_gem_put_pages(obj); > > + else if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR) > > + exynos_drm_put_userptr(obj); > > else > > exynos_drm_free_buf(obj->dev, exynos_gem_obj->flags, buf); > > > > @@ -606,6 +723,223 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device > *dev, void *data, > > return 0; > > } > > > > +static int exynos_drm_get_userptr(struct drm_device *dev, > > + struct exynos_drm_gem_obj *obj, > > + unsigned long userptr, > > + unsigned int write) > > +{ > > + unsigned int get_npages; > > + unsigned long npages = 0; > > + struct vm_area_struct *vma; > > + struct exynos_drm_gem_buf *buf = obj->buffer; > > + int ret; > > + > > + down_read(¤t->mm->mmap_sem); > > + vma = find_vma(current->mm, userptr); > > + > > + /* the memory region mmaped with VM_PFNMAP. */ > > + if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) { > > + unsigned long this_pfn, prev_pfn, pa; > > + unsigned long start, end, offset; > > + struct scatterlist *sgl; > > + int ret; > > + > > + start = userptr; > > + offset = userptr & ~PAGE_MASK; > > + end = start + buf->size; > > + sgl = buf->sgt->sgl; > > + > > + for (prev_pfn = 0; start < end; start += PAGE_SIZE) { > > + ret = follow_pfn(vma, start, &this_pfn); > > + if (ret) > > + goto err; > > + > > + if (prev_pfn == 0) { > > + pa = this_pfn << PAGE_SHIFT; > > + buf->dma_addr = pa + offset; > > + } else if (this_pfn != prev_pfn + 1) { > > + ret = -EINVAL; > > + goto err; > > + } > > + > > + sg_dma_address(sgl) = (pa + offset); > > + sg_dma_len(sgl) = PAGE_SIZE; > > + prev_pfn = this_pfn; > > + pa += PAGE_SIZE; > > + npages++; > > + sgl = sg_next(sgl); > > + } > > + > > + obj->vma = get_vma(vma); > > + if (!obj->vma) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + > > + up_read(¤t->mm->mmap_sem); > > + buf->pfnmap = true; > > + > > + return npages; > > +err: > > + buf->dma_addr = 0; > > + up_read(¤t->mm->mmap_sem); > > + > > + return ret; > > + } > > + > > + up_read(¤t->mm->mmap_sem); > > + > > + /* > > + * lock the vma within userptr to avoid userspace buffer > > + * from being swapped out. > > + */ > > + ret = lock_userptr_vma(buf, 1); > > + if (ret < 0) { > > + DRM_ERROR("failed to lock vma for userptr.\n"); > > + lock_userptr_vma(buf, 0); > > + return 0; > > + } > > + > > + buf->write = write; > > + npages = buf->size >> PAGE_SHIFT; > > + > > + down_read(¤t->mm->mmap_sem); > > + get_npages = get_user_pages(current, current->mm, userptr, > > + npages, write, 1, buf->pages, NULL); > > + up_read(¤t->mm->mmap_sem); > > + if (get_npages != npages) > > + DRM_ERROR("failed to get user_pages.\n"); > > + > > + buf->userptr = userptr; > > + buf->pfnmap = false; > > + > > + return get_npages; > > +} > > + > > +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data, > > + struct drm_file *file_priv) > > +{ > > + struct exynos_drm_private *priv = dev->dev_private; > > + struct exynos_drm_gem_obj *exynos_gem_obj; > > + struct drm_exynos_gem_userptr *args = data; > > + struct exynos_drm_gem_buf *buf; > > + struct scatterlist *sgl; > > + unsigned long size, userptr; > > + unsigned int npages; > > + int ret, get_npages; > > + > > + DRM_DEBUG_KMS("%s\n", __FILE__); > > + > > + if (!args->size) { > > + DRM_ERROR("invalid size.\n"); > > + return -EINVAL; > > + } > > + > > + ret = check_gem_flags(args->flags); > > + if (ret) > > + return ret; > > + > > + size = roundup_gem_size(args->size, EXYNOS_BO_USERPTR); > > + > > + if (size > priv->userptr_limit) { > > + DRM_ERROR("excessed maximum size of userptr.\n"); > > + return -EINVAL; > > + } > > + > > + userptr = args->userptr; > > + > > + buf = exynos_drm_init_buf(dev, size); > > + if (!buf) > > + return -ENOMEM; > > + > > + exynos_gem_obj = exynos_drm_gem_init(dev, size); > > + if (!exynos_gem_obj) { > > + ret = -ENOMEM; > > + goto err_free_buffer; > > + } > > + > > + buf->sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL); > > + if (!buf->sgt) { > > + DRM_ERROR("failed to allocate buf->sgt.\n"); > > + ret = -ENOMEM; > > + goto err_release_gem; > > + } > > + > > + npages = size >> PAGE_SHIFT; > > + > > + ret = sg_alloc_table(buf->sgt, npages, GFP_KERNEL); > > + if (ret < 0) { > > + DRM_ERROR("failed to initailize sg table.\n"); > > + goto err_free_sgt; > > + } > > + > > + buf->pages = kzalloc(npages * sizeof(struct page *), GFP_KERNEL); > > + if (!buf->pages) { > > + DRM_ERROR("failed to allocate buf->pages\n"); > > + ret = -ENOMEM; > > + goto err_free_table; > > + } > > + > > + exynos_gem_obj->buffer = buf; > > + > > + get_npages = exynos_drm_get_userptr(dev, exynos_gem_obj, userptr, > 1); > > + if (get_npages != npages) { > > + DRM_ERROR("failed to get user_pages.\n"); > > + ret = get_npages; > > + goto err_release_userptr; > > + } > > + > > + ret = exynos_drm_gem_handle_create(&exynos_gem_obj->base, > file_priv, > > + &args->handle); > > + if (ret < 0) { > > + DRM_ERROR("failed to create gem handle.\n"); > > + goto err_release_userptr; > > + } > > + > > + sgl = buf->sgt->sgl; > > + > > + /* > > + * if buf->pfnmap is true then update sgl of sgt with pages but > > + * if buf->pfnmap is false then it means the sgl was updated > already > > + * so it doesn't need to update the sgl. > > + */ > > + if (!buf->pfnmap) { > > + unsigned int i = 0; > > + > > + /* set all pages to sg list. */ > > + while (i < npages) { > > + sg_set_page(sgl, buf->pages[i], PAGE_SIZE, 0); > > + sg_dma_address(sgl) = page_to_phys(buf->pages[i]); > > + i++; > > + sgl = sg_next(sgl); > > + } > > + } > > + > > + /* always use EXYNOS_BO_USERPTR as memory type for userptr. */ > > + exynos_gem_obj->flags |= EXYNOS_BO_USERPTR; > > + > > + return 0; > > + > > +err_release_userptr: > > + get_npages--; > > + while (get_npages >= 0) > > + put_page(buf->pages[get_npages--]); > > + kfree(buf->pages); > > + buf->pages = NULL; > > +err_free_table: > > + sg_free_table(buf->sgt); > > +err_free_sgt: > > + kfree(buf->sgt); > > + buf->sgt = NULL; > > +err_release_gem: > > + drm_gem_object_release(&exynos_gem_obj->base); > > + kfree(exynos_gem_obj); > > + exynos_gem_obj = NULL; > > +err_free_buffer: > > + exynos_drm_free_buf(dev, 0, buf); > > + return ret; > > +} > > + > > int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > { struct exynos_drm_gem_obj *exynos_gem_obj; > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h > b/drivers/gpu/drm/exynos/exynos_drm_gem.h > > index 3334c9f..72bd993 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h > > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h > > @@ -29,27 +29,35 @@ > > #define to_exynos_gem_obj(x) container_of(x,\ > > struct exynos_drm_gem_obj, base) > > > > -#define IS_NONCONTIG_BUFFER(f) (f & EXYNOS_BO_NONCONTIG) > > +#define IS_NONCONTIG_BUFFER(f) ((f & EXYNOS_BO_NONCONTIG) ||\ > > + (f & EXYNOS_BO_USERPTR)) > > > > /* > > * exynos drm gem buffer structure. > > * > > * @kvaddr: kernel virtual address to allocated memory region. > > + * @userptr: user space address. > > * @dma_addr: bus address(accessed by dma) to allocated memory region. > > * - this address could be physical address without IOMMU and > > * device address with IOMMU. > > + * @write: whether pages will be written to by the caller. > > * @sgt: sg table to transfer page data. > > * @pages: contain all pages to allocated memory region. > > * @page_size: could be 4K, 64K or 1MB. > > * @size: size of allocated memory region. > > + * @pfnmap: indicate whether memory region from userptr is mmaped with > > + * VM_PFNMAP or not. > > */ > > struct exynos_drm_gem_buf { > > void __iomem *kvaddr; > > + unsigned long userptr; > > dma_addr_t dma_addr; > > + unsigned int write; > > struct sg_table *sgt; > > struct page **pages; > > unsigned long page_size; > > unsigned long size; > > + bool pfnmap; > > }; > > > > /* > > @@ -64,6 +72,8 @@ struct exynos_drm_gem_buf { > > * continuous memory region allocated by user request > > * or at framebuffer creation. > > * @size: total memory size to physically non-continuous memory region. > > + * @vma: a pointer to the vma to user address space and used to release > > + * the pages to user space. > > * @flags: indicate memory type to allocated buffer and cache attruibute. > > * > > * P.S. this object would be transfered to user as kms_bo.handle so > > @@ -73,6 +83,7 @@ struct exynos_drm_gem_obj { > > struct drm_gem_object base; > > struct exynos_drm_gem_buf *buffer; > > unsigned long size; > > + struct vm_area_struct *vma; > > unsigned int flags; > > }; > > > > @@ -130,6 +141,10 @@ int exynos_drm_gem_map_offset_ioctl(struct > drm_device *dev, void *data, > > int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv); > > > > +/* map user space allocated by malloc to pages. */ > > +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data, > > + struct drm_file *file_priv); > > + > > /* get buffer information to memory region allocated by gem. */ > > int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv); > > diff --git a/include/drm/exynos_drm.h b/include/drm/exynos_drm.h > > index 52465dc..33fa1e2 100644 > > --- a/include/drm/exynos_drm.h > > +++ b/include/drm/exynos_drm.h > > @@ -77,6 +77,23 @@ struct drm_exynos_gem_mmap { > > }; > > > > /** > > + * User-requested user space importing structure > > + * > > + * @userptr: user space address allocated by malloc. > > + * @size: size to the buffer allocated by malloc. > > + * @flags: indicate user-desired cache attribute to map the allocated > buffer > > + * to kernel space. > > + * @handle: a returned handle to created gem object. > > + * - this handle will be set by gem module of kernel side. > > + */ > > +struct drm_exynos_gem_userptr { > > + uint64_t userptr; > > + uint64_t size; > > + unsigned int flags; > > + unsigned int handle; > > +}; > > + > > +/** > > * A structure to gem information. > > * > > * @handle: a handle to gem object created. > > @@ -135,8 +152,10 @@ enum e_drm_exynos_gem_mem_type { > > EXYNOS_BO_CACHABLE = 1 << 1, > > /* write-combine mapping. */ > > EXYNOS_BO_WC = 1 << 2, > > + /* user space memory allocated by malloc. */ > > + EXYNOS_BO_USERPTR = 1 << 3, > > EXYNOS_BO_MASK = EXYNOS_BO_NONCONTIG | EXYNOS_BO_CACHABLE > | > > - EXYNOS_BO_WC > > + EXYNOS_BO_WC | EXYNOS_BO_USERPTR > > }; > > > > struct drm_exynos_g2d_get_ver { > > @@ -173,7 +192,7 @@ struct drm_exynos_g2d_exec { > > #define DRM_EXYNOS_GEM_CREATE 0x00 > > #define DRM_EXYNOS_GEM_MAP_OFFSET 0x01 > > #define DRM_EXYNOS_GEM_MMAP 0x02 > > -/* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */ > > +#define DRM_EXYNOS_GEM_USERPTR 0x03 > > #define DRM_EXYNOS_GEM_GET 0x04 > > #define DRM_EXYNOS_USER_LIMIT 0x05 > > #define DRM_EXYNOS_PLANE_SET_ZPOS 0x06 > > @@ -193,6 +212,9 @@ struct drm_exynos_g2d_exec { > > #define DRM_IOCTL_EXYNOS_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + \ > > DRM_EXYNOS_GEM_MMAP, struct drm_exynos_gem_mmap) > > > > +#define DRM_IOCTL_EXYNOS_GEM_USERPTR DRM_IOWR(DRM_COMMAND_BASE + \ > > + DRM_EXYNOS_GEM_USERPTR, struct drm_exynos_gem_userptr) > > + > > #define DRM_IOCTL_EXYNOS_GEM_GET DRM_IOWR(DRM_COMMAND_BASE + \ > > DRM_EXYNOS_GEM_GET, struct drm_exynos_gem_info) > > > > -- > > 1.7.4.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
