[PATCH] gspca - ov534: Fix the light frequency filter.

[PATCH] gspca - ov534: Fix the light frequency filter.

[Jean-Francois Moine]

From: Jean-François Moine <moinejf@free.fr>

The exchanges relative to the light frequency filter were adapted
from a description found in a ms-windows driver. It seems that the
registers were the ones of some other sensor.

This patch was done thanks to the documentation of the right
OmniVision sensors.

Note: The light frequency filter is either off or automatic.
The application will see either off or "50Hz" only.

--- a/drivers/media/usb/gspca/ov534.c
+++ b/drivers/media/usb/gspca/ov534.c
@@ -1038,13 +1038,12 @@
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
-	val = val ? 0x9e : 0x00;
-	if (sd->sensor == SENSOR_OV767x) {
-		sccb_reg_write(gspca_dev, 0x2a, 0x00);
-		if (val)
-			val = 0x9d;	/* insert dummy to 25fps for 50Hz */
-	}
-	sccb_reg_write(gspca_dev, 0x2b, val);
+	if (!val)
+		sccb_reg_write(gspca_dev, 0x13,		/* off */
+				sccb_reg_read(gspca_dev, 0x13) & ~0x20);
+	else
+		sccb_reg_write(gspca_dev, 0x13,		/* auto */
+				sccb_reg_read(gspca_dev, 0x13) | 0x20);
 }
 
 

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH] gspca - ov534: Fix the light frequency filter

[Jean-Francois Moine]

(fix lack of signature)
From: Jean-François Moine <moinejf@free.fr>

The exchanges relative to the light frequency filter were adapted
from a description found in a ms-windows driver. It seems that the
registers were the ones of some other sensor.

This patch was done thanks to the documentation of the right
OmniVision sensors.

Note: The light frequency filter is either off or automatic.
The application will see either off or "50Hz" only.

Tested-by: alexander calderon <fabianp902@gmail.com>
Signed-off-by: Jean-François Moine <moinejf@free.fr>

--- a/drivers/media/usb/gspca/ov534.c
+++ b/drivers/media/usb/gspca/ov534.c
@@ -1038,13 +1038,12 @@
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
-	val = val ? 0x9e : 0x00;
-	if (sd->sensor == SENSOR_OV767x) {
-		sccb_reg_write(gspca_dev, 0x2a, 0x00);
-		if (val)
-			val = 0x9d;	/* insert dummy to 25fps for 50Hz */
-	}
-	sccb_reg_write(gspca_dev, 0x2b, val);
+	if (!val)
+		sccb_reg_write(gspca_dev, 0x13,		/* off */
+				sccb_reg_read(gspca_dev, 0x13) & ~0x20);
+	else
+		sccb_reg_write(gspca_dev, 0x13,		/* auto */
+				sccb_reg_read(gspca_dev, 0x13) | 0x20);
 }
 
 

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Antonio Ospite]

On Thu, 22 Nov 2012 12:46:52 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> (fix lack of signature)
> From: Jean-François Moine <moinejf@free.fr>
> 
> The exchanges relative to the light frequency filter were adapted
> from a description found in a ms-windows driver. It seems that the
> registers were the ones of some other sensor.
>

The PS3 sends the old sequence too AFAIR, for the PS3 Eye.

> This patch was done thanks to the documentation of the right
> OmniVision sensors.
>

In the datasheet I have for ov772x, bit[6] of register 0x13 is described
as:

  Bit[6]: AEC - Step size limit
    0: Step size is limited to vertical blank
    1: Unlimited step size

And the patch makes Light Frequency _NOT_ work with the PS3 eye (based
on ov772x).

What does the ov767x datasheet say?

Maybe we should use the new values only when
	sd->sensor == SENSOR_OV767x

What sensor does Alexander's webcam use?

