| Message ID | 20220224094313.233347-7-jacopo@jmondi.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
Hi Jacopo, Issue with link frequency value reported, see below. On 2/24/22 10:42 AM, Jacopo Mondi wrote: > After having set a new format re-calculate the pixel_rate and link_freq > control values and update them when in MIPI mode. > > Take into account the limitation of the link frequency having to be > strictly smaller than 1GHz when computing the desired link_freq, and > adjust the resulting pixel_rate acounting for the clock tree > configuration. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/ov5640.c | 66 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 64 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index f9763edf2422..791694bfed41 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -31,6 +31,8 @@ > > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > +#define OV5640_LINK_RATE_MAX 490000000U > + > #define OV5640_REG_SYS_RESET02 0x3002 > #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > #define OV5640_REG_SYS_CTRL0 0x3008 > @@ -2412,6 +2414,66 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, > return 0; > } > > +static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > +{ > + const struct ov5640_mode_info *mode = sensor->current_mode; > + struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > + enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > + unsigned int i = 0; > + u32 pixel_rate; > + s64 link_freq; > + u32 num_lanes; > + u32 bpp; > + > + /* > + * Update the pixel rate control value. > + * > + * For DVP mode, maintain the pixel rate calculation using fixed FPS. > + */ > + if (!ov5640_is_csi2(sensor)) { > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > + ov5640_calc_pixel_rate(sensor)); > + > + return 0; > + } > + > + /* > + * The MIPI CSI-2 link frequency should comply with the CSI-2 > + * specification and be lower than 1GHz. > + * > + * Start from the suggested pixel_rate for the current mode and > + * progressively slow it down if it exceeds 1GHz. > + */ > + num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > + bpp = ov5640_code_to_bpp(fmt->code); > + do { > + pixel_rate = ov5640_pixel_rates[pixel_rate_id]; > + link_freq = pixel_rate * bpp / (2 * num_lanes); > + } while (link_freq >= 1000000000U && > + ++pixel_rate_id < OV5640_NUM_PIXEL_RATES); > + > + /* > + * Higher link rates require the clock tree to be programmed with > + * 'mipi_div' = 1; this has the effect of halving the actual output > + * pixel rate in the MIPI domain. > + * > + * Adjust the pixel rate control value to report it correctly to > + * userspace. > + */ > + if (link_freq > OV5640_LINK_RATE_MAX) > + pixel_rate /= 2; Need to divide also the link_frequency (st-mipid02 bridge interfacing is broken here). Patch proposal below: + sensor->current_link_freq = link_freq; + + * Adjust the pixel rate & link frequency control value to report it + * correctly to userspace. */ - if (link_freq > OV5640_LINK_RATE_MAX) + if (link_freq > OV5640_LINK_RATE_MAX) { pixel_rate /= 2; + link_freq /= 2; + } Doing so we cannot relay anymore on link_frequency control reading in ov5640_set_mipi_pclk(), use current_link_freq variable instead @@ -1440,7 +1453,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor) int ret; /* Use the link freq computed at ov5640_update_pixel_rate() time. */ - link_freq = ov5640_csi2_link_freqs[sensor->ctrls.link_freq->cur.val]; + link_freq = sensor->current_link_freq; @@ -3782,6 +3811,7 @@ static int ov5640_probe(struct i2c_client *client) sensor->current_mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480]; sensor->last_mode = sensor->current_mode; + sensor->current_link_freq = OV5640_DEFAULT_LINK_FREQ; > + > + for (i = 0; i < ARRAY_SIZE(ov5640_csi2_link_freqs); ++i) { > + if (ov5640_csi2_link_freqs[i] == link_freq) > + break; > + } > + > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, pixel_rate); > + __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, i); > + > + return 0; > +} > + > static int ov5640_set_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_format *format) > @@ -2451,8 +2513,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > /* update format even if code is unchanged, resolution might change */ > sensor->fmt = *mbus_fmt; > > - __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > - ov5640_calc_pixel_rate(sensor)); > + ov5640_update_pixel_rate(sensor); > + > out: > mutex_unlock(&sensor->lock); > return ret; > Hugues.
Hi Hugues, On Thu, Apr 07, 2022 at 06:25:57PM +0200, Hugues FRUCHET - FOSS wrote: > Hi Jacopo, > > Issue with link frequency value reported, see below. > > On 2/24/22 10:42 AM, Jacopo Mondi wrote: > > After having set a new format re-calculate the pixel_rate and link_freq > > control values and update them when in MIPI mode. > > > > Take into account the limitation of the link frequency having to be > > strictly smaller than 1GHz when computing the desired link_freq, and > > adjust the resulting pixel_rate acounting for the clock tree > > configuration. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/ov5640.c | 66 ++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 64 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index f9763edf2422..791694bfed41 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -31,6 +31,8 @@ > > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > +#define OV5640_LINK_RATE_MAX 490000000U > > + > > #define OV5640_REG_SYS_RESET02 0x3002 > > #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > > #define OV5640_REG_SYS_CTRL0 0x3008 > > @@ -2412,6 +2414,66 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, > > return 0; > > } > > +static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > +{ > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > + struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > > + enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > > + unsigned int i = 0; > > + u32 pixel_rate; > > + s64 link_freq; > > + u32 num_lanes; > > + u32 bpp; > > + > > + /* > > + * Update the pixel rate control value. > > + * > > + * For DVP mode, maintain the pixel rate calculation using fixed FPS. > > + */ > > + if (!ov5640_is_csi2(sensor)) { > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > > + ov5640_calc_pixel_rate(sensor)); > > + > > + return 0; > > + } > > + > > + /* > > + * The MIPI CSI-2 link frequency should comply with the CSI-2 > > + * specification and be lower than 1GHz. > > + * > > + * Start from the suggested pixel_rate for the current mode and > > + * progressively slow it down if it exceeds 1GHz. > > + */ > > + num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > > + bpp = ov5640_code_to_bpp(fmt->code); > > + do { > > + pixel_rate = ov5640_pixel_rates[pixel_rate_id]; > > + link_freq = pixel_rate * bpp / (2 * num_lanes); > > + } while (link_freq >= 1000000000U && > > + ++pixel_rate_id < OV5640_NUM_PIXEL_RATES); > > + > > + /* > > + * Higher link rates require the clock tree to be programmed with > > + * 'mipi_div' = 1; this has the effect of halving the actual output > > + * pixel rate in the MIPI domain. > > + * > > + * Adjust the pixel rate control value to report it correctly to > > + * userspace. > > + */ > > + if (link_freq > OV5640_LINK_RATE_MAX) > > + pixel_rate /= 2; > > Need to divide also the link_frequency (st-mipid02 bridge interfacing is > broken here). Patch proposal below: > > + sensor->current_link_freq = link_freq; > + > + * Adjust the pixel rate & link frequency control value to report it > + * correctly to userspace. > */ > - if (link_freq > OV5640_LINK_RATE_MAX) > + if (link_freq > OV5640_LINK_RATE_MAX) { > pixel_rate /= 2; > + link_freq /= 2; > + } This has been my headache for a long time and I'm still not 100% convinced what I have here is the best solution, but at least works for more much modes than what it used to. Can I ask how did you get to the conclusion that link_rate should be halved ? Is your receiver driver complaining ? Have you sampled the actual link frequency ? I tried applying your patch and 1) on imx8mp with mipi-csis receiver your patch breaks a few modes but still works for most of them. The system seems unstable compared to the original version, and sometimes I get hangs/segfauls for high resolution modes. The receiver driver is the mipi-csis one [1] [1] drivers/media/platform/imx/imx-mipi-csis.c 2) on i.MX6 I spent quite some time debugging why high-res modes do not work there with my series, and my understanding is that the i.MX6 CSI-2 receiver only supports a total bandwidth of 1Gbps/lane, which the high res modes of the ov5640 sensor exceeds, having a clock rate frequency of 672 MHz. (Unrelated: the limitation of 1Gbps might be due to the fact the i.MX6 receiver implements the v1.0 version of the CSI-2 specs, but I found nowhere a confirmation that v1.0 is limited to 1Gbps compared to the 1.5Gbps limit of v1.1). If I apply your patch I can capture 1080p and full res, but the images are crippled. The pixels are repeated multiple times in the final image, I cannot tell if that's an issue on the receiver or due to the link rate being actually faster than what reported with your change. Could you on how you got to halve the reported pixel rate ? Others have tested with other csi-2 receivers which sample link freq using v4l2_get_link_freq() as well, and they have not reported issues afaict. (Please note that using a link_freq that doesn't come from the control in ov5640_set_mipi_pclk() makes it harder to tune the pixel rate from userspace to accommodate more configurations, as I suggested we should do in reply to "[PATCH v5 16/27] media: ov5640: Add VBLANK control" but it might still be doable). Thanks a lot for testing! j [2] That's what I have applied diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index faca36dc4187..910b58fb1e08 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -461,6 +461,7 @@ struct ov5640_dev { bool pending_mode_change; bool streaming; + s64 current_link_freq; }; static inline struct ov5640_dev *to_ov5640_dev(struct v4l2_subdev *sd) @@ -1439,7 +1440,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor) int ret; /* Use the link frequency computed at ov5640_update_pixel_rate() time. */ - link_freq = ov5640_csi2_link_freqs[sensor->ctrls.link_freq->cur.val]; + link_freq = sensor->current_link_freq; /* * - mipi_div - Additional divider for the MIPI lane clock. @@ -2836,6 +2837,8 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) } while (link_freq >= 1000000000U && ++pixel_rate_id < OV5640_NUM_PIXEL_RATES); + sensor->current_link_freq = link_freq; + /* * Higher link rates require the clock tree to be programmed with * 'mipi_div' = 1; this has the effect of halving the actual output @@ -2844,8 +2847,10 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) * Adjust the pixel rate control value to report it correctly to * userspace. */ - if (link_freq > OV5640_LINK_RATE_MAX) + if (link_freq > OV5640_LINK_RATE_MAX) { pixel_rate /= 2; + link_freq /= 2; + } for (i = 0; i < ARRAY_SIZE(ov5640_csi2_link_freqs); ++i) { if (ov5640_csi2_link_freqs[i] == link_freq) @@ -3777,6 +3782,7 @@ static int ov5640_probe(struct i2c_client *client) sensor->current_mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480]; sensor->last_mode = sensor->current_mode; + sensor->current_link_freq = OV5640_DEFAULT_LINK_FREQ; sensor->ae_target = 52; > > > Doing so we cannot relay anymore on link_frequency control reading in > ov5640_set_mipi_pclk(), use current_link_freq variable instead > > @@ -1440,7 +1453,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev > *sensor) > int ret; > > /* Use the link freq computed at ov5640_update_pixel_rate() time. */ > - link_freq = ov5640_csi2_link_freqs[sensor->ctrls.link_freq->cur.val]; > + link_freq = sensor->current_link_freq; > > @@ -3782,6 +3811,7 @@ static int ov5640_probe(struct i2c_client *client) > sensor->current_mode = > &ov5640_mode_data[OV5640_MODE_VGA_640_480]; > sensor->last_mode = sensor->current_mode; > + sensor->current_link_freq = OV5640_DEFAULT_LINK_FREQ; > > > > > > + > > + for (i = 0; i < ARRAY_SIZE(ov5640_csi2_link_freqs); ++i) { > > + if (ov5640_csi2_link_freqs[i] == link_freq) > > + break; > > + } > > + > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, pixel_rate); > > + __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, i); > > + > > + return 0; > > +} > > + > > static int ov5640_set_fmt(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_format *format) > > @@ -2451,8 +2513,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > /* update format even if code is unchanged, resolution might change */ > > sensor->fmt = *mbus_fmt; > > - __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > > - ov5640_calc_pixel_rate(sensor)); > > + ov5640_update_pixel_rate(sensor); > > + > > out: > > mutex_unlock(&sensor->lock); > > return ret; > > > > Hugues.
Hi Jacopo, On 4/11/22 6:46 PM, Jacopo Mondi wrote: > Hi Hugues, > > On Thu, Apr 07, 2022 at 06:25:57PM +0200, Hugues FRUCHET - FOSS wrote: >> Hi Jacopo, >> >> Issue with link frequency value reported, see below. >> >> On 2/24/22 10:42 AM, Jacopo Mondi wrote: >>> After having set a new format re-calculate the pixel_rate and link_freq >>> control values and update them when in MIPI mode. >>> >>> Take into account the limitation of the link frequency having to be >>> strictly smaller than 1GHz when computing the desired link_freq, and >>> adjust the resulting pixel_rate acounting for the clock tree >>> configuration. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> drivers/media/i2c/ov5640.c | 66 ++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 64 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>> index f9763edf2422..791694bfed41 100644 >>> --- a/drivers/media/i2c/ov5640.c >>> +++ b/drivers/media/i2c/ov5640.c >>> @@ -31,6 +31,8 @@ >>> #define OV5640_DEFAULT_SLAVE_ID 0x3c >>> +#define OV5640_LINK_RATE_MAX 490000000U >>> + >>> #define OV5640_REG_SYS_RESET02 0x3002 >>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 >>> #define OV5640_REG_SYS_CTRL0 0x3008 >>> @@ -2412,6 +2414,66 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, >>> return 0; >>> } >>> +static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) >>> +{ >>> + const struct ov5640_mode_info *mode = sensor->current_mode; >>> + struct v4l2_mbus_framefmt *fmt = &sensor->fmt; >>> + enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; >>> + unsigned int i = 0; >>> + u32 pixel_rate; >>> + s64 link_freq; >>> + u32 num_lanes; >>> + u32 bpp; >>> + >>> + /* >>> + * Update the pixel rate control value. >>> + * >>> + * For DVP mode, maintain the pixel rate calculation using fixed FPS. >>> + */ >>> + if (!ov5640_is_csi2(sensor)) { >>> + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, >>> + ov5640_calc_pixel_rate(sensor)); >>> + >>> + return 0; >>> + } >>> + >>> + /* >>> + * The MIPI CSI-2 link frequency should comply with the CSI-2 >>> + * specification and be lower than 1GHz. >>> + * >>> + * Start from the suggested pixel_rate for the current mode and >>> + * progressively slow it down if it exceeds 1GHz. >>> + */ >>> + num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; >>> + bpp = ov5640_code_to_bpp(fmt->code); >>> + do { >>> + pixel_rate = ov5640_pixel_rates[pixel_rate_id]; >>> + link_freq = pixel_rate * bpp / (2 * num_lanes); >>> + } while (link_freq >= 1000000000U && >>> + ++pixel_rate_id < OV5640_NUM_PIXEL_RATES); >>> + >>> + /* >>> + * Higher link rates require the clock tree to be programmed with >>> + * 'mipi_div' = 1; this has the effect of halving the actual output >>> + * pixel rate in the MIPI domain. >>> + * >>> + * Adjust the pixel rate control value to report it correctly to >>> + * userspace. >>> + */ >>> + if (link_freq > OV5640_LINK_RATE_MAX) >>> + pixel_rate /= 2; >> >> Need to divide also the link_frequency (st-mipid02 bridge interfacing is >> broken here). Patch proposal below: >> >> + sensor->current_link_freq = link_freq; >> + >> + * Adjust the pixel rate & link frequency control value to report it >> + * correctly to userspace. >> */ >> - if (link_freq > OV5640_LINK_RATE_MAX) >> + if (link_freq > OV5640_LINK_RATE_MAX) { >> pixel_rate /= 2; >> + link_freq /= 2; >> + } > > This has been my headache for a long time and I'm still not 100% > convinced what I have here is the best solution, but at least works > for more much modes than what it used to. > > Can I ask how did you get to the conclusion that link_rate should be > halved ? Is your receiver driver complaining ? Have you sampled the > actual link frequency ? First because nothing was working on my setup when reaching the OV5640_LINK_RATE_MAX limit, this concerns resolutions >= 1280x720, resolutions below are running fine. Reading code and coming to this exact line, I was thinking that it was an obvious point, because pixel_rate and link_freq are correlated: * link_freq = (pixel_rate * bpp) / (2 * data_lanes) and my hypothesis was that the receiver of your test setup was using the pixel_rate CID and not the link_freq, while my mipid02 receiver reads first the link_freq CID, so it was making sense... Here are the pixel_rate/link_freq/pclk_period tested OK on my setup for all resolutions with 15 & 30fps: QCIF 176x144 RGB565 15fps => OK, got 15 QCIF 176x144 YUYV 15fps => OK, got 15 QCIF 176x144 JPEG 15fps => OK, got 15 rate=48000000, freq=192000000, mipi_div=2, pclk_period=20, htot=1600, height=144, vblank=1856 QCIF 176x144 RGB565 30fps => OK, got 30 QCIF 176x144 YUYV 30fps => OK, got 30 QCIF 176x144 JPEG 30fps => OK, got 30 rate=48000000, freq=192000000, mipi_div=2, pclk_period=20, htot=1600, height=144, vblank=854 QVGA 320x240 RGB565 15fps => OK, got 15 QVGA 320x240 YUYV 15fps => OK, got 15 QVGA 320x240 JPEG 15fps => OK, got 15 rate=48000000, freq=192000000, mipi_div=2, pclk_period=20, htot=1600, height=240, vblank=1760 QVGA 320x240 RGB565 30fps => OK, got 30 QVGA 320x240 YUYV 30fps => OK, got 30 QVGA 320x240 JPEG 30fps => OK, got 30 rate=48000000, freq=192000000, mipi_div=2, pclk_period=20, htot=1600, height=240, vblank=760 VGA 640x480 RGB565 15fps => OK, got 15 VGA 640x480 YUYV 15fps => OK, got 15 VGA 640x480 JPEG 15fps => OK, got 15 rate=48000000, freq=192000000, mipi_div=2, pclk_period=20, htot=1600, height=480, vblank=1520 VGA 640x480 RGB565 30fps => OK, got 30 VGA 640x480 YUYV 30fps => OK, got 30 VGA 640x480 JPEG 30fps => OK, got 30 rate=48000000, freq=192000000, mipi_div=2, pclk_period=20, htot=1600, height=480, vblank=520 480p 720x480 RGB565 15fps => OK, got 15 480p 720x480 YUYV 15fps => OK, got 15 480p 720x480 JPEG 15fps => OK, got 15 rate=96000000, freq=384000000, mipi_div=2, pclk_period=10, htot=1896, height=480, vblank=2895 480p 720x480 RGB565 30fps => OK, got 30 480p 720x480 YUYV 30fps => OK, got 30 480p 720x480 JPEG 30fps => OK, got 30 rate=96000000, freq=384000000, mipi_div=2, pclk_period=10, htot=1896, height=480, vblank=1206 XGA 1024x768 RGB565 15fps => OK, got 15 XGA 1024x768 YUYV 15fps => OK, got 15 XGA 1024x768 JPEG 15fps => OK, got 15 rate=96000000, freq=384000000, mipi_div=2, pclk_period=10, htot=1896, height=768, vblank=2607 XGA 1024x768 RGB565 30fps => OK, got 30 XGA 1024x768 YUYV 30fps => OK, got 30 XGA 1024x768 JPEG 30fps => OK, got 30 rate=96000000, freq=384000000, mipi_div=2, pclk_period=10, htot=1896, height=768, vblank=918 720p 1280x720 RGB565 15fps => OK, got 15 720p 1280x720 YUYV 15fps => OK, got 15 720p 1280x720 JPEG 15fps => OK, got 15 rate=62000000, freq=248000000, mipi_div=1, pclk_period=16, htot=1600, height=720, vblank=1863 720p 1280x720 RGB565 30fps => OK, got 30 720p 1280x720 YUYV 30fps => OK, got 30 720p 1280x720 JPEG 30fps => OK, got 30 rate=62000000, freq=248000000, mipi_div=1, pclk_period=16, htot=1600, height=720, vblank=560 HD 1920x1080 RGB565 15fps => OK, got 15 HD 1920x1080 YUYV 15fps => OK, got 15 HD 1920x1080 JPEG 15fps => OK, got 15 rate=74000000, freq=296000000, mipi_div=1, pclk_period=13, htot=2234, height=1080, vblank=1128 HD 1920x1080 RGB565 30fps => OK, got 30 HD 1920x1080 YUYV 30fps => OK, got 30 HD 1920x1080 JPEG 30fps => OK, got 30 rate=74000000, freq=296000000, mipi_div=1, pclk_period=13, htot=2234, height=1080, vblank=24 5Mp 2592x1944 RGB565 15fps => OK, got 15 5Mp 2592x1944 YUYV 15fps => OK, got 14 5Mp 2592x1944 JPEG 15fps => OK, got 15 rate=84000000, freq=336000000, mipi_div=1, pclk_period=11, htot=2844, height=1944, vblank=24 > > I tried applying your patch and > > 1) on imx8mp with mipi-csis receiver your patch breaks a few modes but > still works for most of them. The system seems unstable compared to > the original version, and sometimes I get hangs/segfauls for high > resolution modes. The receiver driver is the mipi-csis one [1] > > [1] drivers/media/platform/imx/imx-mipi-csis.c There was a problem with pclk_period computation, please git it a try with the new version. > > 2) on i.MX6 I spent quite some time debugging why high-res modes do > not work there with my series, and my understanding is that the i.MX6 > CSI-2 receiver only supports a total bandwidth of 1Gbps/lane, which > the high res modes of the ov5640 sensor exceeds, having a clock rate > frequency of 672 MHz. (Unrelated: the limitation of 1Gbps might be due > to the fact the i.MX6 receiver implements the v1.0 version of the > CSI-2 specs, but I found nowhere a confirmation that v1.0 is limited > to 1Gbps compared to the 1.5Gbps limit of v1.1). > > If I apply your patch I can capture 1080p and full res, but the > images are crippled. The pixels are repeated multiple times in the final > image, I cannot tell if that's an issue on the receiver or due to the > link rate being actually faster than what reported with your change. > > Could you on how you got to halve the reported pixel rate ? > > Others have tested with other csi-2 receivers which sample link freq > using v4l2_get_link_freq() as well, and they have not reported issues > afaict. > I feel that we should share the same test procedure in order to confirm test verdicts for all known platforms. > (Please note that using a link_freq that doesn't come from the control > in ov5640_set_mipi_pclk() makes it harder to tune the pixel rate from > userspace to accommodate more configurations, as I suggested we should > do in reply to "[PATCH v5 16/27] media: ov5640: Add VBLANK control" > but it might still be doable). > > Thanks a lot for testing! > j > > > [2] That's what I have applied > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index faca36dc4187..910b58fb1e08 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -461,6 +461,7 @@ struct ov5640_dev { > > bool pending_mode_change; > bool streaming; > + s64 current_link_freq; > }; > > static inline struct ov5640_dev *to_ov5640_dev(struct v4l2_subdev *sd) > @@ -1439,7 +1440,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor) > int ret; > > /* Use the link frequency computed at ov5640_update_pixel_rate() time. */ > - link_freq = ov5640_csi2_link_freqs[sensor->ctrls.link_freq->cur.val]; > + link_freq = sensor->current_link_freq; > > /* > * - mipi_div - Additional divider for the MIPI lane clock. > @@ -2836,6 +2837,8 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > } while (link_freq >= 1000000000U && > ++pixel_rate_id < OV5640_NUM_PIXEL_RATES); > > + sensor->current_link_freq = link_freq; > + > /* > * Higher link rates require the clock tree to be programmed with > * 'mipi_div' = 1; this has the effect of halving the actual output > @@ -2844,8 +2847,10 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > * Adjust the pixel rate control value to report it correctly to > * userspace. > */ > - if (link_freq > OV5640_LINK_RATE_MAX) > + if (link_freq > OV5640_LINK_RATE_MAX) { > pixel_rate /= 2; > + link_freq /= 2; > + } > > for (i = 0; i < ARRAY_SIZE(ov5640_csi2_link_freqs); ++i) { > if (ov5640_csi2_link_freqs[i] == link_freq) > @@ -3777,6 +3782,7 @@ static int ov5640_probe(struct i2c_client *client) > sensor->current_mode = > &ov5640_mode_data[OV5640_MODE_VGA_640_480]; > sensor->last_mode = sensor->current_mode; > + sensor->current_link_freq = OV5640_DEFAULT_LINK_FREQ; > > sensor->ae_target = 52; > > > > >> >> >> Doing so we cannot relay anymore on link_frequency control reading in >> ov5640_set_mipi_pclk(), use current_link_freq variable instead >> >> @@ -1440,7 +1453,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev >> *sensor) >> int ret; >> >> /* Use the link freq computed at ov5640_update_pixel_rate() time. */ >> - link_freq = ov5640_csi2_link_freqs[sensor->ctrls.link_freq->cur.val]; >> + link_freq = sensor->current_link_freq; >> >> @@ -3782,6 +3811,7 @@ static int ov5640_probe(struct i2c_client *client) >> sensor->current_mode = >> &ov5640_mode_data[OV5640_MODE_VGA_640_480]; >> sensor->last_mode = sensor->current_mode; >> + sensor->current_link_freq = OV5640_DEFAULT_LINK_FREQ; >> >> >> >> >>> + >>> + for (i = 0; i < ARRAY_SIZE(ov5640_csi2_link_freqs); ++i) { >>> + if (ov5640_csi2_link_freqs[i] == link_freq) >>> + break; >>> + } >>> + >>> + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, pixel_rate); >>> + __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, i); >>> + >>> + return 0; >>> +} >>> + >>> static int ov5640_set_fmt(struct v4l2_subdev *sd, >>> struct v4l2_subdev_state *sd_state, >>> struct v4l2_subdev_format *format) >>> @@ -2451,8 +2513,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, >>> /* update format even if code is unchanged, resolution might change */ >>> sensor->fmt = *mbus_fmt; >>> - __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, >>> - ov5640_calc_pixel_rate(sensor)); >>> + ov5640_update_pixel_rate(sensor); >>> + >>> out: >>> mutex_unlock(&sensor->lock); >>> return ret; >>> >> >> Hugues. Hugues.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index f9763edf2422..791694bfed41 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -31,6 +31,8 @@ #define OV5640_DEFAULT_SLAVE_ID 0x3c +#define OV5640_LINK_RATE_MAX 490000000U + #define OV5640_REG_SYS_RESET02 0x3002 #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 #define OV5640_REG_SYS_CTRL0 0x3008 @@ -2412,6 +2414,66 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, return 0; } +static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) +{ + const struct ov5640_mode_info *mode = sensor->current_mode; + struct v4l2_mbus_framefmt *fmt = &sensor->fmt; + enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; + unsigned int i = 0; + u32 pixel_rate; + s64 link_freq; + u32 num_lanes; + u32 bpp; + + /* + * Update the pixel rate control value. + * + * For DVP mode, maintain the pixel rate calculation using fixed FPS. + */ + if (!ov5640_is_csi2(sensor)) { + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, + ov5640_calc_pixel_rate(sensor)); + + return 0; + } + + /* + * The MIPI CSI-2 link frequency should comply with the CSI-2 + * specification and be lower than 1GHz. + * + * Start from the suggested pixel_rate for the current mode and + * progressively slow it down if it exceeds 1GHz. + */ + num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; + bpp = ov5640_code_to_bpp(fmt->code); + do { + pixel_rate = ov5640_pixel_rates[pixel_rate_id]; + link_freq = pixel_rate * bpp / (2 * num_lanes); + } while (link_freq >= 1000000000U && + ++pixel_rate_id < OV5640_NUM_PIXEL_RATES); + + /* + * Higher link rates require the clock tree to be programmed with + * 'mipi_div' = 1; this has the effect of halving the actual output + * pixel rate in the MIPI domain. + * + * Adjust the pixel rate control value to report it correctly to + * userspace. + */ + if (link_freq > OV5640_LINK_RATE_MAX) + pixel_rate /= 2; + + for (i = 0; i < ARRAY_SIZE(ov5640_csi2_link_freqs); ++i) { + if (ov5640_csi2_link_freqs[i] == link_freq) + break; + } + + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, pixel_rate); + __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, i); + + return 0; +} + static int ov5640_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_format *format) @@ -2451,8 +2513,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, /* update format even if code is unchanged, resolution might change */ sensor->fmt = *mbus_fmt; - __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, - ov5640_calc_pixel_rate(sensor)); + ov5640_update_pixel_rate(sensor); + out: mutex_unlock(&sensor->lock); return ret;