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()
}
if (n_sg=-ENOMEM)
return -ENOMEM
Regards
On Fri, Jan 3, 2014 at 4:36 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 01/03/2014 04:17 PM, Ricardo Ribalda Delgado wrote:
>> Hello Hans
>>
>> Thank you very much for your mail.
>>
>> For what I understand sg_alloc_table_from_pages does not allocate any
>> page or bounce buffer, it just take a set of N pages and makes a
>> sg_table from it, on the process it finds out if page A and A+1are on
>> the same pfn and if it is true they will share the sg. So it is a
>> later function that produces the error. As I see it, before this
>> patch we were reimplementing sg_alloc_table_from_pages.
>>
>> Which function is returning -ENOMEM?
>
> That's dma_map_sg(), which uses the scatter list constructed by
> sg_alloc_table_from_pages(). For x86 that ends up in lib/swiotlb.c,
> swiotlb_map_sg_attrs().
>
> Regards,
>
> Hans
>
>>
>>
>> Regards!
>>
>>
>>
>> On Fri, Jan 3, 2014 at 3:52 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> Hi Ricardo,
>>>
>>> I've run into a problem that is caused by this patch:
>>>
>>> On 08/02/2013 04:20 PM, Ricardo Ribalda Delgado wrote:
>>>> Replace the private struct vb2_dma_sg_desc with the struct sg_table so
>>>> we can benefit from all the helping functions in lib/scatterlist.c for
>>>> things like allocating the sg or compacting the descriptor
>>>>
>>>> marvel-ccic and solo6x10 drivers, that uses this api has been updated
>>>>
>>>> Acked-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>>> Reviewed-by: Andre Heider <a.heider@xxxxxxxxx>
>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
>>>> ---
>>>> drivers/media/platform/marvell-ccic/mcam-core.c | 14 +--
>>>> drivers/media/v4l2-core/videobuf2-dma-sg.c | 103 ++++++++------------
>>>> drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 20 ++--
>>>> include/media/videobuf2-dma-sg.h | 10 +-
>>>> 4 files changed, 63 insertions(+), 84 deletions(-)
>>>>
>>>
>>> <snip>
>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>>> index 4999c48..2f86054 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>>
>>> <snip>
>>>
>>>> @@ -99,17 +98,11 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size, gfp_t gfp_fla
>>>> buf->vaddr = NULL;
>>>> buf->write = 0;
>>>> buf->offset = 0;
>>>> - buf->sg_desc.size = size;
>>>> + buf->size = size;
>>>> /* size is already page aligned */
>>>> - buf->sg_desc.num_pages = size >> PAGE_SHIFT;
>>>> -
>>>> - buf->sg_desc.sglist = vzalloc(buf->sg_desc.num_pages *
>>>> - sizeof(*buf->sg_desc.sglist));
>>>> - if (!buf->sg_desc.sglist)
>>>> - goto fail_sglist_alloc;
>>>> - sg_init_table(buf->sg_desc.sglist, buf->sg_desc.num_pages);
>>>> + buf->num_pages = size >> PAGE_SHIFT;
>>>>
>>>> - buf->pages = kzalloc(buf->sg_desc.num_pages * sizeof(struct page *),
>>>> + buf->pages = kzalloc(buf->num_pages * sizeof(struct page *),
>>>> GFP_KERNEL);
>>>> if (!buf->pages)
>>>> goto fail_pages_array_alloc;
>>>> @@ -118,6 +111,11 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size, gfp_t gfp_fla
>>>> if (ret)
>>>> goto fail_pages_alloc;
>>>>
>>>> + ret = sg_alloc_table_from_pages(&buf->sg_table, buf->pages,
>>>> + buf->num_pages, 0, size, gfp_flags);
>>>> + if (ret)
>>>> + goto fail_table_alloc;
>>>> +
>>>> buf->handler.refcount = &buf->refcount;
>>>> buf->handler.put = vb2_dma_sg_put;
>>>> buf->handler.arg = buf;
>>>
>>> The problem here is the switch from sg_init_table to sg_alloc_table_from_pages. If
>>> the PCI hardware only accepts 32-bit DMA transfers, but it is used on a 64-bit OS
>>> with > 4GB physical memory, then the kernel will allocate DMA bounce buffers for you.
>>>
>>> With sg_init_table that works fine since each page in the scatterlist maps to a
>>> bounce buffer that is also just one page, but with sg_alloc_table_from_pages the DMA
>>> bounce buffers can be multiple pages. This is in turn rounded up to the next power of
>>> 2 and allocated in the 32-bit address space. Unfortunately, due to memory fragmentation
>>> this very quickly fails with -ENOMEM.
>>>
>>> I discovered this while converting saa7134 to vb2. I think that when DMA bounce
>>> buffers are needed, then it should revert to sg_init_table.
>>>
>>> I don't know whether this bug also affects non-v4l drivers.
>>>
>>> For now at least I won't try to fix this myself as I have discovered that dma-sg
>>> doesn't work anyway for saa7134 due to a hardware limitation so I will switch to
>>> dma-contig for that driver.
>>>
>>> But at the very least I thought I should write this down so others know about this
>>> subtle problem and perhaps someone else wants to tackle this.
>>>
>>> I actually think that the solo driver is affected by this (I haven't tested it yet).
>>> And at some point we need to convert bttv and cx88 to vb2 as well, and I expect that
>>> they will hit the same problem.
>>>
>>> If someone knows a better solution than switching to sg_init_table if bounce buffers
>>> are needed, then let me know.
>>>
>>> Regards,
>>>
>>> Hans
>>
>>
>>
>
--
Ricardo Ribalda
--
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
