[PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

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

I've attached a patch that correctly sets the max/min/default values
for the hdpvr picture controls. The reason the current values didn't
cause a problem until now is because any firmware <= 0.15 didn't
support them. The latest firmware releases properly support picture
controls and the values in the patch are derived from the windows
driver using SniffUSB2.0.

Thanks to Devin Heitmueller for helping me.

Regards.
--
Taylor

[-- Attachment #2: hdpvr.diff --]
[-- Type: text/x-diff, Size: 1502 bytes --]

diff -r abd3aac6644e linux/drivers/media/video/hdpvr/hdpvr-core.c
--- a/linux/drivers/media/video/hdpvr/hdpvr-core.c	Fri Jul 02 00:38:54 2010 -0300
+++ b/linux/drivers/media/video/hdpvr/hdpvr-core.c	Thu Oct 20 11:14:25 2011 -0400
@@ -262,10 +262,10 @@
 	.bitrate_mode	= HDPVR_CONSTANT,
 	.gop_mode	= HDPVR_SIMPLE_IDR_GOP,
 	.audio_codec	= V4L2_MPEG_AUDIO_ENCODING_AAC,
-	.brightness	= 0x86,
-	.contrast	= 0x80,
-	.hue		= 0x80,
-	.saturation	= 0x80,
+	.brightness	= 0x80,
+	.contrast	= 0x40,
+	.hue		= 0xf,
+	.saturation	= 0x40,
 	.sharpness	= 0x80,
 };
 
diff -r abd3aac6644e linux/drivers/media/video/hdpvr/hdpvr-video.c
--- a/linux/drivers/media/video/hdpvr/hdpvr-video.c	Fri Jul 02 00:38:54 2010 -0300
+++ b/linux/drivers/media/video/hdpvr/hdpvr-video.c	Thu Oct 20 11:14:25 2011 -0400
@@ -731,13 +731,13 @@
 
 	switch (qc->id) {
 	case V4L2_CID_BRIGHTNESS:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x86);
+		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
 	case V4L2_CID_CONTRAST:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x40);
 	case V4L2_CID_SATURATION:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x40);
 	case V4L2_CID_HUE:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		return v4l2_ctrl_query_fill(qc, 0x0, 0x1e, 1, 0xf);
 	case V4L2_CID_SHARPNESS:
 		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
 	case V4L2_CID_MPEG_AUDIO_ENCODING:

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Devin Heitmueller]

On Thu, Oct 20, 2011 at 11:24 AM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
> I've attached a patch that correctly sets the max/min/default values
> for the hdpvr picture controls. The reason the current values didn't
> cause a problem until now is because any firmware <= 0.15 didn't
> support them. The latest firmware releases properly support picture
> controls and the values in the patch are derived from the windows
> driver using SniffUSB2.0.
>
> Thanks to Devin Heitmueller for helping me.
>
> Regards.
> --
> Taylor

Hi Taylor,

What worries me here is the assertion that the controls didn't work at
all in previous firmware and driver versions.  Did you downgrade the
firmware and see that the controls had no effect when using v4l2-ctl?

Janne, any comment on whether the controls *ever* worked?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

On Thu, Oct 20, 2011 at 11:30 AM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Thu, Oct 20, 2011 at 11:24 AM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>> I've attached a patch that correctly sets the max/min/default values
>> for the hdpvr picture controls. The reason the current values didn't
>> cause a problem until now is because any firmware <= 0.15 didn't
>> support them. The latest firmware releases properly support picture
>> controls and the values in the patch are derived from the windows
>> driver using SniffUSB2.0.
>>
>> Thanks to Devin Heitmueller for helping me.
>>
>> Regards.
>> --
>> Taylor
>
> Hi Taylor,
>
> What worries me here is the assertion that the controls didn't work at
> all in previous firmware and driver versions.  Did you downgrade the
> firmware and see that the controls had no effect when using v4l2-ctl?
>

I have 2 HD-PVRs. I ran one with 0x17 and one with 0x15. Using
v4l2-ctl to control the 0x15 unit produced zero changes. It has been
reported by mythtv users in the past the picture control changes did
not work for the HD-PVR.

Regards.
--
Taylor

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Janne Grunau]

On Thu, Oct 20, 2011 at 11:30:11AM -0400, Devin Heitmueller wrote:
> On Thu, Oct 20, 2011 at 11:24 AM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
> > I've attached a patch that correctly sets the max/min/default values
> > for the hdpvr picture controls. The reason the current values didn't
> > cause a problem until now is because any firmware <= 0.15 didn't
> > support them. The latest firmware releases properly support picture
> > controls and the values in the patch are derived from the windows
> > driver using SniffUSB2.0.
> >
> > Thanks to Devin Heitmueller for helping me.
> 
> What worries me here is the assertion that the controls didn't work at
> all in previous firmware and driver versions.  Did you downgrade the
> firmware and see that the controls had no effect when using v4l2-ctl?
> 
> Janne, any comment on whether the controls *ever* worked?

