Re: [PATCH 00/19] media: i2c: imx290: Miscellaneous improvements

Laurent Pinchart
2023-01-14T16:03:23Z

Hello,

On Wed, Oct 19, 2022 at 04:27:13PM +0300, Sakari Ailus wrote:
> On Wed, Oct 19, 2022 at 12:38:21PM +0100, Dave Stevenson wrote:
> > On Wed, 19 Oct 2022 at 11:33, Sakari Ailus wrote:
> > > On Mon, Oct 17, 2022 at 07:07:20PM +0100, Dave Stevenson wrote:
> > > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson wrote:
> > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart wrote:
> > > > > > 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.

I'm with Dave here, I've never seen a good example of how userspace
should use the V4L2_CID_LINK_FREQUENCY control, or even of how the
frequencies listed in DT should be picked (for sensors that can
accommodate a wide range of link frequencies). Sakari, as you've been
the one who pushed for control of the link frequency from userspace,
could you enlighten us ? :-)

--
Regards,

Laurent Pinchart