[PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[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

RE: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[Matthias Wächter]

> @@ -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.

Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[Ricardo Ribalda Delgado]

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

Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[Marek Szyprowski]

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

Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[Ricardo Ribalda Delgado]

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

Re: [PATCH] videobuf2-dma-sg: Support io userptr operations on io memory

[Marek Szyprowski]

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