I've looked at them only at very beginning and if I recall correctly
they had no visible effects. The values in the linux driver were taken
from sniffing the windows driver. I remember that I've verified the
default brightness value since 0x86 looked odd. I'm not sure that I
verified all controls. I might have assumed all controls shared the
same value range.

There were previous reports of the picture controls not working at all.

Janne

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Devin Heitmueller]

On Thu, Oct 20, 2011 at 12:23 PM, Janne Grunau <janne@jannau.net> wrote:
> On Thu, Oct 20, 2011 at 11:30:11AM -0400, Devin Heitmueller wrote:
>> On Thu, Oct 20, 2011 at 11:24 AM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>> > I've attached a patch that correctly sets the max/min/default values
>> > for the hdpvr picture controls. The reason the current values didn't
>> > cause a problem until now is because any firmware <= 0.15 didn't
>> > support them. The latest firmware releases properly support picture
>> > controls and the values in the patch are derived from the windows
>> > driver using SniffUSB2.0.
>> >
>> > Thanks to Devin Heitmueller for helping me.
>>
>> What worries me here is the assertion that the controls didn't work at
>> all in previous firmware and driver versions.  Did you downgrade the
>> firmware and see that the controls had no effect when using v4l2-ctl?
>>
>> Janne, any comment on whether the controls *ever* worked?
>
> I've looked at them only at very beginning and if I recall correctly
> they had no visible effects. The values in the linux driver were taken
> from sniffing the windows driver. I remember that I've verified the
> default brightness value since 0x86 looked odd. I'm not sure that I
> verified all controls. I might have assumed all controls shared the
> same value range.
>
> There were previous reports of the picture controls not working at all.

Hi Janne,

Thanks for taking the time to chime in.

If the controls really were broken all along under Linux, then that's
good to know.  That said, I'm not confident the changes Taylor
proposed should really be run against older firmwares.  There probably
needs to be a check to have the values in question only applied if
firmware >= 16.  If the controls were broken entirely, then we should
probably not advertise them in ENUM_CTRL and S_CTRL should return
-EINVAL if running the old firmware (perhaps put a warning in the
dmesg output saying the controls are unavailable because the user is
not running firmware >= 16).

My immediate concern is about ensuring we don't cause breakage in
older firmware.  For example, we don't know if there are some older
firmware revisions that *did* work with the driver.  The controls
might have worked up to firmware revision 10, then been broken from
11-15, then work again in 16 (with the new hue value needed).  The
safe approach is to only use these new settings if they're running
firmware >= 16.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Janne Grunau]

On Thu, Oct 20, 2011 at 12:35:40PM -0400, Devin Heitmueller wrote:
> On Thu, Oct 20, 2011 at 12:23 PM, Janne Grunau <janne@jannau.net> wrote:
> >
> > I've looked at them only at very beginning and if I recall correctly
> > they had no visible effects. The values in the linux driver were taken
> > from sniffing the windows driver. I remember that I've verified the
> > default brightness value since 0x86 looked odd. I'm not sure that I
> > verified all controls. I might have assumed all controls shared the
> > same value range.
> >
> > There were previous reports of the picture controls not working at all.
> 
> Thanks for taking the time to chime in.

no problem, sorry for ignoring the other mails, I had no time to look
at the problem immediately and then forgot about it.

> If the controls really were broken all along under Linux, then that's
> good to know.  That said, I'm not confident the changes Taylor
> proposed should really be run against older firmwares.  There probably
> needs to be a check to have the values in question only applied if
> firmware >= 16.  If the controls were broken entirely, then we should
> probably not advertise them in ENUM_CTRL and S_CTRL should return
> -EINVAL if running the old firmware (perhaps put a warning in the
> dmesg output saying the controls are unavailable because the user is
> not running firmware >= 16).
> 
> My immediate concern is about ensuring we don't cause breakage in
> older firmware.  For example, we don't know if there are some older
> firmware revisions that *did* work with the driver.  The controls
> might have worked up to firmware revision 10, then been broken from
> 11-15, then work again in 16 (with the new hue value needed).  The
> safe approach is to only use these new settings if they're running
> firmware >= 16.

I think such scenario is unlikely but I don't know it for sure and
I don't want to force anyone to test every firmware version.
Ignoring them for firmware version < 16 should be safe since we assume
they had no effect. Returning -EINVAL might break API-ignoring
applications written with the HD PVR in mind but I think it's a better
approach than silently ignoring those controls.

Janne

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Devin Heitmueller]

On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
> I think such scenario is unlikely but I don't know it for sure and
> I don't want to force anyone to test every firmware version.
> Ignoring them for firmware version < 16 should be safe since we assume
> they had no effect. Returning -EINVAL might break API-ignoring
> applications written with the HD PVR in mind but I think it's a better
> approach than silently ignoring those controls.

