[3/3] media: i2c: imx219: Add support 640x480

  This patch adds support to 640x480 cropped resolution for the sensor

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/imx219.c | 70 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
  

Hi Lad.

Thanks for the patch.

On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> This patch adds support to 640x480 cropped resolution for the sensor

I was a little hesitant to add cropped modes without good reason.
Processing them through an ISP with something like lens shading
compensation requires the ISP to know the crop, so ideally it should
be reflected through the selection API (probably read-only - I'm not
sure you can modify the register set totally dynamically for
cropping).
I know we have the 1080p mode in there already which is cropped, but
that was mainly as it is the only way to get 30fps 1080p over two
CSI-2 lanes. I wonder if there is a better way of reflecting this.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/imx219.c | 70 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 1388c9bc00bb..232ebf41063a 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -54,6 +54,7 @@
>  #define IMX219_VTS_15FPS               0x0dc6
>  #define IMX219_VTS_30FPS_1080P         0x06e3
>  #define IMX219_VTS_30FPS_BINNED                0x06e3
> +#define IMX219_VTS_30FPS_640x480       0x0239
>  #define IMX219_VTS_MAX                 0xffff
>
>  #define IMX219_VBLANK_MIN              4
> @@ -329,6 +330,65 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>         {0x0163, 0x78},
>  };
>
> +static const struct imx219_reg mode_640_480_regs[] = {

Can I ask where these register settings came from? They differ from
references I have in a few odd ways.

There's also a comment at the top of mode arrays declaring the
supported modes and where they came from. Could you update that
please?

> +       {0x0100, 0x00},
> +       {0x30eb, 0x0c},
> +       {0x30eb, 0x05},
> +       {0x300a, 0xff},
> +       {0x300b, 0xff},
> +       {0x30eb, 0x05},
> +       {0x30eb, 0x09},

Datasheet section 3-4 says these are to access manufacturer specific
registers, but the access sequence should be
0x30eb 0x05
0x30eb 0x0c
0x300a 0xff
0x300b 0xff
0x30eb 0x05
0x30eb 0x09
Is there a reason your first two writes are reversed compared to this
published order?

> +       {0x0114, 0x01},
> +       {0x0128, 0x01},

DPHY_CTRL RW MIPI Global timing setting
0:auto mode, 1:manual mode

All the other modes have this as auto mode. Why does this mode need
manual settings, and is something else configuring those manual
values?

> +       {0x012a, 0x18},
> +       {0x012b, 0x00},
> +       {0x0162, 0x0d},
> +       {0x0163, 0xe7},

All the other modes have set line length to 0x0d78 (3448 decimal)
rather than your 0xd37 (3559).
Is there any specific reason for this? If we need a different value,
then we also need to vary IMX219_PPL_DEFAULT and V4L2_CID_HBLANK
depending on mode. Or probably better would be to make it variable,
but that has a load of other implications.

> +       {0x0164, 0x03},
> +       {0x0165, 0xe8},
> +       {0x0166, 0x08},
> +       {0x0167, 0xe7},
> +       {0x0168, 0x02},
> +       {0x0169, 0xf0},
> +       {0x016a, 0x06},
> +       {0x016b, 0xaf},
> +       {0x016c, 0x02},
> +       {0x016d, 0x80},
> +       {0x016e, 0x01},
> +       {0x016f, 0xe0},
> +       {0x0170, 0x01},
> +       {0x0171, 0x01},
> +       {0x0172, 0x00},

0x0172 is IMAGE_ORIENTATION_A, which is handled via V4L2_CID_HFLIP /
V4L2_CID_VFLIP, not in the mode table.

> +       {0x0174, 0x03},
> +       {0x0175, 0x03},
> +       {0x0301, 0x05},
> +       {0x0303, 0x01},
> +       {0x0304, 0x03},
> +       {0x0305, 0x03},
> +       {0x0306, 0x00},
> +       {0x0307, 0x39},
> +       {0x0309, 0x08},

"OPPXCK_DIV. Ouptut pixel clock divider value, default 0x0A."
This looks like it is a change that should be part of the support for
8bit formats.
Have you tested this mode with 10bit readout? Are the data rates correct?

> +       {0x030b, 0x01},
> +       {0x030c, 0x00},
> +       {0x030d, 0x72},
> +       {0x0624, 0x06},
> +       {0x0625, 0x68},
> +       {0x0626, 0x04},
> +       {0x0627, 0xd0},
> +       {0x455e, 0x00},
> +       {0x471e, 0x4b},
> +       {0x4767, 0x0f},
> +       {0x4750, 0x14},
> +       {0x4540, 0x00},
> +       {0x47b4, 0x14},
> +       {0x4713, 0x30},
> +       {0x478b, 0x10},
> +       {0x478f, 0x10},
> +       {0x4793, 0x10},
> +       {0x4797, 0x0e},
> +       {0x479b, 0x0e},
> +};
> +
>  static const char * const imx219_test_pattern_menu[] = {
>         "Disabled",
>         "Color Bars",
> @@ -414,6 +474,16 @@ static const struct imx219_mode supported_modes[] = {
>                         .regs = mode_1640_1232_regs,
>                 },
>         },
> +       {
> +               /* 640x480 30fps mode */
>
> +               .width = 640,
> +               .height = 480,
> +               .vts_def = IMX219_VTS_30FPS_640x480,

I've just run this mode on a Pi and I get a default of about 84fps via
v4l2-ctl to /dev/null. Is the default frame rate expected to be 30fps?
In which case I think the value of IMX219_VTS_30FPS_640x480 is wrong
(I'd expect 0x6e3 again, same as the other modes), or the comments and
define names are wrong. One or other ought to be fixed.

My calculations say that with:
- VBLANK set to 89
- a pixel rate of 182400000 (based on IMX219_PIXEL_RATE)
- HBLANK fixed at 2808
- frame being 640x480
The overall frame size is therefore (640+2808) * (480+89) = 1961912
pixel clocks. That would at first glance appear to give a frame rate
of 92fps. Testing with an alternate tool is giving me timings for
90fps but with a few dropped frames (the dropped frames would explain
v4l2-ctl reading slightly low).

If I amend OPPXCK_DIV to be 0xA (the same as the other modes), then it
doesn't appear to change.
However hold off on investigating the specifics for now - I appear to
be unable to select the 10bit/pixel formats, so I suspect something is
up with patch 2 that added the 8bit support (I was about to review
that anyway).

  Dave

> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> +                       .regs = mode_640_480_regs,
> +               },
> +       },
>  };
>
>  struct imx219 {
> --
> 2.20.1
>
  

Hi Dave,

Thank you for the review.

On Mon, Mar 2, 2020 at 12:50 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Lad.
>
> Thanks for the patch.
>
> On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > This patch adds support to 640x480 cropped resolution for the sensor
>
> I was a little hesitant to add cropped modes without good reason.
> Processing them through an ISP with something like lens shading
> compensation requires the ISP to know the crop, so ideally it should
> be reflected through the selection API (probably read-only - I'm not
> sure you can modify the register set totally dynamically for
> cropping).
> I know we have the 1080p mode in there already which is cropped, but
> that was mainly as it is the only way to get 30fps 1080p over two
> CSI-2 lanes. I wonder if there is a better way of reflecting this.
>
The CSI controller which I am using doesn't support capture of higher
resolutions
as a result I have added support for a lower resolution.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/imx219.c | 70 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 1388c9bc00bb..232ebf41063a 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -54,6 +54,7 @@
> >  #define IMX219_VTS_15FPS               0x0dc6
> >  #define IMX219_VTS_30FPS_1080P         0x06e3
> >  #define IMX219_VTS_30FPS_BINNED                0x06e3
> > +#define IMX219_VTS_30FPS_640x480       0x0239
> >  #define IMX219_VTS_MAX                 0xffff
> >
> >  #define IMX219_VBLANK_MIN              4
> > @@ -329,6 +330,65 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >         {0x0163, 0x78},
> >  };
> >
> > +static const struct imx219_reg mode_640_480_regs[] = {
>
> Can I ask where these register settings came from? They differ from
> references I have in a few odd ways.
>
This driver was developed in house for imx219 sensor.

> There's also a comment at the top of mode arrays declaring the
> supported modes and where they came from. Could you update that
> please?
>
Sure ill  update it to the following:
/*
 * Register sets lifted off the i2C interface from the Raspberry Pi firmware
 * driver for resolutions 3280x2464, 1920x1080 1640x1232.
 * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.
 */

> > +       {0x0100, 0x00},
> > +       {0x30eb, 0x0c},
> > +       {0x30eb, 0x05},
> > +       {0x300a, 0xff},
> > +       {0x300b, 0xff},
> > +       {0x30eb, 0x05},
> > +       {0x30eb, 0x09},
>
> Datasheet section 3-4 says these are to access manufacturer specific
> registers, but the access sequence should be
> 0x30eb 0x05
> 0x30eb 0x0c
> 0x300a 0xff
> 0x300b 0xff
> 0x30eb 0x05
> 0x30eb 0x09
> Is there a reason your first two writes are reversed compared to this
> published order?
>
Agreed I have tested both the sequence works, I have now replaced as
mentioned in datasheet.

> > +       {0x0114, 0x01},
> > +       {0x0128, 0x01},
>
> DPHY_CTRL RW MIPI Global timing setting
> 0:auto mode, 1:manual mode
>
> All the other modes have this as auto mode. Why does this mode need
> manual settings, and is something else configuring those manual
> values?
>
I have reverted it to auto mode.

> > +       {0x012a, 0x18},
> > +       {0x012b, 0x00},
> > +       {0x0162, 0x0d},
> > +       {0x0163, 0xe7},
>
> All the other modes have set line length to 0x0d78 (3448 decimal)
> rather than your 0xd37 (3559).
> Is there any specific reason for this? If we need a different value,
> then we also need to vary IMX219_PPL_DEFAULT and V4L2_CID_HBLANK
> depending on mode. Or probably better would be to make it variable,
> but that has a load of other implications.
>
line length of 0x0d78 works as expected, so I have changed it now.

> > +       {0x0164, 0x03},
> > +       {0x0165, 0xe8},
> > +       {0x0166, 0x08},
> > +       {0x0167, 0xe7},
> > +       {0x0168, 0x02},
> > +       {0x0169, 0xf0},
> > +       {0x016a, 0x06},
> > +       {0x016b, 0xaf},
> > +       {0x016c, 0x02},
> > +       {0x016d, 0x80},
> > +       {0x016e, 0x01},
> > +       {0x016f, 0xe0},
> > +       {0x0170, 0x01},
> > +       {0x0171, 0x01},
> > +       {0x0172, 0x00},
>
> 0x0172 is IMAGE_ORIENTATION_A, which is handled via V4L2_CID_HFLIP /
> V4L2_CID_VFLIP, not in the mode table.
>
dropped this setting.

> > +       {0x0174, 0x03},
> > +       {0x0175, 0x03},
> > +       {0x0301, 0x05},
> > +       {0x0303, 0x01},
> > +       {0x0304, 0x03},
> > +       {0x0305, 0x03},
> > +       {0x0306, 0x00},
> > +       {0x0307, 0x39},
> > +       {0x0309, 0x08},
>
> "OPPXCK_DIV. Ouptut pixel clock divider value, default 0x0A."
> This looks like it is a change that should be part of the support for
> 8bit formats.
> Have you tested this mode with 10bit readout? Are the data rates correct?
>
as you discovered the vlaue should be 0x0A for 640x480. No I havent
tested it for
10bit.

> > +       {0x030b, 0x01},
> > +       {0x030c, 0x00},
> > +       {0x030d, 0x72},
> > +       {0x0624, 0x06},
> > +       {0x0625, 0x68},
> > +       {0x0626, 0x04},
> > +       {0x0627, 0xd0},
> > +       {0x455e, 0x00},
> > +       {0x471e, 0x4b},
> > +       {0x4767, 0x0f},
> > +       {0x4750, 0x14},
> > +       {0x4540, 0x00},
> > +       {0x47b4, 0x14},
> > +       {0x4713, 0x30},
> > +       {0x478b, 0x10},
> > +       {0x478f, 0x10},
> > +       {0x4793, 0x10},
> > +       {0x4797, 0x0e},
> > +       {0x479b, 0x0e},
> > +};
> > +
> >  static const char * const imx219_test_pattern_menu[] = {
> >         "Disabled",
> >         "Color Bars",
> > @@ -414,6 +474,16 @@ static const struct imx219_mode supported_modes[] = {
> >                         .regs = mode_1640_1232_regs,
> >                 },
> >         },
> > +       {
> > +               /* 640x480 30fps mode */
> >
> > +               .width = 640,
> > +               .height = 480,
> > +               .vts_def = IMX219_VTS_30FPS_640x480,
>
> I've just run this mode on a Pi and I get a default of about 84fps via
> v4l2-ctl to /dev/null. Is the default frame rate expected to be 30fps?
> In which case I think the value of IMX219_VTS_30FPS_640x480 is wrong
> (I'd expect 0x6e3 again, same as the other modes), or the comments and
> define names are wrong. One or other ought to be fixed.
>
> My calculations say that with:
> - VBLANK set to 89
> - a pixel rate of 182400000 (based on IMX219_PIXEL_RATE)
> - HBLANK fixed at 2808
> - frame being 640x480
> The overall frame size is therefore (640+2808) * (480+89) = 1961912
> pixel clocks. That would at first glance appear to give a frame rate
> of 92fps. Testing with an alternate tool is giving me timings for
> 90fps but with a few dropped frames (the dropped frames would explain
> v4l2-ctl reading slightly low).
>
I have set the value 0x06e3 and yavta reports 30fps:
Device /dev/video0 opened.ideo0 -c0 -n10 -s640x480 -fSRGGB8  --field
none -R80 -F
Device `R_Car_VIN' on `platform:e6ef4000.video' is a video output
(without mplanes) device.
Video format set: SRGGB8 (42474752) 640x480 (stride 640) field none
buffer size 307200
Video format: SRGGB8 (42474752) 640x480 (stride 640) field none buffer
size 307200
10 buffers requested.
length: 307200 offset: 0 timestamp type/source: mono/EoF
Buffer 0/0 mapped at address 0xffffa8126000.
length: 307200 offset: 307200 timestamp type/source: mono/EoF
Buffer 1/0 mapped at address 0xffffa80db000.
length: 307200 offset: 614400 timestamp type/source: mono/EoF
Buffer 2/0 mapped at address 0xffffa8090000.
length: 307200 offset: 921600 timestamp type/source: mono/EoF
Buffer 3/0 mapped at address 0xffffa8045000.
length: 307200 offset: 1228800 timestamp type/source: mono/EoF
Buffer 4/0 mapped at address 0xffffa7ffa000.
length: 307200 offset: 1536000 timestamp type/source: mono/EoF
Buffer 5/0 mapped at address 0xffffa7faf000.
length: 307200 offset: 1843200 timestamp type/source: mono/EoF
Buffer 6/0 mapped at address 0xffffa7f64000.
length: 307200 offset: 2150400 timestamp type/source: mono/EoF
Buffer 7/0 mapped at address 0xffffa7f19000.
length: 307200 offset: 2457600 timestamp type/source: mono/EoF
Buffer 8/0 mapped at address 0xffffa7ece000.
length: 307200 offset: 2764800 timestamp type/source: mono/EoF
Buffer 9/0 mapped at address 0xffffa7e83000.
0 (0) [-] none 0 307200 B 2227.060205 2227.060268 10.119 fps ts mono/EoF
1 (1) [-] none 1 307200 B 2227.093536 2227.105537 30.002 fps ts mono/EoF
2 (2) [-] none 2 307200 B 2227.126860 2227.301819 30.008 fps ts mono/EoF
3 (3) [-] none 3 307200 B 2227.160185 2227.340688 30.008 fps ts mono/EoF
4 (4) [-] none 4 307200 B 2227.193511 2227.384831 30.007 fps ts mono/EoF
5 (5) [-] none 5 307200 B 2227.226834 2227.431937 30.009 fps ts mono/EoF
6 (6) [-] none 6 307200 B 2227.260160 2227.476214 30.007 fps ts mono/EoF
7 (7) [-] none 7 307200 B 2227.293486 2227.522586 30.007 fps ts mono/EoF
8 (8) [-] none 8 307200 B 2227.326817 2227.564954 30.002 fps ts mono/EoF
9 (9) [-] none 9 307200 B 2227.360143 2227.610001 30.007 fps ts mono/EoF
Captured 10 frames in 0.648624 seconds (15.417250 fps, 4736179.062103 B/s).
10 buffers released.
root@ek874:~#

Cheers,
--Prabhakar

> If I amend OPPXCK_DIV to be 0xA (the same as the other modes), then it
> doesn't appear to change.
> However hold off on investigating the specifics for now - I appear to
> be unable to select the 10bit/pixel formats, so I suspect something is
> up with patch 2 that added the 8bit support (I was about to review
> that anyway).
>
>   Dave
>
> > +               .reg_list = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> > +                       .regs = mode_640_480_regs,
> > +               },
> > +       },
> >  };
> >
> >  struct imx219 {
> > --
> > 2.20.1
> >