[PATCH] Fix wrong register mask in gspca/sonixj.c

[PATCH] Fix wrong register mask in gspca/sonixj.c

[Luiz Ramos]

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 <lramos.prof <at> yahoo.com.br>


--- a/drivers/media/video/gspca/sonixj.c        2011-07-14 13:14:41.000000000 -0300
+++ b/drivers/media/video/gspca/sonixj.c        2011-07-14 13:22:26.000000000 -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;
        }

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Jean-Francois Moine]

On Thu, 14 Jul 2011 19:08:39 -0700 (PDT)
Luiz Ramos <luizzramos@yahoo.com.br> wrote:

> Signed-off-by: Luiz Carlos Ramos <lramos.prof <at> yahoo.com.br>
> 
> 
> --- a/drivers/media/video/gspca/sonixj.c        2011-07-14
> 13:14:41.000000000 -0300 +++
> b/drivers/media/video/gspca/sonixj.c        2011-07-14
> 13:22:26.000000000 -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 <moinejf@free.fr>

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/

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Luiz Ramos]

--- Em sex, 15/7/11, Jean-Francois Moine <moinejf@free.fr> escreveu:

> De: Jean-Francois Moine <moinejf@free.fr>
> 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 <luizzramos@yahoo.com.br>
> wrote:
> 
> > Signed-off-by: Luiz Carlos Ramos <lramos.prof
> <at> yahoo.com.br>
> > 
> > 
> > --- a/drivers/media/video/gspca/sonixj.c   
>     2011-07-14
> > 13:14:41.000000000 -0300 +++
> > b/drivers/media/video/gspca/sonixj.c   
>     2011-07-14
> > 13:22:26.000000000 -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 <moinejf@free.fr>
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks again,

Luiz

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Jean-Francois Moine]

On Fri, 15 Jul 2011 02:57:43 -0700 (PDT)
Luiz Ramos <lramos.prof@yahoo.com.br> 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/

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Luiz Ramos]

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 <moinejf@free.fr> escreveu:

> De: Jean-Francois Moine <moinejf@free.fr>
> Assunto: Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
> Para: "Luiz Ramos" <lramos.prof@yahoo.com.br>
> 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 <lramos.prof@yahoo.com.br>
> 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/
> 

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Jean-Francois Moine]

On Mon, 18 Jul 2011 18:39:14 -0700 (PDT)
Luiz Ramos <lramos.prof@yahoo.com.br> 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/

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Luiz Ramos]

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 <moinejf@free.fr> escreveu:

> De: Jean-Francois Moine <moinejf@free.fr>
> Assunto: Re: [PATCH] Fix wrong register mask in gspca/sonixj.c
> Para: "Luiz Ramos" <lramos.prof@yahoo.com.br>
> 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 <lramos.prof@yahoo.com.br>
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Fix wrong register mask in gspca/sonixj.c

[Jean-Francois Moine]

On Thu, 21 Jul 2011 03:43:13 -0700 (PDT)
Luiz Ramos <lramos.prof@yahoo.com.br> 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/