At this point, let's just make it so that the old behavior is
unchanged for old firmwares, meaning from both an API standpoint as
well as what the values are.  At some point if somebody cares enough
to go back and fix the support so that the controls actually work with
old firmwares, they can take that up as a separate task.  In reality,
it is likely that nobody will ever do that, as the "easy answer" is
just to upgrade to firmware 16.

Taylor, could you please tweak your patch to that effect and resubmit?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>> I think such scenario is unlikely but I don't know it for sure and
>> I don't want to force anyone to test every firmware version.
>> Ignoring them for firmware version < 16 should be safe since we assume
>> they had no effect. Returning -EINVAL might break API-ignoring
>> applications written with the HD PVR in mind but I think it's a better
>> approach than silently ignoring those controls.
>
> At this point, let's just make it so that the old behavior is
> unchanged for old firmwares, meaning from both an API standpoint as
> well as what the values are.  At some point if somebody cares enough
> to go back and fix the support so that the controls actually work with
> old firmwares, they can take that up as a separate task.  In reality,
> it is likely that nobody will ever do that, as the "easy answer" is
> just to upgrade to firmware 16.
>
> Taylor, could you please tweak your patch to that effect and resubmit?
>

Sure, I'll try to get to it tonight and have it tested.

Regards.
--
Taylor

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

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

On Thu, Oct 20, 2011 at 3:26 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
> On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
> <dheitmueller@kernellabs.com> wrote:
>> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>>> I think such scenario is unlikely but I don't know it for sure and
>>> I don't want to force anyone to test every firmware version.
>>> Ignoring them for firmware version < 16 should be safe since we assume
>>> they had no effect. Returning -EINVAL might break API-ignoring
>>> applications written with the HD PVR in mind but I think it's a better
>>> approach than silently ignoring those controls.
>>
>> At this point, let's just make it so that the old behavior is
>> unchanged for old firmwares, meaning from both an API standpoint as
>> well as what the values are.  At some point if somebody cares enough
>> to go back and fix the support so that the controls actually work with
>> old firmwares, they can take that up as a separate task.  In reality,
>> it is likely that nobody will ever do that, as the "easy answer" is
>> just to upgrade to firmware 16.
>>
>> Taylor, could you please tweak your patch to that effect and resubmit?
>>
>
> Sure, I'll try to get to it tonight and have it tested.
>

OK, I've updated the patch per your requests. I made this patch
against the latest kernel source but I'm unable to test since my
2.6.32 kernel has symbol issues with the new v4l code.

Regards.
--
Taylor

[-- Attachment #2: hdpvr_v2.diff --]
[-- Type: text/x-patch, Size: 4241 bytes --]

diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
index 441dacf..687282d 100644
--- a/drivers/media/video/hdpvr/hdpvr-core.c
+++ b/drivers/media/video/hdpvr/hdpvr-core.c
@@ -154,10 +154,20 @@ static int device_authorization(struct hdpvr_device *dev)
 	}
 #endif
 
+	dev->fw_ver = dev->usbc_buf[1];
+
 	v4l2_info(&dev->v4l2_dev, "firmware version 0x%x dated %s\n",
-			  dev->usbc_buf[1], &dev->usbc_buf[2]);
+			  dev->fw_ver, &dev->usbc_buf[2]);
+
+	if (dev->fw_ver > 0x15) {
+		dev->options.brightness	= 0x80;
+		dev->options.contrast	= 0x40;
+		dev->options.hue	= 0xf;
+		dev->options.saturation	= 0x40;
+		dev->options.sharpness	= 0x80;
+	}
 