> Note: The light frequency filter is either off or automatic.
> The application will see either off or "50Hz" only.
> 
> Tested-by: alexander calderon <fabianp902@gmail.com>
> Signed-off-by: Jean-François Moine <moinejf@free.fr>
> 
> --- a/drivers/media/usb/gspca/ov534.c
> +++ b/drivers/media/usb/gspca/ov534.c
> @@ -1038,13 +1038,12 @@
>  {
>  	struct sd *sd = (struct sd *) gspca_dev;
> 

drivers/media/usb/gspca/ov534.c: In function ‘setlightfreq’:
drivers/media/usb/gspca/ov534.c:1039:13: warning: unused variable ‘sd’ [-Wunused-variable]

But that will go away if we check for the sensor again in a next
version of the patch.

> -	val = val ? 0x9e : 0x00;
> -	if (sd->sensor == SENSOR_OV767x) {
> -		sccb_reg_write(gspca_dev, 0x2a, 0x00);
> -		if (val)
> -			val = 0x9d;	/* insert dummy to 25fps for 50Hz */
> -	}
> -	sccb_reg_write(gspca_dev, 0x2b, val);
> +	if (!val)
> +		sccb_reg_write(gspca_dev, 0x13,		/* off */
> +				sccb_reg_read(gspca_dev, 0x13) & ~0x20);
> +	else
> +		sccb_reg_write(gspca_dev, 0x13,		/* auto */
> +				sccb_reg_read(gspca_dev, 0x13) | 0x20);
>  }
>  

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Jean-Francois Moine]

On Fri, 23 Nov 2012 18:09:09 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> On Thu, 22 Nov 2012 12:46:52 +0100
	[snip]
> Jean-Francois Moine <moinejf@free.fr> wrote:
> > This patch was done thanks to the documentation of the right
> > OmniVision sensors.
> 
> In the datasheet I have for ov772x, bit[6] of register 0x13 is described
> as:
> 
>   Bit[6]: AEC - Step size limit
>     0: Step size is limited to vertical blank
>     1: Unlimited step size

Right, but I don't use the bit 6, it is the bit 5:

> > +		sccb_reg_write(gspca_dev, 0x13,		/* auto */
> > +				sccb_reg_read(gspca_dev, 0x13) | 0x20);

which is described as:

   Bit[5]:  Banding filter ON/OFF

> And the patch makes Light Frequency _NOT_ work with the PS3 eye (based
> on ov772x).
> 
> What does the ov767x datasheet say?

Quite the same thing:

   Bit[5]: Banding filter ON/OFF - In order to turn ON the banding
           filter, BD50ST (0x9D) or BD60ST (0x9E) must be set to a
           non-zero value.
           0: OFF
           1: ON

(the registers 9d and 9e are non zero for the ov767x in ov534.c)

> Maybe we should use the new values only when
> 	sd->sensor == SENSOR_OV767x
> 
> What sensor does Alexander's webcam use?

He has exactly the same webcam as yours: 1415:2000 (ps eye) with
sensor ov772x.

> > Note: The light frequency filter is either off or automatic.
> > The application will see either off or "50Hz" only.
> > 
> > Tested-by: alexander calderon <fabianp902@gmail.com>
> > Signed-off-by: Jean-François Moine <moinejf@free.fr>
> > 
> > --- a/drivers/media/usb/gspca/ov534.c
> > +++ b/drivers/media/usb/gspca/ov534.c
> > @@ -1038,13 +1038,12 @@
> >  {
> >  	struct sd *sd = (struct sd *) gspca_dev;
> > 
> 
> drivers/media/usb/gspca/ov534.c: In function ‘setlightfreq’:
> drivers/media/usb/gspca/ov534.c:1039:13: warning: unused variable ‘sd’ [-Wunused-variable]

Thanks.

Well, here is one of the last message I received from Alexander (in
fact, his first name is Fabian):

> Thanks for all your help, it is very kind of you, I used the code below,the
> 60 Hz filter appear to work even at 100fps, but when I used 125 fps it
> didnt work :( , i guess it is something of detection speed. If you have any
> other idea I'll be very thankful.
> 
> Sincerely Fabian Calderon

So, how may we advance?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Antonio Ospite]

On Fri, 23 Nov 2012 19:12:32 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> On Fri, 23 Nov 2012 18:09:09 +0100
> Antonio Ospite <ospite@studenti.unina.it> wrote:
[...]
> > In the datasheet I have for ov772x, bit[6] of register 0x13 is described
> > as:
> > 
> >   Bit[6]: AEC - Step size limit
> >     0: Step size is limited to vertical blank
> >     1: Unlimited step size
> 
> Right, but I don't use the bit 6, it is the bit 5:
>

Ouch, right! :)

