cx88: Enable additional cx2388x features. Version 2

The cx2388x family of broadcast decoders all have features not enabled by the standard cx88 driver. This revised patch series adds controls allowing the chroma AGC and the color killer to be enabled (Version 1 of the series used module parameters). By default both features are disabled as in previous versions of the driver.

The Chroma AGC and the color killer is sometimes needed when using signal sources of less than optimal quality.

The patches applies cleanly to 7370:11fdae6654e8 of
http://linuxtv.org/hg/v4l-dvb/ and have been tested an a HVR-1300. The patches should be applied in order.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

4 files changed, 42 insertions(+), 3 deletions(-)
linux/drivers/media/video/cx88/cx88-blackbird.c |    1 
linux/drivers/media/video/cx88/cx88-core.c      |    6 +++
linux/drivers/media/video/cx88/cx88-video.c     |   37 +++++++++++++++++++++--
linux/drivers/media/video/cx88/cx88.h           |    1 

# HG changeset patch
# User "Frej Drejhammar <[email protected]>" # Date 1205668069 -3600
# Node ID ac2c307ae12abeb35b59dc0bb1aef95ac6f51798 # Parent 11fdae6654e804fdd000b0c3c9f3a4a7344ffd69
cx88: Add user control for chroma AGC

From: "Frej Drejhammar <[email protected]>"

The cx2388x family has support for chroma AGC. This patch adds a user control, "Chroma AGC", controlling it. By default chroma AGC is disabled, as in previous versions of the driver.

