[libcamera-devel] [PATCH v6 09/11] libcamera: pipeline, ipa: raspberrypi: Use new data definition
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 4 03:50:09 CET 2021
Hi Paul,
Thank you for the patch.
On Thu, Dec 24, 2020 at 05:16:11PM +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 v6:
> - rebase on "pipeline: ipa: raspberrypi: Handle failures during IPA
> configuration"
> - move the enums and consts to the mojom file
> - use them with the namespace in the IPA and pipeline handler
> - no longer need to initialize the ControlInfoMap
> - fill in the LS table handle in the pipeline handler instead of passing
> it from the IPA (which was passed to the IPA from the pipeline handler
> in the first place)
> - use custom start()
> - no more postfix _ in generated struct fields
>
> Changes in v5:
> - minor fixes
> - use new struct names
>
> 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 | 11 -
> src/ipa/raspberrypi/raspberrypi.cpp | 181 ++++++-------
> .../pipeline/raspberrypi/raspberrypi.cpp | 244 +++++++++---------
> .../pipeline/raspberrypi/rpi_stream.cpp | 6 +-
> .../pipeline/raspberrypi/rpi_stream.h | 5 +-
> 5 files changed, 208 insertions(+), 239 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index df30b4a2..70ecc5a8 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -16,17 +16,6 @@ namespace libcamera {
>
> namespace RPi {
>
> -enum BufferMask {
> - ID = 0x00ffff,
> - STATS = 0x010000,
> - EMBEDDED_DATA = 0x020000,
> - BAYER_DATA = 0x040000,
> - EXTERNAL_BUFFER = 0x100000,
> -};
> -
> -/* Size of the LS grid allocation. */
> -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) },
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 67f47c11..5acc25a0 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -20,6 +20,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>
>
> @@ -59,7 +60,7 @@ constexpr unsigned int DefaultExposureTime = 20000;
>
> LOG_DEFINE_CATEGORY(IPARPI)
>
> -class IPARPi : public IPAInterface
> +class IPARPi : public ipa::rpi::IPARPiInterface
> {
> public:
> IPARPi()
> @@ -72,21 +73,24 @@ public:
> ~IPARPi()
> {
> if (lsTable_)
> - munmap(lsTable_, RPi::MaxLsGridSize);
> + munmap(lsTable_, ipa::rpi::MaxLsGridSize);
> }
>
> int init(const IPASettings &settings) override;
> - int start(const IPAOperationData &data, IPAOperationData *result) override;
> + void start(const ipa::rpi::StartControls &data,
> + ipa::rpi::StartControls *result, int *ret) override;
> void stop() override {}
>
> void configure(const CameraSensorInfo &sensorInfo,
> const std::map<unsigned int, IPAStream> &streamConfig,
> - const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> - const IPAOperationData &ipaConfig,
> - IPAOperationData *response) override;
> + const std::map<unsigned int, ControlInfoMap> &entityControls,
> + const ipa::rpi::ConfigInput &data,
> + 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::ISPConfig &data) override;
>
> private:
> void setMode(const CameraSensorInfo &sensorInfo);
> @@ -156,15 +160,17 @@ int IPARPi::init(const IPASettings &settings)
> return 0;
> }
>
> -int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
> +void IPARPi::start(const ipa::rpi::StartControls &data,
> + ipa::rpi::StartControls *result, int *ret)
> {
> RPiController::Metadata metadata;
>
> + ASSERT(ret);
> ASSERT(result);
> - result->operation = 0;
> - if (data.operation & RPi::IPA_CONFIG_STARTUP) {
> + result->hasControls = false;
> + if (data.hasControls) {
> /* We have been given some controls to action before start. */
> - queueRequest(data.controls[0]);
> + queueRequest(data.controls);
> }
>
> controller_.SwitchMode(mode_, &metadata);
> @@ -179,8 +185,8 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
> if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> ControlList ctrls(sensorCtrls_);
> applyAGC(&agcStatus, ctrls);
> - result->controls.emplace_back(ctrls);
> - result->operation |= RPi::IPA_CONFIG_SENSOR;
> + result->controls = std::move(ctrls);
> + result->hasControls = true;
> }
>
> /*
> @@ -227,12 +233,11 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
> mistrustCount_ = helper_->MistrustFramesModeSwitch();
> }
>
> - result->data.push_back(dropFrame);
> - result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> + result->dropFrameCount = dropFrame;
>
> firstStart_ = false;
>
> - return 0;
> + *ret = 0;
As start() seems to never fail, you could drop the ret parameter.
> }
>
> void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> @@ -275,30 +280,30 @@ 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.size() != 2) {
> LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> - result->operation = RPi::IPA_CONFIG_FAILED;
> + result->op = ipa::rpi::ConfigFailed;
> return;
> }
>
> - result->operation = 0;
> + result->op = 0;
>
> sensorCtrls_ = entityControls.at(0);
> ispCtrls_ = entityControls.at(1);
>
> if (!validateSensorControls()) {
> LOG(IPARPI, Error) << "Sensor control validation failed.";
> - result->operation = RPi::IPA_CONFIG_FAILED;
> + result->op = ipa::rpi::ConfigFailed;
> return;
> }
>
> if (!validateIspControls()) {
> LOG(IPARPI, Error) << "ISP control validation failed.";
> - result->operation = RPi::IPA_CONFIG_FAILED;
> + result->op = ipa::rpi::ConfigFailed;
> return;
> }
>
> @@ -317,7 +322,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> if (!helper_) {
> LOG(IPARPI, Error) << "Could not create camera helper for "
> << cameraName;
> - result->operation = RPi::IPA_CONFIG_FAILED;
> + result->op = ipa::rpi::ConfigFailed;
> return;
> }
>
> @@ -329,34 +334,29 @@ 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::ConfigStaggeredWrite;
> + result->sensorConfig.gainDelay = gainDelay;
> + result->sensorConfig.exposureDelay = exposureDelay;
> + result->sensorConfig.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);
>
> /* Store the lens shading table pointer and handle if available. */
> - if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
> + if (ipaConfig.op & ipa::rpi::ConfigLsTable) {
> /* Remove any previous table, if there was one. */
> if (lsTable_) {
> - munmap(lsTable_, RPi::MaxLsGridSize);
> + munmap(lsTable_, ipa::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);
Isn't ipaConfig.lsTableHandle already a FileDescriptor ?
> if (lsTableHandle_.isValid()) {
> - lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> + lsTable_ = mmap(nullptr, ipa::rpi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> MAP_SHARED, lsTableHandle_.fd(), 0);
>
> if (lsTable_ == MAP_FAILED) {
> @@ -382,8 +382,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> agcStatus.analogue_gain = DefaultAnalogueGain;
> applyAGC(&agcStatus, ctrls);
>
> - result->controls.emplace_back(ctrls);
> - result->operation |= RPi::IPA_CONFIG_SENSOR;
> + result->op |= ipa::rpi::ConfigSensor;
> + result->controls = std::move(ctrls);
> }
>
> lastMode_ = mode_;
> @@ -408,56 +408,38 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
> }
> }
>
> -void IPARPi::processEvent(const IPAOperationData &event)
> +void IPARPi::signalStatReady(const uint32_t bufferId)
For arithemtic arguments we could drop the const in the template. If
it's too difficult, it doesn't matter much.
> {
> - 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 & ipa::rpi::MaskID, 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::ISPConfig &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);
You could write
prepareISP(data.embeddedbufferId);
and drop the local variable.
> + frameCount_++;
> +
> + /* Ready to push the input buffer into the ISP. */
> + runIsp.emit(bayerbufferId & ipa::rpi::MaskID);
Same here.
> }
>
> void IPARPi::reportMetadata()
> @@ -815,10 +797,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 & ipa::rpi::MaskID);
> }
>
> void IPARPi::prepareISP(unsigned int bufferId)
> @@ -879,12 +858,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);
> }
> }
>
> @@ -941,10 +916,7 @@ void IPARPi::processStats(unsigned int bufferId)
> ControlList ctrls(sensorCtrls_);
> applyAGC(&agcStatus, ctrls);
>
> - IPAOperationData op;
> - op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;
> - op.controls.emplace_back(ctrls);
> - queueFrameAction.emit(0, op);
> + setStaggered.emit(ctrls);
> }
> }
>
> @@ -1114,13 +1086,14 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> .grid_width = w,
> .grid_stride = w,
> .grid_height = h,
> - .dmabuf = lsTableHandle_.fd(),
> + /* .dmabuf will be filled in by pipeline handler. */
> + .dmabuf = 0,
> .ref_transform = 0,
> .corner_sampled = 1,
> .gain_format = GAIN_FORMAT_U4P10
> };
>
> - if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
> + if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > ipa::rpi::MaxLsGridSize) {
> LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
> return;
> }
> @@ -1191,9 +1164,9 @@ const struct IPAModuleInfo ipaModuleInfo = {
> "raspberrypi",
> };
>
> -struct ipa_context *ipaCreate()
> +IPAInterface *ipaCreate()
> {
> - return new IPAInterfaceWrapper(std::make_unique<IPARPi>());
Is there a corresponding header that could be dropped ?
> + return new IPARPi();
> }
>
> } /* extern "C" */
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 7a5f5881..4152ab69 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -17,11 +17,15 @@
> #include <libcamera/control_ids.h>
> #include <libcamera/file_descriptor.h>
> #include <libcamera/formats.h>
> +#include <libcamera/ipa/core_ipa_interface.h>
This is included by libcamera/ipa/raspberrypi_ipa_interface.h, do we
need to include it manually ?
> #include <libcamera/ipa/raspberrypi.h>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> +#include <libcamera/ipa/raspberrypi_ipa_proxy.h>
> #include <libcamera/logging.h>
> #include <libcamera/property_ids.h>
> #include <libcamera/request.h>
>
> +#include <linux/bcm2835-isp.h>
> #include <linux/videodev2.h>
>
> #include "libcamera/internal/bayer_format.h"
> @@ -146,7 +150,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);
> @@ -159,6 +167,8 @@ public:
> void handleState();
> void applyScalerCrop(const ControlList &controls);
>
> + std::unique_ptr<ipa::rpi::IPAProxyRPi> ipa_;
> +
> std::unique_ptr<CameraSensor> sensor_;
> /* Array of Unicam and ISP device streams and associated buffers/streams. */
> RPi::Device<Unicam, 2> unicam_;
> @@ -733,7 +743,7 @@ int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stre
> return ret;
> }
>
> -int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> +int PipelineHandlerRPi::start(Camera *camera, ControlList *controls)
> {
> RPiCameraData *data = cameraData(camera);
> int ret;
> @@ -751,13 +761,13 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
> data->applyScalerCrop(*controls);
>
> /* Start the IPA. */
> - IPAOperationData ipaData = {};
> - IPAOperationData result = {};
> + ipa::rpi::StartControls ipaData = {};
> + ipa::rpi::StartControls result = {};
No need for = {}, there's a default constructor.
> if (controls) {
> - ipaData.operation = RPi::IPA_CONFIG_STARTUP;
> - ipaData.controls.emplace_back(*controls);
> + ipaData.hasControls = true;
> + ipaData.controls = std::move(*controls);
Maybe we could drop the hasControls field and rely on
ipaData.controls.empty() ?
> }
> - ret = data->ipa_->start(ipaData, &result);
> + data->ipa_->start(ipaData, &result, &ret);
> if (ret) {
> LOG(RPI, Error)
> << "Failed to start IPA for " << camera->id();
> @@ -767,15 +777,15 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>
> /* Apply any gain/exposure settings that the IPA may have passed back. */
> ASSERT(data->staggeredCtrl_);
> - if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> - const ControlList &ctrls = result.controls[0];
> + if (result.hasControls) {
> + const ControlList &ctrls = result.controls;
> if (!data->staggeredCtrl_.set(ctrls))
> LOG(RPI, Error) << "V4L2 staggered set failed";
> }
>
> - if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {
> + if (result.dropFrameCount > 0) {
I think you can drop the condition, RPi::IPA_CONFIG_DROP_FRAMES was set
unconditionally.
> /* Configure the number of dropped frames required on startup. */
> - data->dropFrameCount_ = result.data[0];
> + data->dropFrameCount_ = result.dropFrameCount;
> }
>
> /* We need to set the dropFrameCount_ before queueing buffers. */
> @@ -1102,8 +1112,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> * Pass the stats and embedded data buffers to the IPA. No other
> * buffers need to be passed.
> */
> - mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPi::BufferMask::STATS);
> - mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPi::BufferMask::EMBEDDED_DATA);
> + mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::rpi::MaskStats);
> + mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::rpi::MaskEmbeddedData);
>
> return 0;
> }
> @@ -1120,8 +1130,8 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
> * handler and the IPA.
> */
> for (auto const &it : buffers) {
> - ipaBuffers.push_back({ .id = mask | it.first,
> - .planes = it.second->planes() });
> + ipaBuffers.push_back(IPABuffer(mask | it.first,
> + it.second->planes()));
> data->ipaBuffers_.insert(mask | it.first);
> }
>
> @@ -1152,15 +1162,18 @@ 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")
> - };
> + IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
>
> return ipa_->init(settings);
> }
> @@ -1172,8 +1185,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;
It's not nice to have to make a copy of the ControlInfoMap just to pass
this to the serializer :-5 Could you add a \todo in an appropriate place
(I suppose int he patch to the IPC infrastructure that introduces this
need, not in the RPi pipeline handler) ?
> + ipa::rpi::ConfigInput ipaConfig;
>
> /* Get the device format to pass to the IPA. */
> V4L2DeviceFormat sensorFormat;
> @@ -1182,10 +1195,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> unsigned int i = 0;
> for (auto const &stream : isp_) {
> if (stream.isExternal()) {
> - streamConfig[i++] = {
> - .pixelFormat = stream.configuration().pixelFormat,
> - .size = stream.configuration().size
> - };
> + streamConfig[i++] = IPAStream(
> + stream.configuration().pixelFormat,
> + stream.configuration().size);
> }
> }
>
> @@ -1193,17 +1205,17 @@ 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()) {
> - lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
> + lsTable_ = dmaHeap_.alloc("ls_grid", ipa::rpi::MaxLsGridSize);
> if (!lsTable_.isValid())
> return -ENOMEM;
>
> /* Allow the IPA to mmap the LS table via the file descriptor. */
It just occurred to me that this seems to duplicate the mapBuffers()
infrastructure. Could you add a
/*
* \todo Investigate if mapping the lens shading table buffer
* could be handled with mapBuffers().
*/
> - ipaConfig.operation = RPi::IPA_CONFIG_LS_TABLE;
> - ipaConfig.data.push_back(static_cast<unsigned int>(lsTable_.fd()));
> + ipaConfig.op |= ipa::rpi::ConfigLsTable;
> + ipaConfig.lsTableHandle = lsTable_;
> }
>
> /* We store the CameraSensorInfo for digital zoom calculations. */
> @@ -1214,32 +1226,33 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> }
>
> /* Ready the IPA - it must know about the sensor resolution. */
> - IPAOperationData result = {};
> + ipa::rpi::ConfigOutput result = {};
No need for = {} here either. There could be other places below.
>
> ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> &result);
>
> - if (result.operation & RPi::IPA_CONFIG_FAILED) {
> + if (result.op & ipa::rpi::ConfigFailed) {
> LOG(RPI, Error) << "IPA configuration failed!";
> return -EPIPE;
> }
>
> - unsigned int resultIdx = 0;
> - if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> + if (result.op & ipa::rpi::ConfigStaggeredWrite) {
> /*
> * 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.sensorConfig.gainDelay },
> + { V4L2_CID_EXPOSURE,
> + result.sensorConfig.exposureDelay } });
> + sensorMetadata_ = result.sensorConfig.sensorMetadata;
> }
> }
>
> - if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> - const ControlList &ctrls = result.controls[0];
> + if (result.op & ipa::rpi::ConfigSensor) {
Maybe ipa::rpi::ConfigSensor could be dropped, replaced with a
!result.controls.empty() check ?
> + const ControlList &ctrls = result.controls;
> if (!staggeredCtrl_.set(ctrls))
> LOG(RPI, Error) << "V4L2 staggered set failed";
> }
> @@ -1260,90 +1273,87 @@ 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)
> {
> + if (state_ == State::Stopped)
> + handleState();
> +
> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> +
> + 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);
That's interesting, controls being a const reference, I wouldn't have
expected a move to be possible.
> +
> /*
> - * The following actions can 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_V4L2_SET_STAGGERED: {
> - const ControlList &controls = action.controls[0];
> - if (!staggeredCtrl_.set(controls))
> - LOG(RPI, Error) << "V4L2 staggered set failed";
> - goto done;
> + if (updateScalerCrop_) {
> + updateScalerCrop_ = false;
> + scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> + sensorInfo_.outputSize);
> + scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
> }
> + request->metadata().set(controls::ScalerCrop, scalerCrop_);
>
> - case RPi::IPA_ACTION_V4L2_SET_ISP: {
> - ControlList controls = action.controls[0];
> - isp_[Isp::Input].dev()->setControls(&controls);
> - goto done;
> - }
> - }
> + state_ = State::IpaComplete;
>
> - if (state_ == State::Stopped)
> - goto done;
> + handleState();
> +}
>
> - /*
> - * The following actions must not be handled when the pipeline handler
> - * is in a stopped state.
> - */
> - switch (action.operation) {
> - case RPi::IPA_ACTION_STATS_METADATA_COMPLETE: {
> - unsigned int bufferId = action.data[0];
> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> +void RPiCameraData::runIsp(uint32_t bufferId)
> +{
> + if (state_ == State::Stopped)
> + handleState();
>
> - handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>
> - /* Fill the Request metadata buffer with what the IPA has provided */
> - Request *request = requestQueue_.front();
> - request->metadata() = std::move(action.controls[0]);
> + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> + << ", timestamp: " << buffer->metadata().timestamp;
>
> - /*
> - * 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_);
> + isp_[Isp::Input].queueBuffer(buffer);
> + ispOutputCount_ = 0;
> + handleState();
> +}
>
> - state_ = State::IpaComplete;
> - break;
> - }
> +void RPiCameraData::embeddedComplete(uint32_t bufferId)
> +{
> + if (state_ == State::Stopped)
> + handleState();
>
> - 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;
> - }
> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> + handleState();
> +}
>
> - case RPi::IPA_ACTION_RUN_ISP: {
> - unsigned int bufferId = action.data[0];
> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> +void RPiCameraData::setIsp(const ControlList &controls)
> +{
> + ControlList ctrls = controls;
>
> - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> - << ", timestamp: " << buffer->metadata().timestamp;
> + Span<const uint8_t> s =
> + ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> + const bcm2835_isp_lens_shading *lsp =
> + reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
> + bcm2835_isp_lens_shading ls = *lsp;
bcm2835_isp_lens_shading ls =
*reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
The double-copy is a bit of a shame, maybe we should extend the
ControlList and/or ControlValue API to make it possible to modify
controls in place. That's out of scope for this series though.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + ls.dmabuf = lsTable_.fd();
>
> - isp_[Isp::Input].queueBuffer(buffer);
> - ispOutputCount_ = 0;
> - break;
> - }
> + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
> + sizeof(ls) });
> + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
>
> - default:
> - LOG(RPI, Error) << "Unknown action " << action.operation;
> - break;
> - }
> + 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();
> }
>
> @@ -1445,10 +1455,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(ipa::rpi::MaskStats | static_cast<unsigned int>(index));
> } else {
> /* Any other ISP output can be handed back to the application now. */
> handleStreamBuffer(buffer, stream);
> @@ -1552,7 +1559,7 @@ void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *strea
> {
> unsigned int id = stream->getBufferId(buffer);
>
> - if (!(id & RPi::BufferMask::EXTERNAL_BUFFER))
> + if (!(id & ipa::rpi::MaskExternalBuffer))
> return;
>
> /* Stop the Stream object from tracking the buffer. */
> @@ -1652,7 +1659,6 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)
> 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() ||
> @@ -1673,9 +1679,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());
>
> /* Set our state to say the pipeline is active. */
> state_ = State::Busy;
> @@ -1683,14 +1687,14 @@ void RPiCameraData::tryRunPipeline()
> unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);
> unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>
> - LOG(RPI, Debug) << "Signalling RPi::IPA_EVENT_SIGNAL_ISP_PREPARE:"
> + LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> << " 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::ISPConfig ispPrepare;
> + ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> + ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> + ipa_->signalIspPrepare(ispPrepare);
> }
>
> bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 3a5dadab..496dd36f 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -6,6 +6,8 @@
> */
> #include "rpi_stream.h"
>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> +
> #include "libcamera/internal/log.h"
>
> namespace libcamera {
> @@ -70,7 +72,7 @@ int Stream::getBufferId(FrameBuffer *buffer) const
>
> void Stream::setExternalBuffer(FrameBuffer *buffer)
> {
> - bufferMap_.emplace(RPi::BufferMask::EXTERNAL_BUFFER | id_.get(), buffer);
> + bufferMap_.emplace(ipa::rpi::MaskExternalBuffer | id_.get(), buffer);
> }
>
> void Stream::removeExternalBuffer(FrameBuffer *buffer)
> @@ -78,7 +80,7 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
> int id = getBufferId(buffer);
>
> /* Ensure we have this buffer in the stream, and it is marked external. */
> - ASSERT(id != -1 && (id & RPi::BufferMask::EXTERNAL_BUFFER));
> + ASSERT(id != -1 && (id & ipa::rpi::MaskExternalBuffer));
> bufferMap_.erase(id);
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index 0b502f64..701110d0 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -13,6 +13,7 @@
> #include <vector>
>
> #include <libcamera/ipa/raspberrypi.h>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/stream.h>
>
> #include "libcamera/internal/v4l2_videodevice.h"
> @@ -31,13 +32,13 @@ class Stream : public libcamera::Stream
> {
> public:
> Stream()
> - : id_(RPi::BufferMask::ID)
> + : id_(ipa::rpi::MaskID)
> {
> }
>
> Stream(const char *name, MediaEntity *dev, bool importOnly = false)
> : external_(false), importOnly_(importOnly), name_(name),
> - dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPi::BufferMask::ID)
> + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(ipa::rpi::MaskID)
> {
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list