Re: [PATCH] media: i2c: imx219: Implement V4L2_CID_LINK_FREQ control
From: Andrey Konovalov
Date: Wed Jan 27 2021 - 11:05:05 EST
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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx>
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