[libcamera-devel] [PATCH v4 26/37] libcamera: pipeline, ipa: raspberrypi: Use new data definition
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Dec 5 15:41:08 CET 2020
Hi Paul,
On Sat, Dec 05, 2020 at 12:49:12PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Nov 26, 2020 at 05:41:23PM +0200, Laurent Pinchart wrote:
> > 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.
Thank you.
> > > } /* 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?
If you think it's a good idea, I'll agree with you :-)
> > > {
> > > 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.
Ah yes I forgot about that. I think we need to guarantee ControlLists
have a ControlInfoMap to be serialized.
> > > }
> > >
> > > ~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.
It's certainly worth investigating. Not a blocker for merging the
series, but should be fixed earlier than latter if possible. Could you
please record it in a \todo comment if it doesn't get address before
merging ?
> > > + 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.
It's also something we could address on top, it would be good to think
about best practice rules for IPA protocol design, and implementing them
for RPi as an example.
> > 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.
RPi doesn't care right now because there's no isolation, so the original
file descriptor number doesn't need to be preserved separately. Also
fixable on top (with a \todo) if needed.
> > > .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()
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list