On 01/13/2014 02:02 PM, Marek Szyprowski wrote:
> Hello,
>
> On 2014-01-13 10:54, Hans Verkuil wrote:
>> Hi Marek, Ricardo,
>>
>> On 01/08/2014 03:07 PM, Marek Szyprowski wrote:
>>> Hello All,
>>>
>>> On 2014-01-03 16:51, Ricardo Ribalda Delgado wrote:
>>>> Hello Hans
>>>>
>>>> What if we move the dma_map_sg and dma_unmap_sg to the vb2 interface,
>>>> and there do something like:
>>>>
>>>> n_sg= dma_map_sg()
>>>> if (n_sg=-ENOMEM){
>>>> split_table() //Breaks down the sg_table into monopages sg
>>>> n_sg= dma_map_sg()
>>
>> This is not a good approach. Remember that if swiotbl needs to allocate
>> e.g. 17 contiguous pages it will round up to the next power of two, so it
>> allocates 32 pages. So even if dma_map_sg succeeds, it might waste a lot
>> of memory.
>>
>>>> }
>>>> if (n_sg=-ENOMEM)
>>>> return -ENOMEM
>>>
>>> dma_map_sg/dma_unmap_sg should be moved to vb2-dma-sg memory allocator.
>>> The best place for calling them is buf_prepare() and buf_finish()
>>> callbacks. I think that I've already pointed this some time ago, but
>>> unfortunately I didn't find enough time to convert existing code.
>>
>> That would be nice, but this is a separate issue.
>>
>>> For solving the problem described by Hans, I think that vb2-dma-sg
>>> memory allocator should check dma mask of the client device and add
>>> appropriate GFP_DMA or GFP_DMA32 flags to alloc_pages(). This should fix
>>> the issues with failed dma_map_sg due to lack of bouncing buffers.
>>
>> Those GFP flags are for the scatterlist itself, and that can be placed
>> anywhere in memory (frankly, I'm not sure why sg_alloc_table_from_pages
>> has a gfp_flags argument at all and I think it is used incorrectly in
>> videobuf2-dma-sg.c as well).
>
> I was talking about GFP flags passed to alloc_pages in vb2_dma_sg allocator,
> not the sg_alloc_table_from_pages().
>
> IMHO the following changes should fix your problem:
>
> 1. add client struct device pointer to vb2_dma_sg_desc, so vb2_dma_sg
> allocator will be able to check dma parameters of the client device.
>
> 2. add following check to vb2_dma_sg_alloc_compacted():
>
> if (dev->dma_mask) {
> if (dev->dma_mask < DMA_BIT_MASK(32))
> gfp_mask |= GFP_DMA;
> else if (dev->dev_mask == DMA_BIT_MASK(32)
> gfp_mask |= GFP_DMA32;
> }
No, it doesn't. This concerns the USERPTR memory model, so the memory for
the buffer is allocated by userspace, not by the kernel. The MMAP memory
model works fine, that's not where the problem is.
>
>
>> I see two options. The first is the patch I included below: this adds a
>> bool to sg_alloc_table_from_pages() that tells it whether or not page
>> combining should be enabled. It also adds the vb2 queue's gfp_flags as
>> an argument to the get_userptr operation. In videobuf2-dma-sg.c that is
>> checked to see whether or not sg_alloc_table_from_pages() should enable
>> page-combining.
>>
>> The alternative would be to have vb2_queue_init check if the use of
>> V4L2_MEMORY_USERPTR would lead to dma bouncing based on the q->io_modes
>> and q->gfp_flags and if so, remove USERPTR support from io_modes. Do
>> we really want to have page bouncing for video capture?
>
> So the main problem is about user ptr modes? This once again shows that
> the current user pointer API is too limited and should be replaced by
> something more reliable.
The userptr model worked perfectly fine before the memory compaction was
added. This is a pure regression.
Regards,
Hans
--
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
