[libcamera-devel] [PATCH v4 26/37] libcamera: pipeline, ipa: raspberrypi: Use new data definition
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Sat Dec 5 04:49:12 CET 2020
Hi Laurent,
Thank you for the review.
On Thu, Nov 26, 2020 at 05:41:23PM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Fri, Nov 06, 2020 at 07:36:56PM +0900, Paul Elder wrote:
> > Now that we can generate custom functions and data structures with mojo,
> > switch the raspberrypi pipeline handler and IPA to use the custom data
> > structures as defined in the mojom data definition file.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v4:
> > - rename Controls to controls
> > - use the renamed header (raspberrypi_ipa_interface.h)
> >
> > Changes in v3:
> > - use ipa::rpi namespace
> > - rebase on the RPi namespace patch series newly merged into master
> >
> > Changes in v2:
> > - rebased on "libcamera: pipeline: ipa: raspberrypi: Rework drop frame
> > signalling"
> > - use newly customized RPi IPA interface
> > ---
> > include/libcamera/ipa/raspberrypi.h | 40 ++--
> > src/ipa/raspberrypi/raspberrypi.cpp | 160 ++++++---------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 193 +++++++++---------
> > 3 files changed, 183 insertions(+), 210 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index df30b4a2..259f943e 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -28,24 +28,28 @@ enum BufferMask {
> > static constexpr unsigned int MaxLsGridSize = 32 << 10;
> >
> > /* List of controls handled by the Raspberry Pi IPA */
> > -static const ControlInfoMap Controls = {
> > - { &controls::AeEnable, ControlInfo(false, true) },
> > - { &controls::ExposureTime, ControlInfo(0, 999999) },
> > - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > - { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > - { &controls::AwbEnable, ControlInfo(false, true) },
> > - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > - { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > - { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > - { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > -};
> > +static ControlInfoMap controls;
> > +
> > +inline void initializeRPiControls()
> > +{
> > + controls = {
> > + { &controls::AeEnable, ControlInfo(false, true) },
> > + { &controls::ExposureTime, ControlInfo(0, 999999) },
> > + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > + { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > + { &controls::AwbEnable, ControlInfo(false, true) },
> > + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > + { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > + { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > + { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>
> -16,+15. Looks like a mishandled rebase perhaps ? Could you double-check
> the values to make sure they're all fine ?
>
> > + };
> > +}
>
> Given that the IPA doesn't actually need this, I'd like to move this map
> to the pipeline handler. However, the generate serdes code uses this.
The generated serdes code uses it in the event that ControlList doesn't
contain the ControlInfoMap. If ControlList will always have a
ControlInfoMap, or we can skip serdes if it doesn't, then yes, we can
remove the map from here.
> Can it be dropped ? Require all pipeline handlers to define a
> ControlInfoMap in a shared header isn't the right way forward. Maybe
> with your rework of the control serdes code, following our last
> discussion regarding the dependency between control lists and control
> info maps, this will not be needed anymore ?
Yeah. I'll look into to between v5 and v6.
> > } /* namespace RPi */
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f338ff8b..316da7bd 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -19,6 +19,7 @@
> > #include <libcamera/ipa/ipa_interface.h>
> > #include <libcamera/ipa/ipa_module_info.h>
> > #include <libcamera/ipa/raspberrypi.h>
> > +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> > #include <libcamera/request.h>
> > #include <libcamera/span.h>
> >
> > @@ -60,7 +61,7 @@ constexpr unsigned int DefaultExposureTime = 20000;
> >
> > LOG_DEFINE_CATEGORY(IPARPI)
> >
> > -class IPARPi : public IPAInterface
> > +class IPARPi : public ipa::rpi::IPARPiInterface
>
> I think I commented in a previous patch that ipa::rpi::Interface may be
> good enough, without a need to repeat the prefix, but if I haven't, now
> I've expressed it :-)
Well atm we don't require the interface definitions to declare a
namespace. If we do though, then we can drop the prefix, which would
also simplify parsing code :)
Should we require it then?
> > {
> > public:
> > IPARPi()
> > @@ -68,6 +69,7 @@ public:
> > frameCount_(0), checkCount_(0), mistrustCount_(0),
> > lsTable_(nullptr)
> > {
> > + RPi::initializeRPiControls();
>
> The IPA actually doesn't need this.
atm this is necessary to appease the serdes, unless ControlLists are
guaranteed to have a ControlInfoMap.
> > }
> >
> > ~IPARPi()
> > @@ -82,12 +84,14 @@ public:
> >
> > void configure(const CameraSensorInfo &sensorInfo,
> > const std::map<unsigned int, IPAStream> &streamConfig,
> > - const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > - const IPAOperationData &data,
> > - IPAOperationData *response) override;
> > + const std::map<unsigned int, ControlInfoMap> &entityControls,
>
> Ouch, that hurts, having to make a copy of the ControlInfoMap in the
> caller just for the purpose of adding it to this map. Is this something
> that can be fixed ?
Perhaps. We could make it so that all non-arithmetic members of
map/vector in function parameters are references. That should work fine,
right..?
I'll give it a shot between v5 and v6.
> > + const ipa::rpi::ConfigInput &data,
>
> It feels a bit weird passing some data through dedicated parameters, and
> some data in a miscellaneous config structure. I'd ask if we shouldn't
> move everything to ipa::rpi::ConfigInput, but I fear we'll then have
> more of the above copy issue ?
Well, that was just because for now I'm copying the old raspberrypi IPA
interface as-is.
> This seems to be an annoying limitation of the IPA IPC framework. If
> this happens to be simple to fix (which I doubt) it would be nice to
> address the issue, but I assume we should do so on top. Still, if you
> already have ideas on how this could be handled, we can discuss it.
>
> > + ipa::rpi::ConfigOutput *response) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > - void processEvent(const IPAOperationData &event) override;
> > + void signalStatReady(const uint32_t bufferId) override;
> > + void signalQueueRequest(const ControlList &controls) override;
> > + void signalIspPrepare(const ipa::rpi::IspPreparePayload &data) override;
>
> I like this :-)
>
> >
> > private:
> > void setMode(const CameraSensorInfo &sensorInfo);
> > @@ -144,6 +148,11 @@ private:
> >
> > /* LS table allocation passed in from the pipeline handler. */
> > FileDescriptor lsTableHandle_;
> > + /*
> > + * LS table allocation passed in from the pipeline handler,
> > + * in the context of the pipeline handler.
> > + */
> > + int32_t lsTableHandlePH_;
> > void *lsTable_;
> > };
> >
> > @@ -193,15 +202,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> >
> > void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > - const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > - const IPAOperationData &ipaConfig,
> > - IPAOperationData *result)
> > + const std::map<unsigned int, ControlInfoMap> &entityControls,
> > + const ipa::rpi::ConfigInput &ipaConfig,
> > + ipa::rpi::ConfigOutput *result)
> > {
> > if (entityControls.empty())
> > return;
> >
> > - result->operation = 0;
> > -
> > unicamCtrls_ = entityControls.at(0);
> > ispCtrls_ = entityControls.at(1);
> >
> > @@ -225,32 +232,28 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > helper_->GetDelays(exposureDelay, gainDelay);
> > sensorMetadata = helper_->SensorEmbeddedDataPresent();
> >
> > - result->data.push_back(gainDelay);
> > - result->data.push_back(exposureDelay);
> > - result->data.push_back(sensorMetadata);
> > -
> > - result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> > + result->op_ |= ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE;
> > + result->staggeredWriteResult_.gainDelay_ = gainDelay;
> > + result->staggeredWriteResult_.exposureDelay_ = exposureDelay;
> > + result->staggeredWriteResult_.sensorMetadata_ = sensorMetadata;
> > }
> >
> > /* Re-assemble camera mode using the sensor info. */
> > setMode(sensorInfo);
> >
> > - /*
> > - * The ipaConfig.data always gives us the user transform first. Note that
> > - * this will always make the LS table pointer (if present) element 1.
> > - */
> > - mode_.transform = static_cast<libcamera::Transform>(ipaConfig.data[0]);
> > + mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform_);
>
> So much nicer :-)
>
> >
> > /* Store the lens shading table pointer and handle if available. */
> > - if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
> > + if (ipaConfig.op_ & ipa::rpi::RPI_IPA_CONFIG_LS_TABLE) {
> > /* Remove any previous table, if there was one. */
> > if (lsTable_) {
> > munmap(lsTable_, RPi::MaxLsGridSize);
> > lsTable_ = nullptr;
> > }
> >
> > - /* Map the LS table buffer into user space (now element 1). */
> > - lsTableHandle_ = FileDescriptor(ipaConfig.data[1]);
> > + /* Map the LS table buffer into user space. */
> > + lsTableHandle_ = FileDescriptor(ipaConfig.lsTableHandle_);
> > + lsTableHandlePH_ = ipaConfig.lsTableHandleStatic_;
> > if (lsTableHandle_.isValid()) {
> > lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> > MAP_SHARED, lsTableHandle_.fd(), 0);
> > @@ -272,18 +275,15 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > */
> > frameCount_ = 0;
> > checkCount_ = 0;
> > - unsigned int dropFrame = 0;
> > + result->op_ |= ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES;
> > if (controllerInit_) {
> > - dropFrame = helper_->HideFramesModeSwitch();
> > + result->dropFrameCount_ = helper_->HideFramesModeSwitch();
> > mistrustCount_ = helper_->MistrustFramesModeSwitch();
> > } else {
> > - dropFrame = helper_->HideFramesStartup();
> > + result->dropFrameCount_ = helper_->HideFramesStartup();
> > mistrustCount_ = helper_->MistrustFramesStartup();
> > }
> >
> > - result->data.push_back(dropFrame);
> > - result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> > -
> > /* These zero values mean not program anything (unless overwritten). */
> > struct AgcStatus agcStatus;
> > agcStatus.shutter_time = 0.0;
> > @@ -308,9 +308,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > ControlList ctrls(unicamCtrls_);
> > applyAGC(&agcStatus, ctrls);
> > - result->controls.push_back(ctrls);
> >
> > - result->operation |= RPi::IPA_CONFIG_SENSOR;
> > + result->op_ |= ipa::rpi::RPI_IPA_CONFIG_SENSOR;
> > + result->controls_ = ctrls;
>
> std::move() should be more efficient.
>
> > }
> >
> > lastMode_ = mode_;
> > @@ -348,56 +348,38 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
> > }
> > }
> >
> > -void IPARPi::processEvent(const IPAOperationData &event)
> > +void IPARPi::signalStatReady(const uint32_t bufferId)
> > {
> > - switch (event.operation) {
> > - case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
> > - unsigned int bufferId = event.data[0];
> > -
> > - if (++checkCount_ != frameCount_) /* assert here? */
> > - LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > - if (frameCount_ > mistrustCount_)
> > - processStats(bufferId);
> > -
> > - reportMetadata();
> > -
> > - IPAOperationData op;
> > - op.operation = RPi::IPA_ACTION_STATS_METADATA_COMPLETE;
> > - op.data = { bufferId & RPi::BufferMask::ID };
> > - op.controls = { libcameraMetadata_ };
> > - queueFrameAction.emit(0, op);
> > - break;
> > - }
> > + if (++checkCount_ != frameCount_) /* assert here? */
> > + LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > + if (frameCount_ > mistrustCount_)
> > + processStats(bufferId);
> >
> > - case RPi::IPA_EVENT_SIGNAL_ISP_PREPARE: {
> > - unsigned int embeddedbufferId = event.data[0];
> > - unsigned int bayerbufferId = event.data[1];
> > + reportMetadata();
> >
> > - /*
> > - * At start-up, or after a mode-switch, we may want to
> > - * avoid running the control algos for a few frames in case
> > - * they are "unreliable".
> > - */
> > - prepareISP(embeddedbufferId);
> > - frameCount_++;
> > -
> > - /* Ready to push the input buffer into the ISP. */
> > - IPAOperationData op;
> > - op.operation = RPi::IPA_ACTION_RUN_ISP;
> > - op.data = { bayerbufferId & RPi::BufferMask::ID };
> > - queueFrameAction.emit(0, op);
> > - break;
> > - }
> > + statsMetadataComplete.emit(bufferId & RPi::BufferMask::ID, libcameraMetadata_);
> > +}
> >
> > - case RPi::IPA_EVENT_QUEUE_REQUEST: {
> > - queueRequest(event.controls[0]);
> > - break;
> > - }
> > +void IPARPi::signalQueueRequest(const ControlList &controls)
> > +{
> > + queueRequest(controls);
> > +}
> >
> > - default:
> > - LOG(IPARPI, Error) << "Unknown event " << event.operation;
> > - break;
> > - }
> > +void IPARPi::signalIspPrepare(const ipa::rpi::IspPreparePayload &data)
> > +{
> > + unsigned int embeddedbufferId = data.embeddedbufferId_;
> > + unsigned int bayerbufferId = data.bayerbufferId_;
> > +
> > + /*
> > + * At start-up, or after a mode-switch, we may want to
> > + * avoid running the control algos for a few frames in case
> > + * they are "unreliable".
> > + */
> > + prepareISP(embeddedbufferId);
> > + frameCount_++;
> > +
> > + /* Ready to push the input buffer into the ISP. */
> > + runIsp.emit(bayerbufferId & RPi::BufferMask::ID);
> > }
> >
> > void IPARPi::reportMetadata()
> > @@ -498,6 +480,8 @@ void IPARPi::queueRequest(const ControlList &controls)
> > /* Clear the return metadata buffer. */
> > libcameraMetadata_.clear();
> >
> > + LOG(IPARPI, Info) << "Request ctrl length: " << controls.size();
> > +
>
> Debugging leftover ?
>
> > for (auto const &ctrl : controls) {
> > LOG(IPARPI, Info) << "Request ctrl: "
> > << controls::controls.at(ctrl.first)->name()
> > @@ -715,10 +699,7 @@ void IPARPi::queueRequest(const ControlList &controls)
> >
> > void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > {
> > - IPAOperationData op;
> > - op.operation = RPi::IPA_ACTION_EMBEDDED_COMPLETE;
> > - op.data = { bufferId & RPi::BufferMask::ID };
> > - queueFrameAction.emit(0, op);
> > + embeddedComplete.emit(bufferId & RPi::BufferMask::ID);
> > }
> >
> > void IPARPi::prepareISP(unsigned int bufferId)
> > @@ -779,12 +760,8 @@ void IPARPi::prepareISP(unsigned int bufferId)
> > if (dpcStatus)
> > applyDPC(dpcStatus, ctrls);
> >
> > - if (!ctrls.empty()) {
> > - IPAOperationData op;
> > - op.operation = RPi::IPA_ACTION_V4L2_SET_ISP;
> > - op.controls.push_back(ctrls);
> > - queueFrameAction.emit(0, op);
> > - }
> > + if (!ctrls.empty())
> > + setIsp.emit(ctrls);
> > }
> > }
> >
> > @@ -840,10 +817,7 @@ void IPARPi::processStats(unsigned int bufferId)
> > ControlList ctrls(unicamCtrls_);
> > applyAGC(&agcStatus, ctrls);
> >
> > - IPAOperationData op;
> > - op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;
> > - op.controls.push_back(ctrls);
> > - queueFrameAction.emit(0, op);
> > + setStaggered.emit(ctrls);
> > }
> > }
> >
> > @@ -1073,7 +1047,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > .grid_width = w,
> > .grid_stride = w,
> > .grid_height = h,
> > - .dmabuf = lsTableHandle_.fd(),
> > + .dmabuf = lsTableHandlePH_,
>
> This feels like a bit of a hack. How about simplifying it, by dropping
> lsTableHandlePH_ here and filling the field in the pipeline handler
> instead ?
This is also what the raspberrypi IPA was doing before.
Paul
> > .ref_transform = 0,
> > .corner_sampled = 1,
> > .gain_format = GAIN_FORMAT_U4P10
> > @@ -1150,9 +1124,9 @@ const struct IPAModuleInfo ipaModuleInfo = {
> > "raspberrypi",
> > };
> >
> > -struct ipa_context *ipaCreate()
> > +IPAInterface *ipaCreate()
> > {
> > - return new IPAInterfaceWrapper(std::make_unique<IPARPi>());
> > + return new IPARPi();
> > }
> >
> > } /* extern "C" */
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0a60442c..0e9ff16d 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -16,7 +16,9 @@
> > #include <libcamera/control_ids.h>
> > #include <libcamera/file_descriptor.h>
> > #include <libcamera/formats.h>
> > +#include <libcamera/ipa/ipa_proxy_raspberrypi.h>
> > #include <libcamera/ipa/raspberrypi.h>
> > +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> > #include <libcamera/logging.h>
> > #include <libcamera/property_ids.h>
> > #include <libcamera/request.h>
> > @@ -144,7 +146,11 @@ public:
> > int loadIPA();
> > int configureIPA(const CameraConfiguration *config);
> >
> > - void queueFrameAction(unsigned int frame, const IPAOperationData &action);
> > + void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > + void runIsp(uint32_t bufferId);
> > + void embeddedComplete(uint32_t bufferId);
> > + void setIsp(const ControlList &controls);
> > + void setStaggered(const ControlList &controls);
> >
> > /* bufferComplete signal handlers. */
> > void unicamBufferDequeue(FrameBuffer *buffer);
> > @@ -156,6 +162,8 @@ public:
> > void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);
> > void handleState();
> >
> > + std::unique_ptr<ipa::rpi::IPAProxyRPi> ipa_;
> > +
> > CameraSensor *sensor_;
> > /* Array of Unicam and ISP device streams and associated buffers/streams. */
> > RPi::Device<Unicam, 2> unicam_;
> > @@ -448,6 +456,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> > : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> > {
> > + RPi::initializeRPiControls();
> > }
> >
> > CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> > @@ -936,7 +945,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > }
> >
> > /* Register the controls that the Raspberry Pi IPA can handle. */
> > - data->controlInfo_ = RPi::Controls;
> > + data->controlInfo_ = RPi::controls;
> > /* Initialize the camera properties. */
> > data->properties_ = data->sensor_->properties();
> >
> > @@ -1114,11 +1123,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> >
> > int RPiCameraData::loadIPA()
> > {
> > - ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> > + ipa_ = IPAManager::createIPA<ipa::rpi::IPAProxyRPi>(pipe_, 1, 1);
> > +
> > if (!ipa_)
> > return -ENOENT;
> >
> > - ipa_->queueFrameAction.connect(this, &RPiCameraData::queueFrameAction);
> > + ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> > + ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> > + ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > + ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> > + ipa_->setStaggered.connect(this, &RPiCameraData::setStaggered);
> >
> > IPASettings settings{
> > .configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> > @@ -1134,8 +1148,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > static_cast<const RPiCameraConfiguration *>(config);
> >
> > std::map<unsigned int, IPAStream> streamConfig;
> > - std::map<unsigned int, const ControlInfoMap &> entityControls;
> > - IPAOperationData ipaConfig = {};
> > + std::map<unsigned int, ControlInfoMap> entityControls;
> > + ipa::rpi::ConfigInput ipaConfig;
> >
> > /* Get the device format to pass to the IPA. */
> > V4L2DeviceFormat sensorFormat;
> > @@ -1155,7 +1169,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
> >
> > /* Always send the user transform to the IPA. */
> > - ipaConfig.data = { static_cast<unsigned int>(config->transform) };
> > + ipaConfig.transform_ = static_cast<unsigned int>(config->transform);
> >
> > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> > if (!lsTable_.isValid()) {
> > @@ -1164,8 +1178,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > return -ENOMEM;
> >
> > /* Allow the IPA to mmap the LS table via the file descriptor. */
> > - ipaConfig.operation = RPi::IPA_CONFIG_LS_TABLE;
> > - ipaConfig.data.push_back(static_cast<unsigned int>(lsTable_.fd()));
> > + ipaConfig.op_ |= ipa::rpi::RPI_IPA_CONFIG_LS_TABLE;
> > + ipaConfig.lsTableHandle_ = lsTable_;
> > + ipaConfig.lsTableHandleStatic_ = lsTable_.fd();
> > }
> >
> > /* We store the CameraSensorInfo for digital zoom calculations. */
> > @@ -1176,34 +1191,35 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > }
> >
> > /* Ready the IPA - it must know about the sensor resolution. */
> > - IPAOperationData result;
> > + ipa::rpi::ConfigOutput result;
> >
> > ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > &result);
> >
> > - unsigned int resultIdx = 0;
> > - if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> > + if (result.op_ & ipa::rpi::RPI_IPA_CONFIG_STAGGERED_WRITE) {
> > /*
> > * Setup our staggered control writer with the sensor default
> > * gain and exposure delays.
> > */
> > if (!staggeredCtrl_) {
> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> > - sensorMetadata_ = result.data[resultIdx++];
> > + { { V4L2_CID_ANALOGUE_GAIN,
> > + result.staggeredWriteResult_.gainDelay_ },
> > + { V4L2_CID_EXPOSURE,
> > + result.staggeredWriteResult_.exposureDelay_ } });
> > + sensorMetadata_ = result.staggeredWriteResult_.sensorMetadata_;
> > }
> > }
> >
> > - if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> > - const ControlList &ctrls = result.controls[0];
> > + if (result.op_ & ipa::rpi::RPI_IPA_CONFIG_SENSOR) {
> > + const ControlList &ctrls = result.controls_;
> > if (!staggeredCtrl_.set(ctrls))
> > LOG(RPI, Error) << "V4L2 staggered set failed";
> > }
> >
> > - if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {
> > + if (result.op_ & ipa::rpi::RPI_IPA_CONFIG_DROP_FRAMES) {
> > /* Configure the number of dropped frames required on startup. */
> > - dropFrameCount_ = result.data[resultIdx++];
> > + dropFrameCount_ = result.dropFrameCount_;
> > }
> >
> > /*
> > @@ -1222,90 +1238,75 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > return 0;
> > }
> >
> > -void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,
> > - const IPAOperationData &action)
> > +void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> > {
> > - /*
> > - * The following actions can be handled when the pipeline handler is in
> > - * a stopped state.
> > - */
> > - switch (action.operation) {
> > - case RPi::IPA_ACTION_V4L2_SET_STAGGERED: {
> > - const ControlList &controls = action.controls[0];
> > - if (!staggeredCtrl_.set(controls))
> > - LOG(RPI, Error) << "V4L2 staggered set failed";
> > - goto done;
> > - }
> > + if (state_ == State::Stopped)
> > + handleState();
> >
> > - case RPi::IPA_ACTION_V4L2_SET_ISP: {
> > - ControlList controls = action.controls[0];
> > - isp_[Isp::Input].dev()->setControls(&controls);
> > - goto done;
> > - }
> > - }
> > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >
> > - if (state_ == State::Stopped)
> > - goto done;
> > + handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> > +
> > + /* Fill the Request metadata buffer with what the IPA has provided */
> > + Request *request = requestQueue_.front();
> > + request->metadata() = std::move(controls);
> >
> > /*
> > - * The following actions must not be handled when the pipeline handler
> > - * is in a stopped state.
> > + * Also update the ScalerCrop in the metadata with what we actually
> > + * used. But we must first rescale that from ISP (camera mode) pixels
> > + * back into sensor native pixels.
> > + *
> > + * Sending this information on every frame may be helpful.
> > */
> > - switch (action.operation) {
> > - case RPi::IPA_ACTION_STATS_METADATA_COMPLETE: {
> > - unsigned int bufferId = action.data[0];
> > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> > + if (updateScalerCrop_) {
> > + updateScalerCrop_ = false;
> > + scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> > + sensorInfo_.outputSize);
> > + scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
> > + }
> > + request->metadata().set(controls::ScalerCrop, scalerCrop_);
> >
> > - handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> > + state_ = State::IpaComplete;
> >
> > - /* Fill the Request metadata buffer with what the IPA has provided */
> > - Request *request = requestQueue_.front();
> > - request->metadata() = std::move(action.controls[0]);
> > + handleState();
> > +}
> >
> > - /*
> > - * Also update the ScalerCrop in the metadata with what we actually
> > - * used. But we must first rescale that from ISP (camera mode) pixels
> > - * back into sensor native pixels.
> > - *
> > - * Sending this information on every frame may be helpful.
> > - */
> > - if (updateScalerCrop_) {
> > - updateScalerCrop_ = false;
> > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> > - sensorInfo_.outputSize);
> > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
> > - }
> > - request->metadata().set(controls::ScalerCrop, scalerCrop_);
> > +void RPiCameraData::runIsp(uint32_t bufferId)
> > +{
> > + if (state_ == State::Stopped)
> > + handleState();
> >
> > - state_ = State::IpaComplete;
> > - break;
> > - }
> > + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >
> > - case RPi::IPA_ACTION_EMBEDDED_COMPLETE: {
> > - unsigned int bufferId = action.data[0];
> > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > - break;
> > - }
> > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> > + << ", timestamp: " << buffer->metadata().timestamp;
> >
> > - case RPi::IPA_ACTION_RUN_ISP: {
> > - unsigned int bufferId = action.data[0];
> > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> > + isp_[Isp::Input].queueBuffer(buffer);
> > + ispOutputCount_ = 0;
> > + handleState();
> > +}
> >
> > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> > - << ", timestamp: " << buffer->metadata().timestamp;
> > +void RPiCameraData::embeddedComplete(uint32_t bufferId)
> > +{
> > + if (state_ == State::Stopped)
> > + handleState();
> >
> > - isp_[Isp::Input].queueBuffer(buffer);
> > - ispOutputCount_ = 0;
> > - break;
> > - }
> > + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > + handleState();
> > +}
> >
> > - default:
> > - LOG(RPI, Error) << "Unknown action " << action.operation;
> > - break;
> > - }
> > +void RPiCameraData::setIsp(const ControlList &controls)
> > +{
> > + ControlList ctrls = controls;
> > + isp_[Isp::Input].dev()->setControls(&ctrls);
> > + handleState();
> > +}
> >
> > -done:
> > +void RPiCameraData::setStaggered(const ControlList &controls)
> > +{
> > + if (!staggeredCtrl_.set(controls))
> > + LOG(RPI, Error) << "V4L2 staggered set failed";
> > handleState();
> > }
> >
> > @@ -1405,10 +1406,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > * application until after the IPA signals so.
> > */
> > if (stream == &isp_[Isp::Stats]) {
> > - IPAOperationData op;
> > - op.operation = RPi::IPA_EVENT_SIGNAL_STAT_READY;
> > - op.data = { RPi::BufferMask::STATS | static_cast<unsigned int>(index) };
> > - ipa_->processEvent(op);
> > + ipa_->signalStatReady(RPi::BufferMask::STATS | static_cast<unsigned int>(index));
> > } else {
> > /* Any other ISP output can be handed back to the application now. */
> > handleStreamBuffer(buffer, stream);
> > @@ -1581,7 +1579,6 @@ void RPiCameraData::checkRequestCompleted()
> > void RPiCameraData::tryRunPipeline()
> > {
> > FrameBuffer *bayerBuffer, *embeddedBuffer;
> > - IPAOperationData op;
> >
> > /* If any of our request or buffer queues are empty, we cannot proceed. */
> > if (state_ != State::Idle || requestQueue_.empty() ||
> > @@ -1661,9 +1658,7 @@ void RPiCameraData::tryRunPipeline()
> > * queue the ISP output buffer listed in the request to start the HW
> > * pipeline.
> > */
> > - op.operation = RPi::IPA_EVENT_QUEUE_REQUEST;
> > - op.controls = { request->controls() };
> > - ipa_->processEvent(op);
> > + ipa_->signalQueueRequest(request->controls());
> >
> > /* Ready to use the buffers, pop them off the queue. */
> > bayerQueue_.pop();
> > @@ -1679,10 +1674,10 @@ void RPiCameraData::tryRunPipeline()
> > << " Bayer buffer id: " << bayerId
> > << " Embedded buffer id: " << embeddedId;
> >
> > - op.operation = RPi::IPA_EVENT_SIGNAL_ISP_PREPARE;
> > - op.data = { RPi::BufferMask::EMBEDDED_DATA | embeddedId,
> > - RPi::BufferMask::BAYER_DATA | bayerId };
> > - ipa_->processEvent(op);
> > + ipa::rpi::IspPreparePayload ispPrepare;
> > + ispPrepare.embeddedbufferId_ = RPi::BufferMask::EMBEDDED_DATA | embeddedId;
> > + ispPrepare.bayerbufferId_ = RPi::BufferMask::BAYER_DATA | bayerId;
> > + ipa_->signalIspPrepare(ispPrepare);
> > }
> >
> > void RPiCameraData::tryFlushQueues()
More information about the libcamera-devel
mailing list