diff mbox

videobuf2-dma-sg: Minimize the number of dma segments

Message ID CAPybu_226ZWwEW-1AyaccMthLtmdWOjRts116RqMEk91o12TrQ@mail.gmail.com (mailing list archive)
State Superseded
Headers show

Commit Message

Ricardo Ribalda Delgado July 17, 2013, 2:05 p.m. UTC
Hello again

I have made some experiments and have replaced alloc_pages_exact with
alloc_pages of order N. Unfortunatelly vm_insert_page and vm_map_ram
does not work as expected.

vm_insert_page, only insert the PAGE_SIZE bytes of the higher order
page, if I try to add the other pages manually, the function returns
-22 due to count=0.
vm_map_ram is designed to work with exactly order 0 pages.

it works fine if I manually split the page, but the header file
suggest that function should not be called by the drivers... (although
this is not a device driver).

Here you can see my experiment in case you want to replicate it.


---
 drivers/media/v4l2-core/videobuf2-dma-sg.c |   89 ++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 17 deletions(-)

  if (!buf)
@@ -69,14 +130,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx,
unsigned long size, gfp_t gfp_fla
  if (!buf->pages)
  goto fail_pages_array_alloc;

- for (i = 0; i < buf->sg_desc.num_pages; ++i) {
- buf->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO |
-   __GFP_NOWARN | gfp_flags);
- if (NULL == buf->pages[i])
- goto fail_pages_alloc;
- sg_set_page(&buf->sg_desc.sglist[i],
-    buf->pages[i], PAGE_SIZE, 0);
- }
+ ret = vb2_dma_sg_alloc_compacted(buf, gfp_flags);
+ if (ret)
+ goto fail_pages_alloc;

  buf->handler.refcount = &buf->refcount;
  buf->handler.put = vb2_dma_sg_put;
@@ -89,8 +145,6 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx,
unsigned long size, gfp_t gfp_fla
  return buf;

 fail_pages_alloc:
- while (--i >= 0)
- __free_page(buf->pages[i]);
  kfree(buf->pages);

 fail_pages_array_alloc:
@@ -111,9 +165,10 @@ static void vb2_dma_sg_put(void *buf_priv)
  buf->sg_desc.num_pages);
  if (buf->vaddr)
  vm_unmap_ram(buf->vaddr, buf->sg_desc.num_pages);
- vfree(buf->sg_desc.sglist);
  while (--i >= 0)
- __free_page(buf->pages[i]);
+ __free_pages(buf->pages[i], get_order(
+ sg_dma_len(&buf->sg_desc.sglist[i])));
+ vfree(buf->sg_desc.sglist);
  kfree(buf->pages);
  kfree(buf);
  }
@@ -248,14 +303,14 @@ static int vb2_dma_sg_mmap(void *buf_priv,
struct vm_area_struct *vma)
  do {
  int ret;

- ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
+ ret = vm_insert_page(vma, uaddr, buf->pages[i]);
  if (ret) {
  printk(KERN_ERR "Remapping memory, error: %d\n", ret);
  return ret;
  }
-
- uaddr += PAGE_SIZE;
- usize -= PAGE_SIZE;
+ uaddr += sg_dma_len(&buf->sg_desc.sglist[i]);
+ usize -= sg_dma_len(&buf->sg_desc.sglist[i]);
+ i++;
  } while (usize > 0);


--
1.7.10.4



