[libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose a DelayedControls interface

[Niklas Söderlund]

Hi Naushir,

Thanks for your feedback.

On 2020-10-28 11:51:13 +0000, Naushir Patuck wrote:
> Hi Niklas,
> 
> Thank you for the changes.  I will go through all the patches with review
> comment in due course, but wanted to raise a critical point below:
> 
> On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <
> niklas.soderlund at ragnatech.se> 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 },
> > +               };
> >
> 
> This will break the OV5647 sensor usage with Raspberry Pi without
> referring back to a sensor database .  OV5647 has a delay of 2 for gain and
> exposure, so will not work with the above hard-coded settings.

I understand your point, and we really should get started with a 
solution on how to get sensor specific data into CameraSensor database.

But with this series there is no problem for the Raspberry Pi pipeline 
as it does not deal with controls on the CameraSensor but the V4L2 video 
device. Therefor the RPi pipeline creates a private instance of 
DelayedControls witch delays still are initialized by the RPi IPA, just 
as things worked before using StaggerdCtrls.

When developing this I ran DelayedControls and StaggerdCtrls in parallel 
on the RPi and the output of both matched perfectly frame by frame.

> 
> In a more general sense, I wonder if initialising the DelayedControl here
> is the right thing to do?  I have some changes waiting to be submitted for
> review that add framerate control to the Raspberry Pi stack.  In this
> change, I've added a VBLANK control to the staggered writer.  There are
> also some changes to control very long exposures on the imx477 that go
> through the staggered writer.  In these cases, I would have to add values
> to the map above, but other pipeline handlers would never need to use
> them.  This may be a bit wasteful, I don't know...  Would it make more
> sense for the pipeline handlers to initialise the DelayedControls as they
> need to?

First, we are still thinking about how VBLANK and other controls that 
can modify the frame length are to be modelled in the CameraSensor. If 
you have any ideas or tips in this area I would greatly appreciate them.

I really think CameraSensor is the location to initialising the delays, 
this separates the sensor from the pipeline. I think this is important 
as otherwise each pipeline will have have a list of sensors it knows 
about and will not function popery with different ones. By "forcing" 
pipelines to learn about the sensor used thru CameraSensor interface we 
ensure that we can add more sensors in the future without having to 
update all pipelines and/or IPAs. This is even more important if we 
consider closed-source IPAs as we can't just add a new sensor with 
timings recompile.

Yet again as the Raspberry Pi pipeline is video device centric it can't 
really use the CameraSensor to deal with controls and must instead 
create and configure its local version of DelayedControls. I still think 
in the future such pipelines can benefit from a sensor database in 
CameraSensor as it should be possible to query the datbase and use the 
delays reported while still creating a private local DelayedControls.

Does this relive your worries bout this series?

> 
> Regards,
> Naush
> 
> 
> 
> > +
> > +               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.1
> >
> >

-- 
Regards,
Niklas Söderlund