Re: [PATCH 1/2] videobuf2-dma-sg: Allocate pages as contiguous as possible
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.riba...@gmail.com
---
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
--
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
Re: [PATCH 1/2] videobuf2-dma-sg: Allocate pages as contiguous as possible
Hi Ricardo,
I messed up one thing in my initial reply, sorry :(
And two additional nitpicks, while we're at it.
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.
s/houndreds/hundreds/
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.riba...@gmail.com
With those changes you can add:
Reviewed-by: Andre Heider a.hei...@gmail.com
---
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;
+ }
+ order--;
+ }
+
+ split_page(pages, order);
+ for (i = 0; i (1order); i++) {
whitespace nit: (1 order)
+ buf-pages[last_page] = pages[i];
My fault, it should read:
buf-pages[last_page] = pages[i];
+ sg_set_page(buf-sg_desc.sglist[last_page],
+ buf-pages[last_page], PAGE_SIZE, 0);
+ last_page++;
+ }
+
+ size -= PAGE_SIZE order;
+ }
+
+ 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 +114,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 +129,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:
--
1.7.10.4
--
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
Re: [PATCH 1/4] videobuf2-dma-sg: Allocate pages as contiguous as possible
On Fri, Jul 19, 2013 at 09:58:46AM +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 fo dma segments
Signed-off-by: Ricardo Ribalda Delgado ricardo.riba...@gmail.com
---
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..9bf02c3 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 = 0) {
+ __free_page(buf-pages[last_page]);
+ return -ENOMEM;
+ }
Looks leaky, you probably ment:
while (--last_page = 0)
__free_page(buf-pages[last_page]);
return -ENOMEM;
But that still underflows 'last_page', so:
while (last_page--)
__free_page(buf-pages[last_page]);
return -ENOMEM;
+ order--;
+ }
+
+ split_page(pages, order);
+ for (i = 0; i (1order); i++) {
+ buf-pages[last_page] = pages + i;
That makes the reader double check the type of 'pages', why not:
buf-pages[last_page] = pages[i];
+ sg_set_page(buf-sg_desc.sglist[last_page],
+ buf-pages[last_page], PAGE_SIZE, 0);
+ last_page++;
+ }
+
+ size -= PAGE_SIZE order;
+ }
+
+ 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 +114,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 +129,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:
--
1.7.10.4
--
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
--
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
Re: af9035 test needed!
Hi, On Thu, Jan 31, 2013 at 2:46 PM, Michael Krufky mkru...@linuxtv.org wrote: Hey guys... somehow this email slipped through my filters :-( I see it now, and I'll give a look over the patch this weekend. I suspect the merge window opens soon, so... *poke* ;) Any chance for 3.9? Thanks, Andre -- 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
Re: af9035 test needed!
Hi, On Fri, Jan 11, 2013 at 7:38 PM, Antti Palosaari cr...@iki.fi wrote: Could you test that (tda18218 mxl5007t): http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/it9135_tuner I got a 'TerraTec Cinergy T Stick Dual RC (rev. 2)', which is fixed by this series. Any chance to get this into 3.9 (I guess its too late for the USB VID/PID 'fix' for 3.8)? Regards, Andre -- 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
Re: af9035 test needed!
Hi, On Thu, Jan 31, 2013 at 2:59 PM, Antti Palosaari cr...@iki.fi wrote: Thank you for the report! There was someone else who reported it working too. Do you want to your name as tester for the changelog? if I didn't mess up my way of testing feel free to add Tested-by: Andre Heider a.hei...@gmail.com to these patches: af9035: merge af9035 and it9135 eeprom read routines af9035: USB1.1 support (== PID filters) af9035: constify clock tables af9035: [0ccd:0099] TerraTec Cinergy T Stick Dual RC (rev. 2) af9015: reject device TerraTec Cinergy T Stick Dual RC (rev. 2) af9035: fix af9033 demod sampling frequency af9035: add auto configuration heuristic for it9135 af9035: add support for 1st gen it9135 af9033: support for it913x tuners ITE IT913X silicon tuner driver I didn't use any media trees before, and the whole media_build.git shebang seems a little, well, unusual... So I rebased media_tree.git/staging/for_v3.9 on Linus' master and then cherry-picked the patches mentioned above. That gives me: usb 2-1.5: new high-speed USB device number 3 using ehci-pci usb 2-1.5: New USB device found, idVendor=0ccd, idProduct=0099 usb 2-1.5: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 2-1.5: Product: DVB-T TV Stick usb 2-1.5: Manufacturer: ITE Technologies, Inc. input: ITE Technologies, Inc. DVB-T TV Stick as /devices/pci:00/:00:1d.0/usb2/2-1/2-1.5/2-1.5:1.1/input/input20 usb 2-1.5: af9035_identify_state: prechip_version=83 chip_version=01 chip_type=9135 hid-generic 0003:0CCD:0099.0007: input,hidraw4: USB HID v1.01 Keyboard [ITE Technologies, Inc. DVB-T TV Stick] on usb-:00:1d.0-1.5/input1 usb 2-1.5: dvb_usb_v2: found a 'TerraTec Cinergy T Stick Dual RC (rev. 2)' in cold state usb 2-1.5: dvb_usb_v2: downloading firmware from file 'dvb-usb-it9135-01.fw' usb 2-1.5: dvb_usb_af9035: firmware version=12.54.14.0 usb 2-1.5: dvb_usb_v2: found a 'TerraTec Cinergy T Stick Dual RC (rev. 2)' in warm state usb 2-1.5: dvb_usb_af9035: driver does not support 2nd tuner and will disable it usb 2-1.5: dvb_usb_v2: will pass the complete MPEG2 transport stream to the software demuxer DVB: registering new adapter (TerraTec Cinergy T Stick Dual RC (rev. 2)) i2c i2c-18: af9033: firmware version: LINK=255.255.255.255 OFDM=2.47.14.0 usb 2-1.5: DVB: registering adapter 0 frontend 0 (Afatech AF9033 (DVB-T))... Tuner LNA type :38 it913x: ITE Tech IT913X attached usb 2-1.5: dvb_usb_v2: 'TerraTec Cinergy T Stick Dual RC (rev. 2)' successfully initialized and connected I just yesterday got that TerraTec device too and I am going to add dual tuner support. Also, for some reason IT9135 v2 devices are not working - only v1. That is one thing I should fix before merge that stuff. Nice, feel free to CC me if you need any testing. Regards, Andre -- 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
