-
Notifications
You must be signed in to change notification settings - Fork 5.2k
IMX708: Add support for 4-lane operation #6775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rpi-6.12.y
Are you sure you want to change the base?
Conversation
…e number of MIPI lanes
…ngs based on the number of mipi lanes
Leave another comment when you think it is worth rerunning the build checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a more detailed look later.
The datasheet you link to is for IMX 078, not 708.
The main datasheet says max frame rate at full res if 60fps, but that will be at 2.5Gb/s per lane which no Pi supports. 30fps for your 1094Mbit/s data rate seems plausible (just).
Forcing the use of 547MHz link frequency in 4 lane mode is a little annoying. Although dtoverlay=imx708,4lane,link-frequency=450000000
would drop it back down. Does there need to be validation of 2 lane vs 4 lane link frequencies?
drivers/media/i2c/imx708.c
Outdated
{0x3062, 0x00}, | ||
{0x3063, 0x12}, | ||
{0x3063, 0x12}, //0x30, 0x12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want to be changing this register.
I dropped the link frequency back to 450000000 and set the pixel rate to 777.6Mhz and I was able to get a stable output of 28.13fps. I also tried increasing the pixel clock to get closer to 30fps, but it just resulted in artificating and other sensor errors - I tried pixel rates of 854.4Mhz and 801.6Mhz. I've also tried dropping the line length down to 5216 and tried all the current pixel rates in the driver but I just get artifcating and this libcamera error (still at the 450000000 link frequency):
|
will always result in corruption as the CFE overflows. IMX708 is the first sensor where there have been such distinct line length values that could be supported. Unfortunately it means that the pixel rate and HBLANKing values do need to allow for sufficient time per line. It's curious that the IVT multiplier is different between different modes of the sensor. It just makes the driver a little more complicated. I can't think of an obvious reason why the highest value can't be used for all modes, but I haven't looked at the full timings to ensure that it can offload the image data over CSI2 within the relevant line period. There must be a setting of IVT that means the line length of 5216 comes in at >=12.13us I'm afraid I simply don't have time to play with imx708 at present. |
I've successfully been able to get it running at a line length of 5216, but I still run into the limit of >=12.13us for the line length. Anything below that and libcamera complains and the image output is all messed up. The highest I've been able to push the camera is 31fps with a line length of 5216 and a pixel rate of 432Mhz. This combo throws an error in IPARPI and messes with the frame rate calculation, but doesn't cause any visible artifcating on the sensor. Based on my calculations for the bandwith of the pi5, we should be able to get ~35fps, but I'm not seeing any way around this 12.13us line length limit as I'd need to push the pixel rate higher (which lowers the line length) in order to squeeze out the last few fps. Do you have any other ideas? |
As it appears there are such restrictions on the line_length register for IMX708, the only route I can see would be to alter the pixel rate so that the line takes 12.13us to readout with whichever line length setting most sensibly matches that. The max frame rate will then be dictated by the number of lines required in your image. There is an option to overclock RP1 to increase the throughput of the CFE slightly. Those working with imx294 have been doing that - see #6725 and raspberrypi/libcamera#243 |
That did the trick! I didn't realize that the RP1 was limiting the throughput to 380Mpix/s. I overclocked it to 300Mhz as outlined in those PRs, and also overclocked the mipi lanes to 1800Mhz and I'm able to get a stable 45fps output! I think this is pretty much the upper limit we can do on the pi5, unless I can overclock the mipi lanes even higher? |
Hopefully this isn't too off-topic of a question. Has anyone been able to run the IMX708 in a single-lane configuration? ed: I suppose it would just be a change to the line "data-lanes = <1 2 3 4>;", and probably |
Not supported. |
That's disappointing. Thanks for the definitive answer. |
How did you do that? In dphy.c I only see options for going up to 1500Mhz -
|
* Dynamically calculate link frequency registers * Add more pixel rates for testing * Drop line_length_pix to the lowest value for the resolution and mode * Initial attempt to dynamically calculate the pixel rate
@rajeshpoddar I just set |
@6by9 I just pushed a commit that will dynamically calculate the link frequency registers. I'm also trying to dynamically set the pixel rate by setting the line length to the minimum value possible for the resolution/mode and then calculating the appropriate pixel rate based on the link frequency and number of mipi lanes. I want to make sure I handle this calculation properly, so I've got a couple of questions.
|
@@ -792,7 +846,7 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = { | |||
.num_of_regs = ARRAY_SIZE(mode_4608x2592_regs), | |||
.regs = mode_4608x2592_regs, | |||
}, | |||
.pixel_rate = {595200000, 854400000}, | |||
.pixel_rate = {198400000, 304000000}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about this factor-of-3 change of pixel rate and line length (though I never understood the rationale of our original settings either). Is it a real, functional change or just a change in the way things are represented? Does it improve image quality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't change image quality at all, only readout rate.
I suspect that we just copied the values provided by Sony in their big spreadsheet as supposedly the optimised values, but they often wrote the register set based on a requested resolution/frame rate combination whereas we actually want the max that can be achieved by the hardware.
Max pixel rate is going to depend on the link frequency to ensure we can output all the pixel data in the appropriate period of time. I can see a case where the driver ought to verify that the selected link frequency can do that, because otherwise DT can give us unworkable configurations. This is going to need a fair amount more analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Specifically for full-res mode and with 2 lanes, the old rate (595200000) was the Sony recommended value. It has suddenly been divided by 3 (and the line width changed to match). I just wondered why this was changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped the line length down to their minimum possible values for the given resolution, as outlined here. I plan to clean this up so there's only a single entry for line_length_pix, like before. This will ensure the user is always able to set the maximum framerate based on the number of MIPI lanes and the data rate of those lanes.
I then adjusted the pixel rate accordingly for the 2-lane full-res mode to match the original settings with the lower line length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I don't know why Sony recommended the faster rate (and they might not be willing to explain). I thought it might be extra cycles needed for re-Mosaic but if the image is unaffected then perhaps it's not needed.
IMX708_LINK_FREQ_547MHZ, | ||
IMX708_LINK_FREQ_640MHZ, | ||
IMX708_LINK_FREQ_750MHZ, | ||
IMX708_LINK_FREQ_900MHZ, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tangential suggestion: Do we even need discrete values for link frequency: Couldn't we just support all frequencies from 447MHz to 750MHz, in 3MHz steps, if that is what the PLL is capable of?
Of course the default values in DeviceTree will have to be carefully chosen for compatibility and EMC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PLL should be capable of even more than that as both IVT_PREPLLCK_DIV and IVT_PLL_MPY could be modified. Potentially the ccs-pll helper functions could compute PLL settings, but it adds quite a lot of complexity.
You actually get more fun in having to create the array of s64's for the V4L2_CID_LINK_FREQUENCY control - probably stick it in struct imx708
so that every instance of imx708 could choose a preferred link frequency.
Default values would remain as is for 2 lane.
4 lane we probably declare as experimental and not EMC tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need that flexibility -- multiples of 3MHz seem fine -- I wondered if we could get rid of this apparently random-looking list of values, and replace it by a stepped range?
Actually some of these magic-looking numbers are not multiples of 3MHz (547MHz and 640MHz). Are those implemented accurately right now?
I am experimenting at the moment with other clock frequencies like 480MHz and it feels silly to have to add them to the enum just to try them out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic should be:
(24MHz / (reg 0x030d)) * ((reg 0x030e<<8) + (reg 0x030f)) / 2
0x030d is 4 in all modes, giving us 3MHz * (030e/f)
I'd agree that imx708_configure_link_frequency
is computing these values, but there is no point in having the enum. Read the raw link frequency from DT, validate it against selected clock frequency and reg 0x030d, and store that value in struct imx708
.
Actually the LINK_FREQUENCY control can point to that as the single array value, and imx708_configure_link_frequency
can use it to compute the register settings.
@@ -95,6 +111,7 @@ | |||
vcm = <&vcm_node>, "status", | |||
<0>, "=4"; | |||
link-frequency = <&cam_endpoint>,"link-frequencies#0"; | |||
4lane = <0>, "+201+202"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there could be a confusing interaction between link-frequency
and 4lane
overrides, which both try to change <&cam_endpoint>/link-frequencies
. The actual frequency may depend on the order in which the dtparams are given.
If that's the case, is there a cleaner way to do it that would always give priority to the link-frequency
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly there isn't a way to set priorities on DT overrides.
What could be done is to add a fragment 203 which sets the link frequency for 2 lane mode. link-frequency
then updates both 202 and 203, and 4lane
disables 203 as well as enabling 201 and 202. At least that should give a consistent behaviour.
@@ -689,7 +846,7 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = { | |||
.num_of_regs = ARRAY_SIZE(mode_4608x2592_regs), | |||
.regs = mode_4608x2592_regs, | |||
}, | |||
.pixel_rate = 595200000, | |||
.pixel_rate = {198400000, 304000000}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested with 4 lanes your suggested pixel rate of 416000000 for 30fps, and it seems to work fine -- not only at a MIPI link frequency of "547MHz" (which I suspect is actually 546MHz) with no need to overclock RP1; but also at the perhaps more Pi-friendly 492MHz and as low as 480MHz (though not, alas, at 450MHz).
So I would be happy to see 30fps supported here. Of course it depends on the more general question of default link frequency.
This is just to get the ball rolling, will make sure to cleanup commits, descriptions, and break up the dtoverlay and driver into separate PRs.
I successfully have 4-lane operation automatically set by the overlay option inside the driver, dynamically switching the PLL registers as needed! I have tested max resolution @ 30fps and haven't run into any issues so far. rpicam-hello shows the following framerates and resolutions
Is 30fps the upper limit for max resolution on the RPI? I based the settings off what looks to be a 30fps mode for the X2 phone, but this datasheet indicates the max resolution of the sensor is 42fps in 10bit mode.
As for the other the 2304x1296 mode, I just followed 6by9's suggestion of halving the line length paired with the higher link frequency. Maybe more performance can be squeezed out of this one by tuning the PLL settings?