[PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Devin Heitmueller]

The following patch addresses the regression introduced in the cx231xx
driver which stopped the Hauppauge USBLive2 from working.

Confirmed working by both myself and the user who reported the issue
on the KernelLabs blog (Robert DeLuca).

cx231xx: Fix regression introduced which broke the Hauppauge USBLive 2

From: Devin Heitmueller <dheitmueller@kernellabs.com>

At some point during refactoring of the cx231xx driver, the USBLive 2 device
became broken.  This patch results in the device working again.

Thanks to Robert DeLuca for sponsoring this work.

Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
Cc: Robert DeLuca <robertdeluca@me.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-cards.c
b/drivers/media/video/cx231xx/cx231xx-cards.c
index 4b22afe..d02c63a 100644
--- a/drivers/media/video/cx231xx/cx231xx-cards.c
+++ b/drivers/media/video/cx231xx/cx231xx-cards.c
@@ -387,6 +387,7 @@ struct cx231xx_board cx231xx_boards[] = {
 		.norm = V4L2_STD_NTSC,
 		.no_alt_vanc = 1,
 		.external_av = 1,
+		.dont_use_port_3 = 1,
 		.input = {{
 			.type = CX231XX_VMUX_COMPOSITE1,
 			.vmux = CX231XX_VIN_2_1,

Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Mauro Carvalho Chehab]

Hi Devin,

Em 23-07-2011 22:17, Devin Heitmueller escreveu:
> The following patch addresses the regression introduced in the cx231xx
> driver which stopped the Hauppauge USBLive2 from working.
> 
> Confirmed working by both myself and the user who reported the issue
> on the KernelLabs blog (Robert DeLuca).
> 
> cx231xx: Fix regression introduced which broke the Hauppauge USBLive 2
> 
> From: Devin Heitmueller <dheitmueller@kernellabs.com>
> 
> At some point during refactoring of the cx231xx driver, the USBLive 2 device
> became broken.  This patch results in the device working again.
> 
> Thanks to Robert DeLuca for sponsoring this work.
> 
> Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> Cc: Robert DeLuca <robertdeluca@me.com>
> 
> diff --git a/drivers/media/video/cx231xx/cx231xx-cards.c
> b/drivers/media/video/cx231xx/cx231xx-cards.c
> index 4b22afe..d02c63a 100644
> --- a/drivers/media/video/cx231xx/cx231xx-cards.c
> +++ b/drivers/media/video/cx231xx/cx231xx-cards.c
> @@ -387,6 +387,7 @@ struct cx231xx_board cx231xx_boards[] = {
>  		.norm = V4L2_STD_NTSC,
>  		.no_alt_vanc = 1,
>  		.external_av = 1,
> +		.dont_use_port_3 = 1,
>  		.input = {{
>  			.type = CX231XX_VMUX_COMPOSITE1,
>  			.vmux = CX231XX_VIN_2_1,

I proposed the same fix sometime ago, when Gerd reported this issue
for me. His feedback was that this partially fixed the issue, but
he reported that he also needed to increase the set_power_mode delay
from 5 to 50 ms in order to fully initialize the cx231xx hardware,
as on the enclosed patch. It seems he tested with vanilla Fedora kernel.

So, I suspect that HZ may be affecting this driver as well. As you know,
if HZ is configured with 100, the minimum msleep() delay will be 10.
so, instead of waiting for 5ms, it will wait for 10ms for the device
to powerup.

It would be great to configure HZ with 1000 and see the differences with 
and without Gerd's patch.

Cheers,
Mauro.

-

>From a83a7574e07b48b1c6a222d833a7fa0a67133b5c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@gmail.com>
Date: Thu, 16 Dec 2010 17:39:17 +0100
Subject: [PATCH] cx231xx: raise delay after powerup.

Wait a bit longer after power up so the chips have enougth
time to come up before we try talking to them via i2c.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/media/video/cx231xx/cx231xx-avcore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/cx231xx/cx231xx-avcore.c b/drivers/media/video/cx231xx/cx231xx-avcore.c
index cf50faf..cf412cd 100644
--- a/drivers/media/video/cx231xx/cx231xx-avcore.c
+++ b/drivers/media/video/cx231xx/cx231xx-avcore.c
@@ -2412,7 +2412,7 @@ int cx231xx_set_power_mode(struct cx231xx *dev, enum AV_MODE mode)
 		break;
 	}
 
-	msleep(PWR_SLEEP_INTERVAL);
+	msleep(PWR_SLEEP_INTERVAL * 10);
 
 	/* For power saving, only enable Pwr_resetout_n
 	   when digital TV is selected. */
-- 

Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Devin Heitmueller]

On Sun, Jul 24, 2011 at 8:57 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> I proposed the same fix sometime ago, when Gerd reported this issue
> for me. His feedback was that this partially fixed the issue, but
> he reported that he also needed to increase the set_power_mode delay
> from 5 to 50 ms in order to fully initialize the cx231xx hardware,
> as on the enclosed patch. It seems he tested with vanilla Fedora kernel.
>
> So, I suspect that HZ may be affecting this driver as well. As you know,
> if HZ is configured with 100, the minimum msleep() delay will be 10.
> so, instead of waiting for 5ms, it will wait for 10ms for the device
> to powerup.
>
> It would be great to configure HZ with 1000 and see the differences with
> and without Gerd's patch.
>
> Cheers,
> Mauro.

I don't dispute the possibility that there is some *other* bug that
effects users who have some other value for HZ, but neither I nor the
other use saw it.  Without this patch though, the device is broken for
*everybody*.

I would suggest checking in this patch, and separately the HZ issue
can be investigated.

I'll see if I can find some cycles today to reconfigure my kernel with
a different HZ.  Will also check the datasheets and see if Conexant
documented any sort of time for power ramping.  It's not uncommon for
such documentation to include a diagram showing timing for power up.

Devin

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

Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Devin Heitmueller]

On Sun, Jul 24, 2011 at 9:02 AM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> I don't dispute the possibility that there is some *other* bug that
> effects users who have some other value for HZ, but neither I nor the
> other use saw it.  Without this patch though, the device is broken for
> *everybody*.
>
> I would suggest checking in this patch, and separately the HZ issue
> can be investigated.
>
> I'll see if I can find some cycles today to reconfigure my kernel with
> a different HZ.  Will also check the datasheets and see if Conexant
> documented any sort of time for power ramping.  It's not uncommon for
> such documentation to include a diagram showing timing for power up.

Reconfigured CONFIG_HZ to 1000 and indeed I am seeing the problem.
Will try to track down the correct fix this afternoon.

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

RE: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Palash Bandyopadhyay]

Mauro/Devin,

  Can someone give steps to reproduce the problem? Also if we need any particular h/w board to reproduce this problem. I dont seem to recall any delay requirement on the chip at power up/cycle time. Any I also dont recall seeing any problems with the Conexant evk boards. Mauro, have you been able to see this issue with a Conexant board?

Thanks,
Palash
________________________________________
From: Mauro Carvalho Chehab [mchehab@redhat.com]
Sent: Sunday, July 24, 2011 5:57 AM
To: Devin Heitmueller
Cc: Linux Media Mailing List; Gerd Hoffmann; Sri Deevi; Palash Bandyopadhyay
Subject: Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

Hi Devin,

Em 23-07-2011 22:17, Devin Heitmueller escreveu:
> The following patch addresses the regression introduced in the cx231xx
> driver which stopped the Hauppauge USBLive2 from working.
>
> Confirmed working by both myself and the user who reported the issue
> on the KernelLabs blog (Robert DeLuca).
>
> cx231xx: Fix regression introduced which broke the Hauppauge USBLive 2
>
> From: Devin Heitmueller <dheitmueller@kernellabs.com>
>
> At some point during refactoring of the cx231xx driver, the USBLive 2 device
> became broken.  This patch results in the device working again.
>
> Thanks to Robert DeLuca for sponsoring this work.
>
> Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com>
> Cc: Robert DeLuca <robertdeluca@me.com>
>
> diff --git a/drivers/media/video/cx231xx/cx231xx-cards.c
> b/drivers/media/video/cx231xx/cx231xx-cards.c
> index 4b22afe..d02c63a 100644
> --- a/drivers/media/video/cx231xx/cx231xx-cards.c
> +++ b/drivers/media/video/cx231xx/cx231xx-cards.c
> @@ -387,6 +387,7 @@ struct cx231xx_board cx231xx_boards[] = {
>               .norm = V4L2_STD_NTSC,
>               .no_alt_vanc = 1,
>               .external_av = 1,
> +             .dont_use_port_3 = 1,
>               .input = {{
>                       .type = CX231XX_VMUX_COMPOSITE1,
>                       .vmux = CX231XX_VIN_2_1,

I proposed the same fix sometime ago, when Gerd reported this issue
for me. His feedback was that this partially fixed the issue, but
he reported that he also needed to increase the set_power_mode delay
from 5 to 50 ms in order to fully initialize the cx231xx hardware,
as on the enclosed patch. It seems he tested with vanilla Fedora kernel.

So, I suspect that HZ may be affecting this driver as well. As you know,
if HZ is configured with 100, the minimum msleep() delay will be 10.
so, instead of waiting for 5ms, it will wait for 10ms for the device
to powerup.

It would be great to configure HZ with 1000 and see the differences with
and without Gerd's patch.

Cheers,
Mauro.

-

>From a83a7574e07b48b1c6a222d833a7fa0a67133b5c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@gmail.com>
Date: Thu, 16 Dec 2010 17:39:17 +0100
Subject: [PATCH] cx231xx: raise delay after powerup.

Wait a bit longer after power up so the chips have enougth
time to come up before we try talking to them via i2c.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/media/video/cx231xx/cx231xx-avcore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/cx231xx/cx231xx-avcore.c b/drivers/media/video/cx231xx/cx231xx-avcore.c
index cf50faf..cf412cd 100644
--- a/drivers/media/video/cx231xx/cx231xx-avcore.c
+++ b/drivers/media/video/cx231xx/cx231xx-avcore.c
@@ -2412,7 +2412,7 @@ int cx231xx_set_power_mode(struct cx231xx *dev, enum AV_MODE mode)
                break;
        }

-       msleep(PWR_SLEEP_INTERVAL);
+       msleep(PWR_SLEEP_INTERVAL * 10);

        /* For power saving, only enable Pwr_resetout_n
           when digital TV is selected. */
--
Conexant E-mail Firewall (Conexant.Com) made the following annotations
---------------------------------------------------------------------
********************** Legal Disclaimer **************************** 

"This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you." 

********************************************************************** 

---------------------------------------------------------------------

Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Devin Heitmueller]

On Mon, Jul 25, 2011 at 1:48 AM, Palash Bandyopadhyay
<Palash.Bandyopadhyay@conexant.com> wrote:
> Mauro/Devin,
>
>  Can someone give steps to reproduce the problem? Also if we need any particular h/w board to reproduce this problem. I dont seem to recall any delay requirement on the chip at power up/cycle time. Any I also dont recall seeing any problems with the Conexant evk boards. Mauro, have you been able to see this issue with a Conexant board?

Hello Palash,

The problem occurs if the kernel is configured for CONFIG_HZ to 1000
and on the Hauppauge USBLIve 2 hardware design.  I'm assuming it's
tied to two factors:

1.  Some aspect of the power supply tied to the Mako core being a
little slow to startup.  I haven't compared the schematic to the EVK,
but I could do that if we *really* think that is a worthwhile
exercise.
2.  On typical kernels which have CONFIG_HZ set to 100, a call to
msleep(5) will actually take approximately 10ms (due to the clock
resolution).  However, if you have CONFIG_HZ set to 1000 then the call
*actually* takes 5ms which is too short for this design.  In other
words, you would be unlikely to notice the issue unless you had
CONFIG_HZ set to a high enough resolution for the msleep(5) to
actually take 5ms.

The changed as proposed should be very low impact.  For users who have
CONFIG_HZ set to 100 (which is typical), there will be no visible
increase in time (since the call was already taking 10ms).  For users
who have CONFIG_HZ set to 1000, the net effect is that it takes an
extra 20ms to switch hardware modes (since the #define in question is
used at most four times in sequence).

Devin

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

Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Mauro Carvalho Chehab]

Hi Palash,

Em 25-07-2011 07:24, Devin Heitmueller escreveu:
> On Mon, Jul 25, 2011 at 1:48 AM, Palash Bandyopadhyay
> <Palash.Bandyopadhyay@conexant.com> wrote:
>> Mauro/Devin,
>>
>>  Can someone give steps to reproduce the problem? Also if we need any particular h/w board to reproduce this problem. 
> I dont seem to recall any delay requirement on the chip at power up/cycle time. Any I also dont recall seeing any
>  problems with the Conexant evk boards. Mauro, have you been able to see this issue with a Conexant board?

This problem were reported with Hauppauge USBLive 2 by several users.
I had a similar problem with another grabber device. My guess is that,
on non-grabber devices, the additional steps needed to control other
device elements spend some time, and, due to that, the delay timing
issue is not noticed.

The first strategy used to fix such bug is the following patch, that
I've forwarded you, back in December:

>From a83a7574e07b48b1c6a222d833a7fa0a67133b5c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@gmail.com>
Date: Thu, 16 Dec 2010 17:39:17 +0100
Subject: [PATCH] cx231xx: raise delay after powerup.

Wait a bit longer after power up so the chips have enougth
time to come up before we try talking to them via i2c.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/media/video/cx231xx/cx231xx-avcore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/cx231xx/cx231xx-avcore.c b/drivers/media/video/cx231xx/cx231xx-avcore.c
index cf50faf..cf412cd 100644
--- a/drivers/media/video/cx231xx/cx231xx-avcore.c
+++ b/drivers/media/video/cx231xx/cx231xx-avcore.c
@@ -2412,7 +2412,7 @@ int cx231xx_set_power_mode(struct cx231xx *dev, enum AV_MODE mode)
                break;
        }

