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 <hverk...@xs4all.nl> 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.szyprow...@samsung.com>
>>> Reviewed-by: Andre Heider <a.hei...@gmail.com>
>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.riba...@gmail.com>
>>> ---
>>> 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
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
