| Message ID | 1383767329-29985-1-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Mathias Memory managing is definately not my topic. I have done the same as in vb2-dmacontig, and it has worked on my driver (out of tree). I think that if there is something wrong it will also be wrong on the dmacontig part, and much more drivers would be affected, so please also take a look to videobuf2-dma-contig.c and check if there is something wrong there. Best Regards! On Mon, Nov 11, 2013 at 12:36 PM, Matthias Wächter <matthias.waechter@tttech.com> wrote: >> @@ -180,7 +186,26 @@ static void *vb2_dma_sg_get_userptr(void >> *alloc_ctx, unsigned long vaddr, >> if (!buf->pages) >> return NULL; >> >> - num_pages_from_user = get_user_pages(current, current->mm, >> + buf->vma = find_vma(current->mm, vaddr); >> + if (!buf->vma) { >> + dprintk(1, "no vma for address %lu\n", vaddr); >> + return NULL; >> + } >> + >> + 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(buf->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, >> write, > > Can you safely assume that your userptr will cover only one vma? At least, get_user_pages (calling __get_user_pages) does not assume that and calls find_vma() whenever vma->vm_end is reached. > > – Matthias > > CONFIDENTIALITY: The contents of this e-mail are confidential and intended only for the above addressee(s). If you are not the intended recipient, or the person responsible for delivering it to the intended recipient, copying or delivering it to anyone else or using it in any unauthorized manner is prohibited and may be unlawful. If you receive this e-mail by mistake, please notify the sender and the systems administrator at straymail@tttech.com immediately.
Hello, On 2013-11-11 12:36, Matthias Wächter wrote: > > @@ -180,7 +186,26 @@ static void *vb2_dma_sg_get_userptr(void > > *alloc_ctx, unsigned long vaddr, > > if (!buf->pages) > > return NULL; > > > > - num_pages_from_user = get_user_pages(current, current->mm, > > + buf->vma = find_vma(current->mm, vaddr); > > + if (!buf->vma) { > > + dprintk(1, "no vma for address %lu\n", vaddr); > > + return NULL; > > + } > > + > > + 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(buf->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, > > write, > > Can you safely assume that your userptr will cover only one vma? At least, get_user_pages (calling __get_user_pages) does not assume that and calls find_vma() whenever vma->vm_end is reached. We care only about io mappings which cover only one vma. Such mappings are created by other device drivers and can be used mainly for zero-copy buffer sharing between multimedia devices. Although it is technically possible to provide code for multiple vma, there will be no real use case for it. Best regards
Hello Marek Could you review the patch? Is there something that needs to be fixed? Thanks! On Mon, Nov 25, 2013 at 4:41 PM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > > On 2013-11-11 12:36, Matthias Wächter wrote: >> >> > @@ -180,7 +186,26 @@ static void *vb2_dma_sg_get_userptr(void >> > *alloc_ctx, unsigned long vaddr, >> > if (!buf->pages) >> > return NULL; >> > >> > - num_pages_from_user = get_user_pages(current, current->mm, >> > + buf->vma = find_vma(current->mm, vaddr); >> > + if (!buf->vma) { >> > + dprintk(1, "no vma for address %lu\n", vaddr); >> > + return NULL; >> > + } >> > + >> > + 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(buf->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, >> > write, >> >> Can you safely assume that your userptr will cover only one vma? At least, >> get_user_pages (calling __get_user_pages) does not assume that and calls >> find_vma() whenever vma->vm_end is reached. > > > We care only about io mappings which cover only one vma. Such mappings > are created by other device drivers and can be used mainly for > zero-copy buffer sharing between multimedia devices. Although it is > technically possible to provide code for multiple vma, there will be > no real use case for it. > > Best regards > -- > Marek Szyprowski > Samsung R&D Institute Poland >
Hello, On 2013-11-06 20:48, Ricardo Ribalda Delgado wrote: > Memory exported via remap_pfn_range cannot be remapped via > get_user_pages. > > Other videobuf2 methods (like the dma-contig) supports io memory. > > This patch adds support for this kind of memory. > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > drivers/media/v4l2-core/videobuf2-dma-sg.c | 35 ++++++++++++++++++++++++++---- > 1 file changed, 31 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c > index 2f86054..44ddfe1 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c > @@ -40,6 +40,7 @@ struct vb2_dma_sg_buf { > unsigned int num_pages; > atomic_t refcount; > struct vb2_vmarea_handler handler; > + struct vm_area_struct *vma; > }; > > static void vb2_dma_sg_put(void *buf_priv); > @@ -155,6 +156,11 @@ static void vb2_dma_sg_put(void *buf_priv) > } > } > > +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, int write) > { > @@ -180,7 +186,26 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, > if (!buf->pages) > return NULL; > > - num_pages_from_user = get_user_pages(current, current->mm, > + buf->vma = find_vma(current->mm, vaddr); > + if (!buf->vma) { > + dprintk(1, "no vma for address %lu\n", vaddr); > + return NULL; > + } > + > + 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(buf->vma, vaddr, &pfn)) { > + dprintk(1, "no page for address %lu\n", vaddr); > + break; > + } Please add a call to vb2_get_vma(vma) to protect that io memory from being freed, see videobuf2-dma-contig.c for the reference. It is not 100% perfect way, but right now it works for the typical memory regions created by other multimedia devices. You will also need to call vb2_put_vma() later on release. > + 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, > write, > @@ -201,8 +226,9 @@ userptr_fail_alloc_table_from_pages: > userptr_fail_get_user_pages: > dprintk(1, "get_user_pages requested/got: %d/%d]\n", > num_pages_from_user, buf->num_pages); > - while (--num_pages_from_user >= 0) > - put_page(buf->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]); > kfree(buf->pages); > kfree(buf); > return NULL; > @@ -225,7 +251,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv) > while (--i >= 0) { > if (buf->write) > set_page_dirty_lock(buf->pages[i]); > - put_page(buf->pages[i]); > + if (!vma_is_io(buf->vma)) > + put_page(buf->pages[i]); > } > kfree(buf->pages); > kfree(buf); Best regards
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c index 2f86054..44ddfe1 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c @@ -40,6 +40,7 @@ struct vb2_dma_sg_buf { unsigned int num_pages; atomic_t refcount; struct vb2_vmarea_handler handler; + struct vm_area_struct *vma; }; static void vb2_dma_sg_put(void *buf_priv); @@ -155,6 +156,11 @@ static void vb2_dma_sg_put(void *buf_priv) } } +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, int write) { @@ -180,7 +186,26 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr, if (!buf->pages) return NULL; - num_pages_from_user = get_user_pages(current, current->mm, + buf->vma = find_vma(current->mm, vaddr); + if (!buf->vma) { + dprintk(1, "no vma for address %lu\n", vaddr); + return NULL; + } + + 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(buf->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, write, @@ -201,8 +226,9 @@ userptr_fail_alloc_table_from_pages: userptr_fail_get_user_pages: dprintk(1, "get_user_pages requested/got: %d/%d]\n", num_pages_from_user, buf->num_pages); - while (--num_pages_from_user >= 0) - put_page(buf->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]); kfree(buf->pages); kfree(buf); return NULL; @@ -225,7 +251,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv) while (--i >= 0) { if (buf->write) set_page_dirty_lock(buf->pages[i]); - put_page(buf->pages[i]); + if (!vma_is_io(buf->vma)) + put_page(buf->pages[i]); } kfree(buf->pages); kfree(buf);
Memory exported via remap_pfn_range cannot be remapped via get_user_pages. Other videobuf2 methods (like the dma-contig) supports io memory. This patch adds support for this kind of memory. Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- drivers/media/v4l2-core/videobuf2-dma-sg.c | 35 ++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-)