| Message ID | 20121122124652.3a832e33@armhf (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
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
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?
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
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.
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? > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--- 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); }