Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
On Thu, 21 Jul 2011 03:43:13 -0700 (PDT) Luiz Ramos wrote: > Now my doubts. Unless I misunderstood something, it seems these are > the our assumptions regarding reg01 and reg17: > > - reg01 bit 6 is set when bridge runs at 48 MHz; if reset, 24 MHz > - reg17 bits 0..4 is a mask for dividing a sensor clock of 48 MHz, > so > - if reg17 = x | 01 then clock = 48 MHz > - if reg17 = x | 02 then clock = 24 MHz > - if reg17 = x | 04 then clock = 12 MHz > > Putting some printk at the code version 2.13.3, the values of these > registers at the last command are: > > - at 640x480 ... reg01 = 0x66 reg17 = 0x64 > - at 320x240/160x120 ... reg01 = 0x26 reg17 = 0x61 > > So, at 640x480 the bridge would be running at 48 MHz and the sensor > at 12 MHz. At lower resolutions the bridge would be running at 24 MHz > and the sensor at 48 MHz. It seems that this is not what we'd like to > do. >From the documentation, the register 17 contains a divide factor of the bridge clock (the sensor has no clock). So: - reg01 = 0x66 reg17 = 0x64 --> bridge 48 MHz sensor 12 MHz - reg01 = 0x26 reg17 = 0x61 --> bridge 24 MHz sensor 24 MHz > I made some experiences, and noticed that: > > - making reg17 = 0x62 (sensor clock at 24 MHz) and reg01 = 0x26 > (bridge clock at 24 MHz) at 320x240 and lower makes it work again. > I think this reaches the goal of having both clocks at 24 MHz, but > at 10 fps In fact, bridge 24 MHz and sensor 12 MHz. This seems the best configuration. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
Hello, The package version 2.13.3 was compiled, and there are some results: - at 640x480: it works as before (10 fps, good image) - at 320x240 and 160x120: it does not work (no frames when using qv4l2) Now my doubts. Unless I misunderstood something, it seems these are the our assumptions regarding reg01 and reg17: - reg01 bit 6 is set when bridge runs at 48 MHz; if reset, 24 MHz - reg17 bits 0..4 is a mask for dividing a sensor clock of 48 MHz, so - if reg17 = x | 01 then clock = 48 MHz - if reg17 = x | 02 then clock = 24 MHz - if reg17 = x | 04 then clock = 12 MHz Putting some printk at the code version 2.13.3, the values of these registers at the last command are: - at 640x480 ... reg01 = 0x66 reg17 = 0x64 - at 320x240/160x120 ... reg01 = 0x26 reg17 = 0x61 So, at 640x480 the bridge would be running at 48 MHz and the sensor at 12 MHz. At lower resolutions the bridge would be running at 24 MHz and the sensor at 48 MHz. It seems that this is not what we'd like to do. I made some experiences, and noticed that: - making reg17 = 0x62 (sensor clock at 24 MHz) and reg01 = 0x26 (bridge clock at 24 MHz) at 320x240 and lower makes it work again. I think this reaches the goal of having both clocks at 24 MHz, but at 10 fps - making reg17 = 0x61 (sensor at 48 MHz) and reg01 = 0x66 (bridge at 48 MHz) at 640x480 gives me back a "No frame". A side question: would it be associated to the speed limit of USB 1.1 or whatever? There are more experiences to be done, and as there are more conclusions, I'll post them here. But again, to make the code work it's sufficient to change the reg17 or'ing at line 2537 from 0x01 to 0x02, making the sensor run at 24 MHz for resolutions 320x240 and 160x120. For now, it seems to exist a trade-off between sensor clock and exposure, meaning that if you'd like to run at 20 fps, you'd have to use higher clock rates and the images require higher levels of light. Or running at 10 fps gives the ability to capture images in darker places. Does this makes sense? If so, one nice feature for such cameras would be controlling the exposure level by the user interface (ioctl), which in effect puts that decision in the user's hands. Thanks, Luiz Ramos --- Em qua, 20/7/11, Jean-Francois Moine escreveu: > De: Jean-Francois Moine > Assunto: Re: [PATCH] Fix wrong register mask in gspca/sonixj.c > Para: "Luiz Ramos" > Cc: linux-media@vger.kernel.org > Data: Quarta-feira, 20 de Julho de 2011, 8:12 > On Mon, 18 Jul 2011 18:39:14 -0700 > (PDT) > Luiz Ramos > wrote: > [snip] > > I noticed that in 640x480 the device worked fine, but > in 320x240 and > > 160x120 it didn't (I mean: the image is dark). Or'ing > reg17 with 0x04 > > in line 2535 (as it's currently done for VGA) is > sufficient to make > > the webcam work again. The change could be like that: > [snip] > > However, the frame rates get limited to 10 fps. > Without that change, > > and in 320x240 and 160x120, they reach 20 fps (of > darkness). > > > > I can't see what or'ing that register means, and > what's the > > relationship between this and the webcam darkness. It > seems that > > these bits control some kind of clock; this can be > read in the > > program comments. One other argument in favour of this > assumption is > > the fact that the frame rate changes accordingly to > the value of > > these bits. But I can't see how this relates to > exposure. > > > > For my purposes, I'll stay with that change; it's > sufficient for my > > purposes. If you know what I did, please advise me. > :-) > > Hi Luiz, > > You changed the sensor clock from 24MHz to 12MHz. > > The clocks of the sensor and the bridge may have different > values. > In 640x480, the bridge clock is 48MHz (reg01) and the > sensor clock is > 12MHz (reg17: clock / 4). Previously, in 320x240, the > bridge clock was > 48MHz and the sensor clock 24 MHz (reg17: clock / 2). > > I think that the sensor clock must stay the same for a same > frame rate. > So, what about changing the bridge clock only, i.e. bridge > clock 24MHZ > (reg01) and sensor clock 24MHz (reg17: clock / 1)? > > That's what I coded in the last gspca test version (2.13.3) > which is > available in my web site (see below). May you try it? > > Best regards. > > -- > Ken ar c'hentañ | > ** Breizh ha Linux atav! ** > Jef > | http://moinejf.free.fr/ > -- > To unsubscribe from this list: send the line "unsubscribe > linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
On Mon, 18 Jul 2011 18:39:14 -0700 (PDT) Luiz Ramos wrote: [snip] > I noticed that in 640x480 the device worked fine, but in 320x240 and > 160x120 it didn't (I mean: the image is dark). Or'ing reg17 with 0x04 > in line 2535 (as it's currently done for VGA) is sufficient to make > the webcam work again. The change could be like that: [snip] > However, the frame rates get limited to 10 fps. Without that change, > and in 320x240 and 160x120, they reach 20 fps (of darkness). > > I can't see what or'ing that register means, and what's the > relationship between this and the webcam darkness. It seems that > these bits control some kind of clock; this can be read in the > program comments. One other argument in favour of this assumption is > the fact that the frame rate changes accordingly to the value of > these bits. But I can't see how this relates to exposure. > > For my purposes, I'll stay with that change; it's sufficient for my > purposes. If you know what I did, please advise me. :-) Hi Luiz, You changed the sensor clock from 24MHz to 12MHz. The clocks of the sensor and the bridge may have different values. In 640x480, the bridge clock is 48MHz (reg01) and the sensor clock is 12MHz (reg17: clock / 4). Previously, in 320x240, the bridge clock was 48MHz and the sensor clock 24 MHz (reg17: clock / 2). I think that the sensor clock must stay the same for a same frame rate. So, what about changing the bridge clock only, i.e. bridge clock 24MHZ (reg01) and sensor clock 24MHz (reg17: clock / 1)? That's what I coded in the last gspca test version (2.13.3) which is available in my web site (see below). May you try it? Best regards. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
Hello, Jean-François,
I downloaded the tarball you recommended, compiled it and it works nicely.
The original problem also happened with this package. Playing around
with reg17 at some points, and looking the code before the commit of
2010-12-21, I could manage to work around the problem.
Only to remember, in kernel version 2.6.37.6 the webcam became very dark (I
could only see things like a 60 W incandescent lamp, one meter long, or things
like clear blue sky). It seemed there was some problems with auto-exposure,
auto-gain or other similar.
However, the difference which effectively changed the things is the value which
reg17 carries after line 2535 of gspca_sonixj.c (now referring to the "tarball"
code).
I noticed that in 640x480 the device worked fine, but in 320x240 and 160x120 it
didn't (I mean: the image is dark). Or'ing reg17 with 0x04 in line 2535 (as
it's currently done for VGA) is sufficient to make the webcam work again. The
change could be like that:
diff --git a/build/sonixj.c b/build/sonixj.c
index afde673..802dbfa 100644
--- a/build/sonixj.c
+++ b/build/sonixj.c
@@ -2532,6 +2532,10 @@ static int sd_start(struct gspca_dev *gspca_dev)
reg17 &= ~MCK_SIZE_MASK;
reg17 |= 0x04; /* clock / 4 */
}
+ else { /* if 320x240 || 160x120 */
+ reg17 &= ~MCK_SIZE_MASK;
+ reg17 |= 0x04;
+ }
break;
case SENSOR_OV7630:
init = ov7630_sensor_param1;
However, the frame rates get limited to 10 fps. Without that change, and in
320x240 and 160x120, they reach 20 fps (of darkness).
I can't see what or'ing that register means, and what's the relationship
between this and the webcam darkness. It seems that these bits control some
kind of clock; this can be read in the program comments. One other argument in
favour of this assumption is the fact that the frame rate changes accordingly
to the value of these bits. But I can't see how this relates to exposure.
For my purposes, I'll stay with that change; it's sufficient for my purposes.
If you know what I did, please advise me. :-)
Thanks for your help,
Luiz
--- Em sex, 15/7/11, Jean-Francois Moine escreveu:
> De: Jean-Francois Moine
> Assunto: Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
> Para: "Luiz Ramos"
> Cc: linux-media@vger.kernel.org
> Data: Sexta-feira, 15 de Julho de 2011, 14:44
> On Fri, 15 Jul 2011 02:57:43 -0700
> (PDT)
> Luiz Ramos
> wrote:
>
> > Ok, I'm now grabbing this tarball:
> > http://moinejf.free.fr/gspca-2.13.2.tar.gz.
> >
> > The site also features a (some) git repository(ies)
> but I understood
> > you mean the test version, is it right?
>
> Yes. The tarball is simpler to compile and install.
>
> --
> Ken ar c'hentañ |
> ** Breizh ha Linux atav! **
> Jef
> | http://moinejf.free.fr/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
On Fri, 15 Jul 2011 02:57:43 -0700 (PDT) Luiz Ramos wrote: > Ok, I'm now grabbing this tarball: > http://moinejf.free.fr/gspca-2.13.2.tar.gz. > > The site also features a (some) git repository(ies) but I understood > you mean the test version, is it right? Yes. The tarball is simpler to compile and install. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
--- Em sex, 15/7/11, Jean-Francois Moine escreveu: > De: Jean-Francois Moine > Assunto: Re: [PATCH] Fix wrong register mask in gspca/sonixj.c > Para: linux-media@vger.kernel.org > Data: Sexta-feira, 15 de Julho de 2011, 4:48 > On Thu, 14 Jul 2011 19:08:39 -0700 > (PDT) > Luiz Ramos > wrote: > > > Signed-off-by: Luiz Carlos Ramos yahoo.com.br> > > > > > > --- a/drivers/media/video/gspca/sonixj.c > 2011-07-14 > > 13:14:41.0 -0300 +++ > > b/drivers/media/video/gspca/sonixj.c > 2011-07-14 > > 13:22:26.0 -0300 @@ -2386,7 +2386,7 @@ static > int > > sd_start(struct gspca_dev *gs reg_w1(gspca_dev, 0x01, > 0x22); > > msleep(100); reg01 = SCL_SEL_OD | S_PDN_INV; > > - > reg17 &= MCK_SIZE_MASK; > > + > reg17 &= ~MCK_SIZE_MASK; /* that is, > reset bits 4..0 */ > > > reg17 |= 0x04; /* clock / > 4 */ > > > break; > > } > > Acked-by: Jean-François Moine > > Luiz, may you get and try the last gspca tarball from my > web site? (you > will have to redo your patch, because I have not yet > uploaded it) > Ok, I'm now grabbing this tarball: http://moinejf.free.fr/gspca-2.13.2.tar.gz. The site also features a (some) git repository(ies) but I understood you mean the test version, is it right? > -- > Ken ar c'hentañ | > ** Breizh ha Linux atav! ** > Jef > | http://moinejf.free.fr/ > -- > To unsubscribe from this list: send the line "unsubscribe > linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Thanks again, Luiz -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
On Thu, 14 Jul 2011 19:08:39 -0700 (PDT) Luiz Ramos wrote: > Signed-off-by: Luiz Carlos Ramos yahoo.com.br> > > > --- a/drivers/media/video/gspca/sonixj.c2011-07-14 > 13:14:41.0 -0300 +++ > b/drivers/media/video/gspca/sonixj.c2011-07-14 > 13:22:26.0 -0300 @@ -2386,7 +2386,7 @@ static int > sd_start(struct gspca_dev *gs reg_w1(gspca_dev, 0x01, 0x22); > msleep(100); reg01 = SCL_SEL_OD | S_PDN_INV; > - reg17 &= MCK_SIZE_MASK; > + reg17 &= ~MCK_SIZE_MASK; /* that is, reset bits 4..0 */ > reg17 |= 0x04; /* clock / 4 */ > break; > } Acked-by: Jean-François Moine Luiz, may you get and try the last gspca tarball from my web site? (you will have to redo your patch, because I have not yet uploaded it) -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix wrong register mask in gspca/sonixj.c
Hello,
That's the first time I submit a patch to the Linux kernel. I hope I could do
it right; please forgive me in case of mistakes.
The context: when migrating from Slackware 13.1 to 13.37 (kernel 2.6.33.x to
2.6.37.6), there was some sort of regression with the external webcam installed
at the notebook (0x45:6128, SN9C325+OM6802). In the version 2.6.37.6, the
images got *very* dark, making the webcam almost unusable, unless if used with
direct sunlight.
Tracing back what happened, I concluded that a patch issued in 17/Dec ("[media]
gspca - sonixj: Better handling of the bridge...") caused some sort of odd
effects - including this - to this specific model. Enabling debug in both
versions, the values output to reg17 and reg01 seemed to be different from one
to other, and probably not all of them were made purposedly.
Although the work is not finished, for me it's quite certain that the masking
of the reg17 variable at line 2389 of gspca_sonixj.c (now I'm referring to the
latest version of the file at Linus' tree, as of 14/Jul) is not what the
developer intended to do. It seems to be necessary to negate the mask
MCK_SIZE_MASK when doing the "and" operation, resetting bits 0 to 4 and not the
other; if not, the "or" sentence which follows becomes nonsensical.
So, the patch is simply adding a tilde to the sentence. One character only. :-)
If you could, please review this patch and give me some advice in case of
mistakes.
As said above, this patch by itself is not sufficient to restore proper working
of the webcam (now talking about version 2.6.37.6). I am aware that some
patches which follow seem to fix things broken in that version. But trying to
simply backport the latest version to that kernel version doesn't make the
webcam work again. I'll try to go on making more tests and playing with reg17,
in special, with the latest kernel.
Thanks,
Luiz Carlos Ramos
São Paulo - Brazil
Signed-off-by: Luiz Carlos Ramos yahoo.com.br>
--- a/drivers/media/video/gspca/sonixj.c2011-07-14 13:14:41.0
-0300
+++ b/drivers/media/video/gspca/sonixj.c2011-07-14 13:22:26.0
-0300
@@ -2386,7 +2386,7 @@ static int sd_start(struct gspca_dev *gs
reg_w1(gspca_dev, 0x01, 0x22);
msleep(100);
reg01 = SCL_SEL_OD | S_PDN_INV;
- reg17 &= MCK_SIZE_MASK;
+ reg17 &= ~MCK_SIZE_MASK; /* that is, reset bits 4..0 */
reg17 |= 0x04; /* clock / 4 */
break;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