> > > +		sccb_reg_write(gspca_dev, 0x13,		/* auto */
> > > +				sccb_reg_read(gspca_dev, 0x13) | 0x20);
> 
> which is described as:
> 
>    Bit[5]:  Banding filter ON/OFF
> 
> > And the patch makes Light Frequency _NOT_ work with the PS3 eye (based
> > on ov772x).
> > 
> > What does the ov767x datasheet say?
> 
> Quite the same thing:
> 
>    Bit[5]: Banding filter ON/OFF - In order to turn ON the banding
>            filter, BD50ST (0x9D) or BD60ST (0x9E) must be set to a
>            non-zero value.
>            0: OFF
>            1: ON
> 
> (the registers 9d and 9e are non zero for the ov767x in ov534.c)
> 

In the ov767x datasheet I also see that selecting _what_ filter to
apply is done in Bit[3] of register 0x3B, but I couldn't find such info
for ov772x. (FYI a datasheet for ov7740 can be found on the web with
some theory but resisters are not always the same as ov772x).

> > Maybe we should use the new values only when
> > 	sd->sensor == SENSOR_OV767x
> > 
> > What sensor does Alexander's webcam use?
> 
> He has exactly the same webcam as yours: 1415:2000 (ps eye) with
> sensor ov772x.
>
> > > Note: The light frequency filter is either off or automatic.
> > > The application will see either off or "50Hz" only.
> > > 
> > > Tested-by: alexander calderon <fabianp902@gmail.com>
> > > Signed-off-by: Jean-François Moine <moinejf@free.fr>
> > > 
> > > --- a/drivers/media/usb/gspca/ov534.c
> > > +++ b/drivers/media/usb/gspca/ov534.c
> > > @@ -1038,13 +1038,12 @@
> > >  {
> > >  	struct sd *sd = (struct sd *) gspca_dev;
> > > 
> > 
> > drivers/media/usb/gspca/ov534.c: In function ‘setlightfreq’:
> > drivers/media/usb/gspca/ov534.c:1039:13: warning: unused variable ‘sd’ [-Wunused-variable]
> 
> Thanks.
> 
> Well, here is one of the last message I received from Alexander (in
> fact, his first name is Fabian):
> 
> > Thanks for all your help, it is very kind of you, I used the code below,the
> > 60 Hz filter appear to work even at 100fps, but when I used 125 fps it
> > didnt work :( , i guess it is something of detection speed. If you have any
> > other idea I'll be very thankful.
> > 
> > Sincerely Fabian Calderon
>

So he is in a place where a 60Hz power source is used?.

> So, how may we advance?

For now I'd NAK the patch since it is a regression for users
with 50Hz power sources and it looks like it does not _always_ work for
60Hz either.

Should I remove it from patchwork as well?

As I have the webcam and can perform actual tests I'll coordinate with
Fabian to have more details about why light frequency filter is not
working for him with the current code, it works fine for me at 640x480,
even if I can see that its effect is weaker at 320x240.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Jean-Francois Moine]

On Mon, 26 Nov 2012 14:08:06 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> For now I'd NAK the patch since it is a regression for users
> with 50Hz power sources and it looks like it does not _always_ work for
> 60Hz either.
> 
> Should I remove it from patchwork as well?
> 
> As I have the webcam and can perform actual tests I'll coordinate with
> Fabian to have more details about why light frequency filter is not
> working for him with the current code, it works fine for me at 640x480,
> even if I can see that its effect is weaker at 320x240.

I wonder how it could work. Look at the actual code:

	val = val ? 0x9e : 0x00;
	if (sd->sensor == SENSOR_OV767x) {
		sccb_reg_write(gspca_dev, 0x2a, 0x00);
		if (val)
			val = 0x9d;	/* insert dummy to 25fps for 50Hz */
	}
	sccb_reg_write(gspca_dev, 0x2b, val);

According to the ov7720/ov7221 documentation, the register 2b is:

	2B EXHCL 00 RW Dummy Pixel Insert LSB
	               8 LSB for dummy pixel insert in horizontal direction

How could it act on the light frequency filter?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Antonio Ospite]

