On Thu, Jan 23, 2025 at 2:09 AM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Dave,
>
> On Wed, Jan 22, 2025 at 04:24:10PM +0000, Dave Stevenson wrote:
> > Hi Sakari
> >
> > On Wed, 22 Jan 2025 at 12:01, Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > >
> > > Hi Dave,
> > >
> > > On Mon, Jan 20, 2025 at 11:35:40AM +0000, Dave Stevenson wrote:
> > > > Commit ceddfd4493b3 ("media: i2c: imx219: Support four-lane operation")
> > > > added support for device tree to allow configuration of the sensor to
> > > > use 4 lanes with a link frequency of 363MHz, and amended the advertised
> > > > pixel rate to 280.8MPix/s.
> > > >
> > > > However it didn't change any of the PLL settings, so actually it would
> > > > have been running overclocked in the MIPI block, and with the frame
> > > > rate and exposure calculations being wrong as the pixel rate was
> > > > unchanged.
> > > >
> > > > The pixel rate and link frequency advertised were taken from the "Clock
> > > > Setting Example" section of the datasheet. However those are based on an
> > > > external clock of 12MHz, and are unachievable with a clock of 24MHz - it
> > > > seems PREPLLCLK_VT_DIV and PREPLLCK_OP_DIV can ONLY be set via the
> > > > automatic configuration documented in "9-1-2 EXCK_FREQ setting depend on
> > > > INCK frequency", not by writing the registers.
> > > > The closest we can get with a 24MHz clock is 281.6MPix/s and 364MHz.
> > > >
> > > > Dropping all support for the 363MHz link frequency would cause problems
> > > > for existing users, so allow it, but log a warning that the requested
> > > > value is being changed to the supported one.
> > > >
> > > > Fixes: ceddfd4493b3 ("media: i2c: imx219: Support four-lane operation")
> > > > Co-developed-by: Peyton Howe <peyton.howe@xxxxxxxxxxxxx>
> > > > Signed-off-by: Peyton Howe <peyton.howe@xxxxxxxxxxxxx>
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > > > ---
> > > > This was fed back to us by Peyton Howe as giving image corruption
> > > > on a Raspberry Pi with DF Robot imx219 module, and confirmed with
> > > > a Soho Enterprises module.
> > > > Effectively the module was being overclocked and misbehaving.
> > > >
> > > > With this patch and the Soho Enterprises module no image corruption
> > > > is observed, and the frame rates are spot on. I haven't checked
> > > > exposure times, but those should follow frame rate as they are
> > > > based on the same clock.
> > > > ---
> > > > drivers/media/i2c/imx219.c | 78 ++++++++++++++++++++++++++++++++++++----------
> > > > 1 file changed, 61 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index 2d54cea113e1..562b3eb0cb1e 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -133,10 +133,11 @@
> > > >
> > > > /* Pixel rate is fixed for all the modes */
> > > > #define IMX219_PIXEL_RATE 182400000
> > > > -#define IMX219_PIXEL_RATE_4LANE 280800000
> > > > +#define IMX219_PIXEL_RATE_4LANE 281600000
> > > >
> > > > #define IMX219_DEFAULT_LINK_FREQ 456000000
> > > > -#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000
> > > > +#define IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED 363000000
> > > > +#define IMX219_DEFAULT_LINK_FREQ_4LANE 364000000
> > >
> > > This shows again the ill effects of register list based drivers. :-(
> >
> > Seeing as it was the PLL setup that was wrong, I believe the only
> > drivers that may have avoided it are MT9P031, AR0521, and CCS assuming
> > they compute all PLL settings correctly off an arbitrary pixel clock.
>
> ov5640 does that, too, but it has a mode list.
>
> >
> > Unfortunately I had no hardware to test the original patch adding 4
> > lane support, and as the datasheet doesn't lay out the actual PLL
> > configuration required for each option I hadn't twigged it was
> > required. I would have hoped that the author of that patch would have
> > noticed the frame rates were wrong, but things are never perfect.
I didn't see video issues when I submitted the original patch. I
don't know if I just lucked out with having hardware that tolerated
overclocking, or if it was due to the resolution or frame rate I was
testing.
> >
> > And I assume that your comment means that we won't see Intel
> > submitting any register list based drivers in future? I'll be quite
> > happy not having to rework them due to only supporting a 19.2MHz clock
> > :-)
My original design for the IMX219 didn't use a 24MHz ref clock, but I
noticed the datasheet supported different ref clocks. I had
considered adding support for the clock we originally designed, but I
convinced the hardware guys to swap out the clock, so didn't spend a
lot of time on the investigating the PLL's or clocking structure.
Ironically, I was afraid to break something, which I apparently
succeeded in doing anyway. :-(
Sorry about that.
>
> I'd wish I could say I won't merge any new register list based drivers but
> that would mean there would be very, very few new sensor drivers. :-(
> Register list based are here to stay, I'm afraid.
>
> >
> > > >
> > > > /* IMX219 native and active pixel array size. */
> > > > #define IMX219_NATIVE_WIDTH 3296U
> > > > @@ -168,15 +169,6 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
> > > > { CCI_REG8(0x30eb), 0x05 },
> > > > { CCI_REG8(0x30eb), 0x09 },
> > > >
> > > > - /* PLL Clock Table */
> > > > - { IMX219_REG_VTPXCK_DIV, 5 },
> > > > - { IMX219_REG_VTSYCK_DIV, 1 },
> > > > - { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */
> > > > - { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */
> > > > - { IMX219_REG_PLL_VT_MPY, 57 },
> > > > - { IMX219_REG_OPSYCK_DIV, 1 },
> > > > - { IMX219_REG_PLL_OP_MPY, 114 },
> > > > -
> > > > /* Undocumented registers */
> > > > { CCI_REG8(0x455e), 0x00 },
> > > > { CCI_REG8(0x471e), 0x4b },
> > > > @@ -201,6 +193,34 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
> > > > { IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(IMX219_XCLK_FREQ / 1000000) },
> > > > };
> > > >
> > > > +static const struct cci_reg_sequence imx219_2lane_regs[] = {
> > > > + /* PLL Clock Table */
> > > > + { IMX219_REG_VTPXCK_DIV, 5 },
> > > > + { IMX219_REG_VTSYCK_DIV, 1 },
> > > > + { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */
> > > > + { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */
> > > > + { IMX219_REG_PLL_VT_MPY, 57 },
> > > > + { IMX219_REG_OPSYCK_DIV, 1 },
> > > > + { IMX219_REG_PLL_OP_MPY, 114 },
> > > > +
> > > > + /* 2-Lane CSI Mode */
> > > > + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_2_LANE_MODE },
> > > > +};
> > > > +
> > > > +static const struct cci_reg_sequence imx219_4lane_regs[] = {
> > > > + /* PLL Clock Table */
> > > > + { IMX219_REG_VTPXCK_DIV, 5 },
> > > > + { IMX219_REG_VTSYCK_DIV, 1 },
> > > > + { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */
> > > > + { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */
> > > > + { IMX219_REG_PLL_VT_MPY, 88 },
> > > > + { IMX219_REG_OPSYCK_DIV, 1 },
> > > > + { IMX219_REG_PLL_OP_MPY, 91 },
> > > > +
> > > > + /* 4-Lane CSI Mode */
> > > > + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_4_LANE_MODE },
> > > > +};
Instead of having a few tables of registers, would it make sense to
have one common table and a small function to dynamically calculate
these registers based on the ref clock and the desired link rate?
Several of these values appear to be duplicates anyway.
> > > > +
> > > > static const s64 imx219_link_freq_menu[] = {
> > > > IMX219_DEFAULT_LINK_FREQ,
> > > > };
> > > > @@ -662,9 +682,11 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> > > >
> > > > static int imx219_configure_lanes(struct imx219 *imx219)
> > > > {
> > > > - return cci_write(imx219->regmap, IMX219_REG_CSI_LANE_MODE,
> > > > - imx219->lanes == 2 ? IMX219_CSI_2_LANE_MODE :
> > > > - IMX219_CSI_4_LANE_MODE, NULL);
> > > > + /* Write the appropriate PLL settings for the number of MIPI lanes */
> > > > + return cci_multi_reg_write(imx219->regmap,
> > > > + imx219->lanes == 2 ? imx219_2lane_regs : imx219_4lane_regs,
> > > > + imx219->lanes == 2 ? ARRAY_SIZE(imx219_2lane_regs) :
> > > > + ARRAY_SIZE(imx219_4lane_regs), NULL);
> > > > };
> > > >
> > > > static int imx219_start_streaming(struct imx219 *imx219,
> > > > @@ -1036,6 +1058,7 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > > .bus_type = V4L2_MBUS_CSI2_DPHY
> > > > };
> > > > int ret = -EINVAL;
> > > > + bool link_frequency_valid = false;
> > > >
> > > > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > > > if (!endpoint)
> > > > @@ -1062,9 +1085,30 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > > > goto error_out;
> > > > }
> > > >
> > > > - if (ep_cfg.nr_of_link_frequencies != 1 ||
> > > > - (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > > > - IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > > + if (ep_cfg.nr_of_link_frequencies == 1) {
> > > > + switch (imx219->lanes) {
> > > > + case 2:
> > > > + if (ep_cfg.link_frequencies[0] ==
> > > > + IMX219_DEFAULT_LINK_FREQ)
> > > > + link_frequency_valid = true;
> > > > + break;
> > > > + case 4:
> > > > + if (ep_cfg.link_frequencies[0] ==
> > > > + IMX219_DEFAULT_LINK_FREQ_4LANE)
> > > > + link_frequency_valid = true;
> > > > + else if (ep_cfg.link_frequencies[0] ==
> > > > + IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED) {
> > > > + dev_warn(dev, "Link frequency of %d not supported, but has been incorrectly advertised previously\n",
> > > > + IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED);
> > > > + dev_warn(dev, "Using link frequency of %d\n",
> > > > + IMX219_DEFAULT_LINK_FREQ_4LANE);
I grepped the code for 363000000 to see if anyone was using it in
their device tree. Maybe I missed something in linux-next, but I
didn't see it. I know I didn't push my camera device tree entries for
the two devices I had tested, because I was going to use them as
device tree overlays and never got around to pushing them upstream,
but I do occasionally test them. If nobody has a wrong device tree
entry, it seems reasonable to me to simplify this code by just
changing it from 363000000 to 364000000 and eliminate the stuff for
the the unsupported value.
> > >
> > > Would it be helpful to use v4l2_link_freq_to_bitmap() here? The old
> > > frequency requires separate handling but I guess you'll still want to
> > > expose the correct frequency to the user space so it should be just one
> > > condition.
> >
> > I've done a quick prototype using it. I'm not sure it's any cleaner,
> > and possibly ends up being more verbose.
> >
> > As I see it, either you end up with a call to v4l2_link_freq_to_bitmap
> > for each of the 2 and 4 lane cases (6 lines each due to the number of
> > parameters), or you combine both 2 & 4 lane frequency options into one
> > array and then handle that certain bit options are only valid for one
> > or other option, and pass the right value in when calling
> > v4l2_ctrl_new_int_menu(..., V4L2_CID_LINK_FREQ...).
> > Handling the unsupported link frequency requires knowledge of the
> > positions in the array, so either hard coded indices or needing an
> > enum for each index.
> >
> > I've pushed my quick patch to
> > https://github.com/6by9/linux/tree/media_imx219_4lane.
> > Personally I think it detracts from readability in this case, but I'm
> > happy to switch to a cleaned up version of it if you view it as
> > better.
>
Thanks for looking at this.
> I was thinking of separate frequency lists for all three cases. That way
> you'd avoid almost all manual checks of the frequencies.
>
> The question I'd also have is whether we should try to continue to indicate
> the incorrect frequency or not. The values are not an integral part of the
> ABI in my view as new ones can be added, even for the same board. And in
> this case there's just a single one in any given case.
>
> This information is also mainly used to configure the receiver timing and
> wrong values here could lead to errors in reception.
>
> IOW, I'd just show the correct value, independently of what's in firmware.
>
> --
> Kind regards,
>
> Sakari Ailus