-	switch (dev->usbc_buf[1]) {
+	switch (dev->fw_ver) {
 	case HDPVR_FIRMWARE_VERSION:
 		dev->flags &= ~HDPVR_FLAG_AC3_CAP;
 		break;
@@ -169,7 +179,7 @@ static int device_authorization(struct hdpvr_device *dev)
 	default:
 		v4l2_info(&dev->v4l2_dev, "untested firmware, the driver might"
 			  " not work.\n");
-		if (dev->usbc_buf[1] >= HDPVR_FIRMWARE_VERSION_AC3)
+		if (dev->fw_ver >= HDPVR_FIRMWARE_VERSION_AC3)
 			dev->flags |= HDPVR_FLAG_AC3_CAP;
 		else
 			dev->flags &= ~HDPVR_FLAG_AC3_CAP;
@@ -270,6 +280,8 @@ static const struct hdpvr_options hdpvr_default_options = {
 	.bitrate_mode	= HDPVR_CONSTANT,
 	.gop_mode	= HDPVR_SIMPLE_IDR_GOP,
 	.audio_codec	= V4L2_MPEG_AUDIO_ENCODING_AAC,
+	/* original picture controls for firmware version <= 0x15 */
+	/* updated in device_authorization() for newer firmware */
 	.brightness	= 0x86,
 	.contrast	= 0x80,
 	.hue		= 0x80,
diff --git a/drivers/media/video/hdpvr/hdpvr-video.c b/drivers/media/video/hdpvr/hdpvr-video.c
index 087f7c0..36bb057 100644
--- a/drivers/media/video/hdpvr/hdpvr-video.c
+++ b/drivers/media/video/hdpvr/hdpvr-video.c
@@ -722,21 +722,39 @@ static const s32 supported_v4l2_ctrls[] = {
 };
 
 static int fill_queryctrl(struct hdpvr_options *opt, struct v4l2_queryctrl *qc,
-			  int ac3)
+			  int ac3, int fw_ver)
 {
 	int err;
 
+	if (fw_ver > 0x15) {
+		switch (qc->id) {
+		case V4L2_CID_BRIGHTNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_CONTRAST:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x40);
+		case V4L2_CID_SATURATION:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x40);
+		case V4L2_CID_HUE:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0x1e, 1, 0xf);
+		case V4L2_CID_SHARPNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		}
+	} else {
+		switch (qc->id) {
+		case V4L2_CID_BRIGHTNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x86);
+		case V4L2_CID_CONTRAST:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_SATURATION:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_HUE:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_SHARPNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		}
+	}
+
 	switch (qc->id) {
-	case V4L2_CID_BRIGHTNESS:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x86);
-	case V4L2_CID_CONTRAST:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
-	case V4L2_CID_SATURATION:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
-	case V4L2_CID_HUE:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
-	case V4L2_CID_SHARPNESS:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
 	case V4L2_CID_MPEG_AUDIO_ENCODING:
 		return v4l2_ctrl_query_fill(
 			qc, V4L2_MPEG_AUDIO_ENCODING_AAC,
@@ -794,7 +812,8 @@ static int vidioc_queryctrl(struct file *file, void *private_data,
 
 		if (qc->id == supported_v4l2_ctrls[i])
 			return fill_queryctrl(&dev->options, qc,
-					      dev->flags & HDPVR_FLAG_AC3_CAP);
+					      dev->flags & HDPVR_FLAG_AC3_CAP,
+					      dev->fw_ver);
 
 		if (qc->id < supported_v4l2_ctrls[i])
 			break;
diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
index d6439db..fea3c69 100644
--- a/drivers/media/video/hdpvr/hdpvr.h
+++ b/drivers/media/video/hdpvr/hdpvr.h
@@ -113,6 +113,7 @@ struct hdpvr_device {
 	/* usb control transfer buffer and lock */
 	struct mutex		usbc_mutex;
 	u8			*usbc_buf;
+	u8			fw_ver;
 };
 
 static inline struct hdpvr_device *to_hdpvr_dev(struct v4l2_device *v4l2_dev)

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Mauro Carvalho Chehab]

Em 21-10-2011 01:33, Taylor Ralph escreveu:
> On Thu, Oct 20, 2011 at 3:26 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>> On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
>> <dheitmueller@kernellabs.com> wrote:
>>> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>>>> I think such scenario is unlikely but I don't know it for sure and
>>>> I don't want to force anyone to test every firmware version.
>>>> Ignoring them for firmware version < 16 should be safe since we assume
>>>> they had no effect. Returning -EINVAL might break API-ignoring
>>>> applications written with the HD PVR in mind but I think it's a better
>>>> approach than silently ignoring those controls.
>>>
>>> At this point, let's just make it so that the old behavior is
>>> unchanged for old firmwares, meaning from both an API standpoint as
>>> well as what the values are.  At some point if somebody cares enough
>>> to go back and fix the support so that the controls actually work with
>>> old firmwares, they can take that up as a separate task.  In reality,
>>> it is likely that nobody will ever do that, as the "easy answer" is
>>> just to upgrade to firmware 16.
>>>
>>> Taylor, could you please tweak your patch to that effect and resubmit?
>>>
>>
>> Sure, I'll try to get to it tonight and have it tested.
>>
> 
> OK, I've updated the patch per your requests. I made this patch
> against the latest kernel source but I'm unable to test since my
> 2.6.32 kernel has symbol issues with the new v4l code.

Please, add your Signed-off-by: to the patch. This is a requirement for
it to be accepted upstream[1].

Thanks,
Mauro

[1] See: http://linuxtv.org/wiki/index.php/Development:_Submitting_Patches#Developer.27s_Certificate_of_Origin_1.1

> 
> Regards.
> --
> Taylor

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

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

