[PATCH v2] media: i2c: imx471: Fix pixel rate and line length

Christian Murphy posted 1 patch 3 weeks, 2 days ago
drivers/media/i2c/imx471.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
[PATCH v2] media: i2c: imx471: Fix pixel rate and line length
Posted by Christian Murphy 3 weeks, 2 days ago
The pixel rate is derived from the CSI-2 link frequency, 200 MHz * 2 *
4 lanes / 10 bits = 160 MHz, and the line length is the binned array
width, 2328. Neither describes the pixel array.

The VT PLL gives 19.2 MHz / PREPLLCK_VT_DIV 2 * PLL_VT_MPY 121 /
VTPXCK_DIV 6 = 193.6 MHz and the array reads two pixels per clock, so
the pixel rate is 387.2 MHz. LINE_LENGTH_PCK is never written and its
power-on default reads back 5120. Timing frames at several written
line lengths confirms 387.2 MHz at every value.

With the declared values the line duration comes out 14.55 us instead
of 13.22 us and exposure is overstated by 10%: libcamera reports a
maximum ExposureTime longer than the frame.

Derive the pixel rate from the external clock and the VT PLL
parameters the mode table writes, and set the line length to 5120,
which changes HBLANK from 400 to 3192. No register write is added.

Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
Link: https://lore.kernel.org/linux-media/20260728042013.23707-1-hpa@redhat.com/
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Christian Murphy <christian@themurphys.eu>
---

Notes:
    Changes since v1:
    - Calculate the pixel rate from the external clock and the VT PLL
      parameters (Sakari), naming the three the mode table writes so the
      table and the calculation share them. The object code is unchanged.
    
    v1: https://lore.kernel.org/linux-media/20260830173345.15886-1-christian@themurphys.eu/
    
    LINE_LENGTH_PCK reads back 0x1400 (5120) from the streaming sensor.
    Written to 5632, 6144, 5008 and 5120 in one stream, the line period is
    llp / 387.2 MHz at every value (14.5457, 15.8680, 12.9341, 13.2234 us).
    
    Tested on a ThinkPad X1 Carbon Gen 14 (Debian linux 7.1.8-2, libcamera
    0.7.2): controls read back 387200000 / 3192, 17.296 ms frames, maximum
    ExposureTime 17.057 ms. v4l2-compliance 46/46; W=1 and sparse clean.
    The driver is new in v7.3-rc1, so no Cc: stable.

 drivers/media/i2c/imx471.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index 4053aed84340..0559195bcdd2 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -65,9 +65,12 @@
 
 /* PLL */
 #define IMX471_REG_VTPXCK_DIV			CCI_REG8(0x0301)
+#define IMX471_VTPXCK_DIV			6
 #define IMX471_REG_VTSYCK_DIV			CCI_REG8(0x0303)
 #define IMX471_REG_PREPLLCK_VT_DIV		CCI_REG8(0x0305)
+#define IMX471_PREPLLCK_VT_DIV			2
 #define IMX471_REG_PLL_VT_MPY			CCI_REG16(0x0306)
+#define IMX471_PLL_VT_MPY			121
 #define IMX471_REG_OPPXCK_DIV			CCI_REG8(0x0309)
 #define IMX471_REG_OPSYCK_DIV			CCI_REG8(0x030b)
 #define IMX471_REG_PLL_MULT_DRIV		CCI_REG8(0x0310)
@@ -232,10 +235,10 @@ static const struct cci_reg_sequence mode_1928x1088_regs[] = {
 	{ IMX471_REG_DIG_CROP_HEIGHT, 1088 },
 	{ IMX471_REG_X_OUTPUT_SIZE, 1928 },
 	{ IMX471_REG_Y_OUTPUT_SIZE, 1088 },
-	{ IMX471_REG_VTPXCK_DIV, 0x06 },
+	{ IMX471_REG_VTPXCK_DIV, IMX471_VTPXCK_DIV },
 	{ IMX471_REG_VTSYCK_DIV, 0x02 },
-	{ IMX471_REG_PREPLLCK_VT_DIV, 0x02 },
-	{ IMX471_REG_PLL_VT_MPY, 0x0079 },
+	{ IMX471_REG_PREPLLCK_VT_DIV, IMX471_PREPLLCK_VT_DIV },
+	{ IMX471_REG_PLL_VT_MPY, IMX471_PLL_VT_MPY },
 	{ IMX471_REG_OPSYCK_DIV, 0x01 },
 	{ CCI_REG8(0x030d), 0x02 },
 	{ CCI_REG8(0x030e), 0x00 },
@@ -282,7 +285,7 @@ static const struct imx471_mode imx471_modes[] = {
 		.height = 1088,
 		.fll_def = 1308,
 		.fll_min = 1308,
-		.llp = 2328,
+		.llp = 5120,
 		.default_mode_regs = mode_1928x1088_regs,
 		.default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
 	},
@@ -691,8 +694,9 @@ static int imx471_init_controls(struct imx471 *sensor)
 					   0,
 					   link_freq_menu_items);
 