diff -r 11fdae6654e8 -r ac2c307ae12a linux/drivers/media/video/cx88/cx88-blackbird.c
@@ -701,6 +701,7 @@ static const u32 *ctrl_classes[] = {
 static const u32 *ctrl_classes[] = {
 	cx88_user_ctrls,
 	cx2341x_mpeg_ctrls,
+	cx88_priv_ctrls,
 	NULL
 };
 

diff -r 11fdae6654e8 -r ac2c307ae12a linux/drivers/media/video/cx88/cx88-core.c
@@ -958,7 +958,11 @@ int cx88_set_tvnorm(struct cx88_core *co
 
 	dprintk(1,"set_tvnorm: MO_INPUT_FORMAT  0x%08x [old=0x%08x]\n",
 		cxiformat, cx_read(MO_INPUT_FORMAT) & 0x0f);
-	cx_andor(MO_INPUT_FORMAT, 0xf, cxiformat);
+	/* Chroma AGC must be disabled if SECAM is used */
+	if (norm & V4L2_STD_SECAM)
+		cx_andor(MO_INPUT_FORMAT, 0x40f, cxiformat);
+	else
+		cx_andor(MO_INPUT_FORMAT, 0xf, cxiformat);
 
 #if 1
 	// FIXME: as-is from DScaler

diff -r 11fdae6654e8 -r ac2c307ae12a linux/drivers/media/video/cx88/cx88-video.c
@@ -177,6 +177,11 @@ static struct cx8800_fmt* format_by_four
 
 /* ------------------------------------------------------------------- */
 
+/* Private controls for cx2388x */
+#define CX88_CID_PRIVATE_CLASS V4L2_CID_PRIVATE_BASE
+#define CX88_CID_CHROMA_AGC   (V4L2_CID_PRIVATE_BASE + 0)
+#define CX88_CID_LAST_PRIVATE (V4L2_CID_PRIVATE_BASE + 1)
+
 static const struct v4l2_queryctrl no_ctl = {
 	.name  = "42",
 	.flags = V4L2_CTRL_FLAG_DISABLED,

@@ -244,6 +249,18 @@ static struct cx88_ctrl cx8800_ctls[] = 
 		.mask                  = 0x00ff,
 		.shift                 = 0,
 	},{
+		.v = {
+			.id            = CX88_CID_CHROMA_AGC,
+			.name          = "Chroma AGC",
+			.minimum       = 0,
+			.maximum       = 1,
+			.default_value = 0x0,
+			.type          = V4L2_CTRL_TYPE_BOOLEAN,
+		},
+		.reg                   = MO_INPUT_FORMAT,
+		.mask                  = 1 << 10,
+		.shift                 = 10,
+	}, {
 	/* --- audio --- */
 		.v = {
 			.id            = V4L2_CID_AUDIO_MUTE,

@@ -302,8 +319,16 @@ const u32 cx88_user_ctrls[] = {
 };
 EXPORT_SYMBOL(cx88_user_ctrls);
 
+const u32 cx88_priv_ctrls[] = {
+	CX88_CID_PRIVATE_CLASS,
+	CX88_CID_CHROMA_AGC,
+	0
+};
+EXPORT_SYMBOL(cx88_priv_ctrls);
+
 static const u32 *ctrl_classes[] = {
 	cx88_user_ctrls,
+	cx88_priv_ctrls,
 	NULL
 };
 

@@ -311,8 +336,10 @@ int cx8800_ctrl_query(struct v4l2_queryc
 {
 	int i;
 
-	if (qctrl->id < V4L2_CID_BASE ||
-	    qctrl->id >= V4L2_CID_LASTP1)
+	if ((qctrl->id < V4L2_CID_BASE ||
+	     qctrl->id >= V4L2_CID_LASTP1) &&
+	    (qctrl->id < V4L2_CID_PRIVATE_BASE ||
+	     qctrl->id >= CX88_CID_LAST_PRIVATE))
 		return -EINVAL;
 	for (i = 0; i < CX8800_CTLS; i++)
 		if (cx8800_ctls[i].v.id == qctrl->id)

@@ -1225,6 +1252,12 @@ int cx88_set_control(struct cx88_core *c
 		}
 		mask=0xffff;
 		break;
+	case CX88_CID_CHROMA_AGC:
+		/* Do not allow chroma AGC to be enabled for SECAM */
+		value = ((ctl->value - c->off) << c->shift) & c->mask;
+		if (core->tvnorm & V4L2_STD_SECAM && value)
+			return -EINVAL;
+		break;
 	default:
 		value = ((ctl->value - c->off) << c->shift) & c->mask;
 		break;

diff -r 11fdae6654e8 -r ac2c307ae12a linux/drivers/media/video/cx88/cx88.h
@@ -681,6 +681,7 @@ void cx8802_cancel_buffers(struct cx8802
 /* ----------------------------------------------------------- */
 /* cx88-video.c*/
 extern const u32 cx88_user_ctrls[];
+extern const u32 cx88_priv_ctrls[];
 extern int cx8800_ctrl_query(struct v4l2_queryctrl *qctrl);
 int cx88_enum_input (struct cx88_core  *core,struct v4l2_input *i);
 int cx88_set_freq (struct cx88_core  *core,struct v4l2_frequency *f);

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

1 file changed, 15 insertions(+), 1 deletion(-)

linux/drivers/media/video/cx88/cx88-video.c | 16 +++++++++++++++-

# HG changeset patch
# User "Frej Drejhammar <[email protected]>" # Date 1205668146 -3600
# Node ID 8843d200ee091ef49579555ef6ae4de031986342 # Parent ac2c307ae12abeb35b59dc0bb1aef95ac6f51798
cx88: Add user control for color killer

From: "Frej Drejhammar <[email protected]>"

The cx2388x family has a color killer. This patch adds a user control, "Color killer", controlling it. By default the color killer is disabled, as in previous versions of the driver.

diff -r ac2c307ae12a -r 8843d200ee09 linux/drivers/media/video/cx88/cx88-video.c
@@ -180,7 +180,8 @@ static struct cx8800_fmt* format_by_four
 /* Private controls for cx2388x */
 #define CX88_CID_PRIVATE_CLASS V4L2_CID_PRIVATE_BASE
 #define CX88_CID_CHROMA_AGC   (V4L2_CID_PRIVATE_BASE + 0)
-#define CX88_CID_LAST_PRIVATE (V4L2_CID_PRIVATE_BASE + 1)
+#define CX88_CID_COLOR_KILLER (V4L2_CID_PRIVATE_BASE + 1)
+#define CX88_CID_LAST_PRIVATE (V4L2_CID_PRIVATE_BASE + 2)
 
 static const struct v4l2_queryctrl no_ctl = {
 	.name  = "42",

@@ -261,6 +262,18 @@ static struct cx88_ctrl cx8800_ctls[] = 
 		.mask                  = 1 << 10,
 		.shift                 = 10,
 	}, {
+		.v = {
+			.id            = CX88_CID_COLOR_KILLER,
+			.name          = "Color killer",
+			.minimum       = 0,
+			.maximum       = 1,
+			.default_value = 0x0,
+			.type          = V4L2_CTRL_TYPE_BOOLEAN,
+		},
+		.reg                   = MO_INPUT_FORMAT,
+		.mask                  = 1 << 9,
+		.shift                 = 9,
+	}, {
 	/* --- audio --- */
 		.v = {
 			.id            = V4L2_CID_AUDIO_MUTE,

@@ -322,6 +335,7 @@ const u32 cx88_priv_ctrls[] = {
 const u32 cx88_priv_ctrls[] = {
 	CX88_CID_PRIVATE_CLASS,
 	CX88_CID_CHROMA_AGC,
+	CX88_CID_COLOR_KILLER,
 	0
 };
 EXPORT_SYMBOL(cx88_priv_ctrls);

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hi Frej,

I have a few comments about this:

1) Should we really expose these settings to the user? I have my doubts whether the average user would know what to do with this, and I also wonder whether it makes enough of a difference in picture quality. I might be wrong, of course. It would help if you could show examples of the picture quality with and without these settings. Note that a change like 'Chroma AGC must be disabled if SECAM is used' is a bug fix and should clearly go in.

2) Chroma AGC and color killer is also present in other chips (cx2584x, cx23418, possibly other similar Conexant chips). So if we decide on allowing these controls I would prefer making this a standard control, rather than a private one.

3) If we decide on allowing these controls, then they must be documented in the v4l2 spec.

Personally I think these controls are too low-level, but that's just my opinion. Most chips contains a whole array of similar tweaks that you can do, and exposing them all is not the way to go.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hi Hans,

That was my initial take on it, therefore the first version of the patch just added a module parameter. I reasoned that chroma AGC was something you just needed to enable once depending on the quality of your video-source. Then Trent Piepho suggested that the functionality should really be exposed as controls. I think he has a point, consider for example living in a place such as southern Germany where you could receive both German PAL and French SECAM broadcasts. If you then also used a composite/s-video external video source you would want to be able to change the setting depending on your input and the channel you tune to.

For me it does, fiddling with the saturation and hue controls I never managed to get neutral color reproduction. The colors were either washed out or saturated to look like a fifties technicolor movie. The color killer does not make a very large impact for black and white material (the only time it is needed), frankly I'm not sure if its not just the placebo effect. I can live without color killer but definitely not without chroma AGC.

But rather pointless as it currently cannot be enabled...

A quick grep shows that the bttv-driver also exposes chroma AGC as a private control. Cx2584x has chroma AGC enabled by default. Maybe the right thing to do is to enable chroma AGC by default for PAL and NTSC? Chroma AGC is something you'll find on most VCRs and TVs, and then it is on by default.

Personally I'm against dumbing down the driver and not exposing features which are useful. An argument against your stance is that the V4L2-spec defines V4L2_CID_AUTOGAIN, V4L2_CID_AUTO_WHITE_BALANCE and V4L2_CID_HUE_AUTO (half of the functionality of chroma AGC) which are similar functions. Also consider the MPEG controls, most of which are fairly obscure if you are not familiar with the MPEG specs.

Regards,

--Frej

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

V4L2 controls aren't some menu a user must wade though when setting up mythtv. I don't see the harm in adding them. There is even software for windows that will let people mess with these things, so there is a demand.

CAGC came up before and there was a patch. I had a patch for it, but then Mauro changed some stuff in the driver around so my method no longer worked.

One of the things you should do it make the control inactive when in SECAM mode. V4L2 has a flag to indicate controls that don't apply to the device's current mode.

Overall, module parameters for these things is something the V4L1 bttv driver did because controls didn't exist for V4L1. Controls are a better way and we shouldn't use module parameters for video decoding controls.

CAGC makes a difference for me too. Some of my channels are over saturated and some are under saturated and CAGC fixes them. I don't recall if I posted pictures last time CAGC came up, but it really does make a difference.

I haven't ever been able to notice an effect from color killer. Maybe if you had poor reception from a B&W source? Not much black and white on broadcast TV these days.

That's what I would do. Have a standard control for CAGC and turn it on by default.

If I wanted to be told I wasn't worthy to use my hardware, I'd run windows!

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Trent,

I guess it is the V4L2_CTRL_FLAG_INACTIVE flag (in the flag field of struct v4l2_queryctrl) you are referring to, correct?

Good that I'm not the only one who wants/needs it :)

So maybe I should just skip the color killer then...

Then that's what I'll do. Expect a revised version of the patch which enables CAGC by default for PAL+NTSC and implements the V4L2_CTRL_FLAG_INACTIVE by the end of the week.

Hear, hear! :)

Regards,

--Frej

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

I agree with the idea of making Chroma AGC available as a control - I've got the same problem here with at least one channel that is way over-saturated relative to the others.

That said, is this something that would show up in e.g. xawtv or other analog TV programs?

"Life is full of happy and sad events.  If you take the time
to concentrate on the former, you'll get further in life."
Vanessa Ezekowitz  <[email protected]>

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

That's not what it does:
If a color-burst of 25 (NTSC) or 35 (PAL/SECAM) percent or less of the nominal amplitude is detected for 127 consecutive scan lines, the color-difference signals U and V are set to 0. When the low color detection is active, the reduced chrominance signal is still separated from the composite signal to generate the luminance portion of the signal. The resulting Cr and Cb values are 128. Output of the chrominance signal is re-enabled when a color-burst of 43 (NTSC) or 60 (PAL/SECAM) percent or greater of nominal amplitude is detected for 127 consecutive scan lines. Low color detection and removal may be disabled.

I think it's when you have poor reception and can't decode the color signal, which is in the upper part of the band. Rather than decode garbage color, it just turns color off. Or maybe it's for avoiding adding spurious color when you have a B&W signal? I've never been able to get it to do anything I could notice.

Getting the full luma bandwidth would be achieved by turning off the luma notch filter, which should be done already for s-video sources. It might not be a bad idea to have a control to allow this for composite signals, like B&W surveillance cams.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

That's not quite what I meant. I'm responsible of all the MPEG controls, so I'm definitely all for exposing hardware features to the user :-)

What I want to prevent is adding controls as a workaround for what might be a driver bug. So in this case I wonder whether chroma AGC shouldn't be enabled in the cx88 driver as it is for cx2584x.

Looking at the cx25840 datasheet it basically says that it should always be enabled except for component input (YPrPb) or SECAM. So I would suggest doing the same in cx88 rather than adding a control. Only if there are cases where Chroma AGC harms the picture quality rather than improves it, then the addition of a control might become important.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

IMO, the better would be to add both Chroma AGC and Color Killer controls as a generic control.

The default value for Chroma AGC should be changed to match the datasheet recommended way (0 for SECAM, 1 for PAL/NTSC).

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

The data sheet for cx2388x is not so clear. For ACGC it describes what it does and then notes that it can be turned off. The default is off.

For the color-killer there is a similar description, and it then notes that the color-killer can be disabled. The default is disabled.

Is there a procedure for adding controls to the V4L2-spec? Is the docbook source available in a public repository (not just the tarball at v4l2spec.bytesex.org)?

I'll revise the patch to do this.

Regards,

--Frej

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list