Hi Marek,
On Monday 29 November 2010 14:24:35 Marek Szyprowski wrote:
> On Monday, November 29, 2010 10:39 AM Laurent Pinchart wrote:
> > On Friday 19 November 2010 16:55:39 Marek Szyprowski wrote:
> > > From: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > >
> > > Add generic memory handling routines for userspace pointer handling,
> > > contiguous memory verification and mapping.
> > >
> > > Signed-off-by: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > > CC: Pawel Osciak <pawel@xxxxxxxxxx>
[snip]
> > > + if (ret) {
> > > + printk(KERN_ERR "Invalid userspace address\n");
> > > + goto done;
> > > + }
> > > +
> > > + *paddr = (curr_pfn << PAGE_SHIFT) + offset;
> > > +
> > > + for (i = 1; i < num_pages; ++i) {
> > > + prev_pfn = curr_pfn;
> > > + vaddr += PAGE_SIZE;
> > > +
> > > + ret = follow_pfn(vma, vaddr, &curr_pfn);
> > > + if (ret || curr_pfn != prev_pfn + 1) {
> > > + printk(KERN_ERR "Invalid userspace address\n");
> > > + ret = -EINVAL;
> >
> > I would use -EFAULT when curr_pfn != prev_pfn + 1.
>
> I don't think that this situation desires for a EFAULT error code. EINVAL
> is imho enough to let application know that this memory cannot be used for
> the userptr buffer.
-EINVAL is used quite a lot already to report many different error conditions.
-EFAULT would let application know that the issue is about the pointer address
as opposed to the buffer index for instance.
[snip]
> > > + * Returns 0 on success.
> > > + */
> > > +int vb2_mmap_pfn_range(struct vm_area_struct *vma, unsigned long
> > > paddr, + unsigned long size,
> > > + const struct vm_operations_struct *vm_ops,
> > > + void *priv)
> > > +{
> > > + int ret;
> > > +
> > > + size = min_t(unsigned long, vma->vm_end - vma->vm_start, size);
> > > +
> > > + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> >
> > Now we're getting to the dirty part :-) Do we always want to map the
> > memory uncached ?
>
> That's how it is handled by the videobuf1 and probably most of drivers that
> use physically contiguous memory. I don't want to change it right now.
> This will be a way too much for a single step. Once drivers will settle
> down on vb2 we may start thinking of adding support for CACHED vs.
> UNCACHED mappings.
OK.
> > I'm not too sure about what the use cases for this function are. It seems
> > to be used by the DMA coherent and CMA allocators. Are you sure they
> > will always use physically contiguous memory ?
>
> Yes, both of them are designed for physically contiguous memory. I just
> noticed that the videobuf1 used the dma-contig name which is a bit better
> for this case. ;)
OK.
> > > + ret = remap_pfn_range(vma, vma->vm_start, paddr >> PAGE_SHIFT,
> > > + size, vma->vm_page_prot);
> > > + if (ret) {
> > > + printk(KERN_ERR "Remapping memory failed, error: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + vma->vm_flags |= VM_DONTEXPAND | VM_RESERVED;
> > > + vma->vm_private_data = priv;
> > > + vma->vm_ops = vm_ops;
> > > +
> > > + vm_ops->open(vma);
> > > +
> > > + printk(KERN_DEBUG "%s: mapped paddr 0x%08lx at 0x%08lx, size %ld\n",
> > > + __func__, paddr, vma->vm_start, size);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * vb2_get_userptr() - acquire an area pointed to by userspace addres
> > > vaddr + * @vaddr: virtual userspace address to the given area
> > > + *
> > > + * This function attempts to acquire an area mapped in the userspace
> > > for + * the duration of a hardware operation.
> > > + *
> > > + * Returns a virtual memory region associated with the given vaddr on
> > > success + * or NULL.
> > > + */
> > > +struct vm_area_struct *vb2_get_userptr(unsigned long vaddr)
> > > +{
> > > + struct mm_struct *mm = current->mm;
> > > + struct vm_area_struct *vma;
> > > + struct vm_area_struct *vma_copy;
> > > +
> > > + vma_copy = kmalloc(sizeof(struct vm_area_struct), GFP_KERNEL);
> > > + if (vma_copy == NULL)
> > > + return NULL;
> > > +
> > > + down_read(&mm->mmap_sem);
> > > +
> > > + vma = find_vma(mm, vaddr);
> > > + if (!vma)
> > > + goto done;
> > > +
> > > + if (vma->vm_ops && vma->vm_ops->open)
> > > + vma->vm_ops->open(vma);
> > > +
> > > + if (vma->vm_file)
> > > + get_file(vma->vm_file);
> >
> > Could you explain what this is for ?
>
> This is called when you access physically contiguous memory from different
> device which has been earlier mmaped by the application. This way
> applications uses userptr mode with dmacontig. When doing this, you must
> ensure somehow that this memory won't be released/freed before you finish.
> This is achieved by calling open() callback and increasing the use count
> of the mmaped file. Same things happen when your application calls fork()
> and a new process gets access to your mmaped device.
Good point.
Don't you also need to pin the pages to memory with get_user_pages() ?
--
Regards,
Laurent Pinchart
--
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
