Hi Andre, Nice catch! thanks. I have just uploaded a new version. https://patchwork.linuxtv.org/patch/19502/ https://patchwork.linuxtv.org/patch/19503/ Thanks for your help On Fri, Aug 2, 2013 at 10:46 AM, Andre Heider <a.heider@xxxxxxxxx> wrote: > Hi Ricardo, > > sorry for the late answer, but the leak I mentioned in my first reply is still there, see below. > > On Fri, Jul 19, 2013 at 07:02:33PM +0200, 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. In the worst case it will behave as before, but most >> of the times it will reduce the number of dma segments >> >> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx> >> --- >> drivers/media/v4l2-core/videobuf2-dma-sg.c | 60 +++++++++++++++++++++++----- >> 1 file changed, 49 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..c053605 100644 >> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c >> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c >> @@ -42,10 +42,55 @@ 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; >> + int i; >> + >> + 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--) { >> + __free_page(buf->pages[last_page]); >> + return -ENOMEM; >> + } > > The return statement doesn't make sense in the while() scope, that way you wouldn't need the loop at all. > > To prevent leaking pages of prior iterations (those with higher orders), pull the return out of there: > > while (last_page--) > __free_page(buf->pages[last_page]); > return -ENOMEM; > > Regards, > Andre -- 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
