[libcamera-devel] [PATCH v2 5/8] libcamera: camera_sensor: Expose a DelayedControls interface

[Jacopo Mondi]

Hi Niklas,

On Mon, Nov 23, 2020 at 10:49:39PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-11-18 15:23:19 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Nov 10, 2020 at 01:27:07AM +0100, Niklas Söderlund wrote:
> > > Expose the optional DelayedControls interface to pipeline handlers. If
> > > used by a pipeline generic delays are used. In the future the delay
> > > values should be fetched to match the specific sensor module, either
> > > from a new kernel interface or worst case a sensors database.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  include/libcamera/internal/camera_sensor.h |  5 ++++
> > >  src/libcamera/camera_sensor.cpp            | 31 ++++++++++++++++++++++
> > >  2 files changed, 36 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index b9eba89f00fba882..6be1cfd49134c534 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -14,6 +14,7 @@
> > >  #include <libcamera/controls.h>
> > >  #include <libcamera/geometry.h>
> > >
> > > +#include "libcamera/internal/delayed_controls.h"
> > >  #include "libcamera/internal/formats.h"
> > >  #include "libcamera/internal/log.h"
> > >  #include "libcamera/internal/v4l2_subdevice.h"
> > > @@ -61,6 +62,8 @@ public:
> > >  	ControlList getControls(const std::vector<uint32_t> &ids);
> > >  	int setControls(ControlList *ctrls);
> > >
> > > +	DelayedControls *delayedContols();
> > > +
> > >  	const ControlList &properties() const { return properties_; }
> > >  	int sensorInfo(CameraSensorInfo *info) const;
> > >
> > > @@ -83,6 +86,8 @@ private:
> > >  	std::vector<Size> sizes_;
> > >
> > >  	ControlList properties_;
> > > +
> > > +	std::unique_ptr<DelayedControls> delayedControls_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 935de528c4963453..6c92c97f4cc2547a 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls)
> > >  	return subdev_->setControls(ctrls);
> > >  }
> > >
> > > +/**
> > > + * \brief Get the sensors delayed controls interface
> > > + *
> > > + * Access the sensors delayed controls interface. This interface aids pipelines
> > > + * keeping tack of controls that needs time to take effect. The interface is
> > > + * optional to use and does not interact with setControls() and getControls()
> > > + * that operates directly on the sensor device.
> > > + *
> > > + * \sa DelayedControls
> > > + * \return The delayed controls interface
> > > + */
> > > +DelayedControls *CameraSensor::delayedContols()
> > > +{
> > > +	if (!delayedControls_) {
> > > +		/*
> > > +		 * \todo Read dealy values from the sensor itself or from a
> > > +		 * a sensor database. For now use generic values taken from
> > > +		 * the Raspberry Pi and listed as generic values.
> > > +		 */
> > > +		std::unordered_map<uint32_t, unsigned int> delays = {
> > > +			{ V4L2_CID_ANALOGUE_GAIN, 1 },
> > > +			{ V4L2_CID_EXPOSURE, 2 },
> > > +		};
> >
> > I would really not hardcode them here.
> >
> > As long as we don't have a sensor database or a place where these
> > values can be fetched from isn't it better to provide something like
> >
> >         using DelayMap = std::unordered_map<const ControlId *, unsigned int>;
> >
> >         CameraSensor::initControlDelays(DelayMap map)
> >         {
> >                 ....
> >                 /*
> >                  * CameraSensor gets the numerical ID from the
> >                  * DelayMap and construct a new map making sure the
> >                  * controls are supported by the subdev
> >                  */
> > 		delayedControls_ =
> > 			std::make_unique<DelayedControls>(subdev_.get(), delays);
> >
> >         }
> >
> > So that pipeline handlers receive delays from the IPA (as RPi
> > currently does if I'm not mistaken) and intialize Delayed controls on
> > the CameraSensor passing controls as standard libcamera ControlId ?
>
> I'm sorry I don't like this idea and I think the RPi approach is
> backwards :-) If we allow IPA's or pipelines to set the sensor delays we
> will IMHO end up with delay values coming from a wide range of sources.
>
> Some will come from pipelines or IPAs that link with libcamera that do
> utilise a sensor database. Some will come from hard coded values in
> pipeline or inside IPAs (like RPi today) and thus limit the usage of
> that IPA only to those sensors. It becomes even worse if we think about
> closed source IPAs where new sensors can not be added or incorrect
> delays corrected.
>
> I think the best solution is to embed the database lookup inside the
> CameraSensor class and have pipelines read the delays readout delays
> from the CameraSensor instance . If the IPA needs to be informed of the
> delays the pipeline should pass the delay values and not the sensor name
> to the IPA.
>
> For this reason I think its good to hardcode the values here as this is
> the location we should drop in a sensor database lookup once we have it.
>

I understand. Let's try with this approach to encourage us and other
developers to grow the sensor database and make it a first class
citizen. We could scale back to shortcuts eventually, but I agree
let's not start with that in mind.

Thanks
  j

> >
> > > +
> > > +		delayedControls_ =
> > > +			std::make_unique<DelayedControls>(subdev_.get(), delays);
> > > +	}
> > > +
> > > +	return delayedControls_.get();
> > > +}
> > > +
> > >  /**
> > >   * \brief Assemble and return the camera sensor info
> > >   * \param[out] info The camera sensor info
> > > --
> > > 2.29.2
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund