[Linaro-mm-sig] [RFCv2 PATCH 2/9 - 4/4] v4l: vb2-dma-contig: update and code refactoring

[Tomasz Stanislawski]

Hi Laurent,

On 03/22/2012 03:42 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Thursday 22 March 2012 14:36:33 Tomasz Stanislawski wrote:
>> Hi Laurent,
>> Thank you very much for your comments and question.
>> They were very useful.
> 
> You're welcome.
> 
>> Please refer to the comments below.
>>
>> On 03/22/2012 11:50 AM, Laurent Pinchart wrote:
>>> On Thursday 22 March 2012 11:02:23 Laurent Pinchart wrote:
>>>> From: Tomasz Stanislawski <t.stanislaws at samsung.com>
>>>>
>>>> This patch combines updates and fixes to dma-contig allocator.
>>>> Moreover the allocator code was refactored.
>>>> The most important changes are:
>>>> - functions were reordered
>>>> - move compression of scatterlist to separete function
>>>> - add support for multichunk but contiguous scatterlists
>>>> - simplified implementation of vb2-dma-contig context structure
>>>> - let mmap method to use dma_mmap_writecombine
>>>> - add support for scatterlist in userptr mode
> 
> [snip]
> 
>>>> diff --git a/drivers/media/video/videobuf2-dma-contig.c
>>>> b/drivers/media/video/videobuf2-dma-contig.c index c898e6f..9965465
>>>> 100644
>>>> --- a/drivers/media/video/videobuf2-dma-contig.c
>>>> +++ b/drivers/media/video/videobuf2-dma-contig.c
> 
> [snip]
> 
>>>>  static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
>>>>  {
>>>>  
>>>>  	struct device *dev = alloc_ctx;
>>>>  	struct vb2_dc_buf *buf;
>>>>
>>>> +	int ret;
>>>> +	int
>>>> n_pages;
>>>>
>>>>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>>>  	if (!buf)
>>>>  	
>>>>  		return ERR_PTR(-ENOMEM);
>>>>
>>>> -	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, 
> GFP_KERNEL);
>>>> +	buf->dev = dev;
>>>> +	buf->size = size;
>>>> +	buf->vaddr = dma_alloc_coherent(buf->dev, buf->size, &buf->dma_addr,
>>>> +		GFP_KERNEL);
>>>> +
>>>> +	ret = -ENOMEM;
>>>>
>>>>  	if (!buf->vaddr) {
>>>>
>>>> -		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
>>>> -		kfree(buf);
>>>> -		return ERR_PTR(-ENOMEM);
>>>> +		dev_err(dev, "dma_alloc_coherent of size %ld failed\n",
>>>> +			size);
>>>> +		goto fail_buf;
>>>>
>>>>  	}
>>>>
>>>> -	buf->dev = dev;
>>>> -	buf->size = size;
>>>> +	WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
>>>> +	WARN_ON(buf->dma_addr & ~PAGE_MASK);
>>>> +
>>>> +	n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>>>> +
>>>> +	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
>>>> +	if (!pages) {
>>>> +		printk(KERN_ERR "failed to alloc page table\n");
>>>> +		goto fail_dma;
>>>> +	}
>>>> +
>>>> +	ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages);
>>>
>>> As the only purpose of this is to retrieve a list of pages that will be
>>> used to create a single-entry sgt, wouldn't it be possible to shortcut
>>> the code and get the physical address of the buffer directly ?
>>
>> The physical address should not be used since they are meaningless in a
>> context of different devices. It seams that only the list of pages is
>> more-or-less portable between different drivers.
> 
> The pages are physically contiguous. The physical address of the first page is 
> thus all you need.

No. DMA-CONTIG buffers do not have to be physically contiguous. Please refer below.

> 
> struct page and physical addresses can be used interchangeably in this case if 
> I'm not mistaken. If you want to go with pages, you could use the first page 
> only instead of the physical buffer address.
> 

Ok. There are bus addresses, physical addresses, DMA addresses and PFNs.
As I understand PFNs and 'struct page' can be interchanged, at least in one direction.
The PFNs are used to create a bus address, I mean addresses that are recognized by
a RAM chip. So a list of PFNs seams to be the most portable way of describing
the memory, isn't it?

>> The physical address is already present in buf->dma_addr, but it is only
>> valid if the device has no MMU. Notice that vb2-dma-contig possess no
>> knowledge if MMU is present for a given device.
> 
> That's why buf->dma_addr can't be considered as a physical address. It's only 
> useful in the device context.
> 

ok

>> The sg list is not going to be single-entry if the device is provided with
>> its own MMU.
> 
> There's something I don't get then. vb2-dma-contig deals with physically 
> contiguous buffers. The buffer is backed by physically contiguous pages, so 
> the sg list should have a single entry.
> 

As I understand dma-contig deal with DMA contiguous buffers, it means buffers
that are contiguous from device point of view. Therefore those buffers do NOT
have to be physically contiguous if the device has its own IOMMU.

>>>> +	if (ret < 0) {
>>>> +		printk(KERN_ERR "failed to get buffer pages from DMA API\n");
>>>> +		goto fail_pages;
>>>> +	}
>>>> +	if (ret != n_pages) {
>>>> +		ret = -EFAULT;
>>>> +		printk(KERN_ERR "failed to get all pages from DMA API\n");
>>>> +		goto fail_pages;
>>>> +	}
>>>> +
>>>> +	buf->sgt_base = vb2_dc_pages_to_sgt(pages, n_pages, 0, 0);
>>>> +	if (IS_ERR(buf->sgt_base)) {
>>>> +		ret = PTR_ERR(buf->sgt_base);
>>>> +		printk(KERN_ERR "failed to prepare sg table\n");
>>>> +		goto fail_pages;
>>>> +	}
>>>
>>> buf->sgt_base isn't used in this patch. I would move the buf->sgt_base
>>> creation code to the patch that uses it then, or to its own patch just
>>> before the patch that uses it.
>>
>> Good point. The sgt_base is used by exporter only. Thanks for noticing it.
>>
>>>> +
>>>> +	/* pages are no longer needed */
>>>> +	kfree(pages);
>>>>
>>>>  	buf->handler.refcount = &buf->refcount;
>>>>  	buf->handler.put = vb2_dc_put;
> 
> [snip]
> 
>>>>  /*********************************************/
>>>>  /*       callbacks for USERPTR buffers       */
>>>>  /*********************************************/
>>>>
>>>> +static inline int vma_is_io(struct vm_area_struct *vma)
>>>> +{
>>>> +	return !!(vma->vm_flags & (VM_IO | VM_PFNMAP));
>>>
>>> Isn't VM_PFNMAP enough ? Wouldn't it be possible (at least in theory) to
>>> get a discontinuous physical range with VM_IO ?
>>
>> Frankly, I found that that in get_user_pages flags are checked against
>> (VM_IO | VM_PFNMAP). Probably for noMMU (not no IOMMU) case it is possible
>> to get vma with VM_IO on and VM_PFNMAP off, isn't it?
>>
>> The problem is that this framework should work in both cases so this
>> check was added just in case :).
> 
> OK. We can leave it here and deal with problems if they arise :-)
> 
>>>> +}
>>>> +
>>>> +static int vb2_dc_get_pages(unsigned long start, struct page **pages,
>>>> +	int n_pages, struct vm_area_struct **copy_vma, int write)
>>>> +{
>>>> +	struct vm_area_struct *vma;
>>>> +	int n = 0; /* number of get pages */
>>>> +	int ret = -EFAULT;
>>>> +
>>>> +	/* entering critical section for mm access */
>>>> +	down_read(&current->mm->mmap_sem);
>>>
>>> This will generate AB-BA deadlock warnings if lockdep is enabled. This
>>> function is called with the queue lock held, and the mmap() handler which
>>> takes the queue lock is called with current->mm->mmap_sem held.
>>>
>>> This is a known issue with videobuf2, not specific to this patch. The
>>> warning is usually a false positive (which we still need to fix, as it
>>> worries users), but can become a real issue if an MMAP queue and a
>>> USERPTR queue are created by a driver with the same queue lock.
>>
>> Good point. Do you know any good solution to this problem?
> 
> http://patchwork.linuxtv.org/patch/8455/
> 
> It seems QBUF is safe, but PREPAREBUF isn't (both call __buf_prepare, which 
> end up calling the memops get_userptr operation).
> 
> I'll post a patch to fix it for PREPAREBUF. If I'm not mistaken, you can drop 
> the down_read/up_read here.
> 

ok. Thanks for the link.

>>>> +	vma = find_vma(current->mm, start);
>>>> +	if (!vma) {
>>>> +		printk(KERN_ERR "no vma for address %lu\n", start);
>>>> +		goto cleanup;
>>>> +	}
>>>> +
>>>> +	if (vma_is_io(vma)) {
>>>> +		unsigned long pfn;
>>>> +
>>>> +		if (vma->vm_end - start < n_pages * PAGE_SIZE) {
>>>> +			printk(KERN_ERR "vma is too small\n");
>>>> +			goto cleanup;
>>>> +		}
>>>> +
>>>> +		for (n = 0; n < n_pages; ++n, start += PAGE_SIZE) {
>>>> +			ret = follow_pfn(vma, start, &pfn);
>>>> +			if (ret) {
>>>> +				printk(KERN_ERR "no page for address %lu\n",
>>>> +					start);
>>>> +				goto cleanup;
>>>> +			}
>>>> +			pages[n] = pfn_to_page(pfn);
>>>> +			get_page(pages[n]);
>>>
>>> This worries me. When the VM_PFNMAP flag is set, the memory pages are not
>>> backed by a struct page. Creating a struct page pointer out of it can be
>>> an acceptable hack (for instance to store a page in an scatterlist with
>>> sg_set_page() and then retrieve its physical address with sg_phys()), but
>>> you should not expect the struct page to be valid for anything else.
>>> Calling get_page() on it will likely crash.
>>
>> You are completetly right. This is the corner case where list of pages is
>> not a portable way of describing the memory.
>> Maybe pfn_valid should be used to check validity of the page (pfn)
>> before getting it?
> 
> I think you should just drop the get_page() call. There's no page, so there's 
> no need to get a reference count to it.

The problem is that get_user_pages does call get_page. Not calling get_page
will break the symmetry between PFNMAP and non-PFNMAP buffers. Maybe checking
page validity before get_page/put_page is enough?

> 
> The VM_PFNMAP flag is mostly used with memory out of the kernel allocator's 
> control if I'm not mistaken. The main use case I've seen is memory reserved at 
> boot time and use as a frame buffer for instance. In that case the pages can't 
> go away, as there no page in the first place.
> 
> This won't fix the DMA SG problem though (see below).
> 
>>>> +		}
>>>> +	} else {
>>>> +		n = get_user_pages(current, current->mm, start & PAGE_MASK,
>>>> +			n_pages, write, 1, pages, NULL);
>>>> +		if (n != n_pages) {
>>>> +			printk(KERN_ERR "got only %d of %d user pages\n",
>>>> +				n, n_pages);
>>>> +			goto cleanup;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	*copy_vma = vb2_get_vma(vma);
>>>> +	if (!*copy_vma) {
>>>> +		printk(KERN_ERR "failed to copy vma\n");
>>>> +		ret = -ENOMEM;
>>>> +		goto cleanup;
>>>> +	}
>>>
>>> Do we really need to make a copy of the VMA ? The only reason why we store
>>> a pointer to it is to check the flags in vb2_dc_put_userptr(). We could
>>> store the flags instead and avoid vb2_get_dma()/vb2_put_dma() calls
>>> altogether.
>>
>> I remember that there was a very good reason of copying this vma structure.
>> You caught me on 'cargo-cult' programming.
>> I will do some reverse engineering and try to answer it soon.
> 
> OK :-) I'm not copying the VMA in the OMAP3 ISP driver, which is why this 
> caught my eyes. If you find the reason why copying it is needed, please add a 
> comment to the code.
>  

The reason of copying vma was that 'struct vma' has no reference counters.
Therefore it could be deleted after mm lock is freed, ending with freeing its all
pages belonging to vma. To prevent it, a copy of vma is created. Notice that
inside vb2_get_vma the callback open is called for original vma, preventing
memory from being released. On vb2_put_vma the complementary close is called.

>>>> +
>>>> +	/* leaving critical section for mm access */
>>>> +	up_read(&current->mm->mmap_sem);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +cleanup:
>>>> +	up_read(&current->mm->mmap_sem);
>>>> +
>>>> +	/* putting user pages if used, can be done wothout the lock */
>>>> +	while (n)
>>>> +		put_page(pages[--n]);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>>
>>>>  static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
>>>> -					unsigned long size, int write)
>>>> +	unsigned long size, int write)
>>>>  {
>>>>  
>>>>  	struct vb2_dc_buf *buf;
>>>>
>>>> -	struct vm_area_struct *vma;
>>>> -	dma_addr_t dma_addr = 0;
>>>> -	int ret;
>>>> +	unsigned long start, end, offset, offset2;
>>>> +	struct page **pages;
>>>> +	int n_pages;
>>>> +	int ret = 0;
>>>> +	struct sg_table *sgt;
>>>> +	unsigned long contig_size;
>>>>
>>>>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>>>  	if (!buf)
>>>>  	
>>>>  		return ERR_PTR(-ENOMEM);
>>>>
>>>> -	ret = vb2_get_contig_userptr(vaddr, size, &vma, &dma_addr);
>>>> +	buf->dev = alloc_ctx;
>>>> +	buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
>>>> +
>>>> +	start = (unsigned long)vaddr & PAGE_MASK;
>>>> +	offset = (unsigned long)vaddr & ~PAGE_MASK;
>>>> +	end = PAGE_ALIGN((unsigned long)vaddr + size);
>>>> +	offset2 = end - (unsigned long)vaddr - size;
>>>> +	n_pages = (end - start) >> PAGE_SHIFT;
>>>> +
>>>> +	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
>>>> +	if (!pages) {
>>>> +		ret = -ENOMEM;
>>>> +		printk(KERN_ERR "failed to allocate pages table\n");
>>>> +		goto fail_buf;
>>>> +	}
>>>> +
>>>> +	/* extract page list from userspace mapping */
>>>> +	ret = vb2_dc_get_pages(start, pages, n_pages, &buf->vma, write);
>>>>
>>>>  	if (ret) {
>>>>
>>>> -		printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n",
>>>> -				vaddr);
>>>> -		kfree(buf);
>>>> -		return ERR_PTR(ret);
>>>> +		printk(KERN_ERR "failed to get user pages\n");
>>>> +		goto fail_pages;
>>>> +	}
>>>> +
>>>> +	sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2);
>>>> +	if (!sgt) {
>>>> +		printk(KERN_ERR "failed to create scatterlist table\n");
>>>> +		ret = -ENOMEM;
>>>> +		goto fail_get_pages;
>>>>
>>>>  	}
>>>
>>> This looks overly complex to me. You create a multi-chunk sgt out of the
>>> user pointer address and map it completely, and then check if it starts
>>> with a big enough contiguous chunk.
>>
>> Notice that vb2_dc_pages_to_sgt does compress contiguous ranges of pfns
>> (pages). So if the memory is contiguous, then sigle chunk sglist is
>> produced. The memory used to store pages list is just temporary. It is
>> freed after the sglist is created.
> 
> That's exactly my point. The memory needs to be contiguous to be usable. If it 
> isn't, vb2-dma-contig will only use the first contiguous chunk. We could thus 
> simplify the code by hardcoding the single-chunk assumption. vb2-dma-contig 
> would walk user user pages list (or the PFN, depending on the VMA flags) and 
> stop at the first discontinuity. It would then create a single-entry sg list 
> and operate on that, without mapping or otherwise touching the rest of the 
> VMA, which is unusable to the device anyway.
> 

[see above]

>>> Why don't you create an sgt with a single continuous
>>> chunk then ? In the VM_PFNMAP case you could check whether the area is
>>> contiguous when you follow the PFNs, stop at the first discontinuity, and
>>> create an sgt with a single element right there. You would then need to
>>> call vb2_dc_pages_to_sgt() in the normal case only, and stop at the first
>>> discontinuity as well.
>>
>> Discontinuity of pfns is not a problem if a device has own IOMMU. It is not
>> known for vb2-dma-contig if mapping this multi-chunk sglist will succeed
>> until calling and checking a result of dma_map_sg.
> 
> If the device has an IOMMU it won't need contiguous memory. Shouldn't it then 
> use vb2-dma-sg instead ?
> 

No. The vb2-dma-sg is used for devices that can deal with memory accessed from
multiple chunks. For example a device with attached DMA engine that can
handle a scattergather list.

>> Why bothering if both VM_PFNMAP and non-VM_PFNMAP are handled in the same
>> way after list of pages is obtained? Trating them the same way allows to
>> reuse code and simplify the program flow.
>>
>> The DMA framework does not provide any way to force single chunk mapping in
>> sg. If the device is capable of mapping discontinous pages into a single
>> chunk the DMA framework will probably do merge the pages. The documentation
>> encourages to merge the list but it is not obligatory.
>>
>> The reason is that if 'struct scatterlist' contains no dma_length field,
>> what is controlled by CONFIG_NEED_SG_DMA_LENGTH macro, then field length
>> is used instead. Chunk marging cannot be done in such a case.
>> This is the reason why I look for the longest contiguous block.
>>
>>>> +	/* pages are no longer needed */
>>>> +	kfree(pages);
>>>> +	pages = NULL;
>>>> +
>>>> +	sgt->nents = dma_map_sg(buf->dev, sgt->sgl, sgt->orig_nents,
>>>> +		buf->dma_dir);
>>>> +	if (sgt->nents <= 0) {
>>>> +		printk(KERN_ERR "failed to map scatterlist\n");
>>>> +		ret = -EIO;
>>>> +		goto fail_sgt;
>>>> +	}
>>>> +
>>>> +	contig_size = vb2_dc_get_contiguous_size(sgt);
>>>> +	if (contig_size < size) {
>>>> +		printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n",
>>>> +			contig_size, size);
>>>> +		ret = -EFAULT;
>>>> +		goto fail_map_sg;
>>>> +	}
>>>> +
>>>> +	buf->dma_addr = sg_dma_address(sgt->sgl);
>>>>
>>>>  	buf->size = size;
>>>>
>>>> -	buf->dma_addr = dma_addr;
>>>> -	buf->vma = vma;
>>>> +	buf->dma_sgt = sgt;
>>>> +
>>>> +	atomic_inc(&buf->refcount);
>>>>
>>>>  	return buf;
>>>>
>>>> +
>>>> +fail_map_sg:
>>>> +	dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>>>
>>> I think this will break in the VM_PFNMAP case on non-coherent
>>> architectures. arm_dma_unmap_page() will call __dma_page_dev_to_cpu() in
>>> that case, which can dereference struct page. As explain above, the
>>> struct page isn't valid with VM_PFNMAP. I haven't check the dma_map_sg()
>>> and dma_sync_sg_*() calls, but changes are they might break as well.
>>
>> It will crash as long it is true that there is no struct page behind given
>> pfn. In practice, I found that VM_PFNMAP means that one cannot assume that
>> there is a 'struct page' behind PFNs of a given mapping. Thoses struct
>> pages really exists for all our drivers. Anyway, I agree that using those
>> pages is a hack.
> 
> They don't exist for the memory used as frame buffer on the OMAP3 (or at least 
> didn't exist in the N900 and N9, I haven't checked since). This could become 
> just a bad distant memory when drivers will use CMA.
> 

I see. That is why one should check page validity before calling get_pages.
Other way would be splitting logic between PFNMAP and non-PFNMAP
userspace mappings.

>> It could be avoided if vb2_dc_get_pages returned a list of PFNs. Anyway,
>> those PFNs have to be transformed to pages to create an sglist. Those
>> pointers might be accessed somewhere deep inside dma_map_sg internals.
>>
>> The quite good solution would be dropping support for VM_PFNMAP mappings
>> since they cannot be handled reliably.
> 
> We should either drop VM_PFNMAP support or fix the DMA SG mapping API to 
> support VM_PFNMAP-style memory. I would vote for the former, as that's way 
> simpler and we have no VM_PFNMAP use case right now.
> 

Good point. But this way we have new dependencies on DMA mapping framework.

>>>> +
>>>> +fail_sgt:
>>>> +	vb2_dc_put_sgtable(sgt, 0);
>>>> +
>>>> +fail_get_pages:
>>>> +	while (pages && n_pages)
>>>> +		put_page(pages[--n_pages]);
>>>> +	vb2_put_vma(buf->vma);
>>>> +
>>>> +fail_pages:
>>>> +	kfree(pages); /* kfree is NULL-proof */
>>>> +
>>>> +fail_buf:
>>>> +	kfree(buf);
>>>> +
>>>> +	return ERR_PTR(ret);
>>>>
>>>>  }
> 

Regards,
Tomasz Stanislawski