Hello,
After a long pause I'm sending second version of my patch series to abstract vma handling from the various media drivers. After this patch set drivers have to know much less details about vmas, their types, and locking. My motivation for the series is that I want to change get_user_pages() locking and I want to handle subtle locking details in as few places as possible.
The core of the series is the new helper get_vaddr_pfns() which is given a virtual address and it fills in PFNs into provided array. If PFNs correspond to normal pages it also grabs references to these pages. The difference from get_user_pages() is that this function can also deal with pfnmap, mixed, and io mappings which is what the media drivers need.
I have tested the patches with vivid driver so at least vb2 code got some exposure. Conversion of other drivers was just compile-tested so I'd like to ask respective maintainers if they could have a look. Also I'd like to ask mm folks to check patch 2/9 implementing the helper. Thanks!
Honza
Currently vb2 core acquires mmap_sem just around call to __qbuf_userptr(). However since commit f035eb4e976ef5 (videobuf2: fix lockdep warning) it isn't necessary to acquire it so early as we no longer have to drop queue mutex before acquiring mmap_sem. So push acquisition of mmap_sem down into .get_userptr and .put_userptr memops so that the semaphore is acquired for a shorter time and it is clearer what it is needed for.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/v4l2-core/videobuf2-core.c | 2 -- drivers/media/v4l2-core/videobuf2-dma-contig.c | 7 +++++++ drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++++ drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++++- 4 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index bc08a829bc13..e1fbdecb0b00 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -1630,9 +1630,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) ret = __qbuf_mmap(vb, b); break; case V4L2_MEMORY_USERPTR: - down_read(¤t->mm->mmap_sem); ret = __qbuf_userptr(vb, b); - up_read(¤t->mm->mmap_sem); break; case V4L2_MEMORY_DMABUF: ret = __qbuf_dmabuf(vb, b); diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c index b481d20c8372..96eceabb307b 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -526,7 +526,9 @@ static void vb2_dc_put_userptr(void *buf_priv) sg_free_table(sgt); kfree(sgt); } + down_read(¤t->mm->mmap_sem); vb2_put_vma(buf->vma); + up_read(¤t->mm->mmap_sem); kfree(buf); }
@@ -610,6 +612,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, goto fail_buf; }
+ down_read(¤t->mm->mmap_sem); /* current->mm->mmap_sem is taken by videobuf2 core */ vma = find_vma(current->mm, vaddr); if (!vma) { @@ -637,6 +640,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, if (ret) { unsigned long pfn; if (vb2_dc_get_user_pfn(start, n_pages, vma, &pfn) == 0) { + up_read(¤t->mm->mmap_sem); buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, pfn); buf->size = size; kfree(pages); @@ -646,6 +650,7 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, pr_err("failed to get user pages\n"); goto fail_vma; } + up_read(¤t->mm->mmap_sem);
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); if (!sgt) { @@ -708,10 +713,12 @@ fail_get_user_pages: while (n_pages) put_page(pages[--n_pages]);
+ down_read(¤t->mm->mmap_sem); fail_vma: vb2_put_vma(buf->vma);
fail_pages: + up_read(¤t->mm->mmap_sem); kfree(pages); /* kfree is NULL-proof */
fail_buf: diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c index b1838abb6d00..71510e4f7d7c 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c @@ -263,6 +263,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, if (!buf->pages) goto userptr_fail_alloc_pages;
+ down_read(¤t->mm->mmap_sem); vma = find_vma(current->mm, vaddr); if (!vma) { dprintk(1, "no vma for address %lu\n", vaddr); @@ -301,6 +302,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, 1, /* force */ buf->pages, NULL); + up_read(¤t->mm->mmap_sem);
if (num_pages_from_user != buf->num_pages) goto userptr_fail_get_user_pages; @@ -328,8 +330,10 @@ userptr_fail_get_user_pages: if (!vma_is_io(buf->vma)) while (--num_pages_from_user >= 0) put_page(buf->pages[num_pages_from_user]); + down_read(¤t->mm->mmap_sem); vb2_put_vma(buf->vma); userptr_fail_find_vma: + up_read(¤t->mm->mmap_sem); kfree(buf->pages); userptr_fail_alloc_pages: kfree(buf); @@ -362,7 +366,9 @@ static void vb2_dma_sg_put_userptr(void *buf_priv) put_page(buf->pages[i]); } kfree(buf->pages); + down_read(¤t->mm->mmap_sem); vb2_put_vma(buf->vma); + up_read(¤t->mm->mmap_sem); kfree(buf); }
diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c index bcde88572429..c060cf9662fa 100644 --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c @@ -89,7 +89,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr, offset = vaddr & ~PAGE_MASK; buf->size = size;
- + down_read(¤t->mm->mmap_sem); vma = find_vma(current->mm, vaddr); if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) { if (vb2_get_contig_userptr(vaddr, size, &vma, &physp)) @@ -121,6 +121,7 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr, if (!buf->vaddr) goto fail_get_user_pages; } + up_read(¤t->mm->mmap_sem);
buf->vaddr += offset; return buf; @@ -133,6 +134,7 @@ fail_get_user_pages: kfree(buf->pages);
fail_pages_array_alloc: + up_read(¤t->mm->mmap_sem); kfree(buf);
return NULL; @@ -144,6 +146,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv) unsigned long vaddr = (unsigned long)buf->vaddr & PAGE_MASK; unsigned int i;
+ down_read(¤t->mm->mmap_sem); if (buf->pages) { if (vaddr) vm_unmap_ram((void *)vaddr, buf->n_pages); @@ -157,6 +160,7 @@ static void vb2_vmalloc_put_userptr(void *buf_priv) vb2_put_vma(buf->vma); iounmap((__force void __iomem *)buf->vaddr); } + up_read(¤t->mm->mmap_sem); kfree(buf); }
Provide new function get_vaddr_pfns(). This function maps virtual addresses from given start and fills given array with page frame numbers of the corresponding pages. If given start belongs to a normal vma, the function grabs reference to each of the pages to pin them in memory. If start belongs to VM_IO | VM_PFNMAP vma, we don't touch page structures. Caller should make sure pfns aren't reused for anything else while he is using them.
This function is created for various drivers to simplify handling of their buffers.
Signed-off-by: Jan Kara jack@suse.cz --- include/linux/mm.h | 38 +++++++++++ mm/gup.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h index 47a93928b90f..a5045df92454 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1279,6 +1279,44 @@ long get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, int write, int force, struct page **pages); int get_user_pages_fast(unsigned long start, int nr_pages, int write, struct page **pages); + +/* Container for pinned pfns / pages */ +struct pinned_pfns { + unsigned int nr_allocated_pfns; /* Number of pfns we have space for */ + unsigned int nr_pfns; /* Number of pfns stored in pfns array */ + unsigned int got_ref:1; /* Did we pin pfns by getting page ref? */ + unsigned int is_pages:1; /* Does array contain pages or pfns? */ + void *ptrs[0]; /* Array of pinned pfns / pages. + * Use pfns_vector_pages() or + * pfns_vector_pfns() for access */ +}; + +struct pinned_pfns *pfns_vector_create(int nr_pfns); +void pfns_vector_destroy(struct pinned_pfns *pfns); +int get_vaddr_pfns(unsigned long start, int nr_pfns, int write, int force, + struct pinned_pfns *pfns); +void put_vaddr_pfns(struct pinned_pfns *pfns); +int pfns_vector_to_pages(struct pinned_pfns *pfns); + +static inline int pfns_vector_count(struct pinned_pfns *pfns) +{ + return pfns->nr_pfns; +} + +static inline struct page **pfns_vector_pages(struct pinned_pfns *pfns) +{ + if (!pfns->is_pages) + return NULL; + return (struct page **)(pfns->ptrs); +} + +static inline unsigned long *pfns_vector_pfns(struct pinned_pfns *pfns) +{ + if (pfns->is_pages) + return NULL; + return (unsigned long *)(pfns->ptrs); +} + struct kvec; int get_kernel_pages(const struct kvec *iov, int nr_pages, int write, struct page **pages); diff --git a/mm/gup.c b/mm/gup.c index a6e24e246f86..63903913ab04 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -819,6 +819,186 @@ long get_user_pages(struct task_struct *tsk, struct mm_struct *mm, EXPORT_SYMBOL(get_user_pages);
/** + * get_vaddr_pfns() - map virtual addresses to pfns + * @start: starting user address + * @nr_pfns: number of pfns from start to map + * @write: whether pages will be written to by the caller + * @force: whether to force write access even if user mapping is + * readonly. This will result in the page being COWed even + * in MAP_SHARED mappings. You do not want this. + * @pfns: structure which receives pfns of the pages mapped. + * It should have space for at least nr_pfns pfns. + * + * This function maps virtual addresses from @start and fills @pfns structure + * with page frame numbers of corresponding pages. If @start belongs to a + * normal vma, the function grabs reference to each of the pages to pin them in + * memory. If @start belongs to VM_IO | VM_PFNMAP vma, we don't touch page + * structures. Caller should make sure pfns aren't reused for anything else + * while he is using them. + * + * This function takes care of grabbing mmap_sem as necessary. + */ +int get_vaddr_pfns(unsigned long start, int nr_pfns, int write, int force, + struct pinned_pfns *pfns) +{ + struct mm_struct *mm = current->mm; + struct vm_area_struct *vma; + int ret = 0; + int err; + + if (nr_pfns <= 0) + return 0; + + if (nr_pfns > pfns->nr_allocated_pfns) + nr_pfns = pfns->nr_allocated_pfns; + + down_read(&mm->mmap_sem); + vma = find_vma_intersection(mm, start, start + 1); + if (!vma) { + ret = -EFAULT; + goto out; + } + if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) { + pfns->got_ref = 1; + pfns->is_pages = 1; + ret = get_user_pages(current, mm, start, nr_pfns, write, force, + pfns_vector_pages(pfns), NULL); + goto out; + } + + pfns->got_ref = 0; + pfns->is_pages = 0; + do { + unsigned long *nums = pfns_vector_pfns(pfns); + + while (ret < nr_pfns && start + PAGE_SIZE <= vma->vm_end) { + err = follow_pfn(vma, start, &nums[ret]); + if (err) { + if (ret == 0) + ret = err; + goto out; + } + start += PAGE_SIZE; + ret++; + } + /* + * We stop if we have enough pages or if VMA doesn't completely + * cover the tail page. + */ + if (ret >= nr_pfns || start < vma->vm_end) + break; + vma = find_vma_intersection(mm, start, start + 1); + } while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP)); +out: + up_read(&mm->mmap_sem); + if (!ret) + ret = -EFAULT; + if (ret > 0) + pfns->nr_pfns = ret; + return ret; +} +EXPORT_SYMBOL(get_vaddr_pfns); + +/** + * put_vaddr_pfns() - drop references to pages if get_vaddr_pfns() acquired them + * @pfns: structure with pfns we pinned + * + * Drop references to pages if get_vaddr_pfns() acquired them. We also + * invalidate the array of pfns so that it is prepared for the next call into + * get_vaddr_pfns(). + */ +void put_vaddr_pfns(struct pinned_pfns *pfns) +{ + int i; + + if (!pfns->got_ref) + goto out; + if (pfns->is_pages) { + struct page **pages = pfns_vector_pages(pfns); + + for (i = 0; i < pfns->nr_pfns; i++) + put_page(pages[i]); + } else { + unsigned long *nums = pfns_vector_pfns(pfns); + + for (i = 0; i < pfns->nr_pfns; i++) + put_page(pfn_to_page(nums[i])); + } + pfns->got_ref = 0; +out: + pfns->nr_pfns = 0; +} +EXPORT_SYMBOL(put_vaddr_pfns); + +/** + * pfns_vector_to_pages - convert vector of pfns to vector of page pointers + * @pfns: structure with pinned pfns + * + * Convert @pfns to not contain array of pfns but array of page pointers. + * If the conversion is successful, return 0. Otherwise return an error. + */ +int pfns_vector_to_pages(struct pinned_pfns *pfns) +{ + int i; + unsigned long *nums; + struct page **pages; + + if (pfns->is_pages) + return 0; + nums = pfns_vector_pfns(pfns); + for (i = 0; i < pfns->nr_pfns; i++) + if (!pfn_valid(nums[i])) + return -EINVAL; + pages = (struct page **)nums; + for (i = 0; i < pfns->nr_pfns; i++) + pages[i] = pfn_to_page(nums[i]); + pfns->is_pages = 1; + return 0; +} +EXPORT_SYMBOL(pfns_vector_to_pages); + +/** + * pfns_vector_create() - allocate & initialize structure for pinned pfns + * @nr_pfns: number of pfns slots we should reserve + * + * Allocate and initialize struct pinned_pfns to be able to hold @nr_pfns + * pfns. + */ +struct pinned_pfns *pfns_vector_create(int nr_pfns) +{ + struct pinned_pfns *pfns; + int size = sizeof(struct pinned_pfns) + sizeof(unsigned long) * nr_pfns; + + if (WARN_ON_ONCE(nr_pfns <= 0)) + return NULL; + if (size <= PAGE_SIZE) + pfns = kmalloc(size, GFP_KERNEL); + else + pfns = vmalloc(size); + if (!pfns) + return NULL; + pfns->nr_allocated_pfns = nr_pfns; + pfns->nr_pfns = 0; + return pfns; +} +EXPORT_SYMBOL(pfns_vector_create); + +/** + * pfns_vector_destroy() - free memory allocated to carry pinned pfns + * @pfns: Memory to free + * + * Free structure allocated by pfns_vector_create() to carry pinned pfns. + */ +void pfns_vector_destroy(struct pinned_pfns *pfns) +{ + if (!is_vmalloc_addr(pfns)) + kfree(pfns); + else + vfree(pfns); +} +EXPORT_SYMBOL(pfns_vector_destroy); + +/** * get_dump_page() - pin user page in memory while writing it to core dump * @addr: user address *
On Tue, Mar 17, 2015 at 12:56:32PM +0100, Jan Kara wrote:
Provide new function get_vaddr_pfns(). This function maps virtual addresses from given start and fills given array with page frame numbers of the corresponding pages. If given start belongs to a normal vma, the function grabs reference to each of the pages to pin them in memory. If start belongs to VM_IO | VM_PFNMAP vma, we don't touch page structures. Caller should make sure pfns aren't reused for anything else while he is using them.
This function is created for various drivers to simplify handling of their buffers.
Signed-off-by: Jan Kara jack@suse.cz
include/linux/mm.h | 38 +++++++++++ mm/gup.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h index 47a93928b90f..a5045df92454 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1279,6 +1279,44 @@ long get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, int write, int force, struct page **pages); int get_user_pages_fast(unsigned long start, int nr_pages, int write, struct page **pages);
+/* Container for pinned pfns / pages */ +struct pinned_pfns {
- unsigned int nr_allocated_pfns; /* Number of pfns we have space for */
- unsigned int nr_pfns; /* Number of pfns stored in pfns array */
- unsigned int got_ref:1; /* Did we pin pfns by getting page ref? */
- unsigned int is_pages:1; /* Does array contain pages or pfns? */
The bit field is probably overkill as I expect it'll get padded out for pointer alignment anyway. Just use bools.
is_pfns is less ambiguous than is_pages but not very important.
The naming is not great in general. Only struct pages are pinned in the traditional meaning of the word. The raw PFNs are not so there is no such thing as a "pinned pfns". It might be better just to call it frame_vectors and document that it's either raw PFNs that the caller should be responsible for or struct pages that are pinned.
- void *ptrs[0]; /* Array of pinned pfns / pages.
* Use pfns_vector_pages() or* pfns_vector_pfns() for access */+};
+struct pinned_pfns *pfns_vector_create(int nr_pfns); +void pfns_vector_destroy(struct pinned_pfns *pfns); +int get_vaddr_pfns(unsigned long start, int nr_pfns, int write, int force,
struct pinned_pfns *pfns);
It's not critical but I generally prefer to see bools for new APIs like this.
unsigned int for nr_pfns.
+void put_vaddr_pfns(struct pinned_pfns *pfns); +int pfns_vector_to_pages(struct pinned_pfns *pfns);
+static inline int pfns_vector_count(struct pinned_pfns *pfns) +{
- return pfns->nr_pfns;
+}
should return unsigned int.
+static inline struct page **pfns_vector_pages(struct pinned_pfns *pfns) +{
- if (!pfns->is_pages)
return NULL;- return (struct page **)(pfns->ptrs);
+}
+static inline unsigned long *pfns_vector_pfns(struct pinned_pfns *pfns) +{
- if (pfns->is_pages)
return NULL;- return (unsigned long *)(pfns->ptrs);
+}
struct kvec; int get_kernel_pages(const struct kvec *iov, int nr_pages, int write, struct page **pages); diff --git a/mm/gup.c b/mm/gup.c index a6e24e246f86..63903913ab04 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -819,6 +819,186 @@ long get_user_pages(struct task_struct *tsk, struct mm_struct *mm, EXPORT_SYMBOL(get_user_pages);
/**
- get_vaddr_pfns() - map virtual addresses to pfns
- @start: starting user address
- @nr_pfns: number of pfns from start to map
- @write: whether pages will be written to by the caller
- @force: whether to force write access even if user mapping is
readonly. This will result in the page being COWed even
in MAP_SHARED mappings. You do not want this.
- @pfns: structure which receives pfns of the pages mapped.
It should have space for at least nr_pfns pfns.
- This function maps virtual addresses from @start and fills @pfns structure
- with page frame numbers of corresponding pages. If @start belongs to a
- normal vma, the function grabs reference to each of the pages to pin them in
- memory. If @start belongs to VM_IO | VM_PFNMAP vma, we don't touch page
- structures. Caller should make sure pfns aren't reused for anything else
- while he is using them.
- This function takes care of grabbing mmap_sem as necessary.
- */
+int get_vaddr_pfns(unsigned long start, int nr_pfns, int write, int force,
struct pinned_pfns *pfns)+{
- struct mm_struct *mm = current->mm;
- struct vm_area_struct *vma;
- int ret = 0;
- int err;
- if (nr_pfns <= 0)
return 0;
I know I suggested that nr_pfns should be unsigned earlier and then I saw this. Is there any valid use of the API that would pass in a negative number here?
- if (nr_pfns > pfns->nr_allocated_pfns)
nr_pfns = pfns->nr_allocated_pfns;
Should this be a WARN_ON_ONCE? You recover from it obviously but the return value is not documented to say that it could return less than requested. Of course, this is implied but a caller might assume it's due to a transient error instead of broken API usage.
- down_read(&mm->mmap_sem);
- vma = find_vma_intersection(mm, start, start + 1);
- if (!vma) {
ret = -EFAULT;goto out;- }
Returning -EFAULT means that the return value has to be signed but the structure itself has unsigned int for counters so the maximum number of PFNs supported is not available. We'd never want that number of PFNs pinned but still it might make sense to enforce the maximum possible size of the structure that takes into account the values needed for returning errors.
- if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
pfns->got_ref = 1;pfns->is_pages = 1;ret = get_user_pages(current, mm, start, nr_pfns, write, force,pfns_vector_pages(pfns), NULL);goto out;- }
- pfns->got_ref = 0;
- pfns->is_pages = 0;
- do {
unsigned long *nums = pfns_vector_pfns(pfns);while (ret < nr_pfns && start + PAGE_SIZE <= vma->vm_end) {err = follow_pfn(vma, start, &nums[ret]);if (err) {if (ret == 0)ret = err;goto out;}start += PAGE_SIZE;ret++;}/** We stop if we have enough pages or if VMA doesn't completely* cover the tail page.*/if (ret >= nr_pfns || start < vma->vm_end)break;vma = find_vma_intersection(mm, start, start + 1);- } while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
Documentation probably should mention that a range that strides VMA types will return partial results.
+out:
- up_read(&mm->mmap_sem);
- if (!ret)
ret = -EFAULT;
Really? Maybe we just failed to call get_user_pages on any of them. The meaning of -EFAULT probably needs a comment.
- if (ret > 0)
pfns->nr_pfns = ret;- return ret;
+} +EXPORT_SYMBOL(get_vaddr_pfns);
+/**
- put_vaddr_pfns() - drop references to pages if get_vaddr_pfns() acquired them
- @pfns: structure with pfns we pinned
- Drop references to pages if get_vaddr_pfns() acquired them. We also
- invalidate the array of pfns so that it is prepared for the next call into
- get_vaddr_pfns().
- */
+void put_vaddr_pfns(struct pinned_pfns *pfns) +{
- int i;
- if (!pfns->got_ref)
goto out;- if (pfns->is_pages) {
struct page **pages = pfns_vector_pages(pfns);for (i = 0; i < pfns->nr_pfns; i++)put_page(pages[i]);
Ok.
- } else {
unsigned long *nums = pfns_vector_pfns(pfns);for (i = 0; i < pfns->nr_pfns; i++)put_page(pfn_to_page(nums[i]));- }
I'm missing something here. The !is_pages branch in get_vaddr_pfns() used follow_pfn() and it does not take a reference on the struct page so I'm not sure what this is putting exactly.
- pfns->got_ref = 0;
+out:
- pfns->nr_pfns = 0;
+} +EXPORT_SYMBOL(put_vaddr_pfns);
EXPORT_SYMBOL_GPL?
+/**
- pfns_vector_to_pages - convert vector of pfns to vector of page pointers
- @pfns: structure with pinned pfns
- Convert @pfns to not contain array of pfns but array of page pointers.
- If the conversion is successful, return 0. Otherwise return an error.
- */
+int pfns_vector_to_pages(struct pinned_pfns *pfns) +{
- int i;
- unsigned long *nums;
- struct page **pages;
- if (pfns->is_pages)
return 0;- nums = pfns_vector_pfns(pfns);
- for (i = 0; i < pfns->nr_pfns; i++)
if (!pfn_valid(nums[i]))return -EINVAL;- pages = (struct page **)nums;
- for (i = 0; i < pfns->nr_pfns; i++)
pages[i] = pfn_to_page(nums[i]);- pfns->is_pages = 1;
- return 0;
+} +EXPORT_SYMBOL(pfns_vector_to_pages);
+/**
- pfns_vector_create() - allocate & initialize structure for pinned pfns
- @nr_pfns: number of pfns slots we should reserve
- Allocate and initialize struct pinned_pfns to be able to hold @nr_pfns
- pfns.
- */
+struct pinned_pfns *pfns_vector_create(int nr_pfns) +{
- struct pinned_pfns *pfns;
- int size = sizeof(struct pinned_pfns) + sizeof(unsigned long) * nr_pfns;
Should be sizeof(void *)
- if (WARN_ON_ONCE(nr_pfns <= 0))
return NULL;- if (size <= PAGE_SIZE)
pfns = kmalloc(size, GFP_KERNEL);
kmalloc supports larger sizes than this but I suspect this was deliberate to avoid high-order allocations.
- else
pfns = vmalloc(size);- if (!pfns)
return NULL;- pfns->nr_allocated_pfns = nr_pfns;
- pfns->nr_pfns = 0;
- return pfns;
+} +EXPORT_SYMBOL(pfns_vector_create);
+/**
- pfns_vector_destroy() - free memory allocated to carry pinned pfns
- @pfns: Memory to free
- Free structure allocated by pfns_vector_create() to carry pinned pfns.
- */
+void pfns_vector_destroy(struct pinned_pfns *pfns) +{
- if (!is_vmalloc_addr(pfns))
kfree(pfns);- else
vfree(pfns);+} +EXPORT_SYMBOL(pfns_vector_destroy);
+/**
- get_dump_page() - pin user page in memory while writing it to core dump
- @addr: user address
-- 2.1.4
-- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Thu 30-04-15 16:55:31, Mel Gorman wrote:
On Tue, Mar 17, 2015 at 12:56:32PM +0100, Jan Kara wrote:
Provide new function get_vaddr_pfns(). This function maps virtual addresses from given start and fills given array with page frame numbers of the corresponding pages. If given start belongs to a normal vma, the function grabs reference to each of the pages to pin them in memory. If start belongs to VM_IO | VM_PFNMAP vma, we don't touch page structures. Caller should make sure pfns aren't reused for anything else while he is using them.
This function is created for various drivers to simplify handling of their buffers.
Signed-off-by: Jan Kara jack@suse.cz
include/linux/mm.h | 38 +++++++++++ mm/gup.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h index 47a93928b90f..a5045df92454 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1279,6 +1279,44 @@ long get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, int write, int force, struct page **pages); int get_user_pages_fast(unsigned long start, int nr_pages, int write, struct page **pages);
+/* Container for pinned pfns / pages */ +struct pinned_pfns {
- unsigned int nr_allocated_pfns; /* Number of pfns we have space for */
- unsigned int nr_pfns; /* Number of pfns stored in pfns array */
- unsigned int got_ref:1; /* Did we pin pfns by getting page ref? */
- unsigned int is_pages:1; /* Does array contain pages or pfns? */
The bit field is probably overkill as I expect it'll get padded out for pointer alignment anyway. Just use bools.
Makes sense.
is_pfns is less ambiguous than is_pages but not very important.
The naming is not great in general. Only struct pages are pinned in the traditional meaning of the word. The raw PFNs are not so there is no such thing as a "pinned pfns". It might be better just to call it frame_vectors and document that it's either raw PFNs that the caller should be responsible for or struct pages that are pinned.
Good point. I'll try to come up with a better name.
<snip> - I agree with minor comments there.
/**
- get_vaddr_pfns() - map virtual addresses to pfns
- @start: starting user address
- @nr_pfns: number of pfns from start to map
- @write: whether pages will be written to by the caller
- @force: whether to force write access even if user mapping is
readonly. This will result in the page being COWed even
in MAP_SHARED mappings. You do not want this.
- @pfns: structure which receives pfns of the pages mapped.
It should have space for at least nr_pfns pfns.
- This function maps virtual addresses from @start and fills @pfns structure
- with page frame numbers of corresponding pages. If @start belongs to a
- normal vma, the function grabs reference to each of the pages to pin them in
- memory. If @start belongs to VM_IO | VM_PFNMAP vma, we don't touch page
- structures. Caller should make sure pfns aren't reused for anything else
- while he is using them.
- This function takes care of grabbing mmap_sem as necessary.
- */
+int get_vaddr_pfns(unsigned long start, int nr_pfns, int write, int force,
struct pinned_pfns *pfns)+{
- struct mm_struct *mm = current->mm;
- struct vm_area_struct *vma;
- int ret = 0;
- int err;
- if (nr_pfns <= 0)
return 0;I know I suggested that nr_pfns should be unsigned earlier and then I saw this. Is there any valid use of the API that would pass in a negative number here?
No. It was there just because I had ints everywhere without too much thought (it's shorter to type *grin*). I'll put unsigned types where it makes sense and add a test for INT_MAX so that get_vaddr_pfns() can still return negative errors.
- if (nr_pfns > pfns->nr_allocated_pfns)
nr_pfns = pfns->nr_allocated_pfns;Should this be a WARN_ON_ONCE? You recover from it obviously but the return value is not documented to say that it could return less than requested. Of course, this is implied but a caller might assume it's due to a transient error instead of broken API usage.
Yep, good idea.
- down_read(&mm->mmap_sem);
- vma = find_vma_intersection(mm, start, start + 1);
- if (!vma) {
ret = -EFAULT;goto out;- }
Returning -EFAULT means that the return value has to be signed but the structure itself has unsigned int for counters so the maximum number of PFNs supported is not available. We'd never want that number of PFNs pinned but still it might make sense to enforce the maximum possible size of the structure that takes into account the values needed for returning errors.
Yes. See above.
- if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
pfns->got_ref = 1;pfns->is_pages = 1;ret = get_user_pages(current, mm, start, nr_pfns, write, force,pfns_vector_pages(pfns), NULL);goto out;- }
- pfns->got_ref = 0;
- pfns->is_pages = 0;
- do {
unsigned long *nums = pfns_vector_pfns(pfns);while (ret < nr_pfns && start + PAGE_SIZE <= vma->vm_end) {err = follow_pfn(vma, start, &nums[ret]);if (err) {if (ret == 0)ret = err;goto out;}start += PAGE_SIZE;ret++;}/** We stop if we have enough pages or if VMA doesn't completely* cover the tail page.*/if (ret >= nr_pfns || start < vma->vm_end)break;vma = find_vma_intersection(mm, start, start + 1);- } while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
Documentation probably should mention that a range that strides VMA types will return partial results.
Good point. Will add that.
+out:
- up_read(&mm->mmap_sem);
- if (!ret)
ret = -EFAULT;Really? Maybe we just failed to call get_user_pages on any of them. The meaning of -EFAULT probably needs a comment.
So the only real case where we could have 0 here is when get_user_pages() returns 0 for whatever reason. It seemed better to me to translate 0 to -EFAULT since all the drivers I saw did that anyway (at least those that didn't forget about checking for 0). It just seems as a less error-prone interface to return error or number > 0. I agree that -EFAULT doesn't necessarily always make sense so if you have idea for a better error I'm open to suggestions.
- if (ret > 0)
pfns->nr_pfns = ret;- return ret;
+} +EXPORT_SYMBOL(get_vaddr_pfns);
+/**
- put_vaddr_pfns() - drop references to pages if get_vaddr_pfns() acquired them
- @pfns: structure with pfns we pinned
- Drop references to pages if get_vaddr_pfns() acquired them. We also
- invalidate the array of pfns so that it is prepared for the next call into
- get_vaddr_pfns().
- */
+void put_vaddr_pfns(struct pinned_pfns *pfns) +{
- int i;
- if (!pfns->got_ref)
goto out;- if (pfns->is_pages) {
struct page **pages = pfns_vector_pages(pfns);for (i = 0; i < pfns->nr_pfns; i++)put_page(pages[i]);Ok.
- } else {
unsigned long *nums = pfns_vector_pfns(pfns);for (i = 0; i < pfns->nr_pfns; i++)put_page(pfn_to_page(nums[i]));- }
I'm missing something here. The !is_pages branch in get_vaddr_pfns() used follow_pfn() and it does not take a reference on the struct page so I'm not sure what this is putting exactly.
So this case should have covered the situation when we have a vector of normal pages (thus got_ref == 1) and convert that to a vector of pfns. Noone currently does that and that function for conversion even doesn't seem to exist but it seems to me at least omap driver may need it and the conversion I did has a bug. Anyway, I'll recheck the situation and either drop this or make the function more comprehensible.
- pfns->got_ref = 0;
+out:
- pfns->nr_pfns = 0;
+} +EXPORT_SYMBOL(put_vaddr_pfns);
EXPORT_SYMBOL_GPL?
I've just followed what __get_user_pages() does...
+/**
- pfns_vector_to_pages - convert vector of pfns to vector of page pointers
- @pfns: structure with pinned pfns
- Convert @pfns to not contain array of pfns but array of page pointers.
- If the conversion is successful, return 0. Otherwise return an error.
- */
+int pfns_vector_to_pages(struct pinned_pfns *pfns) +{
- int i;
- unsigned long *nums;
- struct page **pages;
- if (pfns->is_pages)
return 0;- nums = pfns_vector_pfns(pfns);
- for (i = 0; i < pfns->nr_pfns; i++)
if (!pfn_valid(nums[i]))return -EINVAL;- pages = (struct page **)nums;
- for (i = 0; i < pfns->nr_pfns; i++)
pages[i] = pfn_to_page(nums[i]);- pfns->is_pages = 1;
- return 0;
+} +EXPORT_SYMBOL(pfns_vector_to_pages);
+/**
- pfns_vector_create() - allocate & initialize structure for pinned pfns
- @nr_pfns: number of pfns slots we should reserve
- Allocate and initialize struct pinned_pfns to be able to hold @nr_pfns
- pfns.
- */
+struct pinned_pfns *pfns_vector_create(int nr_pfns) +{
- struct pinned_pfns *pfns;
- int size = sizeof(struct pinned_pfns) + sizeof(unsigned long) * nr_pfns;
Should be sizeof(void *)
OK.
- if (WARN_ON_ONCE(nr_pfns <= 0))
return NULL;- if (size <= PAGE_SIZE)
pfns = kmalloc(size, GFP_KERNEL);kmalloc supports larger sizes than this but I suspect this was deliberate to avoid high-order allocations.
Exactly.
Thanks a lot for review Mel!
Honza
Convert omap_vout_uservirt_to_phys() to use get_vaddr_pfns() instead of hand made mapping of virtual address to physical address. Also the function leaked page reference from get_user_pages() so fix that by properly release the reference when omap_vout_buffer_release() is called.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/platform/omap/omap_vout.c | 67 +++++++++++++++------------------ 1 file changed, 31 insertions(+), 36 deletions(-)
diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c index ba2d8f973d58..e7d342bb71dd 100644 --- a/drivers/media/platform/omap/omap_vout.c +++ b/drivers/media/platform/omap/omap_vout.c @@ -195,46 +195,34 @@ static int omap_vout_try_format(struct v4l2_pix_format *pix) }
/* - * omap_vout_uservirt_to_phys: This inline function is used to convert user - * space virtual address to physical address. + * omap_vout_get_userptr: Convert user space virtual address to physical + * address. */ -static unsigned long omap_vout_uservirt_to_phys(unsigned long virtp) +static int omap_vout_get_userptr(struct videobuf_buffer *vb, u32 virtp, + u32 *physp) { - unsigned long physp = 0; - struct vm_area_struct *vma; - struct mm_struct *mm = current->mm; + struct pinned_pfns *pfns; + int ret;
/* For kernel direct-mapped memory, take the easy way */ - if (virtp >= PAGE_OFFSET) - return virt_to_phys((void *) virtp); - - down_read(¤t->mm->mmap_sem); - vma = find_vma(mm, virtp); - if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) { - /* this will catch, kernel-allocated, mmaped-to-usermode - addresses */ - physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start); - up_read(¤t->mm->mmap_sem); - } else { - /* otherwise, use get_user_pages() for general userland pages */ - int res, nr_pages = 1; - struct page *pages; + if (virtp >= PAGE_OFFSET) { + *physp = virt_to_phys((void *)virtp); + return 0; + }
- res = get_user_pages(current, current->mm, virtp, nr_pages, 1, - 0, &pages, NULL); - up_read(¤t->mm->mmap_sem); + pfns = pfns_vector_create(1); + if (!pfns) + return -ENOMEM;
- if (res == nr_pages) { - physp = __pa(page_address(&pages[0]) + - (virtp & ~PAGE_MASK)); - } else { - printk(KERN_WARNING VOUT_NAME - "get_user_pages failed\n"); - return 0; - } + ret = get_vaddr_pfns(virtp, 1, 1, 0, pfns); + if (ret != 1) { + pfns_vector_destroy(pfns); + return -EINVAL; } + *physp = __pfn_to_phys(pfns_vector_pfns(pfns)[0]); + vb->priv = pfns;
- return physp; + return 0; }
/* @@ -788,11 +776,15 @@ static int omap_vout_buffer_prepare(struct videobuf_queue *q, * address of the buffer */ if (V4L2_MEMORY_USERPTR == vb->memory) { + int ret; + if (0 == vb->baddr) return -EINVAL; /* Physical address */ - vout->queued_buf_addr[vb->i] = (u8 *) - omap_vout_uservirt_to_phys(vb->baddr); + ret = omap_vout_get_userptr(vb, vb->baddr, + (u32 *)&vout->queued_buf_addr[vb->i]); + if (ret < 0) + return ret; } else { unsigned long addr, dma_addr; unsigned long size; @@ -841,9 +833,12 @@ static void omap_vout_buffer_release(struct videobuf_queue *q, struct omap_vout_device *vout = q->priv_data;
vb->state = VIDEOBUF_NEEDS_INIT; + if (vb->memory == V4L2_MEMORY_USERPTR && vb->priv) { + struct pinned_pfns *pfns = vb->priv;
- if (V4L2_MEMORY_MMAP != vout->memory) - return; + put_vaddr_pfns(pfns); + pfns_vector_destroy(pfns); + } }
/*
Provide simple helper functions to map virtual address range into an array of pfns.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/v4l2-core/videobuf2-memops.c | 57 ++++++++++++++++++++++++++++++ include/media/videobuf2-memops.h | 4 +++ 2 files changed, 61 insertions(+)
diff --git a/drivers/media/v4l2-core/videobuf2-memops.c b/drivers/media/v4l2-core/videobuf2-memops.c index 81c1ad8b2cf1..80ade22b920c 100644 --- a/drivers/media/v4l2-core/videobuf2-memops.c +++ b/drivers/media/v4l2-core/videobuf2-memops.c @@ -137,6 +137,63 @@ int vb2_get_contig_userptr(unsigned long vaddr, unsigned long size, EXPORT_SYMBOL_GPL(vb2_get_contig_userptr);
/** + * vb2_create_pfnvec() - map virtual addresses to pfns + * @start: Virtual user address where we start mapping + * @length: Length of a range to map + * @write: Should we map for writing into the area + * + * This function allocates and fills in a vector with pfns corresponding to + * virtual address range passed in arguments. If pfns have corresponding pages, + * page references are also grabbed to pin pages in memory. The function + * returns pointer to the vector on success and error pointer in case of + * failure. Returned vector needs to be freed via vb2_destroy_pfnvec(). + */ +struct pinned_pfns *vb2_create_pfnvec(unsigned long start, unsigned long length, + bool write) +{ + int ret; + unsigned long first, last; + unsigned long nr; + struct pinned_pfns *pfns; + + first = start >> PAGE_SHIFT; + last = (start + length - 1) >> PAGE_SHIFT; + nr = last - first + 1; + pfns = pfns_vector_create(nr); + if (!pfns) + return ERR_PTR(-ENOMEM); + ret = get_vaddr_pfns(start, nr, write, 1, pfns); + if (ret < 0) + goto out_destroy; + /* We accept only complete set of PFNs */ + if (ret != nr) { + ret = -EFAULT; + goto out_release; + } + return pfns; +out_release: + put_vaddr_pfns(pfns); +out_destroy: + pfns_vector_destroy(pfns); + return ERR_PTR(ret); +} +EXPORT_SYMBOL(vb2_create_pfnvec); + +/** + * vb2_destroy_pfnvec() - release vector of mapped pfns + * @pfns: vector of pfns to release + * + * This releases references to all pages in the vector @pfns (if corresponding + * pfns are backed by pages) and frees the passed vector. + */ +void vb2_destroy_pfnvec(struct pinned_pfns *pfns) +{ + put_vaddr_pfns(pfns); + pfns_vector_destroy(pfns); +} +EXPORT_SYMBOL(vb2_destroy_pfnvec); + +/** * vb2_common_vm_open() - increase refcount of the vma * @vma: virtual memory region for the mapping * diff --git a/include/media/videobuf2-memops.h b/include/media/videobuf2-memops.h index f05444ca8c0c..868f9c1cd92d 100644 --- a/include/media/videobuf2-memops.h +++ b/include/media/videobuf2-memops.h @@ -15,6 +15,7 @@ #define _MEDIA_VIDEOBUF2_MEMOPS_H
#include <media/videobuf2-core.h> +#include <linux/mm.h>
/** * vb2_vmarea_handler - common vma refcount tracking handler @@ -36,5 +37,8 @@ int vb2_get_contig_userptr(unsigned long vaddr, unsigned long size, struct vm_area_struct *vb2_get_vma(struct vm_area_struct *vma); void vb2_put_vma(struct vm_area_struct *vma);
+struct pinned_pfns *vb2_create_pfnvec(unsigned long start, unsigned long length, + bool write); +void vb2_destroy_pfnvec(struct pinned_pfns *pfns);
#endif
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/v4l2-core/videobuf2-dma-sg.c | 97 +++++------------------------- 1 file changed, 15 insertions(+), 82 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c index 71510e4f7d7c..cc82c30d02cf 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c @@ -38,6 +38,7 @@ struct vb2_dma_sg_buf { struct device *dev; void *vaddr; struct page **pages; + struct pinned_pfns *pfns; int offset; enum dma_data_direction dma_dir; struct sg_table sg_table; @@ -51,7 +52,6 @@ struct vb2_dma_sg_buf { unsigned int num_pages; atomic_t refcount; struct vb2_vmarea_handler handler; - struct vm_area_struct *vma;
struct dma_buf_attachment *db_attach; }; @@ -224,25 +224,17 @@ static void vb2_dma_sg_finish(void *buf_priv) dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); }
-static inline int vma_is_io(struct vm_area_struct *vma) -{ - return !!(vma->vm_flags & (VM_IO | VM_PFNMAP)); -} - static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, unsigned long size, enum dma_data_direction dma_dir) { struct vb2_dma_sg_conf *conf = alloc_ctx; struct vb2_dma_sg_buf *buf; - unsigned long first, last; - int num_pages_from_user; - struct vm_area_struct *vma; struct sg_table *sgt; DEFINE_DMA_ATTRS(attrs); + struct pinned_pfns *pfns;
dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs); - buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return NULL; @@ -253,63 +245,19 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, buf->offset = vaddr & ~PAGE_MASK; buf->size = size; buf->dma_sgt = &buf->sg_table; + pfns = vb2_create_pfnvec(vaddr, size, buf->dma_dir == DMA_FROM_DEVICE); + if (IS_ERR(pfns)) + goto userptr_fail_pfnvec; + buf->pfns = pfns;
- first = (vaddr & PAGE_MASK) >> PAGE_SHIFT; - last = ((vaddr + size - 1) & PAGE_MASK) >> PAGE_SHIFT; - buf->num_pages = last - first + 1; - - buf->pages = kzalloc(buf->num_pages * sizeof(struct page *), - GFP_KERNEL); - if (!buf->pages) - goto userptr_fail_alloc_pages; - - down_read(¤t->mm->mmap_sem); - vma = find_vma(current->mm, vaddr); - if (!vma) { - dprintk(1, "no vma for address %lu\n", vaddr); - goto userptr_fail_find_vma; - } - - if (vma->vm_end < vaddr + size) { - dprintk(1, "vma at %lu is too small for %lu bytes\n", - vaddr, size); - goto userptr_fail_find_vma; - } - - buf->vma = vb2_get_vma(vma); - if (!buf->vma) { - dprintk(1, "failed to copy vma\n"); - goto userptr_fail_find_vma; - } - - if (vma_is_io(buf->vma)) { - for (num_pages_from_user = 0; - num_pages_from_user < buf->num_pages; - ++num_pages_from_user, vaddr += PAGE_SIZE) { - unsigned long pfn; - - if (follow_pfn(vma, vaddr, &pfn)) { - dprintk(1, "no page for address %lu\n", vaddr); - break; - } - buf->pages[num_pages_from_user] = pfn_to_page(pfn); - } - } else - num_pages_from_user = get_user_pages(current, current->mm, - vaddr & PAGE_MASK, - buf->num_pages, - buf->dma_dir == DMA_FROM_DEVICE, - 1, /* force */ - buf->pages, - NULL); - up_read(¤t->mm->mmap_sem); - - if (num_pages_from_user != buf->num_pages) - goto userptr_fail_get_user_pages; + if (pfns_vector_to_pages(pfns)) + goto userptr_fail_sgtable; + buf->pages = pfns_vector_pages(pfns); + buf->num_pages = pfns_vector_count(pfns);
if (sg_alloc_table_from_pages(buf->dma_sgt, buf->pages, buf->num_pages, buf->offset, size, 0)) - goto userptr_fail_alloc_table_from_pages; + goto userptr_fail_sgtable;
sgt = &buf->sg_table; /* @@ -323,19 +271,9 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
userptr_fail_map: sg_free_table(&buf->sg_table); -userptr_fail_alloc_table_from_pages: -userptr_fail_get_user_pages: - dprintk(1, "get_user_pages requested/got: %d/%d]\n", - buf->num_pages, num_pages_from_user); - if (!vma_is_io(buf->vma)) - while (--num_pages_from_user >= 0) - put_page(buf->pages[num_pages_from_user]); - down_read(¤t->mm->mmap_sem); - vb2_put_vma(buf->vma); -userptr_fail_find_vma: - up_read(¤t->mm->mmap_sem); - kfree(buf->pages); -userptr_fail_alloc_pages: +userptr_fail_sgtable: + vb2_destroy_pfnvec(pfns); +userptr_fail_pfnvec: kfree(buf); return NULL; } @@ -362,13 +300,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv) while (--i >= 0) { if (buf->dma_dir == DMA_FROM_DEVICE) set_page_dirty_lock(buf->pages[i]); - if (!vma_is_io(buf->vma)) - put_page(buf->pages[i]); } - kfree(buf->pages); - down_read(¤t->mm->mmap_sem); - vb2_put_vma(buf->vma); - up_read(¤t->mm->mmap_sem); + vb2_destroy_pfnvec(buf->pfns); kfree(buf); }
Convert vb2_vmalloc_get_userptr() to use passed vector of pfns. When we are doing that there's no need to allocate page array and some code can be simplified.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/v4l2-core/videobuf2-vmalloc.c | 94 +++++++++++------------------ 1 file changed, 36 insertions(+), 58 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c index c060cf9662fa..eb1d9971c54e 100644 --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c @@ -23,11 +23,9 @@
struct vb2_vmalloc_buf { void *vaddr; - struct page **pages; - struct vm_area_struct *vma; + struct pinned_pfns *pfns; enum dma_data_direction dma_dir; unsigned long size; - unsigned int n_pages; atomic_t refcount; struct vb2_vmarea_handler handler; struct dma_buf *dbuf; @@ -76,10 +74,8 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr, enum dma_data_direction dma_dir) { struct vb2_vmalloc_buf *buf; - unsigned long first, last; - int n_pages, offset; - struct vm_area_struct *vma; - dma_addr_t physp; + struct pinned_pfns *pfns; + int n_pages, offset, i;
buf = kzalloc(sizeof(*buf), GFP_KERNEL); if (!buf) @@ -88,53 +84,36 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr, buf->dma_dir = dma_dir; offset = vaddr & ~PAGE_MASK; buf->size = size; - - down_read(¤t->mm->mmap_sem); - vma = find_vma(current->mm, vaddr); - if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) { - if (vb2_get_contig_userptr(vaddr, size, &vma, &physp)) - goto fail_pages_array_alloc; - buf->vma = vma; - buf->vaddr = (__force void *)ioremap_nocache(physp, size); - if (!buf->vaddr) - goto fail_pages_array_alloc; + pfns = vb2_create_pfnvec(vaddr, size, dma_dir == DMA_FROM_DEVICE); + if (IS_ERR(pfns)) + goto fail_pfnvec_create; + buf->pfns = pfns; + n_pages = pfns_vector_count(pfns); + if (pfns_vector_to_pages(pfns) < 0) { + unsigned long *nums = pfns_vector_pfns(pfns); + + /* + * We cannot get page pointers for these pfns. Check memory is + * physically contiguous and use direct mapping. + */ + for (i = 1; i < n_pages; i++) + if (nums[i-1] + 1 != nums[i]) + goto fail_map; + buf->vaddr = (__force void *) + ioremap_nocache(nums[0] << PAGE_SHIFT, size); } else { - first = vaddr >> PAGE_SHIFT; - last = (vaddr + size - 1) >> PAGE_SHIFT; - buf->n_pages = last - first + 1; - buf->pages = kzalloc(buf->n_pages * sizeof(struct page *), - GFP_KERNEL); - if (!buf->pages) - goto fail_pages_array_alloc; - - /* current->mm->mmap_sem is taken by videobuf2 core */ - n_pages = get_user_pages(current, current->mm, - vaddr & PAGE_MASK, buf->n_pages, - dma_dir == DMA_FROM_DEVICE, - 1, /* force */ - buf->pages, NULL); - if (n_pages != buf->n_pages) - goto fail_get_user_pages; - - buf->vaddr = vm_map_ram(buf->pages, buf->n_pages, -1, + buf->vaddr = vm_map_ram(pfns_vector_pages(pfns), n_pages, -1, PAGE_KERNEL); - if (!buf->vaddr) - goto fail_get_user_pages; } - up_read(¤t->mm->mmap_sem);
+ if (!buf->vaddr) + goto fail_map; buf->vaddr += offset; return buf;
-fail_get_user_pages: - pr_debug("get_user_pages requested/got: %d/%d]\n", n_pages, - buf->n_pages); - while (--n_pages >= 0) - put_page(buf->pages[n_pages]); - kfree(buf->pages); - -fail_pages_array_alloc: - up_read(¤t->mm->mmap_sem); +fail_map: + vb2_destroy_pfnvec(pfns); +fail_pfnvec_create: kfree(buf);
return NULL; @@ -145,22 +124,21 @@ static void vb2_vmalloc_put_userptr(void *buf_priv) struct vb2_vmalloc_buf *buf = buf_priv; unsigned long vaddr = (unsigned long)buf->vaddr & PAGE_MASK; unsigned int i; + struct page **pages; + unsigned int n_pages;
- down_read(¤t->mm->mmap_sem); - if (buf->pages) { + if (buf->pfns->is_pages) { + n_pages = pfns_vector_count(buf->pfns); + pages = pfns_vector_pages(buf->pfns); if (vaddr) - vm_unmap_ram((void *)vaddr, buf->n_pages); - for (i = 0; i < buf->n_pages; ++i) { - if (buf->dma_dir == DMA_FROM_DEVICE) - set_page_dirty_lock(buf->pages[i]); - put_page(buf->pages[i]); - } - kfree(buf->pages); + vm_unmap_ram((void *)vaddr, n_pages); + if (buf->dma_dir == DMA_FROM_DEVICE) + for (i = 0; i < n_pages; i++) + set_page_dirty_lock(pages[i]); } else { - vb2_put_vma(buf->vma); iounmap((__force void __iomem *)buf->vaddr); } - up_read(¤t->mm->mmap_sem); + vb2_destroy_pfnvec(buf->pfns); kfree(buf); }
Convert vb2_dc_get_userptr() to use passed vector of pfns. When we are doing that there's no need to allocate page array and some code can be simplified.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/v4l2-core/videobuf2-dma-contig.c | 213 ++++--------------------- 1 file changed, 32 insertions(+), 181 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 96eceabb307b..d3cefc5c98bc 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -32,15 +32,13 @@ struct vb2_dc_buf { dma_addr_t dma_addr; enum dma_data_direction dma_dir; struct sg_table *dma_sgt; + struct pinned_pfns *pfns;
/* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount; struct sg_table *sgt_base;
- /* USERPTR related */ - struct vm_area_struct *vma; - /* DMABUF related */ struct dma_buf_attachment *db_attach; }; @@ -49,24 +47,6 @@ struct vb2_dc_buf { /* scatterlist table functions */ /*********************************************/
- -static void vb2_dc_sgt_foreach_page(struct sg_table *sgt, - void (*cb)(struct page *pg)) -{ - struct scatterlist *s; - unsigned int i; - - for_each_sg(sgt->sgl, s, sgt->orig_nents, i) { - struct page *page = sg_page(s); - unsigned int n_pages = PAGE_ALIGN(s->offset + s->length) - >> PAGE_SHIFT; - unsigned int j; - - for (j = 0; j < n_pages; ++j, ++page) - cb(page); - } -} - static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) { struct scatterlist *s; @@ -423,92 +403,12 @@ static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv, unsigned long flags) /* callbacks for USERPTR buffers */ /*********************************************/
-static inline int vma_is_io(struct vm_area_struct *vma) -{ - return !!(vma->vm_flags & (VM_IO | VM_PFNMAP)); -} - -static int vb2_dc_get_user_pfn(unsigned long start, int n_pages, - struct vm_area_struct *vma, unsigned long *res) -{ - unsigned long pfn, start_pfn, prev_pfn; - unsigned int i; - int ret; - - if (!vma_is_io(vma)) - return -EFAULT; - - ret = follow_pfn(vma, start, &pfn); - if (ret) - return ret; - - start_pfn = pfn; - start += PAGE_SIZE; - - for (i = 1; i < n_pages; ++i, start += PAGE_SIZE) { - prev_pfn = pfn; - ret = follow_pfn(vma, start, &pfn); - - if (ret) { - pr_err("no page for address %lu\n", start); - return ret; - } - if (pfn != prev_pfn + 1) - return -EINVAL; - } - - *res = start_pfn; - return 0; -} - -static int vb2_dc_get_user_pages(unsigned long start, struct page **pages, - int n_pages, struct vm_area_struct *vma, - enum dma_data_direction dma_dir) -{ - if (vma_is_io(vma)) { - unsigned int i; - - for (i = 0; i < n_pages; ++i, start += PAGE_SIZE) { - unsigned long pfn; - int ret = follow_pfn(vma, start, &pfn); - - if (!pfn_valid(pfn)) - return -EINVAL; - - if (ret) { - pr_err("no page for address %lu\n", start); - return ret; - } - pages[i] = pfn_to_page(pfn); - } - } else { - int n; - - n = get_user_pages(current, current->mm, start & PAGE_MASK, - n_pages, dma_dir == DMA_FROM_DEVICE, 1, pages, NULL); - /* negative error means that no page was pinned */ - n = max(n, 0); - if (n != n_pages) { - pr_err("got only %d of %d user pages\n", n, n_pages); - while (n) - put_page(pages[--n]); - return -EFAULT; - } - } - - return 0; -} - -static void vb2_dc_put_dirty_page(struct page *page) -{ - set_page_dirty_lock(page); - put_page(page); -} - static void vb2_dc_put_userptr(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + int i; + struct page **pages;
if (sgt) { DEFINE_DMA_ATTRS(attrs); @@ -520,15 +420,13 @@ static void vb2_dc_put_userptr(void *buf_priv) */ dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir, &attrs); - if (!vma_is_io(buf->vma)) - vb2_dc_sgt_foreach_page(sgt, vb2_dc_put_dirty_page); - + pages = pfns_vector_pages(buf->pfns); + for (i = 0; i < pfns_vector_count(buf->pfns); i++) + set_page_dirty_lock(pages[i]); sg_free_table(sgt); kfree(sgt); } - down_read(¤t->mm->mmap_sem); - vb2_put_vma(buf->vma); - up_read(¤t->mm->mmap_sem); + vb2_destroy_pfnvec(buf->pfns); kfree(buf); }
@@ -568,13 +466,10 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, { struct vb2_dc_conf *conf = alloc_ctx; struct vb2_dc_buf *buf; - unsigned long start; - unsigned long end; + struct pinned_pfns *pfns; unsigned long offset; - struct page **pages; - int n_pages; + int n_pages, i; int ret = 0; - struct vm_area_struct *vma; struct sg_table *sgt; unsigned long contig_size; unsigned long dma_align = dma_get_cache_alignment(); @@ -600,76 +495,43 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, buf->dev = conf->dev; buf->dma_dir = dma_dir;
- start = vaddr & PAGE_MASK; offset = vaddr & ~PAGE_MASK; - end = PAGE_ALIGN(vaddr + size); - n_pages = (end - start) >> PAGE_SHIFT; - - pages = kmalloc(n_pages * sizeof(pages[0]), GFP_KERNEL); - if (!pages) { - ret = -ENOMEM; - pr_err("failed to allocate pages table\n"); + pfns = vb2_create_pfnvec(vaddr, size, dma_dir == DMA_FROM_DEVICE); + if (IS_ERR(pfns)) { + ret = PTR_ERR(pfns); goto fail_buf; } + buf->pfns = pfns; + n_pages = pfns_vector_count(pfns); + ret = pfns_vector_to_pages(pfns); + if (ret < 0) { + unsigned long *nums = pfns_vector_pfns(pfns);
- down_read(¤t->mm->mmap_sem); - /* current->mm->mmap_sem is taken by videobuf2 core */ - vma = find_vma(current->mm, vaddr); - if (!vma) { - pr_err("no vma for address %lu\n", vaddr); - ret = -EFAULT; - goto fail_pages; - } - - if (vma->vm_end < vaddr + size) { - pr_err("vma at %lu is too small for %lu bytes\n", vaddr, size); - ret = -EFAULT; - goto fail_pages; - } - - buf->vma = vb2_get_vma(vma); - if (!buf->vma) { - pr_err("failed to copy vma\n"); - ret = -ENOMEM; - goto fail_pages; - } - - /* extract page list from userspace mapping */ - ret = vb2_dc_get_user_pages(start, pages, n_pages, vma, - dma_dir == DMA_FROM_DEVICE); - if (ret) { - unsigned long pfn; - if (vb2_dc_get_user_pfn(start, n_pages, vma, &pfn) == 0) { - up_read(¤t->mm->mmap_sem); - buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, pfn); - buf->size = size; - kfree(pages); - return buf; - } - - pr_err("failed to get user pages\n"); - goto fail_vma; + /* + * Failed to convert to pages... Check the memory is physically + * contiguous and use direct mapping + */ + for (i = 1; i < n_pages; i++) + if (nums[i-1] + 1 != nums[i]) + goto fail_pfnvec; + buf->dma_addr = vb2_dc_pfn_to_dma(buf->dev, nums[0]); + goto out; } - up_read(¤t->mm->mmap_sem);
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); if (!sgt) { pr_err("failed to allocate sg table\n"); ret = -ENOMEM; - goto fail_get_user_pages; + goto fail_pfnvec; }
- ret = sg_alloc_table_from_pages(sgt, pages, n_pages, + ret = sg_alloc_table_from_pages(sgt, pfns_vector_pages(pfns), n_pages, offset, size, GFP_KERNEL); if (ret) { pr_err("failed to initialize sg table\n"); goto fail_sgt; }
- /* pages are no longer needed */ - kfree(pages); - pages = NULL; - /* * No need to sync to the device, this will happen later when the * prepare() memop is called. @@ -691,8 +553,9 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, }
buf->dma_addr = sg_dma_address(sgt->sgl); - buf->size = size; buf->dma_sgt = sgt; +out: + buf->size = size;
return buf;
@@ -701,25 +564,13 @@ fail_map_sg: buf->dma_dir, &attrs);
fail_sgt_init: - if (!vma_is_io(buf->vma)) - vb2_dc_sgt_foreach_page(sgt, put_page); sg_free_table(sgt);
fail_sgt: kfree(sgt);
-fail_get_user_pages: - if (pages && !vma_is_io(buf->vma)) - while (n_pages) - put_page(pages[--n_pages]); - - down_read(¤t->mm->mmap_sem); -fail_vma: - vb2_put_vma(buf->vma); - -fail_pages: - up_read(¤t->mm->mmap_sem); - kfree(pages); /* kfree is NULL-proof */ +fail_pfnvec: + vb2_destroy_pfnvec(pfns);
fail_buf: kfree(buf);
Conversion to the use of pinned pfns made some functions unused. Remove them. Also there's no need to lock mmap_sem in __buf_prepare() anymore.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/media/v4l2-core/videobuf2-memops.c | 114 ----------------------------- include/media/videobuf2-memops.h | 6 -- 2 files changed, 120 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-memops.c b/drivers/media/v4l2-core/videobuf2-memops.c index 80ade22b920c..08daaa5c4e2d 100644 --- a/drivers/media/v4l2-core/videobuf2-memops.c +++ b/drivers/media/v4l2-core/videobuf2-memops.c @@ -23,120 +23,6 @@ #include <media/videobuf2-memops.h>
/** - * vb2_get_vma() - acquire and lock the virtual memory area - * @vma: given virtual memory area - * - * This function attempts to acquire an area mapped in the userspace for - * the duration of a hardware operation. The area is "locked" by performing - * the same set of operation that are done when process calls fork() and - * memory areas are duplicated. - * - * Returns a copy of a virtual memory region on success or NULL. - */ -struct vm_area_struct *vb2_get_vma(struct vm_area_struct *vma) -{ - struct vm_area_struct *vma_copy; - - vma_copy = kmalloc(sizeof(*vma_copy), GFP_KERNEL); - if (vma_copy == NULL) - 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; -} -EXPORT_SYMBOL_GPL(vb2_get_vma); - -/** - * vb2_put_userptr() - release a userspace virtual memory area - * @vma: virtual memory region associated with the area to be released - * - * This function releases the previously acquired memory area after a hardware - * operation. - */ -void vb2_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); -} -EXPORT_SYMBOL_GPL(vb2_put_vma); - -/** - * vb2_get_contig_userptr() - lock physically contiguous userspace mapped memory - * @vaddr: starting virtual address of the area to be verified - * @size: size of the area - * @res_paddr: will return physical address for the given vaddr - * @res_vma: will return locked copy of struct vm_area for the given area - * - * This function will go through memory area of size @size mapped at @vaddr and - * verify that the underlying physical pages are contiguous. If they are - * contiguous the virtual memory area is locked and a @res_vma is filled with - * the copy and @res_pa set to the physical address of the buffer. - * - * Returns 0 on success. - */ -int vb2_get_contig_userptr(unsigned long vaddr, unsigned long size, - struct vm_area_struct **res_vma, dma_addr_t *res_pa) -{ - struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; - unsigned long offset, start, end; - unsigned long this_pfn, prev_pfn; - dma_addr_t pa = 0; - - start = vaddr; - offset = start & ~PAGE_MASK; - end = start + size; - - vma = find_vma(mm, start); - - if (vma == NULL || vma->vm_end < end) - return -EFAULT; - - 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; - - prev_pfn = this_pfn; - } - - /* - * Memory is contigous, lock vma and return to the caller - */ - *res_vma = vb2_get_vma(vma); - if (*res_vma == NULL) - return -ENOMEM; - - *res_pa = pa + offset; - return 0; -} -EXPORT_SYMBOL_GPL(vb2_get_contig_userptr); - -/** * vb2_create_pfnvec() - map virtual addresses to pfns * @start: Virtual user address where we start mapping * @length: Length of a range to map diff --git a/include/media/videobuf2-memops.h b/include/media/videobuf2-memops.h index 868f9c1cd92d..01b4325947f1 100644 --- a/include/media/videobuf2-memops.h +++ b/include/media/videobuf2-memops.h @@ -31,12 +31,6 @@ struct vb2_vmarea_handler {
extern const struct vm_operations_struct vb2_common_vm_ops;
-int vb2_get_contig_userptr(unsigned long vaddr, unsigned long size, - struct vm_area_struct **res_vma, dma_addr_t *res_pa); - -struct vm_area_struct *vb2_get_vma(struct vm_area_struct *vma); -void vb2_put_vma(struct vm_area_struct *vma); - struct pinned_pfns *vb2_create_pfnvec(unsigned long start, unsigned long length, bool write); void vb2_destroy_pfnvec(struct pinned_pfns *pfns);
Convert g2d_userptr_get_dma_addr() to pin pages using get_vaddr_pfn(). This removes the knowledge about vmas and mmap_sem locking from exynos driver. Also it fixes a problem that the function has been mapping user provided address without holding mmap_sem.
Signed-off-by: Jan Kara jack@suse.cz --- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 91 ++++++++++--------------------- drivers/gpu/drm/exynos/exynos_drm_gem.c | 97 --------------------------------- 2 files changed, 30 insertions(+), 158 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c index 81a250830808..8949354a85a1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c @@ -190,10 +190,8 @@ struct g2d_cmdlist_userptr { dma_addr_t dma_addr; unsigned long userptr; unsigned long size; - struct page **pages; - unsigned int npages; + struct pinned_pfns *pfns; struct sg_table *sgt; - struct vm_area_struct *vma; atomic_t refcount; bool in_pool; bool out_of_list; @@ -363,6 +361,7 @@ static void g2d_userptr_put_dma_addr(struct drm_device *drm_dev, { struct g2d_cmdlist_userptr *g2d_userptr = (struct g2d_cmdlist_userptr *)obj; + struct page **pages;
if (!obj) return; @@ -382,19 +381,21 @@ out: exynos_gem_unmap_sgt_from_dma(drm_dev, g2d_userptr->sgt, DMA_BIDIRECTIONAL);
- exynos_gem_put_pages_to_userptr(g2d_userptr->pages, - g2d_userptr->npages, - g2d_userptr->vma); + pages = pfns_vector_pages(g2d_userptr->pfns); + if (pages) { + int i;
- exynos_gem_put_vma(g2d_userptr->vma); + for (i = 0; i < pfns_vector_count(g2d_userptr->pfns); i++) + set_page_dirty_lock(pages[i]); + } + put_vaddr_pfns(g2d_userptr->pfns); + pfns_vector_destroy(g2d_userptr->pfns);
if (!g2d_userptr->out_of_list) list_del_init(&g2d_userptr->list);
sg_free_table(g2d_userptr->sgt); kfree(g2d_userptr->sgt); - - drm_free_large(g2d_userptr->pages); kfree(g2d_userptr); }
@@ -413,6 +414,7 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct drm_device *drm_dev, struct vm_area_struct *vma; unsigned long start, end; unsigned int npages, offset; + struct pinned_pfns *pfns; int ret;
if (!size) { @@ -456,65 +458,37 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct drm_device *drm_dev, return ERR_PTR(-ENOMEM);
atomic_set(&g2d_userptr->refcount, 1); + g2d_userptr->size = size;
start = userptr & PAGE_MASK; offset = userptr & ~PAGE_MASK; end = PAGE_ALIGN(userptr + size); npages = (end - start) >> PAGE_SHIFT; - g2d_userptr->npages = npages; + pfns = g2d_userptr->pfns = pfns_vector_create(npages); + if (!pfns) + goto out_free;
- pages = drm_calloc_large(npages, sizeof(struct page *)); - if (!pages) { - DRM_ERROR("failed to allocate pages.\n"); - ret = -ENOMEM; - goto err_free; - } - - down_read(¤t->mm->mmap_sem); - vma = find_vma(current->mm, userptr); - if (!vma) { - up_read(¤t->mm->mmap_sem); - DRM_ERROR("failed to get vm region.\n"); + ret = get_vaddr_pfn(start, npages, 1, 1, pfns); + if (ret != npages) { + DRM_ERROR("failed to get user pages from userptr.\n"); + if (ret < 0) + goto err_destroy_pfns; ret = -EFAULT; - goto err_free_pages; + goto err_put_pfns; } - - if (vma->vm_end < userptr + size) { - up_read(¤t->mm->mmap_sem); - DRM_ERROR("vma is too small.\n"); + if (pfns_vector_to_pages(pfns) < 0) { ret = -EFAULT; - goto err_free_pages; + goto err_put_pfns; }
- g2d_userptr->vma = exynos_gem_get_vma(vma); - if (!g2d_userptr->vma) { - up_read(¤t->mm->mmap_sem); - DRM_ERROR("failed to copy vma.\n"); - ret = -ENOMEM; - goto err_free_pages; - } - - g2d_userptr->size = size; - - ret = exynos_gem_get_pages_from_userptr(start & PAGE_MASK, - npages, pages, vma); - if (ret < 0) { - up_read(¤t->mm->mmap_sem); - DRM_ERROR("failed to get user pages from userptr.\n"); - goto err_put_vma; - } - - up_read(¤t->mm->mmap_sem); - g2d_userptr->pages = pages; - sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); if (!sgt) { ret = -ENOMEM; - goto err_free_userptr; + goto err_put_pfns; }
- ret = sg_alloc_table_from_pages(sgt, pages, npages, offset, - size, GFP_KERNEL); + ret = sg_alloc_table_from_pages(sgt, pfns_vector_pages(pfns), npages, + offset, size, GFP_KERNEL); if (ret < 0) { DRM_ERROR("failed to get sgt from pages.\n"); goto err_free_sgt; @@ -549,16 +523,11 @@ err_sg_free_table: err_free_sgt: kfree(sgt);
-err_free_userptr: - exynos_gem_put_pages_to_userptr(g2d_userptr->pages, - g2d_userptr->npages, - g2d_userptr->vma); - -err_put_vma: - exynos_gem_put_vma(g2d_userptr->vma); +err_put_pfns: + put_vaddr_pfns(pfns);
-err_free_pages: - drm_free_large(pages); +err_destroy_pfns: + pfns_vector_destroy(pfns);
err_free: kfree(g2d_userptr); diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 0d5b9698d384..47068ae44ced 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -378,103 +378,6 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, return 0; }
-struct vm_area_struct *exynos_gem_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; -} - -void exynos_gem_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); -} - -int exynos_gem_get_pages_from_userptr(unsigned long start, - unsigned int npages, - struct page **pages, - struct vm_area_struct *vma) -{ - int get_npages; - - /* the memory region mmaped with VM_PFNMAP. */ - if (vma_is_io(vma)) { - unsigned int i; - - for (i = 0; i < npages; ++i, start += PAGE_SIZE) { - unsigned long pfn; - int ret = follow_pfn(vma, start, &pfn); - if (ret) - return ret; - - pages[i] = pfn_to_page(pfn); - } - - if (i != npages) { - DRM_ERROR("failed to get user_pages.\n"); - return -EINVAL; - } - - return 0; - } - - get_npages = get_user_pages(current, current->mm, start, - npages, 1, 1, pages, NULL); - get_npages = max(get_npages, 0); - if (get_npages != npages) { - DRM_ERROR("failed to get user_pages.\n"); - while (get_npages) - put_page(pages[--get_npages]); - return -EFAULT; - } - - return 0; -} - -void exynos_gem_put_pages_to_userptr(struct page **pages, - unsigned int npages, - struct vm_area_struct *vma) -{ - if (!vma_is_io(vma)) { - unsigned int i; - - for (i = 0; i < npages; i++) { - set_page_dirty_lock(pages[i]); - - /* - * undo the reference we took when populating - * the table. - */ - put_page(pages[i]); - } - } -} - int exynos_gem_map_sgt_with_dma(struct drm_device *drm_dev, struct sg_table *sgt, enum dma_data_direction dir)
Hello,
On Tue 17-03-15 12:56:30, Jan Kara wrote:
After a long pause I'm sending second version of my patch series to abstract vma handling from the various media drivers. After this patch set drivers have to know much less details about vmas, their types, and locking. My motivation for the series is that I want to change get_user_pages() locking and I want to handle subtle locking details in as few places as possible.
The core of the series is the new helper get_vaddr_pfns() which is given a virtual address and it fills in PFNs into provided array. If PFNs correspond to normal pages it also grabs references to these pages. The difference from get_user_pages() is that this function can also deal with pfnmap, mixed, and io mappings which is what the media drivers need.
I have tested the patches with vivid driver so at least vb2 code got some exposure. Conversion of other drivers was just compile-tested so I'd like to ask respective maintainers if they could have a look. Also I'd like to ask mm folks to check patch 2/9 implementing the helper. Thanks!
Ping? Any reactions?
Honza
On 04/02/2015 05:02 PM, Jan Kara wrote:
Hello,
On Tue 17-03-15 12:56:30, Jan Kara wrote:
After a long pause I'm sending second version of my patch series to abstract vma handling from the various media drivers. After this patch set drivers have to know much less details about vmas, their types, and locking. My motivation for the series is that I want to change get_user_pages() locking and I want to handle subtle locking details in as few places as possible.
The core of the series is the new helper get_vaddr_pfns() which is given a virtual address and it fills in PFNs into provided array. If PFNs correspond to normal pages it also grabs references to these pages. The difference from get_user_pages() is that this function can also deal with pfnmap, mixed, and io mappings which is what the media drivers need.
I have tested the patches with vivid driver so at least vb2 code got some exposure. Conversion of other drivers was just compile-tested so I'd like to ask respective maintainers if they could have a look. Also I'd like to ask mm folks to check patch 2/9 implementing the helper. Thanks!
Ping? Any reactions?
For patch 1/9:
Acked-by: Hans Verkuil hans.verkuil@cisco.com
For the other patches I do not feel qualified to give Acks. I've Cc-ed Pawel and Marek who have a better understanding of the mm internals than I do. Hopefully they can review the code.
It definitely looks like a good idea, and if nobody else will comment on the vb2 patches in the next 2 weeks, then I'll try to review it myself (for whatever that's worth).
Regards,
Hans
Dear All,
On 2015-04-02 17:25, Hans Verkuil wrote:
On 04/02/2015 05:02 PM, Jan Kara wrote:
Hello,
On Tue 17-03-15 12:56:30, Jan Kara wrote:
After a long pause I'm sending second version of my patch series to abstract vma handling from the various media drivers. After this patch set drivers have to know much less details about vmas, their types, and locking. My motivation for the series is that I want to change get_user_pages() locking and I want to handle subtle locking details in as few places as possible.
The core of the series is the new helper get_vaddr_pfns() which is given a virtual address and it fills in PFNs into provided array. If PFNs correspond to normal pages it also grabs references to these pages. The difference from get_user_pages() is that this function can also deal with pfnmap, mixed, and io mappings which is what the media drivers need.
I have tested the patches with vivid driver so at least vb2 code got some exposure. Conversion of other drivers was just compile-tested so I'd like to ask respective maintainers if they could have a look. Also I'd like to ask mm folks to check patch 2/9 implementing the helper. Thanks!
Ping? Any reactions?
For patch 1/9:
Acked-by: Hans Verkuil hans.verkuil@cisco.com
For the other patches I do not feel qualified to give Acks. I've Cc-ed Pawel and Marek who have a better understanding of the mm internals than I do. Hopefully they can review the code.
It definitely looks like a good idea, and if nobody else will comment on the vb2 patches in the next 2 weeks, then I'll try to review it myself (for whatever that's worth).
I'm really sorry that I didn't manage to find time to review this patchset. I really like the idea of moving pfn lookup from videobuf2/driver to some common code in mm and it is really great that someone managed to provide nice generic code for it.
I've applied the whole patchset onto v4.0 and tested it on Odroid U3 (with some additional patches). VideoBuf2-dc works still fine with USERPTR gathered from other's device mmaped buffer. You can add my:
Acked-by: Marek Szyprowski m.szyprowski@samsung.com Tested-by: Marek Szyprowski m.szyprowski@samsung.com
for the patches 1-8. Patch 9/9 doesn't apply anymore, so I've skipped it. Patch 2 needs a small fixup - you need to add '#include <linux/vmalloc.h>', because otherwise it doesn't compile. There have been also a minor conflict to be resolved in patch 7.
Best regards
Hi Marek,
On 04/24/2015 12:59 PM, Marek Szyprowski wrote:
Dear All,
On 2015-04-02 17:25, Hans Verkuil wrote:
On 04/02/2015 05:02 PM, Jan Kara wrote:
Hello,
On Tue 17-03-15 12:56:30, Jan Kara wrote:
After a long pause I'm sending second version of my patch series to abstract vma handling from the various media drivers. After this patch set drivers have to know much less details about vmas, their types, and locking. My motivation for the series is that I want to change get_user_pages() locking and I want to handle subtle locking details in as few places as possible.
The core of the series is the new helper get_vaddr_pfns() which is given a virtual address and it fills in PFNs into provided array. If PFNs correspond to normal pages it also grabs references to these pages. The difference from get_user_pages() is that this function can also deal with pfnmap, mixed, and io mappings which is what the media drivers need.
I have tested the patches with vivid driver so at least vb2 code got some exposure. Conversion of other drivers was just compile-tested so I'd like to ask respective maintainers if they could have a look. Also I'd like to ask mm folks to check patch 2/9 implementing the helper. Thanks!
Ping? Any reactions?
For patch 1/9:
Acked-by: Hans Verkuil hans.verkuil@cisco.com
For the other patches I do not feel qualified to give Acks. I've Cc-ed Pawel and Marek who have a better understanding of the mm internals than I do. Hopefully they can review the code.
It definitely looks like a good idea, and if nobody else will comment on the vb2 patches in the next 2 weeks, then I'll try to review it myself (for whatever that's worth).
I'm really sorry that I didn't manage to find time to review this patchset. I really like the idea of moving pfn lookup from videobuf2/driver to some common code in mm and it is really great that someone managed to provide nice generic code for it.
I've applied the whole patchset onto v4.0 and tested it on Odroid U3 (with some additional patches). VideoBuf2-dc works still fine with USERPTR gathered from other's device mmaped buffer. You can add my:
Acked-by: Marek Szyprowski m.szyprowski@samsung.com Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks!
for the patches 1-8. Patch 9/9 doesn't apply anymore, so I've skipped it. Patch 2 needs a small fixup - you need to add '#include <linux/vmalloc.h>', because otherwise it doesn't compile. There have been also a minor conflict to be resolved in patch 7.
I've just added patch 1/9 to my pull request for 4.2. But for patch 2/9 I need Acks from the mm maintainers. I think it makes sense if patches 2-8 all go in together via the linux-media tree. Jan, can you reach out to the right devs to get Acks?
Regards,
Hans
On Fri 24-04-15 13:07:37, Hans Verkuil wrote:
Hi Marek,
On 04/24/2015 12:59 PM, Marek Szyprowski wrote:
Dear All,
On 2015-04-02 17:25, Hans Verkuil wrote:
On 04/02/2015 05:02 PM, Jan Kara wrote:
Hello,
On Tue 17-03-15 12:56:30, Jan Kara wrote:
After a long pause I'm sending second version of my patch series to abstract vma handling from the various media drivers. After this patch set drivers have to know much less details about vmas, their types, and locking. My motivation for the series is that I want to change get_user_pages() locking and I want to handle subtle locking details in as few places as possible.
The core of the series is the new helper get_vaddr_pfns() which is given a virtual address and it fills in PFNs into provided array. If PFNs correspond to normal pages it also grabs references to these pages. The difference from get_user_pages() is that this function can also deal with pfnmap, mixed, and io mappings which is what the media drivers need.
I have tested the patches with vivid driver so at least vb2 code got some exposure. Conversion of other drivers was just compile-tested so I'd like to ask respective maintainers if they could have a look. Also I'd like to ask mm folks to check patch 2/9 implementing the helper. Thanks!
Ping? Any reactions?
For patch 1/9:
Acked-by: Hans Verkuil hans.verkuil@cisco.com
For the other patches I do not feel qualified to give Acks. I've Cc-ed Pawel and Marek who have a better understanding of the mm internals than I do. Hopefully they can review the code.
It definitely looks like a good idea, and if nobody else will comment on the vb2 patches in the next 2 weeks, then I'll try to review it myself (for whatever that's worth).
I'm really sorry that I didn't manage to find time to review this patchset. I really like the idea of moving pfn lookup from videobuf2/driver to some common code in mm and it is really great that someone managed to provide nice generic code for it.
I've applied the whole patchset onto v4.0 and tested it on Odroid U3 (with some additional patches). VideoBuf2-dc works still fine with USERPTR gathered from other's device mmaped buffer. You can add my:
Acked-by: Marek Szyprowski m.szyprowski@samsung.com Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks!
Thank you both for having a look at the patches!
for the patches 1-8. Patch 9/9 doesn't apply anymore, so I've skipped it. Patch 2 needs a small fixup - you need to add '#include <linux/vmalloc.h>', because otherwise it doesn't compile. There have been also a minor conflict to be resolved in patch 7.
I've just added patch 1/9 to my pull request for 4.2. But for patch 2/9 I need Acks from the mm maintainers. I think it makes sense if patches 2-8 all go in together via the linux-media tree. Jan, can you reach out to the right devs to get Acks?
Sure, I'll ping some mm guys explicitely.
Honza
dri-devel@lists.freedesktop.org
-
Hans Verkuil -
Jan Kara -
Marek Szyprowski -
Mel Gorman