[PATCH] cx88: implement sharpness control

[PATCH] cx88: implement sharpness control

[istvan_v]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

This patch adds support for V4L2_CID_SHARPNESS by changing the luma peak
filter and notch filter. It can be set in the range 0 to 9, with 0 being
the original and default mode.
One minor problem is that other code that sets the registers being used
(for example when switching TV channels) could reset the control to the
default. This could be avoided by making changes so that the bits used
to implement this control are not overwritten.

Signed-off-by: Istvan Varga <istvanv@users.sourceforge.net>

[-- Attachment #2: cx88-sharpness.patch --]
[-- Type: text/x-patch, Size: 2301 bytes --]

diff -r -d -N -U4 v4l-dvb-a79dd2ae4d0e.old/linux/drivers/media/video/cx88/cx88-video.c v4l-dvb-a79dd2ae4d0e/linux/drivers/media/video/cx88/cx88-video.c
--- v4l-dvb-a79dd2ae4d0e.old/linux/drivers/media/video/cx88/cx88-video.c	2010-03-23 03:39:52.000000000 +0100
+++ v4l-dvb-a79dd2ae4d0e/linux/drivers/media/video/cx88/cx88-video.c	2010-03-23 19:07:26.000000000 +0100
@@ -220,9 +220,24 @@
 		.off                   = 0,
 		.reg                   = MO_UV_SATURATION,
 		.mask                  = 0x00ff,
 		.shift                 = 0,
-	},{
+	}, {
+		.v = {
+			.id            = V4L2_CID_SHARPNESS,
+			.name          = "Sharpness",
+			.minimum       = 0,
+			.maximum       = 9,
+			.default_value = 0x0,
+			.type          = V4L2_CTRL_TYPE_INTEGER,
+		},
+		.off		       = 0,
+		/* NOTE: the value is converted and written to both even
+		   and odd registers in the code */
+		.reg                   = MO_FILTER_ODD,
+		.mask                  = 7 << 7,
+		.shift                 = 7,
+	}, {
 		.v = {
 			.id            = V4L2_CID_CHROMA_AGC,
 			.name          = "Chroma AGC",
 			.minimum       = 0,
@@ -300,8 +315,9 @@
 	V4L2_CID_HUE,
 	V4L2_CID_AUDIO_VOLUME,
 	V4L2_CID_AUDIO_BALANCE,
 	V4L2_CID_AUDIO_MUTE,
+	V4L2_CID_SHARPNESS,
 	V4L2_CID_CHROMA_AGC,
 	V4L2_CID_COLOR_KILLER,
 	0
 };
@@ -1187,8 +1203,13 @@
 		break;
 	case V4L2_CID_AUDIO_VOLUME:
 		ctl->value = 0x3f - (value & 0x3f);
 		break;
+	case V4L2_CID_SHARPNESS:
+		ctl->value = (value & 0x0380) >> 6;
+		ctl->value = (ctl->value < 8 ? 0 : (ctl->value - 6))
+			     | ((cx_read(MO_HTOTAL) & 0x0800) >> 11);
+		break;
 	default:
 		ctl->value = ((value + (c->off << c->shift)) & c->mask) >> c->shift;
 		break;
 	}
@@ -1246,8 +1267,16 @@
 			value=(value*0x5a)/0x7f<<8|value;
 		}
 		mask=0xffff;
 		break;
+	case V4L2_CID_SHARPNESS:
+		/* use 4xFsc or square pixel notch filter */
+		cx_andor(MO_HTOTAL, 0x1800, (ctl->value & 1) << 11);
+		/* 0b000, 0b100, 0b101, 0b110, or 0b111 */
+		value = (ctl->value < 2 ? 0 : (((ctl->value + 6) & 0x0E) << 6));
+		/* needs to be set for both fields */
+		cx_andor(MO_FILTER_EVEN, mask, value);
+		break;
 	case V4L2_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)

Re: [PATCH] cx88: implement sharpness control

[Mauro Carvalho Chehab]

istvan_v@mailbox.hu wrote:
> This patch adds support for V4L2_CID_SHARPNESS by changing the luma peak
> filter and notch filter. It can be set in the range 0 to 9, with 0 being
> the original and default mode.
> One minor problem is that other code that sets the registers being used
> (for example when switching TV channels) could reset the control to the
> default. This could be avoided by making changes so that the bits used
> to implement this control are not overwritten.
> 
> Signed-off-by: Istvan Varga <istvanv@users.sourceforge.net>
> 
> +	case V4L2_CID_SHARPNESS:
> +		/* use 4xFsc or square pixel notch filter */
> +		cx_andor(MO_HTOTAL, 0x1800, (ctl->value & 1) << 11);
> +		/* 0b000, 0b100, 0b101, 0b110, or 0b111 */
> +		value = (ctl->value < 2 ? 0 : (((ctl->value + 6) & 0x0E) << 6));
> +		/* needs to be set for both fields */
> +		cx_andor(MO_FILTER_EVEN, mask, value);
> +		break;

You're not adjusting the sharpness. Instead, you're changing the vertical tap filter,
and just for the even frames, plus the notch filter.
Tricky, and you're probably affecting the sharpness, but on an indirect and non-linear
way, as you're adjusting different measures.

This doesn't seem right, at least on my eyes. If this is really needed, it would
be better to break it into two controls (one for the notch filter and another for the
vertical tap filter).

Also, the "side effect" is not good: if you're using those bits, your code should assure
that no other part of the driver will touch on the used bits, and that the device will
be initialized with the default standard.

-- 

Cheers,
Mauro

Re: [PATCH] cx88: implement sharpness control

[istvan_v]

On 04/09/2010 06:33 AM, Mauro Carvalho Chehab wrote:

> You're not adjusting the sharpness. Instead, you're changing the vertical tap filter,
> and just for the even frames, plus the notch filter.
> Tricky, and you're probably affecting the sharpness, but on an indirect and non-linear
> way, as you're adjusting different measures.

No, I actually change the luma peak filter, which can add a peak of up
to about 6 dB centered at the chroma subcarrier frequency. So, it does
increase sharpness, and horizontally, not vertically. I did test the
patch before submitting it. Also, there is code in the patch that sets
the parameter for the odd field as well. So, in fact, this control
affects three registers: two for the peak filter, and a third one for
the notch filter.

> If this is really needed, it would be better to break it into two controls
> (one for the notch filter and another for the vertical tap filter).

Yes, that could possibly have been a good idea to have a separate
control for the notch filter. It was just easier to implement this way,
and I think there is no existing control that would be well suited for
the notch filter. Since both parameters do affect sharpness in some way,
I implemented it as a single control, rather than creating a separate
new one.

> Also, the "side effect" is not good: if you're using those bits, your code should assure
> that no other part of the driver will touch on the used bits, and that the device will
> be initialized with the default standard.

That is correct, and I did note that it would be addressed later if the
general idea of implementing the control is accepted. Fortunately, the
registers are changed elsewhere at only about two places.

By the way, did you have a look at the other two cx88 patches
(containing minor fixes only) I sent at the same time ?