Hello,
On Tuesday, January 03, 2012 8:48 AM javier Martin wrote:
> On 2 January 2012 20:00, Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> > Hi Javier,
> >
> > Thanks for the patch.
> >
> > On Monday 02 January 2012 15:12:22 Javier Martin wrote:
> >> Some video devices need to use contiguous memory
> >> which is not backed by pages as it happens with
> >> vmalloc. This patch provides userptr handling for
> >> those devices.
> >
> > What's your main use case ? Capturing to the frame buffer ?
>
> My main use case is capturing to my mx2_emmaprp mem2mem driver which
> converts from YUV422 to YUV420 format in HW.
>
> >> Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> >> ---
> >> drivers/media/video/videobuf2-vmalloc.c | 66
> >> +++++++++++++++++++----------- 1 files changed, 42 insertions(+), 24
> >> deletions(-)
> >>
> >> diff --git a/drivers/media/video/videobuf2-vmalloc.c
> >> b/drivers/media/video/videobuf2-vmalloc.c index 03aa62f..5bc7cec 100644
> >> --- a/drivers/media/video/videobuf2-vmalloc.c
> >> +++ b/drivers/media/video/videobuf2-vmalloc.c
> >> @@ -15,6 +15,7 @@
> >> #include <linux/sched.h>
> >> #include <linux/slab.h>
> >> #include <linux/vmalloc.h>
> >> +#include <linux/io.h>
> >
> > Please keep headers sorted alphabetically.
>
> OK. I didn't know that was the rule.
>
> >> #include <media/videobuf2-core.h>
> >> #include <media/videobuf2-memops.h>
> >> @@ -71,6 +72,8 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx,
> >> unsigned long vaddr, struct vb2_vmalloc_buf *buf;
> >> unsigned long first, last;
> >> int n_pages, offset;
> >> + struct vm_area_struct *vma;
> >> + unsigned long int physp;
> >>
> >> buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> >> if (!buf)
> >> @@ -80,23 +83,34 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx,
> >> unsigned long vaddr, offset = vaddr & ~PAGE_MASK;
> >> buf->size = size;
> >>
> >> - 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, write, 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, PAGE_KERNEL);
> >> - if (!buf->vaddr)
> >> - goto fail_get_user_pages;
> >> + vma = find_vma(current->mm, vaddr);
> >> + if (vma && (vma->vm_flags & VM_IO) && (vma->vm_pgoff)) {
> >> + physp = (vma->vm_pgoff << PAGE_SHIFT) + (vaddr - vma->vm_start);
> >> + buf->vaddr = ioremap_nocache(physp, size);
> >> + if (!buf->vaddr)
> >> + goto fail_pages_array_alloc;
> >
> > What if the region spans multiple VMAs ? Shouldn't you at least verify that
> > the region is physically contiguous, and that all VMAs share the same flags ?
> > That's what the OMAP3 ISP driver does (in ispqueue.c). Maybe it's overkill
> > though.
> >
> > If you do that, the could might be cleaner if you split this function in two,
> > as in the OMAP3 ISP driver.
>
> Yes, I suspected this could probably be troublesome. I'll take a look
> at OMAP3 ISP and see what I can do.
You can grab most of the required code for proper vma locking and contiguous pfn
extraction from videobuf2-dma_contig.c (vb2_dma_contig_get_userptr) and
videobuf2-memops.c (vb2_get_contig_userptr function), although the latter still
has some incomplete assumptions (it works correctly only with platforms where
dma address equals physical address in system memory - we will fix this soon).
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