On Mon, 26 Nov 2012 16:23:18 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> On Mon, 26 Nov 2012 14:08:06 +0100
> Antonio Ospite <ospite@studenti.unina.it> wrote:
> 
> > For now I'd NAK the patch since it is a regression for users
> > with 50Hz power sources and it looks like it does not _always_ work for
> > 60Hz either.
> > 
> > Should I remove it from patchwork as well?
> > 
> > As I have the webcam and can perform actual tests I'll coordinate with
> > Fabian to have more details about why light frequency filter is not
> > working for him with the current code, it works fine for me at 640x480,
> > even if I can see that its effect is weaker at 320x240.
> 
> I wonder how it could work. Look at the actual code:
> 
> 	val = val ? 0x9e : 0x00;
> 	if (sd->sensor == SENSOR_OV767x) {
> 		sccb_reg_write(gspca_dev, 0x2a, 0x00);
> 		if (val)
> 			val = 0x9d;	/* insert dummy to 25fps for 50Hz */
> 	}
> 	sccb_reg_write(gspca_dev, 0x2b, val);
> 
> According to the ov7720/ov7221 documentation, the register 2b is:
> 
> 	2B EXHCL 00 RW Dummy Pixel Insert LSB
> 	               8 LSB for dummy pixel insert in horizontal direction
> 
> How could it act on the light frequency filter?

Warning: guess-work follows.

When the light filter (i.e. from the v4l2 POV) is ON, the frame rate is
actually lower than the one expected; that could be the effect of the
insertion of dummy pixels in the data processed by the sensor (the
streamed data keeps always the same size tho), maybe it is just a trick
but the fact is that the flickering goes away.

The weaker result at 320x240 could be due to the amount of dummy pixels
tailored for the higher resolution, IIRC the PS3 dumps were performed
only at 640x480.

BTW the documentation might also be wrong or inaccurate.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Jean-Francois Moine]

On Mon, 26 Nov 2012 18:12:41 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> BTW the documentation might also be wrong or inaccurate.

The ov7670 documentation has exactly the same description of the
register 0x2b, and I don't think that the manufacturer would greatly
change the meaning of such low registers in so close sensors.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Hans de Goede]

Hi Jean-Francois, Antonio Ospite,

Could it be that you're both right, and that the register
Jean-Francois suggest is used (0x13) and uses in his patch
is for enabling / disabling the light-freq filter, where
as the register which were used before this patch
(0x2a, 0x2b) are used to select the light frequency to
filter for?

That would explain everything the 2 50 / 60 hz testers are
seeing. This assumes that reg 0x13 has the filter always
enabled before the patch, and the code before the patch
simply changes the filter freq to such a value it
effectively disables the filter for 50 Hz. This also
assumes that the default values in 0x2a and 0x2b are
valid for 60hz, which explains why Jean Francois' patch
works for 60 Hz, so with all this combined we should
have all pieces of the puzzle ...

Anyone wants to do a patch to prove I'm right (or wrong :)
?

Regards,

Hans


