Hi Adam,
thanks for testing
On Mon, Dec 03, 2018 at 10:28:04AM -0600, Adam Ford wrote:
> On Thu, Nov 29, 2018 at 8:49 AM Jacopo Mondi <jacopo+renesas@xxxxxxxxxx> wrote:
> >
> > The MIPI clock generation tree uses the MIPI_DIV divider to generate both the
> > MIPI SCALER CLOCK and the PIXEL_CLOCK. It seems the MIPI BIT CLK frequency is
> > generated by either the MIPI SCALER or the PIXEL CLOCK, depending if the
> > currently applied image mode uses the scaler or not.
> >
> > Make the MIPI_DIV selection value depend on the subsampling mode used by the
> > currently applied image mode.
> >
> > Tested with:
> > 172x144 320x240, 640x480, 1024x756, 1024x768, 1280x720, 1920x1080 in YUYV mode
> > at 10, 15, 20, and 30 FPS with MIPI CSI-2 2 lanes mode.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>
> I am not able to apply this patch to 4.19.6, 4.19-RC5, or Linux-media master [1]
>
> Is there a specific branch/repo somewhere I can pull? I was able to
> apply patch 1/2 just fine, but 2/2 wouldn't apply
Reading the cover letter:
"these two patches should be applied on top of Maxime's clock tree rework v5"
Maxime has included those 2 in his v6. You may want to test that one
:)
Thanks
j
>
> [1] - git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git
>
> adam
> > ---
> > Maxime,
> > this patch is not just a cosmetic updates to the 'set_mipi_pclk()'
> > comment block as I anticipated, but it actually fix the framerate handling
> > compared for CSI-2, which in you v5 results halved for most modes. I have
> > not included any "Fixes:" tag, as I hope this patch will get in with your v5.
> >
> > That's my bad, as in the patches I sent to be applied on top of your v4 I
> > forgot to add a change, and the requested SCLK rate was always half of what
> > it was actually required.
> >
> > Also, I have left out from this patches most of Sam's improvements (better SCLK
> > selection policies, and programming of register OV5640_REG_PCLK_PERIOD as for
> > from testing they're not required, and I don't want to pile more patches than
> > necessary for the next merge window, not to slow down the clock tree rework
> > inclusion. We can get back to it after it got merged.
> >
> > This are the test result obtained by capturing 100 frames with yavta and
> > inspecting the reported fps results.
> >
> > Results are pretty neat, 2592x1944 mode apart, which runs a bit slow.
> >
> > capturing 176x144 @ 10FPS
> > Captured 100 frames in 10.019398 seconds (9.980640 fps, 505898.657683 B/s).
> > capturing 176x144 @ 15FPS
> > Captured 100 frames in 6.681860 seconds (14.965892 fps, 758591.132803 B/s).
> > capturing 176x144 @ 20FPS
> > Captured 100 frames in 5.056204 seconds (19.777683 fps, 1002491.196755 B/s).
> > capturing 176x144 @ 30FPS
> > Captured 100 frames in 3.357204 seconds (29.786686 fps, 1509827.521040 B/s).
> >
> > capturing 320x240 @ 10FPS
> > Captured 100 frames in 10.015432 seconds (9.984591 fps, 1533633.245951 B/s).
> > capturing 320x240 @ 15FPS
> > Captured 100 frames in 6.684035 seconds (14.961021 fps, 2298012.872049 B/s).
> > capturing 320x240 @ 20FPS
> > Captured 100 frames in 5.019164 seconds (19.923635 fps, 3060270.391218 B/s).
> > capturing 320x240 @ 30FPS
> > Captured 100 frames in 3.352991 seconds (29.824115 fps, 4580984.103432 B/s).
> >
> > capturing 640x480 @ 10FPS
> > Captured 100 frames in 9.990389 seconds (10.009620 fps, 6149910.678538 B/s).
> > capturing 640x480 @ 15FPS
> > Captured 100 frames in 6.856242 seconds (14.585249 fps, 8961176.838123 B/s).
> > capturing 640x480 @ 20FPS
> > Captured 100 frames in 5.030264 seconds (19.879670 fps, 12214069.053476 B/s).
> > capturing 640x480 @ 30FPS
> > Captured 100 frames in 3.364612 seconds (29.721103 fps, 18260645.750580 B/s).
> >
> > capturing 720x480 @ 10FPS
> > Captured 100 frames in 10.022488 seconds (9.977562 fps, 6896491.169279 B/s).
> > capturing 720x480 @ 15FPS
> > Captured 100 frames in 6.891968 seconds (14.509643 fps, 10029065.232208 B/s).
> > capturing 720x480 @ 20FPS
> > Captured 100 frames in 5.234133 seconds (19.105360 fps, 13205624.616211 B/s).
> > capturing 720x480 @ 30FPS
> > Captured 100 frames in 3.602298 seconds (27.760055 fps, 19187750.044688 B/s).
> >
> > capturing 720x576 @ 10FPS
> > Captured 100 frames in 10.197244 seconds (9.806571 fps, 8133961.937805 B/s).
> > capturing 720x576 @ 15FPS
> > Captured 100 frames in 6.925244 seconds (14.439924 fps, 11977050.339261 B/s).
> > capturing 720x576 @ 20FPS
> > Captured 100 frames in 4.999968 seconds (20.000127 fps, 16588905.060854 B/s).
> > capturing 720x576 @ 30FPS
> > Captured 100 frames in 3.487568 seconds (28.673276 fps, 23782762.085212 B/s).
> >
> > capturing 1024x768 @ 10FPS
> > Captured 100 frames in 10.107128 seconds (9.894007 fps, 15561928.174298 B/s).
> > capturing 1024x768 @ 15FPS
> > Captured 100 frames in 6.810568 seconds (14.683062 fps, 23094459.169337 B/s).
> > capturing 1024x768 @ 20FPS
> > Captured 100 frames in 5.012039 seconds (19.951960 fps, 31381719.096759 B/s).
> > capturing 1024x768 @ 30FPS
> > Captured 100 frames in 3.346338 seconds (29.883407 fps, 47002534.905114 B/s).
> >
> > capturing 1280x720 @ 10FPS
> > Captured 100 frames in 9.957613 seconds (10.042567 fps, 18510459.665283 B/s).
> > capturing 1280x720 @ 15FPS
> > Captured 100 frames in 6.597751 seconds (15.156679 fps, 27936790.986673 B/s).
> > capturing 1280x720 @ 20FPS
> > Captured 100 frames in 4.946115 seconds (20.217888 fps, 37265611.495083 B/s).
> > capturing 1280x720 @ 30FPS
> > Captured 100 frames in 3.301329 seconds (30.290825 fps, 55832049.080847 B/s).
> >
> > capturing 1920x1080 @ 10FPS
> > Captured 100 frames in 10.024745 seconds (9.975316 fps, 41369629.470131 B/s).
> > capturing 1920x1080 @ 15FPS
> > Captured 100 frames in 6.674363 seconds (14.982702 fps, 62136260.577244 B/s).
> > capturing 1920x1080 @ 20FPS
> > Captured 100 frames in 5.102174 seconds (19.599488 fps, 81282998.172684 B/s).
> > capturing 1920x1080 @ 30FPS
> > Captured 100 frames in 3.341157 seconds (29.929746 fps, 124124642.214916 B/s).
> >
> > capturing 2592x1944 @ 10FPS
> > Captured 100 frames in 13.019132 seconds (7.681004 fps, 77406819.428913 B/s).
> > capturing 2592x1944 @ 15FPS
> > Captured 100 frames in 8.819705 seconds (11.338248 fps, 114263413.559267 B/s).
> > capturing 2592x1944 @ 20FPS
> > Captured 100 frames in 6.876134 seconds (14.543055 fps, 146560487.484508 B/s).
> > capturing 2592x1944 @ 30FPS
> > Captured 100 frames in 4.359511 seconds (22.938352 fps, 231165743.130365 B/s).
> > ---
> > drivers/media/i2c/ov5640.c | 93 +++++++++++++++++++++++-----------------------
> > 1 file changed, 46 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index c659efe918a4..13b7a0d04840 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -740,8 +740,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> > * | +---------------> SCLK 2X
> > * | +-------------+
> > * +->| PCLK Div | - reg 0x3108, bits 4-5
> > - * +-+-----------+
> > - * +---------------> PCLK
> > + * ++------------+
> > + * + +-----------+
> > + * +->| P_DIV | - reg 0x3035, bits 0-3
> > + * +-----+-----+
> > + * +------------> PCLK
> > *
> > * This is deviating from the datasheet at least for the register
> > * 0x3108, since it's said here that the PCLK would be clocked from
> > @@ -751,7 +754,6 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> > * - the PLL pre-divider output rate should be in the 4-27MHz range
> > * - the PLL multiplier output rate should be in the 500-1000MHz range
> > * - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> > - * - MIPI SCLK = (bpp / lanes) / PCLK
> > *
> > * In the two latter cases, these constraints are met since our
> > * factors are hardcoded. If we were to change that, we would need to
> > @@ -777,10 +779,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> > #define OV5640_SYSDIV_MAX 16
> >
> > /*
> > - * This is supposed to be ranging from 1 to 16, but the value is always
> > - * set to 2 in the vendor kernels.
> > + * Hardcode these values for scaler and non-scaler modes.
> > + * FIXME: to be re-calcualted for 1 data lanes setups
> > */
> > -#define OV5640_MIPI_DIV 2
> > +#define OV5640_MIPI_DIV_PCLK 2
> > +#define OV5640_MIPI_DIV_SCLK 1
> >
> > /*
> > * This is supposed to be ranging from 1 to 2, but the value is always
> > @@ -892,65 +895,61 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
> > * for the MIPI CSI-2 output.
> > *
> > - * @rate: The requested bandwidth in bytes per second.
> > - * It is calculated as: HTOT * VTOT * FPS * bpp
> > + * @rate: The requested bandwidth per lane in bytes per second.
> > + * 'Bandwidth Per Lane' is calculated as:
> > + * bpl = HTOT * VTOT * FPS * bpp / num_lanes;
> > *
> > * This function use the requested bandwidth to calculate:
> > - * - sample_rate = bandwidth / bpp;
> > - * - mipi_clk = bandwidth / num_lanes / 2; ( / 2 for CSI-2 DDR)
> > + * - sample_rate = bpl / (bpp / num_lanes);
> > + * = bpl / (PLL_RDIV * BIT_DIV * PCLK_DIV * MIPI_DIV / num_lanes);
> > + *
> > + * - mipi_sclk = bpl / MIPI_DIV / 2; ( / 2 is for CSI-2 DDR)
> > *
> > - * The bandwidth corresponds to the SYSCLK frequency generated by the
> > - * PLL pre-divider, the PLL multiplier and the SYS divider (see the clock
> > - * tree documented here above).
> > + * with these fixed parameters:
> > + * PLL_RDIV = 2;
> > + * BIT_DIVIDER = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> > + * PCLK_DIV = 1;
> > *
> > - * From the SYSCLK frequency, the MIPI CSI-2 clock tree generates the
> > - * pixel clock and the MIPI BIT clock as follows:
> > + * The MIPI clock generation differs for modes that use the scaler and modes
> > + * that do not. In case the scaler is in use, the MIPI_SCLK generates the MIPI
> > + * BIT CLk, and thus:
> > *
> > - * MIPI_BIT_CLK = SYSCLK / MIPI_DIV / 2;
> > - * PIXEL_CLK = SYSCLK / PLL_RDVI / BIT_DIVIDER / PCLK_DIV / MIPI_DIV
> > + * - mipi_sclk = bpl / MIPI_DIV / 2;
> > + * MIPI_DIV = 1;
> > *
> > - * with this fixed parameters:
> > - * PLL_RDIV = 2;
> > - * BIT_DIVIDER = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> > - * PCLK_DIV = 1;
> > + * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
> > + * from the pixel clock, and thus:
> > *
> > - * With these values we have:
> > + * - sample_rate = bpl / (bpp / num_lanes);
> > + * = bpl / (2 * 2 * 1 * MIPI_DIV / num_lanes);
> > + * = bpl / (4 * MIPI_DIV / num_lanes);
> > + * - MIPI_DIV = bpp / (4 * num_lanes);
> > *
> > - * pixel_clock = bandwidth / bpp
> > - * = bandwidth / 4 / MIPI_DIV;
> > + * FIXME: this have been tested with 16bpp and 2 lanes setup only.
> > + * MIPI_DIV is fixed to value 2, but it -might- be changed according to the
> > + * above formula for setups with 1 lane or image formats with different bpp.
> > *
> > - * And so we can calculate MIPI_DIV as:
> > - * MIPI_DIV = bpp / 4;
> > + * FIXME: this deviates from the sensor manual documentation which is quite
> > + * thin on the MIPI clock tree generation part.
> > */
> > static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
> > unsigned long rate)
> > {
> > const struct ov5640_mode_info *mode = sensor->current_mode;
> > - u8 mipi_div = OV5640_MIPI_DIV;
> > u8 prediv, mult, sysdiv;
> > + u8 mipi_div;
> > int ret;
> >
> > - /* FIXME:
> > - * Practical experience shows we get a correct frame rate by
> > - * halving the bandwidth rate by 2, to slow down SYSCLK frequency.
> > - * Divide both SYSCLK and MIPI_DIV by two (with OV5640_MIPI_DIV
> > - * currently fixed at value '2', while according to the above
> > - * formula it should have been = bpp / 4 = 4).
> > - *
> > - * So that:
> > - * pixel_clock = bandwidth / 2 / bpp
> > - * = bandwidth / 2 / 4 / MIPI_DIV;
> > - * MIPI_DIV = bpp / 4 / 2;
> > - */
> > - rate /= 2;
> > -
> > - /* FIXME:
> > - * High resolution modes (1280x720, 1920x1080) requires an higher
> > - * clock speed. Half the MIPI_DIVIDER value to double the output
> > - * pixel clock and MIPI_CLK speeds.
> > + /*
> > + * 1280x720 is reported to use 'SUBSAMPLING' only,
> > + * but according to the sensor manual it goes through the
> > + * scaler before subsampling.
> > */
> > - if (mode->hact > 1024)
> > - mipi_div /= 2;
> > + if (mode->dn_mode == SCALING ||
> > + (mode->id == OV5640_MODE_720P_1280_720))
> > + mipi_div = OV5640_MIPI_DIV_SCLK;
> > + else
> > + mipi_div = OV5640_MIPI_DIV_PCLK;
> >
> > ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> >
> > --
> > 2.7.4
> >
Attachment:
signature.asc
Description: PGP signature
