| Message ID | 20201229160318.77536-8-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thanks for the patch. On 29/12/2020 17:03, Niklas Söderlund wrote: > Attach to the IPA and allow it to push V4L2 controls that applies on the > 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@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 = {}; > + > 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. */ > + 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, > + }; > + > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > + > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, > + ipaConfig, &result); > + > + if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || > + (result.data.size() != 1) || (result.data.at(0) != 1)) { > + 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 }, > + }; I agree, the controls we want to have need to be parsed in some way. I am wondering what makes a control beeing a "delayedControl" and what is not ? Looking at the RPi pipeline and IPA, I can see the V4L2_CID_ANALOGUE_GAIN and V4L2_CID_EXPOSURE controls beeing delayed controls, but in the RPi IPA there is for example a call to the V4L2_CID_DIGITAL_GAIN control. Would we need a database to know which controls are effectively to be delayed ones, and when the sensor is configured, parse the controls available and add the corresponding ones to the map ? This is an open question :-). > + data->delayedCtrls_ = > + std::make_unique<DelayedControls>(cio2->sensor()->device(), > + delays); > + 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) > +{ > + 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; > + } > +} > + > /* ----------------------------------------------------------------------------- > * Buffer Ready slots > */ >
Hi Niklas, On 12/29/20 9:33 PM, Niklas Söderlund wrote: > Attach to the IPA and allow it to push V4L2 controls that applies on the > 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@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 = {}; > + > 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. */ > + 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, > + }; > + > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > + > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, > + ipaConfig, &result); > + > + if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || > + (result.data.size() != 1) || (result.data.at(0) != 1)) { > + 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 s/dealy/delay/ > + * a sensor database. For now use generic values taken from > + * the Raspberry Pi and listed as generic values. may be you meant : s/generic values/'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); > + 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) > +{ > + 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; > + } > +} > + > /* ----------------------------------------------------------------------------- > * Buffer Ready slots > */
Hi Niklas, 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 > 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@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 > + > 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. > + 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 ? > + > + 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 > + (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. > + 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 > + 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) > +{ > + 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@jmondi.org> Thanks j > /* ----------------------------------------------------------------------------- > * Buffer Ready slots > */ > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jean-Michel, On Wed, Dec 30, 2020 at 06:00:51PM +0100, Jean-Michel Hautbois wrote: > On 29/12/2020 17:03, Niklas Söderlund wrote: > > Attach to the IPA and allow it to push V4L2 controls that applies on the > > 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@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 = {}; > > + > > 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. */ > > + 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, > > + }; > > + > > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > > + > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, > > + ipaConfig, &result); > > + > > + if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || > > + (result.data.size() != 1) || (result.data.at(0) != 1)) { > > + 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 }, > > + }; > > I agree, the controls we want to have need to be parsed in some way. > I am wondering what makes a control beeing a "delayedControl" and what > is not ? > > Looking at the RPi pipeline and IPA, I can see the > V4L2_CID_ANALOGUE_GAIN and V4L2_CID_EXPOSURE controls beeing delayed > controls, but in the RPi IPA there is for example a call to the > V4L2_CID_DIGITAL_GAIN control. > > Would we need a database to know which controls are effectively to be > delayed ones, and when the sensor is configured, parse the controls > available and add the corresponding ones to the map ? > This is an open question :-). Yes, we do, and the DelayedControl instance should be moved to the CameraSensor class, with control handling becoming more transparent for the pipeline handlers. > > + data->delayedCtrls_ = > > + std::make_unique<DelayedControls>(cio2->sensor()->device(), > > + delays); > > + 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) > > +{ > > + 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; > > + } > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Buffer Ready slots > > */
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@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 > > > + > > 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. > > + 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. > > + > > + 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. > > + 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) > > + 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 ? > > +{ > > + 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@jmondi.org> Same, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > /* ----------------------------------------------------------------------------- > > * Buffer Ready slots > > */
Hi Niklas, Quick comment on my findings last friday, On 12/29/20 9:33 PM, Niklas Söderlund wrote: > Attach to the IPA and allow it to push V4L2 controls that applies on the > 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@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 = {}; > + > 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. */ > + 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, > + }; > + > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > + > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, > + ipaConfig, &result); > + > + if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || > + (result.data.size() != 1) || (result.data.at(0) != 1)) { > + LOG(IPU3, Warning) << "Failed to configure IPA"; > + ret = -EINVAL; > + goto error; Running your branch as is, `ipu3/ipa` triggered this path and spit out EBUSY with freeBuffers() below - so one of the quick things Jacopo noticed that you need to stop the ImgU and CIO2 here first. I had applied the change locally and it fixed it for me. I also wrote a associated patch(nothing major) for simplifying error paths with subject "ipu3: Simplify error path" - maybe it's interesting to you. > + } > + > 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); > + 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) > +{ > + 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; > + } > +} > + > /* ----------------------------------------------------------------------------- > * Buffer Ready slots > */
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@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@jmondi.org> > > Same, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > /* ----------------------------------------------------------------------------- > > > * Buffer Ready slots > > > */ > > -- > Regards, > > Laurent Pinchart
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 = {}; + 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. */ + 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, + }; + + entityControls.emplace(0, data->cio2_.sensor()->controls()); + + data->ipa_->configure(sensorInfo, streamConfig, entityControls, + ipaConfig, &result); + + if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || + (result.data.size() != 1) || (result.data.at(0) != 1)) { + 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); + 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) +{ + 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; + } +} + /* ----------------------------------------------------------------------------- * Buffer Ready slots */
Attach to the IPA and allow it to push V4L2 controls that applies on the 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@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(+)