[libcamera-devel] [PATCH 6/9] libcamera: camera_sensor: Expose a DelayedControls interface
Naushir Patuck
naush at raspberrypi.com
Thu Oct 29 11:58:19 CET 2020
Hi Niklas,
Thank you for the clarification.
On Wed, 28 Oct 2020 at 13:26, Niklas Söderlund <
niklas.soderlund at ragnatech.se> wrote:
> 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.
>
Yes, I see that now. So there should be no problems for pipeline handlers
to allocate a DelayedControl for its own use? In that case, the above
should not be a problem. But you are correct, this does show the need to
get a central sensor database available soon.
>
> >
> > 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 could possibly share with you (privately for now, to avoid too much noise
on this mailing list) my changes to add VBLANK control, to discuss what I
have done?
>
> 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.
>
Yes, I agree with this. Just want to raise the point that some sensors
will have controls not available to others, e.g. imx477 long exposure
settings. So it would be good to have as much flexibility in the sensor
database to allow for these variations.
>
> Does this relive your worries bout this series?
>
Indeed it does. I will get to reviewing the full patchset shortly and send
over my comments.
Best regards,
Naush
>
> >
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201029/9b04e7fd/attachment-0001.htm>
More information about the libcamera-devel
mailing list