[libcamera-devel] [PATCH 1/4] libcamera: Move IPA sensor controls validation to CameraSensor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 22 18:01:33 CET 2022
Hi Jacopo,
CC'ing Sakari as I'm sure he loves gains.
On Tue, Nov 22, 2022 at 09:46:44AM +0100, Jacopo Mondi wrote:
> On Mon, Nov 21, 2022 at 11:01:42PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 04, 2022 at 12:20:22PM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Jacopo Mondi via libcamera-devel (2022-11-04 10:50:01)
> > > > The CameraSensor class validates that the sensor driver in use supports
> > > > the controls required for IPA modules to work correctly.
> > > >
> > > > For in-tree IPA modules, whose pipeline handlers already use
> > > > CameraSensor there's no need to validate such controls again.
> > > >
> > > > Remove controls validation from the IPU3 and RkISP1 IPA modules and rely
> > > > on CameraSensor doing that at initialization time.
> > > >
> > > > The list of mandatory controls is expanded to add V4L2_CID_ANALOGUE_GAIN
> > > > without which IPA modules cannot function.
> > > >
> > > > The new requirement only applies to RAW sensors, platforms like UVC and
> > > > Simple are not impacted by this change.
> > > >
> > > > While at it, expand the sensor driver requirements documentation to
> > > > include V4L2_ANALOGUE_GAIN in the list of mandatory controls a sensor
> > > > driver has to support.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > I think this is a good cleanup.
> > > If in the future we want to support non-raw sensors in IPU3/RKISP1
> > > (which we might) we'll need to do various other refactoring. But I
> > > actually think this is the right way forwards, moving things that are
> > > specific to V4L2 into the CameraSensor class, and perhaps making it
> > > easier to use 'libcamera' controls as the interface between IPA/PH.
> >
> > Fully agreed.
> >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > ---
> > > > Documentation/sensor_driver_requirements.rst | 7 +++++
> > > > src/ipa/ipu3/ipu3.cpp | 29 --------------------
> > > > src/ipa/rkisp1/rkisp1.cpp | 12 +-------
> > > > src/libcamera/camera_sensor.cpp | 1 +
> > > > 4 files changed, 9 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> > > > index b0854be3328a..3abc8f35924a 100644
> > > > --- a/Documentation/sensor_driver_requirements.rst
> > > > +++ b/Documentation/sensor_driver_requirements.rst
> > > > @@ -24,16 +24,23 @@ The sensor driver is assumed to be fully compliant with the V4L2 specification.
> > > >
> > > > For RAW sensors, the sensor driver shall support the following V4L2 controls:
> > > >
> > > > +* `V4L2_CID_ANALOGUE_GAIN`_
> > > > * `V4L2_CID_EXPOSURE`_
> > > > * `V4L2_CID_HBLANK`_
> > > > * `V4L2_CID_PIXEL_RATE`_
> > > > * `V4L2_CID_VBLANK`_
> > > >
> > > > +.. _V4L2_CID_ANALOGUE_GAIN: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
> > > > .. _V4L2_CID_EXPOSURE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
> > > > .. _V4L2_CID_HBLANK: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
> > > > .. _V4L2_CID_PIXEL_RATE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-process.html
> > > > .. _V4L2_CID_VBLANK: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
> > > >
> > > > +The ``ANALOGUE_GAIN`` control units are sensor-specific. libcamera requires
> > > > +a sensor-specific CameraSensorHelper implementation to translate between the
> > > > +sensor specific ``gain code`` and the analogue ``gain value`` expressed as an
> > > > +absolute number as defined by ``controls::AnalogueGain``.
> > > > +
> > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera
> > > > requires it to be expressed as a number of image lines. Camera sensor drivers
> > > > that do not comply with this requirement will need to be adapted or will produce
> > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > > index a9a2b49ca95b..08ee6eb30cc5 100644
> > > > --- a/src/ipa/ipu3/ipu3.cpp
> > > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > > @@ -171,8 +171,6 @@ private:
> > > > ControlInfoMap *ipaControls);
> > > > void updateSessionConfiguration(const ControlInfoMap &sensorControls);
> > > >
> > > > - bool validateSensorControls();
> > > > -
> > > > void setControls(unsigned int frame);
> > > > void calculateBdsGrid(const Size &bdsOutputSize);
> > > >
> > > > @@ -292,28 +290,6 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> > > > *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> > > > }
> > > >
> > > > -/**
> > > > - * \brief Validate that the sensor controls mandatory for the IPA exists
> > > > - */
> > > > -bool IPAIPU3::validateSensorControls()
> > > > -{
> > > > - static const uint32_t ctrls[] = {
> > > > - V4L2_CID_ANALOGUE_GAIN,
> > > > - V4L2_CID_EXPOSURE,
> > > > - V4L2_CID_VBLANK,
> > > > - };
> > > > -
> > > > - for (auto c : ctrls) {
> > > > - if (sensorCtrls_.find(c) == sensorCtrls_.end()) {
> > > > - LOG(IPAIPU3, Error) << "Unable to find sensor control "
> > > > - << utils::hex(c);
> > > > - return false;
> > > > - }
> > > > - }
> > > > -
> > > > - return true;
> > > > -}
> > > > -
> > > > /**
> > > > * \brief Initialize the IPA module and its controls
> > > > *
> > > > @@ -512,11 +488,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > >
> > > > calculateBdsGrid(configInfo.bdsOutputSize);
> > > >
> > > > - if (!validateSensorControls()) {
> > > > - LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > /* Update the camera controls using the new sensor settings. */
> > > > updateControls(sensorInfo_, sensorCtrls_, ipaControls);
> > > >
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 069c901bb141..c64cf380445e 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -215,20 +215,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > > ctrls_ = entityControls.at(0);
> > > >
> > > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> > > > - if (itExp == ctrls_.end()) {
> > > > - LOG(IPARkISP1, Error) << "Can't find exposure control";
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> > > > - if (itGain == ctrls_.end()) {
> > > > - LOG(IPARkISP1, Error) << "Can't find gain control";
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > int32_t minExposure = itExp->second.min().get<int32_t>();
> > > > int32_t maxExposure = itExp->second.max().get<int32_t>();
> > > >
> > > > + const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> > > > int32_t minGain = itGain->second.min().get<int32_t>();
> > > > int32_t maxGain = itGain->second.max().get<int32_t>();
> > > >
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index 572a313a8f99..ea373458a164 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -301,6 +301,7 @@ int CameraSensor::validateSensorDriver()
> > > > * required by the CameraSensor class.
> > > > */
> > > > static constexpr uint32_t mandatoryControls[] = {
> > > > + V4L2_CID_ANALOGUE_GAIN,
> >
> > I think that will break OV5640 with the simple pipeline handler and the
> > ISI pipeline handler. The driver exposes raw formats, so mandatory
> > controls are checked, and the analogue gain is implemented as
> > V4L2_CID_GAIN, not V4L2_CID_ANALOGUE_GAIN.
>
> It does, in facts.
>
> Should the driver be fixed or should this series be changed ?
I'd prefer the former if possible :-)
> Looking at the ov5640 implementation and documentation, CID_GAIN
> operates registers described as "Real gain". In the documentation I
> have there is no mention of "analogue gain" but only of "real gain"
> and a separate "digital gain".
>
> The real gain is said to be capable of a 64x gain increase, which
> seems a little high for analogue only. Maybe it bundles analogue+digital
> together ?
That is a good question. Why do you ask good questions ? :-)
Based on the documentation I've found, there's an AGC pre-gain in
register 0x3a13, expressed as a 6-bit value (plus an enable bit in bit
6). The driver hardcodes it to 0x43, which one application note states
is equal to x1.047. The documentation also states that 0x40 is equel to
x1.000. The pre-gain thus seems to be expressed as in 1/64 increments,
and thus ranges from x1.00 to x1.984. I have no idea what it does
though.
We then have an AGC gain limit, in registers 0x3a18 and 0x3a19,
expressed as a 10-bit "real gain format" value. One application note
sets it to 0x00f8 and states it is equal to x15.5, so it appears to be
expressed in 1/16 increments, up to x63.9375.
The manual gain is stored in registers 0x350a and 0x350b, also as a
10-bit "real gain format" value. It is documented in the application
note as a Q6.4 values, up to x63.9375.
One version of the datasheet I've found indicates that the sensor
supports a digital gain:
The OV5640 supports 1/2/4 digital gain. Normally, the gain is
controlled automatically by the automatic gain control (AGC) block.
It isn't clear how that would be controlled manually.
I haven't found any indication regarding whether the gain controlled
through registers 0x350a and 0x350b is an analog gain only or also
includes digital gain. The words "real gain" don't necessarily mean
"combined analog and digital gains". Some OmniVision sensors (such as
the OV8858) are documented as supoprting different formats for the gain
values, selectable through a register bit, and they are called "real
gain format" and "sensor gain format". For that sensor, we have (one of)
the gain registers documented as
0x3503[2]=0, gain[7:0] is real gain format, where low 4 bits are
fraction bits, for example, 0x10 is 1x gain, 0x28 is 2.5x gain
If 0x3503[2]=1, gain[7:0] is sensor gain format, gain[7:4] is coarse
gain, 00000: 1x, 00001: 2x, 00011: 4x, 00111: 8x, gain[7] is 1,
gain[3:0] is fine gain. For example, 0x10 is 1x gain, 0x30 is 2x gain,
0x70 is 4x gain
(The second part of the text makes little sense)
"Real gain" may thus refer to the combination of the coarse and fine
analog gains as a single value.
I'm tempted to consider the OV5640 0x350a and 0x350b registers as
controlling the analog gain only until proven wrong. If and when that
happens, we could restrict the range of the analog gain control value to
lower than x64 and add a separate digital gain control.
Any opinion ?
> > > > V4L2_CID_EXPOSURE,
> > > > V4L2_CID_HBLANK,
> > > > V4L2_CID_PIXEL_RATE,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list