On Mon, Nov 7, 2011 at 7:21 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 21-10-2011 01:33, Taylor Ralph escreveu:
>> On Thu, Oct 20, 2011 at 3:26 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>>> On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
>>> <dheitmueller@kernellabs.com> wrote:
>>>> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>>>>> I think such scenario is unlikely but I don't know it for sure and
>>>>> I don't want to force anyone to test every firmware version.
>>>>> Ignoring them for firmware version < 16 should be safe since we assume
>>>>> they had no effect. Returning -EINVAL might break API-ignoring
>>>>> applications written with the HD PVR in mind but I think it's a better
>>>>> approach than silently ignoring those controls.
>>>>
>>>> At this point, let's just make it so that the old behavior is
>>>> unchanged for old firmwares, meaning from both an API standpoint as
>>>> well as what the values are.  At some point if somebody cares enough
>>>> to go back and fix the support so that the controls actually work with
>>>> old firmwares, they can take that up as a separate task.  In reality,
>>>> it is likely that nobody will ever do that, as the "easy answer" is
>>>> just to upgrade to firmware 16.
>>>>
>>>> Taylor, could you please tweak your patch to that effect and resubmit?
>>>>
>>>
>>> Sure, I'll try to get to it tonight and have it tested.
>>>
>>
>> OK, I've updated the patch per your requests. I made this patch
>> against the latest kernel source but I'm unable to test since my
>> 2.6.32 kernel has symbol issues with the new v4l code.
>
> Please, add your Signed-off-by: to the patch. This is a requirement for
> it to be accepted upstream[1].
>
> Thanks,
> Mauro
>
> [1] See: http://linuxtv.org/wiki/index.php/Development:_Submitting_Patches#Developer.27s_Certificate_of_Origin_1.1
>
>>
>> Regards.
>> --
>> Taylor
>
>

Sorry about that. The updated patch is attached.

Thanks.
--
Taylor

[-- Attachment #2: hdpvr_v3.diff --]
[-- Type: application/octet-stream, Size: 4290 bytes --]

diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
index 441dacf..687282d 100644
--- a/drivers/media/video/hdpvr/hdpvr-core.c
+++ b/drivers/media/video/hdpvr/hdpvr-core.c
@@ -154,10 +154,20 @@ static int device_authorization(struct hdpvr_device *dev)
 	}
 #endif
 
+	dev->fw_ver = dev->usbc_buf[1];
+
 	v4l2_info(&dev->v4l2_dev, "firmware version 0x%x dated %s\n",
-			  dev->usbc_buf[1], &dev->usbc_buf[2]);
+			  dev->fw_ver, &dev->usbc_buf[2]);
+
+	if (dev->fw_ver > 0x15) {
+		dev->options.brightness	= 0x80;
+		dev->options.contrast	= 0x40;
+		dev->options.hue	= 0xf;
+		dev->options.saturation	= 0x40;
+		dev->options.sharpness	= 0x80;
+	}
 