-	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
+	/* The pixel array reads out two pixels per VT pixel clock */
+	pixel_rate = IMX471_EXT_CLK / IMX471_PREPLLCK_VT_DIV *
+		     IMX471_PLL_VT_MPY / IMX471_VTPXCK_DIV * 2;
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
 			  V4L2_CID_PIXEL_RATE, pixel_rate,

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.53.0
Re: [PATCH v2] media: i2c: imx471: Fix pixel rate and line length
Posted by Kate Hsuan 3 weeks ago
Hi Christian,

Thank you for working on this.

On Thu, Sep 3, 2026 at 3:36 AM Christian Murphy <christian@themurphys.eu> wrote:
>
> The pixel rate is derived from the CSI-2 link frequency, 200 MHz * 2 *
> 4 lanes / 10 bits = 160 MHz, and the line length is the binned array
> width, 2328. Neither describes the pixel array.
>
> The VT PLL gives 19.2 MHz / PREPLLCK_VT_DIV 2 * PLL_VT_MPY 121 /
> VTPXCK_DIV 6 = 193.6 MHz and the array reads two pixels per clock, so
> the pixel rate is 387.2 MHz. LINE_LENGTH_PCK is never written and its
> power-on default reads back 5120. Timing frames at several written
> line lengths confirms 387.2 MHz at every value.
>
> With the declared values the line duration comes out 14.55 us instead
> of 13.22 us and exposure is overstated by 10%: libcamera reports a
> maximum ExposureTime longer than the frame.
>
> Derive the pixel rate from the external clock and the VT PLL
> parameters the mode table writes, and set the line length to 5120,
> which changes HBLANK from 400 to 3192. No register write is added.
>
> Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
> Link: https://lore.kernel.org/linux-media/20260728042013.23707-1-hpa@redhat.com/
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Christian Murphy <christian@themurphys.eu>
> ---
>
> Notes:
>     Changes since v1:
>     - Calculate the pixel rate from the external clock and the VT PLL
>       parameters (Sakari), naming the three the mode table writes so the
>       table and the calculation share them. The object code is unchanged.
>
>     v1: https://lore.kernel.org/linux-media/20260830173345.15886-1-christian@themurphys.eu/
>
>     LINE_LENGTH_PCK reads back 0x1400 (5120) from the streaming sensor.
>     Written to 5632, 6144, 5008 and 5120 in one stream, the line period is
>     llp / 387.2 MHz at every value (14.5457, 15.8680, 12.9341, 13.2234 us).
>
>     Tested on a ThinkPad X1 Carbon Gen 14 (Debian linux 7.1.8-2, libcamera
>     0.7.2): controls read back 387200000 / 3192, 17.296 ms frames, maximum
>     ExposureTime 17.057 ms. v4l2-compliance 46/46; W=1 and sparse clean.
>     The driver is new in v7.3-rc1, so no Cc: stable.
>
>  drivers/media/i2c/imx471.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> index 4053aed84340..0559195bcdd2 100644
> --- a/drivers/media/i2c/imx471.c
> +++ b/drivers/media/i2c/imx471.c
> @@ -65,9 +65,12 @@
>
>  /* PLL */
>  #define IMX471_REG_VTPXCK_DIV                  CCI_REG8(0x0301)
> +#define IMX471_VTPXCK_DIV                      6
>  #define IMX471_REG_VTSYCK_DIV                  CCI_REG8(0x0303)
>  #define IMX471_REG_PREPLLCK_VT_DIV             CCI_REG8(0x0305)
> +#define IMX471_PREPLLCK_VT_DIV                 2
>  #define IMX471_REG_PLL_VT_MPY                  CCI_REG16(0x0306)
> +#define IMX471_PLL_VT_MPY                      121
>  #define IMX471_REG_OPPXCK_DIV                  CCI_REG8(0x0309)
>  #define IMX471_REG_OPSYCK_DIV                  CCI_REG8(0x030b)
>  #define IMX471_REG_PLL_MULT_DRIV               CCI_REG8(0x0310)
> @@ -232,10 +235,10 @@ static const struct cci_reg_sequence mode_1928x1088_regs[] = {
>         { IMX471_REG_DIG_CROP_HEIGHT, 1088 },
>         { IMX471_REG_X_OUTPUT_SIZE, 1928 },
>         { IMX471_REG_Y_OUTPUT_SIZE, 1088 },
> -       { IMX471_REG_VTPXCK_DIV, 0x06 },
> +       { IMX471_REG_VTPXCK_DIV, IMX471_VTPXCK_DIV },
>         { IMX471_REG_VTSYCK_DIV, 0x02 },
> -       { IMX471_REG_PREPLLCK_VT_DIV, 0x02 },
> -       { IMX471_REG_PLL_VT_MPY, 0x0079 },
> +       { IMX471_REG_PREPLLCK_VT_DIV, IMX471_PREPLLCK_VT_DIV },
> +       { IMX471_REG_PLL_VT_MPY, IMX471_PLL_VT_MPY },
>         { IMX471_REG_OPSYCK_DIV, 0x01 },
>         { CCI_REG8(0x030d), 0x02 },
>         { CCI_REG8(0x030e), 0x00 },
> @@ -282,7 +285,7 @@ static const struct imx471_mode imx471_modes[] = {
>                 .height = 1088,
>                 .fll_def = 1308,
>                 .fll_min = 1308,
> -               .llp = 2328,
> +               .llp = 5120,
>                 .default_mode_regs = mode_1928x1088_regs,
>                 .default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
>         },
> @@ -691,8 +694,9 @@ static int imx471_init_controls(struct imx471 *sensor)
>                                            0,
>                                            link_freq_menu_items);
>
> -       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> -       pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
> +       /* The pixel array reads out two pixels per VT pixel clock */
> +       pixel_rate = IMX471_EXT_CLK / IMX471_PREPLLCK_VT_DIV *
> +                    IMX471_PLL_VT_MPY / IMX471_VTPXCK_DIV * 2;
>
>         v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
>                           V4L2_CID_PIXEL_RATE, pixel_rate,
>
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
> --
> 2.53.0
>

