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

[Kate Hsuan]

Hi Christian,

Thank you for working on this.

On Thu, Sep 3, 2026 at 3:36 AM Christian Murphy <christian@xxxxxxxxxxxxx> 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@xxxxxxxxxx/
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Christian Murphy <christian@xxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxx/
>
>     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@xxxxxxxxxx/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