* [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory
@ 2013-11-06 19:48 Ricardo Ribalda Delgado
2013-11-11 11:36 ` Matthias Wächter
2013-11-26 11:20 ` Marek Szyprowski
0 siblings, 2 replies; 6+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-06 19:48 UTC (permalink / raw)
To: Pawel Osciak, Marek Szyprowski, Kyungmin Park,
Mauro Carvalho Chehab, open list:VIDEOBUF2 FRAMEWORK,
sylvester.nawrocki
Cc: Ricardo Ribalda Delgado
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;
+ }
+ 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);
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory
2013-11-06 19:48 [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory Ricardo Ribalda Delgado
@ 2013-11-11 11:36 ` Matthias Wächter
2013-11-25 8:59 ` Ricardo Ribalda Delgado
2013-11-25 15:41 ` Marek Szyprowski
2013-11-26 11:20 ` Marek Szyprowski
1 sibling, 2 replies; 6+ messages in thread
From: Matthias Wächter @ 2013-11-11 11:36 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Pawel Osciak, Marek Szyprowski,
Kyungmin Park, Mauro Carvalho Chehab,
open list:VIDEOBUF2 FRAMEWORK, sylvester.nawrocki
> @@ -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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory
2013-11-11 11:36 ` Matthias Wächter
@ 2013-11-25 8:59 ` Ricardo Ribalda Delgado
2013-11-25 15:41 ` Marek Szyprowski
1 sibling, 0 replies; 6+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-25 8:59 UTC (permalink / raw)
To: Matthias Wächter
Cc: Pawel Osciak, Marek Szyprowski, Kyungmin Park,
Mauro Carvalho Chehab, open list:VIDEOBUF2 FRAMEWORK,
sylvester.nawrocki
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.
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory
2013-11-11 11:36 ` Matthias Wächter
2013-11-25 8:59 ` Ricardo Ribalda Delgado
@ 2013-11-25 15:41 ` Marek Szyprowski
2013-11-25 16:22 ` Ricardo Ribalda Delgado
1 sibling, 1 reply; 6+ messages in thread
From: Marek Szyprowski @ 2013-11-25 15:41 UTC (permalink / raw)
To: Matthias Wächter, Ricardo Ribalda Delgado, Pawel Osciak,
Kyungmin Park, Mauro Carvalho Chehab,
open list:VIDEOBUF2 FRAMEWORK, sylvester.nawrocki
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory
2013-11-25 15:41 ` Marek Szyprowski
@ 2013-11-25 16:22 ` Ricardo Ribalda Delgado
0 siblings, 0 replies; 6+ messages in thread
From: Ricardo Ribalda Delgado @ 2013-11-25 16:22 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Matthias Wächter, Pawel Osciak, Kyungmin Park,
Mauro Carvalho Chehab, open list:VIDEOBUF2 FRAMEWORK,
sylvester.nawrocki
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
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory
2013-11-06 19:48 [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory Ricardo Ribalda Delgado
2013-11-11 11:36 ` Matthias Wächter
@ 2013-11-26 11:20 ` Marek Szyprowski
1 sibling, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2013-11-26 11:20 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Pawel Osciak, Mauro Carvalho Chehab,
open list:VIDEOBUF2 FRAMEWORK, sylvester.nawrocki
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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-26 11:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-06 19:48 [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory Ricardo Ribalda Delgado
2013-11-11 11:36 ` Matthias Wächter
2013-11-25 8:59 ` Ricardo Ribalda Delgado
2013-11-25 15:41 ` Marek Szyprowski
2013-11-25 16:22 ` Ricardo Ribalda Delgado
2013-11-26 11:20 ` Marek Szyprowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).