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

[Naushir Patuck]

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.

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?

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201028/45c6845b/attachment.htm>