[libcamera-devel] [PATCH v2 07/11] libcamera: ipu3: Attach to an IPA and allow it to set sensor controls
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Feb 3 15:35:44 CET 2021
Hi Jacopo and Laurent,
Thanks for your feedback.
On 2021-01-10 18:46:46 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Thu, Jan 07, 2021 at 01:40:50PM +0100, Jacopo Mondi wrote:
> > On Tue, Dec 29, 2020 at 05:03:14PM +0100, Niklas Söderlund wrote:
> > > Attach to the IPA and allow it to push V4L2 controls that applies on the
>
> s/applies on/apply to/
>
> > > camera sensor. The IPA is not fully integrated in the pipeline as
> > > statistics and parameters buffers are not yet allocated, processed by
> > > the IPA nor queued to the hardware.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > * Changes since v1
> > > - Rewrite to not use CameraSensor.
> > > - Fix mistake where configuration sen to IPA was overwritten.
> > > - Check that IPA configuration was successful before starting.
> > > - Update commit message.
> > > ---
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 103 +++++++++++++++++++++++++++
> > > 1 file changed, 103 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index a87ce8f3378ba2fe..95f1b75dc8be5d40 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -14,11 +14,14 @@
> > > #include <libcamera/camera.h>
> > > #include <libcamera/control_ids.h>
> > > #include <libcamera/formats.h>
> > > +#include <libcamera/ipa/ipu3.h>
> > > #include <libcamera/request.h>
> > > #include <libcamera/stream.h>
> > >
> > > #include "libcamera/internal/camera_sensor.h"
> > > +#include "libcamera/internal/delayed_controls.h"
> > > #include "libcamera/internal/device_enumerator.h"
> > > +#include "libcamera/internal/ipa_manager.h"
> > > #include "libcamera/internal/log.h"
> > > #include "libcamera/internal/media_device.h"
> > > #include "libcamera/internal/pipeline_handler.h"
> > > @@ -53,6 +56,8 @@ public:
> > > {
> > > }
> > >
> > > + int loadIPA();
> > > +
> > > void imguOutputBufferReady(FrameBuffer *buffer);
> > > void cio2BufferReady(FrameBuffer *buffer);
> > >
> > > @@ -62,6 +67,11 @@ public:
> > > Stream outStream_;
> > > Stream vfStream_;
> > > Stream rawStream_;
> > > +
> > > + std::unique_ptr<DelayedControls> delayedCtrls_;
> > > +
> > > +private:
> > > + void actOnIpa(unsigned int id, const IPAOperationData &op);
> > > };
> > >
> > > class IPU3CameraConfiguration : public CameraConfiguration
> > > @@ -590,6 +600,13 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
> > > IPU3CameraData *data = cameraData(camera);
> > > CIO2Device *cio2 = &data->cio2_;
> > > ImgUDevice *imgu = data->imgu_;
> > > +
> > > + CameraSensorInfo sensorInfo = {};
> > > + std::map<unsigned int, IPAStream> streamConfig;
> > > + std::map<unsigned int, const ControlInfoMap &> entityControls;
> > > + IPAOperationData ipaConfig;
> > > + IPAOperationData result = {};
> >
> > You could s/= {}/{}
> > but it's minor
> >
I could but I find it harder to read.
> > > +
> > > int ret;
> > >
> > > /* Allocate buffers for internal pipeline usage. */
> > > @@ -597,6 +614,11 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
> > > if (ret)
> > > return ret;
> > >
> > > + IPAOperationData ipaData = {};
> > > + ret = data->ipa_->start(ipaData, nullptr);
> > > + if (ret)
> > > + goto error;
> > > +
> > > /*
> > > * Start the ImgU video devices, buffers will be queued to the
> > > * ImgU output and viewfinder when requests will be queued.
> > > @@ -612,9 +634,40 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
> > > goto error;
> > > }
> > >
> > > + /* Inform IPA of stream configuration and sensor controls. */
> > > + ret = data->cio2_.sensor()->sensorInfo(&sensorInfo);
> > > + if (ret) {
> > > + /* \todo Turn to hard failure once sensors info is mandatory. */
> >
> > we're rather working in the direction of making SensorInfo never fail.
> > I think you can drop this todo and hard fail here as if you have a
> > failure, it's because something bad happened when reading controls or
> > selection targets.
>
> Agreed.
I thinks this shows the age of this patch :-)
This TODO was recorded as the call failed at the time of patch creation.
Now the calls succeeds and this should indeed be turned into a hard
fail. Will do so in next version.
>
> > > + LOG(IPU3, Warning) << "Camera sensor information not available";
> > > + sensorInfo = {};
> > > + ret = 0;
> > > + }
> > > +
> > > + streamConfig[0] = {
> > > + .pixelFormat = data->outStream_.configuration().pixelFormat,
> > > + .size = data->outStream_.configuration().size,
> > > + };
> > > + streamConfig[1] = {
> > > + .pixelFormat = data->vfStream_.configuration().pixelFormat,
> > > + .size = data->vfStream_.configuration().size,
> > > + };
> >
> > Is this ok if one of the two streams (likely vfStream_) is not
> > assigned ?
>
> Based on our current IPA interface, I'd say only active streams should
> be reported here. We're however moving to a per-pipeline handler IPA
> protocol, and it would be fine having a hardcoded array of two streams,
> as long, of course, as there's enough information for the IPA to tell
> which streams are active (invalid pixel format for instance). Obviously
> a hardcoded array isn't required with the custom IPA protocol, we can
> still device to pass the active streams only in that case.
>
> The StreamConfiguration() stored in a Stream is initialized with
> appropriate defaults, but if we configure the camera with two streams,
> and then again with a single stream, I think we'll have stale values.
The viewfinder is always configured in configue(), even if it's not
needed by the configuration we are asked to do, so it will never be
stale. It's also always started in start(). The only different between
if the configuration contains viewifinder or not is that we won't allow
applications to queue buffers to it if its not.
I'm open to any solution here that moves us forward as quickly as
possible. But given that the stream configuration is not consumed by the
IPA and is only expressed here to satisfy our current IPA interface that
that we know will be replaced soon I would prefers to either keep this
as-is or remove this and simply pass an empty map to ipa
configuration(). Creating something complex here to only report active
streams seems a bit pointless at this point.
>
> > > +
> > > + entityControls.emplace(0, data->cio2_.sensor()->controls());
> > > +
> > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > + ipaConfig, &result);
> > > +
> > > + if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) ||
> >
> > This seems like it can't happen at the moment, but it's ok
>
> Yes, I'm not sure what the idea behind the check is.
>
> > > + (result.data.size() != 1) || (result.data.at(0) != 1)) {
> >
> > I think the fact that returning 1 in data[0] means success should be
> > documented in the IPA protocol. Ah wait, we don't have one.
>
> RPi checks
>
> if (result.operation & RPi::IPA_CONFIG_FAILED)
>
> and we could do something similar for now.
Wops, All of this result check is left from a experiment I did. This
should indeed be removed. Thanks for spotting it.
>
> > > + LOG(IPU3, Warning) << "Failed to configure IPA";
> > > + ret = -EINVAL;
> > > + goto error;
> > > + }
> > > +
> > > return 0;
> > >
> > > error:
> > > + data->ipa_->stop();
> > > freeBuffers(camera);
> > > LOG(IPU3, Error) << "Failed to start camera " << camera->id();
> > >
> > > @@ -631,6 +684,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > > if (ret)
> > > LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> > >
> > > + data->ipa_->stop();
> > > +
> > > freeBuffers(camera);
> > > }
> > >
> > > @@ -762,12 +817,32 @@ int PipelineHandlerIPU3::registerCameras()
> > > if (ret)
> > > continue;
> > >
> > > + ret = data->loadIPA();
> > > + if (ret)
> > > + continue;
> > > +
> > > /* Initialize the camera properties. */
> > > data->properties_ = cio2->sensor()->properties();
> > >
> > > /* Initialze the camera controls. */
> > > data->controlInfo_ = IPU3Controls;
> > >
> > > + /*
> > > + * \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 },
> > > + };
> > > +
> > > + data->delayedCtrls_ =
> > > + std::make_unique<DelayedControls>(cio2->sensor()->device(),
> > > + delays);
> >
> > sigh
> >
> > we should get the sensor database in sooner than later and move this
> > to the camera sensor before the same patter spread in multiple
> > pipelines
>
> I can't disagree on this :-) (moving on the review of the corresponding
> patches next)
I too would like to get the database in place as soon as possible.
>
> > > + data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> > > + &DelayedControls::applyControls);
> > > +
> > > /**
> > > * \todo Dynamically assign ImgU and output devices to each
> > > * stream and camera; as of now, limit support to two cameras
> > > @@ -811,6 +886,34 @@ int PipelineHandlerIPU3::registerCameras()
> > > return numCameras ? 0 : -ENODEV;
> > > }
> > >
> > > +int IPU3CameraData::loadIPA()
> > > +{
> > > + ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> > > + if (!ipa_)
> > > + return -ENOENT;
> > > +
> > > + ipa_->queueFrameAction.connect(this, &IPU3CameraData::actOnIpa);
> > > +
> > > + ipa_->init(IPASettings{});
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,
> > > + const IPAOperationData &action)
>
> Maybe s/actOnIpa/queueFrameAction/ to match the other pipeline handlers
> ?
Sure.
>
> > > +{
> > > + switch (action.operation) {
> > > + case IPU3_IPA_ACTION_SET_SENSOR_CONTROLS: {
> > > + const ControlList &controls = action.controls[0];
> > > + delayedCtrls_->push(controls);
> > > + break;
> > > + }
> > > + default:
> > > + LOG(IPU3, Error) << "Unknown action " << action.operation;
> > > + break;
> > > + }
> > > +}
> > > +
> >
> > Overall, that's a good starting point
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Same,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > > /* -----------------------------------------------------------------------------
> > > * Buffer Ready slots
> > > */
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list