Message-ID: <YysOnZt+yRGNw2CN@paasikivi.fi.intel.com>
Date:   Wed, 21 Sep 2022 13:16:13 +0000
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Mikhail Rudenko <mike.rudenko@...il.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Shawn Tu <shawnx.tu@...el.com>, Jimmy Su <jimmy.su@...el.com>,
        Arnd Bergmann <arnd@...db.de>, Arec Kao <arec.kao@...el.com>,
        Marek Vasut <marex@...x.de>, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/2] Add Omnivision OV4689 image sensor driver

Hi Mikhail,

On Tue, Sep 20, 2022 at 11:31:01PM +0300, Mikhail Rudenko wrote:
> 
> Hi Laurent, Sakari,
> 
> On 2022-09-19 at 16:49 +03, Laurent Pinchart <laurent.pinchart@...asonboard.com> wrote:
> 
> > Hello,
> >
> > On Mon, Sep 19, 2022 at 10:31:02AM +0000, Sakari Ailus wrote:
> >> On Mon, Sep 19, 2022 at 10:01:06AM +0300, Mikhail Rudenko wrote:
> >> > On 2022-09-19 at 06:40 GMT, Sakari Ailus wrote:
> >> > > On Fri, Sep 16, 2022 at 12:27:42AM +0300, Mikhail Rudenko wrote:
> >> > >> On 2022-09-14 at 10:58 +01, Dave Stevenson wrote:
> >> > >> > On Sun, 11 Sept 2022 at 21:02, Mikhail Rudenko wrote:
> >> > >> >>
> >> > >> >> Hello,
> >> > >> >>
> >> > >> >> this series implements support for Omnivision OV4689 image
> >> > >> >> sensor. The Omnivision OV4689 is a high performance, 1/3-inch, 4
> >> > >> >> megapixel image sensor. Ihis chip supports high frame rate speeds up
> >> > >> >> to 90 fps at 2688x1520 resolution. It is programmable through an I2C
> >> > >> >> interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
> >> > >> >> connection.
> >> > >> >>
> >> > >> >> The driver is based on Rockchip BSP kernel [1]. It implements 4-lane CSI-2
> >> > >> >> and single 2688x1520 @ 30 fps mode. The driver was tested on Rockchip
> >> > >> >> 3399-based FriendlyElec NanoPi M4 board with MCAM400 camera
> >> > >> >> module.
> >> > >> >> While porting the driver, I stumbled upon two issues:
> >> >
> >> > [snip]
> >> >
> >> > >> >> (2) The original driver exposes analog gain range 0x0 - 0x7ff, but the
> >> > >> >> gain is not linear across that range. Instead, it is piecewise linear
> >> > >> >> (and discontinuous). 0x0-0xff register values result in 0x-2x gain,
> >> > >> >> 0x100-0x1ff to 0x-4x, 0x300-0x3ff to 0x-8x, and 0x700-0x7ff to 0x-16x,
> >> > >> >> with more linear segments in between. Rockchip's camera engine code
> >> > >> >> chooses one of the above segments depenging on the desired gain
> >> > >> >> value. The question is, how should we proceed keeping in mind
> >> > >> >> libcamera use case? Should the whole 0x0-0x7ff be exposed as-is and
> >> > >> >> libcamera will do the mapping, or the driver will do the mapping
> >> > >> >> itself and expose some logical gain units not tied to the actual gain
> >> > >> >> register value? Meanwhile, this driver conservatively exposes only
> >> > >> >> 0x0-0xf8 gain register range.
> >> > >> >
> >> > >> > The datasheet linked above says "for the gain formula, please contact
> >> > >> > your local OmniVision FAE" :-(
> >> > >> > I would assume that the range is from 1x rather than 0x - people
> >> > >> > rarely want a totally black image that 0x would give. Or is it ranges
> >> > >> > of 1x - 2x, 2x - 4x, 4x - 8x, and 8x - 16x?
> >> > >>
> >> > >> A picture is worth a thousand words, so I've attached the results of my
> >> > >> experimentation with the gain register. They were obtained with Rockchip
> >> > >> 3399, with AEC, AGC and black level subtraction disabled. The image was
> >> > >> converted from 10-bit RGGB to 8-bit YUV 4:2:0 by the Rockchip ISP.
> >
> > Is that full or limited range YUV ?
> >
> >> > > Based on that it looks like their medication may have been a little too
> >> > > strong.
> >> > >
> >> > > Could this be implemented so that the control value would be linear linear
> >> > > but its range would correspond 1x--16x values?
> >> > >
> >> > > libcamera will be able to cope with that.
> >> >
> >> > According to the following fragment of the Rockchip camera engine sensor
> >> > configuration file for ov4689 [1]
> >> >
> >> >     <Linear index="1" type="double" size="[4 7]">
> >> >        [1 2 128 0 1 128 255
> >> >         2 4 64 -248 1 376 504
> >> >         4 8 32 -756 1 884 1012
> >> >         8 16 16 -1784 1 1912 2040]
> >> >     </Linear>,
> >> >
> >> > it uses gain register value range 128-255 for gain 1x-2x, 376-504 for
> >> > gain 2x-4x, 884-1024 for 4x-8x, and 1912-2040 for 8x-16x. Do you suggest
> >
> > That looks *really* weird. I would have understood [384, 511], [896,
> > 1023] and [1920, 2047], but not those intervals.
> >
> > The driver hardcodes bit 0x3503[2] to 1, which means "sensor gain
> > format". Maybe setting it to 0 ("real gain format") would produce saner
> > results ?
> >
> >> > to implement this calculation in the sensor driver and expose some
> >> > linear "logical" gain to userspace (ranging, e.g., 128-2048 for gains
> >> > 1x-16x)?
> >>
> >> Yes. This way the user space can somehow work without knowing this special
> >> implementation, even though the granularity changes over the range. I guess
> >> the granularity would need to be known in libcamera but that's a separate
> >> issue.
> >
> > I can live with that.
> 
> I got some fresh data regarding gain setting, with gain register value
> ranging from 0 to 4096, please check the attached plot. What is the best
> way to expose this to userspace in your opinion?

I know I requested this to be changed to be as linear as possible, but it
would seem that the gain values do not seem to match exactly what is
documented. So you'd need to experimentally find where you'd need to switch
the range and you might arrive at better values after the initial
implementation. There might be differences between units, too, and if there
were tuning values, you'd find this in sensor EEPROM.

Therefore I think this appears to fit less well for driver implementation.
I'm fine with exposing this to the user space as-is although it doesn't
make a great user space inteface. It's just a poor hardware implementation
but there's nothing we can do about it.

-- 
Kind regards,

Sakari Ailus

Powered by blists - more mailing lists