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 these are the module sizes before and after: Before: zr36060 8396 1 saa7185 5872 0 saa7111 6448 0 v4l2_common 13568 2 saa7185,saa7111 zr36067 71280 0 i2c_algo_bit 5252 1 zr36067 videocodec 6092 2 zr36060,zr36067 videodev 33840 2 v4l2_common,zr36067 v4l1_compat 11524 1 videodev After: zr36060 8396 1 saa7185 4084 1 saa7115 14640 1 zr36067 65516 0 videocodec 6092 2 zr36060,zr36067 v4l2_common 12736 3 saa7185,saa7115,zr36067 videodev 33840 4 saa7185,saa7115,zr36067,v4l2_common v4l1_compat 11524 1 videodev i2c_algo_bit 5252 1 zr36067 > > In addition the zoran driver contained experimental support for highmem > > allocations which relied on a 'big phys area' kernel patch that was > > never merged into the kernel, and the only reason this was needed at > > all is that > > bigphys_area and highmem are different. highmem lets you leave off some > memory by giving the kernel a mem parameter and the zoran driver attempts > to access it via ioremap(). bigphys_area was a patch that never made it > into the kernel. > > > a long time ago kmalloc could only allocate up to 128 kB of contiguous > > memory. These days those limitations are gone, and all this code could > > be > > Even though kmalloc supports higher order allocations now, they can > easily fail. The right approach which most drivers take is to allocate up front when the driver is loaded. Zoran should be modified to do the same. > > behaviors with i2c (misdetections and not detecting decoders at all > > sometimes) are now fixed. The v4l2 behavior has also been improved > > considerably. > > Any examples of what's improved? Those are the various bug fixes mentions in the changelog. Mostly relating to the G/S/TRY_FMT handling. > > What is not working is playback of video. However, it turns out that > > that has been broken for some time now. The hope is that now that the > > driver has been cleaned up someone can actually go in and try to > > discover what broke. Note that video pass-through is working fine. > > I tried playback with the driver from last month and it appears to work > fine. Weird. I never could get it to work at all. At best it would just stall after 10-20 seconds. The behavior was identical whether I used the driver with or without my changes. 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