On 11/23/2012 07:12 PM, Jean-Francois Moine wrote:
> On Fri, 23 Nov 2012 18:09:09 +0100
> Antonio Ospite <ospite@studenti.unina.it> wrote:
>
>> On Thu, 22 Nov 2012 12:46:52 +0100
> 	[snip]
>> Jean-Francois Moine <moinejf@free.fr> wrote:
>>> This patch was done thanks to the documentation of the right
>>> OmniVision sensors.
>>
>> In the datasheet I have for ov772x, bit[6] of register 0x13 is described
>> as:
>>
>>    Bit[6]: AEC - Step size limit
>>      0: Step size is limited to vertical blank
>>      1: Unlimited step size
>
> Right, but I don't use the bit 6, it is the bit 5:
>
>>> +		sccb_reg_write(gspca_dev, 0x13,		/* auto */
>>> +				sccb_reg_read(gspca_dev, 0x13) | 0x20);
>
> which is described as:
>
>     Bit[5]:  Banding filter ON/OFF
>
>> And the patch makes Light Frequency _NOT_ work with the PS3 eye (based
>> on ov772x).
>>
>> What does the ov767x datasheet say?
>
> Quite the same thing:
>
>     Bit[5]: Banding filter ON/OFF - In order to turn ON the banding
>             filter, BD50ST (0x9D) or BD60ST (0x9E) must be set to a
>             non-zero value.
>             0: OFF
>             1: ON
>
> (the registers 9d and 9e are non zero for the ov767x in ov534.c)
>
>> Maybe we should use the new values only when
>> 	sd->sensor == SENSOR_OV767x
>>
>> What sensor does Alexander's webcam use?
>
> He has exactly the same webcam as yours: 1415:2000 (ps eye) with
> sensor ov772x.
>
>>> Note: The light frequency filter is either off or automatic.
>>> The application will see either off or "50Hz" only.
>>>
>>> Tested-by: alexander calderon <fabianp902@gmail.com>
>>> Signed-off-by: Jean-François Moine <moinejf@free.fr>
>>>
>>> --- a/drivers/media/usb/gspca/ov534.c
>>> +++ b/drivers/media/usb/gspca/ov534.c
>>> @@ -1038,13 +1038,12 @@
>>>   {
>>>   	struct sd *sd = (struct sd *) gspca_dev;
>>>
>>
>> drivers/media/usb/gspca/ov534.c: In function ‘setlightfreq’:
>> drivers/media/usb/gspca/ov534.c:1039:13: warning: unused variable ‘sd’ [-Wunused-variable]
>
> Thanks.
>
> Well, here is one of the last message I received from Alexander (in
> fact, his first name is Fabian):
>
>> Thanks for all your help, it is very kind of you, I used the code below,the
>> 60 Hz filter appear to work even at 100fps, but when I used 125 fps it
>> didnt work :( , i guess it is something of detection speed. If you have any
>> other idea I'll be very thankful.
>>
>> Sincerely Fabian Calderon
>
> So, how may we advance?
>

Re: [PATCH] gspca - ov534: Fix the light frequency filter

[Antonio Ospite]

On Thu, 29 Nov 2012 10:25:19 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi Jean-Francois, Antonio Ospite,
> 
> Could it be that you're both right, and that the register
> Jean-Francois suggest is used (0x13) and uses in his patch
> is for enabling / disabling the light-freq filter, where
> as the register which were used before this patch
> (0x2a, 0x2b) are used to select the light frequency to
> filter for?
>

I too thought about something along this line after looking in the
"OV7670 Implementation Guide": there is a relationship between the
banding filter and the maximum exposure, and the latter is somewhat
related to dummy lines/pixels. So this would make sense.

> That would explain everything the 2 50 / 60 hz testers are
> seeing. This assumes that reg 0x13 has the filter always
> enabled before the patch, and the code before the patch
> simply changes the filter freq to such a value it
> effectively disables the filter for 50 Hz. This also
> assumes that the default values in 0x2a and 0x2b are
> valid for 60hz, which explains why Jean Francois' patch
> works for 60 Hz, so with all this combined we should
> have all pieces of the puzzle ...
> 
> Anyone wants to do a patch to prove I'm right (or wrong :)
> ?

I contacted Fabian Alexander Calderon off-list using the email address
in the tested-by line in the patch sent by Jean-Francois, I am waiting
for a reply from him.

I can cook something which uses register 0x13 and still makes the
filter apply on 50Hz, but I'll test for an actual test before
submitting it.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?