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

[Laurent Pinchart]

On Sat, Dec 05, 2020 at 03:39:12AM +0200, Laurent Pinchart wrote:
> Hi Jacopo and Niklas,
> 
> On Fri, Nov 27, 2020 at 10:56:08AM +0100, Jacopo Mondi wrote:
> > On Mon, Nov 23, 2020 at 11:12:31PM +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();
> 
> s/delayedContols/delayedControls/
> 
> > > +
> > >  	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'm debated.
> > 
> > As I see this, the current interface will have to evolve, on top of
> > this series. I think pipeline handler will not have to get a
> > 'DelayedControls' object and interface with it, but rather the
> > CameraSensor will use it internally for either:
> > - all controls marked as delayed in the sensor database
> > - controls initialized as delayed by the pipeline handler (which won't
> >   be sensor-agnostic anymore)
> > 
> > The current implementation aims to be a drop-in replacement for
> > StaggeredCtrl so I'm more than fine with the proposed enanchement
> > being performed on top. At the same time as you know this specific
> > bits will break ov5647 capture on RPi, and I don't think that's right.
> > 
> > As a possible plan I would propose:
> > - replace staggered ctrls with this series and allow pipeline handler
> >   to pass in delays to maintain 1-to-1 compatibility with the existing
> > - grow the sensor database and the CameraSensor interface to remove
> >   direct pipeline->DelayedControl interactions on top
> > 
> > Am I missing a valid use case for pipeline handler having to deal with
> > DelayedControls directly ?
> 
> I'm also wondering about the same. The main use case is indeed the
> CameraSensor class. I wouldn't rule out other potential users, which
> would be nicely served by the fact that the DelayedControls class can be
> instantiated separately from CameraSensor, but I'm not sure what those
> would be.
> 
> In order to make the pipeline handler's life as easy as possible, I'd
> like to completely hide the DelayedControl instance in the CameraSensor
> class. This assumes that there would be no reason for a pipeline handler
> to deal with internals of delayed controls, ever, which may not always
> be true (maybe we'll need hacks in some cases, or maybe there are some
> clever use cases I can't think about now, but I can't see how those
> would be pipeline handler-specific, so implementing them in CameraSensor
> would make most sense to me at this point).
> 
> How about leaving this patch out for now, and moving delayed controls
> handling in CameraSensor in a second step ? Nobody uses this function in
> this series :-)

I was wrong about that, it's used in the rkisp1 pipeline handler (and I
assume by the IPU3 pipeline handler, on top of this series). Still, I
think we could duplicate this code in those two pipeline handlers, and
then move it to the CameraSensor class.

> > > +
> > > +		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

-- 
Regards,

Laurent Pinchart