TC358743: Corrupted image on capturing 1080p@50Hz at RGB24 with 4 lanes #6322
Comments
|
I suspect it's the FIFO trigger point in the TC358743 for matching HDMI rate to CSI2 rate. The mainline driver only cared about 720p60 and 1080p60 over 4 lanes, and only using a CSI2 link data rate of 594Mbit/s/lane. Revert to the 594Mbit/s mode by using |
|
Sorry, ignore my stupid deleted comment :) I'll try link-frequency now. |
|
Added
I have this spreadsheet, but I don't understand exactly what I should do here, my expertise in this area is not enough :/
I also read something similar. But without RGB, the display of the screen in DRM will not work (for the same function for which you recently fixed H.264 for us). |
|
I could probably add a fifo level for 1080p@50hz/RGB, but I don't quite understand what the more general condition for switching to this level is. Apparently, some other video modes may have problems too. |
My hunch was right then that the default configuration had insufficient bandwidth for 1080p60 RGB24.
Put your timing (1080p50 = 148.500 1920/528/44/148/+ 1080/4/5/36/+) in in row 13, select "original" in B14. "FIFO Delay" field is the one you're looking for at H24. I get min 120, max 511, which implies that the driver setting of 374 isn't being set.
|
|
Hm, I've tried 511 and it broke both 60 and 50 Hz modes. Something wrong: |
|
Typo by me |
|
Unchanged 374 value: But is it a right order?.. |
|
Ok, the order is right. I found that minimal value you calculated makes 50Hz working, and also fixing 720p@30Hz. |
4.13.3 I2C Read Access Translation
0x78 being 120. Looking at it, I suspect 1080p50 RGB is only using 3 lanes (spreadsheet says it needs 898Mbit/s on 3 lanes), so it's likely to be the modes that are at the top end of the bandwidth requirements for the number of lanes that have issues. If Actually I think that is totally it - it's using bt->height in the calculation, therefore the time per line (which is the critical thing for FIFO usage) is going to be wrong and quite likely to result in FIFO over/under flows. Use a mode with a long blanking period and the numbers will be totally garbage. |
Well, I tried, it's not working. Very weird.
Sorry, I don't get it, about "little more time". Could you explain a bit? Meanwhile, I made a shitty patch with fifo_level tuning. I can keep it for my work, or I can submit it as PR here. What you say? In my opinion, this is too specific a thing and resembles a dirty hack, on the other hand, it fully provides the capabilities of the chip (or almost completely, I can test other resolutions from my EDID). diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 5b6f74e56..d99d1c84b 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -54,6 +54,8 @@ MODULE_LICENSE("GPL");
#define POLL_INTERVAL_CEC_MS 10
#define POLL_INTERVAL_MS 1000
+#define FIFO_LEVEL_DEFAULT 374
+
static const struct v4l2_dv_timings_cap tc358743_timings_cap = {
.type = V4L2_DV_BT_656_1120,
/* keep this initialization for compatibility with GCC < 4.4.6 */
@@ -546,6 +548,24 @@ static inline void enable_stream(struct v4l2_subdev *sd, bool enable)
i2c_wr32(sd, TXOPTIONCNTRL, MASK_CONTCLKMODE);
/* Unmute video */
i2c_wr8(sd, VI_MUTE, MASK_AUTO_MUTE);
+
+ if (state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24) {
+ /* https://github.com/raspberrypi/linux/issues/6322 */
+ struct v4l2_bt_timings *bt = &state->timings.bt;
+ struct tc358743_platform_data *pdata = &state->pdata;
+ unsigned f = fps(bt);
+ unsigned fifo_level = FIFO_LEVEL_DEFAULT;
+ if (
+ (bt->width == 1920 && bt->height == 1080 && f == 50)
+ || (bt->width == 1280 && bt->height == 720 && f == 30)
+ ) {
+ fifo_level = 120;
+ }
+ if (i2c_rd16(sd, FIFOCTL) != fifo_level) {
+ pdata->fifo_level = fifo_level;
+ i2c_wr16(sd, FIFOCTL, pdata->fifo_level);
+ }
+ }
} else {
/* Mute video so that all data lanes go to LSP11 state.
* No data is output to CSI Tx block. */
@@ -1289,6 +1309,8 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
"yes" : "no");
v4l2_info(sd, "CEC enabled: %s\n",
(i2c_rd16(sd, CECEN) & MASK_CECEN) ? "yes" : "no");
+ v4l2_info(sd, "FIFO level: %d\n",
+ (i2c_rd16(sd, FIFOCTL)));
v4l2_info(sd, "-----Signal status-----\n");
v4l2_info(sd, "TMDS signal detected: %s\n",
hdmi_sys_status & MASK_S_TMDS ? "yes" : "no");
@@ -1940,7 +1962,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
state->pdata.enable_hdcp = false;
/* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
- state->pdata.fifo_level = 374;
+ state->pdata.fifo_level = FIFO_LEVEL_DEFAULT;
/*
* The PLL input clock is obtained by dividing refclk by pll_prd.
* It must be between 6 MHz and 40 MHz, lower frequency is better. |
https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/media/i2c/tc358743.c#L690 with (typos excepted) If using only
Sorry I won't accept that hack. Any fix needs to be justifiable, and ideally upstreamed. |
I see. I'll test it and come back, thank you.
Oh, don't be sorry. As I said, this is a ugly hack and I don't like it too. If your formula works well, then I will prefer it ofc. |
It's not working at all for any RGB24, I'm just getting device timeout on select(). |
|
Here is a list of modes for which my hack helps: I also found a mode where glitches look different and fifo_level tuning does not help (something changes a little, but only slightly): |
You don't say what that mode is. I'm afraid that as this isn't a Raspberry Pi product the support level provided for it is relatively limited (same as third party cameras). The vendor of the board really should be providing the support, and I'm afraid I can't dedicate any significant amount of time to investigating issues. I believe TC358743 is EOL from Toshiba now, and I don't believe we (Raspberry Pi) have any support contacts there anymore, so it rather limits options for getting support from Toshiba. You may get some further support from mainline (ie linux-media mailing list) as AIUI Cisco are using the chip in some of their video conferencing products. I think they were using a TI based platform (certainly not configured via device tree as they passed in platform data) .... https://www.spinics.net/lists/linux-media/msg116360.html they use a fifo_level of 300. |
Sorry, 1280x1024@60Hz with RGB24, for example.
I understand and appreciate the time you are taking to do this. If you find time to throw up some hypotheses or suggest changes to the code, I'm ready to continue testing anything to help with this, again and again.
In one of the topics about TC358743 and Pi5, you wrote that Raspberry will do some kind of project on this chip, so maybe fixing the magic fifo_level computing will be a win-win for us. If I still bother you with these endless problems with Toshiba, please let me know and we will try to figure it out on our side, although it will be hard and long. |
OK, I may have a quick look when time allows.
As above, I expect that the issue will be modes where:
Identifying which category it is would be the first step. Second category would be fixed if the FIFO trigger can be set dynamically. The first may need to increase the number of lanes (which may then trigger the second).
Our project is largely for CI purposes, so as long as the main resolutions work then it's probably good enough. Issues in the TC358743 are more likely to be worked around rather than investigated and fixed.
Feel free to ask, but you may well find the response is fairly limited. |
|
Thanks for the tips. My HW engineer and I will carefully read the datasheet and maybe find the right way to use blanking and other things.
Ah, I see. This is what we are already doing with PiKVM hardware. I have some experience with this and have written code to test video capture. Let me know if you interested.
Same. Solving problems in general can take too long, so a patch for certain conditions is often an acceptable solution for me.
Thank you, I appreciate it. |
|
I found a bug: in some cases we got incorrect lanes number. Adding hsync/vsync to calculations (like you did before) brokes calculations at all because of u32 overflow due to diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 5b6f74e56..715ca429c 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -687,10 +687,15 @@ static unsigned tc358743_num_csi_lanes_needed(struct v4l2_subdev *sd)
struct tc358743_platform_data *pdata = &state->pdata;
u32 bits_pr_pixel =
(state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16) ? 16 : 24;
- u32 bps = bt->width * bt->height * fps(bt) * bits_pr_pixel;
+ u32 bps = (bt->width + bt->hsync) * (bt->height + bt->vsync)
+ * fps(bt) * bits_pr_pixel;
u32 bps_pr_lane = (pdata->refclk_hz / pdata->pll_prd) * pdata->pll_fbd;
- return DIV_ROUND_UP(bps, bps_pr_lane);
+ /*
+ * We're don't using DIV_ROUND_UP() here because its formula
+ * causes u32 overflow when values are too big.
+ */
+ return (bps / bps_pr_lane) + (((bps % bps_pr_lane) != 0) ? 1 : 0);
}
static void tc358743_set_csi(struct v4l2_subdev *sd)Also the UPD: But this is for 4lane=1 and CM4. On Pi4 and UYVY, this overestimates the required number of lanes to 3 for 1080p@50Hz (2 without a patch). It is unclear how it is supposed to be calculated in the right way. BTW I'm not sure if porch offvalues are needed because it doesn't seem to be used in the case of TC358743. They are not used in current frame geometry calculations, so they are not needed for accurate calculation of lanes either. Correct me if I'm wrong: https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/media/i2c/tc358743.c#L342 PS: Thanks for the research direction, now I understand a little more. |
|
Great news that the theory is looking fairly plausible. Per line period on CSI2 you need the pixel data plus 48 bits of header/footer, a line start packet , and a line end packet (each 32bits). Each packet will also need an LP to HS transition beforehand, and an HS to LP transition afterwards. The time period of those can be computed, but I forget the numbers off the top of my head. AIUI Vertically we do want to factor in all the blanking lines to avoid the CSI2 line readout period exceeding the HDMI line period. ... Feeding the relevant values for 1080p50 UYVY into the spreadsheet. It's claiming a minimum lane speed of 898.12Mbit/s/lane on 2 lanes. (Ah, it computes the LP<>HS transition time in B61 as 675ns, based on all the magic clock times in C42-C60). So the formula in the driver should replicate that.
|
|
That's interesting... I'll check the documentation again, maybe I missed some recommendations about calculating periods. By the way, do we really need to calculate the number of lanes dynamically? Why can't we always use the maximum number of lanes available in hardware for any resolutions, even small? Perhaps the limitations of the chip? I'm going to check it.
But why? I thought modulo might be banned in the kernel, but a cursory search showed that it is used, so it shouldn't be a big problem. And u64 is not available on 32-bit. |
If you increase the CSI2 total data rate, then you really come down to FIFO trigger points. 1Gbit/s/lane and 4 lanes will shift 250MPix/s. An HDMI pixel clock of say 65MHz for 1024x768@60 is 65MPix/s. (Plumbing the numbers into the spreadsheet claims that a FIFO delay of 376 is acceptable, but it agrees with my 4usec active period on CSI2, so I think it just doesn't take that possibility into account).
Just preference upstream - use the macros wherever possible, or rearrange the computation. |
|
Revised (but not tested on hardware) version of the function. I believe it gives the right numbers for me with the 3 resolutions I've just tried it with (1024x768@60, 1080p50, and 1080p60). The fact that The one bit I haven't addressed is the overhead of LP<>HS transitions. If I can quantify it approximately with regard the pixel rate, then adding a bit to The commit message will need to mention that the maths is close to overflow and you're right that it can't use DIV_ROUND_UP. Max |
|
Thank you, I'll test it and let you know. The algorithm sounds very plausible.
The strange thing is, it didn't occur to me why this might be necessary.
I would still add rounding for paranoid reasons. We can divide both |
|
Sorry for the delay. I encountered that division to |
|
The "joys" of 64 bit arithmetic in 32bit kernels. Seeing as the maximum pixel clock the TC358743 supports is around 165MHz, and that fits into a 32bit word, we can do a simple cast down from the u64 of Over and above the last diff |
|
Okay, it's compiling. But for 1080p@50Hz it gaves 3 lanes instead of 2. |
|
I haven't got it set up, but my numbers say 2 lanes. hdmi_line_freq = 148500000 / (1920+528+44+148) = 56250 (56.250kHz matches HDMI spec for VIC 31). returned value = (1728000000 / 972000000) + 1 = 2. |
|
Ah, you're using 16 bit per pixel, I'm testing on 24 bit, so csi2_data_rate=2592000000 in my case. My previous patch gave 4 lines for 1080p@50Hz at RGB24, while the picture was correct. With your patch and 3 lines, I see artifacts like on the picture in the issue, divided into three color sections, and most likely we need to adjust the fifo_level for this. |
|
I'm confused
1080p50 has only ever worked as UYVY on 2 lanes. Was your comment meant to be "it gaves 3 lanes instead of 4"? 4 lanes are not required according to the spreadsheet. I'll accept that reality may differ from the spreadsheet, but need to be able to justify that difference. |
I'm already confused myself. Talking about 1080p50: your formula gaves 2 lanes for UYVY and 3 lanes for RGB24. And it requires fifo_level correction, as you indicated, I'll try. My previous patch gaves 4 lanes for RGB24, and it was usable with default fifo_level=374. |
Describe the bug
When I'm trying to capture image with RGB24 format at 1080p@50Hz, I'm getting corrupted image like this:
At the same time, 60Hz is working fine and 30Hz is fine too. There seems to be something wrong with either stride or synchronization. UYVY is working fine at all frequencies.
Steps to reproduce the behaviour
gst-launch-1.0 -v -e v4l2src io-mode=4 ! video/x-raw,format=RGB ! capssetter caps="video/x-raw,format=BGR" ! v4l2convert output-io-mode=5 capture-io-mode=4 ! video/x-raw,width=1920,height=1080,format=BGRA ! kmssink(see on HDMI display)ustreamer --host:: --port=8080 --format=rgb24 --format-swap-rgb --dv-timings --persistent(see on http://pi:8080).Device (s)
Raspberry Pi CM4
System
Linux pikvm 6.6.45-2-rpi #1 SMP Wed Aug 21 00:26:23 UTC 2024 armv7l GNU/LinuxLogs
Additional context
No response
The text was updated successfully, but these errors were encountered: