diff mbox

[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran

Message ID 20090301161800.4ce70acd@hyperion.delvare (mailing list archive)
State RFC
Headers show

Commit Message

Jean Delvare March 1, 2009, 3:18 p.m. UTC
Hi Trent,

On Sun, 1 Mar 2009 06:38:03 -0800 (PST), Trent Piepho wrote:
> On Sun, 22 Feb 2009, Hans Verkuil wrote:
> > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran for the
> > following:
> 
> I have some questions about your changes.
> (...)
> > - zoran: remove broken BIGPHYS_AREA and BUZ_HIMEM code, and allow for
> > kmallocs > 128 kB
> 
> It looks like the last time the bigphys_area patch was updated was to
> 2.6.11 so there probably isn't much point is supporting that anymore.  But
> is the highmem code broken?  Trying to allocate multiple megabyte buffers
> on systems without gigabytes of memory doesn't work very well.  You get
> nice things like this in your kernel log:
> 
> tvtime: page allocation failure. order:8, mode:0x40d0
>  [<b0138243>] __alloc_pages+0x29b/0x2aa
>  [<b014a371>] __slab_alloc+0x192/0x343

This can be "fixed" with the following patch:

Subject: zoran: Don't frighten users with failed buffer allocation

kmalloc() can fail for large video buffers. By default the kernel
complains loudly about allocation failures, but we don't want to
frighten the user, so ask kmalloc() to keep quiet on such failures.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 linux/drivers/media/video/zoran/zoran_driver.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


Some other v4l drivers do that already. Of course the allocation still
fails. But I doubt that restoring the highmem code is the right fix. For
one thing, it didn't work for me when I tried it. Did it ever work for
you? For another, I remember the big fat warning next to that piece of
code, which said that if any other piece of kernel code tried to do the
same, everything would break into pieces.

So, if the allocation failures are really an issue for anyone out there,
I'd rather have the zr36067 driver (optionally) pre-allocate buffers so
that it stands are greater chance to get them. This should be fairly
easy to implement, and safe too.

I am curious if the allocation problem really bothers anyone though. I
suspect that users that have a Zoran adapter on low-memory machines use
the MJPEG interface (lavrec/lavplay) and not the V4L2 interface.

> > - zoran: use slider flag with volume etc. controls.
> 
> Volume control?  zoran has no audio.
> 
> > - zoran: fix field typo.
> 
> Why not merge this patch into the patch that created the typo?  It only
> takes seconds if you use the features of modern SCMs.
> 
> > - zoran: set bytesperline to 0 when using MJPEG.
> 
> Why not merge this patch into the one that created the bug?

While I tend to agree with you, I fear it is too late in this case, as
Mauro as already pulled Hans' changes.
diff mbox

Patch

--- v4l-dvb-zoran.orig/linux/drivers/media/video/zoran/zoran_driver.c	2009-02-23 09:48:49.000000000 +0100
+++ v4l-dvb-zoran/linux/drivers/media/video/zoran/zoran_driver.c	2009-02-23 12:51:21.000000000 +0100
@@ -212,7 +212,8 @@  v4l_fbuffer_alloc (struct file *file)
 				ZR_DEVNAME(zr), i);
 
 		//udelay(20);
-		mem = kmalloc(fh->v4l_buffers.buffer_size, GFP_KERNEL);
+		mem = kmalloc(fh->v4l_buffers.buffer_size,
+			      GFP_KERNEL | __GFP_NOWARN);
 		if (!mem) {
 			dprintk(1,
 				KERN_ERR