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@xxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
---
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.
