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;
Subject: Re: [PATCH] media: i2c: imx219: Implement V4L2_CID_LINK_FREQ control
To:     Dave Stevenson <dave.stevenson@raspberrypi.com>
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>
References: <20210126074934.26980-1-andrey.konovalov@linaro.org>
 <CAPY8ntBmVoe_dMKhd7imcQYRcdJLn4PG3rXzZvGBRVbeCjiL0A@mail.gmail.com>
From:   Andrey Konovalov <andrey.konovalov@linaro.org>
Message-ID: <4c72313b-1b4a-0c7a-a553-144b17763aaa@linaro.org>
Date:   Tue, 26 Jan 2021 18:55:42 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAPY8ntBmVoe_dMKhd7imcQYRcdJLn4PG3rXzZvGBRVbeCjiL0A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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

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