-       msleep(PWR_SLEEP_INTERVAL);
+       msleep(PWR_SLEEP_INTERVAL * 10);

        /* For power saving, only enable Pwr_resetout_n
           when digital TV is selected. */

> The problem occurs if the kernel is configured for CONFIG_HZ to 1000
> and on the Hauppauge USBLIve 2 hardware design.  I'm assuming it's
> tied to two factors:
> 
> 1.  Some aspect of the power supply tied to the Mako core being a
> little slow to startup.  I haven't compared the schematic to the EVK,
> but I could do that if we *really* think that is a worthwhile
> exercise.
> 2.  On typical kernels which have CONFIG_HZ set to 100, a call to
> msleep(5) will actually take approximately 10ms (due to the clock
> resolution).  However, if you have CONFIG_HZ set to 1000 then the call
> *actually* takes 5ms which is too short for this design.  In other
> words, you would be unlikely to notice the issue unless you had
> CONFIG_HZ set to a high enough resolution for the msleep(5) to
> actually take 5ms.
> 
> The changed as proposed should be very low impact.  For users who have
> CONFIG_HZ set to 100 (which is typical), there will be no visible
> increase in time (since the call was already taking 10ms).  For users
> who have CONFIG_HZ set to 1000, the net effect is that it takes an
> extra 20ms to switch hardware modes (since the #define in question is
> used at most four times in sequence).

