Hi Dave, On Wed, Oct 19, 2022 at 12:38:21PM +0100, Dave Stevenson wrote: > Hi Sakari > > Thanks for the response > > On Wed, 19 Oct 2022 at 11:33, Sakari Ailus <sakari.ailus@xxxxxx> wrote: > > > > Hi Dave, > > > > On Mon, Oct 17, 2022 at 07:07:20PM +0100, Dave Stevenson wrote: > > > Hi Laurent > > > > > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson > > > <dave.stevenson@xxxxxxxxxxxxxxx> wrote: > > > > > > > > Hi Laurent > > > > > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart > > > > <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > Hi Dave, > > > > > > > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > > > > Hi Laurent > > > > > > > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > > > > patches for imx290 and there's little point in causing grief to each > > > > > > other with conflicts. > > > > > > Or I could take the non-controversial patches from your set and send a > > > > > > combined patch set? > > > > > > > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > > > > submit IMX327 support on top ? :-) > > > > > > > > Thanks - I'll review it tomorrow and sort my patches on top again. > > > > > > I've reworked my patches on top of yours. It gives r/w VBLANK and > > > HBLANK, and corrects PIXEL_RATE. > > > I'm testing on our 6.0 branch, but > > > https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c > > > is against the linux-media branch. > > > > > > I've messed something up in the "media: i2c: imx290: Support 60fps in > > > 2 lane operation" patch at present - I'm looking into what has gone > > > wrong, as the earlier versions of that patch worked fine. The branch > > > will get force-pushed once I've fixed it. > > > > > > > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > > > > > > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > > > > supported by the driver. I have patches to add 10bit support, but I > > > > don't increase the frame rate in them. > > > > > > > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > > > > again I haven't looked at adding support, partly as I don't have a > > > > datasheet for that variant. I may see if the change for 120fps 10bit > > > > on imx290 works in 12 bit mode for IMX462. > > > > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > > > > be supported (2 lanes not permitted), so there will be more link > > > > frequency messing required to support it. The basic numbers say that > > > > is fast enough for 12bit as well, so there's hope. > > > > > > I guess seeing as I'm messing with the clock setup, I may as well keep > > > going and look at the 120fps modes. It's a little trickier as the Pi > > > ISP will be on the edge at those rates, but it should be good enough. > > > > > > There is an awkward question with regard link-frequencies. Is there a > > > need to support multiple sets of link-frequency, or do we support any > > > set of 2? > > > ie for imx290, on 4 lanes do we want: > > > - 891Mbit/s for 1080p120 10bit > > > - 445.5Mbit/s for 1080p60 10 or 12 bit > > > - 594Mbit/s for 720p120 10bit > > > - 297Mbit/s for 720p60 10 and 12 bit > > > all to be present in DT? > > > If only 891 and 594 then you're limited to 10 bit images, but > > > otherwise it should be fully functional. The max frame interval would > > > be half that of 445.5 and 297 though, so there are compromises, but > > > who/what then controls the link_frequency to switch between the > > > ranges? > > > > It's up to the user space to set that control in a general case. I guess > > there are no specific rules on how many you should put to DT, but generally > > those that are useful should be there. > > Does the driver have to support multiple sets of link frequencies > simultaneously? It's up to the driver. A driver may support fewer features than the hardware allows, and with sensors this is commonly the case. So I don't think this would be special somehow. Of course if a driver supports just one and the DT has more, the end result may not be desirable in terms of what actually works. With e.g. CCS the user can choose any link frequency for which there is a valid PLL tree configuration with current settings. Depending on e.g. the bit depth, for instance, some frequencies may be unavailable. > > The way the driver is currently written you have one link freq for > 1080p and one for 720p (2/3rds the rate used for 1080p). You could > retain using only 2 link frequencies at a time. > > If DT/ACPI gives us 222.75MHz and 148.5MHz on 4 lanes, or 445.5MHz and > 297MHz on 2 lanes, with IMX327, IMX290 or IMX462, then use the current > configuration that can do 0.03 to 60fps as RAW10 or RAW12. > If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an > IMX290 (not IMX327), then use a new configuration that can do 0.06 to > 120fps as RAW10 only. > If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an > IMX462, then use a new configuration that can do 0.06 to 120fps as > RAW10 or RAW12. > If the configuration doesn't fall into any of these categories, then > it is rejected. Seems reasonable. > > Whoever is putting together the DT/ACPI for the device can then choose > whether they value the lower minimum frame rate and RAW12, or the > higher frame rate but are prepared to sacrifice RAW12. (As we use > dtoverlays, we can add overrides so that person is the end user). > Trying to cram that lot in so that it can all be used simultaneously > will get quite ugly - the register configuration is not quite as > simple as one might hope, and you'd be filtering the permitted modes > and bit depths all over the place. > > As I mentioned at the Media Summit in Dublin I've had users wanting > IMX462 for astronomy use cases, so halving the max exposure time by > dropping the current max 60fps configuration won't be popular. I guess > you could incorrectly use an IMX327 compatible string in the DT when > using an IMX290/462 to force the behaviour, but that feels even more > of a hack. > > > I wonder why 12 bpp output isn't possible at the double frequency. Of > > course it is possible the sensor's clock tree makes that impossible but it > > is still unusual. > > It is a little weird. As noted in the later emails, I have put > together settings to get 120fps running, and have tried it on both > IMX462 and IMX290. > > 720p120 RAW12 works fine on both, as do 720p120 and 1080p120 in RAW10. > However 1080p120 RAW12 doesn't work on either, so I suspect it is > something in the CSI2 block configuration that can't quite hit that > data rate without further changes. I'll see if Sony wants to be > friendly with datasheets for the IMX462. The receiver block driver could refuse to streamon if the pixel rate * bpp is too high. But I understand in this case it shouldn't be a limitation. And it doesn't really help the user to find which configurations would work. > > > > > > > I can see another can of worms being opened here! > > > > If this is what the sensor does, how else it should be operated? > > As above, link frequency remains read only based on DT or ACPI, and > that then restricts the configurations that are possible. > > I've never seen a good userspace example of using > V4L2_CID_LINK_FREQUENCY, so without that it always seems to be a > setting that is generally only used by the CSI-2 receiver to adapt > appropriately to the data rate. > To my mind it falls into the same category as binning and cropping - > it's lovely to expose the full feature set, but that is just passing > the buck to some heuristics in userspace. Generally the user of the > camera doesn't care (they just want their camera to work) so > realistically you're looking at libcamera, and it doesn't necessarily > have sufficient information about the sensor or use case to make a > good decision. The CCS driver changes the link frequency based on other configuration (mbus code) if the selected one cannot be used. As this is a board specific configuration, fixing these values for a sensor is not a feasible approach in libcamera. -- Kind regards, Sakari Ailus
