[v2,05/10] mali-c55: Plumb the IPA module in

20636 diff mbox series

From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Plumb the Pipeline-IPA loop in.

Load the IPA module at camera creation time and create the loop between
the pipeline and the IPA.

When a new Request is queued the IPA is asked to prepare the parameters
buffer, once ready it notifies the pipeline which queues the parameters
to the ISP along with a buffer for statistics and frames,

Once statistics are ready they get passed to the IPA which upates its
settings for the next frame.

Acked-by: Nayden Kanchev  <nayden.kanchev@arm.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
Changes in v2:

	- None

 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 403 +++++++++++++++++--
 1 file changed, 360 insertions(+), 43 deletions(-)

Quoting Daniel Scally (2024-07-09 15:49:45)
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Plumb the Pipeline-IPA loop in.
> 
> Load the IPA module at camera creation time and create the loop between
> the pipeline and the IPA.
> 
> When a new Request is queued the IPA is asked to prepare the parameters
> buffer, once ready it notifies the pipeline which queues the parameters
> to the ISP along with a buffer for statistics and frames,
> 
> Once statistics are ready they get passed to the IPA which upates its
> settings for the next frame.
> 
> Acked-by: Nayden Kanchev  <nayden.kanchev@arm.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> Changes in v2:
> 
>         - None
> 
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 403 +++++++++++++++++--
>  1 file changed, 360 insertions(+), 43 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 1e5674fc..dd523d8d 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -23,13 +23,19 @@
>  #include <libcamera/geometry.h>
>  #include <libcamera/stream.h>
>  
> +#include <libcamera/ipa/mali-c55_ipa_interface.h>
> +#include <libcamera/ipa/mali-c55_ipa_proxy.h>
> +
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/request.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
> @@ -70,6 +76,16 @@ constexpr Size kMaliC55MinSize = { 128, 128 };
>  constexpr Size kMaliC55MaxSize = { 8192, 8192 };
>  constexpr unsigned int kMaliC55ISPInternalFormat = MEDIA_BUS_FMT_RGB121212_1X36;
>  
> +struct MaliC55FrameInfo {
> +       Request *request;
> +
> +       FrameBuffer *paramBuffer;
> +       FrameBuffer *statBuffer;
> +
> +       bool paramsDone;
> +       bool statsDone;
> +};
> +
>  class MaliC55CameraData : public Camera::Private
>  {
>  public:
> @@ -79,6 +95,7 @@ public:
>         }
>  
>         int init();
> +       int loadIPA();
>  
>         /* Deflect these functionalities to either TPG or CameraSensor. */
>         const std::vector<Size> sizes(unsigned int mbusCode) const;
> @@ -87,7 +104,7 @@ public:
>         int pixfmtToMbusCode(const PixelFormat &pixFmt) const;
>         const PixelFormat &bestRawFormat() const;
>  
> -       void updateControls();
> +       void updateControls(const ControlInfoMap &ipaControls);
>  
>         PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;
>         Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;
> @@ -100,8 +117,15 @@ public:
>         Stream frStream_;
>         Stream dsStream_;
>  
> +       std::unique_ptr<ipa::mali_c55::IPAProxyMaliC55> ipa_;
> +       std::vector<IPABuffer> ipaStatBuffers_;
> +       std::vector<IPABuffer> ipaParamBuffers_;
> +
> +       std::unique_ptr<DelayedControls> delayedCtrls_;
> +
>  private:
>         void initTPGData();
> +       void setSensorControls(const ControlList &sensorControls);
>  
>         std::string id_;
>         std::vector<unsigned int> tpgCodes_;
> @@ -166,6 +190,11 @@ void MaliC55CameraData::initTPGData()
>         tpgResolution_ = tpgSizes_.back();
>  }
>  
> +void MaliC55CameraData::setSensorControls(const ControlList &sensorControls)
> +{
> +       delayedCtrls_->push(sensorControls);
> +}
> +
>  const std::vector<Size> MaliC55CameraData::sizes(unsigned int mbusCode) const
>  {
>         if (sensor_)
> @@ -268,7 +297,7 @@ const PixelFormat &MaliC55CameraData::bestRawFormat() const
>         return invalidPixFmt;
>  }
>  
> -void MaliC55CameraData::updateControls()
> +void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls)
>  {
>         if (!sensor_)
>                 return;
> @@ -286,6 +315,9 @@ void MaliC55CameraData::updateControls()
>                 ControlInfo(ispMinCrop, sensorInfo.analogCrop,
>                             sensorInfo.analogCrop);
>  
> +       for (auto const &c : ipaControls)
> +               controls.emplace(c.first, c.second);
> +
>         controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);
>  }
>  
> @@ -339,6 +371,46 @@ Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &si
>         return bestSize;
>  }
>  
> +int MaliC55CameraData::loadIPA()
> +{
> +       int ret;
> +
> +       /* Do not initialize IPA for TPG. */
> +       if (!sensor_)
> +               return 0;
> +
> +       ipa_ = IPAManager::createIPA<ipa::mali_c55::IPAProxyMaliC55>(pipe(), 1, 1);
> +       if (!ipa_)
> +               return -ENOENT;
> +
> +       ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);
> +
> +       std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml");
> +       if (ipaTuningFile.empty())
> +               ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");

