[06/16] media: i2c: ov9282: Correct HTS register for configured pixel rate

  The calculations from pixel rate, width+hblank, and height+vblank
do not give the correct framerate - it's half the speed it should
be.

Whilst not documented as such, the TIMING_HTS register (0x380c/d)
appears to be in units of 2 pixels.
The default is 0x2d8 (728) which can not be valid as-is when the
frame is 1280 active pixels wide. Doubling to 0x5b0 (1456) results
in the correct timings.

This driver isn't using the default frame width + hblank, so
use 0x02fd which is half of the width of 1280 and hblank of 250
which is reported to userspace. With this the frame rate calculations
work correctly.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Hi Dave,

On Wed, Oct 05, 2022 at 04:27:59PM +0100, Dave Stevenson wrote:
> The calculations from pixel rate, width+hblank, and height+vblank
> do not give the correct framerate - it's half the speed it should
> be.
>
> Whilst not documented as such, the TIMING_HTS register (0x380c/d)
> appears to be in units of 2 pixels.
> The default is 0x2d8 (728) which can not be valid as-is when the
> frame is 1280 active pixels wide. Doubling to 0x5b0 (1456) results
> in the correct timings.

Not sure I get this one. If TIMING_HTS has a 2-pixels units, won't
0x2d8 be correct (as it equals to your desired 1456 pixels) ?

>
> This driver isn't using the default frame width + hblank, so
> use 0x02fd which is half of the width of 1280 and hblank of 250
> which is reported to userspace. With this the frame rate calculations
> work correctly.

So this patch is basically changing the default vblank from 728 to 760 ?

Should we rather move the per-mode blankings to the mode definition
and program them when applying a new mode ?

>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index f7823d584522..1cd6cb4addfb 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -242,8 +242,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3809, 0x00},
>  	{0x380a, 0x02},
>  	{0x380b, 0xd0},
> -	{0x380c, 0x05},
> -	{0x380d, 0xfa},
> +	{0x380c, 0x02},
> +	{0x380d, 0xfd},
>  	{0x3810, 0x00},
>  	{0x3811, 0x08},
>  	{0x3812, 0x00},
> --
> 2.34.1
>
  

Hi Jacopo

Thanks for the review (missed responding on my first pass)

On Thu, 6 Oct 2022 at 10:23, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave,
>
> On Wed, Oct 05, 2022 at 04:27:59PM +0100, Dave Stevenson wrote:
> > The calculations from pixel rate, width+hblank, and height+vblank
> > do not give the correct framerate - it's half the speed it should
> > be.
> >
> > Whilst not documented as such, the TIMING_HTS register (0x380c/d)
> > appears to be in units of 2 pixels.
> > The default is 0x2d8 (728) which can not be valid as-is when the
> > frame is 1280 active pixels wide. Doubling to 0x5b0 (1456) results
> > in the correct timings.
>
> Not sure I get this one. If TIMING_HTS has a 2-pixels units, won't
> 0x2d8 be correct (as it equals to your desired 1456 pixels) ?

Bad wording by me.
From the datasheet the default register value is 0x2d8, which is less
than the width.
Treating it as 0x2d8 * 2 = 1456 gives the right numbers.

> >
> > This driver isn't using the default frame width + hblank, so
> > use 0x02fd which is half of the width of 1280 and hblank of 250
> > which is reported to userspace. With this the frame rate calculations
> > work correctly.
>
> So this patch is basically changing the default vblank from 728 to 760 ?

Again bad wording.
The driver was using a value of 0x5fa intending it to be 1530 pixels.
(As discussed later, this is due to the driver using non-continuous
clock mode, which needs an increased hblanking).
However as the units are 2-pixels, the actual value required is 0x5fa
/ 2 = 0x2fd.

I'll reword this.

> Should we rather move the per-mode blankings to the mode definition
> and program them when applying a new mode ?
>
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index f7823d584522..1cd6cb4addfb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -242,8 +242,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x3809, 0x00},
> >       {0x380a, 0x02},
> >       {0x380b, 0xd0},
> > -     {0x380c, 0x05},
> > -     {0x380d, 0xfa},
> > +     {0x380c, 0x02},
> > +     {0x380d, 0xfd},
> >       {0x3810, 0x00},
> >       {0x3811, 0x08},
> >       {0x3812, 0x00},
> > --
> > 2.34.1
> >