On Wed, Jul 17, 2013 at 11:43 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Hi Marek
>
>  alloc_pages_exact returns pages of order 0, every single page is
> filled into buf->pages, that then is used by vb2_dma_sg_mmap(), that
> also expects order 0 pages (its loops increments in PAGE_SIZE). The
> code has been tested on real HW.
>
> Your concern is that that alloc_pages_exact splits the higher order pages?
>
> Do you want that videobuf2-dma-sg to have support for higher order pages?
>
>
> Best regards
>
> PS: sorry for the duplicated post, previous one contained html parts
> and was rejected by the list
>
> On Wed, Jul 17, 2013 at 10:27 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> Hello,
>>
>>
>> On 7/15/2013 11:34 AM, Ricardo Ribalda Delgado wrote:
>>>
>>> Most DMA engines have limitations regarding the number of DMA segments
>>> (sg-buffers) that they can handle. Videobuffers can easily spread through
>>> houndreds of pages.
>>>
>>> In the previous aproach, the pages were allocated individually, this
>>> could led to the creation houndreds of dma segments (sg-buffers) that
>>> could not be handled by some DMA engines.
>>>
>>> This patch tries to minimize the number of DMA segments by using
>>> alloc_pages_exact. In the worst case it will behave as before, but most
>>> of the times it will reduce the number fo dma segments
>>>
>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>
>>
>> I like the idea, but the code doesn't seem to be correct. buf->pages array
>> is
>> needed for vb2_dma_sg_mmap() function to map pages to userspace. However in
>> your
>> code I don't see any place where you fill it with the pages of order higher
>> than 0. AFAIR vm_insert_page() can handle compound pages, but
>> alloc_pages_exact()
>> splits such pages into a set of pages of order 0, so you need to change your
>> code
>> to correctly fill buf->pages array.
>>
>>
>>> ---
>>>   drivers/media/v4l2-core/videobuf2-dma-sg.c |   49
>>> +++++++++++++++++++++-------
>>>   1 file changed, 38 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>> b/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>> index 16ae3dc..67a94ab 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>> @@ -42,10 +42,44 @@ struct vb2_dma_sg_buf {
>>>     static void vb2_dma_sg_put(void *buf_priv);
>>>   +static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf,
>>> +               gfp_t gfp_flags)
>>> +{
>>> +       unsigned int last_page = 0;
>>> +       void *vaddr = NULL;
>>> +       unsigned int req_pages;
>>> +
>>> +       while (last_page < buf->sg_desc.num_pages) {
>>> +               req_pages = buf->sg_desc.num_pages-last_page;
>>> +               while (req_pages >= 1) {
>>> +                       vaddr = alloc_pages_exact(req_pages*PAGE_SIZE,
>>> +                               GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN);
>>> +                       if (vaddr)
>>> +                               break;
>>> +                       req_pages >>= 1;
>>> +               }
>>> +               if (!vaddr) {
>>> +                       while (--last_page >= 0)
>>> +                               __free_page(buf->pages[last_page]);
>>> +                       return -ENOMEM;
>>> +               }
>>> +               while (req_pages) {
>>> +                       buf->pages[last_page] = virt_to_page(vaddr);
>>> +                       sg_set_page(&buf->sg_desc.sglist[last_page],
>>> +                                       buf->pages[last_page], PAGE_SIZE,
>>> 0);
>>> +                       vaddr += PAGE_SIZE;
>>> +                       last_page++;
>>> +                       req_pages--;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size, gfp_t
>>> gfp_flags)
>>>   {
>>>         struct vb2_dma_sg_buf *buf;
>>> -       int i;
>>> +       int ret;
>>>         buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>>         if (!buf)
>>> @@ -69,14 +103,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx,
>>> unsigned long size, gfp_t gfp_fla
>>>         if (!buf->pages)
>>>                 goto fail_pages_array_alloc;
>>>   -     for (i = 0; i < buf->sg_desc.num_pages; ++i) {
>>> -               buf->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO |
>>> -                                          __GFP_NOWARN | gfp_flags);
>>> -               if (NULL == buf->pages[i])
>>> -                       goto fail_pages_alloc;
>>> -               sg_set_page(&buf->sg_desc.sglist[i],
>>> -                           buf->pages[i], PAGE_SIZE, 0);
>>> -       }
>>> +       ret = vb2_dma_sg_alloc_compacted(buf, gfp_flags);
>>> +       if (ret)
>>> +               goto fail_pages_alloc;
>>>         buf->handler.refcount = &buf->refcount;
>>>         buf->handler.put = vb2_dma_sg_put;
>>> @@ -89,8 +118,6 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
>>> long size, gfp_t gfp_fla
>>>         return buf;
>>>     fail_pages_alloc:
>>> -       while (--i >= 0)
>>> -               __free_page(buf->pages[i]);
>>>         kfree(buf->pages);
>>>     fail_pages_array_alloc:
>>
>>
>> Best regards
>> --
>> Marek Szyprowski
>> Samsung R&D Institute Poland
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Ricardo Ribalda
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index 16ae3dc..bbb3680 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -24,6 +24,8 @@ 
 static int debug;
 module_param(debug, int, 0644);

+#define PAGE_SPLIT
+
 #define dprintk(level, fmt, arg...) \
  do { \
  if (debug >= level) \
@@ -42,10 +44,69 @@  struct vb2_dma_sg_buf {

 static void vb2_dma_sg_put(void *buf_priv);

+static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf,
+ gfp_t gfp_flags)
+{
+ unsigned int last_page = 0;
+ int size = buf->sg_desc.size;
+
+ while (size > 0) {
+ struct page *pages;
+ int order;
+#ifdef PAGE_SPLIT
+ int i;
+#endif
+
+ order = get_order(size);
+ /* Dont over allocate*/
+ if ((PAGE_SIZE << order) > size)
+ order--;
+
+ pages = NULL;
+ while (!pages) {
+ pages = alloc_pages(GFP_KERNEL | __GFP_ZERO |
+ __GFP_NOWARN | gfp_flags, order);
+ if (pages)
+ break;
+
+
+ if (order == 0)
+ while (--last_page >= 0) {
+ __free_pages(
+ buf->pages[last_page],
+ get_order(sg_dma_len
+ (&buf->sg_desc.sglist[last_page])));
+ return -ENOMEM;
+ }
+ order--;
+ }
+
+#ifdef PAGE_SPLIT
+ split_page(pages, order);
+ for (i = 0; i < (1<<order); i++) {
+ buf->pages[last_page] = pages + i;
+ sg_set_page(&buf->sg_desc.sglist[last_page],
+ buf->pages[last_page], PAGE_SIZE, 0);
+ last_page++;
+ }
+#else
+ buf->pages[last_page] = pages;
+ sg_set_page(&buf->sg_desc.sglist[last_page],
+ buf->pages[last_page], PAGE_SIZE << order, 0);
+ last_page++;
+#endif
+ size -= PAGE_SIZE << order;
+ }
+
+ buf->sg_desc.num_pages = last_page;
+
+ return 0;
+}
+
 static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size,
gfp_t gfp_flags)
 {
  struct vb2_dma_sg_buf *buf;
- int i;
+ int ret;

  buf = kzalloc(sizeof *buf, GFP_KERNEL);