I think this (ipa_->configurationFile) has since been modified to take
the fallback file path. So this probably needs to be updated with a
rebase to 

	ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");


> +
> +       /* We need to inform the IPA of the sensor configuration */
> +       ipa::mali_c55::IPAConfigInfo ipaConfig{};
> +
> +       ret = sensor_->sensorInfo(&ipaConfig.sensorInfo);
> +       if (ret)
> +               return ret;
> +
> +       ipaConfig.sensorControls = sensor_->controls();
> +
> +       ControlInfoMap ipaControls;
> +       ret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig,
> +                        &ipaControls);
> +       if (ret) {
> +               LOG(MaliC55, Error) << "Failed to initialise the Mali-C55 IPA";
> +               return ret;
> +       }
> +
> +       updateControls(ipaControls);
> +
> +       return 0;
> +}
> +
>  class MaliC55CameraConfiguration : public CameraConfiguration
>  {
>  public:
> @@ -348,6 +420,7 @@ public:
>         }
>  
>         Status validate() override;
> +       const Transform &combinedTransform() { return combinedTransform_; }
>  
>         V4L2SubdeviceFormat sensorFormat_;
>  
> @@ -355,6 +428,7 @@ private:
>         static constexpr unsigned int kMaxStreams = 2;
>  
>         const MaliC55CameraData *data_;
> +       Transform combinedTransform_;
>  };
>  
>  CameraConfiguration::Status MaliC55CameraConfiguration::validate()
> @@ -364,6 +438,19 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
>         if (config_.empty())
>                 return Invalid;
>  
> +       /*
> +        * The TPG doesn't support flips, so we only need to calculate a
> +        * transform if we have a sensor.
> +        */
> +       if (data_->sensor_) {
> +               Orientation requestedOrientation = orientation;
> +               combinedTransform_ = data_->sensor_->computeTransform(&orientation);
> +               if (orientation != requestedOrientation)
> +                       status = Adjusted;
> +       } else {
> +               combinedTransform_ = Transform::Rot0;
> +       }
> +
>         /* Only 2 streams available. */
>         if (config_.size() > kMaxStreams) {
>                 config_.resize(kMaxStreams);
> @@ -521,7 +608,10 @@ public:
>         int queueRequestDevice(Camera *camera, Request *request) override;
>  
>         void bufferReady(FrameBuffer *buffer);
> +       void paramsBufferReady(FrameBuffer *buffer);
>         void statsBufferReady(FrameBuffer *buffer);
> +       void paramsComputed(unsigned int requestId);
> +       void statsProcessed(unsigned int requestId, const ControlList &metadata);
>  
>         bool match(DeviceEnumerator *enumerator) override;
>  
> @@ -565,6 +655,10 @@ private:
>                         pipe.stream = nullptr;
>         }
>  
> +       MaliC55FrameInfo *findFrameInfo(FrameBuffer *buffer);
> +       MaliC55FrameInfo *findFrameInfo(Request *request);
> +       void tryComplete(MaliC55FrameInfo *info);
> +
>         int configureRawStream(MaliC55CameraData *data,
>                                const StreamConfiguration &config,
>                                V4L2SubdeviceFormat &subdevFormat);
> @@ -574,7 +668,7 @@ private:
>  
>         void applyScalerCrop(Camera *camera, const ControlList &controls);
>  
> -       void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
> +       bool registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
>                                 const std::string &name);
>         bool registerTPGCamera(MediaLink *link);
>         bool registerSensorCamera(MediaLink *link);
> @@ -590,6 +684,8 @@ private:
>         std::vector<std::unique_ptr<FrameBuffer>> paramsBuffers_;
>         std::queue<FrameBuffer *> availableParamsBuffers_;
>  
> +       std::map<unsigned int, MaliC55FrameInfo> frameInfoMap_;
> +
>         std::array<MaliC55Pipe, MaliC55NumPipes> pipes_;
>  
>         bool dsFitted_;
> @@ -838,6 +934,13 @@ int PipelineHandlerMaliC55::configure(Camera *camera,
>         if (ret)
>                 return ret;
>  
> +       if (data->sensor_) {
> +               ret = data->sensor_->setFormat(&subdevFormat,
> +                                              maliConfig->combinedTransform());
> +               if (ret)
> +                       return ret;
> +       }
> +
>         if (data->csi_) {
>                 ret = data->csi_->setFormat(0, &subdevFormat);
>                 if (ret)
> @@ -940,7 +1043,34 @@ int PipelineHandlerMaliC55::configure(Camera *camera,
>                 return ret;
>         }
>  
> -       data->updateControls();
> +       if (!data->ipa_)
> +               return 0;
> +
> +       /* We need to inform the IPA of the sensor configuration */
> +       ipa::mali_c55::IPAConfigInfo ipaConfig{};
> +
> +       ret = data->sensor_->sensorInfo(&ipaConfig.sensorInfo);
> +       if (ret)
> +               return ret;
> +
> +       ipaConfig.sensorControls = data->sensor_->controls();
> +
> +       /*
> +        * And we also need to tell the IPA the bayerOrder of the data (as
> +        * affected by any flips that we've configured)
> +        */
> +       const Transform &combinedTransform = maliConfig->combinedTransform();
> +       BayerFormat::Order bayerOrder = data->sensor_->bayerOrder(combinedTransform);
> +
> +       ControlInfoMap ipaControls;
> +       ret = data->ipa_->configure(ipaConfig, utils::to_underlying(bayerOrder),
> +                                   &ipaControls);
> +       if (ret) {
> +               LOG(MaliC55, Error) << "Failed to configure IPA";
> +               return ret;
> +       }
> +
> +       data->updateControls(ipaControls);
>  
>         return 0;
>  }
> @@ -954,8 +1084,10 @@ int PipelineHandlerMaliC55::exportFrameBuffers(Camera *camera, Stream *stream,
>         return pipe->cap->exportBuffers(count, buffers);
>  }
>  
> -void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)
> +void PipelineHandlerMaliC55::freeBuffers(Camera *camera)
>  {
> +       MaliC55CameraData *data = cameraData(camera);
> +
>         while (!availableStatsBuffers_.empty())
>                 availableStatsBuffers_.pop();
>         while (!availableParamsBuffers_.empty())
> @@ -964,11 +1096,18 @@ void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)
>         statsBuffers_.clear();
>         paramsBuffers_.clear();
>  
> +       if (data->ipa_) {
> +               data->ipa_->unmapBuffers(data->ipaStatBuffers_);
> +               data->ipa_->unmapBuffers(data->ipaParamBuffers_);
> +       }
> +       data->ipaStatBuffers_.clear();
> +       data->ipaParamBuffers_.clear();
> +
>         if (stats_->releaseBuffers())
>                 LOG(MaliC55, Error) << "Failed to release stats buffers";
>  
>         if (params_->releaseBuffers())
> -               LOG(MaliC55, Error) << "Failed to release stats buffers";
> +               LOG(MaliC55, Error) << "Failed to release params buffers";

