Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
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 kh...@linux-fr.org
---
linux/drivers/media/video/zoran/zoran_driver.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- v4l-dvb-zoran.orig/linux/drivers/media/video/zoran/zoran_driver.c
2009-02-23 09:48:49.0 +0100
+++ v4l-dvb-zoran/linux/drivers/media/video/zoran/zoran_driver.c
2009-02-23 12:51:21.0 +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
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.
--
Jean Delvare
--
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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
On Sun, 22 Feb 2009, Hans Verkuil wrote:
Also the v4l1 ioctls have been removed and instead zoran relies on the v4l1
compat layer.
I tried testing v4l1 with mplayer and it doesn't seem to work correctly.
[pid 29030] ioctl(3, VIDIOCSYNC, 0x884d790) = -1 EBUSY (Device or resource busy)
[pid 29030] gettimeofday({1235920259, 869629}, NULL) = 0
[pid 29030] ioctl(3, VIDIOCMCAPTURE, 0x884d790) = -1 EBUSY (Device or resource
busy)
[pid 29030] write(2, \nioctl mcapture failed: Device o..., 48
VIDIOCSYNC and VIDIOCMCAPTURE always return EBUSY. Though, it does appear
that mplayer and the driver actually do work and capture video.
It looks like this is because v4l1-compat calls VIDIOC_STREAMON for each
call to VIDIOCMCAPTURE or VIDIOCSYNC. The zoran driver returns EBUSY if
you try to do that while streaming is already on.
Maybe the zoran driver should detect stream on from a file handle that
already has streaming on as a no-op? Or maybe v4l1-compat should ignore
EBUSY errors from VIDIOC_STREAMON in those compat functions?
Fixing this in correctly in v4l1-compat is hard because that module doesn't
have any device state so it can't keep track if it needs to call
VIDIOC_STREAMON or not. Lots of v4l1 apps expect that the driver will
start streaming automatically when they call VIDIOCMCAPTURE.
--
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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
On Sunday 01 March 2009 15:38:03 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: convert to video_ioctl2 and remove 'ready_to_be_freed' hack.
It looks like this patch deleted the code relating to this comment:
* We don't free the buffers right on munmap() because that
* causes oopses (kfree() inside munmap() oopses for no
* apparent reason - it's also not reproduceable in any way,
* but moving the free code outside the munmap() handler fixes
* all this... If someone knows why, please explain me (Ronald)
Do you have an explanation of why it's safe to do this? There's nothing
in the patch description (there is no patch description) about it.
I should have expanded on that. There were several reasons for doing this:
1) the hack itself wouldn't work anymore if you convert to video_ioctl2
since there is no easy entrypoint for all the ioctls. It's not the reason
for removing it, but it is what brought it to my attention.
2) it's a really ancient hack and I'm convinced whatever bug caused this has
long since been fixed.
3) if there still is a bug, then this code isn't the way to do it, you need
to fix the real bug.
Rather than do difficult things to keep a highly questionable patch alive I
removed it in its entirely. Should it crop up, then I'll fix it the right
way. This driver is full of cruft and stuff like this only makes it hard to
maintain. Note that in all the testing that this driver got it hasn't been
seen at all.
- 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
See Jean's answer. I couldn't get highmem to work either, although I admit
that I didn't spend much time on it.
- zoran: use slider flag with volume etc. controls.
Volume control? zoran has no audio.
My mistake. I meant brightness, contrast, etc.
- 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.
A typo in the zoran driver before I started work on it, not a typo in one of
my patches.
- zoran: set bytesperline to 0 when using MJPEG.
Why not merge this patch into the one that created the bug?
Also a bug in the zoran driver before I started to work on it.
- zoran: fix G_FMT
You sure about that? Each jpeg buffer only has one field.
It brings it in line with the behavior of S_FMT and TRY_FMT. Whether those
are correct is questionable, but with this fix in I at least get back the
same format with G_FMT as I set with S_FMT. Before I would get back half
the height.
- zoran: if reqbufs is called with count == 0, do a streamoff.
Did you mean to change the debug level of those printks? It's not in
your patch description.
Yes, that was intentional. These messages are fine at higher debug levels,
but I don't want to see them during normal operation.
- zoran: TRY_FMT and S_FMT now do the same parameter checks.
Is this is mistake that was left off of a previous patch? No patch
description so I'm not sure.
No, this is a real fix for pre-existing zoran driver bugs.
- bt819: convert to v4l2_subdev.
- bt819: that delay include is needed after all.
Can these two not be folded?
Yes.
- zoran: convert to v4l2_device/v4l2_subdev.
+static const unsigned short adv717x_addrs[] = { 0x6a, 0x6b, 0x2a, 0x2b,
I2C_CLIENT_END };
adv7171/5 are at 0x2a and 0x2b, while adv7170 is at 0x6a and 0x6b.
Is this from the datasheets? The drivers say that adv7170/5 are at 0x6a/6b,
while adv7171/6 are at 0x2a/b. In other words, they both use the same four
addresses. I didn't check the datasheets, though.
- zoran: increase bufsize to a value suitable for 768x576.
How about folding this into the first patch that changed v4l_bufsize to
810?
A bit too late :-(
This is a huge patch series that brings the zoran driver completely up
to date with the latest v4l2 framework. In particular it converts it to
use video_ioctl2, the communication with the i2c modules is converted
to from v4l1 to v4l2 and that made the conversion to
v4l2_device/v4l2_subdev possible.
As a result of this the saa7111.c and saa7114.c drivers are removed and
instead the saa7115.c driver is used which can handle these older
saa711x devices as well.
Any figures for driver size before and after?
Which driver? saa7115 is of course unchanged, but
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
On Friday 27 February 2009 01:33:09 Mauro Carvalho Chehab wrote: On Sun, 22 Feb 2009 18:58:29 +0100 Hans Verkuil hverk...@xs4all.nl wrote: It took me several long weekends to get all this work done, but I think it's been very worthwhile. Very great job! Yet, I had to add some patches to manually fix a few merge conflicts after your series. I'll try to fold those patches at the proper places to avoid breaking bisect upstream. A side note: we should now remove the video_decoder.h headers. There are only 3 drivers left using it. I'll add an RFC asking for people to help with the remaining ones, or otherwise strip them from kernel. I have a v4l-dvb-vino tree pending that converts the last video_decoder.h users. I'm waiting for test results, but the guy I've asked to do the testing (the original vino author) has very little time (and I suspect interest), so if I do not get feedback in time then I'll just make a pull request for that tree and get it in untested. I also noted that it was still included in mxb.c: it can certainly be removed there. Note that now that the zoran tree is merged, the video_encoder.h header has become obsolete. It's not in our tree, so you will have to manually remove it from the upstream kernel. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
On Sun, 22 Feb 2009, Hans Verkuil wrote: Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran for the following: You might consider posting to the mjpeg users list. Maybe there are some people who used the playback feature more often. -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
Hans Verkuil wrote: It took me several long weekends to get all this work done, but I think it's been very worthwhile. Kudos! Well done -- another positive change! Together these cards use the ... ks0127 i2c devices quick question(s) - is this what I think it is -- a remote controller IC ? If so, do you know who manufactures this IC? (I'm trying to connect the dots with the problematic ks003 and ks007 ICs found on some cards) -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-zoran
On Sun, 22 Feb 2009 13:32:08 -0500, CityK wrote: Hans Verkuil wrote: It took me several long weekends to get all this work done, but I think it's been very worthwhile. Kudos! Well done -- another positive change! I cant only second this. Yay! Together these cards use the ... ks0127 i2c devices quick question(s) - is this what I think it is -- a remote controller IC ? If so, do you know who manufactures this IC? (I'm trying to connect the dots with the problematic ks003 and ks007 ICs found on some cards) The KS0127 is a video decoder, and it's made by Samsung. -- Jean Delvare -- 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
