[libcamera-devel] [PATCH 1/4] libcamera: Move IPA sensor controls validation to CameraSensor
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 22 09:46:44 CET 2022
Hi Laurent,
On Mon, Nov 21, 2022 at 11:01:42PM +0200, Laurent Pinchart wrote:
> Hello,
>
> 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 ?
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 ?
> > > V4L2_CID_EXPOSURE,
> > > V4L2_CID_HBLANK,
> > > V4L2_CID_PIXEL_RATE,
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list