-	switch (dev->usbc_buf[1]) {
+	switch (dev->fw_ver) {
 	case HDPVR_FIRMWARE_VERSION:
 		dev->flags &= ~HDPVR_FLAG_AC3_CAP;
 		break;
@@ -169,7 +179,7 @@ static int device_authorization(struct hdpvr_device *dev)
 	default:
 		v4l2_info(&dev->v4l2_dev, "untested firmware, the driver might"
 			  " not work.\n");
-		if (dev->usbc_buf[1] >= HDPVR_FIRMWARE_VERSION_AC3)
+		if (dev->fw_ver >= HDPVR_FIRMWARE_VERSION_AC3)
 			dev->flags |= HDPVR_FLAG_AC3_CAP;
 		else
 			dev->flags &= ~HDPVR_FLAG_AC3_CAP;
@@ -270,6 +280,8 @@ static const struct hdpvr_options hdpvr_default_options = {
 	.bitrate_mode	= HDPVR_CONSTANT,
 	.gop_mode	= HDPVR_SIMPLE_IDR_GOP,
 	.audio_codec	= V4L2_MPEG_AUDIO_ENCODING_AAC,
+	/* original picture controls for firmware version <= 0x15 */
+	/* updated in device_authorization() for newer firmware */
 	.brightness	= 0x86,
 	.contrast	= 0x80,
 	.hue		= 0x80,
diff --git a/drivers/media/video/hdpvr/hdpvr-video.c b/drivers/media/video/hdpvr/hdpvr-video.c
index 087f7c0..36bb057 100644
--- a/drivers/media/video/hdpvr/hdpvr-video.c
+++ b/drivers/media/video/hdpvr/hdpvr-video.c
@@ -722,21 +722,39 @@ static const s32 supported_v4l2_ctrls[] = {
 };
 
 static int fill_queryctrl(struct hdpvr_options *opt, struct v4l2_queryctrl *qc,
-			  int ac3)
+			  int ac3, int fw_ver)
 {
 	int err;
 
+	if (fw_ver > 0x15) {
+		switch (qc->id) {
+		case V4L2_CID_BRIGHTNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_CONTRAST:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x40);
+		case V4L2_CID_SATURATION:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x40);
+		case V4L2_CID_HUE:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0x1e, 1, 0xf);
+		case V4L2_CID_SHARPNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		}
+	} else {
+		switch (qc->id) {
+		case V4L2_CID_BRIGHTNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x86);
+		case V4L2_CID_CONTRAST:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_SATURATION:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_HUE:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		case V4L2_CID_SHARPNESS:
+			return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
+		}
+	}
+
 	switch (qc->id) {
-	case V4L2_CID_BRIGHTNESS:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x86);
-	case V4L2_CID_CONTRAST:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
-	case V4L2_CID_SATURATION:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
-	case V4L2_CID_HUE:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
-	case V4L2_CID_SHARPNESS:
-		return v4l2_ctrl_query_fill(qc, 0x0, 0xff, 1, 0x80);
 	case V4L2_CID_MPEG_AUDIO_ENCODING:
 		return v4l2_ctrl_query_fill(
 			qc, V4L2_MPEG_AUDIO_ENCODING_AAC,
@@ -794,7 +812,8 @@ static int vidioc_queryctrl(struct file *file, void *private_data,
 
 		if (qc->id == supported_v4l2_ctrls[i])
 			return fill_queryctrl(&dev->options, qc,
-					      dev->flags & HDPVR_FLAG_AC3_CAP);
+					      dev->flags & HDPVR_FLAG_AC3_CAP,
+					      dev->fw_ver);
 
 		if (qc->id < supported_v4l2_ctrls[i])
 			break;
diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
index d6439db..fea3c69 100644
--- a/drivers/media/video/hdpvr/hdpvr.h
+++ b/drivers/media/video/hdpvr/hdpvr.h
@@ -113,6 +113,7 @@ struct hdpvr_device {
 	/* usb control transfer buffer and lock */
 	struct mutex		usbc_mutex;
 	u8			*usbc_buf;
+	u8			fw_ver;
 };
 
 static inline struct hdpvr_device *to_hdpvr_dev(struct v4l2_device *v4l2_dev)

Signed-off-by: Taylor Ralph <tralph@mythtv.org>

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

On Mon, Nov 7, 2011 at 7:54 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
> On Mon, Nov 7, 2011 at 7:21 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 21-10-2011 01:33, Taylor Ralph escreveu:
>>> On Thu, Oct 20, 2011 at 3:26 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>>>> On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
>>>> <dheitmueller@kernellabs.com> wrote:
>>>>> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>>>>>> I think such scenario is unlikely but I don't know it for sure and
>>>>>> I don't want to force anyone to test every firmware version.
>>>>>> Ignoring them for firmware version < 16 should be safe since we assume
>>>>>> they had no effect. Returning -EINVAL might break API-ignoring
>>>>>> applications written with the HD PVR in mind but I think it's a better
>>>>>> approach than silently ignoring those controls.
>>>>>
>>>>> At this point, let's just make it so that the old behavior is
>>>>> unchanged for old firmwares, meaning from both an API standpoint as
>>>>> well as what the values are.  At some point if somebody cares enough
>>>>> to go back and fix the support so that the controls actually work with
>>>>> old firmwares, they can take that up as a separate task.  In reality,
>>>>> it is likely that nobody will ever do that, as the "easy answer" is
>>>>> just to upgrade to firmware 16.
>>>>>
>>>>> Taylor, could you please tweak your patch to that effect and resubmit?
>>>>>
>>>>
>>>> Sure, I'll try to get to it tonight and have it tested.
>>>>
>>>
>>> OK, I've updated the patch per your requests. I made this patch
>>> against the latest kernel source but I'm unable to test since my
>>> 2.6.32 kernel has symbol issues with the new v4l code.
>>
>> Please, add your Signed-off-by: to the patch. This is a requirement for
>> it to be accepted upstream[1].
>>
>> Thanks,
>> Mauro
>>
>> [1] See: http://linuxtv.org/wiki/index.php/Development:_Submitting_Patches#Developer.27s_Certificate_of_Origin_1.1
>>
>>>
>>> Regards.
>>> --
>>> Taylor
>>
>>
>
> Sorry about that. The updated patch is attached.
>
> Thanks.
> --
> Taylor

Ping. Has this patch been committed yet?

Thanks.
--
Taylor

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Taylor Ralph]

