Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210126074934.26980-1-andrey.konovalov@linaro.org>
 <CAPY8ntBmVoe_dMKhd7imcQYRcdJLn4PG3rXzZvGBRVbeCjiL0A@mail.gmail.com> <4c72313b-1b4a-0c7a-a553-144b17763aaa@linaro.org>
In-Reply-To: <4c72313b-1b4a-0c7a-a553-144b17763aaa@linaro.org>
From:   Dave Stevenson <dave.stevenson@raspberrypi.com>
Date:   Wed, 27 Jan 2021 15:25:54 +0000
Message-ID: <CAPY8ntBsbaxaLHrHnoJNUgUBZ7Lg-HnNtdyaMB0qLeRZW=x2Pw@mail.gmail.com>
Subject: Re: [PATCH] media: i2c: imx219: Implement V4L2_CID_LINK_FREQ control
To:     Andrey Konovalov <andrey.konovalov@linaro.org>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Linux Media Mailing List <linux-media@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Peter Griffin <peter.griffin@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

Based on that I'm happy.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> > 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
> >>