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

[Laurent Pinchart]

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 :-)

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