I proposed a v2 patch of line pck setting and pixel rate estimation.
It can be found here
https://lore.kernel.org/linux-media/20260904075113.125751-1-hpa@redhat.com/T/#t

According to the datasheet, since this sensor runs in PLL dual mode so
the PLL configurations in the op domain are used to calculate the data
rate and pixel rate.
(If it runs in PLL single mode, vt domain settings are considered for
the data rate. If I understand the clock tree correctly)

My patchset includes naming the registers and pixel rate calculation.
Could you please look into it?

Thank you.


-- 
BR,
Kate
Re: [PATCH v2] media: i2c: imx471: Fix pixel rate and line length
Posted by Christian Murphy 2 weeks, 6 days ago
Hi Kate,

Thanks for the series.

On Fri, Sep 4, 2026 at 04:59:58PM +0800, Kate Hsuan wrote:
> According to the datasheet, since this sensor runs in PLL dual mode so
> the PLL configurations in the op domain are used to calculate the data
> rate and pixel rate.

The OP PLL clocks the CSI-2 output. PIXEL_RATE is the pixel array rate
(ext-ctrls-image-process.rst), set by the VT PLL. Your 159.36 MHz is
close to the output-derived 160 MHz the driver has.

LINE_LENGTH_PCK reads back 5120 and a frame at FLL 1308 takes
17.296 ms, so 387.2 MHz. At 159.36 MHz it would take 42 ms.

I tried writing 2560 while streaming: at FLL 1308, 6.61 us lines,
115.6 fps, frames intact, no drops or ISYS errors during the test.
With .llp still 2328 and your pixel rate, libcamera would calculate
14.6 us lines and 52 fps. The measured stream carries 2.4 Gbps of
RAW10, above the 1.6 Gbps a 200 MHz link carries on four lanes, so the
200 MHz in the driver and ipu-bridge is understated, a separate fix.

Writing 2328 gives 6.01 us lines, but the IPU gets 474 rows of
black and flags every frame BAD_FRAME_DIM. Stepping down from 2560,
2544 still gives an image, 2536 down does not, and 2416 down arrives
truncated, so 2560 sits just above the working limit here. The
12.05 us bound I gave Sakari was wrong, but 2328 does not work either.

Using 2560 to raise the frame rate makes sense. With your planned
HBLANK write, .llp needs to be 2560 too, with PIXEL_RATE at 387.2 MHz.
My patch uses 5120 because that is what the sensor currently runs at.
Could we combine the pixel-rate correction with your HBLANK change
and use .llp = 2560 consistently? Happy to test.

Kind regards,

Christian
Re: [PATCH v2] media: i2c: imx471: Fix pixel rate and line length
Posted by Kate Hsuan 2 weeks, 3 days ago
Hi Christian,

Thank you for your patches.

On Sun, Sep 6, 2026 at 1:12 AM Christian Murphy <christian@themurphys.eu> wrote:
>
> Hi Kate,
>
> Thanks for the series.
>
> On Fri, Sep 4, 2026 at 04:59:58PM +0800, Kate Hsuan wrote:
> > According to the datasheet, since this sensor runs in PLL dual mode so
> > the PLL configurations in the op domain are used to calculate the data
> > rate and pixel rate.
>
> The OP PLL clocks the CSI-2 output. PIXEL_RATE is the pixel array rate
> (ext-ctrls-image-process.rst), set by the VT PLL. Your 159.36 MHz is
> close to the output-derived 160 MHz the driver has.
>
> LINE_LENGTH_PCK reads back 5120 and a frame at FLL 1308 takes
> 17.296 ms, so 387.2 MHz. At 159.36 MHz it would take 42 ms.

I went to the VT domain at the beginning, but after reading the
datasheet, it mentioned, "IOPSYCK determines the pixel rate". I
switched to the OP domain. Perhpas, the datasheet and MIPI datasheet
confused me.

>
> I tried writing 2560 while streaming: at FLL 1308, 6.61 us lines,
> 115.6 fps, frames intact, no drops or ISYS errors during the test.
> With .llp still 2328 and your pixel rate, libcamera would calculate
> 14.6 us lines and 52 fps. The measured stream carries 2.4 Gbps of

That is why I try to set it to the original value 2560 for line length PCK.
Idealy, the sensor supports 2328x1304 @150fps (binning mode)
based on the new value 0x0a00 (2560) (ipu6-drivers), libcamera reports
115.63 fps on my X9-15.

> RAW10, above the 1.6 Gbps a 200 MHz link carries on four lanes, so the
> 200 MHz in the driver and ipu-bridge is understated, a separate fix.
>
> Writing 2328 gives 6.01 us lines, but the IPU gets 474 rows of
> black and flags every frame BAD_FRAME_DIM. Stepping down from 2560,
> 2544 still gives an image, 2536 down does not, and 2416 down arrives
> truncated, so 2560 sits just above the working limit here. The
> 12.05 us bound I gave Sakari was wrong, but 2328 does not work either.


>
> Using 2560 to raise the frame rate makes sense. With your planned
> HBLANK write, .llp needs to be 2560 too, with PIXEL_RATE at 387.2 MHz.
> My patch uses 5120 because that is what the sensor currently runs at.
> Could we combine the pixel-rate correction with your HBLANK change
> and use .llp = 2560 consistently? Happy to test.
It's ok. sure

for the line length PCK part, I try to propose a variable setting like
what mt9m114 did.
Line length PCK can be changed along with HBLANK control. This may
allow hblank to be configured and fps to be changed.

Compare to your pixel rate equation with the equation mentioned in the
MIPI datasheet + PLL tree in the imx471 datasheet.

imx471 signal processing rate is determined by ext freq,
VT_PREPLLCK_DIV, VT_PLL_MPY, and VT_SYCK_DIV * VT_PXCK_DIV.
so it may look like
signal processing rate = (ext freq * VT_PLL_MPY) / (VT_PREPLLCK_DIV *
VT_SYCK_DIV * VT_PXCK_DIV).
(I think this is per lane)

and
MIPI spec mentions the pixel speed is:
ext_vt_pixel_clk_freq_MHz = (ext_clk_freq_MHz * vt_pll_multiplier *
num_of_vt_lanes) / (vt_pre_pll_clk_div * vt_sys_clk_div *
vt_pixel_clk_div)
(19200000 * 121 * 4) / (2 * 2 * 6) = 387200000

The result is the same as your pixel rate 387.2Mhz. Could you consider
this approach?

>
> Kind regards,
>
> Christian
>


--
BR,
Kate