I agree with Devin. The impact is probably not visible for the users whose
boards are already working, but it is very significant for the ones that
have grabber devices whose devices don't work due to the delay issue, as
the device will start working.

>From my side, I don't mind if you want to solve it with another strategy,
for example, like on a code that would check if the device was actually
powered-up, but, if just increasing the delay to 10 ms is is enough and won't
cause side effects, I would just get the simplest fix.

Cheers,
Mauro

RE: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Palash Bandyopadhyay]

Mauro and Devin,

  Thanks for the explainations. The fix proposed is ok and as Devin mentioned, should have very low impact. I'll see if I can dig into the issue to root cause it. 

Thanks for your help.

Rgds,
Palash

Signed off by Palash Bandyopadhyay

________________________________________
From: Mauro Carvalho Chehab [mchehab@redhat.com]
Sent: Monday, July 25, 2011 5:58 AM
To: Devin Heitmueller
Cc: Palash Bandyopadhyay; Linux Media Mailing List; Gerd Hoffmann; Sri Deevi
Subject: Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

Hi Palash,

Em 25-07-2011 07:24, Devin Heitmueller escreveu:
> On Mon, Jul 25, 2011 at 1:48 AM, Palash Bandyopadhyay
> <Palash.Bandyopadhyay@conexant.com> wrote:
>> Mauro/Devin,
>>
>>  Can someone give steps to reproduce the problem? Also if we need any particular h/w board to reproduce this problem.
> I dont seem to recall any delay requirement on the chip at power up/cycle time. Any I also dont recall seeing any
>  problems with the Conexant evk boards. Mauro, have you been able to see this issue with a Conexant board?

