[PATCH 3/4] drm/exynos: added userptr feature.

Inki Dae inki.dae at samsung.com
Tue May 15 01:17:34 PDT 2012


Hi Rob,

> -----Original Message-----
> From: robdclark at gmail.com [mailto:robdclark at gmail.com] On Behalf Of Rob
> Clark
> Sent: Tuesday, May 15, 2012 4:35 PM
> To: Inki Dae
> Cc: airlied at linux.ie; dri-devel at lists.freedesktop.org;
> kyungmin.park at samsung.com; sw0312.kim at samsung.com
> Subject: Re: [PATCH 3/4] drm/exynos: added userptr feature.
> 
> On Mon, Apr 23, 2012 at 7:43 AM, Inki Dae <inki.dae at samsung.com> wrote:
> > this feature could be used to use memory region allocated by malloc() in
> user
> > mode and mmaped memory region allocated by other memory allocators.
> userptr
> > interface can identify memory type through vm_flags value and would get
> > pages or page frame numbers to user space appropriately.
> 
> I apologize for being a little late to jump in on this thread, but...
> 
> I must confess to not being a huge fan of userptr.  It really is
> opening a can of worms, and seems best avoided if at all possible.
> I'm not entirely sure the use-case for which you require this, but I
> wonder if there isn't an alternative way?   I mean, the main case I
> could think of for mapping userspace memory would be something like
> texture upload.  But could that be handled in an alternative way,
> something like a pwrite or texture_upload API, which could temporarily
> pin the userspace memory, kick off a dma from that user buffer to a
> proper GEM buffer, and then unpin the user buffer when the DMA
> completes?
> 

This feature have being discussed with drm and linux-mm guys and I have
posted this patch four times.
So please, see below e-mail threads.
http://www.spinics.net/lists/dri-devel/msg22729.html

and then please, give me your opinions.

Thanks,
Inki Dae

> BR,
> -R
> 
> > Signed-off-by: Inki Dae <inki.dae at samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c |    2 +
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |  258
> +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |   13 ++-
> >  include/drm/exynos_drm.h                |   25 +++-
> >  4 files changed, 296 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index f58a487..5bb0361 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -211,6 +211,8 @@ static struct drm_ioctl_desc exynos_ioctls[] = {
> >                        DRM_AUTH),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MMAP,
> >                        exynos_drm_gem_mmap_ioctl, DRM_UNLOCKED |
DRM_AUTH),
> > +       DRM_IOCTL_DEF_DRV(EXYNOS_GEM_USERPTR,
> > +                       exynos_drm_gem_userptr_ioctl, DRM_UNLOCKED),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_PLANE_SET_ZPOS,
> exynos_plane_set_zpos_ioctl,
> >                        DRM_UNLOCKED | DRM_AUTH),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_VIDI_CONNECTION,
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > index afd0cd4..b68d4ea 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > @@ -66,6 +66,43 @@ 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);
> > +}
> > +
> >  static void update_vm_cache_attr(struct exynos_drm_gem_obj *obj,
> >                                        struct vm_area_struct *vma)
> >  {
> > @@ -254,6 +291,41 @@ 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;
> > +
> > +       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)
> > @@ -293,6 +365,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 +680,190 @@ 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;
> > +
> > +       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;
> > +
> > +               start = userptr;
> > +               offset = userptr & ~PAGE_MASK;
> > +               end = start + buf->size;
> > +               sgl = buf->sgt->sgl;
> > +
> > +               for (prev_pfn = 0; start < end; start += PAGE_SIZE) {
> > +                       int ret = follow_pfn(vma, start, &this_pfn);
> > +                       if (ret)
> > +                               return ret;
> > +
> > +                       if (prev_pfn == 0)
> > +                               pa = this_pfn << PAGE_SHIFT;
> > +                       else if (this_pfn != prev_pfn + 1)
> > +                               return -EFAULT;
> > +
> > +                       sg_dma_address(sgl) = (pa + offset);
> > +                       sg_dma_len(sgl) = PAGE_SIZE;
> > +                       prev_pfn = this_pfn;
> > +                       pa += PAGE_SIZE;
> > +                       npages++;
> > +                       sgl = sg_next(sgl);
> > +               }
> > +
> > +               buf->dma_addr = pa + offset;
> > +
> > +               obj->vma = get_vma(vma);
> > +               if (!obj->vma)
> > +                       return -ENOMEM;
> > +
> > +               buf->pfnmap = true;
> > +
> > +               return npages;
> > +       }
> > +
> > +       buf->write = write;
> > +       npages = buf->size >> PAGE_SHIFT;
> > +
> > +       down_read(&current->mm->mmap_sem);
> > +       get_npages = get_user_pages(current, current->mm, userptr,
> > +                                       npages, write, 1, buf->pages,
NULL);
> > +       up_read(&current->mm->mmap_sem);
> > +       if (get_npages != npages)
> > +               DRM_ERROR("failed to get user_pages.\n");
> > +
> > +       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_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);
> > +       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_init_object(struct drm_gem_object *obj)
> >  {
> >        DRM_DEBUG_KMS("%s\n", __FILE__);
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > index efc8252..1de2241 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > @@ -29,7 +29,8 @@
> >  #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.
> > @@ -38,18 +39,23 @@
> >  * @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;
> >        dma_addr_t              dma_addr;
> > +       unsigned int            write;
> >        struct sg_table         *sgt;
> >        struct page             **pages;
> >        unsigned long           page_size;
> >        unsigned long           size;
> > +       bool                    pfnmap;
> >  };
> >
> >  /*
> > @@ -73,6 +79,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;
> >  };
> >
> > @@ -127,6 +134,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);
> > +
> >  /* initialize gem object. */
> >  int exynos_drm_gem_init_object(struct drm_gem_object *obj);
> >
> > diff --git a/include/drm/exynos_drm.h b/include/drm/exynos_drm.h
> > index 2d6eb06..48eda6e 100644
> > --- a/include/drm/exynos_drm.h
> > +++ b/include/drm/exynos_drm.h
> > @@ -75,6 +75,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 for user connection request of virtual display.
> >  *
> >  * @connection: indicate whether doing connetion or not by user.
> > @@ -105,13 +122,16 @@ 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
> >  };
> >
> >  #define DRM_EXYNOS_GEM_CREATE          0x00
> >  #define DRM_EXYNOS_GEM_MAP_OFFSET      0x01
> >  #define DRM_EXYNOS_GEM_MMAP            0x02
> > +#define DRM_EXYNOS_GEM_USERPTR         0x03
> >  /* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
> >  #define DRM_EXYNOS_PLANE_SET_ZPOS      0x06
> >  #define DRM_EXYNOS_VIDI_CONNECTION     0x07
> > @@ -125,6 +145,9 @@ enum e_drm_exynos_gem_mem_type {
> >  #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_PLANE_SET_ZPOS      
 DRM_IOWR(DRM_COMMAND_BASE
> + \
> >                DRM_EXYNOS_PLANE_SET_ZPOS, struct
drm_exynos_plane_set_zpos)
> >
> > --
> > 1.7.4.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel



More information about the dri-devel mailing list