On Wed, Dec 21, 2011 at 5:14 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
> On Mon, Nov 7, 2011 at 7:54 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>> On Mon, Nov 7, 2011 at 7:21 AM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>>> Em 21-10-2011 01:33, Taylor Ralph escreveu:
>>>> On Thu, Oct 20, 2011 at 3:26 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>>>>> On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
>>>>> <dheitmueller@kernellabs.com> wrote:
>>>>>> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>>>>>>> I think such scenario is unlikely but I don't know it for sure and
>>>>>>> I don't want to force anyone to test every firmware version.
>>>>>>> Ignoring them for firmware version < 16 should be safe since we assume
>>>>>>> they had no effect. Returning -EINVAL might break API-ignoring
>>>>>>> applications written with the HD PVR in mind but I think it's a better
>>>>>>> approach than silently ignoring those controls.
>>>>>>
>>>>>> At this point, let's just make it so that the old behavior is
>>>>>> unchanged for old firmwares, meaning from both an API standpoint as
>>>>>> well as what the values are.  At some point if somebody cares enough
>>>>>> to go back and fix the support so that the controls actually work with
>>>>>> old firmwares, they can take that up as a separate task.  In reality,
>>>>>> it is likely that nobody will ever do that, as the "easy answer" is
>>>>>> just to upgrade to firmware 16.
>>>>>>
>>>>>> Taylor, could you please tweak your patch to that effect and resubmit?
>>>>>>
>>>>>
>>>>> Sure, I'll try to get to it tonight and have it tested.
>>>>>
>>>>
>>>> OK, I've updated the patch per your requests. I made this patch
>>>> against the latest kernel source but I'm unable to test since my
>>>> 2.6.32 kernel has symbol issues with the new v4l code.
>>>
>>> Please, add your Signed-off-by: to the patch. This is a requirement for
>>> it to be accepted upstream[1].
>>>
>>> Thanks,
>>> Mauro
>>>
>>> [1] See: http://linuxtv.org/wiki/index.php/Development:_Submitting_Patches#Developer.27s_Certificate_of_Origin_1.1
>>>
>>>>
>>>> Regards.
>>>> --
>>>> Taylor
>>>
>>>
>>
>> Sorry about that. The updated patch is attached.
>>
>> Thanks.
>> --
>> Taylor
>
> Ping. Has this patch been committed yet?
>

PING. Again, has the patch been committed? It's needed for new
firmware that is shipping from Hauppauge.

Thanks.
--
Taylor

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Jarod Wilson]

On Mon, Nov 7, 2011 at 7:54 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
> On Mon, Nov 7, 2011 at 7:21 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 21-10-2011 01:33, Taylor Ralph escreveu:
>>> On Thu, Oct 20, 2011 at 3:26 PM, Taylor Ralph <taylor.ralph@gmail.com> wrote:
>>>> On Thu, Oct 20, 2011 at 2:14 PM, Devin Heitmueller
>>>> <dheitmueller@kernellabs.com> wrote:
>>>>> On Thu, Oct 20, 2011 at 1:08 PM, Janne Grunau <j@jannau.net> wrote:
>>>>>> I think such scenario is unlikely but I don't know it for sure and
>>>>>> I don't want to force anyone to test every firmware version.
>>>>>> Ignoring them for firmware version < 16 should be safe since we assume
>>>>>> they had no effect. Returning -EINVAL might break API-ignoring
>>>>>> applications written with the HD PVR in mind but I think it's a better
>>>>>> approach than silently ignoring those controls.
>>>>>
>>>>> At this point, let's just make it so that the old behavior is
>>>>> unchanged for old firmwares, meaning from both an API standpoint as
>>>>> well as what the values are.  At some point if somebody cares enough
>>>>> to go back and fix the support so that the controls actually work with
>>>>> old firmwares, they can take that up as a separate task.  In reality,
>>>>> it is likely that nobody will ever do that, as the "easy answer" is
>>>>> just to upgrade to firmware 16.
>>>>>
>>>>> Taylor, could you please tweak your patch to that effect and resubmit?
>>>>>
>>>>
>>>> Sure, I'll try to get to it tonight and have it tested.

Looks sane to me, and really needs to get in ASAP. I'd even suggest we
get it sent to stable, as these newer firmware HDPVR are pretty wonky
with any current kernel.

Acked-by: Jarod Wilson <jarod@redhat.com>
Reviewed-by: Jarod Wilson <jarod@redhat.com>
CC: stable@vger.kernel.org

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Devin Heitmueller]

On Tue, Feb 14, 2012 at 3:43 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
> Looks sane to me, and really needs to get in ASAP. I'd even suggest we
> get it sent to stable, as these newer firmware HDPVR are pretty wonky
> with any current kernel.
>
> Acked-by: Jarod Wilson <jarod@redhat.com>
> Reviewed-by: Jarod Wilson <jarod@redhat.com>
> CC: stable@vger.kernel.org

Where did the process break down here?  Taylor did this patch *months*
ago, and there has been absolutely no comment with why it wouldn't go
upstream.  If he hadn't been diligent in pinging the ML repeatedly, it
would have been lost.

Are there other patches that have hit the ML that aren't getting upstream?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15

[Jarod Wilson]

On Tue, Feb 14, 2012 at 4:32 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Tue, Feb 14, 2012 at 3:43 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
>> Looks sane to me, and really needs to get in ASAP. I'd even suggest we
>> get it sent to stable, as these newer firmware HDPVR are pretty wonky
>> with any current kernel.
>>
>> Acked-by: Jarod Wilson <jarod@redhat.com>
>> Reviewed-by: Jarod Wilson <jarod@redhat.com>
>> CC: stable@vger.kernel.org
>
> Where did the process break down here?  Taylor did this patch *months*
> ago, and there has been absolutely no comment with why it wouldn't go
> upstream.  If he hadn't been diligent in pinging the ML repeatedly, it
> would have been lost.

It looks like for some reason, the v3 patch got eaten. :\