This problem were reported with Hauppauge USBLive 2 by several users.
I had a similar problem with another grabber device. My guess is that,
on non-grabber devices, the additional steps needed to control other
device elements spend some time, and, due to that, the delay timing
issue is not noticed.

The first strategy used to fix such bug is the following patch, that
I've forwarded you, back in December:

>From a83a7574e07b48b1c6a222d833a7fa0a67133b5c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@gmail.com>
Date: Thu, 16 Dec 2010 17:39:17 +0100
Subject: [PATCH] cx231xx: raise delay after powerup.

Wait a bit longer after power up so the chips have enougth
time to come up before we try talking to them via i2c.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/media/video/cx231xx/cx231xx-avcore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/cx231xx/cx231xx-avcore.c b/drivers/media/video/cx231xx/cx231xx-avcore.c
index cf50faf..cf412cd 100644
--- a/drivers/media/video/cx231xx/cx231xx-avcore.c
+++ b/drivers/media/video/cx231xx/cx231xx-avcore.c
@@ -2412,7 +2412,7 @@ int cx231xx_set_power_mode(struct cx231xx *dev, enum AV_MODE mode)
                break;
        }

-       msleep(PWR_SLEEP_INTERVAL);
+       msleep(PWR_SLEEP_INTERVAL * 10);

        /* For power saving, only enable Pwr_resetout_n
           when digital TV is selected. */