Minor fix could be mentioned in the commit message, no specific need to
pull to it's own patch, but this is independent from the main $COMMIT.

>  
>         return;
>  }
> @@ -976,6 +1115,7 @@ void PipelineHandlerMaliC55::freeBuffers([[maybe_unused]] Camera *camera)
>  int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
>  {
>         MaliC55CameraData *data = cameraData(camera);
> +       unsigned int ipaBufferId = 1;
>         unsigned int bufferCount;
>         int ret;
>  
> @@ -988,27 +1128,51 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
>         if (ret < 0)
>                 return ret;
>  
> -       for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> +       for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
> +               buffer->setCookie(ipaBufferId++);
> +               data->ipaStatBuffers_.emplace_back(buffer->cookie(),
> +                                                  buffer->planes());
>                 availableStatsBuffers_.push(buffer.get());
> +       }
>  
>         ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
>         if (ret < 0)
>                 return ret;
>  
> -       for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> +       for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
> +               buffer->setCookie(ipaBufferId++);
> +               data->ipaParamBuffers_.emplace_back(buffer->cookie(),
> +                                                   buffer->planes());
>                 availableParamsBuffers_.push(buffer.get());
> +       }
> +
> +       if (data->ipa_) {
> +               data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
> +               data->ipa_->mapBuffers(data->ipaParamBuffers_, false);
> +       }
>  
>         return 0;
>  }
>  
>  int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>  {
> +       MaliC55CameraData *data = cameraData(camera);
>         int ret;
>  
>         ret = allocateBuffers(camera);
>         if (ret)
>                 return ret;
>  
> +       if (data->ipa_) {
> +               ret = data->ipa_->start();
> +               if (ret) {
> +                       LOG(MaliC55, Error)
> +                               << "Failed to start IPA" << camera->id();
> +                       freeBuffers(camera);
> +                       return ret;
> +               }
> +       }
> +
>         for (MaliC55Pipe &pipe : pipes_) {
>                 if (!pipe.stream)
>                         continue;
> @@ -1018,6 +1182,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control
>                 ret = pipe.cap->importBuffers(stream->configuration().bufferCount);
>                 if (ret) {
>                         LOG(MaliC55, Error) << "Failed to import buffers";
> +                       if (data->ipa_)
> +                               data->ipa_->stop();
>                         freeBuffers(camera);
>                         return ret;
>                 }
> @@ -1025,6 +1191,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control
>                 ret = pipe.cap->streamOn();
>                 if (ret) {
>                         LOG(MaliC55, Error) << "Failed to start stream";
> +                       if (data->ipa_)
> +                               data->ipa_->stop();
>                         freeBuffers(camera);
>                         return ret;
>                 }
> @@ -1034,6 +1202,9 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control
>         if (ret) {
>                 LOG(MaliC55, Error) << "Failed to start stats stream";
>  
> +               if (data->ipa_)
> +                       data->ipa_->stop();
> +
>                 for (MaliC55Pipe &pipe : pipes_) {
>                         if (pipe.stream)
>                                 pipe.cap->streamOff();
> @@ -1048,6 +1219,8 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control
>                 LOG(MaliC55, Error) << "Failed to start params stream";
>  
>                 stats_->streamOff();
> +               if (data->ipa_)
> +                       data->ipa_->stop();
>  


I think I've said this on later patches, because I'm going backwards in
time ... but the cleanup helpers might help a lot here. But maybe that's
something to do 'on top' anyway.


>                 for (MaliC55Pipe &pipe : pipes_) {
>                         if (pipe.stream)
> @@ -1058,11 +1231,19 @@ int PipelineHandlerMaliC55::start(Camera *camera, [[maybe_unused]] const Control
>                 return ret;
>         }
>  
> +       ret = isp_->setFrameStartEnabled(true);
> +       if (ret)
> +               LOG(MaliC55, Error) << "Failed to enable frame start events";
> +
>         return 0;
>  }
>  
> -void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)
> +void PipelineHandlerMaliC55::stopDevice(Camera *camera)
>  {
> +       MaliC55CameraData *data = cameraData(camera);
> +
> +       isp_->setFrameStartEnabled(false);
> +
>         for (MaliC55Pipe &pipe : pipes_) {
>                 if (!pipe.stream)
>                         continue;
> @@ -1073,6 +1254,8 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera)
>  
>         stats_->streamOff();
>         params_->streamOff();
> +       if (data->ipa_)
> +               data->ipa_->stop();
>         freeBuffers(camera);
>  }
>  
> @@ -1174,64 +1357,179 @@ void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera,
>  
>  int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request)
>  {
> -       FrameBuffer *statsBuffer;
> -       int ret;
> +       MaliC55CameraData *data = cameraData(camera);
> +
> +       /* Do not run the IPA if the TPG is in use. */
> +       if (!data->ipa_) {
> +               MaliC55FrameInfo frameInfo;
> +               frameInfo.request = request;
> +               frameInfo.statBuffer = nullptr;
> +               frameInfo.paramBuffer = nullptr;
> +               frameInfo.paramsDone = true;
> +               frameInfo.statsDone = true;
> +
> +               frameInfoMap_[request->sequence()] = frameInfo;
> +
> +               for (auto &[stream, buffer] : request->buffers()) {
> +                       MaliC55Pipe *pipe = pipeFromStream(data, stream);
> +
> +                       pipe->cap->queueBuffer(buffer);
> +               }
> +
> +               return 0;
> +       }

All of the 'if (!data->ipa_)' conditions make me wonder if we shouldn't
somehow abstract out and make a distinct 'no op' IPA for TPG which does
things for TPG only.

But the amount of work I could envisage in doing so, or trying to make a
generic no-op IPA with the same interface ... in a way that actually
saves code would probably end up adding more complexity than "if !ipa"
handling...


>  
>         if (availableStatsBuffers_.empty()) {
>                 LOG(MaliC55, Error) << "Stats buffer underrun";
>                 return -ENOENT;
>         }
>  
> -       statsBuffer = availableStatsBuffers_.front();
> +       if (availableParamsBuffers_.empty()) {
> +               LOG(MaliC55, Error) << "Params buffer underrun";
> +               return -ENOENT;
> +       }
> +
> +       MaliC55FrameInfo frameInfo;
> +       frameInfo.request = request;
> +
> +       frameInfo.statBuffer = availableStatsBuffers_.front();
>         availableStatsBuffers_.pop();
> +       frameInfo.paramBuffer = availableParamsBuffers_.front();
> +       availableParamsBuffers_.pop();
>  
> -       /*
> -        * We need to associate the Request to this buffer even though it's a
> -        * purely internal one because we will need to use request->sequence()
> -        * later.
> -        */
> -       statsBuffer->_d()->setRequest(request);
> +       frameInfo.paramsDone = false;
> +       frameInfo.statsDone = false;
>  
> -       for (auto &[stream, buffer] : request->buffers()) {
> -               MaliC55Pipe *pipe = pipeFromStream(cameraData(camera), stream);
> +       frameInfoMap_[request->sequence()] = frameInfo;
>  
> -               ret = pipe->cap->queueBuffer(buffer);
> -               if (ret)
> -                       return ret;
> +       data->ipa_->queueRequest(request->sequence(), request->controls());
> +       data->ipa_->fillParams(request->sequence(),
> +                              frameInfo.paramBuffer->cookie());
> +
> +       return 0;
> +}
> +
> +MaliC55FrameInfo *PipelineHandlerMaliC55::findFrameInfo(Request *request)
> +{
> +       for (auto &[sequence, info] : frameInfoMap_) {
> +               if (info.request == request)
> +                       return &info;
>         }
>  
> -       /*
> -        * Some controls need to be applied immediately, as in example,
> -        * the ScalerCrop one.
> -        *
> -        * \todo Move it buffer queue time (likely after the IPA has filled in
> -        * the parameters buffer) once we have plumbed the IPA loop in.
> -        */
> -       applyScalerCrop(camera, request->controls());
> +       return nullptr;
> +}
>  
> -       ret = stats_->queueBuffer(statsBuffer);
> -       if (ret)
> -               return ret;
> +MaliC55FrameInfo *PipelineHandlerMaliC55::findFrameInfo(FrameBuffer *buffer)
> +{
> +       for (auto &[sequence, info] : frameInfoMap_) {
> +               if (info.paramBuffer == buffer ||
> +                   info.statBuffer == buffer)
> +                       return &info;
> +       }
>  
> -       return 0;
> +       return nullptr;
> +}
> +
> +void PipelineHandlerMaliC55::tryComplete(MaliC55FrameInfo *info)
> +{
> +       if (!info->paramsDone)
> +               return;
> +       if (!info->statsDone)
> +               return;
> +
> +       Request *request = info->request;
> +       if (request->hasPendingBuffers())
> +               return;
> +
> +       if (info->statBuffer)
> +               availableStatsBuffers_.push(info->statBuffer);
> +       if (info->paramBuffer)
> +               availableParamsBuffers_.push(info->paramBuffer);
> +
> +       frameInfoMap_.erase(request->sequence());
> +
> +       completeRequest(request);
>  }
>  
>  void PipelineHandlerMaliC55::bufferReady(FrameBuffer *buffer)
>  {
>         Request *request = buffer->request();
> +       MaliC55FrameInfo *info = findFrameInfo(request);
> +       ASSERT(info);
>  
>         if (completeBuffer(request, buffer))
> -               completeRequest(request);
> +               tryComplete(info);
> +}
> +
> +void PipelineHandlerMaliC55::paramsBufferReady(FrameBuffer *buffer)
> +{
> +       MaliC55FrameInfo *info = findFrameInfo(buffer);
> +       ASSERT(info);
> +
> +       info->paramsDone = true;
> +
> +       tryComplete(info);
>  }
>  
>  void PipelineHandlerMaliC55::statsBufferReady(FrameBuffer *buffer)
>  {
> -       availableStatsBuffers_.push(buffer);
> +       MaliC55FrameInfo *info = findFrameInfo(buffer);
> +       ASSERT(info);
> +
> +       Request *request = info->request;
> +       MaliC55CameraData *data = cameraData(request->_d()->camera());
> +
> +       ControlList sensorControls = data->delayedCtrls_->get(buffer->metadata().sequence);
> +
> +       data->ipa_->processStats(request->sequence(), buffer->cookie(),
> +                                sensorControls);
>  }
>  
> -void PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
> +void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
> +{
> +       MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];
> +       Request *request = frameInfo.request;
> +       MaliC55CameraData *data = cameraData(request->_d()->camera());
> +
> +       /*
> +        * Queue buffers for stats and params, then queue buffers to the capture
> +        * video devices.
> +        */
> +
> +       frameInfo.paramBuffer->_d()->metadata().planes()[0].bytesused =
> +               sizeof(struct mali_c55_params_buffer);
> +       params_->queueBuffer(frameInfo.paramBuffer);
> +       stats_->queueBuffer(frameInfo.statBuffer);
> +
> +       for (auto &[stream, buffer] : request->buffers()) {
> +               MaliC55Pipe *pipe = pipeFromStream(data, stream);
> +
> +               pipe->cap->queueBuffer(buffer);
> +       }
> +}
> +
> +void PipelineHandlerMaliC55::statsProcessed(unsigned int requestId,
> +                                           const ControlList &metadata)
> +{
> +       MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];
> +
> +       frameInfo.statsDone = true;
> +       frameInfo.request->metadata().merge(metadata);
> +
> +       tryComplete(&frameInfo);
> +}
> +
> +bool PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr<MaliC55CameraData> data,
>                                                 const std::string &name)
>  {
> +       if (data->loadIPA())
> +               return false;
> +
> +       if (data->ipa_) {
> +               data->ipa_->statsProcessed.connect(this, &PipelineHandlerMaliC55::statsProcessed);
> +               data->ipa_->paramsComputed.connect(this, &PipelineHandlerMaliC55::paramsComputed);
> +       }
> +
>         std::set<Stream *> streams{ &data->frStream_ };
>         if (dsFitted_)
>                 streams.insert(&data->dsStream_);
> @@ -1239,6 +1537,8 @@ void PipelineHandlerMaliC55::registerMaliCamera(std::unique_ptr<MaliC55CameraDat
>         std::shared_ptr<Camera> camera = Camera::create(std::move(data),
>                                                         name, streams);
>         registerCamera(std::move(camera));
> +
> +       return true;
>  }
>  
>  /*
> @@ -1264,9 +1564,7 @@ bool PipelineHandlerMaliC55::registerTPGCamera(MediaLink *link)
>         if (data->init())
>                 return false;
>  
> -       registerMaliCamera(std::move(data), name);
> -
> -       return true;
> +       return registerMaliCamera(std::move(data), name);
>  }
>  
>  /*
> @@ -1293,9 +1591,27 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)
>                         return false;
>  
>                 data->properties_ = data->sensor_->properties();
> -               data->updateControls();
>  
> -               registerMaliCamera(std::move(data), sensor->name());
> +               /*
> +                * \todo Read delay values from the sensor itself or from a
> +                * a sensor database. For now use generic values taken from
> +                * the Raspberry Pi and listed as 'generic values'.
> +                */
> +               std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +                       { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> +                       { V4L2_CID_EXPOSURE, { 2, false } },
> +               };

We *REALLY* need to fix this for all the libipa pipeline handlers.

So much so - that I would probably be tempted to say "Hey - lets not
merge more pipeline handlers that hardcode the wrong delays until we fix
the CameraSensorHelper to also report the delays per sensor.

It's "just" a case of adding a delay map in the CameraSensorHelper
instances right?

It shouldn't be hard... but no one is doing it.


If that were done then I'd give this patch my RB tag ;-)



> +
> +               data->delayedCtrls_ =
> +                       std::make_unique<DelayedControls>(data->sensor_->device(),
> +                                                         params);
> +               isp_->frameStart.connect(data->delayedCtrls_.get(),
> +                                        &DelayedControls::applyControls);
> +
> +               /* \todo: Init properties. */
> +
> +               if (!registerMaliCamera(std::move(data), sensor->name()))
> +                       return false;
>         }
>  
>         return true;
> @@ -1362,6 +1678,7 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
>         }
>  
>         stats_->bufferReady.connect(this, &PipelineHandlerMaliC55::statsBufferReady);
> +       params_->bufferReady.connect(this, &PipelineHandlerMaliC55::paramsBufferReady);
>  
>         ispSink = isp_->entity()->getPadByIndex(0);
>         if (!ispSink || ispSink->links().empty()) {
> -- 
> 2.34.1
>