gspca - ov534: Fix the light frequency filter

PATCH In review Jean-Francois Moine 3 participants 9 messages Nov 22, 2012 – Nov 29, 2012

Subsystems (2)

Media (V4L/DVB) Gspca USB webcam driver

lore

Share & tools

lore

Jean-Francois Moine

author

2012-11-22 18:53 UTC #0 lore raw

PATCH 1/1 gspca - ov534: Fix the light frequency filter

(fix lack of signature)
From: Jean-François Moine <[email protected]>

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 <[email protected]>

Signed-off-by Jean-François Moine <[email protected]>

drivers/media/usb/gspca/ov534.c +6 -7

@@ -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);
 }

Signature · 2 lines

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

Antonio Ospite

2012-11-23 17:09 UTC #1 lore raw

Quoted 7 lines · On Thu, 22 Nov 2012 12:46:52 +0100 Jean-Francois Moine <[email protected]> wrote:

(fix lack of signature)
From: Jean-François Moine <[email protected]>
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.

Quoted 3 lines

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?

drivers/media/usb/gspca/ov534.c line 1040

Quoted 12 lines

Note: The light frequency filter is either off or automatic. The application will see either off or "50Hz" only.
Tested-by alexander calderon <[email protected]>
Signed-off-by Jean-François Moine <[email protected]>
drivers/media/usb/gspca/ov534.c +0 -0
@@ -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.

Quoted 5 lines

drivers/media/usb/gspca/ov534.c +0 -5

-	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 */

drivers/media/usb/gspca/ov534.c line 1048

Quoted 10 lines

drivers/media/usb/gspca/ov534.c +6 -2

-	}
-	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

Signature · 6 lines

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?

Jean-Francois Moine

author

2012-11-23 18:11 UTC #2 lore raw

Quoted 1 line · On Fri, 23 Nov 2012 18:09:09 +0100 Antonio Ospite <[email protected]>…

On Thu, 22 Nov 2012 12:46:52 +0100

	[snip]

Quoted 10 lines

Quoted 2 lines · Jean-Francois Moine
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:

Quoted 2 lines

drivers/media/usb/gspca/ov534.c line 1046
Quoted 2 lines
drivers/media/usb/gspca/ov534.c +2 -0
+		sccb_reg_write(gspca_dev, 0x13,		/* auto */
+				sccb_reg_read(gspca_dev, 0x13) | 0x20);

which is described as:

Bit[5]: Banding filter ON/OFF

Quoted 4 lines

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)

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines
Note: The light frequency filter is either off or automatic. The application will see either off or "50Hz" only.
Tested-by alexander calderon <[email protected]>
Signed-off-by Jean-François Moine <[email protected]>

Quoted 9 lines

drivers/media/usb/gspca/ov534.c line 1040
Quoted 6 lines
drivers/media/usb/gspca/ov534.c +0 -0
@@ -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):

Quoted 6 lines

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?

Signature · 2 lines

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

Antonio Ospite

2012-11-26 13:08 UTC #3 lore raw

Quoted 2 lines · On Fri, 23 Nov 2012 19:12:32 +0100 Jean-Francois Moine <[email protected]> wrote:

On Fri, 23 Nov 2012 18:09:09 +0100
Antonio Ospite <[email protected]> wrote:

[...]

Quoted 9 lines

Quoted 6 lines
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! :)

Quoted 14 lines

Quoted 2 lines
drivers/media/usb/gspca/ov534.c line 1046
Quoted 2 lines
drivers/media/usb/gspca/ov534.c +2 -0
+		sccb_reg_write(gspca_dev, 0x13,		/* auto */
+				sccb_reg_read(gspca_dev, 0x13) | 0x20);
which is described as:
Bit[5]: Banding filter ON/OFF
Quoted 4 lines
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:

Quoted 8 lines

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).

Quoted 24 lines

Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Note: The light frequency filter is either off or automatic. The application will see either off or "50Hz" only.
Tested-by alexander calderon <[email protected]>
Signed-off-by Jean-François Moine <[email protected]>
Quoted 9 lines
drivers/media/usb/gspca/ov534.c line 1040
Quoted 6 lines
drivers/media/usb/gspca/ov534.c +0 -0
@@ -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]

Quoted 12 lines

Thanks.
Well, here is one of the last message I received from Alexander (in fact, his first name is Fabian):
Quoted 6 lines
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?.

Quoted 1 line

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

Signature · 6 lines

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?

Jean-Francois Moine

author

2012-11-26 15:22 UTC #4 lore raw

Quoted 10 lines · On Mon, 26 Nov 2012 14:08:06 +0100 Antonio Ospite <[email protected]>…

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?

Signature · 2 lines

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

Antonio Ospite

2012-11-26 17:12 UTC #5 lore raw

Quoted 24 lines · On Mon, 26 Nov 2012 16:23:18 +0100 Jean-Francois Moine <[email protected]> wrote:

Quoted 10 lines · On Mon, 26 Nov 2012 14:08:06 +0100 Antonio Ospite <[email protected]>…
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);

Quoted 6 lines

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

Signature · 6 lines

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?

Jean-Francois Moine

author

2012-11-26 17:50 UTC #6 lore raw

Quoted 1 line · On Mon, 26 Nov 2012 18:12:41 +0100 Antonio Ospite <[email protected]>…

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.

Signature · 2 lines

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

Hans de Goede

2012-11-29 09:23 UTC #7 lore raw

In reply to Jean-Francois Moine

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

Quoted 78 lines · Jean-Francois Moine

Quoted 1 line · On Fri, 23 Nov 2012 18:09:09 +0100 Antonio Ospite <[email protected]>…
On Thu, 22 Nov 2012 12:46:52 +0100
	[snip]
Quoted 10 lines
Quoted 2 lines · Jean-Francois Moine
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:
Quoted 2 lines
drivers/media/usb/gspca/ov534.c line 1046
Quoted 2 lines
drivers/media/usb/gspca/ov534.c +2 -0
+		sccb_reg_write(gspca_dev, 0x13,		/* auto */
+				sccb_reg_read(gspca_dev, 0x13) | 0x20);
which is described as:
Bit[5]: Banding filter ON/OFF
Quoted 4 lines
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)
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Note: The light frequency filter is either off or automatic. The application will see either off or "50Hz" only.
Tested-by alexander calderon <[email protected]>
Signed-off-by Jean-François Moine <[email protected]>
Quoted 9 lines
drivers/media/usb/gspca/ov534.c line 1040
Quoted 6 lines
drivers/media/usb/gspca/ov534.c +0 -0
@@ -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):
Quoted 6 lines
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?

Antonio Ospite

2012-11-29 22:15 UTC #8 lore raw

Quoted 9 lines · On Thu, 29 Nov 2012 10:25:19 +0100 Hans de Goede <[email protected]> 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.

Quoted 10 lines

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 ...

Quoted 2 lines

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

Signature · 6 lines

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?