> The problem occurs if the kernel is configured for CONFIG_HZ to 1000
> and on the Hauppauge USBLIve 2 hardware design.  I'm assuming it's
> tied to two factors:
>
> 1.  Some aspect of the power supply tied to the Mako core being a
> little slow to startup.  I haven't compared the schematic to the EVK,
> but I could do that if we *really* think that is a worthwhile
> exercise.
> 2.  On typical kernels which have CONFIG_HZ set to 100, a call to
> msleep(5) will actually take approximately 10ms (due to the clock
> resolution).  However, if you have CONFIG_HZ set to 1000 then the call
> *actually* takes 5ms which is too short for this design.  In other
> words, you would be unlikely to notice the issue unless you had
> CONFIG_HZ set to a high enough resolution for the msleep(5) to
> actually take 5ms.
>
> The changed as proposed should be very low impact.  For users who have
> CONFIG_HZ set to 100 (which is typical), there will be no visible
> increase in time (since the call was already taking 10ms).  For users
> who have CONFIG_HZ set to 1000, the net effect is that it takes an
> extra 20ms to switch hardware modes (since the #define in question is
> used at most four times in sequence).

I agree with Devin. The impact is probably not visible for the users whose
boards are already working, but it is very significant for the ones that
have grabber devices whose devices don't work due to the delay issue, as
the device will start working.

>From my side, I don't mind if you want to solve it with another strategy,
for example, like on a code that would check if the device was actually
powered-up, but, if just increasing the delay to 10 ms is is enough and won't
cause side effects, I would just get the simplest fix.

Cheers,
Mauro
Conexant E-mail Firewall (Conexant.Com) made the following annotations
---------------------------------------------------------------------
********************** Legal Disclaimer **************************** 

"This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you." 

********************************************************************** 

---------------------------------------------------------------------

Re: [PATCH] Fix regression introduced which broke the Hauppauge USBLive 2

[Doychin Dokov]

На 24.7.2011 г. 04:17 ч., Devin Heitmueller написа:
> The following patch addresses the regression introduced in the cx231xx
> driver which stopped the Hauppauge USBLive2 from working.
>
> Confirmed working by both myself and the user who reported the issue
> on the KernelLabs blog (Robert DeLuca).
>

I can also confirm this patch works fine for me.

[  839.052752] cx231xx: Cx231xx Audio Extension initialized
[  850.221325] cx231xx #0: cx231xx_start_stream():: ep_mask = 4
[  850.224774] cx231xx #0: cx231xx_init_audio_isoc: Starting ISO AUDIO 
transfers
[  851.150358] cx231xx #0: cx231xx_stop_stream():: ep_mask = 4

It was not possible to capture audio from the device before that.