On Wed, 2010-08-04 at 11:40 -0400, Steven Toth wrote: > On 7/31/10 2:28 PM, Andy Walls wrote: > > On Sat, 2010-07-31 at 12:31 -0400, Steven Toth wrote: > >> Mauro, > >> > >> Please pull from http://www.kernellabs.com/hg/~stoth/cx23885-mpx > > >> A pretty large patch set which adds a number of important features to > >> the cx23885 driver. > > > > I have a few cx25840 related comments: > > > > 2. The raw VBI startup you added in the cx25840 module is not going to > > serve you well. It is true that it is harmless to existing > > non-CX2388[578] boards. However, any app action that causes > > cx25840_std_setup() to be called will change register 0x474 to probably > > something you are not expecting. > > It's held up very well in testing. With ntsc-zv-vbi and mencoder I'm not seeing > any issues. It could be that the API sequence is working in the customer favor > but non-the-less I don't have any open bugs against VBI. I suspect userspace maybe somewhat forgiving for raw VBI samples, for example to handle situations where CC data appears to be on the wrong line, etc. Although from inspection, I think if you changes from PAL or SECAM back to NTSC, that register value will be changed from what your change initializes it to. In other words, I don't doubt that your change "works", but, IMO, it appears incomplete and symptoms will likely show up for someone, someday. > It should be made clear that VBI never previous worked on the cx23885, for any > cards. Yes. This is a step forward for cx23885 analog users. > > > > It would be better for you, in the long run, to fix up > > cx25840_std_setup() and cx25840_s_raw_fmt() to suit your needs, rather > > than a one off setting in the init function.. > > > > (I will admit the setup of the vblank, voffset, etc. in the cx25840 is > > not trivial, due to the way the ivtv driver likes to capture raw vbi > > lines and sliced vbi as if it were raw vbi data. I puzzled most of it > > out and fixed it up in the cx18 driver to be more sensible. It took a > > bit of staring at the BT878 data sheet to figure out the timing of the > > VBI slicer too, since the data is missing from later data sheets.) > > Ack. VBI via the cx25840 is a mess generally. What makes it really difficult is the regression testing required on any change to the vertical settings (vblank, voffset, etc.). ivtv sliced VBI capture depends on those settings, so changes must be coordinated with ivtv. Changes in ivtv sliced VBI handling means the modules that handle SAA717X and SAA7115 (and CX2584x) hardware would need to be instpected and tweaked if necessary, plus regression testing done on actual hardware. > However, the cx25840 driver is close enough to the avcore that doing a > cx23885-avcore.c and largely closing all of the source code from cx25840 is an > exercise in growing the linuxtv tree for little value. I disagree. In modern computing systems disk space and memory are cheap and only getting cheaper. $ wc -l * 678 cx25840-audio.c 1831 cx25840-core.c 108 cx25840-core.h 167 cx25840-firmware.c 257 cx25840-vbi.c 8 Kconfig 6 Makefile 3055 total $ ls -hl * -rw-rw-r--. 1 andy andy 17K 2009-10-29 19:15 cx25840-audio.c -rw-rw-r--. 1 andy andy 52K 2010-06-12 09:34 cx25840-core.c -rw-rw-r--. 1 andy andy 4.1K 2010-05-15 22:20 cx25840-core.h -rw-rw-r--. 1 andy andy 4.3K 2009-10-29 19:15 cx25840-firmware.c -rw-rw-r--. 1 andy andy 6.9K 2010-05-15 22:20 cx25840-vbi.c -rw-rw-r--. 1 andy andy 263 2009-09-26 19:59 Kconfig -rw-rw-r--. 1 andy andy 171 2009-09-26 19:59 Makefile $ size --format=SysV /lib/modules/2.6.32.16-141.fc12.x86_64/kernel/drivers/media/video/cx25840/cx25840.ko /lib/modules/2.6.32.16-141.fc12.x86_64/kernel/drivers/media/video/cx25840/cx25840.ko : section size addr .note.gnu.build-id 36 0 .text 15004 0 .exit.text 23 0 .init.text 114 0 .rodata 1712 0 .rodata.str1.1 3323 0 .modinfo 499 0 __param 80 0 __mcount_loc 352 0 .data 8 0 .gnu.linkonce.this_module 592 0 .bss 220 0 .gnu_debuglink 24 0 Total 21987 That's peanuts for disk space and memory. Skilled volunteer time to maintain code is the scarce commodity. The current conditional code branches in the cx25840 module hides bugs, increases complexity, and makes maintenance tasks take longer or makes them near impossible. I have changes implemented in cx18-av-*.[ch] that I would have loved to implement in cx25840, but the complexities of dealing with multiple subtly different chips, weird ivtv-ish dependency chains with other drivers, and the large suite of hardware required for proper regression testing makes that a costly endeavor. Here was a legacy cx25840 bug hiding in plain sight due to conditional statements obscuring the issue: http://linuxtv.org/hg/v4l-dvb/file/80d278a74d8a/linux/drivers/media/video/cx25840/cx25840-audio.c#l115 Can you see it in this "case 32000:" block? I know I didn't see it until I started teasing apart the conditional code branches into separate functions. Here's the simplified case statement with the bug fixed: http://linuxtv.org/hg/v4l-dvb/file/5f58b345e32e/linux/drivers/media/video/cx25840/cx25840-audio.c#l165 The CX23418's integrated A/V decoder is most similar to the CX2584x and CX2583x cores. The CX2388[578] and CX2310[12] A/V cores are closer to each other but farther away from the original CX2584x. Here's an example: http://linuxtv.org/hg/v4l-dvb/file/a14d56c730c4/linux/drivers/media/video/cx25840/cx25840-core.c#l975 Register 0x420 for the CX2584x controls a chroma component saturation. Now check the CX23888 datasheet and you'll see that register 0x420 has nothing to do with chroma saturation. Reusing the cx25840 module for the more modern cores CX2388[578] and CX2310[12] just hides these problems, making them harder to find. Fixing them in the cx25840 inserts yet another conditional code branch. Right now there are approximately $ grep is_cx2 * | wc -l 44 chip conditional code checks. The current changeset shows that it will only grow: adding complexity that has no benefit to CX2584x and CX2583x based boards and adding complexity that slows the development of CX2388[578] and CX2310[12] drivers. >From a maintenance perspective, I see quite a case for splitting CX2310[12] and CX2388[578] support out of the cx25840 module and putting it somewhere else. The gains in reduced maintainenance time and effort far outweigh the cost of any bloat. > > 3. This caught my eye: > > > > 8 + if (is_cx2388x(state)) { > > 9 + /* for cx23885 volume doesn't work, > > 10 + * the calculation always results in > > 11 + * e4 regardless. > > 12 + */ > > 13 + cx25840_write(client, 0x8d4, volume); > > 14 + } else > > 15 + cx25840_write(client, 0x8d4, 228 - (vol * 2)); > > > > Why is the result always 0xe4? Is that what the register always reads > > back? > > Yes. The cx25840 module expects the volume control to have a range from 0-65535. It divides the input control value by 512 (i.e. volume >> 9). If you are using a control in the cx23885 driver with a range from 0-100 or 0-255, that would explain why it doesn't work. BTW, the actual relationship the cx25840 module is implementing is explained in the end of this old post: http://www.gossamer-threads.com/lists/ivtv/devel/39868#39868 > > > > Note that your change won't have an intuitive effect, because you > > dropped the '-' sign in front of the volume. The user's slider control > > will likely work in reverse: up is soft, down is loud. > > This is a mistake on my end, although it represents no known regression given > that 0xe4 note and the lact of audio support in the kernel prior to this > patchset for the cx23885. I guess my real gripe is with the conditional code branch when it is not needed AFAICT, if one feeds the cx25840 module with a volume control range of 0-65535 that it is expecting. IIRC the change set also does not have a complementary conditional code branch when reading out the volume value from the register. (But I don't have time to double check that right now - late for work.) Regards, Andy > > I can say the V4L2 control to cx25840 volume scale mapping is a little > > funny. Here is documentation on the mapping from V4L2 volume control > > values to dB to cx25840 register values: > > > > http://linuxtv.org/hg/v4l-dvb/file/9652f85e688a/linux/drivers/media/video/cx18/cx18-alsa-mixer.c#l38 > > > > Thanks. > > > It is non-linear at the bottom end with a reg value of 228 = 0xe4 for > > 3/16ths of the entire range. This is due to the cx25840 module's desire > > to keep volume levels at the same perceptible level between the MSP400 > > and the CX2584x chips for users with multiple ivtv cards in their > > machine. (Yet another ivtv legacy.) > > > > You ought to look at what values are coming from the V4L2 volume control > > slider and see what's wrong. A V4L2 volume control value in the range > > 0xee00-0xeeff should correspond to 0 dB gain. > > It's been a while but form memory I added debug to the function and passed the > volume control and found the register values (and calculations to be way off), > certainly not what I expected. I figured this was an issue outside of the realm > of the cx23885 and thus isolated my change to the cx23885 only, not effecting > any other products. > > Regards, > > - Steve > -- 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