http://patchwork.linuxtv.org/patch/8183/ is the v2, in state Changes
Requested, but you can see in the comments a mail that says v3 is
attached, which contains the requested change (added s-o-b). A v3
patch object is nowhere to be found though. The patch *was* indeed
attached to the mail though, I've got it here in my linux-media
mailbox.

So at least on this one, I think I'm blaming patchwork, but it would
be good to better understand how that patch got eaten, and to know if
indeed its happened to other patches as well.

-- 
Jarod Wilson
jarod@wilsonet.com

missing patches in patchwork (was Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15)

[Janne Grunau]

On 2012-02-14 17:09:55 -0500, Jarod Wilson wrote:
> On Tue, Feb 14, 2012 at 4:32 PM, Devin Heitmueller
> <dheitmueller@kernellabs.com> wrote:
> > On Tue, Feb 14, 2012 at 3:43 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
> >> Looks sane to me, and really needs to get in ASAP. I'd even suggest we
> >> get it sent to stable, as these newer firmware HDPVR are pretty wonky
> >> with any current kernel.
> >>
> >> Acked-by: Jarod Wilson <jarod@redhat.com>
> >> Reviewed-by: Jarod Wilson <jarod@redhat.com>
> >> CC: stable@vger.kernel.org
> >
> > Where did the process break down here?  Taylor did this patch *months*
> > ago, and there has been absolutely no comment with why it wouldn't go
> > upstream.  If he hadn't been diligent in pinging the ML repeatedly, it
> > would have been lost.
> 
> It looks like for some reason, the v3 patch got eaten. :\
> 
> http://patchwork.linuxtv.org/patch/8183/ is the v2, in state Changes
> Requested, but you can see in the comments a mail that says v3 is
> attached, which contains the requested change (added s-o-b). A v3
> patch object is nowhere to be found though. The patch *was* indeed
> attached to the mail though, I've got it here in my linux-media
> mailbox.
> 
> So at least on this one, I think I'm blaming patchwork, but it would
> be good to better understand how that patch got eaten, and to know if
> indeed its happened to other patches as well.

Patchwork ignored the patch because of its mime type. Patchwork only 
handles text/{x-patch,x-diff,plain} but the v3 patch was attached as
application/octet-stream.

I have a clumsy patch to handle application/octet-stream for libav's
patchwork instance. I'll try to find time to clean it up and submit it
upstream.

Janne

Re: missing patches in patchwork (was Re: [PATCH] [media] hdpvr: update picture controls to support firmware versions > 0.15)

[Mauro Carvalho Chehab]

Em 15-02-2012 09:46, Janne Grunau escreveu:
> On 2012-02-14 17:09:55 -0500, Jarod Wilson wrote:
>> On Tue, Feb 14, 2012 at 4:32 PM, Devin Heitmueller
>> <dheitmueller@kernellabs.com> wrote:
>>> On Tue, Feb 14, 2012 at 3:43 PM, Jarod Wilson <jarod@wilsonet.com> wrote:
>>>> Looks sane to me, and really needs to get in ASAP. I'd even suggest we
>>>> get it sent to stable, as these newer firmware HDPVR are pretty wonky
>>>> with any current kernel.
>>>>
>>>> Acked-by: Jarod Wilson <jarod@redhat.com>
>>>> Reviewed-by: Jarod Wilson <jarod@redhat.com>
>>>> CC: stable@vger.kernel.org
>>>
>>> Where did the process break down here?  Taylor did this patch *months*
>>> ago, and there has been absolutely no comment with why it wouldn't go
>>> upstream.  If he hadn't been diligent in pinging the ML repeatedly, it
>>> would have been lost.
>>
>> It looks like for some reason, the v3 patch got eaten. :\
>>
>> http://patchwork.linuxtv.org/patch/8183/ is the v2, in state Changes
>> Requested, but you can see in the comments a mail that says v3 is
>> attached, which contains the requested change (added s-o-b). A v3
>> patch object is nowhere to be found though. The patch *was* indeed
>> attached to the mail though, I've got it here in my linux-media
>> mailbox.
>>
>> So at least on this one, I think I'm blaming patchwork, but it would
>> be good to better understand how that patch got eaten, and to know if
>> indeed its happened to other patches as well.
> 
> Patchwork ignored the patch because of its mime type. Patchwork only 
> handles text/{x-patch,x-diff,plain} but the v3 patch was attached as
> application/octet-stream.

Yeah, octect-stream should be used only for binary files. Patchwork discards
it, as it doesn't make sense to try to parse a binary stuff. Btw, most ML's
simply discard emails with octect-stream, as they could offer a security
threat, as malicious code could be there, affecting the html logs. Even when
they don't discard, it is typical that the html public ML archives to discard
such emails, due to the same reason.

> 
> I have a clumsy patch to handle application/octet-stream for libav's
> patchwork instance. I'll try to find time to clean it up and submit it
> upstream.
> 
> Janne