Messages in this thread | | | Subject | Re: [PATCH] media: i2c: imx219: Implement V4L2_CID_LINK_FREQ control | From | Andrey Konovalov <> | Date | Wed, 27 Jan 2021 19:00:13 +0300 |
| |
Hi David,
On 27.01.2021 18:25, Dave Stevenson wrote: > Hi Andrey > > On Tue, 26 Jan 2021 at 15:55, Andrey Konovalov > <andrey.konovalov@linaro.org> wrote: >> >> Hi Dave, >> >> On 26.01.2021 16:01, Dave Stevenson wrote: >>> Hi Andrey >>> >>> On Tue, 26 Jan 2021 at 07:50, Andrey Konovalov >>> <andrey.konovalov@linaro.org> wrote: >>>> >>>> This control is needed for imx219 driver, as the link frequency >>>> is independent from the pixel rate in this case, and can't be >>>> calculated from the pixel rate. >>>> >>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >>>> --- >>>> drivers/media/i2c/imx219.c | 15 ++++++++++++++- >>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c >>>> index 92a8d52776b8..6e3382b85a90 100644 >>>> --- a/drivers/media/i2c/imx219.c >>>> +++ b/drivers/media/i2c/imx219.c >>>> @@ -390,6 +390,10 @@ static const struct imx219_reg raw10_framefmt_regs[] = { >>>> {0x0309, 0x0a}, >>>> }; >>>> >>>> +static const s64 imx219_link_freq_menu[] = { >>>> + IMX219_DEFAULT_LINK_FREQ, >>> >>> Link frequency is one of the parameters that is largely irrelevant on >>> the Pi, so I've partially ignored it. >> >> I faced a problem with the imx219 8-bit modes not working with the camss driver >> (drivers/media/platform/qcom/camss), as based on the link frequency calculated >> from the pixel rate the driver sets the csiphy clock to 100MHz which is too low >> for the actual link frequency (4 * 100MHz < 456MHz), and the captured image >> becomes garbage. >> >>> Is the link frequency really the same for all modes? Even 8 bit vs 10 >>> bit readout? >> >> Yes, this is exactly the case. >> >>> The pixel rate is constant at 182.4Mpix/s for all modes. >> >> Right. >> >>> Switching to 8 bit changes register 0x0309 (op_pix_clk_div) from 10 to 8. >>> Figure 43 "Clock System Block Diagram" in the datasheet I have says >>> this reduces the divider to the FIFO between the pipeline and MIPI. As >>> we haven't changed PLL2 or Pre-div2 I'd expect the link frequency to >>> stay the same, >> >> That's true. >> >>> but that leaves me confused over that FIFO clock as >>> it'll go UP in frequency. I can't quite see how that works, but it >>> clearly does. >> >> Yes, the FIFO makes it possible for the different write and read rates to work. >> There are few words regarding this in the datasheet, but this isn't enough >> to fully understand how it works: >> "If, Pix Rate of PLL1 domain < Data Rate of PLL2 domain, data is always >> correctly output from the sensor" (page 81) >> >> If I read the datasheet right, for 10-bit modes the both rates are the same >> (91.2 MHz). In the 8-bit modes the "Data Rate" increases to 114 MHz while >> the "Pix Rate" remains at 91.2 MHz. > > There looks to be some magic in there. Without knowing the details of > the size of the FIFO and when it triggers the output stage to start > sending data, it's hard to say exactly what is going on. > I'm guessing that it has to be large enough to take a whole line, and > triggers when the line is complete. The CSI2 bus can then run at the > frequency defined, independent of pixel rate or bit depth. > >>> Both 8 and 10 bit modes do read out at the same frame / pixel rate, >>> therefore that bit is correct, but that leaves me puzzling over link >>> frequency. I have no information on how big that FIFO is, or how it's >>> clocked on input and output. >>> >>> Simplest option is that as I need to go into the office in the next >>> day or so I'll pop into the lab and measure it in each mode. >> >> That would be nice! >> In my home "office" I only have a small piece of hardware which claims >> to be able to deal with 2 signals up to 72MHz each, which is not enough >> for such kind of measurements. > > My home "office" is likewise compromised, but the work office is still > available and has suitable equipment :-) > > I can't be that precise as I'm not bothering to get differential > probes out and the like, but it does appear that the clock lane is > running at the same speed for both 8 and 10 bit. > I measured (cursors off the scope, measured over 4 cycles) 2.21ns for > 8 bit, and 2.18ns for 10 bit, corresponding to 452 and 458MHz > respectively. Those are well within the experimental error of my setup > to be the 456MHz defined in the driver.
I expected that, but having this confirmed experimentally is so great!
> Based on that I'm happy. > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Thanks, Andrey
>>> Otherwise I have no issues with the implementation of the patch. >>> >>> Dave >> >> Thanks, >> Andrey >> >>>> +}; >>>> + >>>> static const char * const imx219_test_pattern_menu[] = { >>>> "Disabled", >>>> "Color Bars", >>>> @@ -547,6 +551,7 @@ struct imx219 { >>>> struct v4l2_ctrl_handler ctrl_handler; >>>> /* V4L2 Controls */ >>>> struct v4l2_ctrl *pixel_rate; >>>> + struct v4l2_ctrl *link_freq; >>>> struct v4l2_ctrl *exposure; >>>> struct v4l2_ctrl *vflip; >>>> struct v4l2_ctrl *hflip; >>>> @@ -1269,7 +1274,7 @@ static int imx219_init_controls(struct imx219 *imx219) >>>> int i, ret; >>>> >>>> ctrl_hdlr = &imx219->ctrl_handler; >>>> - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11); >>>> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -1283,6 +1288,14 @@ static int imx219_init_controls(struct imx219 *imx219) >>>> IMX219_PIXEL_RATE, 1, >>>> IMX219_PIXEL_RATE); >>>> >>>> + imx219->link_freq = >>>> + v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops, >>>> + V4L2_CID_LINK_FREQ, >>>> + ARRAY_SIZE(imx219_link_freq_menu) - 1, 0, >>>> + imx219_link_freq_menu); >>>> + if (imx219->link_freq) >>>> + imx219->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + >>>> /* Initial vblank/hblank/exposure parameters based on current mode */ >>>> imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, >>>> V4L2_CID_VBLANK, IMX219_VBLANK_MIN, >>>> -- >>>> 2.17.1 >>>>
| |